diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 1942471bf..c467cf724 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -39,7 +39,7 @@ jobs: # Initializes the CodeQL tools for scanning. - name: Initialize CodeQL - uses: github/codeql-action/init@0b21cf2492b6b02c465a3e5d7c473717ad7721ba # pinv3.23.1 + uses: github/codeql-action/init@3ab4101902695724f9365a384f86c1074d94e18c # pinv3.24.7 with: languages: ${{ matrix.language }} # If you wish to specify custom queries, you can do so here or in a config file. @@ -50,7 +50,7 @@ jobs: # Autobuild attempts to build any compiled languages (C/C++, C#, or Java). # If this step fails, then you should remove it and run the build manually (see below) - name: Autobuild - uses: github/codeql-action/autobuild@0b21cf2492b6b02c465a3e5d7c473717ad7721ba # pinv3.23.1 + uses: github/codeql-action/autobuild@3ab4101902695724f9365a384f86c1074d94e18c # pinv3.24.7 # ℹī¸ Command-line programs to run using the OS shell. # 📚 https://git.io/JvXDl @@ -64,4 +64,4 @@ jobs: # make release - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@0b21cf2492b6b02c465a3e5d7c473717ad7721ba # pinv3.23.1 + uses: github/codeql-action/analyze@3ab4101902695724f9365a384f86c1074d94e18c # pinv3.24.7 diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml index dcc982dc9..bedc0937a 100644 --- a/.github/workflows/golangci-lint.yml +++ b/.github/workflows/golangci-lint.yml @@ -13,10 +13,10 @@ jobs: uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # pin@v4.1.1 - name: Setup Go - uses: actions/setup-go@93397bea11091df50f3d7e59dc26a7711a8bcfbe # pin@4.1.0 + uses: actions/setup-go@0c52d547c9bc32b1aa3301fd7a9cb496313a4491 # pin@5.0.0 with: go-version: "1.21" - name: golangci-lint - uses: golangci/golangci-lint-action@3a919529898de77ec3da873e3063ca4b10e7f5cc #pin@3.7.0 + uses: golangci/golangci-lint-action@3cfe3a4abbb849e10058ce4af15d205b6da42804 #pin@4.0.0 with: version: v1.55 diff --git a/.gitignore b/.gitignore index ef9806616..cdb68ceb5 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,7 @@ .idea/ .vimrc .vscode/ +.devcontainer/ _dist/ _dist_versions/ bin/ diff --git a/cmd/helm/install.go b/cmd/helm/install.go index 4274a1f2c..fe5354ac6 100644 --- a/cmd/helm/install.go +++ b/cmd/helm/install.go @@ -94,7 +94,11 @@ And in the following example, 'foo' is set to '{"key1":"value1","key2":"bar"}': $ helm install --set-json='foo={"key1":"value1","key2":"value2"}' --set-json='foo.key2="bar"' myredis ./redis To check the generated manifests of a release without installing the chart, -the '--debug' and '--dry-run' flags can be combined. +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. 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. @@ -159,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, &client.PostRendererHooks) diff --git a/cmd/helm/install_test.go b/cmd/helm/install_test.go index b34d1455c..6da3df0f9 100644 --- a/cmd/helm/install_test.go +++ b/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) diff --git a/cmd/helm/testdata/output/install-dry-run-with-secret-hidden.txt b/cmd/helm/testdata/output/install-dry-run-with-secret-hidden.txt new file mode 100644 index 000000000..2f928c3dc --- /dev/null +++ b/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 + diff --git a/cmd/helm/testdata/output/install-dry-run-with-secret.txt b/cmd/helm/testdata/output/install-dry-run-with-secret.txt new file mode 100644 index 000000000..6c094ef6a --- /dev/null +++ b/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 + diff --git a/cmd/helm/testdata/output/install-hide-secret.txt b/cmd/helm/testdata/output/install-hide-secret.txt new file mode 100644 index 000000000..aaf73b478 --- /dev/null +++ b/cmd/helm/testdata/output/install-hide-secret.txt @@ -0,0 +1 @@ +Error: INSTALLATION FAILED: Hiding Kubernetes secrets requires a dry-run mode diff --git a/cmd/helm/testdata/testcharts/chart-with-secret/Chart.yaml b/cmd/helm/testdata/testcharts/chart-with-secret/Chart.yaml new file mode 100644 index 000000000..46d069e1c --- /dev/null +++ b/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 diff --git a/cmd/helm/testdata/testcharts/chart-with-secret/templates/configmap.yaml b/cmd/helm/testdata/testcharts/chart-with-secret/templates/configmap.yaml new file mode 100644 index 000000000..ce9c27d56 --- /dev/null +++ b/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 diff --git a/cmd/helm/testdata/testcharts/chart-with-secret/templates/secret.yaml b/cmd/helm/testdata/testcharts/chart-with-secret/templates/secret.yaml new file mode 100644 index 000000000..b1e1cff56 --- /dev/null +++ b/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 diff --git a/cmd/helm/upgrade.go b/cmd/helm/upgrade.go index 7c15b8baa..eab7a6d0e 100644 --- a/cmd/helm/upgrade.go +++ b/cmd/helm/upgrade.go @@ -72,6 +72,10 @@ parameters, and existing values will be merged with any values set via '--values 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. 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 { @@ -143,6 +147,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 { @@ -243,6 +248,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") diff --git a/cmd/helm/upgrade_test.go b/cmd/helm/upgrade_test.go index 485267d1d..64cd6e417 100644 --- a/cmd/helm/upgrade_test.go +++ b/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") + } +} diff --git a/go.mod b/go.mod index e200d4fcb..2270faabb 100644 --- a/go.mod +++ b/go.mod @@ -34,7 +34,7 @@ require ( github.com/stretchr/testify v1.8.4 github.com/xeipuuv/gojsonschema v1.2.0 golang.org/x/crypto v0.17.0 - golang.org/x/term v0.15.0 + golang.org/x/term v0.18.0 golang.org/x/text v0.14.0 k8s.io/api v0.29.0 k8s.io/apiextensions-apiserver v0.29.0 @@ -150,7 +150,7 @@ require ( golang.org/x/net v0.17.0 // indirect golang.org/x/oauth2 v0.10.0 // indirect golang.org/x/sync v0.3.0 // indirect - golang.org/x/sys v0.15.0 // indirect + golang.org/x/sys v0.18.0 // indirect golang.org/x/time v0.3.0 // indirect google.golang.org/appengine v1.6.7 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20230822172742-b8732ec3820d // indirect diff --git a/go.sum b/go.sum index 2799262df..b3941a045 100644 --- a/go.sum +++ b/go.sum @@ -483,14 +483,14 @@ golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.2.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.15.0 h1:h48lPFYpsTvQJZF4EKyI4aLHaev3CxivZmv7yZig9pc= -golang.org/x/sys v0.15.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/sys v0.18.0 h1:DBdB3niSjOA/O0blCZBqDefyWNYveAYMNF1Wum0DYQ4= +golang.org/x/sys v0.18.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.0.0-20220526004731-065cf7ba2467/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.2.0/go.mod h1:TVmDHMZPmdnySmBfhjOoOdhjzdE1h4u1VwSiw2l1Nuc= -golang.org/x/term v0.15.0 h1:y/Oo/a/q3IXu26lQgl04j/gjuBDOBlx7X6Om1j2CPW4= -golang.org/x/term v0.15.0/go.mod h1:BDl952bC7+uMoWR75FIrCDx79TPU9oHkTZ9yRbYOrX0= +golang.org/x/term v0.18.0 h1:FcHjZXDMxI8mM3nwhX9HlKop4C0YQvCVCdwYl2wOtE8= +golang.org/x/term v0.18.0/go.mod h1:ILwASektA3OnRv7amZ1xhE/KTR+u50pbXfZ03+6Nx58= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= diff --git a/pkg/action/action.go b/pkg/action/action.go index fb2226df0..fadb9162d 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -114,7 +114,8 @@ func (cfg *Configuration) renderResources( mainPostRenderer postrender.PostRenderer, hooksPostRenderer postrender.PostRenderer, interactWithRemote, - enableDNS bool, + enableDNS, + hideSecret bool, ) ([]*release.Hook, *bytes.Buffer, string, error) { hs := []*release.Hook{} b := bytes.NewBuffer(nil) @@ -212,7 +213,11 @@ func (cfg *Configuration) renderResources( 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 { diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index c4ef6c056..fdcfa7558 100644 --- a/pkg/action/action_test.go +++ b/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{ diff --git a/pkg/action/install.go b/pkg/action/install.go index c527b30db..dc1bbcebf 100644 --- a/pkg/action/install.go +++ b/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 @@ -231,6 +234,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 } @@ -314,6 +322,7 @@ func (i *Install) RunWithContext(ctx context.Context, chrt *chart.Chart, vals ma i.PostRendererHooks, interactWithRemote, i.EnableDNS, + i.HideSecret, ) // Even for errors, attach this if available if manifestDoc != nil { diff --git a/pkg/action/install_test.go b/pkg/action/install_test.go index b3ad2118c..4c54cd686 100644 --- a/pkg/action/install_test.go +++ b/pkg/action/install_test.go @@ -256,6 +256,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) diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 42db0aff8..0688fbb5c 100644 --- a/pkg/action/upgrade.go +++ b/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. @@ -193,6 +196,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 { @@ -273,6 +281,7 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[strin u.PostRendererHooks, interactWithRemote, u.EnableDNS, + u.HideSecret, ) if err != nil { return nil, nil, err diff --git a/pkg/action/upgrade_test.go b/pkg/action/upgrade_test.go index e259605ce..78b4347e3 100644 --- a/pkg/action/upgrade_test.go +++ b/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) +} diff --git a/pkg/chart/metadata.go b/pkg/chart/metadata.go index 97bfc2c0c..a08a97cd1 100644 --- a/pkg/chart/metadata.go +++ b/pkg/chart/metadata.go @@ -16,6 +16,7 @@ limitations under the License. package chart import ( + "path/filepath" "strings" "unicode" @@ -110,6 +111,11 @@ func (md *Metadata) Validate() error { if md.Name == "" { return ValidationError("chart.metadata.name is required") } + + if md.Name != filepath.Base(md.Name) { + return ValidationErrorf("chart.metadata.name %q is invalid", md.Name) + } + if md.Version == "" { return ValidationError("chart.metadata.version is required") } diff --git a/pkg/chart/metadata_test.go b/pkg/chart/metadata_test.go index c8b89ae55..62aea7261 100644 --- a/pkg/chart/metadata_test.go +++ b/pkg/chart/metadata_test.go @@ -40,6 +40,11 @@ func TestValidate(t *testing.T) { &Metadata{APIVersion: "v2", Version: "1.0"}, ValidationError("chart.metadata.name is required"), }, + { + "chart without name", + &Metadata{Name: "../../test", APIVersion: "v2", Version: "1.0"}, + ValidationError("chart.metadata.name \"../../test\" is invalid"), + }, { "chart without version", &Metadata{Name: "test", APIVersion: "v2"}, diff --git a/pkg/chartutil/errors.go b/pkg/chartutil/errors.go index fcdcc27ea..0a4046d2e 100644 --- a/pkg/chartutil/errors.go +++ b/pkg/chartutil/errors.go @@ -33,3 +33,11 @@ type ErrNoValue struct { } func (e ErrNoValue) Error() string { return fmt.Sprintf("%q is not a value", e.Key) } + +type ErrInvalidChartName struct { + Name string +} + +func (e ErrInvalidChartName) Error() string { + return fmt.Sprintf("%q is not a valid chart name", e.Name) +} diff --git a/pkg/chartutil/save.go b/pkg/chartutil/save.go index 2ce4eddaf..4ee90709c 100644 --- a/pkg/chartutil/save.go +++ b/pkg/chartutil/save.go @@ -39,6 +39,10 @@ var headerBytes = []byte("+aHR0cHM6Ly95b3V0dS5iZS96OVV6MWljandyTQo=") // directory, writing the chart's contents to that subdirectory. func SaveDir(c *chart.Chart, dest string) error { // Create the chart directory + err := validateName(c.Name()) + if err != nil { + return err + } outdir := filepath.Join(dest, c.Name()) if fi, err := os.Stat(outdir); err == nil && !fi.IsDir() { return errors.Errorf("file %s already exists and is not a directory", outdir) @@ -149,6 +153,10 @@ func Save(c *chart.Chart, outDir string) (string, error) { } func writeTarContents(out *tar.Writer, c *chart.Chart, prefix string) error { + err := validateName(c.Name()) + if err != nil { + return err + } base := filepath.Join(prefix, c.Name()) // Pull out the dependencies of a v1 Chart, since there's no way @@ -242,3 +250,15 @@ func writeToTar(out *tar.Writer, name string, body []byte) error { _, err := out.Write(body) return err } + +// If the name has directory name has characters which would change the location +// they need to be removed. +func validateName(name string) error { + nname := filepath.Base(name) + + if nname != name { + return ErrInvalidChartName{name} + } + + return nil +} diff --git a/pkg/chartutil/save_test.go b/pkg/chartutil/save_test.go index db485c7cb..98b4e641b 100644 --- a/pkg/chartutil/save_test.go +++ b/pkg/chartutil/save_test.go @@ -106,6 +106,24 @@ func TestSave(t *testing.T) { } }) } + + c := &chart.Chart{ + Metadata: &chart.Metadata{ + APIVersion: chart.APIVersionV1, + Name: "../ahab", + Version: "1.2.3", + }, + Lock: &chart.Lock{ + Digest: "testdigest", + }, + Files: []*chart.File{ + {Name: "scheherazade/shahryar.txt", Data: []byte("1,001 Nights")}, + }, + } + _, err := Save(c, tmp) + if err == nil { + t.Fatal("Expected error saving chart with invalid name") + } } // Creates a copy with a different schema; does not modify anything. @@ -232,4 +250,15 @@ func TestSaveDir(t *testing.T) { if len(c2.Files) != 1 || c2.Files[0].Name != c.Files[0].Name { t.Fatal("Files data did not match") } + + tmp2 := t.TempDir() + c.Metadata.Name = "../ahab" + pth := filepath.Join(tmp2, "tmpcharts") + if err := os.MkdirAll(filepath.Join(pth), 0755); err != nil { + t.Fatal(err) + } + + if err := SaveDir(c, pth); err.Error() != "\"../ahab\" is not a valid chart name" { + t.Fatalf("Did not get expected error for chart named %q", c.Name()) + } } diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index f7ab1a568..db2487d16 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -262,6 +262,32 @@ func TestDownloadAll(t *testing.T) { if _, err := os.Stat(filepath.Join(chartPath, "charts", "signtest-0.1.0.tgz")); os.IsNotExist(err) { t.Error(err) } + + // A chart with a bad name like this cannot be loaded and saved. Handling in + // the loading and saving will return an error about the invalid name. In + // this case, the chart needs to be created directly. + badchartyaml := `apiVersion: v2 +description: A Helm chart for Kubernetes +name: ../bad-local-subchart +version: 0.1.0` + if err := os.MkdirAll(filepath.Join(chartPath, "testdata", "bad-local-subchart"), 0755); err != nil { + t.Fatal(err) + } + err = os.WriteFile(filepath.Join(chartPath, "testdata", "bad-local-subchart", "Chart.yaml"), []byte(badchartyaml), 0644) + if err != nil { + t.Fatal(err) + } + + badLocalDep := &chart.Dependency{ + Name: "../bad-local-subchart", + Repository: "file://./testdata/bad-local-subchart", + Version: "0.1.0", + } + + err = m.downloadAll([]*chart.Dependency{badLocalDep}) + if err == nil { + t.Fatal("Expected error for bad dependency name") + } } func TestUpdateBeforeBuild(t *testing.T) { diff --git a/pkg/getter/ocigetter.go b/pkg/getter/ocigetter.go index 209786bd7..0547cdcbb 100644 --- a/pkg/getter/ocigetter.go +++ b/pkg/getter/ocigetter.go @@ -119,6 +119,7 @@ func (g *OCIGetter) newRegistryClient() (*registry.Client, error) { IdleConnTimeout: 90 * time.Second, TLSHandshakeTimeout: 10 * time.Second, ExpectContinueTimeout: 1 * time.Second, + Proxy: http.ProxyFromEnvironment, } }) diff --git a/pkg/kube/wait.go b/pkg/kube/wait.go index ecdd38940..6b747d551 100644 --- a/pkg/kube/wait.go +++ b/pkg/kube/wait.go @@ -19,6 +19,7 @@ package kube // import "helm.sh/helm/v3/pkg/kube" import ( "context" "fmt" + "net/http" "time" "github.com/pkg/errors" @@ -32,6 +33,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/cli-runtime/pkg/resource" "k8s.io/apimachinery/pkg/util/wait" ) @@ -50,10 +52,27 @@ func (w *waiter) waitForResources(created ResourceList) error { ctx, cancel := context.WithTimeout(context.Background(), w.timeout) defer cancel() + numberOfErrors := make([]int, len(created)) + for i := range numberOfErrors { + numberOfErrors[i] = 0 + } + return wait.PollUntilContextCancel(ctx, 2*time.Second, true, func(ctx context.Context) (bool, error) { - for _, v := range created { + waitRetries := 30 + for i, v := range created { ready, err := w.c.IsReady(ctx, v) - if !ready || err != nil { + + if waitRetries > 0 && w.isRetryableError(err, v) { + numberOfErrors[i]++ + if numberOfErrors[i] > waitRetries { + w.log("Max number of retries reached") + return false, err + } + w.log("Retrying as current number of retries %d less than max number of retries %d", numberOfErrors[i]-1, waitRetries) + return false, nil + } + numberOfErrors[i] = 0 + if !ready { return false, err } } @@ -61,6 +80,25 @@ func (w *waiter) waitForResources(created ResourceList) error { }) } +func (w *waiter) isRetryableError(err error, resource *resource.Info) bool { + if err == nil { + return false + } + w.log("Error received when checking status of resource %s. Error: '%s', Resource details: '%s'", resource.Name, err, resource) + if ev, ok := err.(*apierrors.StatusError); ok { + statusCode := ev.Status().Code + retryable := w.isRetryableHTTPStatusCode(statusCode) + w.log("Status code received: %d. Retryable error? %t", statusCode, retryable) + return retryable + } + w.log("Retryable error? %t", true) + return true +} + +func (w *waiter) isRetryableHTTPStatusCode(httpStatusCode int32) bool { + return httpStatusCode == 0 || httpStatusCode == http.StatusTooManyRequests || (httpStatusCode >= 500 && httpStatusCode != http.StatusNotImplemented) +} + // waitForDeletedResources polls to check if all the resources are deleted or a timeout is reached func (w *waiter) waitForDeletedResources(deleted ResourceList) error { w.log("beginning wait for %d resources to be deleted with timeout of %v", len(deleted), w.timeout) diff --git a/pkg/lint/rules/chartfile.go b/pkg/lint/rules/chartfile.go index 70532ad4f..910602b7d 100644 --- a/pkg/lint/rules/chartfile.go +++ b/pkg/lint/rules/chartfile.go @@ -106,6 +106,10 @@ func validateChartName(cf *chart.Metadata) error { if cf.Name == "" { return errors.New("name is required") } + name := filepath.Base(cf.Name) + if name != cf.Name { + return fmt.Errorf("chart name %q is invalid", cf.Name) + } return nil } diff --git a/pkg/lint/rules/chartfile_test.go b/pkg/lint/rules/chartfile_test.go index 087cda047..a06d7dc31 100644 --- a/pkg/lint/rules/chartfile_test.go +++ b/pkg/lint/rules/chartfile_test.go @@ -30,16 +30,19 @@ import ( ) const ( + badCharNametDir = "testdata/badchartname" badChartDir = "testdata/badchartfile" anotherBadChartDir = "testdata/anotherbadchartfile" ) var ( + badChartNamePath = filepath.Join(badCharNametDir, "Chart.yaml") badChartFilePath = filepath.Join(badChartDir, "Chart.yaml") nonExistingChartFilePath = filepath.Join(os.TempDir(), "Chart.yaml") ) var badChart, _ = chartutil.LoadChartfile(badChartFilePath) +var badChartName, _ = chartutil.LoadChartfile(badChartNamePath) // Validation functions Test func TestValidateChartYamlNotDirectory(t *testing.T) { @@ -69,6 +72,11 @@ func TestValidateChartName(t *testing.T) { if err == nil { t.Errorf("validateChartName to return a linter error, got no error") } + + err = validateChartName(badChartName) + if err == nil { + t.Error("expected validateChartName to return a linter error for an invalid name, got no error") + } } func TestValidateChartVersion(t *testing.T) { diff --git a/pkg/lint/rules/testdata/badchartname/Chart.yaml b/pkg/lint/rules/testdata/badchartname/Chart.yaml new file mode 100644 index 000000000..64f8fb8bf --- /dev/null +++ b/pkg/lint/rules/testdata/badchartname/Chart.yaml @@ -0,0 +1,5 @@ +apiVersion: v2 +description: A Helm chart for Kubernetes +version: 0.1.0 +name: "../badchartname" +type: application diff --git a/pkg/lint/rules/testdata/badchartname/values.yaml b/pkg/lint/rules/testdata/badchartname/values.yaml new file mode 100644 index 000000000..9f367033b --- /dev/null +++ b/pkg/lint/rules/testdata/badchartname/values.yaml @@ -0,0 +1 @@ +# Default values for badchartfile. diff --git a/pkg/plugin/plugin.go b/pkg/plugin/plugin.go index fb3bc5215..5bb743481 100644 --- a/pkg/plugin/plugin.go +++ b/pkg/plugin/plugin.go @@ -175,6 +175,10 @@ var validPluginName = regexp.MustCompile("^[A-Za-z0-9_-]+$") // validatePluginData validates a plugin's YAML data. func validatePluginData(plug *Plugin, filepath string) error { + // When metadata section missing, initialize with no data + if plug.Metadata == nil { + plug.Metadata = &Metadata{} + } if !validPluginName.MatchString(plug.Metadata.Name) { return fmt.Errorf("invalid plugin name at %q", filepath) } diff --git a/pkg/plugin/plugin_test.go b/pkg/plugin/plugin_test.go index 1ec2d5304..725052346 100644 --- a/pkg/plugin/plugin_test.go +++ b/pkg/plugin/plugin_test.go @@ -353,6 +353,11 @@ func TestSetupEnvWithSpace(t *testing.T) { } func TestValidatePluginData(t *testing.T) { + // A mock plugin missing any metadata. + mockMissingMeta := &Plugin{ + Dir: "no-such-dir", + } + for i, item := range []struct { pass bool plug *Plugin @@ -363,6 +368,7 @@ func TestValidatePluginData(t *testing.T) { {false, mockPlugin("$foo -bar")}, // Test leading chars {false, mockPlugin("foo -bar ")}, // Test trailing chars {false, mockPlugin("foo\nbar")}, // Test newline + {false, mockMissingMeta}, // Test if the metadata section missing } { err := validatePluginData(item.plug, fmt.Sprintf("test-%d", i)) if item.pass && err != nil { diff --git a/pkg/pusher/ocipusher.go b/pkg/pusher/ocipusher.go index 94154d389..b37a0c605 100644 --- a/pkg/pusher/ocipusher.go +++ b/pkg/pusher/ocipusher.go @@ -29,6 +29,7 @@ import ( "helm.sh/helm/v3/internal/tlsutil" "helm.sh/helm/v3/pkg/chart/loader" "helm.sh/helm/v3/pkg/registry" + "helm.sh/helm/v3/pkg/time/ctime" ) // OCIPusher is the default OCI backend handler @@ -89,6 +90,9 @@ func (pusher *OCIPusher) push(chartRef, href string) error { path.Join(strings.TrimPrefix(href, fmt.Sprintf("%s://", registry.OCIScheme)), meta.Metadata.Name), meta.Metadata.Version) + chartCreationTime := ctime.Created(stat) + pushOpts = append(pushOpts, registry.PushOptCreationTime(chartCreationTime.Format(time.RFC3339))) + _, err = client.Push(chartBytes, ref, pushOpts...) return err } diff --git a/pkg/registry/client.go b/pkg/registry/client.go index 7538cf69b..a287afaf7 100644 --- a/pkg/registry/client.go +++ b/pkg/registry/client.go @@ -527,9 +527,9 @@ type ( } pushOperation struct { - provData []byte - strictMode bool - test bool + provData []byte + strictMode bool + creationTime string } ) @@ -583,7 +583,7 @@ func (c *Client) Push(data []byte, ref string, options ...PushOption) (*PushResu descriptors = append(descriptors, provDescriptor) } - ociAnnotations := generateOCIAnnotations(meta, operation.test) + ociAnnotations := generateOCIAnnotations(meta, operation.creationTime) manifestData, manifest, err := content.GenerateManifest(&configDescriptor, ociAnnotations, descriptors...) if err != nil { @@ -652,10 +652,10 @@ func PushOptStrictMode(strictMode bool) PushOption { } } -// PushOptTest returns a function that sets whether test setting on push -func PushOptTest(test bool) PushOption { +// PushOptCreationDate returns a function that sets the creation time +func PushOptCreationTime(creationTime string) PushOption { return func(operation *pushOperation) { - operation.test = test + operation.creationTime = creationTime } } diff --git a/pkg/registry/util.go b/pkg/registry/util.go index 8baf0852a..4bff39495 100644 --- a/pkg/registry/util.go +++ b/pkg/registry/util.go @@ -166,10 +166,10 @@ func NewRegistryClientWithTLS(out io.Writer, certFile, keyFile, caFile string, i } // generateOCIAnnotations will generate OCI annotations to include within the OCI manifest -func generateOCIAnnotations(meta *chart.Metadata, test bool) map[string]string { +func generateOCIAnnotations(meta *chart.Metadata, creationTime string) map[string]string { // Get annotations from Chart attributes - ociAnnotations := generateChartOCIAnnotations(meta, test) + ociAnnotations := generateChartOCIAnnotations(meta, creationTime) // Copy Chart annotations annotations: @@ -190,7 +190,7 @@ annotations: } // getChartOCIAnnotations will generate OCI annotations from the provided chart -func generateChartOCIAnnotations(meta *chart.Metadata, test bool) map[string]string { +func generateChartOCIAnnotations(meta *chart.Metadata, creationTime string) map[string]string { chartOCIAnnotations := map[string]string{} chartOCIAnnotations = addToMap(chartOCIAnnotations, ocispec.AnnotationDescription, meta.Description) @@ -198,10 +198,12 @@ func generateChartOCIAnnotations(meta *chart.Metadata, test bool) map[string]str chartOCIAnnotations = addToMap(chartOCIAnnotations, ocispec.AnnotationVersion, meta.Version) chartOCIAnnotations = addToMap(chartOCIAnnotations, ocispec.AnnotationURL, meta.Home) - if !test { - chartOCIAnnotations = addToMap(chartOCIAnnotations, ocispec.AnnotationCreated, helmtime.Now().UTC().Format(time.RFC3339)) + if len(creationTime) == 0 { + creationTime = helmtime.Now().UTC().Format(time.RFC3339) } + chartOCIAnnotations = addToMap(chartOCIAnnotations, ocispec.AnnotationCreated, creationTime) + if len(meta.Sources) > 0 { chartOCIAnnotations = addToMap(chartOCIAnnotations, ocispec.AnnotationSource, meta.Sources[0]) } diff --git a/pkg/registry/util_test.go b/pkg/registry/util_test.go index fdf09360b..908ea4950 100644 --- a/pkg/registry/util_test.go +++ b/pkg/registry/util_test.go @@ -29,6 +29,8 @@ import ( func TestGenerateOCIChartAnnotations(t *testing.T) { + nowString := helmtime.Now().Format(time.RFC3339) + tests := []struct { name string chart *chart.Metadata @@ -43,6 +45,7 @@ func TestGenerateOCIChartAnnotations(t *testing.T) { map[string]string{ "org.opencontainers.image.title": "oci", "org.opencontainers.image.version": "0.0.1", + "org.opencontainers.image.created": nowString, }, }, { @@ -56,6 +59,7 @@ func TestGenerateOCIChartAnnotations(t *testing.T) { map[string]string{ "org.opencontainers.image.title": "oci", "org.opencontainers.image.version": "0.0.1", + "org.opencontainers.image.created": nowString, "org.opencontainers.image.description": "OCI Helm Chart", "org.opencontainers.image.url": "https://helm.sh", }, @@ -76,6 +80,7 @@ func TestGenerateOCIChartAnnotations(t *testing.T) { map[string]string{ "org.opencontainers.image.title": "oci", "org.opencontainers.image.version": "0.0.1", + "org.opencontainers.image.created": nowString, "org.opencontainers.image.description": "OCI Helm Chart", "org.opencontainers.image.url": "https://helm.sh", "org.opencontainers.image.authors": "John Snow", @@ -95,6 +100,7 @@ func TestGenerateOCIChartAnnotations(t *testing.T) { map[string]string{ "org.opencontainers.image.title": "oci", "org.opencontainers.image.version": "0.0.1", + "org.opencontainers.image.created": nowString, "org.opencontainers.image.description": "OCI Helm Chart", "org.opencontainers.image.url": "https://helm.sh", "org.opencontainers.image.authors": "John Snow (john@winterfell.com)", @@ -115,6 +121,7 @@ func TestGenerateOCIChartAnnotations(t *testing.T) { map[string]string{ "org.opencontainers.image.title": "oci", "org.opencontainers.image.version": "0.0.1", + "org.opencontainers.image.created": nowString, "org.opencontainers.image.description": "OCI Helm Chart", "org.opencontainers.image.url": "https://helm.sh", "org.opencontainers.image.authors": "John Snow (john@winterfell.com), Jane Snow", @@ -133,6 +140,7 @@ func TestGenerateOCIChartAnnotations(t *testing.T) { map[string]string{ "org.opencontainers.image.title": "oci", "org.opencontainers.image.version": "0.0.1", + "org.opencontainers.image.created": nowString, "org.opencontainers.image.description": "OCI Helm Chart", "org.opencontainers.image.source": "https://github.com/helm/helm", }, @@ -141,7 +149,7 @@ func TestGenerateOCIChartAnnotations(t *testing.T) { for _, tt := range tests { - result := generateChartOCIAnnotations(tt.chart, true) + result := generateChartOCIAnnotations(tt.chart, nowString) if !reflect.DeepEqual(tt.expect, result) { t.Errorf("%s: expected map %v, got %v", tt.name, tt.expect, result) @@ -152,6 +160,8 @@ func TestGenerateOCIChartAnnotations(t *testing.T) { func TestGenerateOCIAnnotations(t *testing.T) { + nowString := helmtime.Now().Format(time.RFC3339) + tests := []struct { name string chart *chart.Metadata @@ -166,6 +176,7 @@ func TestGenerateOCIAnnotations(t *testing.T) { map[string]string{ "org.opencontainers.image.title": "oci", "org.opencontainers.image.version": "0.0.1", + "org.opencontainers.image.created": nowString, }, }, { @@ -183,6 +194,7 @@ func TestGenerateOCIAnnotations(t *testing.T) { "org.opencontainers.image.title": "oci", "org.opencontainers.image.version": "0.0.1", "org.opencontainers.image.description": "OCI Helm Chart", + "org.opencontainers.image.created": nowString, "extrakey": "extravlue", "anotherkey": "anothervalue", }, @@ -203,6 +215,7 @@ func TestGenerateOCIAnnotations(t *testing.T) { "org.opencontainers.image.title": "oci", "org.opencontainers.image.version": "0.0.1", "org.opencontainers.image.description": "OCI Helm Chart", + "org.opencontainers.image.created": nowString, "extrakey": "extravlue", }, }, @@ -210,7 +223,7 @@ func TestGenerateOCIAnnotations(t *testing.T) { for _, tt := range tests { - result := generateOCIAnnotations(tt.chart, true) + result := generateOCIAnnotations(tt.chart, nowString) if !reflect.DeepEqual(tt.expect, result) { t.Errorf("%s: expected map %v, got %v", tt.name, tt.expect, result) @@ -220,12 +233,16 @@ func TestGenerateOCIAnnotations(t *testing.T) { } func TestGenerateOCICreatedAnnotations(t *testing.T) { + + nowTime := helmtime.Now() + nowTimeString := nowTime.Format(time.RFC3339) + chart := &chart.Metadata{ Name: "oci", Version: "0.0.1", } - result := generateOCIAnnotations(chart, false) + result := generateOCIAnnotations(chart, nowTimeString) // Check that created annotation exists if _, ok := result[ocispec.AnnotationCreated]; !ok { @@ -237,4 +254,22 @@ func TestGenerateOCICreatedAnnotations(t *testing.T) { t.Errorf("%s annotation with value '%s' not in RFC3339 format", ocispec.AnnotationCreated, result[ocispec.AnnotationCreated]) } + // Verify default creation time set + result = generateOCIAnnotations(chart, "") + + // Check that created annotation exists + if _, ok := result[ocispec.AnnotationCreated]; !ok { + t.Errorf("%s annotation not created", ocispec.AnnotationCreated) + } + + if createdTimeAnnotation, err := helmtime.Parse(time.RFC3339, result[ocispec.AnnotationCreated]); err != nil { + t.Errorf("%s annotation with value '%s' not in RFC3339 format", ocispec.AnnotationCreated, result[ocispec.AnnotationCreated]) + + // Verify creation annotation after time test began + if !nowTime.Before(createdTimeAnnotation) { + t.Errorf("%s annotation with value '%s' not configured properly. Annotation value is not after %s", ocispec.AnnotationCreated, result[ocispec.AnnotationCreated], nowTimeString) + } + + } + } diff --git a/pkg/registry/utils_test.go b/pkg/registry/utils_test.go index 74aa0dbc0..d7aba2bb7 100644 --- a/pkg/registry/utils_test.go +++ b/pkg/registry/utils_test.go @@ -216,9 +216,12 @@ func initCompromisedRegistryTestServer() string { } func testPush(suite *TestSuite) { + + testingChartCreationTime := "1977-09-02T22:04:05Z" + // Bad bytes ref := fmt.Sprintf("%s/testrepo/testchart:1.2.3", suite.DockerRegistryHost) - _, err := suite.RegistryClient.Push([]byte("hello"), ref, PushOptTest(true)) + _, err := suite.RegistryClient.Push([]byte("hello"), ref, PushOptCreationTime(testingChartCreationTime)) suite.NotNil(err, "error pushing non-chart bytes") // Load a test chart @@ -229,20 +232,20 @@ func testPush(suite *TestSuite) { // non-strict ref (chart name) ref = fmt.Sprintf("%s/testrepo/boop:%s", suite.DockerRegistryHost, meta.Version) - _, err = suite.RegistryClient.Push(chartData, ref, PushOptTest(true)) + _, err = suite.RegistryClient.Push(chartData, ref, PushOptCreationTime(testingChartCreationTime)) suite.NotNil(err, "error pushing non-strict ref (bad basename)") // non-strict ref (chart name), with strict mode disabled - _, err = suite.RegistryClient.Push(chartData, ref, PushOptStrictMode(false), PushOptTest(true)) + _, err = suite.RegistryClient.Push(chartData, ref, PushOptStrictMode(false), PushOptCreationTime(testingChartCreationTime)) suite.Nil(err, "no error pushing non-strict ref (bad basename), with strict mode disabled") // non-strict ref (chart version) ref = fmt.Sprintf("%s/testrepo/%s:latest", suite.DockerRegistryHost, meta.Name) - _, err = suite.RegistryClient.Push(chartData, ref, PushOptTest(true)) + _, err = suite.RegistryClient.Push(chartData, ref, PushOptCreationTime(testingChartCreationTime)) suite.NotNil(err, "error pushing non-strict ref (bad tag)") // non-strict ref (chart version), with strict mode disabled - _, err = suite.RegistryClient.Push(chartData, ref, PushOptStrictMode(false), PushOptTest(true)) + _, err = suite.RegistryClient.Push(chartData, ref, PushOptStrictMode(false), PushOptCreationTime(testingChartCreationTime)) suite.Nil(err, "no error pushing non-strict ref (bad tag), with strict mode disabled") // basic push, good ref @@ -251,7 +254,7 @@ func testPush(suite *TestSuite) { meta, err = extractChartMeta(chartData) suite.Nil(err, "no error extracting chart meta") ref = fmt.Sprintf("%s/testrepo/%s:%s", suite.DockerRegistryHost, meta.Name, meta.Version) - _, err = suite.RegistryClient.Push(chartData, ref, PushOptTest(true)) + _, err = suite.RegistryClient.Push(chartData, ref, PushOptCreationTime(testingChartCreationTime)) suite.Nil(err, "no error pushing good ref") _, err = suite.RegistryClient.Pull(ref) @@ -269,7 +272,7 @@ func testPush(suite *TestSuite) { // push with prov ref = fmt.Sprintf("%s/testrepo/%s:%s", suite.DockerRegistryHost, meta.Name, meta.Version) - result, err := suite.RegistryClient.Push(chartData, ref, PushOptProvData(provData), PushOptTest(true)) + result, err := suite.RegistryClient.Push(chartData, ref, PushOptProvData(provData), PushOptCreationTime(testingChartCreationTime)) suite.Nil(err, "no error pushing good ref with prov") _, err = suite.RegistryClient.Pull(ref) @@ -281,12 +284,12 @@ func testPush(suite *TestSuite) { suite.Equal(ref, result.Ref) suite.Equal(meta.Name, result.Chart.Meta.Name) suite.Equal(meta.Version, result.Chart.Meta.Version) - suite.Equal(int64(684), result.Manifest.Size) + suite.Equal(int64(742), result.Manifest.Size) suite.Equal(int64(99), result.Config.Size) suite.Equal(int64(973), result.Chart.Size) suite.Equal(int64(695), result.Prov.Size) suite.Equal( - "sha256:b57e8ffd938c43253f30afedb3c209136288e6b3af3b33473e95ea3b805888e6", + "sha256:fbbade96da6050f68f94f122881e3b80051a18f13ab5f4081868dd494538f5c2", result.Manifest.Digest) suite.Equal( "sha256:8d17cb6bf6ccd8c29aace9a658495cbd5e2e87fc267876e86117c7db681c9580", @@ -354,12 +357,12 @@ func testPull(suite *TestSuite) { suite.Equal(ref, result.Ref) suite.Equal(meta.Name, result.Chart.Meta.Name) suite.Equal(meta.Version, result.Chart.Meta.Version) - suite.Equal(int64(684), result.Manifest.Size) + suite.Equal(int64(742), result.Manifest.Size) suite.Equal(int64(99), result.Config.Size) suite.Equal(int64(973), result.Chart.Size) suite.Equal(int64(695), result.Prov.Size) suite.Equal( - "sha256:b57e8ffd938c43253f30afedb3c209136288e6b3af3b33473e95ea3b805888e6", + "sha256:fbbade96da6050f68f94f122881e3b80051a18f13ab5f4081868dd494538f5c2", result.Manifest.Digest) suite.Equal( "sha256:8d17cb6bf6ccd8c29aace9a658495cbd5e2e87fc267876e86117c7db681c9580", @@ -370,7 +373,7 @@ func testPull(suite *TestSuite) { suite.Equal( "sha256:b0a02b7412f78ae93324d48df8fcc316d8482e5ad7827b5b238657a29a22f256", result.Prov.Digest) - suite.Equal("{\"schemaVersion\":2,\"config\":{\"mediaType\":\"application/vnd.cncf.helm.config.v1+json\",\"digest\":\"sha256:8d17cb6bf6ccd8c29aace9a658495cbd5e2e87fc267876e86117c7db681c9580\",\"size\":99},\"layers\":[{\"mediaType\":\"application/vnd.cncf.helm.chart.provenance.v1.prov\",\"digest\":\"sha256:b0a02b7412f78ae93324d48df8fcc316d8482e5ad7827b5b238657a29a22f256\",\"size\":695},{\"mediaType\":\"application/vnd.cncf.helm.chart.content.v1.tar+gzip\",\"digest\":\"sha256:e5ef611620fb97704d8751c16bab17fedb68883bfb0edc76f78a70e9173f9b55\",\"size\":973}],\"annotations\":{\"org.opencontainers.image.description\":\"A Helm chart for Kubernetes\",\"org.opencontainers.image.title\":\"signtest\",\"org.opencontainers.image.version\":\"0.1.0\"}}", + suite.Equal("{\"schemaVersion\":2,\"config\":{\"mediaType\":\"application/vnd.cncf.helm.config.v1+json\",\"digest\":\"sha256:8d17cb6bf6ccd8c29aace9a658495cbd5e2e87fc267876e86117c7db681c9580\",\"size\":99},\"layers\":[{\"mediaType\":\"application/vnd.cncf.helm.chart.provenance.v1.prov\",\"digest\":\"sha256:b0a02b7412f78ae93324d48df8fcc316d8482e5ad7827b5b238657a29a22f256\",\"size\":695},{\"mediaType\":\"application/vnd.cncf.helm.chart.content.v1.tar+gzip\",\"digest\":\"sha256:e5ef611620fb97704d8751c16bab17fedb68883bfb0edc76f78a70e9173f9b55\",\"size\":973}],\"annotations\":{\"org.opencontainers.image.created\":\"1977-09-02T22:04:05Z\",\"org.opencontainers.image.description\":\"A Helm chart for Kubernetes\",\"org.opencontainers.image.title\":\"signtest\",\"org.opencontainers.image.version\":\"0.1.0\"}}", string(result.Manifest.Data)) suite.Equal("{\"name\":\"signtest\",\"version\":\"0.1.0\",\"description\":\"A Helm chart for Kubernetes\",\"apiVersion\":\"v1\"}", string(result.Config.Data)) diff --git a/pkg/repo/index.go b/pkg/repo/index.go index 8a23ba060..40b11c5cf 100644 --- a/pkg/repo/index.go +++ b/pkg/repo/index.go @@ -359,10 +359,14 @@ func loadIndex(data []byte, source string) (*IndexFile, error) { log.Printf("skipping loading invalid entry for chart %q from %s: empty entry", name, source) continue } + // When metadata section missing, initialize with no data + if cvs[idx].Metadata == nil { + cvs[idx].Metadata = &chart.Metadata{} + } if cvs[idx].APIVersion == "" { cvs[idx].APIVersion = chart.APIVersionV1 } - if err := cvs[idx].Validate(); err != nil { + if err := cvs[idx].Validate(); ignoreSkippableChartValidationError(err) != nil { log.Printf("skipping loading invalid entry for chart %q %q from %s: %s", name, cvs[idx].Version, source, err) cvs = append(cvs[:idx], cvs[idx+1:]...) } @@ -388,3 +392,23 @@ func jsonOrYamlUnmarshal(b []byte, i interface{}) error { } return yaml.UnmarshalStrict(b, i) } + +// ignoreSkippableChartValidationError inspect the given error and returns nil if +// the error isn't important for index loading +// +// In particular, charts may introduce validations that don't impact repository indexes +// And repository indexes may be generated by older/non-complient software, which doesn't +// conform to all validations. +func ignoreSkippableChartValidationError(err error) error { + verr, ok := err.(chart.ValidationError) + if !ok { + return err + } + + // https://github.com/helm/helm/issues/12748 (JFrog repository strips alias field) + if strings.HasPrefix(verr.Error(), "validation: more than one dependency with name or alias") { + return nil + } + + return err +} diff --git a/pkg/repo/index_test.go b/pkg/repo/index_test.go index cb9317f7e..91486670b 100644 --- a/pkg/repo/index_test.go +++ b/pkg/repo/index_test.go @@ -20,6 +20,7 @@ import ( "bufio" "bytes" "encoding/json" + "fmt" "net/http" "os" "path/filepath" @@ -69,6 +70,10 @@ entries: name: grafana foo: - + bar: + - digest: "sha256:1234567890abcdef" + urls: + - https://charts.helm.sh/stable/alpine-1.0.0.tgz ` ) @@ -592,3 +597,50 @@ func TestAddFileIndexEntriesNil(t *testing.T) { } } } + +func TestIgnoreSkippableChartValidationError(t *testing.T) { + type TestCase struct { + Input error + ErrorSkipped bool + } + testCases := map[string]TestCase{ + "nil": { + Input: nil, + }, + "generic_error": { + Input: fmt.Errorf("foo"), + }, + "non_skipped_validation_error": { + Input: chart.ValidationError("chart.metadata.type must be application or library"), + }, + "skipped_validation_error": { + Input: chart.ValidationErrorf("more than one dependency with name or alias %q", "foo"), + ErrorSkipped: true, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + result := ignoreSkippableChartValidationError(tc.Input) + + if tc.Input == nil { + if result != nil { + t.Error("expected nil result for nil input") + } + return + } + + if tc.ErrorSkipped { + if result != nil { + t.Error("expected nil result for skipped error") + } + return + } + + if tc.Input != result { + t.Error("expected the result equal to input") + } + + }) + } +} diff --git a/pkg/time/ctime/ctime.go b/pkg/time/ctime/ctime.go new file mode 100644 index 000000000..f2998d76d --- /dev/null +++ b/pkg/time/ctime/ctime.go @@ -0,0 +1,25 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package ctime + +import ( + "os" + "time" +) + +func Created(fi os.FileInfo) time.Time { + return created(fi) +} diff --git a/pkg/time/ctime/ctime_linux.go b/pkg/time/ctime/ctime_linux.go new file mode 100644 index 000000000..c3cea1d78 --- /dev/null +++ b/pkg/time/ctime/ctime_linux.go @@ -0,0 +1,30 @@ +//go:build linux + +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package ctime + +import ( + "os" + "syscall" + "time" +) + +func created(fi os.FileInfo) time.Time { + st := fi.Sys().(*syscall.Stat_t) + //nolint + return time.Unix(int64(st.Ctim.Sec), int64(st.Ctim.Nsec)) +} diff --git a/pkg/time/ctime/ctime_other.go b/pkg/time/ctime/ctime_other.go new file mode 100644 index 000000000..f21ed7347 --- /dev/null +++ b/pkg/time/ctime/ctime_other.go @@ -0,0 +1,27 @@ +//go:build !linux + +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package ctime + +import ( + "os" + "time" +) + +func created(fi os.FileInfo) time.Time { + return fi.ModTime() +}