msb-public
/
helm
mirror of https://github.com/helm/helm
1
0
Code Issues Projects Releases Wiki Activity

feat(hip-0025): add sequencing lint rules

Browse Source 
Lint validation for subchart and resource-group dependency cycles,
orphan group references, and the both-or-neither readiness annotation
rule.

Refs: HIP-0025
Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com>
pull/32038/head
Rohit Gudi 4 weeks ago
parent 57ef3b41be
commit 8800cd7a3c
No known key found for this signature in database
GPG Key ID: 4D9E5BA7BBE1EB29
4 changed files with 647 additions and 0 deletions
Show Stats Download Patch File Download Diff File

1
pkg/chart/v2/lint/lint.go
Unescape View File

@ -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
}

42
pkg/chart/v2/lint/lint_test.go
Unescape View File

@ -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)
}

256
pkg/chart/v2/lint/rules/sequencing.go
Unescape View File

@ -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)
}

348
pkg/chart/v2/lint/rules/sequencing_test.go
Unescape View File

@ -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())
}
Write Preview
Loading…
Cancel
Save