From 2466944bd04f5cb1400af25405ef36cea309904f Mon Sep 17 00:00:00 2001 From: caretak3r <50377477+caretak3r@users.noreply.github.com> Date: Wed, 18 Feb 2026 23:04:33 -0500 Subject: [PATCH] fix(spec): verification fixes for HIP-0025 resource sequencing - Prevent duplicate edges in DAG (corrupted in-degree count) - Deduplicate dependency edges in ParseResourceGroups - Fix disabled subchart detection logic contradicting documentation - Fix findSubchart alias lookup to use parent chart's Dependencies - Set SequencingInfo before deployment for failure recovery - Handle zero Timeout without immediate deadline failure - Restore chart dependency Enabled values in lint rule - Add 'resource in multiple groups' lint check - Update template delimiters to include chart path per HIP spec - Replace custom contains helpers with strings.Contains --- pkg/action/install.go | 14 ++++---- pkg/action/rollback.go | 2 +- pkg/action/sequencing.go | 31 +++++++++++++--- pkg/action/sequencing_test.go | 18 ++-------- pkg/action/upgrade.go | 13 +++---- pkg/chart/v2/lint/rules/sequencing.go | 30 +++++++++++++++- pkg/chart/v2/util/dag.go | 8 +++++ pkg/chart/v2/util/dag_test.go | 27 ++++++++++++++ pkg/chart/v2/util/subchart_dag.go | 12 +++---- pkg/cmd/template.go | 36 +++++++++++++++++-- .../output/template-ordered-delimiters.txt | 8 ++--- pkg/release/v1/util/resource_group.go | 13 +++++-- 12 files changed, 161 insertions(+), 51 deletions(-) diff --git a/pkg/action/install.go b/pkg/action/install.go index efd4ba8c4..ef7d542e2 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -549,7 +549,14 @@ func (i *Install) performSequencedInstall(ctx context.Context, chrt *chart.Chart waitForJobs: i.WaitForJobs, timeout: i.Timeout, readinessTimeout: readinessTimeout, - deadline: time.Now().Add(i.Timeout), + deadline: computeDeadline(i.Timeout), + } + + // Set SequencingInfo before deployment so that failure recovery (e.g., + // rollback-on-failure) can also use sequenced deletion order. + rel.SequencingInfo = &release.SequencingInfo{ + Enabled: true, + Strategy: string(i.WaitStrategy), } if err := sd.deployChartLevel(ctx, chrt, manifests); err != nil { @@ -563,11 +570,6 @@ func (i *Install) performSequencedInstall(ctx context.Context, chrt *chart.Chart } } - rel.SequencingInfo = &release.SequencingInfo{ - Enabled: true, - Strategy: string(i.WaitStrategy), - } - if len(i.Description) > 0 { rel.SetStatus(rcommon.StatusDeployed, i.Description) } else { diff --git a/pkg/action/rollback.go b/pkg/action/rollback.go index dd88944d2..6adc4c7dd 100644 --- a/pkg/action/rollback.go +++ b/pkg/action/rollback.go @@ -361,7 +361,7 @@ func (r *Rollback) performSequencedRollback(ctx context.Context, currentRelease, waitForJobs: r.WaitForJobs, timeout: r.Timeout, readinessTimeout: readinessTimeout, - deadline: time.Now().Add(r.Timeout), + deadline: computeDeadline(r.Timeout), upgradeMode: true, currentResources: current, // upgradeCSAFieldManager: always true for rollback (same as existing performRollback) diff --git a/pkg/action/sequencing.go b/pkg/action/sequencing.go index 29b5e37b4..463e65287 100644 --- a/pkg/action/sequencing.go +++ b/pkg/action/sequencing.go @@ -30,6 +30,15 @@ import ( releaseutil "helm.sh/helm/v4/pkg/release/v1/util" ) +// computeDeadline returns a deadline based on the given timeout duration. +// If timeout is zero or negative, it returns the zero time (no deadline). +func computeDeadline(timeout time.Duration) time.Time { + if timeout <= 0 { + return time.Time{} + } + return time.Now().Add(timeout) +} + // GroupManifestsByDirectSubchart groups manifests by the direct subchart they belong to. // The parent chart's own manifests (templates directly under `/templates/`) are // returned under the empty string key "". @@ -383,13 +392,27 @@ func (s *sequencedDeployment) waitForResources(resources kube.ResourceList) erro } // findSubchart finds the subchart chart object within chrt's direct dependencies by name or alias. +// It uses the parent chart's Metadata.Dependencies to resolve aliases, since the alias is stored +// on the Dependency struct in Chart.yaml, not on the subchart's own Metadata. func findSubchart(chrt *chartv2.Chart, nameOrAlias string) *chartv2.Chart { + // Build a map from subchart chart name → alias from the parent's dependency declarations. + aliasMap := make(map[string]string) // chart name → effective name (alias or name) + 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() { - alias := dep.Metadata.Annotations["alias"] - if alias == "" { - alias = dep.Name() + effective := dep.Name() + if alias, ok := aliasMap[dep.Name()]; ok { + effective = alias } - if alias == nameOrAlias || dep.Name() == nameOrAlias { + if effective == nameOrAlias || dep.Name() == nameOrAlias { return dep } } diff --git a/pkg/action/sequencing_test.go b/pkg/action/sequencing_test.go index a6841d1c0..423e9ae5b 100644 --- a/pkg/action/sequencing_test.go +++ b/pkg/action/sequencing_test.go @@ -122,10 +122,10 @@ func TestBuildManifestYAML(t *testing.T) { t.Error("expected non-empty YAML output") } // Should contain content from both manifests - if !contains(result, "name: cm1") { + if !strings.Contains(result, "name: cm1") { t.Error("expected cm1 in output") } - if !contains(result, "name: cm2") { + if !strings.Contains(result, "name: cm2") { t.Error("expected cm2 in output") } } @@ -137,20 +137,6 @@ func TestBuildManifestYAML_Empty(t *testing.T) { } } -// contains checks if s contains substr. -func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsStr(s, substr)) -} - -func containsStr(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -} - // TestInstallRelease_StoresSequencingInfo verifies that a sequenced install stores // SequencingInfo in the release record. func TestInstallRelease_StoresSequencingInfo(t *testing.T) { diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 209acc103..93b35a816 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -608,12 +608,18 @@ func (u *Upgrade) performSequencedUpgrade(ctx context.Context, chrt *chartv2.Cha waitForJobs: u.WaitForJobs, timeout: u.Timeout, readinessTimeout: readinessTimeout, - deadline: time.Now().Add(u.Timeout), + deadline: computeDeadline(u.Timeout), upgradeMode: true, currentResources: current, upgradeCSAFieldManager: upgradeCSAFieldManager, } + // Set SequencingInfo before deployment so that failure recovery can use sequenced order. + upgradedRelease.SequencingInfo = &release.SequencingInfo{ + Enabled: true, + Strategy: string(u.WaitStrategy), + } + if err := sd.deployChartLevel(ctx, chrt, manifests); err != nil { return u.failRelease(upgradedRelease, nil, err) } @@ -644,11 +650,6 @@ func (u *Upgrade) performSequencedUpgrade(ctx context.Context, chrt *chartv2.Cha currentRelease.Info.Status = rcommon.StatusSuperseded u.cfg.recordRelease(currentRelease) - - upgradedRelease.SequencingInfo = &release.SequencingInfo{ - Enabled: true, - Strategy: string(u.WaitStrategy), - } upgradedRelease.Info.Status = rcommon.StatusDeployed if len(u.Description) > 0 { upgradedRelease.Info.Description = u.Description diff --git a/pkg/chart/v2/lint/rules/sequencing.go b/pkg/chart/v2/lint/rules/sequencing.go index 916d72052..e2c4b91f3 100644 --- a/pkg/chart/v2/lint/rules/sequencing.go +++ b/pkg/chart/v2/lint/rules/sequencing.go @@ -70,9 +70,17 @@ func validateSubchartSequencing(c *chart.Chart) error { // In lint context, conditions/tags cannot be evaluated. Treat all deps as // enabled to catch cycles that would manifest with any values configuration. - for _, dep := range c.Metadata.Dependencies { + // Save and restore original Enabled values to avoid mutating the chart. + origEnabled := make([]bool, len(c.Metadata.Dependencies)) + for i, dep := range c.Metadata.Dependencies { + origEnabled[i] = dep.Enabled dep.Enabled = true } + defer func() { + for i, dep := range c.Metadata.Dependencies { + dep.Enabled = origEnabled[i] + } + }() dag, err := chartutil.BuildSubchartDAG(c) if err != nil { @@ -149,6 +157,26 @@ func validateRenderedSequencingAnnotations(linter *support.Linter, c *chart.Char }) } + // Check for resources assigned to multiple different groups. + resourceGroups := make(map[string]string) // "kind/name" → group + for _, m := range manifests { + if m.Head == nil || m.Head.Metadata == nil { + continue + } + group, hasGroup := m.Head.Metadata.Annotations[releaseutil.AnnotationResourceGroup] + if !hasGroup || group == "" { + continue + } + key := m.Head.Kind + "/" + m.Head.Metadata.Name + if prev, exists := resourceGroups[key]; exists && prev != group { + linter.RunLinterRule(support.ErrorSev, linter.ChartDir, + fmt.Errorf("resource %q is assigned to multiple resource-groups (%q and %q); each resource must belong to exactly one group", + key, prev, group)) + } else { + resourceGroups[key] = group + } + } + // Check partial readiness annotations. for _, m := range manifests { if m.Head == nil || m.Head.Metadata == nil { diff --git a/pkg/chart/v2/util/dag.go b/pkg/chart/v2/util/dag.go index 61407d98e..46923c103 100644 --- a/pkg/chart/v2/util/dag.go +++ b/pkg/chart/v2/util/dag.go @@ -29,6 +29,7 @@ import ( type DAG struct { nodes map[string]struct{} edges map[string][]string // from -> []to (dependents) + edgeSet map[string]struct{} // "from\x00to" -> exists (dedup guard) inDegree map[string]int // node -> number of prerequisites } @@ -37,6 +38,7 @@ func NewDAG() *DAG { return &DAG{ nodes: make(map[string]struct{}), edges: make(map[string][]string), + edgeSet: make(map[string]struct{}), inDegree: make(map[string]int), } } @@ -63,6 +65,12 @@ func (d *DAG) AddEdge(from, to string) error { if _, ok := d.nodes[to]; !ok { return fmt.Errorf("unknown node %q", to) } + // Silently skip duplicate edges to prevent in-degree corruption. + key := from + "\x00" + to + if _, exists := d.edgeSet[key]; exists { + return nil + } + d.edgeSet[key] = struct{}{} d.edges[from] = append(d.edges[from], to) d.inDegree[to]++ return nil diff --git a/pkg/chart/v2/util/dag_test.go b/pkg/chart/v2/util/dag_test.go index 3b3756608..a2753482b 100644 --- a/pkg/chart/v2/util/dag_test.go +++ b/pkg/chart/v2/util/dag_test.go @@ -201,6 +201,33 @@ func TestDAGGetBatches_NodesWithoutEdges(t *testing.T) { } } +func TestDAGAddEdge_DuplicateIdempotent(t *testing.T) { + d := NewDAG() + d.AddNode("a") + d.AddNode("b") + if err := d.AddEdge("a", "b"); err != nil { + t.Fatal(err) + } + // Adding the same edge again should be a no-op. + if err := d.AddEdge("a", "b"); err != nil { + t.Fatal(err) + } + batches, err := d.GetBatches() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // Should be 2 batches: [a] then [b], NOT corrupted by double in-degree. + if len(batches) != 2 { + t.Fatalf("expected 2 batches, got %d: %v", len(batches), batches) + } + if !containsExactly(batches[0], "a") { + t.Errorf("batch 0 should be [a], got %v", batches[0]) + } + if !containsExactly(batches[1], "b") { + t.Errorf("batch 1 should be [b], got %v", batches[1]) + } +} + // helper: checks slice contains exactly these elements (order-independent) func containsExactly(slice []string, items ...string) bool { if len(slice) != len(items) { diff --git a/pkg/chart/v2/util/subchart_dag.go b/pkg/chart/v2/util/subchart_dag.go index 79bc31816..60ea04851 100644 --- a/pkg/chart/v2/util/subchart_dag.go +++ b/pkg/chart/v2/util/subchart_dag.go @@ -70,15 +70,11 @@ func BuildSubchartDAG(c *chart.Chart) (*DAG, error) { if dep.Alias != "" { effective = dep.Alias } + // A dependency is disabled when Enabled is false AND there's no condition or + // tag that could re-enable it at runtime. Conditions and tags are evaluated + // at install-time from user-supplied values, so we can't resolve them here. + // If Enabled is false but a condition/tag is set, treat as potentially enabled. disabled := !dep.Enabled && dep.Condition == "" && len(dep.Tags) == 0 - // A dependency with Enabled=false explicitly is disabled. - // We also check: if Enabled is true OR there's a condition/tag, treat as - // potentially enabled (conditions are runtime values we can't evaluate here). - // For DAG purposes: if Enabled is explicitly false and no condition/tag, - // treat as disabled. Otherwise treat as enabled. - if !dep.Enabled { - disabled = true - } byName[effective] = &depInfo{dep: dep, disabled: disabled} } diff --git a/pkg/cmd/template.go b/pkg/cmd/template.go index 226955223..b794e9ee0 100644 --- a/pkg/cmd/template.go +++ b/pkg/cmd/template.go @@ -280,14 +280,16 @@ func renderOrderedTemplate(manifest string, out io.Writer) error { } // Output each batch's groups with START/END delimiters. - // m.Content already contains the "# Source:" comment from rel.Manifest. + // Format per HIP-0025: ## START resource-group: + // where chart-path is derived from the manifest source (e.g., "foo" or "foo/bar"). for _, batch := range batches { for _, groupName := range batch { - fmt.Fprintf(out, "## START resource-group: %s\n", groupName) + chartPath := chartPathFromGroup(result.Groups[groupName]) + fmt.Fprintf(out, "## START resource-group: %s %s\n", chartPath, groupName) for _, m := range result.Groups[groupName] { fmt.Fprintf(out, "---\n%s\n", m.Content) } - fmt.Fprintf(out, "## END resource-group: %s\n", groupName) + fmt.Fprintf(out, "## END resource-group: %s %s\n", chartPath, groupName) } } @@ -299,6 +301,34 @@ func renderOrderedTemplate(manifest string, out io.Writer) error { return nil } +// chartPathFromGroup extracts the chart/subchart path from the first manifest's +// "# Source:" comment in its content. +// For "# Source: mychart/templates/foo.yaml" → "mychart". +// For "# Source: mychart/charts/sub/templates/bar.yaml" → "mychart/sub". +func chartPathFromGroup(manifests []releaseutil.Manifest) string { + if len(manifests) == 0 { + return "" + } + content := manifests[0].Content + // Look for "# Source: " line. + for _, line := range strings.Split(content, "\n") { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "# Source: ") { + source := strings.TrimPrefix(line, "# Source: ") + // Extract chart path up to "/templates/" + idx := strings.Index(source, "/templates/") + if idx < 0 { + return source + } + p := source[:idx] + // Collapse "parent/charts/sub" into "parent/sub" per HIP spec. + p = strings.ReplaceAll(p, "/charts/", "/") + return p + } + } + return "" +} + func isTestHook(h *release.Hook) bool { return slices.Contains(h.Events, release.HookTest) } diff --git a/pkg/cmd/testdata/output/template-ordered-delimiters.txt b/pkg/cmd/testdata/output/template-ordered-delimiters.txt index 2ec83b7fb..6f9324f4b 100644 --- a/pkg/cmd/testdata/output/template-ordered-delimiters.txt +++ b/pkg/cmd/testdata/output/template-ordered-delimiters.txt @@ -1,4 +1,4 @@ -## START resource-group: databases +## START resource-group: sequenced-chart databases --- # Source: sequenced-chart/templates/aa-databases-configmap.yaml apiVersion: v1 @@ -9,8 +9,8 @@ metadata: helm.sh/resource-group: databases data: host: localhost -## END resource-group: databases -## START resource-group: app +## END resource-group: sequenced-chart databases +## START resource-group: sequenced-chart app --- # Source: sequenced-chart/templates/bb-app-configmap.yaml apiVersion: v1 @@ -22,7 +22,7 @@ metadata: helm.sh/depends-on/resource-groups: '["databases"]' data: db_host: localhost -## END resource-group: app +## END resource-group: sequenced-chart app --- # Source: sequenced-chart/templates/cc-unsequenced-configmap.yaml apiVersion: v1 diff --git a/pkg/release/v1/util/resource_group.go b/pkg/release/v1/util/resource_group.go index 5b66d8b2e..e5f3fe593 100644 --- a/pkg/release/v1/util/resource_group.go +++ b/pkg/release/v1/util/resource_group.go @@ -121,8 +121,17 @@ func ParseResourceGroups(manifests []Manifest) (ResourceGroupResult, []string) { // Only record deps if we haven't already moved this group to unsequenced. if _, ok := result.Groups[p.groupName]; ok { if len(p.deps) > 0 { - existing := result.GroupDeps[p.groupName] - result.GroupDeps[p.groupName] = append(existing, p.deps...) + // Deduplicate deps to prevent in-degree corruption in the DAG. + seen := make(map[string]struct{}) + for _, d := range result.GroupDeps[p.groupName] { + seen[d] = struct{}{} + } + for _, d := range p.deps { + if _, exists := seen[d]; !exists { + result.GroupDeps[p.groupName] = append(result.GroupDeps[p.groupName], d) + seen[d] = struct{}{} + } + } } } nextPending: