From 179841fd04696587112e891db4ea2d062775e4a3 Mon Sep 17 00:00:00 2001 From: Rohit Gudi <50377477+caretak3r@users.noreply.github.com> Date: Thu, 11 Jun 2026 22:39:17 -0400 Subject: [PATCH] feat(hip-0025): add sequencing DAG and Chart.yaml depends-on schema Generic DAG with deterministic topological batching and cycle detection, the subchart dependency DAG builder (reading depends-on field and the helm.sh/depends-on/subcharts annotation against post-processed c.Dependencies() state), and the new depends-on field on Chart.yaml dependency entries. Refs: HIP-0025 Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com> --- pkg/chart/v2/dependency.go | 6 + pkg/chart/v2/dependency_json_test.go | 64 +++ pkg/chart/v2/dependency_test.go | 19 + pkg/chart/v2/util/dag.go | 155 ++++++++ pkg/chart/v2/util/dag_test.go | 232 +++++++++++ pkg/chart/v2/util/dependencies.go | 7 + pkg/chart/v2/util/subchart_dag.go | 253 ++++++++++++ pkg/chart/v2/util/subchart_dag_test.go | 527 +++++++++++++++++++++++++ 8 files changed, 1263 insertions(+) create mode 100644 pkg/chart/v2/dependency_json_test.go create mode 100644 pkg/chart/v2/util/dag.go create mode 100644 pkg/chart/v2/util/dag_test.go create mode 100644 pkg/chart/v2/util/subchart_dag.go create mode 100644 pkg/chart/v2/util/subchart_dag_test.go diff --git a/pkg/chart/v2/dependency.go b/pkg/chart/v2/dependency.go index 5a92ef305..459baa879 100644 --- a/pkg/chart/v2/dependency.go +++ b/pkg/chart/v2/dependency.go @@ -47,6 +47,9 @@ type Dependency struct { ImportValues []any `json:"import-values,omitempty" yaml:"import-values,omitempty"` // Alias usable alias to be used for the chart Alias string `json:"alias,omitempty" yaml:"alias,omitempty"` + // DependsOn is a list of subchart names or aliases that must be deployed + // before this subchart. + DependsOn []string `json:"depends-on,omitempty" yaml:"depends-on,omitempty"` } // Validate checks for common problems with the dependency datastructure in @@ -63,6 +66,9 @@ func (d *Dependency) Validate() error { for i := range d.Tags { d.Tags[i] = sanitizeString(d.Tags[i]) } + for i := range d.DependsOn { + d.DependsOn[i] = sanitizeString(d.DependsOn[i]) + } if d.Alias != "" && !aliasNameFormat.MatchString(d.Alias) { return ValidationErrorf("dependency %q has disallowed characters in the alias", d.Name) } diff --git a/pkg/chart/v2/dependency_json_test.go b/pkg/chart/v2/dependency_json_test.go new file mode 100644 index 000000000..8a2777739 --- /dev/null +++ b/pkg/chart/v2/dependency_json_test.go @@ -0,0 +1,64 @@ +/* +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 v2 + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sigs.k8s.io/yaml" +) + +func TestDependencyDependsOnJSONOmitEmpty(t *testing.T) { + dependency := Dependency{ + Name: "app", + Repository: "https://example.com/charts", + } + + data, err := json.Marshal(dependency) + require.NoError(t, err) + + assert.NotContains(t, string(data), "depends-on") +} + +func TestDependencyDependsOnJSONTag(t *testing.T) { + dependency := Dependency{ + Name: "app", + Repository: "https://example.com/charts", + DependsOn: []string{"database"}, + } + + data, err := json.Marshal(dependency) + require.NoError(t, err) + + assert.Contains(t, string(data), `"depends-on":["database"]`) +} + +func TestDependencyDependsOnYAMLBackwardCompatibility(t *testing.T) { + input := []byte(` +name: app +repository: https://example.com/charts +`) + + var dependency Dependency + err := yaml.Unmarshal(input, &dependency) + require.NoError(t, err) + + assert.Nil(t, dependency.DependsOn) +} diff --git a/pkg/chart/v2/dependency_test.go b/pkg/chart/v2/dependency_test.go index 35919bd7a..968512c68 100644 --- a/pkg/chart/v2/dependency_test.go +++ b/pkg/chart/v2/dependency_test.go @@ -42,3 +42,22 @@ func TestValidateDependency(t *testing.T) { } } } + +func TestValidateDependency_SanitizesDependsOn(t *testing.T) { + dep := &Dependency{ + Name: "example", + DependsOn: []string{"data\abase", "cache\rqueue"}, + } + if err := dep.Validate(); err != nil { + t.Fatalf("unexpected error: %s", err) + } + // Sanitized like Name/Tags/Condition: non-printable runes are dropped and + // whitespace runes are normalized to spaces, so stray characters don't break + // subchart depends-on lookups. + if dep.DependsOn[0] != "database" { + t.Errorf("DependsOn[0] not sanitized: got %q, want %q", dep.DependsOn[0], "database") + } + if dep.DependsOn[1] != "cache queue" { + t.Errorf("DependsOn[1] not sanitized: got %q, want %q", dep.DependsOn[1], "cache queue") + } +} diff --git a/pkg/chart/v2/util/dag.go b/pkg/chart/v2/util/dag.go new file mode 100644 index 000000000..7bfe5d07d --- /dev/null +++ b/pkg/chart/v2/util/dag.go @@ -0,0 +1,155 @@ +/* +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 util + +import ( + "fmt" + "maps" + "sort" + "strings" +) + +// DAG is a directed acyclic graph of string-keyed nodes, used for resource-group +// and subchart dependency ordering. +// +// Edges are directed: AddEdge("a", "b") means "b depends on a" (a must come before b). +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 +} + +// NewDAG creates an empty DAG. +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), + } +} + +// AddNode registers a node in the DAG. Duplicate adds are idempotent. +func (d *DAG) AddNode(name string) { + if _, ok := d.nodes[name]; ok { + return + } + + d.nodes[name] = struct{}{} + d.edges[name] = nil + d.inDegree[name] = 0 +} + +// AddEdge adds a directed edge: "to" depends on "from" (from is deployed before to). +// Returns an error if either node is unknown or if a self-loop is requested. +func (d *DAG) AddEdge(from, to string) error { + if from == to { + return fmt.Errorf("self-loop not allowed: %q", from) + } + if _, ok := d.nodes[from]; !ok { + return fmt.Errorf("unknown node %q", from) + } + if _, ok := d.nodes[to]; !ok { + return fmt.Errorf("unknown node %q", to) + } + + 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 +} + +// GetBatches performs a topological sort using Kahn's algorithm and returns +// the nodes grouped into deployment batches. Each batch contains nodes that +// can be deployed in parallel. Batches are ordered: batch 0 has no prerequisites, +// batch 1 depends only on batch 0, etc. +// +// Returns an error if a cycle is detected, including the names of the nodes +// involved in the cycle. +func (d *DAG) GetBatches() ([][]string, error) { + if len(d.nodes) == 0 { + return nil, nil + } + + inDegree := make(map[string]int, len(d.inDegree)) + maps.Copy(inDegree, d.inDegree) + + var batches [][]string + processed := 0 + + for { + var batch []string + for node := range d.nodes { + if inDegree[node] == 0 { + batch = append(batch, node) + inDegree[node] = -1 + } + } + + if len(batch) == 0 { + break + } + + sort.Strings(batch) + batches = append(batches, batch) + processed += len(batch) + + for _, node := range batch { + for _, dependent := range d.edges[node] { + inDegree[dependent]-- + } + } + } + + if processed == len(d.nodes) { + return batches, nil + } + + cycleNodes := make([]string, 0, len(d.nodes)-processed) + for node := range d.nodes { + if inDegree[node] > 0 { + cycleNodes = append(cycleNodes, node) + } + } + sort.Strings(cycleNodes) + + return nil, fmt.Errorf("cycle detected among nodes: %s", strings.Join(cycleNodes, ", ")) +} + +// Nodes returns a sorted slice of all node names in the DAG. +func (d *DAG) Nodes() []string { + nodes := make([]string, 0, len(d.nodes)) + for node := range d.nodes { + nodes = append(nodes, node) + } + sort.Strings(nodes) + + return nodes +} + +// HasNode reports whether the DAG contains a node with the given name. +func (d *DAG) HasNode(name string) bool { + _, ok := d.nodes[name] + return ok +} diff --git a/pkg/chart/v2/util/dag_test.go b/pkg/chart/v2/util/dag_test.go new file mode 100644 index 000000000..a029abea2 --- /dev/null +++ b/pkg/chart/v2/util/dag_test.go @@ -0,0 +1,232 @@ +/* +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 util + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDAG_BatchOrdering(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + nodes []string + edges [][2]string + expected [][]string + }{ + { + name: "empty DAG", + expected: nil, + }, + { + name: "single node", + nodes: []string{"a"}, + expected: [][]string{{"a"}}, + }, + { + name: "linear chain", + nodes: []string{"a", "b", "c"}, + edges: [][2]string{{"a", "b"}, {"b", "c"}}, + expected: [][]string{ + {"a"}, + {"b"}, + {"c"}, + }, + }, + { + name: "diamond", + nodes: []string{"a", "b", "c", "d"}, + edges: [][2]string{{"a", "b"}, {"a", "c"}, {"b", "d"}, {"c", "d"}}, + expected: [][]string{ + {"a"}, + {"b", "c"}, + {"d"}, + }, + }, + { + name: "multiple roots", + nodes: []string{"a", "b", "c"}, + edges: [][2]string{{"a", "c"}, {"b", "c"}}, + expected: [][]string{ + {"a", "b"}, + {"c"}, + }, + }, + { + name: "duplicate edge is idempotent", + nodes: []string{"a", "b"}, + edges: [][2]string{{"a", "b"}, {"a", "b"}}, + expected: [][]string{ + {"a"}, + {"b"}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + dag := buildDAG(t, tt.nodes, tt.edges) + + batches, err := dag.GetBatches() + require.NoError(t, err) + assert.Equal(t, tt.expected, batches) + }) + } +} + +func TestDAG_CycleDetection(t *testing.T) { + t.Parallel() + + dag := buildDAG(t, []string{"a", "b", "c"}, [][2]string{{"a", "b"}, {"b", "c"}, {"c", "a"}}) + + batches, err := dag.GetBatches() + require.Error(t, err) + assert.Nil(t, batches) + assert.ErrorContains(t, err, "cycle") + assert.ErrorContains(t, err, "a") + assert.ErrorContains(t, err, "b") + assert.ErrorContains(t, err, "c") +} + +func TestDAG_SelfLoop(t *testing.T) { + t.Parallel() + + dag := NewDAG() + dag.AddNode("a") + + err := dag.AddEdge("a", "a") + require.Error(t, err) + assert.ErrorContains(t, err, "self-loop") +} + +func TestDAG_UnknownNode(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + from string + to string + match string + }{ + { + name: "missing source node", + from: "missing", + to: "a", + match: "unknown node", + }, + { + name: "missing destination node", + from: "a", + to: "missing", + match: "unknown node", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + dag := NewDAG() + dag.AddNode("a") + + err := dag.AddEdge(tt.from, tt.to) + require.Error(t, err) + assert.ErrorContains(t, err, tt.match) + }) + } +} + +func TestDAG_DisconnectedComponents(t *testing.T) { + t.Parallel() + + dag := buildDAG(t, + []string{"alpha", "beta", "gamma", "delta", "epsilon", "zeta"}, + [][2]string{{"alpha", "gamma"}, {"beta", "delta"}, {"delta", "epsilon"}}, + ) + + batches, err := dag.GetBatches() + require.NoError(t, err) + assert.Equal(t, [][]string{ + {"alpha", "beta", "zeta"}, + {"delta", "gamma"}, + {"epsilon"}, + }, batches) +} + +func TestDAG_LargeGraph(t *testing.T) { + t.Parallel() + + const nodeCount = 128 + + nodes := make([]string, 0, nodeCount) + edges := make([][2]string, 0, nodeCount-1) + for i := range nodeCount { + nodes = append(nodes, fmt.Sprintf("node-%03d", i)) + if i == 0 { + continue + } + edges = append(edges, [2]string{ + fmt.Sprintf("node-%03d", i-1), + fmt.Sprintf("node-%03d", i), + }) + } + + dag := buildDAG(t, nodes, edges) + + batches, err := dag.GetBatches() + require.NoError(t, err) + require.Len(t, batches, nodeCount) + + for i, batch := range batches { + assert.Equal(t, []string{fmt.Sprintf("node-%03d", i)}, batch) + } +} + +func TestDAG_NodeHelpers(t *testing.T) { + t.Parallel() + + dag := NewDAG() + dag.AddNode("charlie") + dag.AddNode("alpha") + dag.AddNode("bravo") + + assert.True(t, dag.HasNode("alpha")) + assert.False(t, dag.HasNode("delta")) + assert.Equal(t, []string{"alpha", "bravo", "charlie"}, dag.Nodes()) +} + +func buildDAG(t *testing.T, nodes []string, edges [][2]string) *DAG { + t.Helper() + + dag := NewDAG() + for _, node := range nodes { + dag.AddNode(node) + } + + for _, edge := range edges { + require.NoError(t, dag.AddEdge(edge[0], edge[1])) + } + + return dag +} diff --git a/pkg/chart/v2/util/dependencies.go b/pkg/chart/v2/util/dependencies.go index f28a4f4b1..5921a2dbd 100644 --- a/pkg/chart/v2/util/dependencies.go +++ b/pkg/chart/v2/util/dependencies.go @@ -148,6 +148,13 @@ func processDependencyEnabled(c *chart.Chart, v map[string]any, path string) err return nil } + // Resolve depends-on references while dependency entries still carry + // their original chart names; the alias rewrite below (req.Name = + // req.Alias) makes those names unrecoverable afterwards. + if err := resolveDependsOnReferences(c); err != nil { + return err + } + var chartDependencies []*chart.Chart // If any dependency is not a part of Chart.yaml // then this should be added to chartDependencies. diff --git a/pkg/chart/v2/util/subchart_dag.go b/pkg/chart/v2/util/subchart_dag.go new file mode 100644 index 000000000..e614b0870 --- /dev/null +++ b/pkg/chart/v2/util/subchart_dag.go @@ -0,0 +1,253 @@ +/* +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 util + +import ( + "encoding/json" + "fmt" + "strings" + + chart "helm.sh/helm/v4/pkg/chart/v2" +) + +const ( + // AnnotationDependsOnSubcharts is the Chart.yaml annotation key for declaring + // parent-chart dependencies on subcharts. The value is a JSON string array of + // subchart names (or aliases) that must be ready before parent resources are + // installed. + AnnotationDependsOnSubcharts = "helm.sh/depends-on/subcharts" +) + +// BuildSubchartDAG constructs a DAG from a chart's subchart dependency declarations. +// +// Subchart-to-subchart ordering is read from the depends-on field on Chart.yaml +// dependency entries. The helm.sh/depends-on/subcharts metadata annotation is +// validated as the parent chart's dependencies on subcharts; parent resources +// are deployed after subchart batches by the action layer. +// +// Subcharts are keyed by their effective name (alias if set, otherwise name). +// ProcessDependencies must have been called on the chart first: it prunes +// disabled subcharts, applies alias renames, and rewrites depends-on +// references — dependency depends-on entries and the +// helm.sh/depends-on/subcharts annotation — to effective names (see +// resolveDependsOnReferences). BuildSubchartDAG therefore resolves references +// against effective names only. This holds for charts decoded from release +// storage too: they were processed before being stored, so their persisted +// depends-on references already use effective names. +func BuildSubchartDAG(c *chart.Chart) (*DAG, error) { + dag := NewDAG() + + if c == nil || c.Metadata == nil { + return dag, nil + } + + // Build the set of subcharts that survived ProcessDependencies. + loaded := make(map[string]bool, len(c.Dependencies())) + for _, sub := range c.Dependencies() { + loaded[sub.Name()] = true + } + + // 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 { + if dep == nil { + continue + } + eff := effectiveDependencyName(dep) + if !loaded[eff] || nodes[eff] { + continue + } + nodes[eff] = true + dag.AddNode(eff) + } + + for _, dep := range c.Metadata.Dependencies { + if dep == nil { + continue + } + eff := effectiveDependencyName(dep) + if !nodes[eff] { + continue + } + + for _, prerequisite := range dep.DependsOn { + if !nodes[prerequisite] { + return nil, fmt.Errorf("subchart %q depends-on unknown or disabled subchart %q", eff, prerequisite) + } + if err := dag.AddEdge(prerequisite, eff); err != nil { + return nil, fmt.Errorf("adding sequencing edge %s→%s: %w", prerequisite, eff, err) + } + } + } + + if err := validateParentSubchartDependencies(c.Metadata.Annotations[AnnotationDependsOnSubcharts], nodes); err != nil { + return nil, err + } + + return dag, nil +} + +// resolveDependsOnReferences rewrites depends-on references — Chart.yaml +// dependency depends-on entries and the helm.sh/depends-on/subcharts +// annotation — from a subchart's original chart name to its effective name +// (alias if set, otherwise name). It must run while original names are still +// present in c.Metadata.Dependencies, i.e. before processDependencyEnabled +// applies its Name = Alias rewrite; that rewrite makes original names +// unrecoverable. The rewritten references are what release storage persists +// and what BuildSubchartDAG consumes, including at uninstall time when the +// chart is decoded from the release record. +// +// A reference matching more than one subchart (e.g. the original name of a +// chart pulled in under two aliases) is rejected as ambiguous. References +// matching no dependency are left unchanged so BuildSubchartDAG can report +// them as unknown or disabled, and a malformed annotation is left for +// BuildSubchartDAG's JSON parse error. Resolution is idempotent: effective +// names resolve to themselves, so reprocessing an already-processed chart is +// a no-op. +func resolveDependsOnReferences(c *chart.Chart) error { + refs := newSubchartRefs() + for _, dep := range c.Metadata.Dependencies { + if dep == nil { + continue + } + eff := effectiveDependencyName(dep) + refs.register(eff, eff) + refs.register(dep.Name, eff) + } + + for _, dep := range c.Metadata.Dependencies { + if dep == nil { + continue + } + for i, ref := range dep.DependsOn { + eff, found, isAmbiguous := refs.resolve(ref) + if isAmbiguous { + return fmt.Errorf("subchart %q depends-on ambiguous subchart reference %q; reference it by alias to disambiguate", effectiveDependencyName(dep), ref) + } + if found { + dep.DependsOn[i] = eff + } + } + } + + return resolveAnnotationDependsOn(c, refs) +} + +// resolveAnnotationDependsOn rewrites the parent chart's +// helm.sh/depends-on/subcharts annotation entries to effective subchart names. +func resolveAnnotationDependsOn(c *chart.Chart, refs *subchartRefs) error { + annotation := strings.TrimSpace(c.Metadata.Annotations[AnnotationDependsOnSubcharts]) + if annotation == "" { + return nil + } + + var prerequisites []string + if err := json.Unmarshal([]byte(annotation), &prerequisites); err != nil { + // Malformed JSON is reported by BuildSubchartDAG with full context. + return nil + } + + changed := false + for i, ref := range prerequisites { + eff, found, isAmbiguous := refs.resolve(ref) + if isAmbiguous { + return fmt.Errorf("annotation %s references ambiguous subchart %q; reference it by alias to disambiguate", AnnotationDependsOnSubcharts, ref) + } + if found && eff != ref { + prerequisites[i] = eff + changed = true + } + } + if !changed { + return nil + } + + encoded, err := json.Marshal(prerequisites) + if err != nil { + return fmt.Errorf("re-encoding %s annotation: %w", AnnotationDependsOnSubcharts, err) + } + c.Metadata.Annotations[AnnotationDependsOnSubcharts] = string(encoded) + return nil +} + +// subchartRefs resolves depends-on references to effective subchart names +// while processing dependencies, i.e. while original chart names are still +// present. A reference may be a subchart's effective name (alias if set, +// otherwise name) or its original chart name. A reference that maps to more +// than one subchart is recorded as ambiguous and rejected on use, rather than +// being silently resolved to one of them. +type subchartRefs struct { + byRef map[string]string + ambiguous map[string]bool +} + +func newSubchartRefs() *subchartRefs { + return &subchartRefs{ + byRef: make(map[string]string), + ambiguous: make(map[string]bool), + } +} + +// register maps ref to the effective subchart name eff. If ref already resolves +// to a different subchart, it is flagged ambiguous. +func (s *subchartRefs) register(ref, eff string) { + if ref == "" { + return + } + if existing, ok := s.byRef[ref]; ok && existing != eff { + s.ambiguous[ref] = true + return + } + s.byRef[ref] = eff +} + +// resolve returns the effective subchart name for ref. found is false for an +// unknown reference; isAmbiguous is true when ref maps to multiple subcharts. +func (s *subchartRefs) resolve(ref string) (eff string, found, isAmbiguous bool) { + if s.ambiguous[ref] { + return "", false, true + } + eff, found = s.byRef[ref] + return eff, found, false +} + +func validateParentSubchartDependencies(annotation string, nodes map[string]bool) error { + annotation = strings.TrimSpace(annotation) + if annotation == "" { + return nil + } + + var prerequisites []string + if err := json.Unmarshal([]byte(annotation), &prerequisites); err != nil { + return fmt.Errorf("parsing %s annotation as JSON string array: %w", AnnotationDependsOnSubcharts, err) + } + + for _, prerequisite := range prerequisites { + if !nodes[prerequisite] { + return fmt.Errorf("annotation %s references unknown or disabled subchart %q", AnnotationDependsOnSubcharts, prerequisite) + } + } + + return nil +} + +func effectiveDependencyName(dep *chart.Dependency) string { + if dep.Alias != "" { + return dep.Alias + } + return dep.Name +} diff --git a/pkg/chart/v2/util/subchart_dag_test.go b/pkg/chart/v2/util/subchart_dag_test.go new file mode 100644 index 000000000..d5afc887b --- /dev/null +++ b/pkg/chart/v2/util/subchart_dag_test.go @@ -0,0 +1,527 @@ +/* +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 util + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + chart "helm.sh/helm/v4/pkg/chart/v2" +) + +func TestBuildSubchartDAG_Empty(t *testing.T) { + t.Parallel() + + batches := batchesForChart(t, newChart("parent")) + assert.Empty(t, batches) +} + +func TestBuildSubchartDAG_NoDependencies(t *testing.T) { + t.Parallel() + + c := newChart("parent", + enabledDependency("nginx"), + enabledDependency("rabbitmq"), + enabledDependency("postgres"), + ) + + assertBatches(t, c, [][]string{{"nginx", "postgres", "rabbitmq"}}) +} + +func TestBuildSubchartDAG_LinearOrder(t *testing.T) { + t.Parallel() + + c := newChart("parent", + enabledDependency("postgres"), + enabledDependency("rabbitmq", "postgres"), + enabledDependency("app", "rabbitmq"), + ) + + assertBatches(t, c, [][]string{{"postgres"}, {"rabbitmq"}, {"app"}}) +} + +func TestBuildSubchartDAG_AliasResolution(t *testing.T) { + t.Parallel() + + // Simulates post-ProcessDependencies state: the aliased entry has had + // Name rewritten to its alias, and depends-on entries were rewritten to + // effective names. (The real-pipeline path is covered by the + // TestProcessDependencies_* tests below.) + c := newChart("parent", + &chart.Dependency{Name: "primary-db", Alias: "primary-db", Enabled: true}, + enabledDependency("app", "primary-db"), + ) + + assertBatches(t, c, [][]string{{"primary-db"}, {"app"}}) +} + +func TestBuildSubchartDAG_DisabledSubchart(t *testing.T) { + t.Parallel() + + c := newChart("parent", + &chart.Dependency{Name: "cache", Enabled: false}, + enabledDependency("app", "cache"), + ) + + _, err := BuildSubchartDAG(c) + require.Error(t, err) + assert.ErrorContains(t, err, `depends-on unknown or disabled subchart "cache"`) +} + +func TestBuildSubchartDAG_DisabledSubchartNotReferenced(t *testing.T) { + t.Parallel() + + c := newChart("parent", + &chart.Dependency{Name: "cache", Enabled: false}, + enabledDependency("app"), + ) + + assertBatches(t, c, [][]string{{"app"}}) +} + +func TestBuildSubchartDAG_CycleDetection(t *testing.T) { + t.Parallel() + + c := newChart("parent", + enabledDependency("a", "b"), + enabledDependency("b", "c"), + enabledDependency("c", "a"), + ) + + dag, err := BuildSubchartDAG(c) + require.NoError(t, err) + + batches, err := dag.GetBatches() + require.Error(t, err) + assert.Nil(t, batches) + assert.ErrorContains(t, err, "cycle") +} + +func TestBuildSubchartDAG_AnnotationBasedParentDependencies(t *testing.T) { + t.Parallel() + + c := newChart("parent", + enabledDependency("postgres"), + enabledDependency("nginx"), + ) + c.Metadata.Annotations = map[string]string{ + AnnotationDependsOnSubcharts: `["nginx"]`, + } + + assertBatches(t, c, [][]string{{"nginx", "postgres"}}) +} + +func TestBuildSubchartDAG_HIPExample(t *testing.T) { + t.Parallel() + + c := newChart("foo", + enabledDependency("nginx"), + enabledDependency("rabbitmq"), + enabledDependency("bar", "nginx", "rabbitmq"), + ) + c.Metadata.Annotations = map[string]string{ + AnnotationDependsOnSubcharts: `["bar", "rabbitmq"]`, + } + + assertBatches(t, c, [][]string{{"nginx", "rabbitmq"}, {"bar"}}) +} + +func TestBuildSubchartDAG_MixedDeclarations(t *testing.T) { + t.Parallel() + + c := newChart("parent", + enabledDependency("database"), + enabledDependency("api", "database"), + enabledDependency("worker"), + ) + c.Metadata.Annotations = map[string]string{ + AnnotationDependsOnSubcharts: `["worker"]`, + } + + assertBatches(t, c, [][]string{{"database", "worker"}, {"api"}}) +} + +func TestBuildSubchartDAG_InvalidAnnotationJSON(t *testing.T) { + t.Parallel() + + c := newChart("parent", enabledDependency("api")) + c.Metadata.Annotations = map[string]string{ + AnnotationDependsOnSubcharts: `["api",`, + } + + _, err := BuildSubchartDAG(c) + require.Error(t, err) + assert.ErrorContains(t, err, "parsing "+AnnotationDependsOnSubcharts+" annotation") +} + +func TestBuildSubchartDAG_ObjectAnnotationRejected(t *testing.T) { + t.Parallel() + + c := newChart("parent", + enabledDependency("postgres"), + enabledDependency("nginx"), + ) + c.Metadata.Annotations = map[string]string{ + AnnotationDependsOnSubcharts: `{"nginx":["postgres"]}`, + } + + _, err := BuildSubchartDAG(c) + require.Error(t, err) + assert.ErrorContains(t, err, "JSON string array") +} + +func TestBuildSubchartDAG_NonExistentReference(t *testing.T) { + t.Parallel() + + c := newChart("parent", enabledDependency("app", "missing")) + + _, err := BuildSubchartDAG(c) + require.Error(t, err) + assert.ErrorContains(t, err, `depends-on unknown or disabled subchart "missing"`) +} + +func TestBuildSubchartDAG_AnnotationUnknownSubchart(t *testing.T) { + t.Parallel() + + c := newChart("parent", enabledDependency("postgres")) + c.Metadata.Annotations = map[string]string{ + AnnotationDependsOnSubcharts: `["app"]`, + } + + _, err := BuildSubchartDAG(c) + require.Error(t, err) + assert.ErrorContains(t, err, "unknown or disabled subchart") +} + +func TestBuildSubchartDAG_NestedSubcharts(t *testing.T) { + t.Parallel() + + root := newChart("parent", + enabledDependency("database"), + enabledDependency("application", "database"), + ) + nested := newChart("application", + enabledDependency("cache"), + enabledDependency("worker", "cache"), + ) + // Replace auto-stubs with real chart objects so nested DAG validation works. + root.SetDependencies( + &chart.Chart{Metadata: &chart.Metadata{Name: "database"}}, + nested, + ) + + assertBatches(t, root, [][]string{{"database"}, {"application"}}) + 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) { + t.Parallel() + + c := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "parent", + Dependencies: []*chart.Dependency{ + {Name: "ghost", Enabled: true}, + }, + }, + } + // Note: no AddDependency call — c.Dependencies() is empty. + + batches := batchesForChart(t, c) + assert.Empty(t, batches, "no loaded deps should yield empty DAG") +} + +// TestBuildSubchartDAG_AnnotationReferencesUnloadedDep verifies that an +// annotation referencing a subchart present in metadata but pruned from +// c.Dependencies() (e.g., disabled by ProcessDependencies) produces an error. +func TestBuildSubchartDAG_AnnotationReferencesUnloadedDep(t *testing.T) { + t.Parallel() + + c := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "parent", + Dependencies: []*chart.Dependency{ + {Name: "loaded-dep", Enabled: true}, + {Name: "pruned-dep", Enabled: true}, + }, + Annotations: map[string]string{ + AnnotationDependsOnSubcharts: `["pruned-dep"]`, + }, + }, + } + // Only loaded-dep is in c.Dependencies(); pruned-dep is not. + c.AddDependency(&chart.Chart{Metadata: &chart.Metadata{Name: "loaded-dep"}}) + + _, err := BuildSubchartDAG(c) + require.Error(t, err) + assert.ErrorContains(t, err, `unknown or disabled subchart "pruned-dep"`) +} + +func assertBatches(t *testing.T, c *chart.Chart, expected [][]string) { + t.Helper() + assert.Equal(t, expected, batchesForChart(t, c)) +} + +func batchesForChart(t *testing.T, c *chart.Chart) [][]string { + t.Helper() + + dag, err := BuildSubchartDAG(c) + require.NoError(t, err) + + batches, err := dag.GetBatches() + require.NoError(t, err) + + return batches +} + +func newChart(name string, deps ...*chart.Dependency) *chart.Chart { + c := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: name, + Dependencies: deps, + }, + } + // Simulate post-ProcessDependencies state: enabled deps appear in + // c.Dependencies() under their effective name (alias if set). + for _, dep := range deps { + if dep == nil || !dep.Enabled { + continue + } + subName := dep.Alias + if subName == "" { + subName = dep.Name + } + c.AddDependency(&chart.Chart{ + Metadata: &chart.Metadata{Name: subName}, + }) + } + return c +} + +func enabledDependency(name string, dependsOn ...string) *chart.Dependency { + return &chart.Dependency{ + Name: name, + Enabled: true, + DependsOn: dependsOn, + } +} + +// pipelineChart builds a parent chart whose loaded dependencies are named +// after the entries' ORIGINAL chart names, exactly as loader.LoadDir produces +// them, so tests exercise the real ProcessDependencies pipeline (alias rename +// + depends-on resolution) instead of hand-constructing post-processed state. +func pipelineChart(deps ...*chart.Dependency) *chart.Chart { + c := &chart.Chart{ + Metadata: &chart.Metadata{ + Name: "parent", + Version: "0.1.0", + APIVersion: chart.APIVersionV2, + Dependencies: deps, + }, + } + added := make(map[string]bool) + for _, dep := range deps { + if added[dep.Name] { + continue + } + added[dep.Name] = true + c.AddDependency(&chart.Chart{ + Metadata: &chart.Metadata{ + Name: dep.Name, + Version: "0.1.0", + APIVersion: chart.APIVersionV2, + }, + }) + } + return c +} + +func pipelineDependency(name, alias string, dependsOn ...string) *chart.Dependency { + return &chart.Dependency{ + Name: name, + Version: "0.1.0", + Alias: alias, + DependsOn: dependsOn, + } +} + +// TestProcessDependencies_ResolvesDependsOnByOriginalName is the regression +// test for hip-0025-92r. Through the real pipeline, processDependencyEnabled +// rewrites an aliased entry's Name to its alias; before the fix that made the +// original chart name unrecoverable and depends-on: ["postgres"] failed to +// resolve at DAG build time. +func TestProcessDependencies_ResolvesDependsOnByOriginalName(t *testing.T) { + t.Parallel() + + c := pipelineChart( + pipelineDependency("postgres", "primary-db"), + pipelineDependency("app", "", "postgres"), + ) + + require.NoError(t, ProcessDependencies(c, map[string]any{})) + + // The alias rename ran, and the depends-on reference was rewritten to the + // effective name — which is also what release storage persists, keeping + // the sequenced uninstall path working. + assert.Equal(t, "primary-db", c.Metadata.Dependencies[0].Name) + assert.Equal(t, []string{"primary-db"}, c.Metadata.Dependencies[1].DependsOn) + + assertBatches(t, c, [][]string{{"primary-db"}, {"app"}}) +} + +func TestProcessDependencies_ResolvesDependsOnByAlias(t *testing.T) { + t.Parallel() + + c := pipelineChart( + pipelineDependency("postgres", "primary-db"), + pipelineDependency("app", "", "primary-db"), + ) + + require.NoError(t, ProcessDependencies(c, map[string]any{})) + + assert.Equal(t, []string{"primary-db"}, c.Metadata.Dependencies[1].DependsOn) + assertBatches(t, c, [][]string{{"primary-db"}, {"app"}}) +} + +func TestProcessDependencies_AmbiguousDependsOnRejected(t *testing.T) { + t.Parallel() + + // The same chart aliased twice makes its original name ambiguous; + // referencing it by that name must be rejected, not silently resolved. + c := pipelineChart( + pipelineDependency("postgres", "db1"), + pipelineDependency("postgres", "db2"), + pipelineDependency("app", "", "postgres"), + ) + + err := ProcessDependencies(c, map[string]any{}) + require.Error(t, err) + assert.ErrorContains(t, err, `ambiguous subchart reference "postgres"`) +} + +func TestProcessDependencies_RewritesSubchartAnnotation(t *testing.T) { + t.Parallel() + + c := pipelineChart( + pipelineDependency("postgres", "primary-db"), + pipelineDependency("app", ""), + ) + c.Metadata.Annotations = map[string]string{ + AnnotationDependsOnSubcharts: `["postgres", "app"]`, + } + + require.NoError(t, ProcessDependencies(c, map[string]any{})) + + assert.Equal(t, `["primary-db","app"]`, c.Metadata.Annotations[AnnotationDependsOnSubcharts]) + assertBatches(t, c, [][]string{{"app", "primary-db"}}) +} + +func TestProcessDependencies_AmbiguousAnnotationRejected(t *testing.T) { + t.Parallel() + + c := pipelineChart( + pipelineDependency("postgres", "db1"), + pipelineDependency("postgres", "db2"), + ) + c.Metadata.Annotations = map[string]string{ + AnnotationDependsOnSubcharts: `["postgres"]`, + } + + err := ProcessDependencies(c, map[string]any{}) + require.Error(t, err) + assert.ErrorContains(t, err, `references ambiguous subchart "postgres"`) +} + +func TestProcessDependencies_UnknownDependsOnReportedByDAG(t *testing.T) { + t.Parallel() + + // Unknown references are left alone by the resolver so BuildSubchartDAG + // keeps reporting them with its established error message. + c := pipelineChart( + pipelineDependency("app", "", "missing"), + ) + + require.NoError(t, ProcessDependencies(c, map[string]any{})) + + _, err := BuildSubchartDAG(c) + require.Error(t, err) + assert.ErrorContains(t, err, `unknown or disabled subchart "missing"`) +} + +func TestProcessDependencies_DependsOnRewriteIdempotent(t *testing.T) { + t.Parallel() + + c := pipelineChart( + pipelineDependency("postgres", "primary-db"), + pipelineDependency("app", "", "postgres"), + ) + + require.NoError(t, ProcessDependencies(c, map[string]any{})) + // Lint and the sequenced-uninstall re-render run ProcessDependencies again + // on the already-processed chart; the second pass must neither error nor + // corrupt the rewritten references. + require.NoError(t, ProcessDependencies(c, map[string]any{})) + + assert.Equal(t, []string{"primary-db"}, c.Metadata.Dependencies[1].DependsOn) + assertBatches(t, c, [][]string{{"primary-db"}, {"app"}}) +} + +func TestProcessDependencies_PlainAndAliasedSameChart(t *testing.T) { + t.Parallel() + + // "svc" is deployed both under its own name and under an alias: the bare + // name matches two subcharts and is ambiguous, while the alias resolves. + ambiguous := pipelineChart( + pipelineDependency("svc", ""), + pipelineDependency("svc", "svc2"), + pipelineDependency("app", "", "svc"), + ) + err := ProcessDependencies(ambiguous, map[string]any{}) + require.Error(t, err) + assert.ErrorContains(t, err, `ambiguous subchart reference "svc"`) + + byAlias := pipelineChart( + pipelineDependency("svc", ""), + pipelineDependency("svc", "svc2"), + pipelineDependency("app", "", "svc2"), + ) + require.NoError(t, ProcessDependencies(byAlias, map[string]any{})) + assertBatches(t, byAlias, [][]string{{"svc", "svc2"}, {"app"}}) +} + +func TestProcessDependencies_SwappedAliasesAmbiguous(t *testing.T) { + t.Parallel() + + // x is aliased to y while y is aliased to x: each bare name now refers to + // two different subcharts, so references to either are rejected as + // ambiguous rather than silently resolved to one of them. + c := pipelineChart( + pipelineDependency("x", "y"), + pipelineDependency("y", "x"), + pipelineDependency("app", "", "x"), + ) + + err := ProcessDependencies(c, map[string]any{}) + require.Error(t, err) + assert.ErrorContains(t, err, `ambiguous subchart reference "x"`) +}