diff --git a/cmd/helm.go b/cmd/helm.go index 55d3ccea..c186b619 100644 --- a/cmd/helm.go +++ b/cmd/helm.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "regexp" + "slices" "strconv" "strings" @@ -331,8 +332,14 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) { if !d.disableValidation && d.clusterAccessAllowed() { isHelmV4, err := isHelmVersionGreaterThanEqual(helmV4Version) if err == nil && isHelmV4 { - // Flag --validate has been deprecated, use '--dry-run=server' instead in Helm v4+ - flags = append(flags, "--dry-run=server") + // For Helm v4, we use --dry-run=server by default to get correct .Capabilities.APIVersions. + // This is only applied if the user hasn't explicitly set --dry-run=client, --dry-run=true, or --dry-run=false. + // Note: dryRunMode="true" behaves like "client" (no cluster access). + // Note: dryRunMode="false" behaves like "none" (no dry-run flag at all). + // See https://github.com/databus23/helm-diff/issues/894 + if !slices.Contains([]string{"client", "true", "false"}, d.dryRunMode) { + flags = append(flags, "--dry-run=server") + } } else { flags = append(flags, "--validate") } @@ -353,42 +360,34 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) { // To keep the full compatibility with older helm-diff versions, // we pass --dry-run to `helm template` only if Helm is greater than v3.13.0. if useDryRunService, err := isHelmVersionAtLeast(minHelmVersionWithDryRunLookupSupport); err == nil && useDryRunService { - // However, which dry-run mode to use is still not clear. - // - // For compatibility with the old and new helm-diff options, - // old and new helm, we assume that the user wants to use the older `helm template --dry-run=client` mode - // if helm-diff has been invoked with any of the following flags: - // - // * no dry-run flags (to be consistent with helm-template) - // * --dry-run - // * --dry-run="" - // * --dry-run=client - // - // and the newer `helm template --dry-run=server` mode when invoked with: - // - // * --dry-run=server - // - // Any other values should result in errors. - // - // See the fllowing link for more details: - // - https://github.com/databus23/helm-diff/pull/458 - // - https://github.com/helm/helm/pull/9426#issuecomment-1501005666 - if d.dryRunMode == "server" { - // This is for security reasons! - // - // We give helm-template the additional cluster access for the helm `lookup` function - // only if the user has explicitly requested it by --dry-run=server, - // - // In other words, although helm-diff-upgrade implies limited cluster access by default, - // helm-diff-upgrade without a --dry-run flag does NOT imply - // full cluster-access via helm-template --dry-run=server! - flags = append(flags, "--dry-run=server") - } else { - // Since helm-diff 3.9.0 and helm 3.13.0, we pass --dry-run=client to `helm template` by default. - // This doesn't make any difference for helm-diff itself, - // because helm-template w/o flags is equivalent to helm-template --dry-run=client. - // See https://github.com/helm/helm/pull/9426#discussion_r1181397259 - flags = append(flags, "--dry-run=client") + isHelmV4, _ := isHelmVersionGreaterThanEqual(helmV4Version) + + // For Helm v4, --dry-run=server may already have been added above when + // clusterAccessAllowed() is true and d.dryRunMode is not "client", "true", or "false". + // In that case (Helm v4 and d.dryRunMode not "client"/"true"/"false"), we skip adding any + // additional dry-run flag here. In all other cases (Helm v3 or d.dryRunMode is "client"/"true"), + // we add the appropriate dry-run mode below. + // Note: dryRunMode="false" means no dry-run flag at all. + if d.dryRunMode == "false" { + // "false" means no dry-run, skip adding any dry-run flag + } else if !(isHelmV4 && !slices.Contains([]string{"client", "true"}, d.dryRunMode)) { + if d.dryRunMode == "server" { + // This is for security reasons! + // + // We give helm-template the additional cluster access for the helm `lookup` function + // only if the user has explicitly requested it by --dry-run=server, + // + // In other words, although helm-diff-upgrade implies limited cluster access by default, + // helm-diff-upgrade without a --dry-run flag does NOT imply + // full cluster-access via helm-template --dry-run=server! + flags = append(flags, "--dry-run=server") + } else { + // Since helm-diff 3.9.0 and helm 3.13.0, we pass --dry-run=client to `helm template` by default. + // This doesn't make any difference for helm-diff itself, + // because helm-template w/o flags is equivalent to helm-template --dry-run=client. + // See https://github.com/helm/helm/pull/9426#discussion_r1181397259 + flags = append(flags, "--dry-run=client") + } } } diff --git a/cmd/helm_test.go b/cmd/helm_test.go index bd11e086..d1f23737 100644 --- a/cmd/helm_test.go +++ b/cmd/helm_test.go @@ -1,11 +1,250 @@ package cmd import ( + "reflect" + "slices" "testing" "github.com/google/go-cmp/cmp" ) +type dryRunFlagsConfig struct { + isHelmV4 bool + supportsDryRunLookup bool + clusterAccessAllowed bool + disableValidation bool + dryRunMode string +} + +func getTemplateDryRunFlags(cfg dryRunFlagsConfig) []string { + var flags []string + + if !cfg.disableValidation && cfg.clusterAccessAllowed { + if cfg.isHelmV4 { + if !slices.Contains([]string{"client", "true", "false"}, cfg.dryRunMode) { + flags = append(flags, "--dry-run=server") + } + } else { + flags = append(flags, "--validate") + } + } + + if cfg.supportsDryRunLookup { + if cfg.dryRunMode == "false" { + // "false" means no dry-run, skip adding any dry-run flag + } else if !(cfg.isHelmV4 && !slices.Contains([]string{"client", "true"}, cfg.dryRunMode)) { + if cfg.dryRunMode == "server" { + flags = append(flags, "--dry-run=server") + } else { + flags = append(flags, "--dry-run=client") + } + } + } + + return flags +} + +func TestGetTemplateDryRunFlags(t *testing.T) { + cases := []struct { + name string + config dryRunFlagsConfig + expected []string + }{ + { + name: "Helm v4 with no explicit dry-run flag uses server mode", + config: dryRunFlagsConfig{ + isHelmV4: true, + supportsDryRunLookup: true, + clusterAccessAllowed: true, + disableValidation: false, + dryRunMode: "none", + }, + expected: []string{"--dry-run=server"}, + }, + { + name: "Helm v4 with dry-run=client uses client mode", + config: dryRunFlagsConfig{ + isHelmV4: true, + supportsDryRunLookup: true, + clusterAccessAllowed: false, + disableValidation: false, + dryRunMode: "client", + }, + expected: []string{"--dry-run=client"}, + }, + { + name: "Helm v4 with dry-run=server uses server mode", + config: dryRunFlagsConfig{ + isHelmV4: true, + supportsDryRunLookup: true, + clusterAccessAllowed: true, + disableValidation: false, + dryRunMode: "server", + }, + expected: []string{"--dry-run=server"}, + }, + { + name: "Helm v4 with validation disabled and dry-run=none skips dry-run flags", + config: dryRunFlagsConfig{ + isHelmV4: true, + supportsDryRunLookup: true, + clusterAccessAllowed: true, + disableValidation: true, + dryRunMode: "none", + }, + expected: nil, + }, + { + name: "Helm v4 with validation disabled and dry-run=client uses client mode", + config: dryRunFlagsConfig{ + isHelmV4: true, + supportsDryRunLookup: true, + clusterAccessAllowed: true, + disableValidation: true, + dryRunMode: "client", + }, + expected: []string{"--dry-run=client"}, + }, + { + name: "Helm v3 with no explicit dry-run flag uses validate and client", + config: dryRunFlagsConfig{ + isHelmV4: false, + supportsDryRunLookup: true, + clusterAccessAllowed: true, + disableValidation: false, + dryRunMode: "none", + }, + expected: []string{"--validate", "--dry-run=client"}, + }, + { + name: "Helm v3 with dry-run=server uses server mode", + config: dryRunFlagsConfig{ + isHelmV4: false, + supportsDryRunLookup: true, + clusterAccessAllowed: true, + disableValidation: false, + dryRunMode: "server", + }, + expected: []string{"--validate", "--dry-run=server"}, + }, + { + name: "Helm v3 with dry-run=client uses client mode", + config: dryRunFlagsConfig{ + isHelmV4: false, + supportsDryRunLookup: true, + clusterAccessAllowed: false, + disableValidation: false, + dryRunMode: "client", + }, + expected: []string{"--dry-run=client"}, + }, + { + name: "Helm v3 without dry-run lookup support uses only validate", + config: dryRunFlagsConfig{ + isHelmV4: false, + supportsDryRunLookup: false, + clusterAccessAllowed: true, + disableValidation: false, + dryRunMode: "none", + }, + expected: []string{"--validate"}, + }, + { + name: "Helm v4 without dry-run lookup support uses server mode", + config: dryRunFlagsConfig{ + isHelmV4: true, + supportsDryRunLookup: false, + clusterAccessAllowed: true, + disableValidation: false, + dryRunMode: "none", + }, + expected: []string{"--dry-run=server"}, + }, + { + name: "Helm v4 with empty dry-run mode uses server mode", + config: dryRunFlagsConfig{ + isHelmV4: true, + supportsDryRunLookup: true, + clusterAccessAllowed: true, + disableValidation: false, + dryRunMode: "", + }, + expected: []string{"--dry-run=server"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + actual := getTemplateDryRunFlags(tc.config) + if !reflect.DeepEqual(actual, tc.expected) { + t.Errorf("Expected %v, got %v", tc.expected, actual) + } + }) + } +} + +func TestGetTemplateDryRunFlagsBoolModes(t *testing.T) { + cases := []struct { + name string + config dryRunFlagsConfig + expected []string + }{ + { + name: "Helm v3 dryRunMode=true behaves like client", + config: dryRunFlagsConfig{ + isHelmV4: false, + supportsDryRunLookup: true, + clusterAccessAllowed: true, + disableValidation: false, + dryRunMode: "true", + }, + expected: []string{"--validate", "--dry-run=client"}, + }, + { + name: "Helm v3 dryRunMode=false behaves like none", + config: dryRunFlagsConfig{ + isHelmV4: false, + supportsDryRunLookup: true, + clusterAccessAllowed: true, + disableValidation: false, + dryRunMode: "false", + }, + expected: []string{"--validate"}, + }, + { + name: "Helm v4 dryRunMode=true behaves like client", + config: dryRunFlagsConfig{ + isHelmV4: true, + supportsDryRunLookup: true, + clusterAccessAllowed: false, + disableValidation: false, + dryRunMode: "true", + }, + expected: []string{"--dry-run=client"}, + }, + { + name: "Helm v4 dryRunMode=false behaves like none", + config: dryRunFlagsConfig{ + isHelmV4: true, + supportsDryRunLookup: true, + clusterAccessAllowed: true, + disableValidation: false, + dryRunMode: "false", + }, + expected: nil, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + actual := getTemplateDryRunFlags(tc.config) + if !reflect.DeepEqual(actual, tc.expected) { + t.Errorf("Expected %v, got %v", tc.expected, actual) + } + }) + } +} + func TestExtractManifestFromHelmUpgradeDryRunOutput(t *testing.T) { type testdata struct { description string diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 40d01b4c..e96ea39b 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -27,6 +27,7 @@ var ( const ( dryRunNoOptDefVal = "client" + envTrue = "true" ) type diffCmd struct { @@ -116,7 +117,7 @@ func newChartCommand() *cobra.Command { diff := diffCmd{ namespace: os.Getenv("HELM_NAMESPACE"), } - unknownFlags := os.Getenv("HELM_DIFF_IGNORE_UNKNOWN_FLAGS") == "true" + unknownFlags := os.Getenv("HELM_DIFF_IGNORE_UNKNOWN_FLAGS") == envTrue cmd := &cobra.Command{ Use: "upgrade [flags] [RELEASE] [CHART]", @@ -166,10 +167,10 @@ func newChartCommand() *cobra.Command { cmd.SilenceUsage = true // See https://github.com/databus23/helm-diff/issues/253 - diff.useUpgradeDryRun = os.Getenv("HELM_DIFF_USE_UPGRADE_DRY_RUN") == "true" + diff.useUpgradeDryRun = os.Getenv("HELM_DIFF_USE_UPGRADE_DRY_RUN") == envTrue if !diff.threeWayMerge && !cmd.Flags().Changed("three-way-merge") { - enabled := os.Getenv("HELM_DIFF_THREE_WAY_MERGE") == "true" + enabled := os.Getenv("HELM_DIFF_THREE_WAY_MERGE") == envTrue diff.threeWayMerge = enabled if enabled { @@ -178,7 +179,7 @@ func newChartCommand() *cobra.Command { } if !diff.normalizeManifests && !cmd.Flags().Changed("normalize-manifests") { - enabled := os.Getenv("HELM_DIFF_NORMALIZE_MANIFESTS") == "true" + enabled := os.Getenv("HELM_DIFF_NORMALIZE_MANIFESTS") == envTrue diff.normalizeManifests = enabled if enabled { diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go index 6f6752c5..d1dbca28 100644 --- a/cmd/upgrade_test.go +++ b/cmd/upgrade_test.go @@ -1,6 +1,8 @@ package cmd -import "testing" +import ( + "testing" +) func TestIsRemoteAccessAllowed(t *testing.T) { cases := []struct {