Skip to content

Commit c146b46

Browse files
zackbradyszackbradys
committed
initial commit for fixing cmd precedents
1 parent 243671f commit c146b46

10 files changed

Lines changed: 276 additions & 192 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: 20 additions & 29 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:
@@ -493,7 +493,7 @@ func storeImage(ctx context.Context, s *store.Layout, i v1.Image, platform strin
493493
}
494494
}
495495

496-
insecureSkipTLSVerify := derefInsecure(i.InsecureSkipTLSVerify)
496+
insecureSkipTLSVerify := i.InsecureSkipTLSVerify
497497
caFile := i.CaFile
498498

499499
// fetch image along with any associated signatures and attestations.
@@ -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
@@ -831,11 +826,8 @@ func resolveChartJobs(o *flags.SyncOpts, annotations map[string]string, manifest
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: 32 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,8 +2212,8 @@ 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)
@@ -2999,7 +3016,7 @@ func TestStoreImage_CAFileAndInsecure(t *testing.T) {
29993016
t.Run("bad caFile without insecure returns error and stores nothing", func(t *testing.T) {
30003017
s := newTestStore(t)
30013018
insecure := false
3002-
img := v1.Image{Name: ref, CaFile: missingCA, InsecureSkipTLSVerify: &insecure}
3019+
img := v1.Image{Name: ref, CaFile: missingCA, InsecureSkipTLSVerify: insecure}
30033020
err := storeImage(ctx, s, img, "", false,
30043021
defaultRootOpts(s.Root), defaultCliOpts(), "", "", false)
30053022
if err == nil {
@@ -3017,7 +3034,7 @@ func TestStoreImage_CAFileAndInsecure(t *testing.T) {
30173034
t.Fatal(err)
30183035
}
30193036
insecure := false
3020-
img := v1.Image{Name: ref, CaFile: junk, InsecureSkipTLSVerify: &insecure}
3037+
img := v1.Image{Name: ref, CaFile: junk, InsecureSkipTLSVerify: insecure}
30213038
err := storeImage(ctx, s, img, "", false,
30223039
defaultRootOpts(s.Root), defaultCliOpts(), "", "", false)
30233040
if err == nil {
@@ -3031,7 +3048,7 @@ func TestStoreImage_CAFileAndInsecure(t *testing.T) {
30313048
// ignored and the pull still succeeds. If caFile were read first, the
30323049
// pull would error and nothing would be stored.
30333050
insecure := true
3034-
img := v1.Image{Name: ref, CaFile: missingCA, InsecureSkipTLSVerify: &insecure}
3051+
img := v1.Image{Name: ref, CaFile: missingCA, InsecureSkipTLSVerify: insecure}
30353052
err := storeImage(ctx, s, img, "", false,
30363053
defaultRootOpts(s.Root), defaultCliOpts(), "", "", false)
30373054
if err != nil {
@@ -3043,7 +3060,7 @@ func TestStoreImage_CAFileAndInsecure(t *testing.T) {
30433060
t.Run("valid caFile without insecure is accepted", func(t *testing.T) {
30443061
s := newTestStore(t)
30453062
insecure := false
3046-
img := v1.Image{Name: ref, CaFile: writeCAFile(t), InsecureSkipTLSVerify: &insecure}
3063+
img := v1.Image{Name: ref, CaFile: writeCAFile(t), InsecureSkipTLSVerify: insecure}
30473064
err := storeImage(ctx, s, img, "", false,
30483065
defaultRootOpts(s.Root), defaultCliOpts(), "", "", false)
30493066
if err != nil {

0 commit comments

Comments
 (0)