From 97e54c8f11195e21b479606ae762e7c36282f0c2 Mon Sep 17 00:00:00 2001 From: Aleksei Krugliak Date: Mon, 11 May 2026 21:19:12 +0300 Subject: [PATCH] fix: address review on repoURL provenance stamping - Stamp chart.Metadata.RepoURL in Install/Upgrade actions instead of pkg/cmd, so library consumers also get provenance recorded. - Don't mislabel local installs as path; fill RepoURL only for resolved repos, OCI refs, and absolute http(s) URLs. - Unexport ChartDownloader.RepositoryURL; expose via accessor to preserve ABI for downstream unkeyed-struct users. - Fix render test template to use .Chart.RepoURL. Signed-off-by: Aleksei Krugliak --- pkg/action/install.go | 35 ++++++++++- pkg/action/repourl2_test.go | 62 +++++++++++++++++++ .../chart-with-repourl-invalid/Chart.yaml | 5 ++ .../templates/configmap.yaml | 6 ++ .../chart-with-repourl-valid/Chart.yaml | 5 ++ .../templates/configmap.yaml | 6 ++ .../multiplecharts-lint-chart-2/Chart.yaml | 2 +- pkg/action/upgrade.go | 9 +++ pkg/chart/v2/metadata.go | 3 + pkg/cmd/install.go | 6 -- pkg/cmd/upgrade.go | 5 -- pkg/downloader/chart_downloader.go | 14 ++++- 12 files changed, 142 insertions(+), 16 deletions(-) create mode 100644 pkg/action/repourl2_test.go create mode 100644 pkg/action/testdata/charts/chart-with-repourl-invalid/Chart.yaml create mode 100644 pkg/action/testdata/charts/chart-with-repourl-invalid/templates/configmap.yaml create mode 100644 pkg/action/testdata/charts/chart-with-repourl-valid/Chart.yaml create mode 100644 pkg/action/testdata/charts/chart-with-repourl-valid/templates/configmap.yaml diff --git a/pkg/action/install.go b/pkg/action/install.go index 776501a64..567f8a382 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -352,6 +352,16 @@ func (i *Install) RunWithContext(ctx context.Context, ch ci.Charter, vals map[st return nil, fmt.Errorf("user supplied labels contains system reserved label name. System labels: %+v", driver.GetSystemLabels()) } + // Stamp provenance information into the chart metadata if available from + // the ChartPathOptions. Do not overwrite existing metadata.RepoURL set by + // the chart itself. This ensures library consumers who call actions get + // provenance filled in when LocateChart or a downloader discovered it. + if chrt.Metadata != nil && chrt.Metadata.RepoURL == "" { + if i.ChartPathOptions.RepoURL != "" { + chrt.Metadata.RepoURL = i.ChartPathOptions.RepoURL + } + } + rel := i.createRelease(chrt, vals, i.Labels) var manifestDoc *bytes.Buffer @@ -857,6 +867,20 @@ func urlEqual(u1, u2 *url.URL) bool { return u1.Scheme == u2.Scheme && u1.Hostname() == u2.Hostname() && portOrDefault(u1) == portOrDefault(u2) } +// isRemoteChartRef reports whether ref points to a remote chart (an OCI +// reference or an absolute http(s) URL), as opposed to a local path or a +// repo-by-name reference. +func isRemoteChartRef(ref string) bool { + if registry.IsOCI(ref) { + return true + } + u, err := url.Parse(ref) + if err != nil { + return false + } + return u.IsAbs() && (u.Scheme == "http" || u.Scheme == "https") +} + // LocateChart looks for a chart directory in known places, and returns either the full path or an error. // // This does not ensure that the chart is well-formed; only that the requested filename exists. @@ -964,7 +988,16 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) ( if err != nil { return "", err } - c.RepoURL = dl.RepositoryURL + + // Record provenance information about where the chart was resolved from. + // Prefer the URL discovered by the downloader; fall back to the original + // reference when it is an OCI ref or an absolute URL. Leave RepoURL empty + // for local paths and unresolvable references so callers can distinguish. + if u := dl.RepositoryURL(); u != "" { + c.RepoURL = u + } else if c.RepoURL == "" && isRemoteChartRef(name) { + c.RepoURL = name + } lname, err := filepath.Abs(filename) if err != nil { diff --git a/pkg/action/repourl2_test.go b/pkg/action/repourl2_test.go new file mode 100644 index 000000000..676999e88 --- /dev/null +++ b/pkg/action/repourl2_test.go @@ -0,0 +1,62 @@ +package action + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "helm.sh/helm/v4/pkg/chart/v2/loader" + chartutil "helm.sh/helm/v4/pkg/chart/v2/util" +) + +func TestRenderRepoURL2_ValidAndInvalid(t *testing.T) { + cfg := actionConfigFixture(t) + + tests := []struct { + name string + chartPath string + expectPart string + }{ + { + name: "valid repoURL", + chartPath: "testdata/charts/chart-with-repourl-valid", + expectPart: `repoURL: "https://example.com/charts"`, + }, + { + name: "invalid repoURL", + chartPath: "testdata/charts/chart-with-repourl-invalid", + expectPart: `repoURL: "ht!tp://not-a-valid-url"`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ch, err := loader.Load(tt.chartPath) + require.NoError(t, err) + if ch.Metadata == nil { + md, err := chartutil.LoadChartfile(filepath.Join(tt.chartPath, "Chart.yaml")) + require.NoError(t, err) + ch.Metadata = md + } + + _, buf, _, err := cfg.renderResources( + ch, + map[string]interface{}{}, + "test-release", + "", + false, + false, + false, + nil, + false, + false, + false, + ) + require.NoError(t, err) + require.NotNil(t, buf) + assert.Contains(t, buf.String(), tt.expectPart) + }) + } +} diff --git a/pkg/action/testdata/charts/chart-with-repourl-invalid/Chart.yaml b/pkg/action/testdata/charts/chart-with-repourl-invalid/Chart.yaml new file mode 100644 index 000000000..89a08d5ef --- /dev/null +++ b/pkg/action/testdata/charts/chart-with-repourl-invalid/Chart.yaml @@ -0,0 +1,5 @@ +apiVersion: v2 +name: chart-with-repourl-invalid +version: 0.1.0 +description: Test chart containing an invalid repoURL in metadata +repoURL: "ht!tp://not-a-valid-url" diff --git a/pkg/action/testdata/charts/chart-with-repourl-invalid/templates/configmap.yaml b/pkg/action/testdata/charts/chart-with-repourl-invalid/templates/configmap.yaml new file mode 100644 index 000000000..ec74e9cb4 --- /dev/null +++ b/pkg/action/testdata/charts/chart-with-repourl-invalid/templates/configmap.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: chart-with-repourl-invalid-cm +data: + repoURL: {{ .Chart.RepoURL | quote }} diff --git a/pkg/action/testdata/charts/chart-with-repourl-valid/Chart.yaml b/pkg/action/testdata/charts/chart-with-repourl-valid/Chart.yaml new file mode 100644 index 000000000..7f4d9e0bf --- /dev/null +++ b/pkg/action/testdata/charts/chart-with-repourl-valid/Chart.yaml @@ -0,0 +1,5 @@ +apiVersion: v2 +name: chart-with-repourl-valid +version: 0.1.0 +description: Test chart containing a valid repoURL in metadata +repoURL: https://example.com/charts diff --git a/pkg/action/testdata/charts/chart-with-repourl-valid/templates/configmap.yaml b/pkg/action/testdata/charts/chart-with-repourl-valid/templates/configmap.yaml new file mode 100644 index 000000000..2629a1021 --- /dev/null +++ b/pkg/action/testdata/charts/chart-with-repourl-valid/templates/configmap.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: chart-with-repourl-valid-cm +data: + repoURL: {{ .Chart.RepoURL | quote }} diff --git a/pkg/action/testdata/charts/multiplecharts-lint-chart-2/Chart.yaml b/pkg/action/testdata/charts/multiplecharts-lint-chart-2/Chart.yaml index b27de2754..7f348df6a 100644 --- a/pkg/action/testdata/charts/multiplecharts-lint-chart-2/Chart.yaml +++ b/pkg/action/testdata/charts/multiplecharts-lint-chart-2/Chart.yaml @@ -1,4 +1,4 @@ apiVersion: v1 name: multiplecharts-lint-chart-2 version: "1" -icon: "" \ No newline at end of file +icon: "" diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 4c93855b1..5df7438ac 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -312,6 +312,15 @@ func (u *Upgrade) prepareUpgrade(name string, chart *chartv2.Chart, vals map[str u.cfg.Logger().Debug("determined release apply method", slog.Bool("server_side_apply", serverSideApply), slog.String("previous_release_apply_method", lastRelease.ApplyMethod)) + // Stamp provenance information into the chart metadata if available from + // the ChartPathOptions. Only set it when chart metadata doesn't already + // contain a RepoURL so we don't overwrite embedded provenance. + if chart.Metadata != nil && chart.Metadata.RepoURL == "" { + if u.ChartPathOptions.RepoURL != "" { + chart.Metadata.RepoURL = u.ChartPathOptions.RepoURL + } + } + // Store an upgraded release. upgradedRelease := &release.Release{ Name: name, diff --git a/pkg/chart/v2/metadata.go b/pkg/chart/v2/metadata.go index c46007863..fbf19cda2 100644 --- a/pkg/chart/v2/metadata.go +++ b/pkg/chart/v2/metadata.go @@ -52,6 +52,8 @@ type Metadata struct { Home string `json:"home,omitempty"` // Source is the URL to the source code of this chart Sources []string `json:"sources,omitempty"` + // The URL to the chart's repository (provenance information) + RepoURL string `json:"repoURL,omitempty"` // A version string of the chart. Required. Version string `json:"version,omitempty"` // A one-sentence description of the chart @@ -93,6 +95,7 @@ func (md *Metadata) Validate() error { md.Name = sanitizeString(md.Name) md.Description = sanitizeString(md.Description) md.Home = sanitizeString(md.Home) + md.RepoURL = sanitizeString(md.RepoURL) md.Icon = sanitizeString(md.Icon) md.Condition = sanitizeString(md.Condition) md.Tags = sanitizeString(md.Tags) diff --git a/pkg/cmd/install.go b/pkg/cmd/install.go index 6f1576b48..d36cd9e34 100644 --- a/pkg/cmd/install.go +++ b/pkg/cmd/install.go @@ -262,12 +262,6 @@ func runInstall(args []string, client *action.Install, valueOpts *values.Options return nil, err } - if client.ChartPathOptions.RepoURL != "" { - chartRequested.Metadata.RepoURL = client.ChartPathOptions.RepoURL - } else { - chartRequested.Metadata.RepoURL = "path" - } - ac, err := chart.NewAccessor(chartRequested) if err != nil { return nil, err diff --git a/pkg/cmd/upgrade.go b/pkg/cmd/upgrade.go index 821cb49c1..918d6f5b8 100644 --- a/pkg/cmd/upgrade.go +++ b/pkg/cmd/upgrade.go @@ -197,11 +197,6 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { if err != nil { return err } - if client.ChartPathOptions.RepoURL != "" { - ch.Metadata.RepoURL = client.ChartPathOptions.RepoURL - } else { - ch.Metadata.RepoURL = "path" - } ac, err := ci.NewAccessor(ch) if err != nil { diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 73ade62c3..033efc4af 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -84,8 +84,16 @@ type ChartDownloader struct { ContentCache string // Cache specifies the cache implementation to use. - Cache Cache - RepositoryURL string + Cache Cache + + // repositoryURL is the resolved Helm repository URL, set by + // ResolveChartVersion when the chart is looked up via a configured repo. + repositoryURL string +} + +// RepositoryURL returns the resolved Helm repository URL, if any. +func (c *ChartDownloader) RepositoryURL() string { + return c.repositoryURL } // DownloadTo retrieves a chart. Depending on the settings, it may also download a provenance file. @@ -432,7 +440,7 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (string, *url if r != nil && r.Config != nil { if r.Config.URL != "" { - c.RepositoryURL = r.Config.URL + c.repositoryURL = r.Config.URL } if r.Config.CertFile != "" || r.Config.KeyFile != "" || r.Config.CAFile != "" { c.Options = append(c.Options, getter.WithTLSClientConfig(r.Config.CertFile, r.Config.KeyFile, r.Config.CAFile))