diff --git a/README.md b/README.md index 7e75db50..4976fd27 100644 --- a/README.md +++ b/README.md @@ -149,6 +149,14 @@ Examples: # See https://github.com/databus23/helm-diff/issues/278 for more information. HELM_DIFF_IGNORE_UNKNOWN_FLAGS=true helm diff upgrade my-release stable/postgres --wait + # helm-diff disallows the use of the `lookup` function by default. + # To enable it, you must set HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN=true to + # use `helm template --dry-run=server` or + # `helm upgrade --dry-run=server` (in case you also set `HELM_DIFF_USE_UPGRADE_DRY_RUN`) + # See https://github.com/databus23/helm-diff/pull/458 + # for more information. + HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN=true helm diff upgrade my-release datadog/datadog + # Set HELM_DIFF_USE_UPGRADE_DRY_RUN=true to # use `helm upgrade --dry-run` instead of `helm template` to render manifests from the chart. # See https://github.com/databus23/helm-diff/issues/253 for more information. diff --git a/cmd/helm3.go b/cmd/helm3.go index dda15afc..cb3e8c1d 100644 --- a/cmd/helm3.go +++ b/cmd/helm3.go @@ -14,8 +14,9 @@ import ( ) var ( - helmVersionRE = regexp.MustCompile(`Version:\s*"([^"]+)"`) - minHelmVersion = semver.MustParse("v3.1.0-rc.1") + helmVersionRE = regexp.MustCompile(`Version:\s*"([^"]+)"`) + minHelmVersion = semver.MustParse("v3.1.0-rc.1") + // See https://github.com/helm/helm/pull/9426 minHelmVersionWithDryRunLookupSupport = semver.MustParse("v3.13.0") ) @@ -131,7 +132,7 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) { // Let's simulate that in helm-diff. // See https://medium.com/@kcatstack/understand-helm-upgrade-flags-reset-values-reuse-values-6e58ac8f127e shouldDefaultReusingValues := isUpgrade && len(d.values) == 0 && len(d.stringValues) == 0 && len(d.jsonValues) == 0 && len(d.valueFiles) == 0 && len(d.fileValues) == 0 - if (d.reuseValues || shouldDefaultReusingValues) && !d.resetValues && !d.dryRun { + if (d.reuseValues || shouldDefaultReusingValues) && !d.resetValues && d.clusterAccessAllowed() { tmpfile, err := os.CreateTemp("", "existing-values") if err != nil { return nil, err @@ -195,17 +196,28 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) { filter func([]byte) []byte ) + // `--dry-run=client` or `--dry-run=server`? + // + // Or what's the relationoship between helm-diff's --dry-run flag, + // HELM_DIFF_UPGRADE_DRY_RUN env var and the helm upgrade --dry-run flag? + // + // Read on to find out. if d.useUpgradeDryRun { - if d.dryRun { - return nil, fmt.Errorf("`diff upgrade --dry-run` conflicts with HELM_DIFF_USE_UPGRADE_DRY_RUN_AS_TEMPLATE. Either remove --dry-run to enable cluster access, or unset HELM_DIFF_USE_UPGRADE_DRY_RUN_AS_TEMPLATE to make cluster access unnecessary") - } - if d.isAllowUnreleased() { // Otherwise you get the following error when this is a diff for a new install // Error: UPGRADE FAILED: "$RELEASE_NAME" has no deployed releases flags = append(flags, "--install") } + // If the program reaches here, + // we are sure that the user wants to user the `helm upgrade --dry-run` command + // for generating the manifests to be diffed. + // + // So the question is only whether to use `--dry-run=client` or `--dry-run=server`. + // + // As HELM_DIFF_UPGRADE_DRY_RUN is there for producing more complete and correct diff results, + // we use --dry-run=server if the version of helm supports it. + // Otherwise, we use --dry-run=client, as that's the best we can do. if useDryRunService, err := isHelmVersionAtLeast(minHelmVersionWithDryRunLookupSupport); err == nil && useDryRunService { flags = append(flags, "--dry-run=server") } else { @@ -216,7 +228,7 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) { return extractManifestFromHelmUpgradeDryRunOutput(s, d.noHooks) } } else { - if !d.disableValidation && !d.dryRun { + if !d.disableValidation && d.clusterAccessAllowed() { flags = append(flags, "--validate") } @@ -232,8 +244,46 @@ func (d *diffCmd) template(isUpgrade bool) ([]byte, error) { flags = append(flags, "--kube-version", d.kubeVersion) } + // 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 { - flags = append(flags, "--dry-run=server") + // 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") + } } subcmd = "template" diff --git a/cmd/upgrade.go b/cmd/upgrade.go index 5a975f00..70210cfa 100644 --- a/cmd/upgrade.go +++ b/cmd/upgrade.go @@ -13,6 +13,7 @@ import ( jsoniterator "github.com/json-iterator/go" "github.com/pkg/errors" "github.com/spf13/cobra" + "github.com/spf13/pflag" "helm.sh/helm/v3/pkg/action" "helm.sh/helm/v3/pkg/cli" "helm.sh/helm/v3/pkg/kube" @@ -37,7 +38,8 @@ type diffCmd struct { devel bool disableValidation bool disableOpenAPIValidation bool - dryRun bool + dryRunMode string + dryRunModeSpecified bool namespace string // namespace to assume the release to be installed into. Defaults to the current kube config namespace. valueFiles valueFiles values []string @@ -68,6 +70,28 @@ func (d *diffCmd) isAllowUnreleased() bool { return d.allowUnreleased || d.install } +// clusterAccessAllowed returns true if the diff command is allowed to access the cluster at some degree. +// +// helm-diff basically have 3 modes of operation: +// 1. no cluster access at all when --dry-run, --dry-run=client, or --dry-run=true is specified. +// 2. basic cluster access when --dry-run is not specified. +// 3. extra cluster access when --dry-run=server is specified. +// +// clusterAccessAllowed returns true when the mode is either 2 or 3. +// +// If false, helm-diff should not access the cluster at all. +// More concretely: +// - It shouldn't pass --validate to helm-template because it requires cluster access. +// - It shouldn't get the current release manifest using helm-get-manifest because it requires cluster access. +// - It shouldn't get the current release hooks using helm-get-hooks because it requires cluster access. +// - It shouldn't get the current release values using helm-get-values because it requires cluster access. +// +// See also https://github.com/helm/helm/pull/9426#discussion_r1181397259 +func (d *diffCmd) clusterAccessAllowed() bool { + clientOnly := (d.dryRunModeSpecified && d.dryRunMode == "") || d.dryRunMode == "true" || d.dryRunMode == "client" + return !clientOnly +} + const globalUsage = `Show a diff explaining what a helm upgrade would change. This fetches the currently deployed version of a release @@ -85,9 +109,10 @@ func newChartCommand() *cobra.Command { } cmd := &cobra.Command{ - Use: "upgrade [flags] [RELEASE] [CHART]", - Short: "Show a diff explaining what a helm upgrade would change.", - Long: globalUsage, + Use: "upgrade [flags] [RELEASE] [CHART]", + Short: "Show a diff explaining what a helm upgrade would change.", + Long: globalUsage, + DisableFlagParsing: true, Example: strings.Join([]string{ " helm diff upgrade my-release stable/postgresql --values values.yaml", "", @@ -96,6 +121,14 @@ func newChartCommand() *cobra.Command { " # See https://github.com/databus23/helm-diff/issues/278 for more information.", " HELM_DIFF_IGNORE_UNKNOWN_FLAGS=true helm diff upgrade my-release stable/postgres --wait", "", + " # helm-diff disallows the use of the `lookup` function by default.", + " # To enable it, you must set HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN=true to", + " # use `helm template --dry-run=server` or", + " # `helm upgrade --dry-run=server` (in case you also set `HELM_DIFF_USE_UPGRADE_DRY_RUN`)", + " # See https://github.com/databus23/helm-diff/pull/458", + " # for more information.", + " HELM_DIFF_USE_INSECURE_SERVER_SIDE_DRY_RUN=true helm diff upgrade my-release datadog/datadog", + "", " # Set HELM_DIFF_USE_UPGRADE_DRY_RUN=true to", " # use `helm upgrade --dry-run` instead of `helm template` to render manifests from the chart.", " # See https://github.com/databus23/helm-diff/issues/253 for more information.", @@ -118,10 +151,70 @@ func newChartCommand() *cobra.Command { "# Read the flag usage below for more information on --context.", "HELM_DIFF_OUTPUT_CONTEXT=5 helm diff upgrade my-release datadog/datadog", }, "\n"), - Args: func(cmd *cobra.Command, args []string) error { - return checkArgsLength(len(args), "release name", "chart path") - }, RunE: func(cmd *cobra.Command, args []string) error { + // Note that we can't just move this block to PersistentPreRunE, + // because cmd.SetArgs(args) does not persist between PersistentPreRunE and RunE. + // The choice is between: + // 1. Doing this in RunE + // 2. Doing this in PersistentPreRunE, saving args somewhere, and calling cmd.SetArgs(args) again in RunE + // 2 is more complicated without much benefit, so we choose 1. + { + const ( + dryRunUsage = "--dry-run, --dry-run=client, or --dry-run=true disables cluster access and show diff as if it was install. Implies --install, --reset-values, and --disable-validation." + + " --dry-run=server enables the cluster access with helm-get and the lookup template function." + ) + + legacyDryRunFlagSet := pflag.NewFlagSet("upgrade", pflag.ContinueOnError) + legacyDryRun := legacyDryRunFlagSet.Bool("dry-run", false, dryRunUsage) + if err := legacyDryRunFlagSet.Parse(args); err == nil && *legacyDryRun { + diff.dryRunModeSpecified = true + args = legacyDryRunFlagSet.Args() + } else { + cmd.Flags().StringVar(&diff.dryRunMode, "dry-run", "", dryRunUsage) + } + + fmt.Fprintf(os.Stderr, "args after legacy dry-run parsing: %v\n", args) + + // Here we parse the flags ourselves so that we can support + // both --dry-run and --dry-run=ARG syntaxes. + // + // If you don't do this, then cobra will treat --dry-run as + // a "flag needs an argument: --dry-run" error. + // + // This works becase we have `DisableFlagParsing: true`` above. + // Never turn that off, or you'll get the error again. + if err := cmd.Flags().Parse(args); err != nil { + return err + } + + args = cmd.Flags().Args() + + if !diff.dryRunModeSpecified { + dryRunModeSpecified := cmd.Flags().Changed("dry-run") + diff.dryRunModeSpecified = dryRunModeSpecified + + if dryRunModeSpecified && !(diff.dryRunMode == "client" || diff.dryRunMode == "server") { + return fmt.Errorf("flag %q must take an argument of %q or %q but got %q", "dry-run", "client", "server", diff.dryRunMode) + } + } + + cmd.SetArgs(args) + + // We have to do this here because we have to sort out the + // --dry-run flag ambiguity to determine the args to be checked. + // + // In other words, we can't just do: + // + // cmd.Args = func(cmd *cobra.Command, args []string) error { + // return checkArgsLength(len(args), "release name", "chart path") + // } + // + // Because it seems to take precedence over the PersistentPreRunE + if err := checkArgsLength(len(args), "release name", "chart path"); err != nil { + return err + } + } + // Suppress the command usage on error. See #77 for more info cmd.SilenceUsage = true @@ -195,7 +288,6 @@ func newChartCommand() *cobra.Command { f.BoolVar(&diff.devel, "devel", false, "use development versions, too. Equivalent to version '>0.0.0-0'. If --version is set, this is ignored.") f.BoolVar(&diff.disableValidation, "disable-validation", false, "disables rendered templates validation against the Kubernetes cluster you are currently pointing to. This is the same validation performed on an install") f.BoolVar(&diff.disableOpenAPIValidation, "disable-openapi-validation", false, "disables rendered templates validation against the Kubernetes OpenAPI Schema") - f.BoolVar(&diff.dryRun, "dry-run", false, "disables cluster access and show diff as if it was install. Implies --install, --reset-values, and --disable-validation") f.StringVar(&diff.postRenderer, "post-renderer", "", "the path to an executable to be used for post rendering. If it exists in $PATH, the binary will be used, otherwise it will try to look for the executable at the given path") f.StringArrayVar(&diff.postRendererArgs, "post-renderer-args", []string{}, "an argument to the post-renderer (can specify multiple)") f.BoolVar(&diff.insecureSkipTLSVerify, "insecure-skip-tls-verify", false, "skip tls certificate checks for the chart download") @@ -215,7 +307,7 @@ func (d *diffCmd) runHelm3() error { var err error - if !d.dryRun { + if d.clusterAccessAllowed() { releaseManifest, err = getRelease(d.release, d.namespace) } @@ -262,7 +354,7 @@ func (d *diffCmd) runHelm3() error { } currentSpecs := make(map[string]*manifest.MappingResult) - if !newInstall && !d.dryRun { + if !newInstall && d.clusterAccessAllowed() { if !d.noHooks && !d.threeWayMerge { hooks, err := getHooks(d.release, d.namespace) if err != nil { diff --git a/cmd/upgrade_test.go b/cmd/upgrade_test.go new file mode 100644 index 00000000..e779599b --- /dev/null +++ b/cmd/upgrade_test.go @@ -0,0 +1,58 @@ +package cmd + +import "testing" + +func TestIsRemoteAccessAllowed(t *testing.T) { + cases := []struct { + name string + cmd diffCmd + expected bool + }{ + { + name: "no flags", + cmd: diffCmd{}, + expected: true, + }, + { + name: "legacy explicit dry-run flag", + cmd: diffCmd{ + dryRunModeSpecified: true, + dryRunMode: "true", + }, + expected: false, + }, + { + name: "legacy empty dry-run flag", + cmd: diffCmd{ + dryRunModeSpecified: true, + dryRunMode: "", + }, + expected: false, + }, + { + name: "server-side dry-run flag", + cmd: diffCmd{ + dryRunModeSpecified: true, + dryRunMode: "server", + }, + expected: true, + }, + { + name: "client-side dry-run flag", + cmd: diffCmd{ + dryRunModeSpecified: true, + dryRunMode: "client", + }, + expected: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + actual := tc.cmd.clusterAccessAllowed() + if actual != tc.expected { + t.Errorf("Expected %v, got %v", tc.expected, actual) + } + }) + } +} diff --git a/main_test.go b/main_test.go new file mode 100644 index 00000000..bcb441b1 --- /dev/null +++ b/main_test.go @@ -0,0 +1,112 @@ +package main + +import ( + "fmt" + "os" + "reflect" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/databus23/helm-diff/v3/cmd" +) + +func TestMain(m *testing.M) { + if os.Getenv(env) == envValue { + os.Exit(runFakeHelm()) + } + + os.Exit(m.Run()) +} + +func TestHelmDiff(t *testing.T) { + os.Setenv(env, envValue) + defer os.Unsetenv(env) + + helmBin, helmBinSet := os.LookupEnv("HELM_BIN") + os.Setenv("HELM_BIN", os.Args[0]) + defer func() { + if helmBinSet { + os.Setenv("HELM_BIN", helmBin) + } else { + os.Unsetenv("HELM_BIN") + } + }() + + os.Args = []string{"helm-diff", "upgrade", "-f", "test/testdata/test-values.yaml", "test-release", "test/testdata/test-chart"} + require.NoError(t, cmd.New().Execute()) +} + +const ( + env = "BECOME_FAKE_HELM" + envValue = "1" +) + +type fakeHelmSubcmd struct { + cmd []string + args []string + stdout string + stderr string + exitCode int +} + +var helmSubcmdStubs = []fakeHelmSubcmd{ + { + cmd: []string{"version"}, + stdout: `version.BuildInfo{Version:"v3.1.0-rc.1", GitCommit:"12345", GitTreeState:"clean", GoVersion:"go1.20.12"}`, + }, + { + cmd: []string{"get", "manifest"}, + args: []string{"test-release"}, + stdout: `--- +# Source: test-chart/templates/cm.yaml +`, + }, + { + cmd: []string{"template"}, + args: []string{"test-release", "test/testdata/test-chart", "--values", "test/testdata/test-values.yaml", "--validate", "--is-upgrade"}, + }, + { + cmd: []string{"get", "hooks"}, + args: []string{"test-release"}, + }, +} + +func runFakeHelm() int { + var stub *fakeHelmSubcmd + + if len(os.Args) < 2 { + fmt.Fprintln(os.Stderr, "fake helm does not support invocations without subcommands") + return 1 + } + + cmdAndArgs := os.Args[1:] + for i := range helmSubcmdStubs { + s := helmSubcmdStubs[i] + if reflect.DeepEqual(s.cmd, cmdAndArgs[:len(s.cmd)]) { + stub = &s + break + } + } + + if stub == nil { + fmt.Fprintf(os.Stderr, "no stub for %s\n", cmdAndArgs) + return 1 + } + + want := stub.args + if want == nil { + want = []string{} + } + got := cmdAndArgs[len(stub.cmd):] + if !reflect.DeepEqual(want, got) { + fmt.Fprintf(os.Stderr, "want: %v\n", want) + fmt.Fprintf(os.Stderr, "got : %v\n", got) + fmt.Fprintf(os.Stderr, "args : %v\n", os.Args) + fmt.Fprintf(os.Stderr, "env : %v\n", os.Environ()) + return 1 + } + fmt.Fprintf(os.Stdout, "%s", stub.stdout) + fmt.Fprintf(os.Stderr, "%s", stub.stderr) + return stub.exitCode +}