diff --git a/pkg/chart/v2/lint/lint.go b/pkg/chart/v2/lint/lint.go index 23a93c551..4ba4505c3 100644 --- a/pkg/chart/v2/lint/lint.go +++ b/pkg/chart/v2/lint/lint.go @@ -65,6 +65,7 @@ func RunAll(baseDir string, values map[string]any, namespace string, options ... rules.TemplateLinterSkipSchemaValidation(lo.SkipSchemaValidation)) rules.Dependencies(&result) rules.Crds(&result) + rules.Sequencing(&result, namespace, values) return result } diff --git a/pkg/chart/v2/lint/lint_test.go b/pkg/chart/v2/lint/lint_test.go index 4256281e0..765c9312b 100644 --- a/pkg/chart/v2/lint/lint_test.go +++ b/pkg/chart/v2/lint/lint_test.go @@ -17,12 +17,15 @@ limitations under the License. package lint import ( + "path/filepath" "strings" "testing" "time" "github.com/stretchr/testify/assert" + "helm.sh/helm/v4/pkg/chart/common" + chart "helm.sh/helm/v4/pkg/chart/v2" "helm.sh/helm/v4/pkg/chart/v2/lint/support" chartutil "helm.sh/helm/v4/pkg/chart/v2/util" ) @@ -245,3 +248,42 @@ func TestMalformedTemplate(t *testing.T) { } } } + +func TestRunAll_RegistersSequencingRules(t *testing.T) { + dir := t.TempDir() + + testChart := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "sequencinglint", + Version: "0.1.0", + APIVersion: chart.APIVersionV2, + }, + Templates: []*common.File{ + { + Name: "templates/configmap.yaml", + Data: []byte(`apiVersion: v1 +kind: ConfigMap +metadata: + name: partial-readiness + annotations: + helm.sh/readiness-success: '["{.status.phase} == \"Ready\""]' +data: + key: value +`), + }, + }, + } + + assert.NoError(t, chartutil.SaveDir(testChart, dir)) + + messages := RunAll(filepath.Join(dir, testChart.Name()), nil, namespace).Messages + assert.Condition(t, func() bool { + for _, msg := range messages { + if msg.Severity == support.ErrorSev && strings.Contains(msg.Err.Error(), "both must be present or absent together") { + return true + } + } + + return false + }, "expected sequencing lint error in %#v", messages) +} diff --git a/pkg/chart/v2/lint/rules/sequencing.go b/pkg/chart/v2/lint/rules/sequencing.go new file mode 100644 index 000000000..e24f1e6a9 --- /dev/null +++ b/pkg/chart/v2/lint/rules/sequencing.go @@ -0,0 +1,256 @@ +/* +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 rules // import "helm.sh/helm/v4/pkg/chart/v2/lint/rules" + +import ( + "fmt" + "path" + "strings" + + "sigs.k8s.io/yaml" + + "helm.sh/helm/v4/pkg/chart/common" + commonutil "helm.sh/helm/v4/pkg/chart/common/util" + chart "helm.sh/helm/v4/pkg/chart/v2" + "helm.sh/helm/v4/pkg/chart/v2/lint/support" + "helm.sh/helm/v4/pkg/chart/v2/loader" + chartutil "helm.sh/helm/v4/pkg/chart/v2/util" + "helm.sh/helm/v4/pkg/engine" + "helm.sh/helm/v4/pkg/kube" + release "helm.sh/helm/v4/pkg/release/v1" + releaseutil "helm.sh/helm/v4/pkg/release/v1/util" +) + +// Sequencing runs lint rules for HIP-0025 sequencing annotations. +func Sequencing(linter *support.Linter, namespace string, values map[string]any) { + c, err := loader.LoadDir(linter.ChartDir) + if err != nil { + // Chart load errors are already reported by other lint rules. + return + } + + // ProcessDependencies must run before validateSubchartSequencing: + // it prunes disabled subcharts from c.Dependencies() and applies + // alias renames, which BuildSubchartDAG relies on. + if err := chartutil.ProcessDependencies(c, values); err != nil { + // ProcessDependencies rejects invalid depends-on references (e.g. + // ambiguous alias references); surface that instead of silently + // skipping sequencing validation. + linter.RunLinterRule(support.ErrorSev, linter.ChartDir, err) + return + } + + linter.RunLinterRule(support.ErrorSev, linter.ChartDir, validateSubchartSequencing(c)) + validateRenderedSequencingAnnotations(linter, c, namespace, values) +} + +func validateSubchartSequencing(c *chart.Chart) error { + // Note: we must NOT early-return on len(Dependencies) == 0. A chart with + // zero dependencies can still carry a helm.sh/depends-on/subcharts + // annotation that references a non-existent subchart — the orphan case + // BuildSubchartDAG's validateParentSubchartDependencies is meant to catch. + if c.Metadata == nil { + return nil + } + + dag, err := chartutil.BuildSubchartDAG(c) + if err != nil { + return err + } + if _, err := dag.GetBatches(); err != nil { + return fmt.Errorf("subchart circular dependency detected: %w", err) + } + + return nil +} + +func validateRenderedSequencingAnnotations(linter *support.Linter, c *chart.Chart, namespace string, values map[string]any) { + if err := chartutil.ProcessDependencies(c, values); err != nil { + return + } + + options := common.ReleaseOptions{ + Name: "test-release", + Namespace: namespace, + } + caps := common.DefaultCapabilities.Copy() + + coalescedValues, err := commonutil.CoalesceValues(c, values) + if err != nil { + return + } + + valuesToRender, err := commonutil.ToRenderValues(c, coalescedValues, options, caps) + if err != nil { + return + } + + var renderEngine engine.Engine + renderEngine.LintMode = true + + renderedContentMap, err := renderEngine.Render(c, valuesToRender) + if err != nil { + // Template rendering errors are already reported by the Templates lint rule. + return + } + + manifestsByChart := make(map[string][]releaseutil.Manifest) + for templatePath, content := range renderedContentMap { + if strings.TrimSpace(content) == "" { + continue + } + + chartPath := renderedTemplateChartPath(templatePath) + for _, manifest := range parseRenderedManifests(content) { + // HIP-0025 explicitly excludes hooks from sequencing: at install + // time SortManifests routes hook resources out before resource-group + // parsing runs, so their sequencing annotations are ignored. Mirror + // that here — otherwise lint reads a hook's leftover + // resource-group/depends-on annotations into the DAG and can report + // spurious cycles or orphan errors that never occur at runtime. + if isHookManifest(manifest) { + continue + } + validateReadinessAnnotations(linter, templatePath, manifest) + manifestsByChart[chartPath] = append(manifestsByChart[chartPath], manifest) + } + } + + for chartPath, manifests := range manifestsByChart { + validateResourceGroupAnnotations(linter, chartPath, manifests) + } +} + +// isHookManifest reports whether a rendered manifest is a Helm hook. Hooks +// carry the helm.sh/hook annotation and are excluded from HIP-0025 sequencing +// (the install path separates them via SortManifests), so the sequencing lint +// rules must skip them too. +func isHookManifest(manifest releaseutil.Manifest) bool { + if manifest.Head == nil || manifest.Head.Metadata == nil { + return false + } + return strings.TrimSpace(manifest.Head.Metadata.Annotations[release.HookAnnotation]) != "" +} + +func renderedTemplateChartPath(templatePath string) string { + if chartPath, _, ok := strings.Cut(templatePath, "/templates/"); ok { + return chartPath + } + + return path.Dir(templatePath) +} + +func parseRenderedManifests(content string) []releaseutil.Manifest { + rawManifests := releaseutil.SplitManifests(content) + manifests := make([]releaseutil.Manifest, 0, len(rawManifests)) + + for manifestName, raw := range rawManifests { + if strings.TrimSpace(raw) == "" { + continue + } + + var head releaseutil.SimpleHead + if err := yaml.Unmarshal([]byte(raw), &head); err != nil { + continue + } + + if head.Metadata != nil && head.Metadata.Name != "" { + manifestName = head.Metadata.Name + } + + manifests = append(manifests, releaseutil.Manifest{ + Name: manifestName, + Content: raw, + Head: &head, + }) + } + + return manifests +} + +func validateReadinessAnnotations(linter *support.Linter, templatePath string, manifest releaseutil.Manifest) { + if manifest.Head == nil || manifest.Head.Metadata == nil { + return + } + + annotations := manifest.Head.Metadata.Annotations + successRaw := strings.TrimSpace(annotations[kube.AnnotationReadinessSuccess]) + failureRaw := strings.TrimSpace(annotations[kube.AnnotationReadinessFailure]) + hasSuccess := successRaw != "" + hasFailure := failureRaw != "" + + if hasSuccess != hasFailure { + linter.RunLinterRule(support.ErrorSev, templatePath, fmt.Errorf( + "resource %q has only one of %q / %q annotations; both must be present or absent together", + resourceDisplayName(manifest), + kube.AnnotationReadinessSuccess, + kube.AnnotationReadinessFailure, + )) + return + } + + // Both present (or both absent). When present, the JSONPath expressions + // must still be well-formed — the presence-symmetry check alone let + // malformed expressions through at lint time. + for key, raw := range map[string]string{ + kube.AnnotationReadinessSuccess: successRaw, + kube.AnnotationReadinessFailure: failureRaw, + } { + if err := kube.ValidateReadinessExpressions(raw); err != nil { + linter.RunLinterRule(support.ErrorSev, templatePath, fmt.Errorf( + "resource %q has malformed %q annotation: %w", + resourceDisplayName(manifest), key, err, + )) + } + } +} + +func validateResourceGroupAnnotations(linter *support.Linter, chartPath string, manifests []releaseutil.Manifest) { + result, warnings, err := releaseutil.ParseResourceGroups(manifests) + // HIP-0025: lint must fail on orphan resource-group dependencies and + // malformed annotation JSON. Runtime falls back to the unsequenced batch + // for graceful recovery, but the chart author should fix these at lint time. + for _, warning := range warnings { + linter.RunLinterRule(support.ErrorSev, chartPath, fmt.Errorf("%s", warning)) + } + if err != nil { + linter.RunLinterRule(support.ErrorSev, chartPath, err) + return + } + + dag, err := releaseutil.BuildResourceGroupDAG(result) + if err != nil { + linter.RunLinterRule(support.ErrorSev, chartPath, err) + return + } + + if _, err := dag.GetBatches(); err != nil { + linter.RunLinterRule(support.ErrorSev, chartPath, fmt.Errorf("resource-group circular dependency detected: %w", err)) + } +} + +func resourceDisplayName(manifest releaseutil.Manifest) string { + if manifest.Head == nil || manifest.Head.Metadata == nil { + return manifest.Name + } + + if manifest.Head.Kind == "" { + return manifest.Head.Metadata.Name + } + + return fmt.Sprintf("%s/%s", manifest.Head.Kind, manifest.Head.Metadata.Name) +} diff --git a/pkg/chart/v2/lint/rules/sequencing_test.go b/pkg/chart/v2/lint/rules/sequencing_test.go new file mode 100644 index 000000000..478bdaf7d --- /dev/null +++ b/pkg/chart/v2/lint/rules/sequencing_test.go @@ -0,0 +1,348 @@ +/* +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 rules + +import ( + "fmt" + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "helm.sh/helm/v4/pkg/chart/common" + chart "helm.sh/helm/v4/pkg/chart/v2" + "helm.sh/helm/v4/pkg/chart/v2/lint/support" + chartutil "helm.sh/helm/v4/pkg/chart/v2/util" + releaseutil "helm.sh/helm/v4/pkg/release/v1/util" +) + +func TestSequencing_SubchartCircularDependency(t *testing.T) { + t.Parallel() + + root := newChart("testchart", nil) + root.Metadata.Dependencies = []*chart.Dependency{ + {Name: "subchart-a", Version: "0.1.0", Repository: "file://charts/subchart-a", DependsOn: []string{"subchart-b"}}, + {Name: "subchart-b", Version: "0.1.0", Repository: "file://charts/subchart-b", DependsOn: []string{"subchart-a"}}, + } + root.SetDependencies(newChart("subchart-a", nil), newChart("subchart-b", nil)) + + messages := runSequencingLint(t, root) + requireMessage(t, messages, support.ErrorSev, "subchart circular dependency detected") +} + +func TestSequencing_AliasedSubchartDependsOnOriginalName(t *testing.T) { + t.Parallel() + + // Full loader + ProcessDependencies pipeline: an aliased subchart may be + // referenced by its original chart name (HIP-0025: "names or aliases"). + // Before the fix this reported a spurious "unknown or disabled subchart". + root := newChart("testchart", nil) + root.Metadata.Dependencies = []*chart.Dependency{ + {Name: "subchart-a", Version: "0.1.0", Repository: "file://charts/subchart-a", Alias: "aliased-a"}, + {Name: "subchart-b", Version: "0.1.0", Repository: "file://charts/subchart-b", DependsOn: []string{"subchart-a"}}, + } + root.SetDependencies(newChart("subchart-a", nil), newChart("subchart-b", nil)) + + messages := runSequencingLint(t, root) + assert.Empty(t, messages) +} + +func TestSequencing_AmbiguousDependsOnReported(t *testing.T) { + t.Parallel() + + // Ambiguity is now rejected by ProcessDependencies; the lint rule must + // surface that instead of silently skipping sequencing validation. + root := newChart("testchart", nil) + root.Metadata.Dependencies = []*chart.Dependency{ + {Name: "subchart-a", Version: "0.1.0", Repository: "file://charts/subchart-a", Alias: "first"}, + {Name: "subchart-a", Version: "0.1.0", Repository: "file://charts/subchart-a", Alias: "second"}, + {Name: "subchart-b", Version: "0.1.0", Repository: "file://charts/subchart-b", DependsOn: []string{"subchart-a"}}, + } + root.SetDependencies(newChart("subchart-a", nil), newChart("subchart-b", nil)) + + messages := runSequencingLint(t, root) + requireMessage(t, messages, support.ErrorSev, `ambiguous subchart reference "subchart-a"`) +} + +func TestSequencing_SubchartAnnotationRequiresHIPListSyntax(t *testing.T) { + t.Parallel() + + root := newChart("testchart", nil) + root.Metadata.Dependencies = []*chart.Dependency{ + {Name: "subchart-a", Version: "0.1.0", Repository: "file://charts/subchart-a"}, + {Name: "subchart-b", Version: "0.1.0", Repository: "file://charts/subchart-b"}, + } + root.Metadata.Annotations = map[string]string{ + chartutil.AnnotationDependsOnSubcharts: `{"subchart-a":["subchart-b"],"subchart-b":["subchart-a"]}`, + } + root.SetDependencies(newChart("subchart-a", nil), newChart("subchart-b", nil)) + + messages := runSequencingLint(t, root) + requireMessage(t, messages, support.ErrorSev, "JSON string array") +} + +func TestSequencing_SubchartAnnotationOrphanWithNoDependencies(t *testing.T) { + t.Parallel() + + // A chart with ZERO dependencies can still carry a + // helm.sh/depends-on/subcharts annotation referencing a subchart that + // doesn't exist. The lint rule previously early-returned on + // len(Dependencies)==0 and missed this orphan reference entirely. + root := newChart("testchart", nil) + root.Metadata.Annotations = map[string]string{ + chartutil.AnnotationDependsOnSubcharts: `["does-not-exist"]`, + } + + messages := runSequencingLint(t, root) + requireMessage(t, messages, support.ErrorSev, "unknown or disabled subchart") +} + +func TestSequencing_RenderedAnnotationRules(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + templates map[string]string + wantSeverity int + wantSubstring string + }{ + { + name: "partial readiness annotations", + templates: map[string]string{ + "templates/configmap.yaml": manifestYAML("ConfigMap", "partial-readiness", map[string]string{ + releaseutil.AnnotationResourceGroup: "bootstrap", + "helm.sh/readiness-success": `'["{.phase} == \"Ready\""]'`, + }), + }, + wantSeverity: support.ErrorSev, + wantSubstring: "both must be present or absent together", + }, + { + name: "duplicate group assignment", + templates: map[string]string{ + "templates/first.yaml": manifestYAML("ConfigMap", "shared-resource", map[string]string{releaseutil.AnnotationResourceGroup: "bootstrap"}), + "templates/second.yaml": manifestYAML("ConfigMap", "shared-resource", map[string]string{releaseutil.AnnotationResourceGroup: "app"}), + }, + wantSeverity: support.ErrorSev, + wantSubstring: "assigned to multiple resource groups", + }, + { + name: "circular resource-group dependencies", + templates: map[string]string{ + "templates/group-a.yaml": manifestYAML("ConfigMap", "group-a", map[string]string{ + releaseutil.AnnotationResourceGroup: "a", + releaseutil.AnnotationDependsOnResourceGroups: `'["c"]'`, + }), + "templates/group-b.yaml": manifestYAML("ConfigMap", "group-b", map[string]string{ + releaseutil.AnnotationResourceGroup: "b", + releaseutil.AnnotationDependsOnResourceGroups: `'["a"]'`, + }), + "templates/group-c.yaml": manifestYAML("ConfigMap", "group-c", map[string]string{ + releaseutil.AnnotationResourceGroup: "c", + releaseutil.AnnotationDependsOnResourceGroups: `'["b"]'`, + }), + }, + wantSeverity: support.ErrorSev, + wantSubstring: "resource-group circular dependency detected", + }, + { + name: "orphan resource-group reference", + templates: map[string]string{ + "templates/configmap.yaml": manifestYAML("ConfigMap", "orphaned-group", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `'["database"]'`, + }), + }, + wantSeverity: support.ErrorSev, + wantSubstring: `depends-on non-existent group "database"`, + }, + { + name: "malformed depends-on-resource-groups annotation", + templates: map[string]string{ + "templates/configmap.yaml": manifestYAML("ConfigMap", "bad-json", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `not-a-json-array`, + }), + }, + wantSeverity: support.ErrorSev, + wantSubstring: "depends-on", + }, + { + // Both readiness annotations present (so the presence-symmetry + // check passes), but the JSONPath in readiness-success is + // malformed. Presence symmetry alone let this through; the rule + // must validate the expressions compile. + name: "malformed readiness JSONPath with both annotations present", + templates: map[string]string{ + "templates/job.yaml": manifestYAML("Job", "bad-jsonpath", map[string]string{ + "helm.sh/readiness-success": `'["{.status.succeeded >= 1"]'`, + "helm.sh/readiness-failure": `'["{.status.failed} >= 1"]'`, + }), + }, + wantSeverity: support.ErrorSev, + wantSubstring: "malformed", + }, + { + // The comparison value's type IS statically known: an ordering + // operator with a non-numeric literal can never evaluate at + // runtime, so lint flags it as a definite authoring error. + name: "ordering operator with non-numeric readiness value", + templates: map[string]string{ + "templates/job.yaml": manifestYAML("Job", "string-ordering", map[string]string{ + "helm.sh/readiness-success": `'["{.status.phase} > \"Running\""]'`, + "helm.sh/readiness-failure": `'["{.status.failed} >= 1"]'`, + }), + }, + wantSeverity: support.ErrorSev, + wantSubstring: "requires a numeric comparison value", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + messages := runSequencingLint(t, newChart("testchart", tc.templates)) + requireMessage(t, messages, tc.wantSeverity, tc.wantSubstring) + }) + } +} + +func TestSequencing_HookResourcesExcludedFromResourceGroupDAG(t *testing.T) { + t.Parallel() + + // A hook resource carrying sequencing annotations that WOULD form a cycle + // with the non-hook resources must be ignored by the sequencing lint rules, + // exactly as the install path ignores it (SortManifests routes hooks out + // before resource-group parsing). group-b depends on group-a; the hook + // claims group-a depends-on group-b — a cycle only if the hook is counted. + templates := map[string]string{ + "templates/group-a.yaml": manifestYAML("ConfigMap", "group-a", map[string]string{ + releaseutil.AnnotationResourceGroup: "group-a", + }), + "templates/group-b.yaml": manifestYAML("ConfigMap", "group-b", map[string]string{ + releaseutil.AnnotationResourceGroup: "group-b", + releaseutil.AnnotationDependsOnResourceGroups: `'["group-a"]'`, + }), + "templates/hook-job.yaml": manifestYAML("Job", "hook-job", map[string]string{ + "helm.sh/hook": "pre-install", + releaseutil.AnnotationResourceGroup: "group-a", + releaseutil.AnnotationDependsOnResourceGroups: `'["group-b"]'`, + }), + } + + messages := runSequencingLint(t, newChart("hook-chart", templates)) + + for _, m := range messages { + assert.NotContains(t, m.Err.Error(), "circular", + "hook resource must be excluded from the resource-group DAG; got: %v", m.Err) + } +} + +func TestSequencing_NoMessagesWithoutSequencingAnnotations(t *testing.T) { + t.Parallel() + + messages := runSequencingLint(t, newChart("plain-chart", map[string]string{ + "templates/configmap.yaml": manifestYAML("ConfigMap", "plain", nil), + })) + + assert.Empty(t, messages) +} + +func runSequencingLint(t *testing.T, c *chart.Chart) []support.Message { + t.Helper() + + tmpDir := t.TempDir() + require.NoError(t, chartutil.SaveDir(c, tmpDir)) + + linter := support.Linter{ChartDir: filepath.Join(tmpDir, c.Name())} + Sequencing(&linter, "test-namespace", nil) + + return linter.Messages +} + +func requireMessage(t *testing.T, messages []support.Message, severity int, substring string) { + t.Helper() + + for _, message := range messages { + if message.Severity == severity && strings.Contains(message.Err.Error(), substring) { + return + } + } + + t.Fatalf("expected severity %d message containing %q, got %#v", severity, substring, messages) +} + +func newChart(name string, templates map[string]string) *chart.Chart { + files := make([]*common.File, 0, len(templates)) + names := make([]string, 0, len(templates)) + for templateName := range templates { + names = append(names, templateName) + } + sort.Strings(names) + + for _, templateName := range names { + files = append(files, &common.File{ + Name: templateName, + Data: []byte(templates[templateName]), + }) + } + + return &chart.Chart{ + Metadata: &chart.Metadata{ + Name: name, + Version: "0.1.0", + APIVersion: chart.APIVersionV2, + }, + Templates: files, + } +} + +func manifestYAML(kind, name string, annotations map[string]string) string { + if len(annotations) == 0 { + return fmt.Sprintf(`apiVersion: v1 +kind: %s +metadata: + name: %s +data: + key: value +`, kind, name) + } + + keys := make([]string, 0, len(annotations)) + for key := range annotations { + keys = append(keys, key) + } + sort.Strings(keys) + + var annotationBlock strings.Builder + for _, key := range keys { + fmt.Fprintf(&annotationBlock, "\n %s: %s", key, annotations[key]) + } + + return fmt.Sprintf(`apiVersion: v1 +kind: %s +metadata: + name: %s + annotations:%s +data: + key: value +`, kind, name, annotationBlock.String()) +}