Merge pull request #12871 from mattfarina/hide-secret-flag

Enabling hide secrets on install and upgrade dry run

cmd/helm/install.go

@@ -97,8 +97,8 @@ To check the generated manifests of a release without installing the chart,
 the --debug and --dry-run flags can be combined.
 
 The --dry-run flag will output all generated chart manifests, including Secrets
-which can contain sensitive values. Please carefully consider how and when this
-flag is used.
+which can contain sensitive values. To hide Kubernetes Secrets use the
+--hide-secret flag. Please carefully consider how and when these flags are used.
 
 If --verify is set, the chart MUST have a provenance file, and the provenance
 file MUST pass all verification steps.
@@ -163,6 +163,10 @@ func newInstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 	}
 
 	addInstallFlags(cmd, cmd.Flags(), client, valueOpts)
+	// hide-secret is not available in all places the install flags are used so
+	// it is added separately
+	f := cmd.Flags()
+	f.BoolVar(&client.HideSecret, "hide-secret", false, "hide Kubernetes Secrets when also using the --dry-run flag")
 	bindOutputFlag(cmd, &outfmt)
 	bindPostRenderFlag(cmd, &client.PostRenderer)
 

cmd/helm/install_test.go

@@ -252,6 +252,22 @@ func TestInstall(t *testing.T) {
 			cmd:    fmt.Sprintf("install aeneas test/reqtest --username username --password password --repository-config %s --repository-cache %s", repoFile, srv.Root()),
 			golden: "output/install.txt",
 		},
+		{
+			name:   "dry-run displaying secret",
+			cmd:    "install secrets testdata/testcharts/chart-with-secret --dry-run",
+			golden: "output/install-dry-run-with-secret.txt",
+		},
+		{
+			name:   "dry-run hiding secret",
+			cmd:    "install secrets testdata/testcharts/chart-with-secret --dry-run --hide-secret",
+			golden: "output/install-dry-run-with-secret-hidden.txt",
+		},
+		{
+			name:      "hide-secret error without dry-run",
+			cmd:       "install secrets testdata/testcharts/chart-with-secret --hide-secret",
+			wantError: true,
+			golden:    "output/install-hide-secret.txt",
+		},
 	}
 
 	runTestCmd(t, tests)

cmd/helm/testdata/output/install-dry-run-with-secret-hidden.txt

@@ -0,0 +1,20 @@
+NAME: secrets
+LAST DEPLOYED: Fri Sep  2 22:04:05 1977
+NAMESPACE: default
+STATUS: pending-install
+REVISION: 1
+TEST SUITE: None
+HOOKS:
+MANIFEST:
+---
+# Source: chart-with-secret/templates/secret.yaml
+# HIDDEN: The Secret output has been suppressed
+---
+# Source: chart-with-secret/templates/configmap.yaml
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-configmap
+data:
+  foo: bar
+

cmd/helm/testdata/output/install-dry-run-with-secret.txt

@@ -0,0 +1,25 @@
+NAME: secrets
+LAST DEPLOYED: Fri Sep  2 22:04:05 1977
+NAMESPACE: default
+STATUS: pending-install
+REVISION: 1
+TEST SUITE: None
+HOOKS:
+MANIFEST:
+---
+# Source: chart-with-secret/templates/secret.yaml
+apiVersion: v1
+kind: Secret
+metadata:
+  name: test-secret
+stringData:
+  foo: bar
+---
+# Source: chart-with-secret/templates/configmap.yaml
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-configmap
+data:
+  foo: bar
+

cmd/helm/testdata/output/install-hide-secret.txt

@@ -0,0 +1 @@
+Error: INSTALLATION FAILED: Hiding Kubernetes secrets requires a dry-run mode

cmd/helm/testdata/testcharts/chart-with-secret/Chart.yaml

@@ -0,0 +1,4 @@
+apiVersion: v2
+description: Chart with Kubernetes Secret
+name: chart-with-secret
+version: 0.0.1

cmd/helm/testdata/testcharts/chart-with-secret/templates/configmap.yaml

@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-configmap
+data:
+  foo: bar

cmd/helm/testdata/testcharts/chart-with-secret/templates/secret.yaml

