From 17365dcc4a90ba0fef998404d6fcf70660a8c1a8 Mon Sep 17 00:00:00 2001 From: KR Ravindra <42912207+KR-Ravindra@users.noreply.github.com> Date: Tue, 8 Sep 2026 06:37:15 +0000 Subject: [PATCH] Sanitise label values rendered by the helm create scaffold The generated _helpers.tpl builds helm.sh/chart with `replace "+" "_" | trunc 63 | trimSuffix "-"`. Kubernetes label values must also not end in "." or "_", so a version whose 63-character cut lands on either produces a value the API server rejects. The app.kubernetes.io/version label emits .Chart.AppVersion unmodified and is invalid for any appVersion over 63 characters or containing "+". Use `trimAll "-_."` after truncation for helm.sh/chart, and apply the same replace/trunc/trim chain to app.kubernetes.io/version, in both the v2 and the internal v3 scaffold. Add a test that renders the generated chart with such versions and validates the labels with k8s.io/apimachinery's IsValidLabelValue. Signed-off-by: KR Ravindra <42912207+KR-Ravindra@users.noreply.github.com> --- internal/chart/v3/util/create.go | 4 +- internal/chart/v3/util/create_test.go | 60 +++++++++++++++++++++++++++ pkg/chart/v2/util/create.go | 4 +- pkg/chart/v2/util/create_test.go | 60 +++++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 4 deletions(-) diff --git a/internal/chart/v3/util/create.go b/internal/chart/v3/util/create.go index 0c143615e..15b32f35f 100644 --- a/internal/chart/v3/util/create.go +++ b/internal/chart/v3/util/create.go @@ -590,7 +590,7 @@ If release name contains chart name it will be used as a full name. Create chart name and version as used by the chart label. */}} {{- define ".chart" -}} -{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" }} +{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimAll "-_." }} {{- end }} {{/* @@ -600,7 +600,7 @@ Common labels helm.sh/chart: {{ include ".chart" . }} {{ include ".selectorLabels" . }} {{- if .Chart.AppVersion }} -app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} +app.kubernetes.io/version: {{ .Chart.AppVersion | replace "+" "_" | trunc 63 | trimAll "-_." | quote }} {{- end }} app.kubernetes.io/managed-by: {{ .Release.Service }} {{- end }} diff --git a/internal/chart/v3/util/create_test.go b/internal/chart/v3/util/create_test.go index a227e4e7c..de2125dcd 100644 --- a/internal/chart/v3/util/create_test.go +++ b/internal/chart/v3/util/create_test.go @@ -25,9 +25,14 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/validation" + "sigs.k8s.io/yaml" chart "helm.sh/helm/v4/internal/chart/v3" "helm.sh/helm/v4/internal/chart/v3/loader" + "helm.sh/helm/v4/pkg/chart/common" + "helm.sh/helm/v4/pkg/chart/common/util" + "helm.sh/helm/v4/pkg/engine" ) func TestCreate(t *testing.T) { @@ -95,6 +100,61 @@ func TestCreateFrom(t *testing.T) { } } +func TestCreate_LabelValues(t *testing.T) { + tdir := t.TempDir() + + c, err := Create("demo", tdir) + require.NoError(t, err) + + mychart, err := loader.LoadDir(c) + require.NoError(t, err) + + // "demo-" + version is 64 characters, so `trunc 63` cuts right after the + // character that precedes the final "1". + for name, tc := range map[string]struct { + version string + appVersion string + }{ + "truncated chart label ends in dot": { + version: "1.0.0-abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxy.1", + appVersion: "1.16.0", + }, + "truncated chart label ends in underscore": { + version: "1.0.0-abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxy+1", + appVersion: "1.16.0", + }, + "app version with build metadata longer than 63 characters": { + version: "0.1.0", + appVersion: "v1.2.3+build.123456789012345678901234567890123456789012345678901234567890", + }, + } { + t.Run(name, func(t *testing.T) { + mychart.Metadata.Version = tc.version + mychart.Metadata.AppVersion = tc.appVersion + + opts := common.ReleaseOptions{Name: "demo", Namespace: "default", Revision: 1, IsInstall: true} + vals, err := util.ToRenderValues(mychart, map[string]any{}, opts, common.DefaultCapabilities) + require.NoError(t, err) + + out, err := new(engine.Engine).RenderWithContext(t.Context(), mychart, vals) + require.NoError(t, err) + + var sa struct { + Metadata struct { + Labels map[string]string `json:"labels"` + } `json:"metadata"` + } + require.NoError(t, yaml.Unmarshal([]byte(out["demo/templates/serviceaccount.yaml"]), &sa)) + + for _, key := range []string{"helm.sh/chart", "app.kubernetes.io/version"} { + v, ok := sa.Metadata.Labels[key] + require.True(t, ok, "expected label %q to be set", key) + assert.Empty(t, validation.IsValidLabelValue(v), "label %s=%q", key, v) + } + }) + } +} + // TestCreate_Overwrite is a regression test for making sure that files are overwritten. func TestCreate_Overwrite(t *testing.T) { tdir := t.TempDir() diff --git a/pkg/chart/v2/util/create.go b/pkg/chart/v2/util/create.go index 65546e21b..940b8a6df 100644 --- a/pkg/chart/v2/util/create.go +++ b/pkg/chart/v2/util/create.go @@ -589,7 +589,7 @@ If release name contains chart name it will be used as a full name. Create chart name and version as used by the chart label. */}} {{- define ".chart" -}} -{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" }} +{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimAll "-_." }} {{- end }} {{/* @@ -599,7 +599,7 @@ Common labels helm.sh/chart: {{ include ".chart" . }} {{ include ".selectorLabels" . }} {{- if .Chart.AppVersion }} -app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} +app.kubernetes.io/version: {{ .Chart.AppVersion | replace "+" "_" | trunc 63 | trimAll "-_." | quote }} {{- end }} app.kubernetes.io/managed-by: {{ .Release.Service }} {{- end }} diff --git a/pkg/chart/v2/util/create_test.go b/pkg/chart/v2/util/create_test.go index 69ba1336d..b89ca35f4 100644 --- a/pkg/chart/v2/util/create_test.go +++ b/pkg/chart/v2/util/create_test.go @@ -24,9 +24,14 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/validation" + "sigs.k8s.io/yaml" + "helm.sh/helm/v4/pkg/chart/common" + "helm.sh/helm/v4/pkg/chart/common/util" chart "helm.sh/helm/v4/pkg/chart/v2" "helm.sh/helm/v4/pkg/chart/v2/loader" + "helm.sh/helm/v4/pkg/engine" ) func TestCreate(t *testing.T) { @@ -92,6 +97,61 @@ func TestCreateFrom(t *testing.T) { } } +func TestCreate_LabelValues(t *testing.T) { + tdir := t.TempDir() + + c, err := Create("demo", tdir) + require.NoError(t, err) + + mychart, err := loader.LoadDir(c) + require.NoError(t, err) + + // "demo-" + version is 64 characters, so `trunc 63` cuts right after the + // character that precedes the final "1". + for name, tc := range map[string]struct { + version string + appVersion string + }{ + "truncated chart label ends in dot": { + version: "1.0.0-abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxy.1", + appVersion: "1.16.0", + }, + "truncated chart label ends in underscore": { + version: "1.0.0-abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxy+1", + appVersion: "1.16.0", + }, + "app version with build metadata longer than 63 characters": { + version: "0.1.0", + appVersion: "v1.2.3+build.123456789012345678901234567890123456789012345678901234567890", + }, + } { + t.Run(name, func(t *testing.T) { + mychart.Metadata.Version = tc.version + mychart.Metadata.AppVersion = tc.appVersion + + opts := common.ReleaseOptions{Name: "demo", Namespace: "default", Revision: 1, IsInstall: true} + vals, err := util.ToRenderValues(mychart, map[string]any{}, opts, common.DefaultCapabilities) + require.NoError(t, err) + + out, err := new(engine.Engine).RenderWithContext(t.Context(), mychart, vals) + require.NoError(t, err) + + var sa struct { + Metadata struct { + Labels map[string]string `json:"labels"` + } `json:"metadata"` + } + require.NoError(t, yaml.Unmarshal([]byte(out["demo/templates/serviceaccount.yaml"]), &sa)) + + for _, key := range []string{"helm.sh/chart", "app.kubernetes.io/version"} { + v, ok := sa.Metadata.Labels[key] + require.True(t, ok, "expected label %q to be set", key) + assert.Empty(t, validation.IsValidLabelValue(v), "label %s=%q", key, v) + } + }) + } +} + // TestCreate_Overwrite is a regression test for making sure that files are overwritten. func TestCreate_Overwrite(t *testing.T) { tdir := t.TempDir()