Skip to content

Commit 6d8a7cc

Browse files
zackbradyszackbradys
authored
fixed command precedents for hauler store sync (#734)
1 parent bd70379 commit 6d8a7cc

10 files changed

Lines changed: 310 additions & 193 deletions

File tree

‎cmd/hauler/cli/store.go‎

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package cli
33
import (
44
"fmt"
55
"os"
6-
"strconv"
76

87
"github.com/spf13/cobra"
98
"helm.sh/helm/v4/pkg/action"
@@ -78,11 +77,18 @@ func addStoreSync(rso *flags.StoreRootOpts, ro *flags.CliRootOpts) *cobra.Comman
7877
if o.CaFile == "" {
7978
o.CaFile = os.Getenv(consts.CaFile)
8079
}
81-
if o.InsecureSkipTLSVerify == nil {
82-
if v := os.Getenv(consts.InsecureSkipTLSVerify); v != "" {
83-
b, _ := strconv.ParseBool(v)
84-
o.InsecureSkipTLSVerify = &b
85-
}
80+
81+
// record which precedence-carrying flags the user explicitly set, so
82+
// the resolvers can let an explicit CLI value win over per-item/annotation
83+
o.TlogChanged = cmd.Flags().Changed("use-tlog-verify")
84+
o.ExcludeExtrasChanged = cmd.Flags().Changed("exclude-extras")
85+
o.InsecureChanged = cmd.Flags().Changed("insecure-skip-tls-verify")
86+
o.StoreChanged = cmd.Flags().Changed("store")
87+
o.RetriesChanged = cmd.Flags().Changed("retries")
88+
89+
// env var only applies when the flag wasn't set, so an explicit --insecure-skip-tls-verify=false still wins
90+
if !o.InsecureChanged && os.Getenv(consts.InsecureSkipTLSVerify) == "true" {
91+
o.InsecureSkipTLSVerify = true
8692
}
8793

8894
// --dry-run requires --products
@@ -124,12 +130,6 @@ func addStoreSync(rso *flags.StoreRootOpts, ro *flags.CliRootOpts) *cobra.Comman
124130
}
125131
rso.BlobConcurrency = bc
126132

127-
// resolve *bool: nil unless the user explicitly passed the flag
128-
if cmd.Flags().Changed("insecure-skip-tls-verify") {
129-
v, _ := cmd.Flags().GetBool("insecure-skip-tls-verify")
130-
o.InsecureSkipTLSVerify = &v
131-
}
132-
133133
return nil
134134
},
135135
RunE: func(cmd *cobra.Command, args []string) error {
@@ -411,11 +411,9 @@ func addStoreAddImage(rso *flags.StoreRootOpts, ro *flags.CliRootOpts) *cobra.Co
411411
if o.CaFile == "" {
412412
o.CaFile = os.Getenv(consts.CaFile)
413413
}
414-
if o.InsecureSkipTLSVerify == nil {
415-
if v := os.Getenv(consts.InsecureSkipTLSVerify); v != "" {
416-
b, _ := strconv.ParseBool(v)
417-
o.InsecureSkipTLSVerify = &b
418-
}
414+
// env var only applies when the flag wasn't set, so an explicit --insecure-skip-tls-verify=false still wins
415+
if !cmd.Flags().Changed("insecure-skip-tls-verify") && os.Getenv(consts.InsecureSkipTLSVerify) == "true" {
416+
o.InsecureSkipTLSVerify = true
419417
}
420418
return nil
421419
},

‎cmd/hauler/cli/store/add.go‎

Lines changed: 21 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ func AddFileCmd(ctx context.Context, o *flags.AddFileOpts, s *store.Layout, refe
5959
cfg := v1.File{
6060
Path: reference,
6161
CaFile: o.CaFile,
62-
InsecureSkipTLSVerify: &o.InsecureSkipTLSVerify,
62+
InsecureSkipTLSVerify: o.InsecureSkipTLSVerify,
6363
}
6464
if len(o.Name) > 0 {
6565
cfg.Name = o.Name
@@ -83,7 +83,7 @@ func storeFile(ctx context.Context, s *store.Layout, fi v1.File, ro *flags.CliRo
8383

8484
copts := getter.ClientOptions{
8585
NameOverride: fi.Name,
86-
InsecureSkipTLSVerify: derefInsecure(fi.InsecureSkipTLSVerify),
86+
InsecureSkipTLSVerify: fi.InsecureSkipTLSVerify,
8787
CAFile: fi.CaFile,
8888
}
8989

@@ -248,15 +248,15 @@ func AddImageCmd(ctx context.Context, o *flags.AddImageOpts, s *store.Layout, re
248248
func addImageVerifyConfig(o *flags.AddImageOpts) cosign.Config {
249249
switch {
250250
case o.Key != "":
251-
return cosign.Config{Key: o.Key, Tlog: o.Tlog, InsecureSkipTLSVerify: derefInsecure(o.InsecureSkipTLSVerify), CaFile: o.CaFile}
251+
return cosign.Config{Key: o.Key, Tlog: o.Tlog, InsecureSkipTLSVerify: o.InsecureSkipTLSVerify, CaFile: o.CaFile}
252252
case o.CertIdentityRegexp != "" || o.CertIdentity != "":
253253
return cosign.Config{
254254
CertIdentity: o.CertIdentity,
255255
CertIdentityRegexp: o.CertIdentityRegexp,
256256
CertOidcIssuer: o.CertOidcIssuer,
257257
CertOidcIssuerRegexp: o.CertOidcIssuerRegexp,
258258
CertGithubWorkflowRepository: o.CertGithubWorkflowRepository,
259-
InsecureSkipTLSVerify: derefInsecure(o.InsecureSkipTLSVerify),
259+
InsecureSkipTLSVerify: o.InsecureSkipTLSVerify,
260260
CaFile: o.CaFile,
261261
}
262262
default:
@@ -480,7 +480,7 @@ func storeImage(ctx context.Context, s *store.Layout, i v1.Image, platform strin
480480
return err
481481
}
482482

483-
insecureSkipTLSVerify := derefInsecure(i.InsecureSkipTLSVerify)
483+
insecureSkipTLSVerify := i.InsecureSkipTLSVerify
484484
caFile := i.CaFile
485485

486486
log.BaseFromContext(ctx).Debugf("resolving image [%s] (verified=%t, platform=%q, excludeExtras=%t, insecureSkipTLSVerify=%t, caFile=%q, rewrite=%q, digest=%q)", i.Name, verified, platform, excludeExtras, insecureSkipTLSVerify, caFile, rewrite, pinnedDigest)
@@ -777,11 +777,11 @@ type chartJob struct {
777777
//
778778
// The three precedence rules are not uniform. registry is CLI > annotation.
779779
// excludeExtras is a one-way switch that any of the three sources can flip on
780-
// and none can flip off. platform is per-chart > CLI > annotation: an explicit
781-
// --platform is run-time intent and outranks manifest metadata. That last rule
782-
// must stay identical to resolveImageJobs's, or a single `hauler store sync`
783-
// run would pull a chart's discovered images for a different platform than the
784-
// manifest's own Images section.
780+
// and none can flip off, since a plain bool has no unset state. platform is
781+
// CLI > per-chart > annotation. That last rule must stay identical to
782+
// resolveImageJobs's, or a single `hauler store sync` run would pull a
783+
// chart's discovered images for a different platform than the manifest's own
784+
// Images section.
785785
//
786786
// Every job allocates its own *action.ChartPathOptions. flags.AddChartOpts
787787
// holds that as a pointer, so copying the struct alone would leave sibling
@@ -795,20 +795,15 @@ func resolveChartJobs(o *flags.SyncOpts, annotations map[string]string, manifest
795795

796796
jobs := make([]chartJob, 0, len(charts))
797797
for _, ch := range charts {
798-
excludeExtras := o.ExcludeExtras
799-
if !o.ExcludeExtras && annotations[consts.ImageAnnotationExcludeExtras] == "true" {
800-
excludeExtras = true
801-
}
802-
if ch.ExcludeExtras {
803-
excludeExtras = ch.ExcludeExtras
804-
}
798+
excludeExtras := resolveBoolFlag(ch.ExcludeExtras, annotations[consts.ImageAnnotationExcludeExtras] == "true", o.ExcludeExtras, o.ExcludeExtrasChanged)
805799

806800
platform := o.Platform
807-
if o.Platform == "" && annotations[consts.ImageAnnotationPlatform] != "" {
808-
platform = annotations[consts.ImageAnnotationPlatform]
809-
}
810-
if ch.Platform != "" {
811-
platform = ch.Platform
801+
if o.Platform == "" {
802+
if ch.Platform != "" {
803+
platform = ch.Platform
804+
} else if annotations[consts.ImageAnnotationPlatform] != "" {
805+
platform = annotations[consts.ImageAnnotationPlatform]
806+
}
812807
}
813808

814809
var valuesFiles []string
@@ -826,16 +821,13 @@ func resolveChartJobs(o *flags.SyncOpts, annotations map[string]string, manifest
826821
if caFile == "" {
827822
if ch.CaFile != "" {
828823
caFile = ch.CaFile
829-
} else if annotations[consts.ImageAnnotationCaFile] == "true" {
824+
} else if annotations[consts.ImageAnnotationCaFile] != "" {
830825
caFile = annotations[consts.ImageAnnotationCaFile]
831826
}
832827
}
833828

834-
insecureSkipTLSVerify := false
835-
if o.CaFile == "" {
836-
insecureSkipTLSVerify = resolveInsecure(ch.InsecureSkipTLSVerify, annotations, o.InsecureSkipTLSVerify)
837-
} else {
838-
}
829+
// a CA file and skipping TLS verification are mutually exclusive: providing one forces verification on
830+
insecureSkipTLSVerify := o.CaFile == "" && resolveBoolFlag(ch.InsecureSkipTLSVerify, annotations[consts.ImageAnnotationInsecureSkipTLSVerify] == "true", o.InsecureSkipTLSVerify, o.InsecureChanged)
839831

840832
jobs = append(jobs, chartJob{
841833
cfg: ch,
@@ -1381,12 +1373,11 @@ func fetchChart(ctx context.Context, s *store.Layout, j chartJob, tempRoot strin
13811373
// there is no separate per-discovered-image TLS knob in a chart
13821374
// manifest, so the registry a chart's images live in is assumed
13831375
// to share the chart repo's trust configuration.
1384-
chartInsecure := j.opts.ChartOpts.InsecureSkipTLSVerify
13851376
imageJobs = append(imageJobs, imageJob{
13861377
img: v1.Image{
13871378
Name: relocated,
13881379
CaFile: j.opts.ChartOpts.CaFile,
1389-
InsecureSkipTLSVerify: &chartInsecure,
1380+
InsecureSkipTLSVerify: j.opts.ChartOpts.InsecureSkipTLSVerify,
13901381
},
13911382
platform: j.opts.Platform,
13921383
excludeExtras: j.opts.ExcludeExtras,

‎cmd/hauler/cli/store/add_test.go‎

Lines changed: 65 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1798,12 +1798,13 @@ func TestResolveChartJobs_ExcludeExtras(t *testing.T) {
17981798
tests := []struct {
17991799
name string
18001800
cli bool
1801+
cliChanged bool
18011802
annotation string
18021803
perChart bool
18031804
want bool
18041805
}{
18051806
{name: "nothing set", want: false},
1806-
{name: "CLI flag alone", cli: true, want: true},
1807+
{name: "CLI flag alone", cli: true, cliChanged: true, want: true},
18071808
{name: "annotation alone", annotation: "true", want: true},
18081809
{name: "per-chart alone", perChart: true, want: true},
18091810
{
@@ -1816,26 +1817,37 @@ func TestResolveChartJobs_ExcludeExtras(t *testing.T) {
18161817
// --exclude-extras back off; both are one-way switches.
18171818
name: "CLI flag survives an annotation that is not true",
18181819
cli: true,
1820+
cliChanged: true,
18191821
annotation: "false",
18201822
want: true,
18211823
},
18221824
{
1823-
name: "CLI flag survives a false per-chart field",
1824-
cli: true,
1825-
perChart: false,
1826-
want: true,
1825+
name: "CLI flag survives a false per-chart field",
1826+
cli: true,
1827+
cliChanged: true,
1828+
perChart: false,
1829+
want: true,
18271830
},
18281831
{
18291832
name: "annotation survives a false per-chart field",
18301833
annotation: "true",
18311834
perChart: false,
18321835
want: true,
18331836
},
1837+
{
1838+
// An explicit CLI --exclude-extras=false wins outright over an
1839+
// annotation/per-chart true.
1840+
name: "explicit CLI false overrides annotation and per-chart",
1841+
cliChanged: true,
1842+
annotation: "true",
1843+
perChart: true,
1844+
want: false,
1845+
},
18341846
}
18351847

18361848
for _, tc := range tests {
18371849
t.Run(tc.name, func(t *testing.T) {
1838-
o := &flags.SyncOpts{ExcludeExtras: tc.cli}
1850+
o := &flags.SyncOpts{ExcludeExtras: tc.cli, ExcludeExtrasChanged: tc.cliChanged}
18391851
a := map[string]string{}
18401852
if tc.annotation != "" {
18411853
a[consts.ImageAnnotationExcludeExtras] = tc.annotation
@@ -1877,10 +1889,16 @@ func TestResolveChartJobs_Platform(t *testing.T) {
18771889
want: "linux/amd64",
18781890
},
18791891
{
1880-
name: "per-chart wins over both",
1892+
name: "CLI flag wins over annotation and per-chart",
18811893
cli: "linux/amd64",
18821894
annotation: "linux/arm64",
18831895
perChart: "linux/s390x",
1896+
want: "linux/amd64",
1897+
},
1898+
{
1899+
name: "per-chart wins over annotation when CLI flag unset",
1900+
annotation: "linux/arm64",
1901+
perChart: "linux/s390x",
18841902
want: "linux/s390x",
18851903
},
18861904
{
@@ -2155,7 +2173,6 @@ func TestResolveChartJobs_NoCharts(t *testing.T) {
21552173
// TestResolveChartJobs_CredentialFields pins that every TLS/verification
21562174
// field on v1.Chart reaches the job's ChartOpts unchanged.
21572175
func TestResolveChartJobs_CredentialFields(t *testing.T) {
2158-
insecure := true
21592176
ch := v1.Chart{
21602177
Name: "rancher",
21612178
Verify: true,
@@ -2164,7 +2181,7 @@ func TestResolveChartJobs_CredentialFields(t *testing.T) {
21642181
CertFile: "/certs/client.crt",
21652182
KeyFile: "/certs/client.key",
21662183
CaFile: "/certs/ca.crt",
2167-
InsecureSkipTLSVerify: &insecure,
2184+
InsecureSkipTLSVerify: true,
21682185
PlainHTTP: true,
21692186
}
21702187

@@ -2195,14 +2212,47 @@ func TestResolveChartJobs_CredentialFields(t *testing.T) {
21952212
if opts.CaFile != ch.CaFile {
21962213
t.Errorf("CaFile = %q, want %q", opts.CaFile, ch.CaFile)
21972214
}
2198-
if opts.InsecureSkipTLSVerify != derefInsecure(ch.InsecureSkipTLSVerify) {
2199-
t.Errorf("InsecureSkipTLSVerify = %v, want %v", opts.InsecureSkipTLSVerify, derefInsecure(ch.InsecureSkipTLSVerify))
2215+
if opts.InsecureSkipTLSVerify != ch.InsecureSkipTLSVerify {
2216+
t.Errorf("InsecureSkipTLSVerify = %v, want %v", opts.InsecureSkipTLSVerify, ch.InsecureSkipTLSVerify)
22002217
}
22012218
if opts.PlainHTTP != ch.PlainHTTP {
22022219
t.Errorf("PlainHTTP = %v, want %v", opts.PlainHTTP, ch.PlainHTTP)
22032220
}
22042221
}
22052222

2223+
func TestResolveChartJobs_CaFilePrecedence(t *testing.T) {
2224+
tests := []struct {
2225+
name string
2226+
cli string
2227+
annotation string
2228+
perChart string
2229+
want string
2230+
}{
2231+
{name: "annotation used when CLI and per-chart unset", annotation: "/ann/ca.crt", want: "/ann/ca.crt"},
2232+
{name: "per-chart wins over annotation", annotation: "/ann/ca.crt", perChart: "/chart/ca.crt", want: "/chart/ca.crt"},
2233+
{name: "CLI wins over per-chart and annotation", cli: "/cli/ca.crt", annotation: "/ann/ca.crt", perChart: "/chart/ca.crt", want: "/cli/ca.crt"},
2234+
{name: "none set stays empty", want: ""},
2235+
}
2236+
2237+
for _, tc := range tests {
2238+
t.Run(tc.name, func(t *testing.T) {
2239+
o := &flags.SyncOpts{CaFile: tc.cli}
2240+
a := map[string]string{}
2241+
if tc.annotation != "" {
2242+
a[consts.ImageAnnotationCaFile] = tc.annotation
2243+
}
2244+
2245+
jobs, err := resolveChartJobs(o, a, "/manifests", []v1.Chart{{Name: "rancher", CaFile: tc.perChart}})
2246+
if err != nil {
2247+
t.Fatalf("resolveChartJobs: %v", err)
2248+
}
2249+
if got := jobs[0].opts.ChartOpts.CaFile; got != tc.want {
2250+
t.Errorf("CaFile = %q, want %q", got, tc.want)
2251+
}
2252+
})
2253+
}
2254+
}
2255+
22062256
// TestResolveChartJobs_CredentialEnv pins that UsernameEnv/PasswordEnv are
22072257
// resolved into ChartOpts.Username/Password via resolveChartCreds.
22082258
func TestResolveChartJobs_CredentialEnv(t *testing.T) {
@@ -2999,7 +3049,7 @@ func TestStoreImage_CAFileAndInsecure(t *testing.T) {
29993049
t.Run("bad caFile without insecure returns error and stores nothing", func(t *testing.T) {
30003050
s := newTestStore(t)
30013051
insecure := false
3002-
img := v1.Image{Name: ref, CaFile: missingCA, InsecureSkipTLSVerify: &insecure}
3052+
img := v1.Image{Name: ref, CaFile: missingCA, InsecureSkipTLSVerify: insecure}
30033053
err := storeImage(ctx, s, img, "", false,
30043054
defaultRootOpts(s.Root), defaultCliOpts(), "", "", false)
30053055
if err == nil {
@@ -3017,7 +3067,7 @@ func TestStoreImage_CAFileAndInsecure(t *testing.T) {
30173067
t.Fatal(err)
30183068
}
30193069
insecure := false
3020-
img := v1.Image{Name: ref, CaFile: junk, InsecureSkipTLSVerify: &insecure}
3070+
img := v1.Image{Name: ref, CaFile: junk, InsecureSkipTLSVerify: insecure}
30213071
err := storeImage(ctx, s, img, "", false,
30223072
defaultRootOpts(s.Root), defaultCliOpts(), "", "", false)
30233073
if err == nil {
@@ -3031,7 +3081,7 @@ func TestStoreImage_CAFileAndInsecure(t *testing.T) {
30313081
// ignored and the pull still succeeds. If caFile were read first, the
30323082
// pull would error and nothing would be stored.
30333083
insecure := true
3034-
img := v1.Image{Name: ref, CaFile: missingCA, InsecureSkipTLSVerify: &insecure}
3084+
img := v1.Image{Name: ref, CaFile: missingCA, InsecureSkipTLSVerify: insecure}
30353085
err := storeImage(ctx, s, img, "", false,
30363086
defaultRootOpts(s.Root), defaultCliOpts(), "", "", false)
30373087
if err != nil {
@@ -3043,7 +3093,7 @@ func TestStoreImage_CAFileAndInsecure(t *testing.T) {
30433093
t.Run("valid caFile without insecure is accepted", func(t *testing.T) {
30443094
s := newTestStore(t)
30453095
insecure := false
3046-
img := v1.Image{Name: ref, CaFile: writeCAFile(t), InsecureSkipTLSVerify: &insecure}
3096+
img := v1.Image{Name: ref, CaFile: writeCAFile(t), InsecureSkipTLSVerify: insecure}
30473097
err := storeImage(ctx, s, img, "", false,
30483098
defaultRootOpts(s.Root), defaultCliOpts(), "", "", false)
30493099
if err != nil {

0 commit comments

Comments
 (0)