From 8741e6eee4f36ed6921cc135cb1180bda4899b61 Mon Sep 17 00:00:00 2001 From: Rohit Gudi <50377477+caretak3r@users.noreply.github.com> Date: Thu, 11 Jun 2026 22:39:18 -0400 Subject: [PATCH] feat(hip-0025): add ordered wait CLI flags, template output, and dag command The --wait=ordered and --readiness-timeout flags across install/upgrade/uninstall/rollback, ordered helm template output with resource-group delimiters, and the helm dag debugging command. Refs: HIP-0025 Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com> --- pkg/action/action.go | 4 +- pkg/action/rollback_sequenced_test.go | 40 +++ pkg/action/sequencing.go | 32 -- pkg/action/sequencing_test.go | 172 ---------- pkg/action/uninstall.go | 26 +- pkg/action/uninstall_sequenced_test.go | 152 ++++++++- pkg/chart/v2/lint/rules/sequencing.go | 128 +++---- pkg/chart/v2/lint/rules/sequencing_test.go | 101 ++++++ pkg/chart/v2/util/subchart_dag.go | 13 +- pkg/chart/v2/util/subchart_dag_test.go | 45 ++- pkg/cmd/dag.go | 230 +++++++++++++ pkg/cmd/dag_test.go | 60 ++++ pkg/cmd/flags.go | 74 +++- pkg/cmd/flags_test.go | 108 ++++++ pkg/cmd/get_manifest.go | 5 + pkg/cmd/get_manifest_test.go | 35 ++ pkg/cmd/install.go | 3 +- pkg/cmd/rollback.go | 3 +- pkg/cmd/root.go | 1 + pkg/cmd/template.go | 240 +++++++++---- pkg/cmd/template_test.go | 213 ++++++++++++ .../testdata/output/dag-sequenced-chart.txt | 11 + .../output/dag-sequenced-isolated.txt | 6 + .../output/template-ordered-delimiters.txt | 47 +++ .../output/template-ordered-isolated.txt | 45 +++ .../testcharts/sequenced-chart/Chart.yaml | 9 + .../sequenced-chart/charts/worker/Chart.yaml | 5 + .../worker/templates/aa-worker-configmap.yaml | 8 + .../templates/aa-databases-configmap.yaml | 8 + .../templates/bb-app-configmap.yaml | 9 + .../templates/cc-unsequenced-configmap.yaml | 6 + .../sequenced-hook-chart/Chart.yaml | 5 + .../templates/hook-configmap.yaml | 10 + .../templates/plain-configmap.yaml | 8 + .../sequenced-isolated-chart/Chart.yaml | 5 + .../templates/aa-alpha.yaml | 8 + .../templates/bb-beta.yaml | 9 + .../templates/cc-gamma.yaml | 8 + .../templates/dd-plain.yaml | 6 + pkg/cmd/uninstall.go | 2 +- pkg/cmd/uninstall_test.go | 6 + pkg/cmd/upgrade.go | 4 +- .../custom_readiness_status_reader_test.go | 2 +- pkg/kube/readiness.go | 5 +- pkg/release/v1/sequence/builder.go | 61 +++- pkg/release/v1/sequence/builder_test.go | 318 +++++++++++++++++- pkg/release/v1/sequence/plan.go | 26 +- pkg/release/v1/util/manifest.go | 265 +++++++++++++-- pkg/release/v1/util/manifest_test.go | 256 ++++++++++++++ 49 files changed, 2414 insertions(+), 429 deletions(-) create mode 100644 pkg/cmd/dag.go create mode 100644 pkg/cmd/dag_test.go create mode 100644 pkg/cmd/testdata/output/dag-sequenced-chart.txt create mode 100644 pkg/cmd/testdata/output/dag-sequenced-isolated.txt create mode 100644 pkg/cmd/testdata/output/template-ordered-delimiters.txt create mode 100644 pkg/cmd/testdata/output/template-ordered-isolated.txt create mode 100644 pkg/cmd/testdata/testcharts/sequenced-chart/Chart.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-chart/charts/worker/Chart.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-chart/charts/worker/templates/aa-worker-configmap.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-chart/templates/aa-databases-configmap.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-chart/templates/bb-app-configmap.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-chart/templates/cc-unsequenced-configmap.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-hook-chart/Chart.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-hook-chart/templates/hook-configmap.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-hook-chart/templates/plain-configmap.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-isolated-chart/Chart.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/aa-alpha.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/bb-beta.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/cc-gamma.yaml create mode 100644 pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/dd-plain.yaml diff --git a/pkg/action/action.go b/pkg/action/action.go index 8718c20d7..79015de20 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -518,7 +518,9 @@ func (cfg *Configuration) renderResourcesWithFiles(ctx context.Context, ch *char // output dir is only used by `helm template`. // TODO Helm v5: move this logic to template only as it is not // used by install or upgrade - err = writeToFile(newDir, m.Name, m.Content, fileWritten[m.Name]) + // Strip helm-internal sequencing annotations so files written to + // --output-dir stay apply-able, matching the stdout path. + err = writeToFile(newDir, m.Name, releaseutil.StripHelmInternalAnnotations(m.Content), fileWritten[m.Name]) if err != nil { return hs, b, "", nil, err } diff --git a/pkg/action/rollback_sequenced_test.go b/pkg/action/rollback_sequenced_test.go index 8c258f8bf..494492d16 100644 --- a/pkg/action/rollback_sequenced_test.go +++ b/pkg/action/rollback_sequenced_test.go @@ -496,3 +496,43 @@ func TestRollback_Sequenced_DryRun(t *testing.T) { assert.Empty(t, client.deleteCalls) assert.Equal(t, 2, latestRollbackRelease(t, rollback, "rollback-dry-run").Version) } + +// TestRollback_Sequenced_StorageDecodedChart_Subcharts guards the rollback +// leg of bead xmn: the target revision's chart comes from release storage, +// which drops the loaded dependency tree, so the forward plan must be +// rebuilt from the stored metadata and manifest paths: subchart resources +// first, then the parent's own. +func TestRollback_Sequenced_StorageDecodedChart_Subcharts(t *testing.T) { + client := newRecordingKubeClient() + rollback := newRollbackAction(t, client) + + parent := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/parent.yaml", "parent", nil), + }, withName("parent")) + bar := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/bar.yaml", "bar", nil), + }, withName("bar")) + parent.AddDependency(bar) + parent.Metadata.Dependencies = []*chart.Dependency{{Name: "bar", Enabled: true}} + parent.Metadata.Annotations = map[string]string{"helm.sh/depends-on/subcharts": `["bar"]`} + + seedRollbackRelease(t, rollback, "decoded-rollback", 1, rcommon.StatusSuperseded, storageDecodedChart(t, parent), joinManifestDocs( + sourcedManifest("parent/charts/bar/templates/bar.yaml", configMapManifest("bar", nil)), + sourcedManifest("parent/templates/parent.yaml", configMapManifest("parent", nil)), + ), &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/parent.yaml", "parent", nil), + }, withName("parent")) + seedRollbackRelease(t, rollback, "decoded-rollback", 2, rcommon.StatusDeployed, currentChart, joinManifestDocs( + configMapManifest("parent", nil), + configMapManifest("bar", nil), + ), &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}) + + rollback.Version = 1 + require.NoError(t, rollback.Run("decoded-rollback")) + + // Forward deployment order: the subchart's batch first, then the parent's. + assert.Equal(t, [][]string{{"ConfigMap/bar"}, {"ConfigMap/parent"}}, updateTargets(client.updateCalls)) + assert.Equal(t, [][]string{{"ConfigMap/bar"}, {"ConfigMap/parent"}}, client.waitCalls) +} diff --git a/pkg/action/sequencing.go b/pkg/action/sequencing.go index c8044e1ed..bf1236309 100644 --- a/pkg/action/sequencing.go +++ b/pkg/action/sequencing.go @@ -44,38 +44,6 @@ func computeDeadline(timeout time.Duration) time.Time { return time.Now().Add(timeout) } -// GroupManifestsByDirectSubchart groups manifests by the direct subchart they belong to. -// chartPath is the full path-prefix for the current chart level — at the top level -// it is the chart name (e.g. "parent"); at deeper recursion levels it is the joined -// path through "/charts/" segments (e.g. "parent/charts/sub"). -// The current chart level's own manifests are returned under the empty string key "". -// Direct subcharts are keyed by their immediate directory name under -// "/charts//". Nested grandchildren are grouped under their -// direct subchart parent ("sub"), since nested sequencing is handled recursively. -func GroupManifestsByDirectSubchart(manifests []releaseutil.Manifest, chartPath string) map[string][]releaseutil.Manifest { - result := make(map[string][]releaseutil.Manifest) - if chartPath == "" { - result[""] = append(result[""], manifests...) - return result - } - - chartsPrefix := chartPath + "/charts/" - for _, m := range manifests { - if !strings.HasPrefix(m.Name, chartsPrefix) { - result[""] = append(result[""], m) - continue - } - rest := m.Name[len(chartsPrefix):] - subchartName, _, ok := strings.Cut(rest, "/") - if !ok { - result[""] = append(result[""], m) - continue - } - result[subchartName] = append(result[subchartName], m) - } - return result -} - // buildManifestYAML concatenates the Content fields of the given manifests into a single // YAML stream suitable for passing to KubeClient.Build(). func buildManifestYAML(manifests []releaseutil.Manifest) string { diff --git a/pkg/action/sequencing_test.go b/pkg/action/sequencing_test.go index 2ee5fcabf..f88db3d99 100644 --- a/pkg/action/sequencing_test.go +++ b/pkg/action/sequencing_test.go @@ -374,37 +374,6 @@ func mustRelease(t *testing.T, rel ri.Releaser) *release.Release { return out } -func TestSequencing_GroupManifestsByDirectSubchart(t *testing.T) { - manifests := []releaseutil.Manifest{ - makeTestManifest("parent", "parent/templates/one.yaml", nil), - makeTestManifest("db", "parent/charts/database/templates/one.yaml", nil), - makeTestManifest("cache", "parent/charts/database/charts/cache/templates/one.yaml", nil), - } - - grouped := GroupManifestsByDirectSubchart(manifests, "parent") - - require.Len(t, grouped[""], 1) - require.Len(t, grouped["database"], 2) -} - -// TestSequencing_GroupManifestsByDirectSubchart_Nested verifies that when called -// with a deeper chartPath (i.e., during recursion into a subchart), nested -// grandchildren are routed to the correct subchart key instead of being merged -// into the parent batch. -func TestSequencing_GroupManifestsByDirectSubchart_Nested(t *testing.T) { - manifests := []releaseutil.Manifest{ - makeTestManifest("db-own", "parent/charts/database/templates/one.yaml", nil), - makeTestManifest("cache", "parent/charts/database/charts/cache/templates/one.yaml", nil), - } - - grouped := GroupManifestsByDirectSubchart(manifests, "parent/charts/database") - - require.Len(t, grouped[""], 1, "database's own resources should be under the empty key") - require.Len(t, grouped["cache"], 1, "nested cache subchart should be routed under its own key") - require.Equal(t, "parent/charts/database/templates/one.yaml", grouped[""][0].Name) - require.Equal(t, "parent/charts/database/charts/cache/templates/one.yaml", grouped["cache"][0].Name) -} - func TestSequencing_BuildManifestYAML(t *testing.T) { yaml := buildManifestYAML([]releaseutil.Manifest{ makeTestManifest("one", "chart/templates/one.yaml", nil), @@ -1065,147 +1034,6 @@ func TestSequencedDeployment_ApplyBatch_Update_RespectsContextCancellation(t *te }) } -var findSubchart = lookupSubchartForTest - -func lookupSubchartForTest(chrt *chart.Chart, nameOrAlias string) *chart.Chart { - aliasMap := make(map[string]string) - if chrt.Metadata != nil { - for _, dep := range chrt.Metadata.Dependencies { - effective := dep.Name - if dep.Alias != "" { - effective = dep.Alias - } - aliasMap[dep.Name] = effective - } - } - - for _, dep := range chrt.Dependencies() { - effective := dep.Name() - if alias, ok := aliasMap[dep.Name()]; ok { - effective = alias - } - if effective == nameOrAlias || dep.Name() == nameOrAlias { - return dep - } - } - return nil -} - -func TestFindSubchart(t *testing.T) { - // makeSubchart constructs a chart with the given chart-name, its own - // Metadata.Name, which BuildSubchartDAG and the legacy lookup resolve - // against. - makeSubchart := func(chartName string) *chart.Chart { - return &chart.Chart{ - Metadata: &chart.Metadata{ - APIVersion: "v1", - Name: chartName, - Version: "0.1.0", - }, - } - } - - // makeParent attaches subcharts as dependencies and declares the - // parent's Metadata.Dependencies, which carry the Alias field. - makeParent := func(deps []*chart.Chart, metaDeps []*chart.Dependency) *chart.Chart { - parent := &chart.Chart{ - Metadata: &chart.Metadata{ - APIVersion: "v1", - Name: "parent", - Version: "0.1.0", - Dependencies: metaDeps, - }, - } - for _, d := range deps { - parent.AddDependency(d) - } - return parent - } - - t.Run("resolves by chart name when no alias is declared", func(t *testing.T) { - db := makeSubchart("database") - parent := makeParent( - []*chart.Chart{db}, - []*chart.Dependency{{Name: "database"}}, - ) - - got := findSubchart(parent, "database") - require.NotNil(t, got) - assert.Equal(t, "database", got.Name()) - }) - - t.Run("resolves by alias when alias is declared", func(t *testing.T) { - postgres := makeSubchart("postgres") - parent := makeParent( - []*chart.Chart{postgres}, - []*chart.Dependency{{Name: "postgres", Alias: "db"}}, - ) - - got := findSubchart(parent, "db") - require.NotNil(t, got, "alias lookup should resolve to the underlying chart") - assert.Equal(t, "postgres", got.Name()) - }) - - t.Run("resolves by underlying chart name even when an alias is declared", func(t *testing.T) { - // An alias does not hide the chart's real name. Manifests rendered - // under the chart's actual chart-name path should still resolve. - postgres := makeSubchart("postgres") - parent := makeParent( - []*chart.Chart{postgres}, - []*chart.Dependency{{Name: "postgres", Alias: "db"}}, - ) - - got := findSubchart(parent, "postgres") - require.NotNil(t, got) - assert.Equal(t, "postgres", got.Name()) - }) - - t.Run("alias collides with another chart's real name, first match wins", func(t *testing.T) { - // dep1: chart "foo" aliased as "bar". - // dep2: chart "bar" with no alias. - // Query "bar" must resolve deterministically. Current contract: - // iteration order over Dependencies() is preserved, so the first - // dep whose effective name (alias or real) matches the query wins. - foo := makeSubchart("foo") - bar := makeSubchart("bar") - parent := makeParent( - []*chart.Chart{foo, bar}, - []*chart.Dependency{ - {Name: "foo", Alias: "bar"}, - {Name: "bar"}, - }, - ) - - got := findSubchart(parent, "bar") - require.NotNil(t, got, "collision must resolve, not return nil") - assert.Equal(t, "foo", got.Name(), - "first matching dependency wins; aliased 'foo' is declared before raw 'bar'") - - // And the raw name "foo" must still resolve to chart "foo" even - // though its effective name has been shifted by the alias. - gotFoo := findSubchart(parent, "foo") - require.NotNil(t, gotFoo) - assert.Equal(t, "foo", gotFoo.Name()) - }) - - t.Run("returns nil when not found", func(t *testing.T) { - db := makeSubchart("database") - parent := makeParent( - []*chart.Chart{db}, - []*chart.Dependency{{Name: "database"}}, - ) - - assert.Nil(t, findSubchart(parent, "nonexistent")) - }) - - t.Run("returns nil when parent has no dependencies", func(t *testing.T) { - parent := &chart.Chart{ - Metadata: &chart.Metadata{APIVersion: "v1", Name: "parent", Version: "0.1.0"}, - } - assert.Nil(t, findSubchart(parent, "anything")) - }) -} - func TestStripSequencingAnnotations(t *testing.T) { // makeInfo constructs a minimal *resource.Info backed by an unstructured // ConfigMap. stripSequencingAnnotations only needs meta.Accessor to work, diff --git a/pkg/action/uninstall.go b/pkg/action/uninstall.go index 1fb2fc5d7..90c1cceaf 100644 --- a/pkg/action/uninstall.go +++ b/pkg/action/uninstall.go @@ -331,7 +331,16 @@ func (e *joinedErrors) Unwrap() []error { // deleteRelease deletes the release and returns list of delete resources and manifests that were kept in the deletion process. func (u *Uninstall) deleteRelease(rel *release.Release, waiter kube.Waiter) (kube.ResourceList, string, []error) { if rel.IsSequenced() { - return u.deleteReleaseSequenced(rel, waiter) + resources, kept, errs, planned := u.deleteReleaseSequenced(rel, waiter) + if planned { + return resources, kept, errs + } + // The ordered plan could not be rebuilt from the stored release (e.g. + // a corrupted record). Never leave the release stuck in uninstalling + // over ordering: deletion completeness is what matters, so degrade to + // the standard unsequenced deletion below with a warning, exactly as + // upgrade does for removed-resource deletion (deleteRemovedFromOldRelease). + u.cfg.Logger().Warn("unable to rebuild the ordered uninstall plan from the stored release; deleting resources without sequencing", slog.Any("error", joinErrors(errs, "; "))) } var errs []error @@ -385,10 +394,15 @@ func (u *Uninstall) deleteRelease(rel *release.Release, waiter kube.Waiter) (kub // dependencies, which the DAG-walking deleter used to leak (bead i42). // Keep-policy filtering applies to the parsed stream before the plan is built, // so kept resources are never part of the plan. -func (u *Uninstall) deleteReleaseSequenced(rel *release.Release, waiter kube.Waiter) (kube.ResourceList, string, []error) { +// +// The final return value reports whether a plan was built and ordered deletion +// ran (regardless of deletion errors). false means planning failed before any +// deletion was attempted; the caller falls back to unsequenced deletion so a +// planning failure can never leave the release stuck in uninstalling. +func (u *Uninstall) deleteReleaseSequenced(rel *release.Release, waiter kube.Waiter) (kube.ResourceList, string, []error, bool) { manifests, err := sequence.ParseStoredManifests(rel.Manifest) if err != nil { - return nil, rel.Manifest, []error{fmt.Errorf("corrupted release record. You must manually delete the resources: %w", err)} + return nil, "", []error{fmt.Errorf("parsing stored release manifest: %w", err)}, false } filesToKeep, filesToDelete := filterManifestsToKeep(manifests) @@ -402,7 +416,7 @@ func (u *Uninstall) deleteReleaseSequenced(rel *release.Release, waiter kube.Wai plan, err := sequence.Build(rel.Chart, filesToDelete) if err != nil { - return nil, kept.String(), []error{fmt.Errorf("building sequencing plan for uninstall: %w", err)} + return nil, "", []error{fmt.Errorf("building sequencing plan for uninstall: %w", err)}, false } logPlanWarnings(u.cfg.Logger(), plan) @@ -418,10 +432,10 @@ func (u *Uninstall) deleteReleaseSequenced(rel *release.Release, waiter kube.Wai kept.WriteString(skipped) } if len(errs) > 0 { - return allDeleted, kept.String(), errs + return allDeleted, kept.String(), errs, true } } - return allDeleted, kept.String(), nil + return allDeleted, kept.String(), nil, true } // verifyOwnedForDelete verifies which of the built resources are owned by the diff --git a/pkg/action/uninstall_sequenced_test.go b/pkg/action/uninstall_sequenced_test.go index 511c5a088..b43a07db3 100644 --- a/pkg/action/uninstall_sequenced_test.go +++ b/pkg/action/uninstall_sequenced_test.go @@ -18,6 +18,7 @@ package action import ( "bytes" + "encoding/json" "fmt" "log/slog" "strings" @@ -640,7 +641,7 @@ func TestUninstall_Sequenced_SkipsUnownedResources(t *testing.T) { } res, err := uninstall.Run(rel.Name) - is.NoError(err) + require.NoError(t, err) logOutput := logBuffer.String() is.Contains(logOutput, "skipping delete of resource not owned by this release") @@ -663,3 +664,152 @@ func TestUninstall_Sequenced_DeadlineExceededFailsFast(t *testing.T) { require.Len(t, errs, 1) assert.Contains(t, errs[0].Error(), "timed out") } + +// storageDecodedChart encodes and decodes a chart the way the release storage +// codec does (json.Marshal in pkg/storage/driver). The memory driver used by +// actionConfigFixture shares release pointers, so without this helper action +// tests never see the shape real (secrets/configmaps/sql) storage produces: +// a chart whose loaded dependency tree is gone. +func storageDecodedChart(t *testing.T, c *chart.Chart) *chart.Chart { + t.Helper() + + encoded, err := json.Marshal(c) + require.NoError(t, err) + decoded := &chart.Chart{} + require.NoError(t, json.Unmarshal(encoded, decoded)) + require.Empty(t, decoded.Dependencies(), "release codec is expected to drop the loaded dependency tree") + return decoded +} + +// TestUninstall_Sequenced_StorageDecodedChart_NestedSubcharts reproduces the +// live bead-xmn failure: `helm uninstall` of a sequenced nested-subchart +// release read from REAL storage (which drops the chart's loaded dependency +// tree) must still delete in exact reverse deployment order instead of +// failing to build the subchart DAG and stranding the release in +// status=uninstalling. +func TestUninstall_Sequenced_StorageDecodedChart_NestedSubcharts(t *testing.T) { + client := newRecordingKubeClient().withoutBuildClients() + uninstall := newSequencedUninstallAction(t, client) + + parent := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/parent.yaml", "parent", nil), + }, withName("parent")) + bar := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/bar.yaml", "bar", nil), + }, withName("bar")) + nginx := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/nginx.yaml", "nginx", nil), + }, withName("nginx")) + + // Post-ProcessDependencies shape, as install stores it: enabled entries + // only, and the parent-resources annotation that made the live uninstall + // fail with `references unknown or disabled subchart "bar"`. + bar.AddDependency(nginx) + bar.Metadata.Dependencies = []*chart.Dependency{{Name: "nginx", Enabled: true}} + parent.AddDependency(bar) + parent.Metadata.Dependencies = []*chart.Dependency{{Name: "bar", Enabled: true}} + parent.Metadata.Annotations = map[string]string{"helm.sh/depends-on/subcharts": `["bar"]`} + + rel := newUninstallRelease( + "decoded-nested-uninstall", + storageDecodedChart(t, parent), + joinManifestDocs( + sourcedManifest("parent/charts/bar/charts/nginx/templates/nginx.yaml", configMapManifest("nginx", nil)), + sourcedManifest("parent/charts/bar/templates/bar.yaml", configMapManifest("bar", nil)), + sourcedManifest("parent/templates/parent.yaml", configMapManifest("parent", nil)), + ), + &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}, + ) + seedUninstallRelease(t, uninstall, rel) + + _, err := uninstall.Run(rel.Name) + require.NoError(t, err) + + assert.Equal(t, [][]string{{"ConfigMap/parent"}, {"ConfigMap/bar"}, {"ConfigMap/nginx"}}, client.deleteCalls) + assert.Equal(t, client.deleteCalls, client.deleteWaitCalls) +} + +// TestUninstall_Sequenced_StorageDecodedChart_VendoredSubchart extends the +// bead-i42 guard across the storage round-trip: a vendored-but-undeclared +// subchart of a storage-decoded chart is still deleted, after the parent's +// own resources and before declared subcharts. +func TestUninstall_Sequenced_StorageDecodedChart_VendoredSubchart(t *testing.T) { + client := newRecordingKubeClient().withoutBuildClients() + uninstall := newSequencedUninstallAction(t, client) + + parent := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/parent.yaml", "parent", nil), + }, withName("parent")) + parent.Metadata.Dependencies = []*chart.Dependency{{Name: "database", Enabled: true}} + parent.Metadata.Annotations = map[string]string{"helm.sh/depends-on/subcharts": `["database"]`} + + rel := newUninstallRelease( + "decoded-vendored-uninstall", + storageDecodedChart(t, parent), + joinManifestDocs( + sourcedManifest("parent/charts/database/templates/database.yaml", configMapManifest("database", nil)), + sourcedManifest("parent/charts/vendored/templates/vendored.yaml", configMapManifest("vendored", nil)), + sourcedManifest("parent/templates/parent.yaml", configMapManifest("parent", nil)), + ), + &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}, + ) + seedUninstallRelease(t, uninstall, rel) + + _, err := uninstall.Run(rel.Name) + require.NoError(t, err) + + assert.Equal(t, [][]string{{"ConfigMap/parent"}, {"ConfigMap/vendored"}, {"ConfigMap/database"}}, client.deleteCalls) + assert.Equal(t, client.deleteCalls, client.deleteWaitCalls) +} + +// TestUninstall_Sequenced_PlanFailure_FallsBackUnsequenced locks the +// stuck-release fix: when the ordered plan cannot be rebuilt from the stored +// release (here: a resource-group cycle, which Build treats as fatal), the +// uninstall must warn and degrade to the standard unsequenced deletion so the +// release reaches status=uninstalled instead of stranding in uninstalling. +func TestUninstall_Sequenced_PlanFailure_FallsBackUnsequenced(t *testing.T) { + client := newRecordingKubeClient().withoutBuildClients() + logBuffer := &bytes.Buffer{} + + cfg := actionConfigFixture(t) + cfg.SetLogger(slog.NewTextHandler(logBuffer, &slog.HandlerOptions{Level: slog.LevelWarn})) + cfg.KubeClient = client + + uninstall := NewUninstall(cfg) + uninstall.DisableHooks = true + uninstall.KeepHistory = true + uninstall.Timeout = 5 * time.Minute + uninstall.WaitStrategy = kube.OrderedWaitStrategy + + manifest := joinManifestDocs( + configMapManifest("alpha", map[string]string{ + releaseutil.AnnotationResourceGroup: "a", + releaseutil.AnnotationDependsOnResourceGroups: `["b"]`, + }), + configMapManifest("beta", map[string]string{ + releaseutil.AnnotationResourceGroup: "b", + releaseutil.AnnotationDependsOnResourceGroups: `["a"]`, + }), + ) + rel := newUninstallRelease( + "cyclic-uninstall", + buildChartWithTemplates(nil, withName("cyclic-uninstall")), + manifest, + nil, + ) + rel.Sequenced = true + seedUninstallRelease(t, uninstall, rel) + + _, err := uninstall.Run(rel.Name) + require.NoError(t, err) + + assert.Contains(t, logBuffer.String(), "deleting resources without sequencing") + require.Len(t, client.deleteCalls, 1) + assert.ElementsMatch(t, []string{"ConfigMap/alpha", "ConfigMap/beta"}, client.deleteCalls[0]) + + reli, err := uninstall.cfg.Releases.Get(rel.Name, rel.Version) + require.NoError(t, err) + stored, err := releaserToV1Release(reli) + require.NoError(t, err) + assert.Equal(t, rcommon.StatusUninstalled, stored.Info.Status) +} diff --git a/pkg/chart/v2/lint/rules/sequencing.go b/pkg/chart/v2/lint/rules/sequencing.go index ed3948945..8655af0f0 100644 --- a/pkg/chart/v2/lint/rules/sequencing.go +++ b/pkg/chart/v2/lint/rules/sequencing.go @@ -18,8 +18,10 @@ package rules import ( "context" + "errors" "fmt" - "path" + "maps" + "slices" "strings" "sigs.k8s.io/yaml" @@ -33,6 +35,7 @@ import ( "helm.sh/helm/v4/pkg/engine" "helm.sh/helm/v4/pkg/kube" release "helm.sh/helm/v4/pkg/release/v1" + "helm.sh/helm/v4/pkg/release/v1/sequence" releaseutil "helm.sh/helm/v4/pkg/release/v1/util" ) @@ -40,50 +43,46 @@ import ( 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 + return // chart load errors are reported by other lint rules } - - // 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 - } + // Render failures are reported by the Templates rule; manifests stays nil + // so Build still validates the top-level subchart DAG (preserving the old + // pre-render validateSubchartSequencing coverage for broken-template charts). + manifests := collectRenderedManifests(linter, c, namespace, values) - dag, err := chartutil.BuildSubchartDAG(c) + plan, err := sequence.Build(c, manifests) if err != nil { - return err - } - if _, err := dag.GetBatches(); err != nil { - return fmt.Errorf("subchart circular dependency detected: %w", err) + // Build's fatal classes are exactly what fails at install time: + // subchart/resource-group cycles, unknown depends-on refs, malformed + // helm.sh/depends-on/subcharts, multi-group assignment. + linter.RunLinterRule(support.ErrorSev, linter.ChartDir, err) + return } - 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 + for _, w := range plan.Warnings { + path := w.ChartPath + if path == "" { + path = linter.ChartDir + } + switch w.Kind { + case sequence.WarningKindResourceGroupDemotion: + // Runtime recovers by demoting; the chart author must fix these. + linter.RunLinterRule(support.ErrorSev, path, errors.New(w.Message)) + case sequence.WarningKindPartialReadiness: + // Already reported per-template (with better context) by + // validateReadinessAnnotations during collection. + default: // isolated group, undeclared/unresolved subchart + linter.RunLinterRule(support.WarningSev, path, errors.New(w.Message)) + } } +} +func collectRenderedManifests(linter *support.Linter, c *chart.Chart, namespace string, values map[string]any) []releaseutil.Manifest { options := common.ReleaseOptions{ Name: "test-release", Namespace: namespace, @@ -92,12 +91,12 @@ func validateRenderedSequencingAnnotations(linter *support.Linter, c *chart.Char coalescedValues, err := commonutil.CoalesceValues(c, values) if err != nil { - return + return nil } valuesToRender, err := commonutil.ToRenderValues(c, coalescedValues, options, caps) if err != nil { - return + return nil } var renderEngine engine.Engine @@ -106,17 +105,17 @@ func validateRenderedSequencingAnnotations(linter *support.Linter, c *chart.Char renderedContentMap, err := renderEngine.RenderWithContext(context.Background(), c, valuesToRender) if err != nil { // Template rendering errors are already reported by the Templates lint rule. - return + return nil } - manifestsByChart := make(map[string][]releaseutil.Manifest) - for templatePath, content := range renderedContentMap { + var manifests []releaseutil.Manifest + for _, templatePath := range slices.Sorted(maps.Keys(renderedContentMap)) { + content := renderedContentMap[templatePath] if strings.TrimSpace(content) == "" { continue } - chartPath := renderedTemplateChartPath(templatePath) - for _, manifest := range parseRenderedManifests(content) { + for _, manifest := range parseRenderedManifests(templatePath, 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 @@ -127,13 +126,11 @@ func validateRenderedSequencingAnnotations(linter *support.Linter, c *chart.Char continue } validateReadinessAnnotations(linter, templatePath, manifest) - manifestsByChart[chartPath] = append(manifestsByChart[chartPath], manifest) + manifests = append(manifests, manifest) } } - for chartPath, manifests := range manifestsByChart { - validateResourceGroupAnnotations(linter, chartPath, manifests) - } + return manifests } // isHookManifest reports whether a rendered manifest is a Helm hook. Hooks @@ -147,19 +144,12 @@ func isHookManifest(manifest releaseutil.Manifest) bool { 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 { +func parseRenderedManifests(templatePath, content string) []releaseutil.Manifest { rawManifests := releaseutil.SplitManifests(content) manifests := make([]releaseutil.Manifest, 0, len(rawManifests)) - for manifestName, raw := range rawManifests { + for _, manifestName := range slices.Sorted(maps.Keys(rawManifests)) { + raw := rawManifests[manifestName] if strings.TrimSpace(raw) == "" { continue } @@ -169,12 +159,8 @@ func parseRenderedManifests(content string) []releaseutil.Manifest { continue } - if head.Metadata != nil && head.Metadata.Name != "" { - manifestName = head.Metadata.Name - } - manifests = append(manifests, releaseutil.Manifest{ - Name: manifestName, + Name: templatePath, Content: raw, Head: &head, }) @@ -220,30 +206,6 @@ func validateReadinessAnnotations(linter *support.Linter, templatePath string, m } } -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 diff --git a/pkg/chart/v2/lint/rules/sequencing_test.go b/pkg/chart/v2/lint/rules/sequencing_test.go index 478bdaf7d..62c9f5f03 100644 --- a/pkg/chart/v2/lint/rules/sequencing_test.go +++ b/pkg/chart/v2/lint/rules/sequencing_test.go @@ -114,6 +114,107 @@ func TestSequencing_SubchartAnnotationOrphanWithNoDependencies(t *testing.T) { requireMessage(t, messages, support.ErrorSev, "unknown or disabled subchart") } +func TestSequencing_NestedSubchartCircularDependency(t *testing.T) { + t.Parallel() + + // The CHILD's Chart.yaml declares a depends-on cycle between its two + // grandchildren. HEAD's lint only validates the ROOT's subchart DAG, so + // this chart lints clean but fails fatally at install (bead lkx). + child := newChart("child", map[string]string{ + "templates/cm.yaml": manifestYAML("ConfigMap", "child-cm", nil), + }) + child.Metadata.Dependencies = []*chart.Dependency{ + {Name: "grandchild-a", Version: "0.1.0", Repository: "file://charts/grandchild-a", DependsOn: []string{"grandchild-b"}}, + {Name: "grandchild-b", Version: "0.1.0", Repository: "file://charts/grandchild-b", DependsOn: []string{"grandchild-a"}}, + } + child.SetDependencies(newChart("grandchild-a", nil), newChart("grandchild-b", nil)) + + root := newChart("testchart", nil) + root.Metadata.Dependencies = []*chart.Dependency{ + {Name: "child", Version: "0.1.0", Repository: "file://charts/child"}, + } + root.SetDependencies(child) + + messages := runSequencingLint(t, root) + requireMessage(t, messages, support.ErrorSev, "subchart circular dependency detected") + requireMessage(t, messages, support.ErrorSev, "testchart/charts/child") +} + +func TestSequencing_NestedSubchartUnknownDependsOnRef(t *testing.T) { + t.Parallel() + + child := newChart("child", map[string]string{ + "templates/cm.yaml": manifestYAML("ConfigMap", "child-cm", nil), + }) + child.Metadata.Dependencies = []*chart.Dependency{ + {Name: "grandchild-a", Version: "0.1.0", Repository: "file://charts/grandchild-a", DependsOn: []string{"missing"}}, + } + child.SetDependencies(newChart("grandchild-a", nil)) + + root := newChart("testchart", nil) + root.Metadata.Dependencies = []*chart.Dependency{ + {Name: "child", Version: "0.1.0", Repository: "file://charts/child"}, + } + root.SetDependencies(child) + + messages := runSequencingLint(t, root) + requireMessage(t, messages, support.ErrorSev, `depends-on unknown or disabled subchart "missing"`) + requireMessage(t, messages, support.ErrorSev, "testchart/charts/child") +} + +func TestSequencing_NestedResourceGroupCircularDependency(t *testing.T) { + t.Parallel() + + child := newChart("child", map[string]string{ + "templates/a.yaml": manifestYAML("ConfigMap", "group-a", map[string]string{ + releaseutil.AnnotationResourceGroup: "a", + releaseutil.AnnotationDependsOnResourceGroups: `'["b"]'`, + }), + "templates/b.yaml": manifestYAML("ConfigMap", "group-b", map[string]string{ + releaseutil.AnnotationResourceGroup: "b", + releaseutil.AnnotationDependsOnResourceGroups: `'["a"]'`, + }), + }) + root := newChart("testchart", nil) + root.Metadata.Dependencies = []*chart.Dependency{ + {Name: "child", Version: "0.1.0", Repository: "file://charts/child"}, + } + root.SetDependencies(child) + + messages := runSequencingLint(t, root) + requireMessage(t, messages, support.ErrorSev, "resource-group circular dependency detected") + requireMessage(t, messages, support.ErrorSev, "testchart/charts/child") +} + +func TestSequencing_IsolatedGroupWarns(t *testing.T) { + t.Parallel() + + // Two groups, no depends-on edges between them: runtime demotes both to + // the unsequenced batch with a warning. Lint surfaces that demotion as a + // WARNING (not an error — the chart still deploys). + messages := runSequencingLint(t, newChart("testchart", map[string]string{ + "templates/a.yaml": manifestYAML("ConfigMap", "cm-a", map[string]string{releaseutil.AnnotationResourceGroup: "a"}), + "templates/b.yaml": manifestYAML("ConfigMap", "cm-b", map[string]string{releaseutil.AnnotationResourceGroup: "b"}), + })) + requireMessage(t, messages, support.WarningSev, "isolated") +} + +func TestSequencing_UndeclaredSubchartWarns(t *testing.T) { + t.Parallel() + + // Vendored subchart present in charts/ but absent from Chart.yaml + // dependencies: runtime deploys it after declared subcharts with a + // warning. Lint mirrors that as a WARNING. + sub := newChart("vendored", map[string]string{ + "templates/cm.yaml": manifestYAML("ConfigMap", "vendored-cm", nil), + }) + root := newChart("testchart", nil) + root.SetDependencies(sub) // deliberately NOT in root.Metadata.Dependencies + + messages := runSequencingLint(t, root) + requireMessage(t, messages, support.WarningSev, "not declared in Chart.yaml") +} + func TestSequencing_RenderedAnnotationRules(t *testing.T) { t.Parallel() diff --git a/pkg/chart/v2/util/subchart_dag.go b/pkg/chart/v2/util/subchart_dag.go index e614b0870..697e6ca33 100644 --- a/pkg/chart/v2/util/subchart_dag.go +++ b/pkg/chart/v2/util/subchart_dag.go @@ -61,6 +61,16 @@ func BuildSubchartDAG(c *chart.Chart) (*DAG, error) { loaded[sub.Name()] = true } + // Charts decoded from release storage have no loaded dependency tree: + // Chart.dependencies is unexported, so the release codec (json.Marshal in + // pkg/storage/driver) drops it. Metadata.Dependencies survives, and per + // the ProcessDependencies precondition above it was already pruned to the + // enabled set with aliases and depends-on references rewritten. Enabled + // is only ever set true by ProcessDependencies, so it marks entries that + // were deployable at install time; freshly parsed Chart.yaml entries + // default to Enabled=false and stay excluded here. + trustMetadata := len(c.Dependencies()) == 0 + // Each loaded subchart becomes a DAG node keyed by its effective name. nodes := make(map[string]bool, len(c.Metadata.Dependencies)) for _, dep := range c.Metadata.Dependencies { @@ -68,7 +78,8 @@ func BuildSubchartDAG(c *chart.Chart) (*DAG, error) { continue } eff := effectiveDependencyName(dep) - if !loaded[eff] || nodes[eff] { + trustedFromMetadata := trustMetadata && dep.Enabled + if (!loaded[eff] && !trustedFromMetadata) || nodes[eff] { continue } nodes[eff] = true diff --git a/pkg/chart/v2/util/subchart_dag_test.go b/pkg/chart/v2/util/subchart_dag_test.go index d5afc887b..b2c568eb7 100644 --- a/pkg/chart/v2/util/subchart_dag_test.go +++ b/pkg/chart/v2/util/subchart_dag_test.go @@ -230,25 +230,52 @@ func TestBuildSubchartDAG_NestedSubcharts(t *testing.T) { assertBatches(t, nested, [][]string{{"cache"}, {"worker"}}) } -// TestBuildSubchartDAG_MetadataOnlyNoLoadedDeps locks in the post-rewrite -// contract: when c.Metadata.Dependencies is non-empty but c.Dependencies() -// is empty (e.g., chart loaded but ProcessDependencies disabled everything), -// the DAG should have no nodes and produce no error. -func TestBuildSubchartDAG_MetadataOnlyNoLoadedDeps(t *testing.T) { +// TestBuildSubchartDAG_StorageDecodedMetadataTrusted: a chart decoded from +// release storage has Metadata.Dependencies (pruned to the enabled set and +// alias-rewritten by ProcessDependencies before it was stored, Enabled=true) +// but an EMPTY loaded-dependency tree — the release codec drops the +// unexported Chart.dependencies field. Those metadata entries must be +// trusted, or uninstall/rollback of any sequenced release fails (bead xmn). +func TestBuildSubchartDAG_StorageDecodedMetadataTrusted(t *testing.T) { t.Parallel() c := &chart.Chart{ Metadata: &chart.Metadata{ Name: "parent", Dependencies: []*chart.Dependency{ - {Name: "ghost", Enabled: true}, + {Name: "db", Enabled: true}, + {Name: "app", Enabled: true, DependsOn: []string{"db"}}, + }, + Annotations: map[string]string{ + AnnotationDependsOnSubcharts: `["app"]`, + }, + }, + } + // Note: no AddDependency call — c.Dependencies() is empty, as after a + // storage round-trip. + + assertBatches(t, c, [][]string{{"db"}, {"app"}}) +} + +// TestBuildSubchartDAG_MetadataOnlyNotEnabled_Ignored: freshly parsed +// Chart.yaml entries default to Enabled=false — only ProcessDependencies sets +// it true. Without loaded dependencies (e.g. lint before `helm dependency +// build`) such entries stay excluded, preserving the pre-existing contract +// for unprocessed charts. +func TestBuildSubchartDAG_MetadataOnlyNotEnabled_Ignored(t *testing.T) { + t.Parallel() + + c := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "parent", + Dependencies: []*chart.Dependency{ + {Name: "ghost"}, }, }, } - // Note: no AddDependency call — c.Dependencies() is empty. batches := batchesForChart(t, c) - assert.Empty(t, batches, "no loaded deps should yield empty DAG") + assert.Empty(t, batches, "unprocessed metadata-only deps should yield empty DAG") } // TestBuildSubchartDAG_AnnotationReferencesUnloadedDep verifies that an @@ -498,7 +525,7 @@ func TestProcessDependencies_PlainAndAliasedSameChart(t *testing.T) { ) err := ProcessDependencies(ambiguous, map[string]any{}) require.Error(t, err) - assert.ErrorContains(t, err, `ambiguous subchart reference "svc"`) + require.ErrorContains(t, err, `ambiguous subchart reference "svc"`) byAlias := pipelineChart( pipelineDependency("svc", ""), diff --git a/pkg/cmd/dag.go b/pkg/cmd/dag.go new file mode 100644 index 000000000..45cff74b2 --- /dev/null +++ b/pkg/cmd/dag.go @@ -0,0 +1,230 @@ +/* +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 cmd + +import ( + "errors" + "fmt" + "io" + "slices" + "sort" + "strings" + + "github.com/spf13/cobra" + + "helm.sh/helm/v4/pkg/action" + "helm.sh/helm/v4/pkg/chart/common" + chart "helm.sh/helm/v4/pkg/chart/v2" + "helm.sh/helm/v4/pkg/cli/values" + "helm.sh/helm/v4/pkg/cmd/require" + "helm.sh/helm/v4/pkg/release/v1/sequence" + releaseutil "helm.sh/helm/v4/pkg/release/v1/util" +) + +const dagDesc = ` +Print the resource sequencing DAG (directed acyclic graph) for a chart. + +This is a development and troubleshooting command for HIP-0025 sequencing. It +loads the chart, evaluates conditional dependencies against the provided values, +renders templates locally, and prints the deployment order that +'helm install --wait=ordered' would use: + + - Subchart deployment batches, derived from Chart.yaml dependency 'depends-on' + fields and the 'helm.sh/depends-on/subcharts' annotation. + - Resource-group batches per chart level, derived from the + 'helm.sh/resource-group' and 'helm.sh/depends-on/resource-groups' + annotations on rendered manifests. + +Cycles in either DAG are reported as errors. Manifests that lack a +'helm.sh/resource-group' annotation, or whose group was demoted because of a +missing dependency, are listed as "Unsequenced" and would be deployed after the +sequenced batches. + +Hooks are not part of any sequencing DAG (HIP-0025) and are omitted from the +output. No cluster connection is required; the chart is rendered with +client-side dry-run. +` + +func newDagCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { + client := action.NewInstall(cfg) + valueOpts := &values.Options{} + var kubeVersion string + var extraAPIs []string + + cmd := &cobra.Command{ + Use: "dag CHART", + Short: "print the resource sequencing DAG for a chart", + Long: dagDesc, + Args: require.MinimumNArgs(1), + ValidArgsFunction: func(_ *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + return compInstall(args, toComplete, client) + }, + RunE: func(_ *cobra.Command, args []string) error { + if kubeVersion != "" { + parsed, err := common.ParseKubeVersion(kubeVersion) + if err != nil { + return fmt.Errorf("invalid kube version %q: %w", kubeVersion, err) + } + client.KubeVersion = parsed + } + + registryClient, err := newRegistryClient(out, client.CertFile, client.KeyFile, client.CaFile, + client.InsecureSkipTLSVerify, client.PlainHTTP, client.Username, client.Password) + if err != nil { + return fmt.Errorf("missing registry client: %w", err) + } + client.SetRegistryClient(registryClient) + + // Render the chart locally without touching the cluster. Hooks are not + // sequenced per HIP-0025, so we suppress them from the rendered output + // to keep the DAG view focused on install-phase resources. + client.DryRunStrategy = action.DryRunClient + client.ReleaseName = "release-name" + client.Replace = true + client.APIVersions = common.VersionSet(extraAPIs) + client.DisableHooks = true + + rel, err := runInstall(args, client, valueOpts, out) + if err != nil { + return err + } + if rel == nil || rel.Chart == nil { + return errors.New("no chart rendered") + } + + return printSequencingDAG(rel.Chart, strings.TrimSpace(rel.Manifest), out) + }, + } + + f := cmd.Flags() + addValueOptionsFlags(f, valueOpts) + addChartPathOptionsFlags(f, &client.ChartPathOptions) + f.StringVar(&kubeVersion, "kube-version", "", "Kubernetes version used for Capabilities.KubeVersion") + f.StringSliceVarP(&extraAPIs, "api-versions", "a", []string{}, "Kubernetes api versions used for Capabilities.APIVersions (multiple can be specified)") + f.BoolVar(&client.DependencyUpdate, "dependency-update", false, "update dependencies if they are missing before printing the DAG") + + return cmd +} + +// printSequencingDAG walks a processed chart and its rendered manifest stream, +// printing the subchart deployment batches and per-chart resource-group batches +// in the same order 'helm install --wait=ordered' would deploy them. +func printSequencingDAG(chrt *chart.Chart, manifest string, out io.Writer) error { + var manifests []releaseutil.Manifest + if manifest != "" { + parsed, err := sequence.ParseStoredManifests(manifest) + if err != nil { + return fmt.Errorf("parsing rendered manifests: %w", err) + } + manifests = parsed + } + plan, err := sequence.Build(chrt, manifests) + if err != nil { + return err + } + logSequencePlanWarnings(plan) + + levelByPath := make(map[string]*sequence.ChartLevel, len(plan.Levels)) + for i := range plan.Levels { + levelByPath[plan.Levels[i].Path] = &plan.Levels[i] + } + + groupBatchesByPath := make(map[string][]sequence.Batch) + unsequencedByPath := make(map[string]sequence.Batch) + for _, batch := range plan.Batches { + switch batch.Kind { + case sequence.BatchKindGroups: + groupBatchesByPath[batch.ChartPath] = append(groupBatchesByPath[batch.ChartPath], batch) + case sequence.BatchKindUnsequenced: + unsequencedByPath[batch.ChartPath] = batch + } + } + + var printLevel func(level *sequence.ChartLevel) + printLevel = func(level *sequence.ChartLevel) { + indent := strings.Repeat(" ", level.Depth) + fmt.Fprintf(out, "%sChart: %s\n", indent, level.Path) + + if len(level.SubchartBatches) == 0 { + fmt.Fprintf(out, "%s Subchart batches: (none)\n", indent) + } else { + fmt.Fprintf(out, "%s Subchart batches:\n", indent) + for i, batch := range level.SubchartBatches { + fmt.Fprintf(out, "%s Batch %d: %s\n", indent, i+1, strings.Join(batch, ", ")) + } + } + + resourceIndent := indent + " " + groupBatches := groupBatchesByPath[level.Path] + if len(groupBatches) == 0 { + fmt.Fprintf(out, "%sResource-group batches: (none)\n", resourceIndent) + } else { + fmt.Fprintf(out, "%sResource-group batches:\n", resourceIndent) + for i, batch := range groupBatches { + names := make([]string, 0, len(batch.Groups)) + for _, group := range batch.Groups { + names = append(names, group.Name) + } + fmt.Fprintf(out, "%s Batch %d: %s\n", resourceIndent, i+1, strings.Join(names, ", ")) + } + } + + if batch, ok := unsequencedByPath[level.Path]; ok { + names := make([]string, 0, len(batch.Manifests())) + for _, manifest := range batch.Manifests() { + names = append(names, unsequencedResourceLabel(manifest)) + } + sort.Strings(names) + fmt.Fprintf(out, "%sUnsequenced (deployed last): %s\n", resourceIndent, strings.Join(names, ", ")) + } + + printChild := func(name string) { + if slices.Contains(level.Unresolved, name) { + fmt.Fprintf(out, "%s (subchart %q not found in chart dependencies)\n", indent, name) + return + } + if child := levelByPath[level.Path+"/charts/"+name]; child != nil { + printLevel(child) + } + } + + for _, batch := range level.SubchartBatches { + for _, name := range batch { + printChild(name) + } + } + for _, name := range level.Undeclared { + fmt.Fprintf(out, "%s Undeclared subchart %q (deployed unsequenced):\n", indent, name) + printChild(name) + } + } + + if len(plan.Levels) > 0 { + printLevel(&plan.Levels[0]) + } + return nil +} + +func unsequencedResourceLabel(m releaseutil.Manifest) string { + if m.Head != nil && m.Head.Metadata != nil && m.Head.Metadata.Name != "" { + if m.Head.Kind != "" { + return fmt.Sprintf("%s/%s", m.Head.Kind, m.Head.Metadata.Name) + } + return m.Head.Metadata.Name + } + return m.Name +} diff --git a/pkg/cmd/dag_test.go b/pkg/cmd/dag_test.go new file mode 100644 index 000000000..28a392f6a --- /dev/null +++ b/pkg/cmd/dag_test.go @@ -0,0 +1,60 @@ +/* +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 cmd + +import ( + "fmt" + "strings" + "testing" +) + +func TestDagCmd(t *testing.T) { + chartPath := "testdata/testcharts/sequenced-chart" + + tests := []cmdTestCase{ + { + name: "sequenced chart prints subchart and resource-group batches", + cmd: fmt.Sprintf("dag '%s'", chartPath), + golden: "output/dag-sequenced-chart.txt", + }, + { + name: "sequenced chart demotes isolated groups", + cmd: "dag 'testdata/testcharts/sequenced-isolated-chart'", + golden: "output/dag-sequenced-isolated.txt", + }, + } + runTestCmd(t, tests) +} + +func TestDagCmd_RequiresChartArg(t *testing.T) { + _, _, err := executeActionCommandC(storageFixture(), "dag") + if err == nil { + t.Fatal("expected error when chart argument is missing") + } +} + +func TestDagCmd_NonexistentChart(t *testing.T) { + _, _, err := executeActionCommandC(storageFixture(), "dag testdata/testcharts/does-not-exist") + if err == nil { + t.Fatal("expected error for nonexistent chart path") + } + if !strings.Contains(err.Error(), "does-not-exist") && + !strings.Contains(strings.ToLower(err.Error()), "no such file") && + !strings.Contains(strings.ToLower(err.Error()), "not found") { + t.Fatalf("expected error to mention the missing chart, got: %v", err) + } +} diff --git a/pkg/cmd/flags.go b/pkg/cmd/flags.go index 61db07883..83c680919 100644 --- a/pkg/cmd/flags.go +++ b/pkg/cmd/flags.go @@ -25,6 +25,7 @@ import ( "path/filepath" "sort" "strings" + "time" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -85,42 +86,99 @@ func defaultCLIWaitOptions() []kube.WaitOption { } } -type waitValue kube.WaitStrategy +func AddOrderedWaitFlag(cmd *cobra.Command, wait *kube.WaitStrategy) { + cmd.Flags().Var( + newOrderedWaitValue(kube.HookOnlyStrategy, wait), + "wait", + "wait until resources are ready (up to --timeout). Use '--wait' alone for 'watcher' strategy, or specify one of: 'watcher', 'hookOnly', 'legacy', 'ordered'. Default when flag is omitted: 'hookOnly'.", + ) + cmd.Flags().Lookup("wait").NoOptDefVal = string(kube.StatusWatcherStrategy) +} + +type waitValue struct { + wait *kube.WaitStrategy + allowOrdered bool +} func newWaitValue(defaultValue kube.WaitStrategy, ws *kube.WaitStrategy) *waitValue { + return newConfiguredWaitValue(defaultValue, ws, false) +} + +func newOrderedWaitValue(defaultValue kube.WaitStrategy, ws *kube.WaitStrategy) *waitValue { + return newConfiguredWaitValue(defaultValue, ws, true) +} + +func newConfiguredWaitValue(defaultValue kube.WaitStrategy, ws *kube.WaitStrategy, allowOrdered bool) *waitValue { *ws = defaultValue - return (*waitValue)(ws) + return &waitValue{wait: ws, allowOrdered: allowOrdered} } func (ws *waitValue) String() string { - if ws == nil { + if ws == nil || ws.wait == nil { return "" } - return string(*ws) + return string(*ws.wait) } func (ws *waitValue) Set(s string) error { switch s { case string(kube.StatusWatcherStrategy), string(kube.LegacyStrategy), string(kube.HookOnlyStrategy): - *ws = waitValue(s) + *ws.wait = kube.WaitStrategy(s) + return nil + case string(kube.OrderedWaitStrategy): + if !ws.allowOrdered { + break + } + *ws.wait = kube.WaitStrategy(s) return nil case "true": slog.Warn("--wait=true is deprecated (boolean value) and can be replaced with --wait=watcher") - *ws = waitValue(kube.StatusWatcherStrategy) + *ws.wait = kube.StatusWatcherStrategy return nil case "false": slog.Warn("--wait=false is deprecated (boolean value) and can be replaced with --wait=hookOnly") - *ws = waitValue(kube.HookOnlyStrategy) + *ws.wait = kube.HookOnlyStrategy return nil default: - return fmt.Errorf("invalid wait input %q. Valid inputs are %s, %s, and %s", s, kube.StatusWatcherStrategy, kube.HookOnlyStrategy, kube.LegacyStrategy) } + + return fmt.Errorf("invalid wait input %q. Valid inputs are %s", s, formatWaitInputs(ws.allowOrdered)) } func (ws *waitValue) Type() string { return "WaitStrategy" } +func addReadinessTimeoutFlag(f *pflag.FlagSet, readinessTimeout *time.Duration) { + // Default 0 means "unset": the per-batch default of 1m is applied at the + // point of use on the ordered path (install/upgrade/rollback). Defaulting + // the flag itself to 1m would make the readiness-timeout<=timeout check + // reject any plain run with --timeout < 1m, even without --wait=ordered. + f.DurationVar(readinessTimeout, "readiness-timeout", 0, "per-batch timeout when --wait=ordered is used; each resource batch must become ready within this duration (defaults to 1m when unset, and must not exceed --timeout). \"Ready\" is determined by kstatus signals for the resource kind (Deployment/StatefulSet/Pod/etc.) or by helm.sh/readiness-success and helm.sh/readiness-failure annotations when set; vanilla Jobs require --wait-for-jobs for the per-batch readiness gate to apply") +} + +func formatWaitInputs(allowOrdered bool) string { + valid := []string{ + string(kube.StatusWatcherStrategy), + string(kube.HookOnlyStrategy), + string(kube.LegacyStrategy), + } + if allowOrdered { + valid = append(valid, string(kube.OrderedWaitStrategy)) + } + + switch len(valid) { + case 0: + return "" + case 1: + return valid[0] + case 2: + return fmt.Sprintf("%s and %s", valid[0], valid[1]) + default: + return fmt.Sprintf("%s, and %s", strings.Join(valid[:len(valid)-1], ", "), valid[len(valid)-1]) + } +} + func addChartPathOptionsFlags(f *pflag.FlagSet, c *action.ChartPathOptions) { f.StringVar(&c.Version, "version", "", "specify a version constraint for the chart version to use. This constraint can be a specific tag (e.g. 1.1.1) or it may reference a valid range (e.g. ^2.0.0). If this is not specified, the latest version is used") f.BoolVar(&c.Verify, "verify", false, "verify the package before using it") diff --git a/pkg/cmd/flags_test.go b/pkg/cmd/flags_test.go index 09dc61638..84705e187 100644 --- a/pkg/cmd/flags_test.go +++ b/pkg/cmd/flags_test.go @@ -18,6 +18,7 @@ package cmd import ( "fmt" + "io" "testing" "time" @@ -99,6 +100,113 @@ func outputFlagCompletionTest(t *testing.T, cmdName string) { runTestCmd(t, tests) } +func TestWaitFlag(t *testing.T) { + t.Run("install accepts ordered wait", func(t *testing.T) { + cmd := newInstallCmd(&action.Configuration{}, io.Discard) + require.NoError(t, cmd.ParseFlags([]string{"--wait=ordered"})) + require.Equal(t, "ordered", cmd.Flags().Lookup("wait").Value.String()) + }) + + t.Run("upgrade accepts ordered wait", func(t *testing.T) { + cmd := newUpgradeCmd(&action.Configuration{}, io.Discard) + require.NoError(t, cmd.ParseFlags([]string{"--wait=ordered"})) + require.Equal(t, "ordered", cmd.Flags().Lookup("wait").Value.String()) + }) + + t.Run("template accepts ordered wait", func(t *testing.T) { + cmd := newTemplateCmd(&action.Configuration{}, io.Discard) + require.NoError(t, cmd.ParseFlags([]string{"--wait=ordered"})) + require.Equal(t, "ordered", cmd.Flags().Lookup("wait").Value.String()) + }) + + t.Run("rollback accepts ordered wait", func(t *testing.T) { + cmd := newRollbackCmd(&action.Configuration{}, io.Discard) + require.NoError(t, cmd.ParseFlags([]string{"--wait=ordered"})) + require.Equal(t, "ordered", cmd.Flags().Lookup("wait").Value.String()) + }) + + t.Run("uninstall accepts ordered wait", func(t *testing.T) { + cmd := newUninstallCmd(&action.Configuration{}, io.Discard) + require.NoError(t, cmd.ParseFlags([]string{"--wait=ordered"})) + require.Equal(t, "ordered", cmd.Flags().Lookup("wait").Value.String()) + }) +} + +func TestReadinessTimeout(t *testing.T) { + t.Run("install registers readiness-timeout unset by default", func(t *testing.T) { + cmd := newInstallCmd(&action.Configuration{}, io.Discard) + flag := cmd.Flags().Lookup("readiness-timeout") + require.NotNil(t, flag) + require.Equal(t, "0s", flag.DefValue) + require.NoError(t, cmd.ParseFlags([]string{"--readiness-timeout=30s"})) + require.Equal(t, "30s", flag.Value.String()) + }) + + t.Run("upgrade registers readiness-timeout unset by default", func(t *testing.T) { + cmd := newUpgradeCmd(&action.Configuration{}, io.Discard) + flag := cmd.Flags().Lookup("readiness-timeout") + require.NotNil(t, flag) + require.Equal(t, "0s", flag.DefValue) + require.NoError(t, cmd.ParseFlags([]string{"--readiness-timeout=30s"})) + require.Equal(t, "30s", flag.Value.String()) + }) + + t.Run("template and non-applicable commands do not register readiness-timeout", func(t *testing.T) { + require.Nil(t, newTemplateCmd(&action.Configuration{}, io.Discard).Flags().Lookup("readiness-timeout")) + require.Nil(t, newUninstallCmd(&action.Configuration{}, io.Discard).Flags().Lookup("readiness-timeout")) + }) + + t.Run("rollback registers readiness-timeout unset by default", func(t *testing.T) { + cmd := newRollbackCmd(&action.Configuration{}, io.Discard) + flag := cmd.Flags().Lookup("readiness-timeout") + require.NotNil(t, flag) + require.Equal(t, "0s", flag.DefValue) + require.NoError(t, cmd.ParseFlags([]string{"--readiness-timeout=30s"})) + require.Equal(t, "30s", flag.Value.String()) + }) + + t.Run("install rejects readiness-timeout longer than timeout", func(t *testing.T) { + _, _, err := executeActionCommand("install timed-install testdata/testcharts/empty --wait=ordered --timeout 30s --readiness-timeout 60s") + require.Error(t, err) + require.Contains(t, err.Error(), "--readiness-timeout (1m0s) must not exceed --timeout (30s)") + }) + + t.Run("upgrade rejects readiness-timeout longer than timeout", func(t *testing.T) { + releaseName := "timed-upgrade" + relMock, ch, chartPath := prepareMockRelease(t, releaseName) + store := storageFixture() + require.NoError(t, store.Create(relMock(releaseName, 1, ch))) + + _, _, err := executeActionCommandC(store, fmt.Sprintf("upgrade %s '%s' --wait=ordered --timeout 30s --readiness-timeout 60s", releaseName, chartPath)) + require.Error(t, err) + require.Contains(t, err.Error(), "--readiness-timeout (1m0s) must not exceed --timeout (30s)") + }) + + t.Run("upgrade --install fallback carries readiness-timeout to install", func(t *testing.T) { + // Regression for hip-0025-vw1: the upgrade --install copy block dropped + // ReadinessTimeout, so first-time installs silently ignored the flag. + // With an empty store the upgrade falls back to install; if the value + // is carried over, install's readiness-timeout<=timeout guard fires. + store := storageFixture() + _, _, err := executeActionCommandC(store, "upgrade missing-release testdata/testcharts/empty --install --wait=ordered --timeout 30s --readiness-timeout 60s") + require.Error(t, err) + require.Contains(t, err.Error(), "--readiness-timeout (1m0s) must not exceed --timeout (30s)") + }) + + t.Run("readiness-timeout is ignored without ordered wait", func(t *testing.T) { + _, _, err := executeActionCommand("install plain-install testdata/testcharts/empty --readiness-timeout 30s") + require.NoError(t, err) + }) + + // Regression for hip-0025: a plain install with --timeout below the old + // 1m readiness-timeout default and no --readiness-timeout / --wait=ordered + // must not be rejected by the readiness-timeout<=timeout guard. + t.Run("plain install with sub-minute timeout and no readiness flag succeeds", func(t *testing.T) { + _, _, err := executeActionCommand("install short-timeout testdata/testcharts/empty --timeout 30s") + require.NoError(t, err) + }) +} + func TestPostRendererFlagSetOnce(t *testing.T) { cfg := action.Configuration{} client := action.NewInstall(&cfg) diff --git a/pkg/cmd/get_manifest.go b/pkg/cmd/get_manifest.go index a7873e587..3785de061 100644 --- a/pkg/cmd/get_manifest.go +++ b/pkg/cmd/get_manifest.go @@ -34,6 +34,11 @@ This command fetches the generated manifest for a given release. A manifest is a YAML-encoded representation of the Kubernetes resources that were generated from this release's chart(s). If a chart is dependent on other charts, those resources will also be included in the manifest. + +The manifest is printed exactly as stored, including Helm-internal sequencing +annotations such as helm.sh/depends-on/resource-groups. They are part of the +release record and drive sequenced uninstall/rollback. Use 'helm template' for +apply-ready output. ` func newGetManifestCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { diff --git a/pkg/cmd/get_manifest_test.go b/pkg/cmd/get_manifest_test.go index cfb5215bf..a20097d28 100644 --- a/pkg/cmd/get_manifest_test.go +++ b/pkg/cmd/get_manifest_test.go @@ -17,6 +17,7 @@ limitations under the License. package cmd import ( + "strings" "testing" release "helm.sh/helm/v4/pkg/release/v1" @@ -37,6 +38,40 @@ func TestGetManifest(t *testing.T) { runTestCmd(t, tests) } +func TestGetManifestPrintsStoredManifestVerbatim(t *testing.T) { + const annotationLine = ` helm.sh/depends-on/resource-groups: '["db"]'` + manifest := `apiVersion: v1 +kind: ConfigMap +metadata: + name: sequenced + annotations: + helm.sh/resource-group: app +` + annotationLine + ` +data: + key: value +` + rel := release.Mock(&release.MockReleaseOptions{Name: "sequenced"}) + rel.Manifest = manifest + + store := storageFixture() + if err := store.Create(rel); err != nil { + t.Fatal(err) + } + _, out, err := executeActionCommandC(store, "get manifest sequenced") + if err != nil { + t.Fatal(err) + } + + // Invariant pin: get manifest prints the stored release record verbatim, + // including sequencing annotations that template output strips. + if !strings.Contains(out, "helm.sh/depends-on/resource-groups") { + t.Fatalf("expected stored sequencing annotation key in output:\n%s", out) + } + if !strings.Contains(out, annotationLine) { + t.Fatalf("expected exact stored sequencing annotation line in output:\n%s", out) + } +} + func TestGetManifestCompletion(t *testing.T) { checkReleaseCompletion(t, "get manifest", false) } diff --git a/pkg/cmd/install.go b/pkg/cmd/install.go index 182b4a7ff..4a9e5a19c 100644 --- a/pkg/cmd/install.go +++ b/pkg/cmd/install.go @@ -174,6 +174,7 @@ func newInstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { f := cmd.Flags() addInstallFlags(cmd, f, client, valueOpts) + addReadinessTimeoutFlag(f, &client.ReadinessTimeout) // hide-secret is not available in all places the install flags are used so // it is added separately f.BoolVar(&client.HideSecret, "hide-secret", false, "hide Kubernetes Secrets when also using the --dry-run flag") @@ -233,7 +234,7 @@ func addInstallFlags(cmd *cobra.Command, f *pflag.FlagSet, client *action.Instal addValueOptionsFlags(f, valueOpts) addChartPathOptionsFlags(f, &client.ChartPathOptions) - AddWaitFlag(cmd, &client.WaitStrategy) + AddOrderedWaitFlag(cmd, &client.WaitStrategy) cmd.MarkFlagsMutuallyExclusive("force-replace", "force-conflicts") cmd.MarkFlagsMutuallyExclusive("force", "force-conflicts") diff --git a/pkg/cmd/rollback.go b/pkg/cmd/rollback.go index 62d0be4af..6f939d354 100644 --- a/pkg/cmd/rollback.go +++ b/pkg/cmd/rollback.go @@ -101,7 +101,8 @@ func newRollbackCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { f.BoolVar(&client.CleanupOnFail, "cleanup-on-fail", false, "allow deletion of new resources created in this rollback when rollback fails") f.IntVar(&client.MaxHistory, "history-max", settings.MaxHistory, "limit the maximum number of revisions saved per release. Use 0 for no limit") addDryRunFlag(cmd) - AddWaitFlag(cmd, &client.WaitStrategy) + AddOrderedWaitFlag(cmd, &client.WaitStrategy) + addReadinessTimeoutFlag(f, &client.ReadinessTimeout) cmd.MarkFlagsMutuallyExclusive("force-replace", "force-conflicts") cmd.MarkFlagsMutuallyExclusive("force", "force-conflicts") diff --git a/pkg/cmd/root.go b/pkg/cmd/root.go index dbedca026..ea594b5b5 100644 --- a/pkg/cmd/root.go +++ b/pkg/cmd/root.go @@ -266,6 +266,7 @@ func newRootCmdWithConfig(actionConfig *action.Configuration, out io.Writer, arg cmd.AddCommand( // chart commands newCreateCmd(out), + newDagCmd(actionConfig, out), newDependencyCmd(actionConfig, out), newPullCmd(actionConfig, out), newShowCmd(actionConfig, out), diff --git a/pkg/cmd/template.go b/pkg/cmd/template.go index 5645ab33d..1d533c602 100644 --- a/pkg/cmd/template.go +++ b/pkg/cmd/template.go @@ -22,6 +22,7 @@ import ( "fmt" "io" "io/fs" + "log/slog" "os" "path/filepath" "regexp" @@ -35,8 +36,11 @@ import ( "helm.sh/helm/v4/pkg/action" "helm.sh/helm/v4/pkg/chart/common" + chart "helm.sh/helm/v4/pkg/chart/v2" "helm.sh/helm/v4/pkg/cli/values" "helm.sh/helm/v4/pkg/cmd/require" + "helm.sh/helm/v4/pkg/kube" + "helm.sh/helm/v4/pkg/release/v1/sequence" releaseutil "helm.sh/helm/v4/pkg/release/v1/util" ) @@ -105,6 +109,7 @@ func newTemplateCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { client.Replace = true // Skip the name check client.APIVersions = common.VersionSet(extraAPIs) client.IncludeCRDs = includeCrds + orderedTemplateOutput := client.WaitStrategy == kube.OrderedWaitStrategy && len(showFiles) == 0 && client.OutputDir == "" rel, err := runInstall(args, client, valueOpts, out) if err != nil && !settings.Debug { @@ -118,87 +123,108 @@ func newTemplateCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { // We ignore a potential error here because, when the --debug flag was specified, // we always want to print the YAML, even if it is not valid. The error is still returned afterwards. if rel != nil { - var manifests bytes.Buffer - fmt.Fprintln(&manifests, strings.TrimSpace(rel.Manifest)) - if !client.DisableHooks { - fileWritten := make(map[string]bool) - for _, m := range rel.Hooks { - if skipTests && isTestHook(m) { - continue - } - if client.OutputDir == "" { - fmt.Fprintf(&manifests, "---\n# Source: %s\n%s\n", m.Path, m.Manifest) - } else { - newDir := client.OutputDir - if client.UseReleaseName { - newDir = filepath.Join(client.OutputDir, client.ReleaseName) - } - _, err := os.Stat(filepath.Join(newDir, m.Path)) - if err == nil { - fileWritten[m.Path] = true - } - - err = writeToFile(newDir, m.Path, m.Manifest, fileWritten[m.Path]) - if err != nil { - return err + orderedRendered := false + if orderedTemplateOutput { + if renderErr := renderOrderedTemplate(rel.Chart, strings.TrimSpace(rel.Manifest), out); renderErr != nil { + // Honor the --debug contract: always print the manifests, even if + // ordered rendering fails (e.g., a document fails YAML structural + // parsing). Fall back to the flat path with a stderr warning. + fmt.Fprintf(os.Stderr, "WARNING: ordered template rendering failed (%v); falling back to flat output\n", renderErr) + } else { + orderedRendered = true + if !client.DisableHooks { + for _, m := range rel.Hooks { + if skipTests && isTestHook(m) { + continue + } + fmt.Fprintf(out, "---\n# Source: %s\n%s\n", m.Path, releaseutil.StripHelmInternalAnnotations(m.Manifest)) } } } } - - // if we have a list of files to render, then check that each of the - // provided files exists in the chart. - if len(showFiles) > 0 { - // This is necessary to ensure consistent manifest ordering when using --show-only - // with globs or directory names. - splitManifests := releaseutil.SplitManifests(manifests.String()) - manifestsKeys := make([]string, 0, len(splitManifests)) - for k := range splitManifests { - manifestsKeys = append(manifestsKeys, k) - } - sort.Sort(releaseutil.BySplitManifestsOrder(manifestsKeys)) - - manifestNameRegex := regexp.MustCompile("# Source: [^/]+/(.+)") - var manifestsToRender []string - for _, f := range showFiles { - missing := true - // Use linux-style filepath separators to unify user's input path - f = filepath.ToSlash(f) - for _, manifestKey := range manifestsKeys { - manifest := splitManifests[manifestKey] - submatch := manifestNameRegex.FindStringSubmatch(manifest) - if len(submatch) == 0 { + if !orderedRendered { + var manifests bytes.Buffer + fmt.Fprintln(&manifests, strings.TrimSpace(releaseutil.StripHelmInternalAnnotations(rel.Manifest))) + if !client.DisableHooks { + fileWritten := make(map[string]bool) + for _, m := range rel.Hooks { + if skipTests && isTestHook(m) { continue } - manifestName := submatch[1] - // manifest.Name is rendered using linux-style filepath separators on Windows as - // well as macOS/linux. - manifestPathSplit := strings.Split(manifestName, "/") - // manifest.Path is connected using linux-style filepath separators on Windows as - // well as macOS/linux - manifestPath := strings.Join(manifestPathSplit, "/") - - // if the filepath provided matches a manifest path in the - // chart, render that manifest - if matched, _ := filepath.Match(f, manifestPath); !matched { - continue + if client.OutputDir == "" { + fmt.Fprintf(&manifests, "---\n# Source: %s\n%s\n", m.Path, releaseutil.StripHelmInternalAnnotations(m.Manifest)) + } else { + newDir := client.OutputDir + if client.UseReleaseName { + newDir = filepath.Join(client.OutputDir, client.ReleaseName) + } + _, err := os.Stat(filepath.Join(newDir, m.Path)) + if err == nil { + fileWritten[m.Path] = true + } + + err = writeToFile(newDir, m.Path, releaseutil.StripHelmInternalAnnotations(m.Manifest), fileWritten[m.Path]) + if err != nil { + return err + } } - manifestsToRender = append(manifestsToRender, manifest) - missing = false } - if missing { - if installErr != nil && settings.Debug { - // assume the manifest itself is too malformed to be rendered - return installErr + } + + // if we have a list of files to render, then check that each of the + // provided files exists in the chart. + if len(showFiles) > 0 { + // This is necessary to ensure consistent manifest ordering when using --show-only + // with globs or directory names. + splitManifests := releaseutil.SplitManifests(manifests.String()) + manifestsKeys := make([]string, 0, len(splitManifests)) + for k := range splitManifests { + manifestsKeys = append(manifestsKeys, k) + } + sort.Sort(releaseutil.BySplitManifestsOrder(manifestsKeys)) + + manifestNameRegex := regexp.MustCompile("# Source: [^/]+/(.+)") + var manifestsToRender []string + for _, f := range showFiles { + missing := true + // Use linux-style filepath separators to unify user's input path + f = filepath.ToSlash(f) + for _, manifestKey := range manifestsKeys { + manifest := splitManifests[manifestKey] + submatch := manifestNameRegex.FindStringSubmatch(manifest) + if len(submatch) == 0 { + continue + } + manifestName := submatch[1] + // manifest.Name is rendered using linux-style filepath separators on Windows as + // well as macOS/linux. + manifestPathSplit := strings.Split(manifestName, "/") + // manifest.Path is connected using linux-style filepath separators on Windows as + // well as macOS/linux + manifestPath := strings.Join(manifestPathSplit, "/") + + // if the filepath provided matches a manifest path in the + // chart, render that manifest + if matched, _ := filepath.Match(f, manifestPath); !matched { + continue + } + manifestsToRender = append(manifestsToRender, manifest) + missing = false + } + if missing { + if installErr != nil && settings.Debug { + // assume the manifest itself is too malformed to be rendered + return installErr + } + return fmt.Errorf("could not find template %s in chart", f) } - return fmt.Errorf("could not find template %s in chart", f) } + for _, m := range manifestsToRender { + fmt.Fprintf(out, "---\n%s\n", m) + } + } else { + fmt.Fprintf(out, "%s", manifests.String()) } - for _, m := range manifestsToRender { - fmt.Fprintf(out, "---\n%s\n", m) - } - } else { - fmt.Fprintf(out, "%s", manifests.String()) } } @@ -229,6 +255,80 @@ func newTemplateCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { return cmd } +func renderOrderedTemplate(chrt *chart.Chart, manifest string, out io.Writer) error { + if manifest == "" { + return nil + } + + manifests, err := sequence.ParseStoredManifests(manifest) + if err != nil { + // Return the parse error so the caller falls back to the flat-output + // path, which strips Helm-internal annotations before emitting. Writing + // the raw manifest here would re-emit stripped sequencing annotations + // (e.g. helm.sh/depends-on/resource-groups) and break the invariant that + // `helm template` output stays directly apply-able. No output has been + // written to `out` yet at this point, so the fallback cannot duplicate. + return err + } + plan, err := sequence.Build(chrt, manifests) + if err != nil { + // Return the plan error so the caller falls back to the flat-output + // path, preserving `helm template`'s apply-ready annotation stripping + // contract while still surfacing cycles or invalid multi-group resources. + return err + } + logSequencePlanWarnings(plan) + + // Render into a buffer so we can normalize trailing whitespace to match + // the flat path, which TrimSpaces the whole manifest blob then writes a + // single trailing newline (template.go flat branch). Per-manifest emission + // would otherwise leave one trailing blank line after the final document, + // breaking the HIP-0025 byte-for-byte backwards-compat guarantee for charts + // with no sequencing annotations. + var buf bytes.Buffer + for _, batch := range plan.Batches { + switch batch.Kind { + case sequence.BatchKindGroups: + for _, group := range batch.Groups { + fmt.Fprintf(&buf, "## START resource-group: %s %s\n", sequence.DisplayPath(batch.ChartPath), group.Name) + for _, manifest := range group.Manifests { + writeOrderedManifest(&buf, manifest.Content) + } + fmt.Fprintf(&buf, "## END resource-group: %s %s\n", sequence.DisplayPath(batch.ChartPath), group.Name) + } + case sequence.BatchKindUnsequenced: + for _, manifest := range batch.Manifests() { + writeOrderedManifest(&buf, manifest.Content) + } + } + } + _, err = fmt.Fprintln(out, strings.TrimRight(buf.String(), "\n")) + return err +} + +// logSequencePlanWarnings surfaces non-fatal sequencing-plan warnings with the +// same shape as the action layer's logPlanWarnings. +func logSequencePlanWarnings(plan *sequence.Plan) { + for _, w := range plan.Warnings { + slog.Warn("sequencing: "+w.Message, "chart", w.ChartPath) + } +} + +// writeOrderedManifest emits a single manifest document with the same +// inter-document whitespace as the flat `helm template` path +// (pkg/action/action.go), which writes "---\n# Source: %s\n%s\n" per +// manifest using renderer-supplied content that always ends with a newline. +// SplitManifests strips trailing newlines from intermediate chunks, so we +// normalize to exactly one trailing newline here before adding the format's +// own trailing "\n" — producing "---\nCONTENT\n\n" between docs and keeping +// `helm template` byte-identical to default mode for charts with no +// sequencing annotations (HIP-0025 backwards-compat guarantee, S04-05). +func writeOrderedManifest(out io.Writer, content string) { + stripped := releaseutil.StripHelmInternalAnnotations(content) + stripped = strings.TrimRight(stripped, "\n") + "\n" + fmt.Fprintf(out, "---\n%s\n", stripped) +} + func isTestHook(h *release.Hook) bool { return slices.Contains(h.Events, release.HookTest) } diff --git a/pkg/cmd/template_test.go b/pkg/cmd/template_test.go index d508a0505..69f8a8d80 100644 --- a/pkg/cmd/template_test.go +++ b/pkg/cmd/template_test.go @@ -17,9 +17,17 @@ limitations under the License. package cmd import ( + "bytes" "fmt" + "os" "path/filepath" + "strings" "testing" + + "github.com/stretchr/testify/require" + + chart "helm.sh/helm/v4/pkg/chart/v2" + "helm.sh/helm/v4/pkg/release/v1/sequence" ) var chartPath = "testdata/testcharts/subchart" @@ -172,10 +180,215 @@ func TestTemplateCmd(t *testing.T) { cmd: fmt.Sprintf("template '%s' -f %s/extra_values.yaml", chartPath, chartPath), golden: "output/template-subchart-cm-set-file.txt", }, + { + name: "template with ordered wait strategy shows resource group delimiters", + cmd: "template --wait=ordered 'testdata/testcharts/sequenced-chart'", + golden: "output/template-ordered-delimiters.txt", + }, + { + name: "template with ordered wait strategy demotes isolated groups", + cmd: "template --wait=ordered 'testdata/testcharts/sequenced-isolated-chart'", + golden: "output/template-ordered-isolated.txt", + }, } runTestCmd(t, tests) } +func TestTemplateWithoutOrderedWaitHasNoDelimiters(t *testing.T) { + _, out, err := executeActionCommand("template 'testdata/testcharts/sequenced-chart'") + require.NoError(t, err) + require.NotContains(t, out, "## START resource-group:") + require.NotContains(t, out, "## END resource-group:") + require.Contains(t, out, "# Source: sequenced-chart/charts/worker/templates/aa-worker-configmap.yaml") +} + +// TestTemplateOrderedBackwardsCompat asserts the HIP-0025 backwards-compat +// invariant: for a chart with no sequencing annotations, `helm template` and +// `helm template --wait=ordered` produce the same manifest body (after +// stripping the `## START`/`## END` resource-group delimiter lines that only +// the ordered path emits). This is the unit-test analogue of harness scenario +// S04-05; it regressed twice — once when the ordered path's per-manifest +// emission used tighter inter-document whitespace than the flat path, and once +// when the fix for that left an extra trailing blank line at EOF. The trailing +// whitespace is deliberately NOT trimmed here so this test catches both — the +// shell harness's `$()` substitution masks trailing-newline drift, so a stricter +// comparison belongs at the unit level. +func TestTemplateOrderedBackwardsCompat(t *testing.T) { + const chartPath = "testdata/testcharts/alpine" + _, flat, err := executeActionCommand("template t " + chartPath) + require.NoError(t, err) + _, ordered, err := executeActionCommand("template t " + chartPath + " --wait=ordered") + require.NoError(t, err) + + // Strip only the marker lines the ordered path adds; preserve all other + // whitespace including the trailing newline structure. + stripMarkers := func(s string) string { + lines := strings.Split(s, "\n") + out := lines[:0] + for _, l := range lines { + if strings.HasPrefix(l, "## START resource-group:") || + strings.HasPrefix(l, "## END resource-group:") { + continue + } + out = append(out, l) + } + return strings.Join(out, "\n") + } + + require.Equal(t, stripMarkers(flat), stripMarkers(ordered), + "ordered template output must be byte-identical (incl. trailing newline) to flat output for charts without sequencing annotations") +} + +// TestTemplateStripsHelmInternalAnnotations asserts that `helm template` output +// never contains the multi-slash internal annotation key +// `helm.sh/depends-on/resource-groups` — its presence in the K8s API would +// fail annotation-key validation and break `helm template | kubectl apply -f -`. +// Valid sibling keys (single-slash) like `helm.sh/resource-group` must survive. +func TestTemplateStripsHelmInternalAnnotations(t *testing.T) { + const internalKey = "helm.sh/depends-on/resource-groups" + const siblingKey = "helm.sh/resource-group" + + t.Run("flat path", func(t *testing.T) { + _, out, err := executeActionCommand("template 'testdata/testcharts/sequenced-chart'") + require.NoError(t, err) + require.NotContains(t, out, internalKey, "internal annotation must be stripped from flat template output") + require.Contains(t, out, siblingKey, "valid-key sibling annotation must be preserved") + }) + + t.Run("ordered path", func(t *testing.T) { + _, out, err := executeActionCommand("template --wait=ordered 'testdata/testcharts/sequenced-chart'") + require.NoError(t, err) + require.NotContains(t, out, internalKey, "internal annotation must be stripped from ordered template output") + require.Contains(t, out, siblingKey, "valid-key sibling annotation must be preserved") + require.Contains(t, out, "## START resource-group:", "ordered output must still emit group delimiters") + }) + + t.Run("output-dir path", func(t *testing.T) { + dir := t.TempDir() + _, _, err := executeActionCommand(fmt.Sprintf("template 'testdata/testcharts/sequenced-chart' --output-dir '%s'", dir)) + require.NoError(t, err) + files := readOutputDirManifests(t, dir) + require.NotContains(t, files, internalKey, "internal annotation must be stripped from --output-dir files") + require.Contains(t, files, siblingKey, "valid-key sibling annotation must be preserved in --output-dir files") + }) + + t.Run("output-dir hooks path", func(t *testing.T) { + dir := t.TempDir() + _, _, err := executeActionCommand(fmt.Sprintf("template 'testdata/testcharts/sequenced-hook-chart' --output-dir '%s'", dir)) + require.NoError(t, err) + hook, err := os.ReadFile(filepath.Join(dir, "sequenced-hook-chart", "templates", "hook-configmap.yaml")) + require.NoError(t, err) + require.NotContains(t, string(hook), internalKey, "internal annotation must be stripped from hook files under --output-dir") + require.Contains(t, string(hook), siblingKey, "valid-key sibling annotation must be preserved in hook files") + }) +} + +// readOutputDirManifests concatenates every file under an --output-dir tree so +// tests can assert on the rendered file contents as a whole. +func readOutputDirManifests(t *testing.T, dir string) string { + t.Helper() + var sb strings.Builder + err := filepath.WalkDir(dir, func(path string, d os.DirEntry, err error) error { + if err != nil || d.IsDir() { + return err + } + b, err := os.ReadFile(path) + if err != nil { + return err + } + sb.Write(b) + sb.WriteString("\n") + return nil + }) + require.NoError(t, err) + return sb.String() +} + +func TestTemplateOrderedMatchesPlan(t *testing.T) { + manifest := `# Source: parent/templates/aa-alpha.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-alpha + annotations: + helm.sh/resource-group: alpha +--- +# Source: parent/templates/bb-beta.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-beta + annotations: + helm.sh/resource-group: beta + helm.sh/depends-on/resource-groups: '["alpha"]' +--- +# Source: parent/templates/cc-gamma.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-gamma + annotations: + helm.sh/resource-group: gamma +--- +# Source: parent/templates/dd-plain.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-plain +--- +# Source: parent/charts/vendored/templates/aa-vendored.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-vendored + annotations: + helm.sh/resource-group: vendored +` + chrt := &chart.Chart{Metadata: &chart.Metadata{Name: "parent"}} + manifests, err := sequence.ParseStoredManifests(manifest) + require.NoError(t, err) + plan, err := sequence.Build(chrt, manifests) + require.NoError(t, err) + + var out bytes.Buffer + require.NoError(t, renderOrderedTemplate(chrt, manifest, &out)) + require.Equal(t, orderedTemplateSequenceFromPlan(plan), orderedTemplateSequenceFromOutput(out.String())) +} + +func orderedTemplateSequenceFromPlan(plan *sequence.Plan) []string { + var ordered []string + for _, batch := range plan.Batches { + switch batch.Kind { + case sequence.BatchKindGroups: + for _, group := range batch.Groups { + ordered = append(ordered, "group:"+sequence.DisplayPath(batch.ChartPath)+" "+group.Name) + for _, manifest := range group.Manifests { + ordered = append(ordered, "source:"+manifest.Name) + } + } + case sequence.BatchKindUnsequenced: + for _, manifest := range batch.Manifests() { + ordered = append(ordered, "source:"+manifest.Name) + } + } + } + return ordered +} + +func orderedTemplateSequenceFromOutput(output string) []string { + var ordered []string + for line := range strings.SplitSeq(output, "\n") { + if marker, ok := strings.CutPrefix(line, "## START resource-group: "); ok { + ordered = append(ordered, "group:"+marker) + continue + } + if source, ok := strings.CutPrefix(line, "# Source: "); ok { + ordered = append(ordered, "source:"+source) + } + } + return ordered +} + func TestTemplateVersionCompletion(t *testing.T) { repoFile := "testdata/helmhome/helm/repositories.yaml" repoCache := "testdata/helmhome/helm/repository" diff --git a/pkg/cmd/testdata/output/dag-sequenced-chart.txt b/pkg/cmd/testdata/output/dag-sequenced-chart.txt new file mode 100644 index 000000000..97144243e --- /dev/null +++ b/pkg/cmd/testdata/output/dag-sequenced-chart.txt @@ -0,0 +1,11 @@ +Chart: sequenced-chart + Subchart batches: + Batch 1: worker + Resource-group batches: + Batch 1: databases + Batch 2: app + Unsequenced (deployed last): ConfigMap/unsequenced-config + Chart: sequenced-chart/charts/worker + Subchart batches: (none) + Resource-group batches: + Batch 1: bootstrap diff --git a/pkg/cmd/testdata/output/dag-sequenced-isolated.txt b/pkg/cmd/testdata/output/dag-sequenced-isolated.txt new file mode 100644 index 000000000..3bcd72b40 --- /dev/null +++ b/pkg/cmd/testdata/output/dag-sequenced-isolated.txt @@ -0,0 +1,6 @@ +Chart: sequenced-isolated-chart + Subchart batches: (none) + Resource-group batches: + Batch 1: alpha + Batch 2: beta + Unsequenced (deployed last): ConfigMap/cm-gamma, ConfigMap/cm-plain diff --git a/pkg/cmd/testdata/output/template-ordered-delimiters.txt b/pkg/cmd/testdata/output/template-ordered-delimiters.txt new file mode 100644 index 000000000..fff815155 --- /dev/null +++ b/pkg/cmd/testdata/output/template-ordered-delimiters.txt @@ -0,0 +1,47 @@ +## START resource-group: sequenced-chart/worker bootstrap +--- +# Source: sequenced-chart/charts/worker/templates/aa-worker-configmap.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: worker-bootstrap + annotations: + helm.sh/resource-group: bootstrap +data: + worker: ready + +## END resource-group: sequenced-chart/worker bootstrap +## START resource-group: sequenced-chart databases +--- +# Source: sequenced-chart/templates/aa-databases-configmap.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: database-config + annotations: + helm.sh/resource-group: databases +data: + host: localhost + +## END resource-group: sequenced-chart databases +## START resource-group: sequenced-chart app +--- +# Source: sequenced-chart/templates/bb-app-configmap.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: app-config + annotations: + helm.sh/resource-group: app +data: + db_host: localhost + +## END resource-group: sequenced-chart app +--- +# Source: sequenced-chart/templates/cc-unsequenced-configmap.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: unsequenced-config +data: + key: value diff --git a/pkg/cmd/testdata/output/template-ordered-isolated.txt b/pkg/cmd/testdata/output/template-ordered-isolated.txt new file mode 100644 index 000000000..2cc372173 --- /dev/null +++ b/pkg/cmd/testdata/output/template-ordered-isolated.txt @@ -0,0 +1,45 @@ +## START resource-group: sequenced-isolated-chart alpha +--- +# Source: sequenced-isolated-chart/templates/aa-alpha.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-alpha + annotations: + helm.sh/resource-group: alpha +data: + key: alpha + +## END resource-group: sequenced-isolated-chart alpha +## START resource-group: sequenced-isolated-chart beta +--- +# Source: sequenced-isolated-chart/templates/bb-beta.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-beta + annotations: + helm.sh/resource-group: beta +data: + key: beta + +## END resource-group: sequenced-isolated-chart beta +--- +# Source: sequenced-isolated-chart/templates/dd-plain.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-plain +data: + key: plain + +--- +# Source: sequenced-isolated-chart/templates/cc-gamma.yaml +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-gamma + annotations: + helm.sh/resource-group: gamma +data: + key: gamma diff --git a/pkg/cmd/testdata/testcharts/sequenced-chart/Chart.yaml b/pkg/cmd/testdata/testcharts/sequenced-chart/Chart.yaml new file mode 100644 index 000000000..b1bde1d8c --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-chart/Chart.yaml @@ -0,0 +1,9 @@ +apiVersion: v2 +name: sequenced-chart +description: A test chart with ordered template output +type: application +version: 0.1.0 +dependencies: + - name: worker + version: 0.1.0 + repository: file://charts/worker diff --git a/pkg/cmd/testdata/testcharts/sequenced-chart/charts/worker/Chart.yaml b/pkg/cmd/testdata/testcharts/sequenced-chart/charts/worker/Chart.yaml new file mode 100644 index 000000000..8f3807ae6 --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-chart/charts/worker/Chart.yaml @@ -0,0 +1,5 @@ +apiVersion: v2 +name: worker +description: A subchart used to test ordered template output +type: application +version: 0.1.0 diff --git a/pkg/cmd/testdata/testcharts/sequenced-chart/charts/worker/templates/aa-worker-configmap.yaml b/pkg/cmd/testdata/testcharts/sequenced-chart/charts/worker/templates/aa-worker-configmap.yaml new file mode 100644 index 000000000..263ac9e22 --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-chart/charts/worker/templates/aa-worker-configmap.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: worker-bootstrap + annotations: + helm.sh/resource-group: bootstrap +data: + worker: ready diff --git a/pkg/cmd/testdata/testcharts/sequenced-chart/templates/aa-databases-configmap.yaml b/pkg/cmd/testdata/testcharts/sequenced-chart/templates/aa-databases-configmap.yaml new file mode 100644 index 000000000..8ea614d38 --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-chart/templates/aa-databases-configmap.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: database-config + annotations: + helm.sh/resource-group: databases +data: + host: localhost diff --git a/pkg/cmd/testdata/testcharts/sequenced-chart/templates/bb-app-configmap.yaml b/pkg/cmd/testdata/testcharts/sequenced-chart/templates/bb-app-configmap.yaml new file mode 100644 index 000000000..0472c0318 --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-chart/templates/bb-app-configmap.yaml @@ -0,0 +1,9 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: app-config + annotations: + helm.sh/resource-group: app + helm.sh/depends-on/resource-groups: '["databases"]' +data: + db_host: localhost diff --git a/pkg/cmd/testdata/testcharts/sequenced-chart/templates/cc-unsequenced-configmap.yaml b/pkg/cmd/testdata/testcharts/sequenced-chart/templates/cc-unsequenced-configmap.yaml new file mode 100644 index 000000000..03454569c --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-chart/templates/cc-unsequenced-configmap.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: unsequenced-config +data: + key: value diff --git a/pkg/cmd/testdata/testcharts/sequenced-hook-chart/Chart.yaml b/pkg/cmd/testdata/testcharts/sequenced-hook-chart/Chart.yaml new file mode 100644 index 000000000..ccfbcdf4f --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-hook-chart/Chart.yaml @@ -0,0 +1,5 @@ +apiVersion: v2 +name: sequenced-hook-chart +description: Chart with a hook carrying sequencing annotations, used to verify --output-dir stripping +type: application +version: 0.1.0 diff --git a/pkg/cmd/testdata/testcharts/sequenced-hook-chart/templates/hook-configmap.yaml b/pkg/cmd/testdata/testcharts/sequenced-hook-chart/templates/hook-configmap.yaml new file mode 100644 index 000000000..0ecd589cd --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-hook-chart/templates/hook-configmap.yaml @@ -0,0 +1,10 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: hook-configmap + annotations: + "helm.sh/hook": pre-install + helm.sh/resource-group: hooks + helm.sh/depends-on/resource-groups: '["databases"]' +data: + key: value diff --git a/pkg/cmd/testdata/testcharts/sequenced-hook-chart/templates/plain-configmap.yaml b/pkg/cmd/testdata/testcharts/sequenced-hook-chart/templates/plain-configmap.yaml new file mode 100644 index 000000000..3f406021e --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-hook-chart/templates/plain-configmap.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: plain-configmap + annotations: + helm.sh/resource-group: databases +data: + key: value diff --git a/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/Chart.yaml b/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/Chart.yaml new file mode 100644 index 000000000..96fc7e056 --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/Chart.yaml @@ -0,0 +1,5 @@ +apiVersion: v2 +name: sequenced-isolated-chart +description: A test chart with an isolated resource group +type: application +version: 0.1.0 diff --git a/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/aa-alpha.yaml b/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/aa-alpha.yaml new file mode 100644 index 000000000..bc8c2c291 --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/aa-alpha.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-alpha + annotations: + helm.sh/resource-group: alpha +data: + key: alpha diff --git a/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/bb-beta.yaml b/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/bb-beta.yaml new file mode 100644 index 000000000..c52b63bf7 --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/bb-beta.yaml @@ -0,0 +1,9 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-beta + annotations: + helm.sh/resource-group: beta + helm.sh/depends-on/resource-groups: '["alpha"]' +data: + key: beta diff --git a/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/cc-gamma.yaml b/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/cc-gamma.yaml new file mode 100644 index 000000000..99a4284d1 --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/cc-gamma.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-gamma + annotations: + helm.sh/resource-group: gamma +data: + key: gamma diff --git a/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/dd-plain.yaml b/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/dd-plain.yaml new file mode 100644 index 000000000..7423a84ef --- /dev/null +++ b/pkg/cmd/testdata/testcharts/sequenced-isolated-chart/templates/dd-plain.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-plain +data: + key: plain diff --git a/pkg/cmd/uninstall.go b/pkg/cmd/uninstall.go index 315c29a8d..046f6fb5e 100644 --- a/pkg/cmd/uninstall.go +++ b/pkg/cmd/uninstall.go @@ -82,7 +82,7 @@ func newUninstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { f.StringVar(&client.DeletionPropagation, "cascade", "background", "Must be \"background\", \"orphan\", or \"foreground\". Selects the deletion cascading strategy for the dependents. Defaults to background. Use \"foreground\" with --wait to ensure resources with finalizers are fully deleted before returning.") f.DurationVar(&client.Timeout, "timeout", 300*time.Second, "time to wait for any individual Kubernetes operation (like Jobs for hooks)") f.StringVar(&client.Description, "description", "", "add a custom description") - AddWaitFlag(cmd, &client.WaitStrategy) + AddOrderedWaitFlag(cmd, &client.WaitStrategy) return cmd } diff --git a/pkg/cmd/uninstall_test.go b/pkg/cmd/uninstall_test.go index ce436e68c..77f8cdb50 100644 --- a/pkg/cmd/uninstall_test.go +++ b/pkg/cmd/uninstall_test.go @@ -73,6 +73,12 @@ func TestUninstall(t *testing.T) { golden: "output/uninstall-wait.txt", rels: []*release.Release{release.Mock(&release.MockReleaseOptions{Name: "aeneas"})}, }, + { + name: "wait ordered", + cmd: "uninstall aeneas --wait=ordered", + golden: "output/uninstall-wait.txt", + rels: []*release.Release{release.Mock(&release.MockReleaseOptions{Name: "aeneas"})}, + }, { name: "uninstall without release", cmd: "uninstall", diff --git a/pkg/cmd/upgrade.go b/pkg/cmd/upgrade.go index 688842141..d35af3a18 100644 --- a/pkg/cmd/upgrade.go +++ b/pkg/cmd/upgrade.go @@ -141,6 +141,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { instClient.Timeout = client.Timeout instClient.WaitStrategy = client.WaitStrategy instClient.WaitOptions = client.WaitOptions + instClient.ReadinessTimeout = client.ReadinessTimeout instClient.WaitForJobs = client.WaitForJobs instClient.Devel = client.Devel instClient.Namespace = client.Namespace @@ -312,7 +313,8 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { addValueOptionsFlags(f, valueOpts) bindOutputFlag(cmd, &outfmt) bindPostRenderFlag(cmd, &client.PostRenderer, settings) - AddWaitFlag(cmd, &client.WaitStrategy) + AddOrderedWaitFlag(cmd, &client.WaitStrategy) + addReadinessTimeoutFlag(f, &client.ReadinessTimeout) cmd.MarkFlagsMutuallyExclusive("force-replace", "force-conflicts") cmd.MarkFlagsMutuallyExclusive("force", "force-conflicts") diff --git a/pkg/kube/custom_readiness_status_reader_test.go b/pkg/kube/custom_readiness_status_reader_test.go index b9177157a..af4f0eca1 100644 --- a/pkg/kube/custom_readiness_status_reader_test.go +++ b/pkg/kube/custom_readiness_status_reader_test.go @@ -236,7 +236,7 @@ func TestCustomReadinessStatusReaderWarnsOnceForIncomparableExpression(t *testin }) require.NoError(t, unstructured.SetNestedField(u.Object, "Running", "status", "phase")) - for i := 0; i < 2; i++ { + for range 2 { result, err := reader.ReadStatusForObject(context.Background(), nil, u) require.NoError(t, err) assert.Equal(t, status.InProgressStatus, result.Status) diff --git a/pkg/kube/readiness.go b/pkg/kube/readiness.go index f291d7823..564aa0cbf 100644 --- a/pkg/kube/readiness.go +++ b/pkg/kube/readiness.go @@ -106,8 +106,9 @@ func EvaluateCustomReadiness(obj *unstructured.Unstructured, successExprs, failu if !hasSuccess || !hasFailure { // Partial annotations: fall back to kstatus. The warning for this - // case is emitted once per batch by warnIfPartialReadinessAnnotations - // in the sequencing layer, not here (which runs on every poll tick). + // case is emitted once at plan-build time by the sequence plan + // builder (pkg/release/v1/sequence), not here (which runs on every + // poll tick). return ReadinessPending, true, nil, nil } diff --git a/pkg/release/v1/sequence/builder.go b/pkg/release/v1/sequence/builder.go index 602d7740a..2e98c1027 100644 --- a/pkg/release/v1/sequence/builder.go +++ b/pkg/release/v1/sequence/builder.go @@ -101,8 +101,13 @@ func FindSubchart(chrt *chart.Chart, nameOrAlias string) *chart.Chart { // // Preconditions: manifests are hook-free (SortManifests output or a stored // rel.Manifest — hooks are stored separately) and chrt has been through -// ProcessDependencies (true for both freshly-loaded and storage-decoded -// charts, per BuildSubchartDAG's contract). Build does not re-filter hooks. +// ProcessDependencies. Storage-decoded charts satisfy this in metadata only: +// the release codec drops the loaded dependency tree (Chart.dependencies is +// unexported), so only the root chart object and its pruned, alias-rewritten +// Metadata.Dependencies survive. Build handles that shape: the root subchart +// DAG is built from the trusted metadata (see BuildSubchartDAG) and nested +// levels are walked structurally from manifest "# Source:" paths +// (buildStructuralLevel). Build does not re-filter hooks. // chrt == nil yields a flat single-level plan at ChartPath "". // // Errors (fatal — the caller must not proceed): @@ -117,7 +122,8 @@ func FindSubchart(chrt *chart.Chart, nameOrAlias string) *chart.Chart { // - isolated group among ≥2 groups → demoted to unsequenced // - resource with only one of the two readiness annotations (falls back to kstatus) // - rendered subchart not declared in Chart.yaml (deployed after declared subcharts) -// - rendered subchart not resolvable to a chart object (flat fallback at its path) +// - rendered subchart not resolvable to a chart object and with ≥2 nested +// sibling subcharts (structural walk cannot recover sibling ordering) // // Postcondition: every input manifest appears in exactly one batch; // len(manifests) == Σ len(batch.Manifests()). Unit-enforced. @@ -139,8 +145,9 @@ func Build(chrt *chart.Chart, manifests []releaseutil.Manifest) (*Plan, error) { return b.plan, nil } -func (b *builder) warnf(chartPath, format string, args ...any) { +func (b *builder) warnf(kind WarningKind, chartPath, format string, args ...any) { b.plan.Warnings = append(b.plan.Warnings, Warning{ + Kind: kind, ChartPath: chartPath, Message: fmt.Sprintf(format, args...), }) @@ -159,7 +166,7 @@ func (b *builder) buildLevel(c *chart.Chart, manifests []releaseutil.Manifest, c batches, err := dag.GetBatches() if err != nil { - return fmt.Errorf("getting subchart batches for %s: %w", chartPath, err) + return fmt.Errorf("subchart circular dependency detected in %s: %w", chartPath, err) } b.plan.Levels[levelIdx].SubchartBatches = batches @@ -178,7 +185,7 @@ func (b *builder) buildLevel(c *chart.Chart, manifests []releaseutil.Manifest, c continue } b.plan.Levels[levelIdx].Undeclared = append(b.plan.Levels[levelIdx].Undeclared, name) - b.warnf(chartPath, "rendered subchart %q is not declared in Chart.yaml dependencies; sequencing it after declared subcharts", name) + b.warnf(WarningKindUndeclaredSubchart, chartPath, "rendered subchart %q is not declared in Chart.yaml dependencies; sequencing it after declared subcharts", name) if err := b.buildSubchart(c, chartPath, name, grouped[name], depth, levelIdx); err != nil { return err } @@ -197,13 +204,44 @@ func (b *builder) buildSubchart(parent *chart.Chart, chartPath, name string, man sub := FindSubchart(parent, name) if sub == nil { b.plan.Levels[parentLevelIdx].Unresolved = append(b.plan.Levels[parentLevelIdx].Unresolved, name) - b.warnf(chartPath, "subchart %q not found in chart dependencies; deploying its manifests without subchart sequencing", name) - return b.appendResourceGroupBatches(subPath, depth+1, manifests) + return b.buildStructuralLevel(subPath, depth+1, manifests) } return b.buildLevel(sub, manifests, subPath, depth+1) } +// buildStructuralLevel sequences a subtree whose chart object is unavailable. +// Charts decoded from release storage lose their loaded dependency tree (the +// release codec serializes only exported fields, and Chart.dependencies is +// not one), so at uninstall/rollback time nested subchart levels have +// manifests but no chart. The tree structure is recovered from the manifests' +// "# Source:" path prefixes and walked exactly like buildLevel — subchart +// subtrees first, then the level's own resource-group batches (the group +// annotations live on the manifests and fully survive storage). The only +// ordering information lost with the chart is depends-on edges BETWEEN +// sibling subcharts: siblings are walked in name order, and a warning is +// recorded when a level has two or more. +func (b *builder) buildStructuralLevel(chartPath string, depth int, manifests []releaseutil.Manifest) error { + levelIdx := len(b.plan.Levels) + b.plan.Levels = append(b.plan.Levels, ChartLevel{Path: chartPath, Depth: depth}) + + grouped := GroupManifestsByDirectSubchart(manifests, chartPath) + subcharts := slices.DeleteFunc(slices.Sorted(maps.Keys(grouped)), func(name string) bool { return name == "" }) + + if len(subcharts) > 0 { + b.plan.Levels[levelIdx].SubchartBatches = [][]string{subcharts} + } + if len(subcharts) >= 2 { + b.warnf(WarningKindUnresolvedSubchart, chartPath, "chart metadata for %s is unavailable; sequencing its subcharts %v in name order (depends-on between them, if any, is not recoverable)", chartPath, subcharts) + } + for _, name := range subcharts { + if err := b.buildStructuralLevel(chartPath+"/charts/"+name, depth+1, grouped[name]); err != nil { + return err + } + } + return b.appendResourceGroupBatches(chartPath, depth, grouped[""]) +} + func (b *builder) appendResourceGroupBatches(chartPath string, depth int, manifests []releaseutil.Manifest) error { if len(manifests) == 0 { return nil @@ -217,7 +255,7 @@ func (b *builder) appendResourceGroupBatches(chartPath string, depth int, manife hasSuccess := readinessAnnotationSet(annotations, releaseutil.AnnotationReadinessSuccess) hasFailure := readinessAnnotationSet(annotations, releaseutil.AnnotationReadinessFailure) if hasSuccess != hasFailure { - b.warnf(chartPath, "resource %q has only one of %s and %s; falling back to kstatus readiness", manifest.Head.Metadata.Name, releaseutil.AnnotationReadinessSuccess, releaseutil.AnnotationReadinessFailure) + b.warnf(WarningKindPartialReadiness, chartPath, "resource %q has only one of %s and %s; falling back to kstatus readiness", manifest.Head.Metadata.Name, releaseutil.AnnotationReadinessSuccess, releaseutil.AnnotationReadinessFailure) } } @@ -227,6 +265,7 @@ func (b *builder) appendResourceGroupBatches(chartPath string, depth int, manife } for _, warning := range warnings { b.plan.Warnings = append(b.plan.Warnings, Warning{ + Kind: WarningKindResourceGroupDemotion, ChartPath: chartPath, Message: warning, }) @@ -239,7 +278,7 @@ func (b *builder) appendResourceGroupBatches(chartPath string, depth int, manife if len(result.GroupDeps[groupName]) != 0 || dependents[groupName] { continue } - b.warnf(chartPath, "resource-group %q is isolated (no depends-on edges and no dependents); deploying it in the unsequenced batch after sequenced groups", groupName) + b.warnf(WarningKindIsolatedGroup, chartPath, "resource-group %q is isolated (no depends-on edges and no dependents); deploying it in the unsequenced batch after sequenced groups", groupName) unsequenced = append(unsequenced, result.Groups[groupName]...) delete(result.Groups, groupName) delete(result.GroupDeps, groupName) @@ -254,7 +293,7 @@ func (b *builder) appendResourceGroupBatches(chartPath string, depth int, manife groupBatches, err := dag.GetBatches() if err != nil { - return fmt.Errorf("getting resource-group batches for %s: %w", chartPath, err) + return fmt.Errorf("resource-group circular dependency detected in %s: %w", chartPath, err) } dependents := resourceGroupDependents(result.GroupDeps) diff --git a/pkg/release/v1/sequence/builder_test.go b/pkg/release/v1/sequence/builder_test.go index 38a694974..4fbf59156 100644 --- a/pkg/release/v1/sequence/builder_test.go +++ b/pkg/release/v1/sequence/builder_test.go @@ -62,6 +62,156 @@ func TestBuild_EmptyChart(t *testing.T) { assert.Empty(t, plan.Warnings) } +func TestGroupManifestsByDirectSubchart(t *testing.T) { + t.Parallel() + + manifests := []releaseutil.Manifest{ + makeManifest("parent", "parent/templates/one.yaml", nil), + makeManifest("db", "parent/charts/database/templates/one.yaml", nil), + makeManifest("cache", "parent/charts/database/charts/cache/templates/one.yaml", nil), + } + + grouped := GroupManifestsByDirectSubchart(manifests, "parent") + + require.Len(t, grouped[""], 1) + require.Len(t, grouped["database"], 2) +} + +// TestGroupManifestsByDirectSubchart_Nested verifies that when called +// with a deeper chartPath (i.e., during recursion into a subchart), nested +// grandchildren are routed to the correct subchart key instead of being merged +// into the parent batch. +func TestGroupManifestsByDirectSubchart_Nested(t *testing.T) { + t.Parallel() + + manifests := []releaseutil.Manifest{ + makeManifest("db-own", "parent/charts/database/templates/one.yaml", nil), + makeManifest("cache", "parent/charts/database/charts/cache/templates/one.yaml", nil), + } + + grouped := GroupManifestsByDirectSubchart(manifests, "parent/charts/database") + + require.Len(t, grouped[""], 1, "database's own resources should be under the empty key") + require.Len(t, grouped["cache"], 1, "nested cache subchart should be routed under its own key") + require.Equal(t, "parent/charts/database/templates/one.yaml", grouped[""][0].Name) + require.Equal(t, "parent/charts/database/charts/cache/templates/one.yaml", grouped["cache"][0].Name) +} + +func TestFindSubchart(t *testing.T) { + // makeSubchart constructs a chart with the given chart-name, its own + // Metadata.Name, which BuildSubchartDAG and the legacy lookup resolve + // against. + makeSubchart := func(chartName string) *chart.Chart { + return &chart.Chart{ + Metadata: &chart.Metadata{ + APIVersion: "v1", + Name: chartName, + Version: "0.1.0", + }, + } + } + + // makeParent attaches subcharts as dependencies and declares the + // parent's Metadata.Dependencies, which carry the Alias field. + makeParent := func(deps []*chart.Chart, metaDeps []*chart.Dependency) *chart.Chart { + parent := &chart.Chart{ + Metadata: &chart.Metadata{ + APIVersion: "v1", + Name: "parent", + Version: "0.1.0", + Dependencies: metaDeps, + }, + } + for _, d := range deps { + parent.AddDependency(d) + } + return parent + } + + t.Run("resolves by chart name when no alias is declared", func(t *testing.T) { + db := makeSubchart("database") + parent := makeParent( + []*chart.Chart{db}, + []*chart.Dependency{{Name: "database"}}, + ) + + got := FindSubchart(parent, "database") + require.NotNil(t, got) + assert.Equal(t, "database", got.Name()) + }) + + t.Run("resolves by alias when alias is declared", func(t *testing.T) { + postgres := makeSubchart("postgres") + parent := makeParent( + []*chart.Chart{postgres}, + []*chart.Dependency{{Name: "postgres", Alias: "db"}}, + ) + + got := FindSubchart(parent, "db") + require.NotNil(t, got, "alias lookup should resolve to the underlying chart") + assert.Equal(t, "postgres", got.Name()) + }) + + t.Run("resolves by underlying chart name even when an alias is declared", func(t *testing.T) { + // An alias does not hide the chart's real name. Manifests rendered + // under the chart's actual chart-name path should still resolve. + postgres := makeSubchart("postgres") + parent := makeParent( + []*chart.Chart{postgres}, + []*chart.Dependency{{Name: "postgres", Alias: "db"}}, + ) + + got := FindSubchart(parent, "postgres") + require.NotNil(t, got) + assert.Equal(t, "postgres", got.Name()) + }) + + t.Run("alias collides with another chart's real name, first match wins", func(t *testing.T) { + // dep1: chart "foo" aliased as "bar". + // dep2: chart "bar" with no alias. + // Query "bar" must resolve deterministically. Current contract: + // iteration order over Dependencies() is preserved, so the first + // dep whose effective name (alias or real) matches the query wins. + foo := makeSubchart("foo") + bar := makeSubchart("bar") + parent := makeParent( + []*chart.Chart{foo, bar}, + []*chart.Dependency{ + {Name: "foo", Alias: "bar"}, + {Name: "bar"}, + }, + ) + + got := FindSubchart(parent, "bar") + require.NotNil(t, got, "collision must resolve, not return nil") + assert.Equal(t, "foo", got.Name(), + "first matching dependency wins; aliased 'foo' is declared before raw 'bar'") + + // And the raw name "foo" must still resolve to chart "foo" even + // though its effective name has been shifted by the alias. + gotFoo := FindSubchart(parent, "foo") + require.NotNil(t, gotFoo) + assert.Equal(t, "foo", gotFoo.Name()) + }) + + t.Run("returns nil when not found", func(t *testing.T) { + db := makeSubchart("database") + parent := makeParent( + []*chart.Chart{db}, + []*chart.Dependency{{Name: "database"}}, + ) + + assert.Nil(t, FindSubchart(parent, "nonexistent")) + }) + + t.Run("returns nil when parent has no dependencies", func(t *testing.T) { + parent := &chart.Chart{ + Metadata: &chart.Metadata{APIVersion: "v1", Name: "parent", Version: "0.1.0"}, + } + assert.Nil(t, FindSubchart(parent, "anything")) + }) +} + func TestBuild_NoAnnotations_SingleFlatBatch(t *testing.T) { t.Parallel() @@ -374,7 +524,7 @@ func TestBuild_UndeclaredSubchartIncluded(t *testing.T) { assert.Contains(t, reversedPaths, "parent/charts/vendored") } -func TestBuild_UnresolvableSubchart_FlatFallback(t *testing.T) { +func TestBuild_UnresolvableSubchart_StructuralFallback(t *testing.T) { t.Parallel() manifests := []releaseutil.Manifest{ @@ -384,15 +534,177 @@ func TestBuild_UnresolvableSubchart_FlatFallback(t *testing.T) { plan, err := Build(newChart("parent"), manifests) require.NoError(t, err) - assertWarningContains(t, plan, "parent", "not found in chart dependencies") - require.Len(t, plan.Levels, 1) + // ghost is undeclared in parent's metadata (warned as such, unchanged), + // but a single unresolved subchart is fully recovered structurally, so + // there is no sibling-order warning. + assertWarningContains(t, plan, "parent", "not declared") + assertNoWarningContains(t, plan, "name order") + require.Len(t, plan.Levels, 2) assert.Equal(t, []string{"ghost"}, plan.Levels[0].Unresolved) + assert.Equal(t, ChartLevel{Path: "parent/charts/ghost", Depth: 1}, plan.Levels[1]) assert.Equal(t, []string{"parent/charts/ghost", "parent/charts/ghost"}, batchChartPaths(plan)) assert.Equal(t, []int{1, 1}, batchDepths(plan)) assert.Equal(t, [][]string{{"db"}, {"app"}}, batchGroupNames(plan)) assertPlanComplete(t, plan, manifests) } +// storageRoundTrip encodes and decodes a chart the way the release storage +// codec does (json.Marshal in pkg/storage/driver): the unexported loaded +// dependency tree is dropped, only exported fields survive. +func storageRoundTrip(t *testing.T, c *chart.Chart) *chart.Chart { + t.Helper() + + encoded, err := json.Marshal(c) + require.NoError(t, err) + decoded := &chart.Chart{} + require.NoError(t, json.Unmarshal(encoded, decoded)) + require.Empty(t, decoded.Dependencies(), "release codec is expected to drop the loaded dependency tree") + return decoded +} + +// TestBuild_StorageDecodedChart_ThreeLevels reproduces bead xmn: a 3-level +// chart decoded from release storage (as uninstall and rollback receive it) +// must yield the same batch order the freshly loaded chart produced at +// install time, not fail building the subchart DAG. +func TestBuild_StorageDecodedChart_ThreeLevels(t *testing.T) { + t.Parallel() + + grand := newChart("grand") + child := newChart("child", enabledDependency("grand")) + child.SetDependencies(grand) + child.Metadata.Annotations = map[string]string{chartutil.AnnotationDependsOnSubcharts: `["grand"]`} + parent := newChart("parent", enabledDependency("child")) + parent.SetDependencies(child) + parent.Metadata.Annotations = map[string]string{chartutil.AnnotationDependsOnSubcharts: `["child"]`} + + manifests := []releaseutil.Manifest{ + makeManifest("parent", "parent/templates/parent.yaml", nil), + makeManifest("child", "parent/charts/child/templates/child.yaml", nil), + makeManifest("grand", "parent/charts/child/charts/grand/templates/grand.yaml", nil), + } + + fresh, err := Build(parent, manifests) + require.NoError(t, err) + + plan, err := Build(storageRoundTrip(t, parent), manifests) + require.NoError(t, err) + assert.Equal(t, batchChartPaths(fresh), batchChartPaths(plan)) + assert.Equal(t, []string{ + "parent/charts/child/charts/grand", + "parent/charts/child", + "parent", + }, batchChartPaths(plan)) + assert.Equal(t, []int{2, 1, 0}, batchDepths(plan)) + assert.Equal(t, []ChartLevel{ + {Path: "parent", Depth: 0, SubchartBatches: [][]string{{"child"}}, Unresolved: []string{"child"}}, + {Path: "parent/charts/child", Depth: 1, SubchartBatches: [][]string{{"grand"}}}, + {Path: "parent/charts/child/charts/grand", Depth: 2}, + }, plan.Levels) + assert.Empty(t, plan.Warnings) + assertPlanComplete(t, plan, manifests) + + assert.Equal(t, []string{ + "parent", + "parent/charts/child", + "parent/charts/child/charts/grand", + }, batchChartPaths(plan.Reverse())) +} + +// TestBuild_StorageDecodedChart_AliasedSiblingOrder runs the real +// ProcessDependencies pipeline (alias rename + depends-on resolution), then +// storage-decodes the chart: root-level sibling order and alias resolution +// must survive via the stored metadata. +func TestBuild_StorageDecodedChart_AliasedSiblingOrder(t *testing.T) { + t.Parallel() + + parent := pipelineChart( + pipelineDependency("postgres", "primary-db"), + pipelineDependency("app", "", "postgres"), + ) + require.NoError(t, chartutil.ProcessDependencies(parent, map[string]any{})) + manifests := []releaseutil.Manifest{ + makeManifest("app", "parent/charts/app/templates/app.yaml", nil), + makeManifest("primary", "parent/charts/primary-db/templates/primary.yaml", nil), + } + + plan, err := Build(storageRoundTrip(t, parent), manifests) + require.NoError(t, err) + assert.Equal(t, []string{"parent/charts/primary-db", "parent/charts/app"}, batchChartPaths(plan)) + require.NotEmpty(t, plan.Levels) + assert.Equal(t, [][]string{{"primary-db"}, {"app"}}, plan.Levels[0].SubchartBatches) + assertPlanComplete(t, plan, manifests) +} + +// TestBuild_StorageDecodedChart_DisabledDepNotResurrected: a dependency +// disabled by condition at install time is pruned from Metadata.Dependencies +// by ProcessDependencies BEFORE the release is stored, so trusting the stored +// metadata cannot resurrect it at uninstall. +func TestBuild_StorageDecodedChart_DisabledDepNotResurrected(t *testing.T) { + t.Parallel() + + parent := pipelineChart( + &chart.Dependency{Name: "cache", Version: "0.1.0", Condition: "cache.enabled"}, + &chart.Dependency{Name: "db", Version: "0.1.0"}, + &chart.Dependency{Name: "app", Version: "0.1.0", DependsOn: []string{"db"}}, + ) + require.NoError(t, chartutil.ProcessDependencies(parent, map[string]any{ + "cache": map[string]any{"enabled": false}, + })) + + decoded := storageRoundTrip(t, parent) + for _, dep := range decoded.Metadata.Dependencies { + require.NotEqual(t, "cache", dep.Name, "disabled dependency must be pruned from stored metadata") + } + + // The disabled subchart was never rendered, so no manifests exist for it. + manifests := []releaseutil.Manifest{ + makeManifest("app", "parent/charts/app/templates/app.yaml", nil), + makeManifest("db", "parent/charts/db/templates/db.yaml", nil), + makeManifest("parent", "parent/templates/parent.yaml", nil), + } + + plan, err := Build(decoded, manifests) + require.NoError(t, err) + assert.Equal(t, []string{"parent/charts/db", "parent/charts/app", "parent"}, batchChartPaths(plan)) + for _, level := range plan.Levels { + assert.NotContains(t, level.Path, "cache") + } + assertPlanComplete(t, plan, manifests) +} + +// TestBuild_StorageDecodedChart_NestedSiblingOrderWarning: depends-on edges +// between SIBLING subcharts of a NESTED level live in that level's Chart.yaml, +// which storage does not preserve. The structural walk orders such siblings +// by name and says so. +func TestBuild_StorageDecodedChart_NestedSiblingOrderWarning(t *testing.T) { + t.Parallel() + + grandA := newChart("g-a") + grandB := newChart("g-b") + child := newChart("child", enabledDependency("g-a"), enabledDependency("g-b", "g-a")) + child.SetDependencies(grandA, grandB) + parent := newChart("parent", enabledDependency("child")) + parent.SetDependencies(child) + + manifests := []releaseutil.Manifest{ + makeManifest("parent", "parent/templates/parent.yaml", nil), + makeManifest("child", "parent/charts/child/templates/child.yaml", nil), + makeManifest("g-a", "parent/charts/child/charts/g-a/templates/a.yaml", nil), + makeManifest("g-b", "parent/charts/child/charts/g-b/templates/b.yaml", nil), + } + + plan, err := Build(storageRoundTrip(t, parent), manifests) + require.NoError(t, err) + assertWarningContains(t, plan, "parent/charts/child", "name order") + assert.Equal(t, []string{ + "parent/charts/child/charts/g-a", + "parent/charts/child/charts/g-b", + "parent/charts/child", + "parent", + }, batchChartPaths(plan)) + assertPlanComplete(t, plan, manifests) +} + func TestBuild_HookManifestNotFiltered(t *testing.T) { t.Parallel() diff --git a/pkg/release/v1/sequence/plan.go b/pkg/release/v1/sequence/plan.go index 533a2c387..17245e3d1 100644 --- a/pkg/release/v1/sequence/plan.go +++ b/pkg/release/v1/sequence/plan.go @@ -82,9 +82,33 @@ func (b Batch) Manifests() []releaseutil.Manifest { return manifests } +// WarningKind classifies a Warning so consumers can map runtime demotion +// classes to severities without string-matching messages. +type WarningKind uint8 + +const ( + // WarningKindResourceGroupDemotion: a resource or group demoted to the + // unsequenced batch by ParseResourceGroups (malformed depends-on JSON, + // reference to a non-existent group). Lint treats these as errors. + WarningKindResourceGroupDemotion WarningKind = iota + // WarningKindIsolatedGroup: a group with no edges demoted to unsequenced. + WarningKindIsolatedGroup + // WarningKindPartialReadiness: only one of the two readiness annotations + // present; the resource falls back to kstatus. + WarningKindPartialReadiness + // WarningKindUndeclaredSubchart: rendered subchart absent from Chart.yaml. + WarningKindUndeclaredSubchart + // WarningKindUnresolvedSubchart: rendered subchart dir with no resolvable + // chart object (typical for nested levels of storage-decoded charts) whose + // structural walk found ≥2 sibling subcharts, so their depends-on order, + // if any was declared, could not be recovered. + WarningKindUnresolvedSubchart +) + // Warning is a non-fatal misconfiguration surfaced during Build. -// Drivers log them; lint promotes them to errors. +// Drivers log them; lint maps them to rule severities. type Warning struct { + Kind WarningKind ChartPath string // "" for chart-independent warnings Message string } diff --git a/pkg/release/v1/util/manifest.go b/pkg/release/v1/util/manifest.go index bf9d3c2b3..89bef04fc 100644 --- a/pkg/release/v1/util/manifest.go +++ b/pkg/release/v1/util/manifest.go @@ -19,9 +19,12 @@ package util import ( "fmt" "regexp" + "slices" "strconv" "strings" "unicode" + + "sigs.k8s.io/yaml" ) // SimpleHead defines what the structure of the head of a manifest file @@ -82,33 +85,247 @@ func (a BySplitManifestsOrder) Less(i, j int) bool { } func (a BySplitManifestsOrder) Swap(i, j int) { a[i], a[j] = a[j], a[i] } -// helmInternalAnnotationLineRE matches a single YAML line whose key is one of -// helmInternalSequencingAnnotations. The pattern is intentionally line-based: -// Helm always emits these annotations as single-line JSON-encoded values, so a -// surgical line strip preserves the surrounding manifest byte-for-byte and -// keeps `helm template | diff` workflows stable. The regex is compiled once at -// init from helmInternalSequencingAnnotations so that adding a new helm-internal -// key only requires updating the slice. -var helmInternalAnnotationLineRE = func() *regexp.Regexp { - keys := make([]string, len(helmInternalSequencingAnnotations)) - for i, k := range helmInternalSequencingAnnotations { - keys[i] = regexp.QuoteMeta(k) - } - return regexp.MustCompile(`(?m)^[ \t]+(?:` + strings.Join(keys, "|") + `):[^\n]*\r?\n?`) -}() - -// StripHelmInternalAnnotations returns the manifest content with any -// HelmInternalSequencingAnnotations removed. The strip is line-based and -// preserves the surrounding byte order of the input document. Empty content -// passes through unchanged. +// StripHelmInternalAnnotations returns manifest content with Helm-internal +// sequencing annotations removed from top-level object metadata. This exists +// so that `helm template | kubectl apply -f -` remains valid even when charts +// use HIP-0025 sequencing annotation keys with multiple `/` separators, which +// fail Kubernetes annotation-key validation. +// +// The strip is block-scoped and conservative. Each YAML document first takes a +// byte-identity fast path unless the parsed SimpleHead confirms that a +// Helm-internal sequencing key is present in metadata.annotations. Confirmed +// documents are scanned as text, and the function only deletes whole lines +// inside the confirmed top-level metadata.annotations block, including +// continuation lines for stripped values. On any structural doubt, it returns +// the original document verbatim. The failure mode is "annotation retained", +// never "manifest corrupted". // -// This exists so that `helm template` output remains directly apply-able via -// `kubectl apply -f -`, even when charts use HIP-0025 sequencing annotations -// whose keys contain multiple `/` separators (which fail Kubernetes -// annotation-key validation). +// Stored release manifests keep these annotations because they are the +// uninstall/rollback plan input recovered by sequence.ParseStoredManifests. +// `helm get manifest` prints the stored record verbatim by design. Only output +// surfaces such as `helm template` and apply-time paths strip them. This aligns +// with the object-domain visitor stripSequencingAnnotations in pkg/action, +// which also strips only top-level object metadata. Annotations nested in pod +// templates or List items are not functional for sequencing and are no longer +// masked by this text strip. func StripHelmInternalAnnotations(content string) string { if strings.TrimSpace(content) == "" { return content } - return helmInternalAnnotationLineRE.ReplaceAllString(content, "") + + var out strings.Builder + var doc strings.Builder + for _, line := range strings.SplitAfter(content, "\n") { + if isYAMLDocumentSeparatorLine(line) { + out.WriteString(stripHelmInternalAnnotationsFromDoc(doc.String())) + doc.Reset() + out.WriteString(line) + continue + } + doc.WriteString(line) + } + out.WriteString(stripHelmInternalAnnotationsFromDoc(doc.String())) + return out.String() +} + +func stripHelmInternalAnnotationsFromDoc(doc string) string { + if strings.TrimSpace(doc) == "" { + return doc + } + + var head SimpleHead + if err := yaml.Unmarshal([]byte(doc), &head); err != nil { + return doc + } + if head.Metadata == nil || !hasHelmInternalSequencingAnnotation(head.Metadata.Annotations) { + return doc + } + + lines := strings.SplitAfter(doc, "\n") + metadataLine := -1 + for i, line := range lines { + if isExactYAMLKeyLine(line, 0, "metadata") { + metadataLine = i + break + } + } + if metadataLine == -1 { + return doc + } + + metadataStart := metadataLine + 1 + metadataEnd := len(lines) + for i := metadataStart; i < len(lines); i++ { + if isBlankYAMLLine(lines[i]) { + continue + } + if yamlLineIndent(lines[i]) == 0 { + metadataEnd = i + break + } + } + + metadataChildIndent, ok := minimumYAMLIndent(lines[metadataStart:metadataEnd]) + if !ok { + return doc + } + + annotationsLine := -1 + for i := metadataStart; i < metadataEnd; i++ { + if isExactYAMLKeyLine(lines[i], metadataChildIndent, "annotations") { + annotationsLine = i + break + } + } + if annotationsLine == -1 { + return doc + } + + annotationsStart := annotationsLine + 1 + annotationsEnd := metadataEnd + for i := annotationsStart; i < metadataEnd; i++ { + if isBlankYAMLLine(lines[i]) { + continue + } + if yamlLineIndent(lines[i]) <= metadataChildIndent { + annotationsEnd = i + break + } + } + + annotationEntryIndent, ok := minimumYAMLIndent(lines[annotationsStart:annotationsEnd]) + if !ok { + return doc + } + + var out strings.Builder + for i := 0; i < len(lines); { + if i >= annotationsStart && + i < annotationsEnd && + yamlLineIndent(lines[i]) == annotationEntryIndent && + isHelmInternalAnnotationEntryLine(lines[i], annotationEntryIndent) { + i++ + for i < annotationsEnd { + if isBlankYAMLLine(lines[i]) { + nextNonBlank := i + 1 + for nextNonBlank < annotationsEnd && isBlankYAMLLine(lines[nextNonBlank]) { + nextNonBlank++ + } + if nextNonBlank < annotationsEnd && yamlLineIndent(lines[nextNonBlank]) > annotationEntryIndent { + i = nextNonBlank + continue + } + break + } + if yamlLineIndent(lines[i]) <= annotationEntryIndent { + break + } + i++ + } + continue + } + out.WriteString(lines[i]) + i++ + } + return out.String() +} + +func hasHelmInternalSequencingAnnotation(annotations map[string]string) bool { + if annotations == nil { + return false + } + for _, key := range helmInternalSequencingAnnotations { + if _, ok := annotations[key]; ok { + return true + } + } + return false +} + +func isYAMLDocumentSeparatorLine(line string) bool { + body := strings.TrimSuffix(line, "\n") + return strings.TrimRight(body, " \t\r") == "---" +} + +func lineBody(line string) string { + body := strings.TrimSuffix(line, "\n") + return strings.TrimSuffix(body, "\r") +} + +func isBlankYAMLLine(line string) bool { + return strings.TrimSpace(lineBody(line)) == "" +} + +func yamlLineIndent(line string) int { + body := lineBody(line) + for i, r := range body { + if r != ' ' && r != '\t' { + return i + } + } + return len(body) +} + +func isExactYAMLKeyLine(line string, indent int, key string) bool { + body := lineBody(line) + if yamlLineIndent(line) != indent { + return false + } + return strings.TrimRight(body[indent:], " \t") == key+":" +} + +func minimumYAMLIndent(lines []string) (int, bool) { + minIndent := 0 + found := false + for _, line := range lines { + if isBlankYAMLLine(line) { + continue + } + indent := yamlLineIndent(line) + if !found || indent < minIndent { + minIndent = indent + found = true + } + } + return minIndent, found +} + +func isHelmInternalAnnotationEntryLine(line string, indent int) bool { + key, ok := yamlEntryKey(line, indent) + if !ok { + return false + } + return slices.Contains(helmInternalSequencingAnnotations, key) +} + +func yamlEntryKey(line string, indent int) (string, bool) { + body := lineBody(line) + if yamlLineIndent(line) != indent { + return "", false + } + entry := body[indent:] + if entry == "" { + return "", false + } + switch entry[0] { + case '\'', '"': + quote := entry[0] + end := strings.IndexByte(entry[1:], quote) + if end == -1 { + return "", false + } + key := entry[1 : end+1] + rest := entry[end+2:] + if !strings.HasPrefix(rest, ":") { + return "", false + } + return key, true + default: + for _, key := range helmInternalSequencingAnnotations { + if strings.HasPrefix(entry, key+":") { + return key, true + } + } + return "", false + } } diff --git a/pkg/release/v1/util/manifest_test.go b/pkg/release/v1/util/manifest_test.go index f1591bbe6..067658e66 100644 --- a/pkg/release/v1/util/manifest_test.go +++ b/pkg/release/v1/util/manifest_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "sigs.k8s.io/yaml" ) func TestSplitManifests(t *testing.T) { @@ -530,9 +531,11 @@ func TestStripHelmInternalAnnotations(t *testing.T) { tests := []struct { name string input string + expected string mustNotContain []string mustContain []string mustEqualVerbatim bool + mustParseOutput bool }{ { name: "strips multi-slash key, preserves siblings", @@ -598,11 +601,264 @@ data: input: ":\n\tnot valid yaml at all: [", mustEqualVerbatim: true, }, + { + name: "data block scalar look-alike survives", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-rules +data: + rules.txt: | + helm.sh/depends-on/resource-groups: ["a"] + second line +`, + mustEqualVerbatim: true, + }, + { + name: "annotation in metadata AND look-alike in data", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-mixed + annotations: + helm.sh/depends-on/resource-groups: '["db"]' + helm.sh/resource-group: app +data: + rules.txt: | + helm.sh/depends-on/resource-groups: '["db"]' + second line +`, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-mixed + annotations: + helm.sh/resource-group: app +data: + rules.txt: | + helm.sh/depends-on/resource-groups: '["db"]' + second line +`, + }, + { + name: "multi-line annotation value stripped whole", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-multiline + annotations: + helm.sh/depends-on/resource-groups: >- + ["databases", + "cache"] + keep.example.com/x: "y" +data: + k: v +`, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-multiline + annotations: + keep.example.com/x: "y" +data: + k: v +`, + }, + { + name: "folded annotation value with internal blank line stripped whole", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-blank + annotations: + helm.sh/depends-on/resource-groups: >- + ["a", + + "b"] + keep.example.com/x: "y" +data: + k: v +`, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-blank + annotations: + keep.example.com/x: "y" +data: + k: v +`, + mustParseOutput: true, + }, + { + name: "literal annotation value with internal blank line stripped whole", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-literal-blank + annotations: + helm.sh/depends-on/resource-groups: | + ["a", + + "b"] + keep.example.com/x: "y" +data: + script: | + preserve + + me +`, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-literal-blank + annotations: + keep.example.com/x: "y" +data: + script: | + preserve + + me +`, + mustParseOutput: true, + }, + { + name: "value containing '#' handled", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-hash + annotations: + helm.sh/depends-on/resource-groups: '["a#b"]' + keep.example.com/x: "y" +data: + k: v +`, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-hash + annotations: + keep.example.com/x: "y" +data: + k: v +`, + }, + { + name: "multi-doc stream: only affected doc changes", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-clean +data: + k: v +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-annotated + annotations: + helm.sh/depends-on/resource-groups: '["db"]' + keep.example.com/x: "y" +data: + k: v +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-data +data: + rules.txt: | + helm.sh/depends-on/resource-groups: ["db"] + keep +`, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-clean +data: + k: v +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-annotated + annotations: + keep.example.com/x: "y" +data: + k: v +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-data +data: + rules.txt: | + helm.sh/depends-on/resource-groups: ["db"] + keep +`, + }, + { + name: "byte-identity fast path odd formatting", + input: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: cm-fast \n # comment about annotations\n\ndata:\n k: v ", + mustEqualVerbatim: true, + }, + { + name: "byte-identity fast path CRLF", + input: "apiVersion: v1\r\nkind: ConfigMap\r\nmetadata:\r\n name: cm-crlf\r\ndata:\r\n k: v\r\n", + mustEqualVerbatim: true, + }, + { + name: "quoted key form stripped", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-quoted + annotations: + "helm.sh/depends-on/resource-groups": '["a"]' + keep.example.com/x: "y" +data: + k: v +`, + expected: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-quoted + annotations: + keep.example.com/x: "y" +data: + k: v +`, + }, + { + name: "separator inside block scalar does not corrupt", + input: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm-separator +data: + script: | +--- + still byte-identical +`, + mustEqualVerbatim: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := StripHelmInternalAnnotations(tt.input) + if tt.mustParseOutput { + if err := yaml.Unmarshal([]byte(got), map[string]any{}); err != nil { + t.Errorf("expected output to parse as yaml, got error: %v\n%s", err, got) + } + } + if tt.expected != "" { + if got != tt.expected { + t.Errorf("expected exact output:\n%s\ngot:\n%s", tt.expected, got) + } + return + } if tt.mustEqualVerbatim { if got != tt.input { t.Errorf("expected verbatim passthrough, got:\n%s", got)