@@ -0,0 +1,6 @@
+apiVersion: v1
+kind: Secret
+metadata:
+  name: test-secret
+stringData:
+  foo: bar

cmd/helm/upgrade.go

@@ -74,8 +74,8 @@ or '--set' flags. Priority is given to new values.
     $ helm upgrade --reuse-values --set foo=bar --set foo=newbar redis ./redis
 
 The --dry-run flag will output all generated chart manifests, including Secrets
-which can contain sensitive values. Please carefully consider how and when this
-flag is used.
+which can contain sensitive values. To hide Kubernetes Secrets use the
+--hide-secret flag. Please carefully consider how and when these flags are used.
 `
 
 func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
@@ -146,6 +146,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 					instClient.DependencyUpdate = client.DependencyUpdate
 					instClient.Labels = client.Labels
 					instClient.EnableDNS = client.EnableDNS
+					instClient.HideSecret = client.HideSecret
 
 					rel, err := runInstall(args, instClient, valueOpts, out)
 					if err != nil {
@@ -246,6 +247,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 	f.BoolVarP(&client.Install, "install", "i", false, "if a release by this name doesn't already exist, run an install")
 	f.BoolVar(&client.Devel, "devel", false, "use development versions, too. Equivalent to version '>0.0.0-0'. If --version is set, this is ignored")
 	f.StringVar(&client.DryRunOption, "dry-run", "", "simulate an install. If --dry-run is set with no option being specified or as '--dry-run=client', it will not attempt cluster connections. Setting '--dry-run=server' allows attempting cluster connections.")
+	f.BoolVar(&client.HideSecret, "hide-secret", false, "hide Kubernetes Secrets when also using the --dry-run flag")
 	f.Lookup("dry-run").NoOptDefVal = "client"
 	f.BoolVar(&client.Recreate, "recreate-pods", false, "performs pods restart for the resource if applicable")
 	f.MarkDeprecated("recreate-pods", "functionality will no longer be updated. Consult the documentation for other methods to recreate pods")

cmd/helm/upgrade_test.go

@@ -458,3 +458,104 @@ func TestUpgradeInstallWithLabels(t *testing.T) {
 		t.Errorf("Expected {%v}, got {%v}", expectedLabels, updatedRel.Labels)
 	}
 }
+
+func prepareMockReleaseWithSecret(releaseName string, t *testing.T) (func(n string, v int, ch *chart.Chart) *release.Release, *chart.Chart, string) {
+	tmpChart := t.TempDir()
+	configmapData, err := os.ReadFile("testdata/testcharts/chart-with-secret/templates/configmap.yaml")
+	if err != nil {
+		t.Fatalf("Error loading template yaml %v", err)
+	}
+	secretData, err := os.ReadFile("testdata/testcharts/chart-with-secret/templates/secret.yaml")
+	if err != nil {
+		t.Fatalf("Error loading template yaml %v", err)
+	}
+	cfile := &chart.Chart{
+		Metadata: &chart.Metadata{
+			APIVersion:  chart.APIVersionV1,
+			Name:        "testUpgradeChart",
+			Description: "A Helm chart for Kubernetes",
+			Version:     "0.1.0",
+		},
+		Templates: []*chart.File{{Name: "templates/configmap.yaml", Data: configmapData}, {Name: "templates/secret.yaml", Data: secretData}},
+	}
+	chartPath := filepath.Join(tmpChart, cfile.Metadata.Name)
+	if err := chartutil.SaveDir(cfile, tmpChart); err != nil {
+		t.Fatalf("Error creating chart for upgrade: %v", err)
+	}
+	ch, err := loader.Load(chartPath)
+	if err != nil {
+		t.Fatalf("Error loading chart: %v", err)
+	}
+	_ = release.Mock(&release.MockReleaseOptions{
+		Name:  releaseName,
+		Chart: ch,
+	})
+
+	relMock := func(n string, v int, ch *chart.Chart) *release.Release {
+		return release.Mock(&release.MockReleaseOptions{Name: n, Version: v, Chart: ch})
+	}
+
+	return relMock, ch, chartPath
+}
+
+func TestUpgradeWithDryRun(t *testing.T) {
+	releaseName := "funny-bunny-labels"
+	_, _, chartPath := prepareMockReleaseWithSecret(releaseName, t)
+
+	defer resetEnv()()
+
+	store := storageFixture()
+
+	// First install a release into the store so that future --dry-run attempts
+	// have it available.
+	cmd := fmt.Sprintf("upgrade %s --install '%s'", releaseName, chartPath)
+	_, _, err := executeActionCommandC(store, cmd)
+	if err != nil {
+		t.Errorf("unexpected error, got '%v'", err)
+	}
+
+	_, err = store.Get(releaseName, 1)
+	if err != nil {
+		t.Errorf("unexpected error, got '%v'", err)
+	}
+
+	cmd = fmt.Sprintf("upgrade %s --dry-run '%s'", releaseName, chartPath)
+	_, out, err := executeActionCommandC(store, cmd)
+	if err != nil {
+		t.Errorf("unexpected error, got '%v'", err)
+	}
+
+	// No second release should be stored because this is a dry run.
+	_, err = store.Get(releaseName, 2)
+	if err == nil {
+		t.Error("expected error as there should be no new release but got none")
+	}
+
+	if !strings.Contains(out, "kind: Secret") {
+		t.Error("expected secret in output from --dry-run but found none")
+	}
+
+	// Ensure the secret is not in the output
+	cmd = fmt.Sprintf("upgrade %s --dry-run --hide-secret '%s'", releaseName, chartPath)
+	_, out, err = executeActionCommandC(store, cmd)
+	if err != nil {
+		t.Errorf("unexpected error, got '%v'", err)
+	}
+
+	// No second release should be stored because this is a dry run.
+	_, err = store.Get(releaseName, 2)
+	if err == nil {
+		t.Error("expected error as there should be no new release but got none")
+	}
+
+	if strings.Contains(out, "kind: Secret") {
+		t.Error("expected no secret in output from --dry-run --hide-secret but found one")
+	}
+
+	// Ensure there is an error when --hide-secret used without dry-run
+	cmd = fmt.Sprintf("upgrade %s --hide-secret '%s'", releaseName, chartPath)
+	_, _, err = executeActionCommandC(store, cmd)
+	if err == nil {
+		t.Error("expected error when --hide-secret used without --dry-run")
+	}
+}

pkg/action/action.go

@@ -103,7 +103,7 @@ type Configuration struct {
 // TODO: As part of the refactor the duplicate code in cmd/helm/template.go should be removed
 //
 //	This code has to do with writing files to disk.
-func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Values, releaseName, outputDir string, subNotes, useReleaseName, includeCrds bool, pr postrender.PostRenderer, interactWithRemote, enableDNS bool) ([]*release.Hook, *bytes.Buffer, string, error) {
+func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Values, releaseName, outputDir string, subNotes, useReleaseName, includeCrds bool, pr postrender.PostRenderer, interactWithRemote, enableDNS, hideSecret bool) ([]*release.Hook, *bytes.Buffer, string, error) {
 	hs := []*release.Hook{}
 	b := bytes.NewBuffer(nil)
 
@@ -200,7 +200,11 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Valu
 
 	for _, m := range manifests {
 		if outputDir == "" {
-			fmt.Fprintf(b, "---\n# Source: %s\n%s\n", m.Name, m.Content)
+			if hideSecret && m.Head.Kind == "Secret" && m.Head.Version == "v1" {
+				fmt.Fprintf(b, "---\n# Source: %s\n# HIDDEN: The Secret output has been suppressed\n", m.Name)
+			} else {
+				fmt.Fprintf(b, "---\n# Source: %s\n%s\n", m.Name, m.Content)
+			}
 		} else {
 			newDir := outputDir
 			if useReleaseName {

pkg/action/action_test.go

@@ -195,6 +195,13 @@ func withSampleTemplates() chartOption {
 	}
 }
 
+func withSampleSecret() chartOption {
+	return func(opts *chartOptions) {
+		sampleSecret := &chart.File{Name: "templates/secret.yaml", Data: []byte("apiVersion: v1\nkind: Secret\n")}
+		opts.Templates = append(opts.Templates, sampleSecret)
+	}
+}
+
 func withSampleIncludingIncorrectTemplates() chartOption {
 	return func(opts *chartOptions) {
 		sampleTemplates := []*chart.File{

pkg/action/install.go

@@ -69,11 +69,14 @@ type Install struct {
 
 	ChartPathOptions
 
-	ClientOnly               bool
-	Force                    bool
-	CreateNamespace          bool
-	DryRun                   bool
-	DryRunOption             string
+	ClientOnly      bool
+	Force           bool
+	CreateNamespace bool
+	DryRun          bool
+	DryRunOption    string
+	// HideSecret can be set to true when DryRun is enabled in order to hide
+	// Kubernetes Secrets in the output. It cannot be used outside of DryRun.
+	HideSecret               bool
 	DisableHooks             bool
 	Replace                  bool
 	Wait                     bool
@@ -230,6 +233,11 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
 		}
 	}
 
+	// HideSecret must be used with dry run. Otherwise, return an error.
+	if !i.isDryRun() && i.HideSecret {
+		return nil, errors.New("Hiding Kubernetes secrets requires a dry-run mode")
+	}
+
 	if err := i.availableName(); err != nil {
 		return nil, err
 	}
@@ -301,7 +309,7 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma
 	rel := i.createRelease(chrt, vals, i.Labels)
 
 	var manifestDoc *bytes.Buffer
-	rel.Hooks, manifestDoc, rel.Info.Notes, err = i.cfg.renderResources(chrt, valuesToRender, i.ReleaseName, i.OutputDir, i.SubNotes, i.UseReleaseName, i.IncludeCRDs, i.PostRenderer, interactWithRemote, i.EnableDNS)
+	rel.Hooks, manifestDoc, rel.Info.Notes, err = i.cfg.renderResources(chrt, valuesToRender, i.ReleaseName, i.OutputDir, i.SubNotes, i.UseReleaseName, i.IncludeCRDs, i.PostRenderer, interactWithRemote, i.EnableDNS, i.HideSecret)
 	// Even for errors, attach this if available
 	if manifestDoc != nil {
 		rel.Manifest = manifestDoc.String()

pkg/action/install_test.go

@@ -255,6 +255,46 @@ func TestInstallRelease_DryRun(t *testing.T) {
 	is.Equal(res.Info.Description, "Dry run complete")
 }
 
+func TestInstallRelease_DryRunHiddenSecret(t *testing.T) {
+	is := assert.New(t)
+	instAction := installAction(t)
+
+	// First perform a normal dry-run with the secret and confirm its presence.
+	instAction.DryRun = true
+	vals := map[string]interface{}{}
+	res, err := instAction.Run(buildChart(withSampleSecret(), withSampleTemplates()), vals)
+	if err != nil {
+		t.Fatalf("Failed install: %s", err)
+	}
+	is.Contains(res.Manifest, "---\n# Source: hello/templates/secret.yaml\napiVersion: v1\nkind: Secret")
+
+	_, err = instAction.cfg.Releases.Get(res.Name, res.Version)
+	is.Error(err)
+	is.Equal(res.Info.Description, "Dry run complete")
+
+	// Perform a dry-run where the secret should not be present
+	instAction.HideSecret = true
+	vals = map[string]interface{}{}
+	res2, err := instAction.Run(buildChart(withSampleSecret(), withSampleTemplates()), vals)
+	if err != nil {
+		t.Fatalf("Failed install: %s", err)
+	}
+
+	is.NotContains(res2.Manifest, "---\n# Source: hello/templates/secret.yaml\napiVersion: v1\nkind: Secret")
+
+	_, err = instAction.cfg.Releases.Get(res2.Name, res2.Version)
+	is.Error(err)
+	is.Equal(res2.Info.Description, "Dry run complete")
+
+	// Ensure there is an error when HideSecret True but not in a dry-run mode
+	instAction.DryRun = false
+	vals = map[string]interface{}{}
+	_, err = instAction.Run(buildChart(withSampleSecret(), withSampleTemplates()), vals)
+	if err == nil {
+		t.Fatalf("Did not get expected an error when dry-run false and hide secret is true")
+	}
+}
+
 // Regression test for #7955
 func TestInstallRelease_DryRun_Lookup(t *testing.T) {
 	is := assert.New(t)

pkg/action/upgrade.go

@@ -74,6 +74,9 @@ type Upgrade struct {
 	DryRun bool
 	// DryRunOption controls whether the operation is prepared, but not executed with options on whether or not to interact with the remote cluster.
 	DryRunOption string
+	// HideSecret can be set to true when DryRun is enabled in order to hide
+	// Kubernetes Secrets in the output. It cannot be used outside of DryRun.
+	HideSecret bool
 	// Force will, if set to `true`, ignore certain warnings and perform the upgrade anyway.
 	//
 	// This should be used with caution.
@@ -191,6 +194,11 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin
 		return nil, nil, errMissingChart
 	}
 
+	// HideSecret must be used with dry run. Otherwise, return an error.
+	if !u.isDryRun() && u.HideSecret {
+		return nil, nil, errors.New("Hiding Kubernetes secrets requires a dry-run mode")
+	}
+
 	// finds the last non-deleted release with the given name
 	lastRelease, err := u.cfg.Releases.Last(name)
 	if err != nil {
@@ -259,7 +267,7 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin
 		interactWithRemote = true
 	}
 
-	hooks, manifestDoc, notesTxt, err := u.cfg.renderResources(chart, valuesToRender, "", "", u.SubNotes, false, false, u.PostRenderer, interactWithRemote, u.EnableDNS)
+	hooks, manifestDoc, notesTxt, err := u.cfg.renderResources(chart, valuesToRender, "", "", u.SubNotes, false, false, u.PostRenderer, interactWithRemote, u.EnableDNS, u.HideSecret)
 	if err != nil {
 		return nil, nil, err
 	}

pkg/action/upgrade_test.go

@@ -535,3 +535,54 @@ func TestUpgradeRelease_SystemLabels(t *testing.T) {
 
 	is.Equal(fmt.Errorf("user suplied labels contains system reserved label name. System labels: %+v", driver.GetSystemLabels()), err)
 }
+
+func TestUpgradeRelease_DryRun(t *testing.T) {
+	is := assert.New(t)
+	req := require.New(t)
+
+	upAction := upgradeAction(t)
+	rel := releaseStub()
+	rel.Name = "previous-release"
+	rel.Info.Status = release.StatusDeployed
+	req.NoError(upAction.cfg.Releases.Create(rel))
+
+	upAction.DryRun = true
+	vals := map[string]interface{}{}
+
+	ctx, done := context.WithCancel(context.Background())
+	res, err := upAction.RunWithContext(ctx, rel.Name, buildChart(withSampleSecret()), vals)
+	done()
+	req.NoError(err)
+	is.Equal(release.StatusPendingUpgrade, res.Info.Status)
+	is.Contains(res.Manifest, "kind: Secret")
+
+	lastRelease, err := upAction.cfg.Releases.Last(rel.Name)
+	req.NoError(err)
+	is.Equal(lastRelease.Info.Status, release.StatusDeployed)
+	is.Equal(1, lastRelease.Version)
+
+	// Test the case for hiding the secret to ensure it is not displayed
+	upAction.HideSecret = true
+	vals = map[string]interface{}{}
+
+	ctx, done = context.WithCancel(context.Background())
+	res, err = upAction.RunWithContext(ctx, rel.Name, buildChart(withSampleSecret()), vals)
+	done()
+	req.NoError(err)
+	is.Equal(release.StatusPendingUpgrade, res.Info.Status)
+	is.NotContains(res.Manifest, "kind: Secret")
+
+	lastRelease, err = upAction.cfg.Releases.Last(rel.Name)
+	req.NoError(err)
+	is.Equal(lastRelease.Info.Status, release.StatusDeployed)
+	is.Equal(1, lastRelease.Version)
+
+	// Ensure in a dry run mode when using HideSecret
+	upAction.DryRun = false
+	vals = map[string]interface{}{}
+
+	ctx, done = context.WithCancel(context.Background())
+	_, err = upAction.RunWithContext(ctx, rel.Name, buildChart(withSampleSecret()), vals)
+	done()
+	req.Error(err)
+}