From 0e322e0c5f73190d576867f18a98e786e928072b Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov <3595194+idsulik@users.noreply.github.com> Date: Tue, 14 Jan 2025 22:24:24 +0200 Subject: [PATCH] feat(helm): Add helm dependencies support (#9624) * feat(helm): Add helm dependencies support Signed-off-by: Suleiman Dibirov --- docs-v2/content/en/schemas/v4beta12.json | 12 +- pkg/skaffold/deploy/helm/helm.go | 116 ++++--- pkg/skaffold/deploy/helm/util.go | 132 ++++++++ pkg/skaffold/deploy/helm/util_test.go | 365 +++++++++++++++++++++++ pkg/skaffold/schema/defaults/defaults.go | 23 +- pkg/skaffold/schema/latest/config.go | 3 + 6 files changed, 603 insertions(+), 48 deletions(-) create mode 100644 pkg/skaffold/deploy/helm/util.go create mode 100644 pkg/skaffold/deploy/helm/util_test.go diff --git a/docs-v2/content/en/schemas/v4beta12.json b/docs-v2/content/en/schemas/v4beta12.json index 096ef76eeb6..5377523fa5c 100755 --- a/docs-v2/content/en/schemas/v4beta12.json +++ b/docs-v2/content/en/schemas/v4beta12.json @@ -2375,6 +2375,15 @@ "description": "if `true`, Skaffold will send `--create-namespace` flag to Helm CLI. `--create-namespace` flag is available in Helm since version 3.2. Defaults is `false`.", "x-intellij-html-description": "if true, Skaffold will send --create-namespace flag to Helm CLI. --create-namespace flag is available in Helm since version 3.2. Defaults is false." }, + "dependsOn": { + "items": { + "type": "string" + }, + "type": "array", + "description": "a list of Helm release names that this deploy depends on.", + "x-intellij-html-description": "a list of Helm release names that this deploy depends on.", + "default": "[]" + }, "name": { "type": "string", "description": "name of the Helm release. It accepts environment variables via the go template syntax.", @@ -2490,7 +2499,8 @@ "repo", "upgradeOnChange", "overrides", - "packaged" + "packaged", + "dependsOn" ], "additionalProperties": false, "type": "object", diff --git a/pkg/skaffold/deploy/helm/helm.go b/pkg/skaffold/deploy/helm/helm.go index ed0c6d5a2fd..4f2f44136ce 100644 --- a/pkg/skaffold/deploy/helm/helm.go +++ b/pkg/skaffold/deploy/helm/helm.go @@ -261,9 +261,31 @@ func (h *Deployer) Deploy(ctx context.Context, out io.Writer, builds []graph.Art olog.Entry(ctx).Infof("Deploying with helm v%s ...", h.bV) + // Build dependency graph to determine the order of Helm release deployments. + dependencyGraph, err := BuildDependencyGraph(h.Releases) + if err != nil { + return fmt.Errorf("error building dependency graph: %w", err) + } + + // Verify no cycles in the dependency graph + if err := VerifyNoCycles(dependencyGraph); err != nil { + return fmt.Errorf("error verifying dependency graph: %w", err) + } + + // Calculate deployment order + deploymentOrder, err := calculateDeploymentOrder(dependencyGraph) + if err != nil { + return fmt.Errorf("error calculating deployment order: %w", err) + } + + var mu sync2.Mutex nsMap := map[string]struct{}{} manifests := manifest.ManifestList{} - g, ctx := errgroup.WithContext(ctx) + + // Group releases by their dependency level to deploy them in the correct order. + levelGroups := groupReleasesByLevel(deploymentOrder, dependencyGraph) + + g, levelCtx := errgroup.WithContext(ctx) if h.Concurrency == nil || *h.Concurrency == 1 { g.SetLimit(1) @@ -273,58 +295,64 @@ func (h *Deployer) Deploy(ctx context.Context, out io.Writer, builds []graph.Art olog.Entry(ctx).Infof("Installing %d releases concurrently", len(h.Releases)) } - var mu sync2.Mutex - // Deploy every release + releaseNameToRelease := make(map[string]latest.HelmRelease) for _, r := range h.Releases { - g.Go(func() error { - releaseName, err := util.ExpandEnvTemplateOrFail(r.Name, nil) - if err != nil { - return helm.UserErr(fmt.Sprintf("cannot expand release name %q", r.Name), err) - } - chartVersion, err := util.ExpandEnvTemplateOrFail(r.Version, nil) - if err != nil { - return helm.UserErr(fmt.Sprintf("cannot expand chart version %q", r.Version), err) - } + releaseName, err := util.ExpandEnvTemplateOrFail(r.Name, nil) + if err != nil { + return fmt.Errorf("cannot parse the release name template: %w", err) + } + releaseNameToRelease[releaseName] = r + } - repo, err := util.ExpandEnvTemplateOrFail(r.Repo, nil) - if err != nil { - return helm.UserErr(fmt.Sprintf("cannot expand repo %q", r.Repo), err) - } - r.ChartPath, err = util.ExpandEnvTemplateOrFail(r.ChartPath, nil) - if err != nil { - return helm.UserErr(fmt.Sprintf("cannot expand chart path %q", r.ChartPath), err) - } + // Process each level in order + for level, releases := range levelGroups { + if len(levelGroups) > 1 { + olog.Entry(ctx).Infof("Installing level %d/%d releases (%d releases)", level, len(levelGroups), len(releases)) + } else { + olog.Entry(ctx).Infof("Installing releases (%d releases)", len(releases)) + } - m, results, err := h.deployRelease(ctx, out, releaseName, r, builds, h.bV, chartVersion, repo) - if err != nil { - return helm.UserErr(fmt.Sprintf("deploying %q", releaseName), err) - } + // Deploy releases in current level + for _, releaseName := range releases { + release := releaseNameToRelease[releaseName] - mu.Lock() - manifests.Append(m) - mu.Unlock() + g.Go(func() error { + chartVersion, err := util.ExpandEnvTemplateOrFail(release.Version, nil) + if err != nil { + return helm.UserErr(fmt.Sprintf("cannot expand chart version %q", release.Version), err) + } - // Collect namespaces first - newNamespaces := make(map[string]struct{}) - for _, res := range results { - if trimmed := strings.TrimSpace(res.Namespace); trimmed != "" { - newNamespaces[trimmed] = struct{}{} + repo, err := util.ExpandEnvTemplateOrFail(release.Repo, nil) + if err != nil { + return helm.UserErr(fmt.Sprintf("cannot expand repo %q", release.Repo), err) } - } - // Lock only once to update nsMap - mu.Lock() - for ns := range newNamespaces { - nsMap[ns] = struct{}{} - } - mu.Unlock() + release.ChartPath, err = util.ExpandEnvTemplateOrFail(release.ChartPath, nil) + if err != nil { + return helm.UserErr(fmt.Sprintf("cannot expand chart path %q", release.ChartPath), err) + } - return nil - }) - } + m, results, err := h.deployRelease(levelCtx, out, releaseName, release, builds, h.bV, chartVersion, repo) + if err != nil { + return helm.UserErr(fmt.Sprintf("deploying %q", releaseName), err) + } - if err := g.Wait(); err != nil { - return err + mu.Lock() + defer mu.Unlock() + manifests.Append(m) + for _, res := range results { + if trimmed := strings.TrimSpace(res.Namespace); trimmed != "" { + nsMap[trimmed] = struct{}{} + } + } + + return nil + }) + } + + if err := g.Wait(); err != nil { + return err + } } // Let's make sure that every image tag is set with `--set`. diff --git a/pkg/skaffold/deploy/helm/util.go b/pkg/skaffold/deploy/helm/util.go new file mode 100644 index 00000000000..dbb8e599cbd --- /dev/null +++ b/pkg/skaffold/deploy/helm/util.go @@ -0,0 +1,132 @@ +/* +Copyright 2019 The Skaffold 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 helm + +import ( + "fmt" + + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/helm" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/util" +) + +func BuildDependencyGraph(releases []latest.HelmRelease) (map[string][]string, error) { + dependencyGraph := make(map[string][]string) + for _, r := range releases { + releaseName, err := util.ExpandEnvTemplateOrFail(r.Name, nil) + if err != nil { + return nil, helm.UserErr(fmt.Sprintf("cannot expand release name %q", r.Name), err) + } + dependencyGraph[releaseName] = r.DependsOn + } + + return dependencyGraph, nil +} + +// VerifyNoCycles checks if there are any cycles in the dependency graph +func VerifyNoCycles(graph map[string][]string) error { + visited := make(map[string]bool) + recStack := make(map[string]bool) + + var checkCycle func(node string) error + checkCycle = func(node string) error { + if !visited[node] { + visited[node] = true + recStack[node] = true + + for _, dep := range graph[node] { + if !visited[dep] { + if err := checkCycle(dep); err != nil { + return err + } + } else if recStack[dep] { + return fmt.Errorf("cycle detected involving release %s", node) + } + } + } + recStack[node] = false + return nil + } + + for node := range graph { + if !visited[node] { + if err := checkCycle(node); err != nil { + return err + } + } + } + return nil +} + +// calculateDeploymentOrder returns a topologically sorted list of releases, +// ensuring that releases are deployed after their dependencies. +func calculateDeploymentOrder(graph map[string][]string) ([]string, error) { + visited := make(map[string]bool) + order := make([]string, 0) + + var visit func(node string) error + visit = func(node string) error { + if visited[node] { + return nil + } + visited[node] = true + + for _, dep := range graph[node] { + if err := visit(dep); err != nil { + return err + } + } + order = append(order, node) + return nil + } + + for node := range graph { + if err := visit(node); err != nil { + return nil, err + } + } + + return order, nil +} + +// groupReleasesByLevel groups releases by their dependency level +// Level 0 contains releases with no dependencies +// Level 1 contains releases that depend only on level 0 releases +// And so on... +func groupReleasesByLevel(order []string, graph map[string][]string) map[int][]string { + levels := make(map[int][]string) + releaseLevels := make(map[string]int) + + // Calculate level for each release + for _, release := range order { + level := 0 + for _, dep := range graph[release] { + if depLevel, exists := releaseLevels[dep]; exists { + if depLevel >= level { + level = depLevel + 1 + } + } + } + releaseLevels[release] = level + if levels[level] == nil { + levels[level] = make([]string, 0) + } + levels[level] = append(levels[level], release) + } + + return levels +} diff --git a/pkg/skaffold/deploy/helm/util_test.go b/pkg/skaffold/deploy/helm/util_test.go new file mode 100644 index 00000000000..4336f24f30b --- /dev/null +++ b/pkg/skaffold/deploy/helm/util_test.go @@ -0,0 +1,365 @@ +/* +Copyright 2019 The Skaffold 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 helm + +import ( + "slices" + "testing" + + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" + "github.com/GoogleContainerTools/skaffold/v2/testutil" +) + +func TestBuildDependencyGraph(t *testing.T) { + tests := []struct { + description string + releases []latest.HelmRelease + expected map[string][]string + shouldErr bool + }{ + { + description: "simple dependency graph", + releases: []latest.HelmRelease{ + {Name: "release1", DependsOn: []string{"release2"}}, + {Name: "release2", DependsOn: []string{}}, + }, + expected: map[string][]string{ + "release1": {"release2"}, + "release2": {}, + }, + }, + { + description: "no dependencies", + releases: []latest.HelmRelease{ + {Name: "release1"}, + {Name: "release2"}, + }, + expected: map[string][]string{ + "release1": nil, + "release2": nil, + }, + }, + { + description: "multiple dependencies", + releases: []latest.HelmRelease{ + {Name: "release1", DependsOn: []string{"release2", "release3"}}, + {Name: "release2", DependsOn: []string{"release3"}}, + {Name: "release3"}, + }, + expected: map[string][]string{ + "release1": {"release2", "release3"}, + "release2": {"release3"}, + "release3": nil, + }, + }, + { + description: "invalid release name template", + releases: []latest.HelmRelease{ + {Name: "{{.Invalid}}"}, + }, + shouldErr: true, + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + graph, err := BuildDependencyGraph(test.releases) + + if test.shouldErr { + t.CheckError(true, err) + return + } + + t.CheckError(false, err) + t.CheckDeepEqual(len(test.expected), len(graph)) + + for release, deps := range test.expected { + actualDeps, exists := graph[release] + if !exists { + t.Errorf("missing release %s in graph", release) + continue + } + + if len(deps) != len(actualDeps) { + t.Errorf("expected %d dependencies for %s, got %d", len(deps), release, len(actualDeps)) + continue + } + + // Check all expected dependencies exist + for _, dep := range deps { + if !slices.Contains(actualDeps, dep) { + t.Errorf("missing dependency %s for release %s", dep, release) + } + } + } + }) + } +} + +func TestVerifyNoCycles(t *testing.T) { + tests := []struct { + description string + graph map[string][]string + shouldErr bool + }{ + { + description: "no cycles", + graph: map[string][]string{ + "a": {"b", "c"}, + "b": {"c"}, + "c": {}, + }, + shouldErr: false, + }, + { + description: "simple cycle", + graph: map[string][]string{ + "a": {"b"}, + "b": {"a"}, + }, + shouldErr: true, + }, + { + description: "complex cycle", + graph: map[string][]string{ + "a": {"b"}, + "b": {"c"}, + "c": {"d"}, + "d": {"b"}, + }, + shouldErr: true, + }, + { + description: "self dependency", + graph: map[string][]string{ + "a": {"a"}, + }, + shouldErr: true, + }, + { + description: "empty graph", + graph: map[string][]string{}, + shouldErr: false, + }, + { + description: "disconnected components", + graph: map[string][]string{ + "a": {"b"}, + "c": {"d"}, + "b": {}, + "d": {}, + }, + shouldErr: false, + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + err := VerifyNoCycles(test.graph) + + if test.shouldErr { + t.CheckErrorContains("cycle detected", err) + } else { + t.RequireNoError(err) + } + }) + } +} + +func TestCalculateDeploymentOrder(t *testing.T) { + tests := []struct { + description string + graph map[string][]string + expected []string + shouldErr bool + }{ + { + description: "linear dependency chain", + graph: map[string][]string{ + "a": {"b"}, + "b": {"c"}, + "c": {}, + }, + expected: []string{"c", "b", "a"}, + }, + { + description: "multiple dependencies", + graph: map[string][]string{ + "a": {"b", "c"}, + "b": {"d"}, + "c": {"d"}, + "d": {}, + }, + expected: []string{"d", "b", "c", "a"}, + }, + { + description: "no dependencies", + graph: map[string][]string{ + "a": {}, + "b": {}, + "c": {}, + }, + expected: []string{"a", "b", "c"}, + }, + { + description: "diamond dependency", + graph: map[string][]string{ + "a": {"b", "c"}, + "b": {"d"}, + "c": {"d"}, + "d": {}, + }, + expected: []string{"d", "b", "c", "a"}, + }, + { + description: "empty graph", + graph: map[string][]string{}, + expected: []string{}, + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + order, err := calculateDeploymentOrder(test.graph) + + if test.shouldErr { + t.CheckError(true, err) + return + } + + t.CheckError(false, err) + + // Verify order satisfies dependencies + installed := make(map[string]bool) + for _, release := range order { + // Check all dependencies are installed + for _, dep := range test.graph[release] { + if !installed[dep] { + t.Errorf("dependency %s not installed before %s", dep, release) + } + } + installed[release] = true + } + + // Verify all nodes are present + if len(order) != len(test.graph) { + t.Errorf("expected %d nodes, got %d", len(test.graph), len(order)) + } + }) + } +} + +func TestGroupReleasesByLevel(t *testing.T) { + tests := []struct { + description string + order []string + graph map[string][]string + expected map[int][]string + }{ + { + description: "linear chain", + order: []string{"c", "b", "a"}, + graph: map[string][]string{ + "a": {"b"}, + "b": {"c"}, + "c": {}, + }, + expected: map[int][]string{ + 0: {"c"}, + 1: {"b"}, + 2: {"a"}, + }, + }, + { + description: "multiple dependencies at same level", + order: []string{"d", "b", "c", "a"}, + graph: map[string][]string{ + "a": {"b", "c"}, + "b": {"d"}, + "c": {"d"}, + "d": {}, + }, + expected: map[int][]string{ + 0: {"d"}, + 1: {"b", "c"}, + 2: {"a"}, + }, + }, + { + description: "no dependencies", + order: []string{"a", "b", "c"}, + graph: map[string][]string{ + "a": {}, + "b": {}, + "c": {}, + }, + expected: map[int][]string{ + 0: {"a", "b", "c"}, + }, + }, + { + description: "empty graph", + order: []string{}, + graph: map[string][]string{}, + expected: map[int][]string{}, + }, + { + description: "mixed levels", + order: []string{"d", "e", "b", "c", "a"}, + graph: map[string][]string{ + "a": {"b", "c"}, + "b": {"d"}, + "c": {"d", "e"}, + "d": {}, + "e": {}, + }, + expected: map[int][]string{ + 0: {"d", "e"}, + 1: {"b", "c"}, + 2: {"a"}, + }, + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + levels := groupReleasesByLevel(test.order, test.graph) + + t.CheckDeepEqual(len(test.expected), len(levels)) + + for level, releases := range test.expected { + t.CheckDeepEqual(releases, levels[level]) + } + + // Verify level assignments are correct + for level, releases := range levels { + for _, release := range releases { + // Check that all dependencies are at lower levels + for _, dep := range test.graph[release] { + for depLevel, depReleases := range levels { + if slices.Contains(depReleases, dep) { + if depLevel >= level { + t.Errorf("dependency %s at level %d >= release %s at level %d", dep, depLevel, release, level) + } + } + } + } + } + } + }) + } +} diff --git a/pkg/skaffold/schema/defaults/defaults.go b/pkg/skaffold/schema/defaults/defaults.go index a0dc04710cb..596c3fdcaf2 100644 --- a/pkg/skaffold/schema/defaults/defaults.go +++ b/pkg/skaffold/schema/defaults/defaults.go @@ -25,6 +25,7 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/build/kaniko" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/constants" + "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/deploy/helm" kubectx "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/kubernetes/context" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log" "github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest" @@ -46,7 +47,10 @@ func Set(c *latest.SkaffoldConfig) error { defaultToLocalBuild(c) setDefaultTagger(c) setDefaultLogsConfig(c) - setHelmDefaults(c) + err := setHelmDefaults(c) + if err != nil { + return err + } for _, a := range c.Build.Artifacts { setDefaultWorkspace(a) @@ -114,15 +118,28 @@ func Set(c *latest.SkaffoldConfig) error { return nil } -func setHelmDefaults(c *latest.SkaffoldConfig) { +func setHelmDefaults(c *latest.SkaffoldConfig) error { if c.Deploy.LegacyHelmDeploy == nil { - return + return nil } if c.Deploy.LegacyHelmDeploy.Concurrency == nil { defaultConcurrency := 1 c.Deploy.LegacyHelmDeploy.Concurrency = &defaultConcurrency } + + if len(c.Deploy.LegacyHelmDeploy.Releases) > 1 { + graph, err := helm.BuildDependencyGraph(c.Deploy.LegacyHelmDeploy.Releases) + if err != nil { + return err + } + + if err := helm.VerifyNoCycles(graph); err != nil { + return err + } + } + + return nil } // SetDefaultRenderer sets the default manifests to rawYaml. diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go index 26d22fc6600..92840774495 100644 --- a/pkg/skaffold/schema/latest/config.go +++ b/pkg/skaffold/schema/latest/config.go @@ -1073,6 +1073,9 @@ type HelmRelease struct { // Packaged parameters for packaging helm chart (`helm package`). Packaged *HelmPackaged `yaml:"packaged,omitempty"` + + // DependsOn is a list of Helm release names that this deploy depends on. + DependsOn []string `yaml:"dependsOn,omitempty"` } // HelmPackaged parameters for packaging helm chart (`helm package`).