diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 0c13238b..6b712b22 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -144,6 +144,8 @@ jobs: - name: helm diff upgrade --install helm-diff ./helm-diff run: helm diff upgrade --install helm-diff ./helm-diff + env: + HELM_DEBUG: "true" - name: helm upgrade -i helm-diff ./helm-diff run: helm upgrade -i helm-diff ./helm-diff diff --git a/cmd/helm.go b/cmd/helm.go index 55d3ccea..d697ea92 100644 --- a/cmd/helm.go +++ b/cmd/helm.go @@ -331,8 +331,12 @@ 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. + // See https://github.com/databus23/helm-diff/issues/894 + if d.dryRunMode != "client" { + flags = append(flags, "--dry-run=server") + } } else { flags = append(flags, "--validate") } @@ -353,42 +357,28 @@ 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 is already added above if d.dryRunMode != "client" + // Only add it here if d.dryRunMode == "client" or for Helm v3 + if !(isHelmV4 && d.dryRunMode != "client") { + 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..9de2f112 100644 --- a/cmd/helm_test.go +++ b/cmd/helm_test.go @@ -1,11 +1,185 @@ package cmd import ( + "reflect" "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 cfg.dryRunMode != "client" { + flags = append(flags, "--dry-run=server") + } + } else { + flags = append(flags, "--validate") + } + } + + if cfg.supportsDryRunLookup { + if !(cfg.isHelmV4 && cfg.dryRunMode != "client") { + 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 TestExtractManifestFromHelmUpgradeDryRunOutput(t *testing.T) { type testdata struct { description string 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 {