From 4bc613cb67a208ee92738ab8e685dbdfdc694b0a 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): sequence install, upgrade, rollback, and uninstall The sequenced deployment engine driving per-batch create-and-wait across the resource-group and subchart DAGs, wired into install, upgrade, rollback (respecting stored SequencingInfo), and reverse-order uninstall. Default (non-ordered) paths are unchanged. Refs: HIP-0025 Signed-off-by: Rohit Gudi <50377477+caretak3r@users.noreply.github.com> --- pkg/action/action.go | 39 +- pkg/action/backward_compat_test.go | 127 +++ pkg/action/hooks.go | 9 + pkg/action/hooks_test.go | 94 ++ pkg/action/install.go | 128 ++- pkg/action/rollback.go | 197 +++- pkg/action/rollback_sequenced_test.go | 498 +++++++++ pkg/action/rollback_test.go | 85 ++ pkg/action/sequencing.go | 375 +++++++ pkg/action/sequencing_test.go | 1339 ++++++++++++++++++++++++ pkg/action/uninstall.go | 249 +++-- pkg/action/uninstall_sequenced_test.go | 648 ++++++++++++ pkg/action/upgrade.go | 264 ++++- pkg/action/upgrade_sequenced_test.go | 559 ++++++++++ pkg/action/validate.go | 10 +- pkg/action/validate_test.go | 12 + pkg/action/warning_system_test.go | 109 ++ pkg/chart/v2/lint/rules/sequencing.go | 3 +- pkg/release/v1/util/manifest.go | 10 +- pkg/release/v1/util/resource_group.go | 18 +- 20 files changed, 4640 insertions(+), 133 deletions(-) create mode 100644 pkg/action/backward_compat_test.go create mode 100644 pkg/action/rollback_sequenced_test.go create mode 100644 pkg/action/sequencing.go create mode 100644 pkg/action/sequencing_test.go create mode 100644 pkg/action/uninstall_sequenced_test.go create mode 100644 pkg/action/upgrade_sequenced_test.go create mode 100644 pkg/action/warning_system_test.go diff --git a/pkg/action/action.go b/pkg/action/action.go index 6e5d8c15a..b308178a4 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -277,17 +277,24 @@ func splitAndDeannotate(postrendered, fallbackPrefix string) (map[string]string, // // This code has to do with writing files to disk. func (cfg *Configuration) renderResources(ctx context.Context, ch *chart.Chart, values common.Values, releaseName, outputDir string, subNotes, useReleaseName, includeCrds bool, pr postrenderer.PostRenderer, interactWithRemote, enableDNS, hideSecret bool, postRenderStrategy PostRenderStrategy) ([]*release.Hook, *bytes.Buffer, string, error) { + hs, b, notes, _, err := cfg.renderResourcesWithFiles(ctx, ch, values, releaseName, outputDir, subNotes, useReleaseName, includeCrds, pr, interactWithRemote, enableDNS, hideSecret, postRenderStrategy) + return hs, b, notes, err +} + +// renderResourcesWithFiles is the canonical render implementation, also returning the +// sorted manifest slice for sequencing-aware actions. +func (cfg *Configuration) renderResourcesWithFiles(ctx context.Context, ch *chart.Chart, values common.Values, releaseName, outputDir string, subNotes, useReleaseName, includeCrds bool, pr postrenderer.PostRenderer, interactWithRemote, enableDNS, hideSecret bool, postRenderStrategy PostRenderStrategy) ([]*release.Hook, *bytes.Buffer, string, []releaseutil.Manifest, error) { var hs []*release.Hook b := bytes.NewBuffer(nil) caps, err := cfg.getCapabilities() if err != nil { - return hs, b, "", err + return hs, b, "", nil, err } if ch.Metadata.KubeVersion != "" { if !chartutil.IsCompatibleRange(ch.Metadata.KubeVersion, caps.KubeVersion.String()) { - return hs, b, "", fmt.Errorf("chart requires kubeVersion: %s which is incompatible with Kubernetes %s", ch.Metadata.KubeVersion, caps.KubeVersion.Version) + return hs, b, "", nil, fmt.Errorf("chart requires kubeVersion: %s which is incompatible with Kubernetes %s", ch.Metadata.KubeVersion, caps.KubeVersion.Version) } } @@ -300,7 +307,7 @@ func (cfg *Configuration) renderResources(ctx context.Context, ch *chart.Chart, if interactWithRemote && cfg.RESTClientGetter != nil { restConfig, err := cfg.RESTClientGetter.ToRESTConfig() if err != nil { - return hs, b, "", err + return hs, b, "", nil, err } e := engine.New(restConfig) e.EnableDNS = enableDNS @@ -316,7 +323,7 @@ func (cfg *Configuration) renderResources(ctx context.Context, ch *chart.Chart, } if err2 != nil { - return hs, b, "", err2 + return hs, b, "", nil, err2 } // NOTES.txt gets rendered like all the other files, but because it's not a hook nor a resource, @@ -357,7 +364,7 @@ func (cfg *Configuration) renderResources(ctx context.Context, ch *chart.Chart, } fmt.Fprintf(b, "---\n# Source: %s\n%s\n", name, content) } - return hs, b, "", err + return hs, b, "", nil, err } // Build separate files maps for hooks and manifests. @@ -408,17 +415,17 @@ func (cfg *Configuration) renderResources(ctx context.Context, ch *chart.Chart, merged, err := annotateAndMerge(group.files) if err != nil { - return hs, b, notes, fmt.Errorf("error merging %s: %w", group.name, err) + return hs, b, notes, nil, fmt.Errorf("error merging %s: %w", group.name, err) } postRendered, err := pr.Run(bytes.NewBufferString(merged)) if err != nil { - return hs, b, notes, fmt.Errorf("error while running post render on %s: %w", group.name, err) + return hs, b, notes, nil, fmt.Errorf("error while running post render on %s: %w", group.name, err) } rendered, err := splitAndDeannotate(postRendered.String(), group.name) if err != nil { - return hs, b, notes, fmt.Errorf("error while parsing post rendered output for %s: %w", group.name, err) + return hs, b, notes, nil, fmt.Errorf("error while parsing post rendered output for %s: %w", group.name, err) } for k, v := range rendered { @@ -440,22 +447,22 @@ func (cfg *Configuration) renderResources(ctx context.Context, ch *chart.Chart, // Merge files as stream of documents for sending to post renderer merged, err := annotateAndMerge(files) if err != nil { - return hs, b, notes, fmt.Errorf("error merging manifests: %w", err) + return hs, b, notes, nil, fmt.Errorf("error merging manifests: %w", err) } // Run the post renderer postRendered, err := pr.Run(bytes.NewBufferString(merged)) if err != nil { - return hs, b, notes, fmt.Errorf("error while running post render on files: %w", err) + return hs, b, notes, nil, fmt.Errorf("error while running post render on files: %w", err) } // Use the file list and contents received from the post renderer files, err = splitAndDeannotate(postRendered.String(), "") if err != nil { - return hs, b, notes, fmt.Errorf("error while parsing post rendered output: %w", err) + return hs, b, notes, nil, fmt.Errorf("error while parsing post rendered output: %w", err) } default: - return hs, b, notes, fmt.Errorf("unknown post-render strategy: '%s'", postRenderStrategy) + return hs, b, notes, nil, fmt.Errorf("unknown post-render strategy: '%s'", postRenderStrategy) } } @@ -475,7 +482,7 @@ func (cfg *Configuration) renderResources(ctx context.Context, ch *chart.Chart, } fmt.Fprintf(b, "---\n# Source: %s\n%s\n", name, content) } - return hs, b, "", err + return hs, b, "", nil, err } // Aggregate all valid manifests into one big doc. @@ -488,7 +495,7 @@ func (cfg *Configuration) renderResources(ctx context.Context, ch *chart.Chart, } else { err = writeToFile(outputDir, crd.Filename, string(crd.File.Data[:]), fileWritten[crd.Filename]) if err != nil { - return hs, b, "", err + return hs, b, "", nil, err } fileWritten[crd.Filename] = true } @@ -513,13 +520,13 @@ func (cfg *Configuration) renderResources(ctx context.Context, ch *chart.Chart, // used by install or upgrade err = writeToFile(newDir, m.Name, m.Content, fileWritten[m.Name]) if err != nil { - return hs, b, "", err + return hs, b, "", nil, err } fileWritten[m.Name] = true } } - return hs, b, notes, nil + return hs, b, notes, manifests, nil } // RESTClientGetter gets the rest client diff --git a/pkg/action/backward_compat_test.go b/pkg/action/backward_compat_test.go new file mode 100644 index 000000000..b4694c3e4 --- /dev/null +++ b/pkg/action/backward_compat_test.go @@ -0,0 +1,127 @@ +/* +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 action + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "helm.sh/helm/v4/pkg/chart/common" + "helm.sh/helm/v4/pkg/kube" + rcommon "helm.sh/helm/v4/pkg/release/common" + release "helm.sh/helm/v4/pkg/release/v1" + releaseutil "helm.sh/helm/v4/pkg/release/v1/util" +) + +func TestBackwardCompat_OrderedWaitWithoutAnnotations(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + buf := captureWarningOutput(t) + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/first.yaml", "first", nil), + makeConfigMapTemplate("templates/second.yaml", "second", nil), + }, withName("backward-compat-no-annotations")) + + rel := mustRelease(t, mustRunInstall(t, install, ch)) + + require.Len(t, client.createCalls, 1) + assert.ElementsMatch(t, []string{"ConfigMap/first", "ConfigMap/second"}, client.createCalls[0]) + require.Len(t, client.waitCalls, 1) + assert.ElementsMatch(t, client.createCalls[0], client.waitCalls[0]) + assert.Empty(t, buf.String()) + assert.Equal(t, rcommon.StatusDeployed, rel.Info.Status) +} + +func TestBackwardCompat_RollbackWithoutSequencingInfo(t *testing.T) { + client := newRecordingKubeClient() + rollback := newRollbackAction(t, client) + + targetChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + makeConfigMapTemplate("templates/app.yaml", "app", nil), + }, withName("backward-compat-rollback")) + seedRollbackRelease(t, rollback, "backward-compat-rollback", 1, rcommon.StatusSuperseded, targetChart, joinManifestDocs( + configMapManifest("database", nil), + configMapManifest("app", nil), + ), nil) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("backward-compat-rollback")) + seedRollbackRelease(t, rollback, "backward-compat-rollback", 2, rcommon.StatusDeployed, currentChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}) + + rollback.Version = 1 + + var runErr error + assert.NotPanics(t, func() { + runErr = rollback.Run("backward-compat-rollback") + }) + require.NoError(t, runErr) + + require.Len(t, client.updateCalls, 1) + assert.ElementsMatch(t, []string{"ConfigMap/database", "ConfigMap/app"}, client.updateCalls[0].target) + require.Len(t, client.waitCalls, 1) + assert.ElementsMatch(t, []string{"ConfigMap/database", "ConfigMap/app"}, client.waitCalls[0]) + assert.Nil(t, latestRollbackRelease(t, rollback, "backward-compat-rollback").SequencingInfo) +} + +func TestBackwardCompat_MixedAnnotationsUnsequencedLast(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + buf := captureWarningOutput(t) + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + makeConfigMapTemplate("templates/plain-one.yaml", "plain-one", nil), + makeConfigMapTemplate("templates/plain-two.yaml", "plain-two", nil), + }, withName("backward-compat-mixed")) + + rel := mustRelease(t, mustRunInstall(t, install, ch)) + + require.Len(t, client.createCalls, 3) + assert.Equal(t, []string{"ConfigMap/database"}, client.createCalls[0]) + assert.Equal(t, []string{"ConfigMap/app"}, client.createCalls[1]) + assert.ElementsMatch(t, []string{"ConfigMap/plain-one", "ConfigMap/plain-two"}, client.createCalls[2]) + // Every batch, including terminal groups, is waited on. + require.Len(t, client.waitCalls, 3) + assert.Equal(t, []string{"ConfigMap/database"}, client.waitCalls[0]) + assert.Equal(t, []string{"ConfigMap/app"}, client.waitCalls[1]) + assert.ElementsMatch(t, client.createCalls[2], client.waitCalls[2]) + assert.Empty(t, buf.String()) + assert.Equal(t, rcommon.StatusDeployed, rel.Info.Status) +} diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index d7824d69f..459b1dcca 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -84,6 +84,15 @@ func (cfg *Configuration) execHookWithDelayedShutdown(rl *release.Release, hook return shutdownNoOp, fmt.Errorf("unable to build kubernetes object for %s hook %s: %w", hook, h.Path, err) } + // HIP-0025 sequencing annotations are ignored on hooks (spec: "Annotations + // added to resources in hooks will be ignored"). Strip them before apply so + // the multi-slash keys (e.g. helm.sh/depends-on/resource-groups), which are + // not valid Kubernetes annotation keys, do not cause the API server to reject + // the hook. + if err := stripSequencingAnnotations(resources); err != nil { + return shutdownNoOp, fmt.Errorf("unable to strip sequencing annotations from %s hook %s: %w", hook, h.Path, err) + } + // Record the time at which the hook was applied to the cluster h.LastRun = release.HookExecution{ StartedAt: time.Now(), diff --git a/pkg/action/hooks_test.go b/pkg/action/hooks_test.go index b017b1f11..a966bc705 100644 --- a/pkg/action/hooks_test.go +++ b/pkg/action/hooks_test.go @@ -22,6 +22,7 @@ import ( "errors" "fmt" "io" + "maps" "reflect" "strings" "testing" @@ -29,7 +30,9 @@ import ( "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/yaml" "k8s.io/cli-runtime/pkg/resource" @@ -445,6 +448,97 @@ func TestConfiguration_hookSetDeletePolicy(t *testing.T) { } } +// annotationRecordingKubeClient captures the annotations present on each +// resource at Create time, so a test can assert what actually reached the API. +type annotationRecordingKubeClient struct { + kubefake.PrintingKubeClient + createdAnnotations []map[string]string +} + +func (c *annotationRecordingKubeClient) Build(reader io.Reader, _ bool) (kube.ResourceList, error) { + u := &unstructured.Unstructured{} + if err := yaml.NewYAMLOrJSONDecoder(reader, 4096).Decode(u); err != nil { + return kube.ResourceList{}, err + } + return kube.ResourceList{{ + Name: u.GetName(), + Namespace: u.GetNamespace(), + Object: u, + }}, nil +} + +func (c *annotationRecordingKubeClient) Create(resources kube.ResourceList, _ ...kube.ClientCreateOption) (*kube.Result, error) { + for _, info := range resources { + acc, err := meta.Accessor(info.Object) + if err != nil { + continue + } + anns := maps.Clone(acc.GetAnnotations()) + if anns == nil { + anns = map[string]string{} + } + c.createdAnnotations = append(c.createdAnnotations, anns) + } + return &kube.Result{Created: resources}, nil +} + +// TestExecHook_StripsSequencingAnnotations verifies HIP-0025 sequencing +// annotations are removed from hook resources before apply (spec: annotations +// on hooks are ignored). The multi-slash keys are not valid K8s annotation +// keys, so leaving them would make the API server reject the hook. +func TestExecHook_StripsSequencingAnnotations(t *testing.T) { + is := assert.New(t) + + client := &annotationRecordingKubeClient{ + PrintingKubeClient: kubefake.PrintingKubeClient{Out: io.Discard}, + } + configuration := &Configuration{ + Releases: storage.Init(driver.NewMemory()), + KubeClient: client, + Capabilities: common.DefaultCapabilities, + } + + rel := &release.Release{ + Name: "test-release", + Namespace: "test", + Hooks: []*release.Hook{ + { + Name: "test-hook", + Kind: "ConfigMap", + Path: "templates/hook.yaml", + Manifest: `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-hook + namespace: test + annotations: + helm.sh/resource-group: "grp" + helm.sh/depends-on/resource-groups: '["other"]' + keep-me: "yes" +data: + foo: bar +`, + Weight: 0, + Events: []release.HookEvent{release.HookPreInstall}, + }, + }, + } + + err := configuration.execHook(rel, release.HookPreInstall, kube.StatusWatcherStrategy, nil, 600, false) + is.NoError(err) + + if is.Len(client.createdAnnotations, 1) { + anns := client.createdAnnotations[0] + // The multi-slash key is not a valid K8s annotation key and would be + // rejected at apply — it must be stripped from hooks (this is the bug). + is.NotContains(anns, "helm.sh/depends-on/resource-groups", "multi-slash sequencing annotation must be stripped from hooks") + // helm.sh/resource-group is a valid single-slash key; like the + // install/upgrade paths, it is left in place (harmlessly ignored on hooks). + is.Contains(anns, "helm.sh/resource-group", "valid single-slash annotation is retained, matching install/upgrade") + is.Equal("yes", anns["keep-me"], "non-sequencing annotations must survive") + } +} + func TestExecHook_WaitOptionsPassedDownstream(t *testing.T) { is := assert.New(t) diff --git a/pkg/action/install.go b/pkg/action/install.go index fcfae7214..616be4aed 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -56,6 +56,7 @@ import ( ri "helm.sh/helm/v4/pkg/release" rcommon "helm.sh/helm/v4/pkg/release/common" 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" "helm.sh/helm/v4/pkg/repo/v1" "helm.sh/helm/v4/pkg/storage" @@ -100,6 +101,10 @@ type Install struct { Devel bool DependencyUpdate bool Timeout time.Duration + // ReadinessTimeout is the per-batch timeout when --wait=ordered is used. + // Each ordered batch waits at most this long for resources to become ready. + // Must not exceed Timeout. Defaults to 1 minute when zero. + ReadinessTimeout time.Duration Namespace string ReleaseName string GenerateName bool @@ -310,6 +315,10 @@ func (i *Install) RunWithContext(ctx context.Context, ch ci.Charter, vals map[st return nil, fmt.Errorf("release name check failed: %w", err) } + if i.ReadinessTimeout > 0 && i.Timeout > 0 && i.ReadinessTimeout > i.Timeout { + return nil, fmt.Errorf("--readiness-timeout (%s) must not exceed --timeout (%s)", i.ReadinessTimeout, i.Timeout) + } + if err := chartutil.ProcessDependencies(chrt, vals); err != nil { i.cfg.Logger().Error("chart dependencies processing failed", slog.Any("error", err)) return nil, fmt.Errorf("chart dependencies processing failed: %w", err) @@ -374,8 +383,13 @@ func (i *Install) RunWithContext(ctx context.Context, ch ci.Charter, vals map[st rel := i.createRelease(chrt, vals, i.Labels) + var sortedManifests []releaseutil.Manifest var manifestDoc *bytes.Buffer - rel.Hooks, manifestDoc, rel.Info.Notes, err = i.cfg.renderResources(ctx, chrt, valuesToRender, i.ReleaseName, i.OutputDir, i.SubNotes, i.UseReleaseName, i.IncludeCRDs, i.PostRenderer, interactWithServer(i.DryRunStrategy), i.EnableDNS, i.HideSecret, i.PostRenderStrategy) + if i.WaitStrategy == kube.OrderedWaitStrategy { + rel.Hooks, manifestDoc, rel.Info.Notes, sortedManifests, err = i.cfg.renderResourcesWithFiles(ctx, chrt, valuesToRender, i.ReleaseName, i.OutputDir, i.SubNotes, i.UseReleaseName, i.IncludeCRDs, i.PostRenderer, interactWithServer(i.DryRunStrategy), i.EnableDNS, i.HideSecret, i.PostRenderStrategy) + } else { + rel.Hooks, manifestDoc, rel.Info.Notes, err = i.cfg.renderResources(ctx, chrt, valuesToRender, i.ReleaseName, i.OutputDir, i.SubNotes, i.UseReleaseName, i.IncludeCRDs, i.PostRenderer, interactWithServer(i.DryRunStrategy), i.EnableDNS, i.HideSecret, i.PostRenderStrategy) + } // Even for errors, attach this if available if manifestDoc != nil { rel.Manifest = manifestDoc.String() @@ -425,6 +439,18 @@ func (i *Install) RunWithContext(ctx context.Context, ch ci.Charter, vals map[st return rel, nil } + var plan *sequence.Plan + if i.WaitStrategy == kube.OrderedWaitStrategy { + // Build the deployment plan before any cluster or storage side effect: + // DAG errors (cycles, duplicate group membership) now fail cleanly with + // nothing stored and nothing applied. + plan, err = sequence.Build(chrt, sortedManifests) + if err != nil { + rel.SetStatus(rcommon.StatusFailed, "failed to build sequencing plan: "+err.Error()) + return rel, err + } + } + if i.CreateNamespace { ns := &v1.Namespace{ TypeMeta: metav1.TypeMeta{ @@ -469,8 +495,15 @@ func (i *Install) RunWithContext(ctx context.Context, ch ci.Charter, vals map[st return rel, err } - rel, err = i.performInstallCtx(ctx, rel, toBeAdopted, resources) + if i.WaitStrategy == kube.OrderedWaitStrategy { + rel, err = i.performSequencedInstallCtx(ctx, rel, plan, toBeAdopted) + } else { + rel, err = i.performInstallCtx(ctx, rel, toBeAdopted, resources) + } if err != nil { + // failRelease cleans up a failed install via the context-free Uninstall.Run + // public API (upstream keeps Uninstall context-free); threading ctx through the + // whole uninstall subsystem is out of scope for this best-effort recovery path. rel, err = i.failRelease(rel, err) } return rel, err @@ -498,6 +531,89 @@ func (i *Install) performInstallCtx(ctx context.Context, rel *release.Release, t } } +// performSequencedInstallCtx runs performSequencedInstall in a goroutine and +// respects context cancellation, mirroring performInstallCtx. +func (i *Install) performSequencedInstallCtx(ctx context.Context, rel *release.Release, plan *sequence.Plan, toBeAdopted kube.ResourceList) (*release.Release, error) { + type Msg struct { + r *release.Release + e error + } + + resultChan := make(chan Msg, 1) + + go func() { + i.goroutineCount.Add(1) + rel, err := i.performSequencedInstall(ctx, rel, plan, toBeAdopted) + resultChan <- Msg{rel, err} + i.goroutineCount.Add(-1) + }() + + select { + case <-ctx.Done(): + return rel, ctx.Err() + case msg := <-resultChan: + return msg.r, msg.e + } +} + +// performSequencedInstall deploys chart resources in DAG-ordered batches when +// --wait=ordered is active. +func (i *Install) performSequencedInstall(ctx context.Context, rel *release.Release, plan *sequence.Plan, toBeAdopted kube.ResourceList) (*release.Release, error) { + if !i.DisableHooks { + if err := i.cfg.execHook(rel, release.HookPreInstall, i.WaitStrategy, i.WaitOptions, i.Timeout, i.ServerSideApply); err != nil { + return rel, fmt.Errorf("failed pre-install: %w", err) + } + } + + logPlanWarnings(i.cfg.Logger(), plan) + + readinessTimeout := i.ReadinessTimeout + if readinessTimeout <= 0 { + readinessTimeout = time.Minute + } + + sd := &sequencedDeployment{ + cfg: i.cfg, + releaseName: rel.Name, + releaseNamespace: rel.Namespace, + disableOpenAPI: i.DisableOpenAPIValidation, + serverSideApply: i.ServerSideApply, + forceConflicts: i.ForceConflicts, + forceReplace: i.ForceReplace, + waitStrategy: i.WaitStrategy, + waitOptions: i.WaitOptions, + waitForJobs: i.WaitForJobs, + timeout: i.Timeout, + readinessTimeout: readinessTimeout, + deadline: computeDeadline(i.Timeout), + currentResources: toBeAdopted, + upgradeCSAFieldManager: true, + threeWayMergeForUnstructured: i.TakeOwnership && !i.ServerSideApply, + } + + if err := sd.apply(ctx, plan); err != nil { + return rel, err + } + + if !i.DisableHooks { + if err := i.cfg.execHook(rel, release.HookPostInstall, i.WaitStrategy, i.WaitOptions, i.Timeout, i.ServerSideApply); err != nil { + return rel, fmt.Errorf("failed post-install: %w", err) + } + } + + if len(i.Description) > 0 { + rel.SetStatus(rcommon.StatusDeployed, i.Description) + } else { + rel.SetStatus(rcommon.StatusDeployed, "Install complete") + } + + if err := i.recordRelease(rel); err != nil { + i.cfg.Logger().Error("failed to record the release", slog.Any("error", err)) + } + + return rel, nil +} + // getGoroutineCount return the number of running routines func (i *Install) getGoroutineCount() int32 { return i.goroutineCount.Load() @@ -512,6 +628,10 @@ func (i *Install) performInstall(rel *release.Release, toBeAdopted kube.Resource } } + if err := stripSequencingAnnotations(resources); err != nil { + return rel, fmt.Errorf("stripping sequencing annotations: %w", err) + } + // At this point, we can do the install. Note that before we were detecting whether to // do an update, but it's not clear whether we WANT to do an update if the reuse is set // to true, since that is basically an upgrade operation. @@ -674,6 +794,10 @@ func (i *Install) createRelease(chrt *chart.Chart, rawVals map[string]any, label Version: 1, Labels: labels, ApplyMethod: string(determineReleaseSSApplyMethod(i.ServerSideApply)), + // Sequenced is set before the release record is first persisted so any + // failure-cleanup path (uninstall via RollbackOnFailure) sees the + // ordering intent even when the install dies mid-deployment (ll8/0vr). + Sequenced: i.WaitStrategy == kube.OrderedWaitStrategy, } return r diff --git a/pkg/action/rollback.go b/pkg/action/rollback.go index 6fc449c30..21d0a0f1d 100644 --- a/pkg/action/rollback.go +++ b/pkg/action/rollback.go @@ -18,6 +18,7 @@ package action import ( "bytes" + "context" "errors" "fmt" "time" @@ -28,6 +29,7 @@ import ( "helm.sh/helm/v4/pkg/kube" "helm.sh/helm/v4/pkg/release/common" release "helm.sh/helm/v4/pkg/release/v1" + "helm.sh/helm/v4/pkg/release/v1/sequence" "helm.sh/helm/v4/pkg/storage/driver" ) @@ -37,12 +39,13 @@ import ( type Rollback struct { cfg *Configuration - Version int - Timeout time.Duration - WaitStrategy kube.WaitStrategy - WaitOptions []kube.WaitOption - WaitForJobs bool - DisableHooks bool + Version int + Timeout time.Duration + ReadinessTimeout time.Duration + WaitStrategy kube.WaitStrategy + WaitOptions []kube.WaitOption + WaitForJobs bool + DisableHooks bool // DryRunStrategy can be set to prepare, but not execute the operation and whether or not to interact with the remote cluster DryRunStrategy DryRunStrategy // ForceReplace will, if set to `true`, ignore certain warnings and perform the rollback anyway. @@ -72,10 +75,22 @@ func NewRollback(cfg *Configuration) *Rollback { // Run executes 'helm rollback' against the given release. func (r *Rollback) Run(name string) error { + return r.RunWithContext(context.Background(), name) +} + +// RunWithContext executes 'helm rollback' against the given release, propagating +// ctx to the sequenced-wait path. It mirrors the Run/RunWithContext split used by +// Install and Upgrade so callers that already hold a context (e.g. upgrade's +// rollback-on-failure) can thread it through instead of starting a fresh one. +func (r *Rollback) RunWithContext(ctx context.Context, name string) error { if err := r.cfg.KubeClient.IsReachable(); err != nil { return err } + if r.ReadinessTimeout > 0 && r.Timeout > 0 && r.ReadinessTimeout > r.Timeout { + return fmt.Errorf("--readiness-timeout (%s) must not exceed --timeout (%s)", r.ReadinessTimeout, r.Timeout) + } + r.cfg.Releases.MaxHistory = r.MaxHistory r.cfg.Logger().Debug("preparing rollback", "name", name) @@ -92,7 +107,7 @@ func (r *Rollback) Run(name string) error { } r.cfg.Logger().Debug("performing rollback", "name", name) - if _, err := r.performRollback(currentRelease, targetRelease, serverSideApply); err != nil { + if _, err := r.performRollback(ctx, currentRelease, targetRelease, serverSideApply); err != nil { return err } @@ -190,17 +205,22 @@ func (r *Rollback) prepareRollback(name string) (*release.Release, *release.Rele Manifest: previousRelease.Manifest, Hooks: previousRelease.Hooks, ApplyMethod: string(determineReleaseSSApplyMethod(serverSideApply)), + Sequenced: previousRelease.IsSequenced(), } return currentRelease, targetRelease, serverSideApply, nil } -func (r *Rollback) performRollback(currentRelease, targetRelease *release.Release, serverSideApply bool) (*release.Release, error) { +func (r *Rollback) performRollback(ctx context.Context, currentRelease, targetRelease *release.Release, serverSideApply bool) (*release.Release, error) { if isDryRun(r.DryRunStrategy) { r.cfg.Logger().Debug("dry run", "name", targetRelease.Name) return targetRelease, nil } + if targetRelease.IsSequenced() { + return r.performSequencedRollback(ctx, currentRelease, targetRelease, serverSideApply) + } + current, err := r.cfg.KubeClient.Build(bytes.NewBufferString(currentRelease.Manifest), false) if err != nil { return targetRelease, fmt.Errorf("unable to build kubernetes objects from current release manifest: %w", err) @@ -225,6 +245,19 @@ func (r *Rollback) performRollback(currentRelease, targetRelease *release.Releas if err != nil { return targetRelease, fmt.Errorf("unable to set metadata visitor from target release: %w", err) } + + // Strip Helm-internal sequencing annotations from both current and target + // before applying to K8s. The stored Manifest in a release secret retains + // the raw annotations even though they were stripped before the original + // apply; without this strip, SSA on rollback rejects the multi-slash + // annotation key (e.g. helm.sh/depends-on/resource-groups) as invalid. + if err := stripSequencingAnnotations(current); err != nil { + return targetRelease, fmt.Errorf("stripping sequencing annotations from current release: %w", err) + } + if err := stripSequencingAnnotations(target); err != nil { + return targetRelease, fmt.Errorf("stripping sequencing annotations from target release: %w", err) + } + results, err := r.cfg.KubeClient.Update( current, target, @@ -305,3 +338,151 @@ func (r *Rollback) performRollback(currentRelease, targetRelease *release.Releas return targetRelease, nil } + +func (r *Rollback) failRollback(currentRelease, targetRelease *release.Release, created kube.ResourceList, err error) (*release.Release, error) { + msg := fmt.Sprintf("Rollback %q failed: %s", targetRelease.Name, err) + r.cfg.Logger().Warn(msg) + + currentRelease.Info.Status = common.StatusSuperseded + targetRelease.Info.Status = common.StatusFailed + targetRelease.Info.Description = msg + r.cfg.recordRelease(currentRelease) + r.cfg.recordRelease(targetRelease) + + if r.CleanupOnFail && len(created) > 0 { + r.cfg.Logger().Debug("cleanup on fail set, cleaning up resources", "count", len(created)) + _, errs := r.cfg.KubeClient.Delete(created, metav1.DeletePropagationBackground) + if errs != nil { + return targetRelease, fmt.Errorf( + "an error occurred while cleaning up resources after rollback failure: %w: %w", + err, + joinErrors(errs, ", "), + ) + } + r.cfg.Logger().Debug("resource cleanup complete") + } + + return targetRelease, err +} + +// performSequencedRollback deploys the target revision's resources in DAG-ordered batches +// when the target revision was originally deployed with --wait=ordered. +func (r *Rollback) performSequencedRollback(ctx context.Context, currentRelease, targetRelease *release.Release, serverSideApply bool) (*release.Release, error) { + fail := func(created kube.ResourceList, err error) (*release.Release, error) { + return r.failRollback(currentRelease, targetRelease, created, err) + } + + current, err := r.cfg.KubeClient.Build(bytes.NewBufferString(currentRelease.Manifest), false) + if err != nil { + return fail(nil, fmt.Errorf("unable to build kubernetes objects from current release manifest: %w", err)) + } + if err := stripSequencingAnnotations(current); err != nil { + return fail(nil, fmt.Errorf("stripping sequencing annotations from current release: %w", err)) + } + + target, err := r.cfg.KubeClient.Build(bytes.NewBufferString(targetRelease.Manifest), false) + if err != nil { + return fail(nil, fmt.Errorf("unable to build kubernetes objects from target release manifest: %w", err)) + } + + sortedManifests, err := sequence.ParseStoredManifests(targetRelease.Manifest) + if err != nil { + return fail(nil, fmt.Errorf("parsing target release manifest for sequenced rollback: %w", err)) + } + + plan, err := sequence.Build(targetRelease.Chart, sortedManifests) + if err != nil { + return fail(nil, fmt.Errorf("building sequencing plan for rollback: %w", err)) + } + logPlanWarnings(r.cfg.Logger(), plan) + + if !r.DisableHooks { + if err := r.cfg.execHook(targetRelease, release.HookPreRollback, r.WaitStrategy, r.WaitOptions, r.Timeout, serverSideApply); err != nil { + return fail(nil, err) + } + } else { + r.cfg.Logger().Debug("rollback hooks disabled", "name", targetRelease.Name) + } + + readinessTimeout := r.ReadinessTimeout + if readinessTimeout <= 0 { + readinessTimeout = time.Minute + } + + gatingStrategy := r.WaitStrategy + if gatingStrategy == kube.HookOnlyStrategy { + r.cfg.Logger().Info("release was deployed with ordered sequencing; using the status watcher to gate rollback batches (hooks keep the hook-only strategy)", "release", targetRelease.Name) + gatingStrategy = kube.StatusWatcherStrategy + } + + sd := &sequencedDeployment{ + cfg: r.cfg, + releaseName: targetRelease.Name, + releaseNamespace: targetRelease.Namespace, + serverSideApply: serverSideApply, + forceConflicts: r.ForceConflicts, + forceReplace: r.ForceReplace, + waitStrategy: gatingStrategy, + waitOptions: r.WaitOptions, + waitForJobs: r.WaitForJobs, + timeout: r.Timeout, + readinessTimeout: readinessTimeout, + deadline: computeDeadline(r.Timeout), + upgradeMode: true, + currentResources: current, + upgradeCSAFieldManager: true, + } + + if err := sd.apply(ctx, plan); err != nil { + return fail(sd.createdResources, err) + } + + allTargetKeys := make(map[string]bool, len(target)) + for _, resource := range target { + allTargetKeys[objectKey(resource)] = true + } + + removedKeys := make(map[string]bool) + var toBeDeleted kube.ResourceList + for _, resource := range current { + if !allTargetKeys[objectKey(resource)] { + removedKeys[objectKey(resource)] = true + toBeDeleted = append(toBeDeleted, resource) + } + } + + if len(toBeDeleted) > 0 { + waiter, err := getWaiterFor(r.cfg.KubeClient, gatingStrategy, r.WaitOptions...) + if err != nil { + return fail(sd.createdResources, fmt.Errorf("getting waiter for removed-resource deletion: %w", err)) + } + if err := sd.deleteRemovedFromOldRelease(currentRelease, removedKeys, toBeDeleted, waiter); err != nil { + return fail(sd.createdResources, fmt.Errorf("deleting removed resources during rollback: %w", err)) + } + } + + if !r.DisableHooks { + if err := r.cfg.execHook(targetRelease, release.HookPostRollback, r.WaitStrategy, r.WaitOptions, r.Timeout, serverSideApply); err != nil { + return fail(sd.createdResources, err) + } + } + + deployed, err := r.cfg.Releases.DeployedAll(currentRelease.Name) + if err != nil && !errors.Is(err, driver.ErrNoDeployedReleases) { + return fail(sd.createdResources, err) + } + + for _, reli := range deployed { + rel, err := releaserToV1Release(reli) + if err != nil { + return fail(sd.createdResources, err) + } + r.cfg.Logger().Debug("superseding previous deployment", "version", rel.Version) + rel.Info.Status = common.StatusSuperseded + r.cfg.recordRelease(rel) + } + + targetRelease.Info.Status = common.StatusDeployed + + return targetRelease, nil +} diff --git a/pkg/action/rollback_sequenced_test.go b/pkg/action/rollback_sequenced_test.go new file mode 100644 index 000000000..8c258f8bf --- /dev/null +++ b/pkg/action/rollback_sequenced_test.go @@ -0,0 +1,498 @@ +/* +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 action + +import ( + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "helm.sh/helm/v4/pkg/chart/common" + chart "helm.sh/helm/v4/pkg/chart/v2" + "helm.sh/helm/v4/pkg/kube" + rcommon "helm.sh/helm/v4/pkg/release/common" + release "helm.sh/helm/v4/pkg/release/v1" + releaseutil "helm.sh/helm/v4/pkg/release/v1/util" +) + +func newRollbackAction(t *testing.T, kubeClient kube.Interface) *Rollback { + t.Helper() + + cfg := actionConfigFixture(t) + cfg.KubeClient = kubeClient + + rollback := NewRollback(cfg) + rollback.Timeout = 5 * time.Minute + rollback.WaitStrategy = kube.OrderedWaitStrategy + rollback.ServerSideApply = "auto" + + return rollback +} + +func seedRollbackRelease(t *testing.T, rollback *Rollback, name string, version int, status rcommon.Status, ch *chart.Chart, manifest string, sequencingInfo *release.SequencingInfo) { + t.Helper() + + now := time.Now() + rel := &release.Release{ + Name: name, + Namespace: "spaced", + Chart: ch, + Config: map[string]any{}, + Manifest: manifest, + Info: &release.Info{ + FirstDeployed: now, + LastDeployed: now, + Status: status, + Description: "Seeded release", + }, + Version: version, + ApplyMethod: "csa", + SequencingInfo: sequencingInfo, + } + + require.NoError(t, rollback.cfg.Releases.Create(rel)) +} + +func storedRollbackRelease(t *testing.T, rollback *Rollback, name string, version int) *release.Release { + t.Helper() + + reli, err := rollback.cfg.Releases.Get(name, version) + require.NoError(t, err) + + rel, err := releaserToV1Release(reli) + require.NoError(t, err) + + return rel +} + +func latestRollbackRelease(t *testing.T, rollback *Rollback, name string) *release.Release { + t.Helper() + + reli, err := rollback.cfg.Releases.Last(name) + require.NoError(t, err) + + rel, err := releaserToV1Release(reli) + require.NoError(t, err) + + return rel +} + +func TestRollback_Sequenced_Basic(t *testing.T) { + client := newRecordingKubeClient() + rollback := newRollbackAction(t, client) + + targetChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("rollback-basic")) + + sequencingInfo := &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)} + seedRollbackRelease(t, rollback, "rollback-basic", 1, rcommon.StatusSuperseded, targetChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), sequencingInfo) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + makeConfigMapTemplate("templates/app.yaml", "app", nil), + }, withName("rollback-basic")) + seedRollbackRelease(t, rollback, "rollback-basic", 2, rcommon.StatusDeployed, currentChart, joinManifestDocs( + configMapManifest("database", nil), + configMapManifest("app", nil), + ), nil) + + rollback.Version = 1 + require.NoError(t, rollback.Run("rollback-basic")) + + require.Len(t, client.updateCalls, 2) + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/app"}}, updateTargets(client.updateCalls)) + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/app"}}, client.waitCalls) + assert.Equal(t, []string{"ConfigMap/database"}, client.updateCalls[0].current) + assert.Equal(t, []string{"ConfigMap/app"}, client.updateCalls[1].current) + + rel := latestRollbackRelease(t, rollback, "rollback-basic") + assert.Equal(t, 3, rel.Version) + assert.Equal(t, rcommon.StatusDeployed, rel.Info.Status) + assert.True(t, rel.IsSequenced()) + assert.True(t, rel.Sequenced) + assert.Nil(t, rel.SequencingInfo) +} + +// TestRollback_Sequenced_RemovedGroupsDeletedInReverseOrder locks bead +// 7yi-rollback: resources present in the from-revision but absent from the +// rollback target must be deleted batch-by-batch in exact reverse deployment +// order of the FROM revision's plan, with a delete-wait gating each batch, +// not in one unordered bulk call. +func TestRollback_Sequenced_RemovedGroupsDeletedInReverseOrder(t *testing.T) { + client := newRecordingKubeClient() + rollback := newRollbackAction(t, client) + + targetChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/app.yaml", "app", nil), + }, withName("rollback-removed")) + seedRollbackRelease(t, rollback, "rollback-removed", 1, rcommon.StatusSuperseded, targetChart, configMapManifest("app", nil), &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/queue.yaml", "queue", map[string]string{ + releaseutil.AnnotationResourceGroup: "queue", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + makeConfigMapTemplate("templates/app.yaml", "app", nil), + }, withName("rollback-removed")) + seedRollbackRelease(t, rollback, "rollback-removed", 2, rcommon.StatusDeployed, currentChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("queue", map[string]string{ + releaseutil.AnnotationResourceGroup: "queue", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + configMapManifest("app", nil), + ), &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}) + + rollback.Version = 1 + require.NoError(t, rollback.Run("rollback-removed")) + + assert.Equal(t, [][]string{{"ConfigMap/queue"}, {"ConfigMap/database"}}, client.deleteCalls) + assert.Equal(t, client.deleteCalls, client.deleteWaitCalls) +} + +// TestRollback_Sequenced_DefaultWaitStrategyGatesBatches locks bead 4sj for +// rollback: a rollback to a sequenced revision under the DEFAULT hookOnly +// strategy must gate each re-deploy batch on real readiness waits. +func TestRollback_Sequenced_DefaultWaitStrategyGatesBatches(t *testing.T) { + client := newRecordingKubeClient() + cfg := actionConfigFixture(t) + cfg.KubeClient = client + + rollback := NewRollback(cfg) + rollback.Timeout = 5 * time.Minute + rollback.WaitStrategy = kube.HookOnlyStrategy + + targetChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("rollback-default-wait")) + + now := time.Now() + require.NoError(t, rollback.cfg.Releases.Create(&release.Release{ + Name: "rollback-default-wait", + Namespace: "spaced", + Chart: targetChart, + Config: map[string]any{}, + Manifest: joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), + Info: &release.Info{ + FirstDeployed: now, + LastDeployed: now, + Status: rcommon.StatusSuperseded, + Description: "Seeded release", + }, + Version: 1, + ApplyMethod: "csa", + Sequenced: true, + })) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + makeConfigMapTemplate("templates/app.yaml", "app", nil), + }, withName("rollback-default-wait")) + seedRollbackRelease(t, rollback, "rollback-default-wait", 2, rcommon.StatusDeployed, currentChart, joinManifestDocs( + configMapManifest("database", nil), + configMapManifest("app", nil), + ), nil) + + rollback.Version = 1 + require.NoError(t, rollback.Run("rollback-default-wait")) + + require.Len(t, client.updateCalls, 2) + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/app"}}, updateTargets(client.updateCalls)) + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/app"}}, client.waitCalls) + assert.Contains(t, client.waiterStrategies, kube.StatusWatcherStrategy) +} + +func TestRollback_ToNonSequenced(t *testing.T) { + client := newRecordingKubeClient() + rollback := newRollbackAction(t, client) + + targetChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("rollback-to-standard")) + seedRollbackRelease(t, rollback, "rollback-to-standard", 1, rcommon.StatusSuperseded, targetChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), nil) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + makeConfigMapTemplate("templates/app.yaml", "app", nil), + }, withName("rollback-to-standard")) + seedRollbackRelease(t, rollback, "rollback-to-standard", 2, rcommon.StatusDeployed, currentChart, joinManifestDocs( + configMapManifest("database", nil), + configMapManifest("app", nil), + ), nil) + + rollback.Version = 1 + require.NoError(t, rollback.Run("rollback-to-standard")) + + require.Len(t, client.updateCalls, 1) + assert.ElementsMatch(t, []string{"ConfigMap/database", "ConfigMap/app"}, client.updateCalls[0].target) + require.Len(t, client.waitCalls, 1) + assert.ElementsMatch(t, []string{"ConfigMap/database", "ConfigMap/app"}, client.waitCalls[0]) + assert.False(t, latestRollbackRelease(t, rollback, "rollback-to-standard").IsSequenced()) +} + +func TestRollback_FromSequencedToNonSequenced(t *testing.T) { + client := newRecordingKubeClient() + rollback := newRollbackAction(t, client) + + targetChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + makeConfigMapTemplate("templates/app.yaml", "app", nil), + }, withName("rollback-transition")) + seedRollbackRelease(t, rollback, "rollback-transition", 1, rcommon.StatusSuperseded, targetChart, joinManifestDocs( + configMapManifest("database", nil), + configMapManifest("app", nil), + ), nil) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("rollback-transition")) + seedRollbackRelease(t, rollback, "rollback-transition", 2, rcommon.StatusDeployed, currentChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}) + + rollback.Version = 1 + require.NoError(t, rollback.Run("rollback-transition")) + + require.Len(t, client.updateCalls, 1) + assert.ElementsMatch(t, []string{"ConfigMap/database", "ConfigMap/app"}, client.updateCalls[0].target) + assert.False(t, latestRollbackRelease(t, rollback, "rollback-transition").IsSequenced()) +} + +func TestRollback_SequencedFlagPreservedFromLegacyRelease(t *testing.T) { + client := newRecordingKubeClient() + rollback := newRollbackAction(t, client) + + targetChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + }, withName("rollback-sequencing-info")) + sequencingInfo := &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)} + seedRollbackRelease(t, rollback, "rollback-sequencing-info", 1, rcommon.StatusSuperseded, targetChart, configMapManifest("database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), sequencingInfo) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + }, withName("rollback-sequencing-info")) + seedRollbackRelease(t, rollback, "rollback-sequencing-info", 2, rcommon.StatusDeployed, currentChart, configMapManifest("database", nil), nil) + + rollback.Version = 1 + require.NoError(t, rollback.Run("rollback-sequencing-info")) + + rel := latestRollbackRelease(t, rollback, "rollback-sequencing-info") + assert.True(t, rel.IsSequenced()) + assert.True(t, rel.Sequenced) + assert.Nil(t, rel.SequencingInfo) +} + +func TestRollback_Sequenced_Failure(t *testing.T) { + client := newRecordingKubeClient() + client.waitErrorOnCall = 2 + client.waitError = errors.New("timed out waiting for batch") + + rollback := newRollbackAction(t, client) + + targetChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/queue.yaml", "queue", map[string]string{ + releaseutil.AnnotationResourceGroup: "queue", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["queue"]`, + }), + }, withName("rollback-failure")) + seedRollbackRelease(t, rollback, "rollback-failure", 1, rcommon.StatusSuperseded, targetChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("queue", map[string]string{ + releaseutil.AnnotationResourceGroup: "queue", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["queue"]`, + }), + ), &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + makeConfigMapTemplate("templates/app.yaml", "app", nil), + }, withName("rollback-failure")) + seedRollbackRelease(t, rollback, "rollback-failure", 2, rcommon.StatusDeployed, currentChart, joinManifestDocs( + configMapManifest("database", nil), + configMapManifest("app", nil), + ), nil) + + rollback.Version = 1 + err := rollback.Run("rollback-failure") + require.Error(t, err) + assert.Contains(t, err.Error(), "timed out waiting for batch") + + require.Len(t, client.updateCalls, 2) + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/queue"}}, updateTargets(client.updateCalls)) + assert.Empty(t, client.deleteCalls) + + assert.Equal(t, rcommon.StatusFailed, latestRollbackRelease(t, rollback, "rollback-failure").Info.Status) + assert.Equal(t, rcommon.StatusSuperseded, storedRollbackRelease(t, rollback, "rollback-failure", 2).Info.Status) +} + +func TestRollback_Sequenced_CleanupOnFail(t *testing.T) { + client := newRecordingKubeClient() + client.waitErrorOnCall = 2 + client.waitError = errors.New("timed out waiting for batch") + + rollback := newRollbackAction(t, client) + rollback.CleanupOnFail = true + + targetChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/queue.yaml", "queue", map[string]string{ + releaseutil.AnnotationResourceGroup: "queue", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["queue"]`, + }), + }, withName("rollback-cleanup")) + seedRollbackRelease(t, rollback, "rollback-cleanup", 1, rcommon.StatusSuperseded, targetChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("queue", map[string]string{ + releaseutil.AnnotationResourceGroup: "queue", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["queue"]`, + }), + ), &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + makeConfigMapTemplate("templates/app.yaml", "app", nil), + }, withName("rollback-cleanup")) + seedRollbackRelease(t, rollback, "rollback-cleanup", 2, rcommon.StatusDeployed, currentChart, joinManifestDocs( + configMapManifest("database", nil), + configMapManifest("app", nil), + ), nil) + + rollback.Version = 1 + err := rollback.Run("rollback-cleanup") + require.Error(t, err) + assert.Contains(t, err.Error(), "timed out waiting for batch") + assert.Equal(t, [][]string{{"ConfigMap/queue"}}, client.deleteCalls) + assert.Equal(t, rcommon.StatusFailed, latestRollbackRelease(t, rollback, "rollback-cleanup").Info.Status) +} + +func TestRollback_Sequenced_DryRun(t *testing.T) { + client := newRecordingKubeClient() + rollback := newRollbackAction(t, client) + rollback.DryRunStrategy = DryRunClient + + targetChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("rollback-dry-run")) + seedRollbackRelease(t, rollback, "rollback-dry-run", 1, rcommon.StatusSuperseded, targetChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + makeConfigMapTemplate("templates/app.yaml", "app", nil), + }, withName("rollback-dry-run")) + seedRollbackRelease(t, rollback, "rollback-dry-run", 2, rcommon.StatusDeployed, currentChart, joinManifestDocs( + configMapManifest("database", nil), + configMapManifest("app", nil), + ), nil) + + rollback.Version = 1 + require.NoError(t, rollback.Run("rollback-dry-run")) + + assert.Empty(t, client.updateCalls) + assert.Empty(t, client.waitCalls) + assert.Empty(t, client.deleteCalls) + assert.Equal(t, 2, latestRollbackRelease(t, rollback, "rollback-dry-run").Version) +} diff --git a/pkg/action/rollback_test.go b/pkg/action/rollback_test.go index b34adda91..0dc55f765 100644 --- a/pkg/action/rollback_test.go +++ b/pkg/action/rollback_test.go @@ -27,6 +27,9 @@ import ( "helm.sh/helm/v4/pkg/kube" kubefake "helm.sh/helm/v4/pkg/kube/fake" + rcommon "helm.sh/helm/v4/pkg/release/common" + release "helm.sh/helm/v4/pkg/release/v1" + releaseutil "helm.sh/helm/v4/pkg/release/v1/util" ) func TestNewRollback(t *testing.T) { @@ -131,3 +134,85 @@ func TestRollbackRevisionZeroForNonRollback(t *testing.T) { assert.Equal(t, 0, r.Info.RollbackRevision) } + +// TestRollback_StripsSequencingAnnotationsOnPlainPath locks the fix for cap-21 +// (hip-0025-r5y). Scenario: a release was installed plain at rev-1, upgraded +// with sequencing at rev-2 (manifest stored in the secret retains the raw +// helm.sh/depends-on/resource-groups annotation), then rolled back to rev-1. +// Because targetRelease.SequencingInfo is unset, performRollback falls through +// to the plain (non-sequenced) UPDATE path. Without stripping, SSA on the +// rollback rejects the multi-slash annotation key as invalid. This test +// verifies that BOTH current (rev-2 manifest) and target (rev-1 manifest) are +// passed through stripSequencingAnnotations before KubeClient.Update is called. +func TestRollback_StripsSequencingAnnotationsOnPlainPath(t *testing.T) { + const annotation = releaseutil.AnnotationDependsOnResourceGroups + + // rev-2's manifest carries the helm-internal annotation because the + // rendered template kept it; the live K8s objects had it stripped by the + // sequenced upgrade path, but the secret-stored Manifest still has it. + sequencedManifest := `apiVersion: v1 +kind: ConfigMap +metadata: + name: cap21-cm + namespace: spaced + annotations: + "` + annotation + `": "[other-group]" +data: + k: v +` + + // rev-1's manifest is plain. + plainManifest := `apiVersion: v1 +kind: ConfigMap +metadata: + name: cap21-cm + namespace: spaced +data: + k: v +` + + rev1 := releaseStub() + rev1.Name = "cap21" + rev1.Version = 1 + rev1.Info.Status = rcommon.StatusSuperseded + rev1.Manifest = plainManifest + rev1.SequencingInfo = nil // plain install + + rev2 := releaseStub() + rev2.Name = "cap21" + rev2.Version = 2 + rev2.Info.Status = rcommon.StatusDeployed + rev2.Manifest = sequencedManifest + rev2.SequencingInfo = &release.SequencingInfo{Enabled: true, Strategy: "ordered"} + + cfg := actionConfigFixture(t) + require.NoError(t, cfg.Releases.Create(rev1)) + require.NoError(t, cfg.Releases.Create(rev2)) + + recorder := newRecordingKubeClient() + cfg.KubeClient = recorder + + client := NewRollback(cfg) + client.Version = 1 + client.DisableHooks = true + require.NoError(t, client.Run("cap21")) + + require.Len(t, recorder.updateCalls, 1, "exactly one KubeClient.Update call expected on plain rollback path") + call := recorder.updateCalls[0] + + assertNoSequencingAnnotation(t, "current", call.currentResources, annotation) + assertNoSequencingAnnotation(t, "target", call.targetResources, annotation) +} + +func assertNoSequencingAnnotation(t *testing.T, label string, resources kube.ResourceList, key string) { + t.Helper() + for _, info := range resources { + acc := info.Object.(interface { + GetAnnotations() map[string]string + }) + anns := acc.GetAnnotations() + if _, present := anns[key]; present { + t.Fatalf("%s resource %q still carries stripped annotation %q after rollback", label, info.Name, key) + } + } +} diff --git a/pkg/action/sequencing.go b/pkg/action/sequencing.go new file mode 100644 index 000000000..fc1554e2a --- /dev/null +++ b/pkg/action/sequencing.go @@ -0,0 +1,375 @@ +/* +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 action + +import ( + "bytes" + "context" + "errors" + "fmt" + "log/slog" + "strings" + "time" + + "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" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/cli-runtime/pkg/resource" +) + +// 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. +// 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 { + if len(manifests) == 0 { + return "" + } + var buf strings.Builder + for i, m := range manifests { + if i > 0 { + buf.WriteString("---\n") + } + buf.WriteString(m.Content) + buf.WriteString("\n") + } + return buf.String() +} + +// sequencedDeployment executes a sequence.Plan: ordered installation, upgrade, +// or rollback of chart resources, one batch at a time, gating each batch on +// readiness before the next starts. +type sequencedDeployment struct { + cfg *Configuration + releaseName string + releaseNamespace string + disableOpenAPI bool + serverSideApply bool + forceConflicts bool + forceReplace bool + waitStrategy kube.WaitStrategy + waitOptions []kube.WaitOption + waitForJobs bool + timeout time.Duration + readinessTimeout time.Duration + deadline time.Time // overall operation deadline + + // upgradeMode forces every batch through KubeClient.Update. When false + // (install), a batch is still applied via Update when it intersects + // currentResources (resources adopted with --take-ownership). + upgradeMode bool + // currentResources are the objects a batch may be an update of: the full + // old-release resource list on upgrade/rollback, the toBeAdopted set on + // install. Matched per batch by objectKey. + currentResources kube.ResourceList + upgradeCSAFieldManager bool // upgrade client-side apply field manager + // threeWayMergeForUnstructured mirrors performInstall's adoption Update + // (TakeOwnership && !ServerSideApply); always false on upgrade/rollback. + threeWayMergeForUnstructured bool + createdResources kube.ResourceList +} + +// logPlanWarnings surfaces non-fatal sequencing-plan warnings to the user. +func logPlanWarnings(logger *slog.Logger, plan *sequence.Plan) { + for _, w := range plan.Warnings { + logger.Warn("sequencing: "+w.Message, "chart", w.ChartPath) + } +} + +// getWaiterFor returns a waiter for the given strategy, preferring the +// options-aware interface when the client supports it. +func getWaiterFor(client kube.Interface, ws kube.WaitStrategy, opts ...kube.WaitOption) (kube.Waiter, error) { + if c, ok := client.(kube.InterfaceWaitOptions); ok { + return c.GetWaiterWithOptions(ws, opts...) + } + return client.GetWaiter(ws) +} + +// apply deploys the plan batch by batch, in order. +func (s *sequencedDeployment) apply(ctx context.Context, plan *sequence.Plan) error { + for _, batch := range plan.Batches { + if err := s.applyBatch(ctx, batch); err != nil { + return err + } + } + return nil +} + +// applyBatch builds one batch's manifests, sets release metadata, strips +// Helm-internal sequencing annotations, creates or updates the resources, and +// waits for readiness. Context cancellation is honored at each stage boundary: +// before Build, before Create/Update, and before Wait. +func (s *sequencedDeployment) applyBatch(ctx context.Context, batch sequence.Batch) error { + if err := ctx.Err(); err != nil { + return err + } + manifests := batch.Manifests() + if len(manifests) == 0 { + return nil + } + + yaml := buildManifestYAML(manifests) + target, err := s.cfg.KubeClient.Build(bytes.NewBufferString(yaml), !s.disableOpenAPI) + if err != nil { + return fmt.Errorf("building resource batch: %w", err) + } + if len(target) == 0 { + return nil + } + + if err := target.Visit(setMetadataVisitor(s.releaseName, s.releaseNamespace, true)); err != nil { + return fmt.Errorf("setting metadata for resource batch: %w", err) + } + if err := stripSequencingAnnotations(target); err != nil { + return fmt.Errorf("stripping sequencing annotations: %w", err) + } + + if err := ctx.Err(); err != nil { + return err + } + + targetKeys := make(map[string]bool, len(target)) + for _, r := range target { + targetKeys[objectKey(r)] = true + } + var matchingCurrent kube.ResourceList + for _, r := range s.currentResources { + if targetKeys[objectKey(r)] { + matchingCurrent = append(matchingCurrent, r) + } + } + + var result *kube.Result + if s.upgradeMode || len(matchingCurrent) > 0 { + result, err = s.cfg.KubeClient.Update( + matchingCurrent, + target, + kube.ClientUpdateOptionForceReplace(s.forceReplace), + kube.ClientUpdateOptionServerSideApply(s.serverSideApply, s.forceConflicts), + kube.ClientUpdateOptionThreeWayMergeForUnstructured(s.threeWayMergeForUnstructured), + kube.ClientUpdateOptionUpgradeClientSideFieldManager(s.upgradeCSAFieldManager), + ) + if err != nil { + return fmt.Errorf("updating resource batch: %w", err) + } + } else { + result, err = s.cfg.KubeClient.Create( + target, + kube.ClientCreateOptionServerSideApply(s.serverSideApply, false)) + if err != nil { + return fmt.Errorf("creating resource batch: %w", err) + } + } + s.createdResources = append(s.createdResources, result.Created...) + + if err := ctx.Err(); err != nil { + return err + } + + if !batch.Wait { + return nil + } + return s.waitForResources(target, batch.HasCustomReadiness) +} + +// stripSequencingAnnotations removes Helm-internal sequencing annotations from +// resources before they are applied to Kubernetes. This prevents K8s API +// validation errors for annotation keys that are not valid K8s label keys. +func stripSequencingAnnotations(resources kube.ResourceList) error { + return resources.Visit(func(info *resource.Info, err error) error { + if err != nil { + return err + } + acc, accErr := meta.Accessor(info.Object) + if accErr != nil { + return nil // skip non-accessible objects + } + annotations := acc.GetAnnotations() + if len(annotations) == 0 { + return nil + } + changed := false + for _, key := range releaseutil.HelmInternalSequencingAnnotations() { + if _, exists := annotations[key]; exists { + delete(annotations, key) + changed = true + } + } + if changed { + acc.SetAnnotations(annotations) + } + return nil + }) +} + +// deleteRemoved deletes resources of the previous release that are absent from +// the new one, walking an already-reversed plan of the OLD release so removal +// happens in exact reverse deployment order, gating each batch on WaitForDelete +// (HIP-0025 §Sequencing order, bead 7yi). removedKeys holds the objectKey()s +// of resources to delete; batch resources outside the set are skipped. +func (s *sequencedDeployment) deleteRemoved(reversedPlan *sequence.Plan, removedKeys map[string]bool, waiter kube.Waiter) error { + for _, batch := range reversedPlan.Batches { + manifests := batch.Manifests() + if len(manifests) == 0 { + continue + } + resources, err := s.cfg.KubeClient.Build(bytes.NewBufferString(buildManifestYAML(manifests)), false) + if err != nil { + return fmt.Errorf("building removed-resource batch: %w", err) + } + var toDelete kube.ResourceList + for _, r := range resources { + if removedKeys[objectKey(r)] { + toDelete = append(toDelete, r) + } + } + if len(toDelete) == 0 { + continue + } + if _, errs := s.cfg.KubeClient.Delete(toDelete, metav1.DeletePropagationBackground); errs != nil { + return fmt.Errorf("deleting removed resources: %w", joinErrors(errs, ", ")) + } + if !batch.Wait { + continue + } + waitTimeout, err := s.batchWaitTimeout() + if err != nil { + return err + } + if err := waiter.WaitForDelete(toDelete, waitTimeout); err != nil { + return fmt.Errorf("waiting for removed resources to be deleted: %w", err) + } + } + return nil +} + +// deleteRemovedFromOldRelease builds the OLD release's deployment plan from its +// stored manifest and deletes the removed resources batch-by-batch in exact +// reverse deployment order (HIP-0025 §Sequencing order, bead 7yi). When no plan +// can be built from the stored release (pre-sequencing record, unparsable +// manifest), it falls back to one unordered bulk delete with a warning. +func (s *sequencedDeployment) deleteRemovedFromOldRelease(oldRelease *release.Release, removedKeys map[string]bool, toBeDeleted kube.ResourceList, waiter kube.Waiter) error { + oldManifests, err := sequence.ParseStoredManifests(oldRelease.Manifest) + var oldPlan *sequence.Plan + if err == nil { + oldPlan, err = sequence.Build(oldRelease.Chart, oldManifests) + } + if err != nil { + s.cfg.Logger().Warn("unable to build sequencing plan for previous release; deleting removed resources unordered", slog.Any("error", err)) + if _, errs := s.cfg.KubeClient.Delete(toBeDeleted, metav1.DeletePropagationBackground); errs != nil { + return fmt.Errorf("deleting removed resources: %w", joinErrors(errs, ", ")) + } + return nil + } + return s.deleteRemoved(oldPlan.Reverse(), removedKeys, waiter) +} + +// batchWaitTimeout returns the effective per-batch wait timeout: +// min(readinessTimeout, time remaining until the overall deadline). +func (s *sequencedDeployment) batchWaitTimeout() (time.Duration, error) { + waitTimeout := s.readinessTimeout + if !s.deadline.IsZero() { + remaining := time.Until(s.deadline) + if remaining <= 0 { + return 0, errors.New("overall timeout exceeded before waiting for resource batch") + } + if waitTimeout <= 0 || remaining < waitTimeout { + waitTimeout = remaining + } + } + if waitTimeout <= 0 { + waitTimeout = time.Minute // safe default + } + return waitTimeout, nil +} + +// waitForResources waits for the given resources to become ready, applying the +// per-batch and overall deadline constraints. Every batch is waited on (D1): +// leaf-group suppression was removed because it keyed on bare resource names +// (beads xnf/p1m) and made --wait=ordered weaker than --wait for terminal +// groups; Batch.LeafGroups is retained by the builder for display only. +func (s *sequencedDeployment) waitForResources(resources kube.ResourceList, hasCustomReadiness bool) error { + if len(resources) == 0 { + return nil + } + waitTimeout, err := s.batchWaitTimeout() + if err != nil { + return err + } + + var waiter kube.Waiter + waitOptions := append([]kube.WaitOption(nil), s.waitOptions...) + if hasCustomReadiness { + waitOptions = append(waitOptions, kube.WithCustomReadinessStatusReader()) + } + if c, ok := s.cfg.KubeClient.(kube.InterfaceWaitOptions); ok { + waiter, err = c.GetWaiterWithOptions(s.waitStrategy, waitOptions...) + } else { + waiter, err = s.cfg.KubeClient.GetWaiter(s.waitStrategy) + } + if err != nil { + return fmt.Errorf("getting waiter for resource batch: %w", err) + } + + if s.waitForJobs { + return waiter.WaitWithJobs(resources, waitTimeout) + } + return waiter.Wait(resources, waitTimeout) +} diff --git a/pkg/action/sequencing_test.go b/pkg/action/sequencing_test.go new file mode 100644 index 000000000..2ee5fcabf --- /dev/null +++ b/pkg/action/sequencing_test.go @@ -0,0 +1,1339 @@ +/* +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 action + +import ( + "context" + "errors" + "fmt" + "io" + "maps" + "net/http" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + yamlutil "k8s.io/apimachinery/pkg/util/yaml" + "k8s.io/cli-runtime/pkg/resource" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest/fake" + + "helm.sh/helm/v4/pkg/chart/common" + chart "helm.sh/helm/v4/pkg/chart/v2" + "helm.sh/helm/v4/pkg/kube" + kubefake "helm.sh/helm/v4/pkg/kube/fake" + ri "helm.sh/helm/v4/pkg/release" + rcommon "helm.sh/helm/v4/pkg/release/common" + 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" +) + +type recordingKubeClient struct { + kubefake.PrintingKubeClient + createCalls [][]string + updateCalls []updateCall + waitCalls [][]string + watchUntilReadyCalls [][]string + deleteWaitCalls [][]string + deleteCalls [][]string + waiterStrategies []kube.WaitStrategy + operations []string + waitCallCount int + waitErrorOnCall int + waitError error + onBuild func() + // buildRESTClient, when set, attaches a fake REST client to every + // resource.Info produced by Build — needed by tests whose action path + // performs live existence/ownership checks (a real Build always attaches + // a client; most recording tests never hit those paths and omit it). + buildRESTClient func(u *unstructured.Unstructured) resource.RESTClient + // createdObjects tracks "Kind/name" ids that Create has been called for, + // letting stateful fakes answer GETs like a live cluster would. + createdObjects map[string]bool + onCreate func() + onUpdate func() +} + +type updateCall struct { + current []string + target []string + created []string + currentResources kube.ResourceList + targetResources kube.ResourceList +} + +type recordingKubeWaiter struct { + client *recordingKubeClient +} + +func newRecordingKubeClient() *recordingKubeClient { + return &recordingKubeClient{ + PrintingKubeClient: kubefake.PrintingKubeClient{Out: io.Discard, LogOutput: io.Discard}, + // A real Build always attaches a REST client to each Info; mirror + // that with a fake that reports 404 so existence/conflict checks in + // the install/upgrade prologue exercise the real "resource does not + // exist yet" path. Delete-path tests override this (see + // withoutBuildClients) because their ownership checks would read a + // 404 as "already deleted". + buildRESTClient: func(u *unstructured.Unstructured) resource.RESTClient { + return fakeClientWith(http.StatusNotFound, u.GroupVersionKind().GroupVersion(), "") + }, + createdObjects: make(map[string]bool), + } +} + +// withoutBuildClients makes Build produce Infos with no REST client, for +// delete-path tests that rely on the nil-client ownership-verification guard +// (verifyOwnershipBeforeDelete treats a fetch 404 as "already deleted", which +// would wrongly skip deletions for these fixtures). +func (c *recordingKubeClient) withoutBuildClients() *recordingKubeClient { + c.buildRESTClient = nil + return c +} + +// statefulOwnedRESTClient mimics a live cluster across an install-then-delete +// flow: GETs report 404 until Create has been called for the object, and +// afterwards return it labeled/annotated as owned by the given release, so +// post-failure rollback deletion passes ownership checks even though the +// delete path re-Builds fresh resource.Infos from the stored manifest. +func statefulOwnedRESTClient(c *recordingKubeClient, releaseName, releaseNamespace string) func(u *unstructured.Unstructured) resource.RESTClient { + return func(u *unstructured.Unstructured) resource.RESTClient { + gvk := u.GroupVersionKind() + id := fmt.Sprintf("%s/%s", gvk.Kind, u.GetName()) + ownedBody := fmt.Sprintf( + `{"apiVersion":%q,"kind":%q,"metadata":{"name":%q,"labels":{%q:%q},"annotations":{%q:%q,%q:%q}}}`, + gvk.GroupVersion().String(), gvk.Kind, u.GetName(), + appManagedByLabel, appManagedByHelm, + helmReleaseNameAnnotation, releaseName, + helmReleaseNamespaceAnnotation, releaseNamespace, + ) + return &fake.RESTClient{ + GroupVersion: gvk.GroupVersion(), + NegotiatedSerializer: scheme.Codecs.WithoutConversion(), + Client: fake.CreateHTTPClient(func(_ *http.Request) (*http.Response, error) { + header := http.Header{} + header.Set("Content-Type", runtime.ContentTypeJSON) + code, body := http.StatusNotFound, "" + if c.createdObjects[id] { + code, body = http.StatusOK, ownedBody + } + return &http.Response{StatusCode: code, Header: header, Body: stringBody(body)}, nil + }), + } + } +} + +func (c *recordingKubeClient) Create(resources kube.ResourceList, _ ...kube.ClientCreateOption) (*kube.Result, error) { + if c.onCreate != nil { + c.onCreate() + } + ids := resourceIDs(resources) + c.createCalls = append(c.createCalls, ids) + if c.createdObjects != nil { + for _, id := range ids { + c.createdObjects[id] = true + } + } + c.operations = append(c.operations, "create:"+strings.Join(ids, ",")) + return &kube.Result{Created: resources}, nil +} + +func (c *recordingKubeClient) Delete(resources kube.ResourceList, deletionPropagation metav1.DeletionPropagation) (*kube.Result, []error) { + ids := resourceIDs(resources) + c.deleteCalls = append(c.deleteCalls, ids) + c.operations = append(c.operations, "delete:"+strings.Join(ids, ",")) + return c.PrintingKubeClient.Delete(resources, deletionPropagation) +} + +func (c *recordingKubeClient) Update(current, target kube.ResourceList, _ ...kube.ClientUpdateOption) (*kube.Result, error) { + if c.onUpdate != nil { + c.onUpdate() + } + currentIDs := resourceIDs(current) + targetIDs := resourceIDs(target) + + currentSet := make(map[string]struct{}, len(currentIDs)) + for _, id := range currentIDs { + currentSet[id] = struct{}{} + } + + var created kube.ResourceList + var createdIDs []string + for _, info := range target { + id := fmt.Sprintf("%s/%s", info.Object.GetObjectKind().GroupVersionKind().Kind, info.Name) + if _, exists := currentSet[id]; !exists { + created = append(created, info) + createdIDs = append(createdIDs, id) + } + } + + c.updateCalls = append(c.updateCalls, updateCall{ + current: currentIDs, + target: targetIDs, + created: createdIDs, + currentResources: current, + targetResources: target, + }) + c.operations = append(c.operations, "update:"+strings.Join(targetIDs, ",")) + return &kube.Result{Updated: target, Created: created}, nil +} + +func (c *recordingKubeClient) Build(reader io.Reader, _ bool) (kube.ResourceList, error) { + if c.onBuild != nil { + c.onBuild() + } + decoder := yamlutil.NewYAMLOrJSONDecoder(reader, 4096) + var resources kube.ResourceList + + for { + var obj map[string]any + if err := decoder.Decode(&obj); err != nil { + if errors.Is(err, io.EOF) { + break + } + return nil, err + } + if len(obj) == 0 { + continue + } + + u := &unstructured.Unstructured{Object: obj} + gvk := u.GroupVersionKind() + namespace := u.GetNamespace() + if namespace == "" { + namespace = "spaced" + u.SetNamespace(namespace) + } + info := &resource.Info{ + Name: u.GetName(), + Namespace: namespace, + Object: u, + Mapping: &meta.RESTMapping{ + Resource: schema.GroupVersionResource{Group: gvk.Group, Version: gvk.Version, Resource: strings.ToLower(gvk.Kind) + "s"}, + GroupVersionKind: gvk, + Scope: meta.RESTScopeNamespace, + }, + } + if c.buildRESTClient != nil { + info.Client = c.buildRESTClient(u) + } + resources.Append(info) + } + + return resources, nil +} + +func (c *recordingKubeClient) GetWaiter(ws kube.WaitStrategy) (kube.Waiter, error) { + return c.GetWaiterWithOptions(ws) +} + +func (c *recordingKubeClient) GetWaiterWithOptions(ws kube.WaitStrategy, _ ...kube.WaitOption) (kube.Waiter, error) { + c.waiterStrategies = append(c.waiterStrategies, ws) + if ws == kube.HookOnlyStrategy { + // Mirror the real hookOnlyWaiter: general waits are no-ops, only + // hook watches are real (pkg/kube/statuswait.go:297-315). + return &recordingHookOnlyWaiter{client: c}, nil + } + return &recordingKubeWaiter{client: c}, nil +} + +type recordingHookOnlyWaiter struct { + client *recordingKubeClient +} + +func (w *recordingHookOnlyWaiter) Wait(_ kube.ResourceList, _ time.Duration) error { + return nil +} + +func (w *recordingHookOnlyWaiter) WaitWithJobs(_ kube.ResourceList, _ time.Duration) error { + return nil +} + +func (w *recordingHookOnlyWaiter) WaitForDelete(_ kube.ResourceList, _ time.Duration) error { + return nil +} + +func (w *recordingHookOnlyWaiter) WatchUntilReady(resources kube.ResourceList, d time.Duration) error { + return (&recordingKubeWaiter{client: w.client}).WatchUntilReady(resources, d) +} + +func (w *recordingKubeWaiter) Wait(resources kube.ResourceList, _ time.Duration) error { + return w.client.recordWait(resources) +} + +func (w *recordingKubeWaiter) WaitWithJobs(resources kube.ResourceList, _ time.Duration) error { + return w.client.recordWait(resources) +} + +func (w *recordingKubeWaiter) WaitForDelete(resources kube.ResourceList, _ time.Duration) error { + ids := resourceIDs(resources) + w.client.deleteWaitCalls = append(w.client.deleteWaitCalls, ids) + w.client.operations = append(w.client.operations, "wait-delete:"+strings.Join(ids, ",")) + return nil +} + +func (w *recordingKubeWaiter) WatchUntilReady(resources kube.ResourceList, _ time.Duration) error { + ids := resourceIDs(resources) + w.client.watchUntilReadyCalls = append(w.client.watchUntilReadyCalls, ids) + w.client.operations = append(w.client.operations, "watch:"+strings.Join(ids, ",")) + return nil +} + +func (c *recordingKubeClient) recordWait(resources kube.ResourceList) error { + c.waitCallCount++ + ids := resourceIDs(resources) + c.waitCalls = append(c.waitCalls, ids) + c.operations = append(c.operations, "wait:"+strings.Join(ids, ",")) + if c.waitError != nil && c.waitCallCount == c.waitErrorOnCall { + return c.waitError + } + return nil +} + +func resourceIDs(resources kube.ResourceList) []string { + ids := make([]string, 0, len(resources)) + for _, info := range resources { + kind := info.Object.GetObjectKind().GroupVersionKind().Kind + if kind == "" { + kind = "Unknown" + } + ids = append(ids, fmt.Sprintf("%s/%s", kind, info.Name)) + } + return ids +} + +func newSequencedInstallAction(t *testing.T, kubeClient kube.Interface) *Install { + t.Helper() + cfg := actionConfigFixture(t) + cfg.KubeClient = kubeClient + + install := NewInstall(cfg) + install.Namespace = "spaced" + install.ReleaseName = "test-install-release" + install.Timeout = 5 * time.Minute + install.ReadinessTimeout = time.Minute + install.WaitStrategy = kube.OrderedWaitStrategy + return install +} + +func releaseFile(name, content string) *common.File { + return &common.File{Name: name, ModTime: time.Now(), Data: []byte(content)} +} + +func configMapManifest(name string, annotations map[string]string) string { + var b strings.Builder + b.WriteString("apiVersion: v1\nkind: ConfigMap\nmetadata:\n") + _, _ = fmt.Fprintf(&b, " name: %s\n", name) + if len(annotations) > 0 { + b.WriteString(" annotations:\n") + for key, value := range annotations { + _, _ = fmt.Fprintf(&b, " %s: %q\n", key, value) + } + } + b.WriteString("data:\n key: value\n") + return b.String() +} + +func makeConfigMapTemplate(fileName, name string, annotations map[string]string) *common.File { + return releaseFile(fileName, configMapManifest(name, annotations)) +} + +func makeHookTemplate(fileName, name, hook string, extraAnnotations map[string]string) *common.File { + annotations := map[string]string{release.HookAnnotation: hook} + maps.Copy(annotations, extraAnnotations) + return makeConfigMapTemplate(fileName, name, annotations) +} + +func mustRelease(t *testing.T, rel ri.Releaser) *release.Release { + t.Helper() + out, err := releaserToV1Release(rel) + require.NoError(t, err) + 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), + makeTestManifest("two", "chart/templates/two.yaml", nil), + }) + + assert.Contains(t, yaml, "name: one") + assert.Contains(t, yaml, "name: two") +} + +func TestInstall_Sequenced_BasicResourceGroups(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }) + + rel := mustRelease(t, mustRunInstall(t, install, ch)) + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/app"}}, client.createCalls) + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/app"}}, client.waitCalls) + assert.Equal(t, rcommon.StatusDeployed, rel.Info.Status) + assert.True(t, rel.Sequenced) + assert.True(t, rel.IsSequenced()) + assert.Nil(t, rel.SequencingInfo) +} + +func TestInstall_Sequenced_SubchartOrdering(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + parent := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/parent.yaml", "parent", nil), + }, withName("parent")) + database := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + }, withName("database")) + api := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/api.yaml", "api", nil), + }, withName("api")) + + parent.AddDependency(database, api) + parent.Metadata.Dependencies = []*chart.Dependency{ + {Name: "database"}, + {Name: "api", DependsOn: []string{"database"}}, + } + + mustRunInstall(t, install, parent) + + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/api"}, {"ConfigMap/parent"}}, client.createCalls) + assert.Equal(t, client.createCalls, client.waitCalls) +} + +// TestInstall_Sequenced_UndeclaredVendoredSubchartDeployed is a regression for +// hip-0025: a subchart vendored into charts/ but absent from Chart.yaml +// dependencies is rendered, so under --wait=ordered its manifests must still be +// applied. Previously they were grouped by subchart yet never matched a DAG +// batch (the DAG is built from Chart.yaml dependencies only), so they were +// silently dropped. Undeclared subcharts are unsequenced: they deploy after the +// declared subchart batches and before the parent's own resources. +func TestInstall_Sequenced_UndeclaredVendoredSubchartDeployed(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + parent := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/parent.yaml", "parent", nil), + }, withName("parent")) + declared := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + }, withName("database")) + vendored := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/vendored.yaml", "vendored", nil), + }, withName("vendored")) + + parent.AddDependency(declared, vendored) + // Only "database" is declared in Chart.yaml dependencies; "vendored" sits in + // charts/ but is undeclared. + parent.Metadata.Dependencies = []*chart.Dependency{ + {Name: "database"}, + } + + mustRunInstall(t, install, parent) + + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/vendored"}, {"ConfigMap/parent"}}, client.createCalls) +} + +// TestInstall_Sequenced_LeafGroupsWaited locks D1: ordered is a --wait +// flavor, so terminal groups are gated too. This makes readiness-failure +// detection and --atomic work for them. +func TestInstall_Sequenced_LeafGroupsWaited(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/base.yaml", "base", map[string]string{ + releaseutil.AnnotationResourceGroup: "base", + }), + makeConfigMapTemplate("templates/leaf.yaml", "leaf", map[string]string{ + releaseutil.AnnotationResourceGroup: "leaf", + releaseutil.AnnotationDependsOnResourceGroups: `["base"]`, + kube.AnnotationReadinessSuccess: `["{.never} == match"]`, + kube.AnnotationReadinessFailure: `["{.never} == fail"]`, + }), + }) + + mustRunInstall(t, install, ch) + assert.Equal(t, [][]string{{"ConfigMap/base"}, {"ConfigMap/leaf"}}, client.createCalls) + assert.Equal(t, [][]string{{"ConfigMap/base"}, {"ConfigMap/leaf"}}, client.waitCalls) +} + +func TestInstall_Sequenced_TerminalGroupFailureFailsInstall(t *testing.T) { + client := newRecordingKubeClient() + client.waitErrorOnCall = 2 + client.waitError = assert.AnError + install := newSequencedInstallAction(t, client) + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/base.yaml", "base", map[string]string{ + releaseutil.AnnotationResourceGroup: "base", + }), + makeConfigMapTemplate("templates/leaf.yaml", "leaf", map[string]string{ + releaseutil.AnnotationResourceGroup: "leaf", + releaseutil.AnnotationDependsOnResourceGroups: `["base"]`, + }), + }) + + _, err := install.RunWithContext(context.Background(), ch, map[string]any{}) + require.Error(t, err, "a terminal group's readiness failure must fail the install") + assert.Equal(t, [][]string{{"ConfigMap/base"}, {"ConfigMap/leaf"}}, client.createCalls) + last, getErr := install.cfg.Releases.Last(install.ReleaseName) + require.NoError(t, getErr) + assert.Equal(t, rcommon.StatusFailed, mustRelease(t, last).Info.Status) +} + +// TestInstall_Sequenced_NonLeafReadinessStillWaitsOnFailure is the regression +// guard for cap-04/cap-05/cap-32: when a resource HAS a dependent, its +// readiness IS waited on. Simulates the worker.Wait returning an error +// (e.g. readiness-failure JSONPath fired) and confirms the install aborts. +// Without this guard, a future "ignore-readiness" change could silently +// regress all custom-readiness behavior. +func TestInstall_Sequenced_NonLeafReadinessStillWaitsOnFailure(t *testing.T) { + client := newRecordingKubeClient() + client.waitErrorOnCall = 1 + client.waitError = assert.AnError + install := newSequencedInstallAction(t, client) + + ch := buildChartWithTemplates([]*common.File{ + // bootstrap has a dependent (app), so its readiness must be waited on. + makeConfigMapTemplate("templates/bootstrap.yaml", "bootstrap", map[string]string{ + releaseutil.AnnotationResourceGroup: "bootstrap", + kube.AnnotationReadinessSuccess: `["{.ready} == true"]`, + kube.AnnotationReadinessFailure: `["{.failed} == true"]`, + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["bootstrap"]`, + }), + }) + + _, err := install.RunWithContext(context.Background(), ch, map[string]any{}) + require.Error(t, err, "bootstrap readiness wait failure must abort install") + require.NotEmpty(t, client.waitCalls, "bootstrap wait call must have been recorded") + assert.Equal(t, []string{"ConfigMap/bootstrap"}, client.waitCalls[0]) + // app group's batch never gets applied because bootstrap wait failed first. + require.NotContains(t, client.operations, "create:ConfigMap/app", "app should not be created after bootstrap failed") +} + +func TestInstall_Sequenced_Combined(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + parent := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/parent.yaml", "parent", nil), + }, withName("parent")) + database := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/setup.yaml", "db-setup", map[string]string{ + releaseutil.AnnotationResourceGroup: "setup", + }), + makeConfigMapTemplate("templates/data.yaml", "db-data", map[string]string{ + releaseutil.AnnotationResourceGroup: "data", + releaseutil.AnnotationDependsOnResourceGroups: `["setup"]`, + }), + }, withName("database")) + api := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/api.yaml", "api", map[string]string{ + releaseutil.AnnotationResourceGroup: "api", + }), + }, withName("api")) + + parent.AddDependency(database, api) + parent.Metadata.Dependencies = []*chart.Dependency{ + {Name: "database"}, + {Name: "api", DependsOn: []string{"database"}}, + } + + mustRunInstall(t, install, parent) + + assert.Equal(t, [][]string{ + {"ConfigMap/db-setup"}, + {"ConfigMap/db-data"}, + {"ConfigMap/api"}, + {"ConfigMap/parent"}, + }, client.createCalls) +} + +func TestInstall_Sequenced_NoAnnotations(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/first.yaml", "first", nil), + makeConfigMapTemplate("templates/second.yaml", "second", nil), + }) + + rel := mustRelease(t, mustRunInstall(t, install, ch)) + + require.Len(t, client.createCalls, 1) + assert.ElementsMatch(t, []string{"ConfigMap/first", "ConfigMap/second"}, client.createCalls[0]) + assert.Equal(t, rcommon.StatusDeployed, rel.Info.Status) +} + +func TestInstall_Sequenced_UnsequencedLast(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/orphan.yaml", "orphan", map[string]string{ + releaseutil.AnnotationResourceGroup: "orphan", + releaseutil.AnnotationDependsOnResourceGroups: `["missing"]`, + }), + makeConfigMapTemplate("templates/plain.yaml", "plain", nil), + }) + + mustRunInstall(t, install, ch) + + require.Len(t, client.createCalls, 2) + assert.Equal(t, []string{"ConfigMap/database"}, client.createCalls[0]) + assert.ElementsMatch(t, []string{"ConfigMap/orphan", "ConfigMap/plain"}, client.createCalls[1]) +} + +func TestInstall_Sequenced_ReadinessTimeout(t *testing.T) { + client := newRecordingKubeClient() + client.waitErrorOnCall = 1 + client.waitError = errors.New("timed out waiting for batch") + + install := newSequencedInstallAction(t, client) + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }) + + rel, err := install.Run(ch, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "timed out waiting for batch") + assert.Len(t, client.createCalls, 1) + + last, getErr := install.cfg.Releases.Last(install.ReleaseName) + require.NoError(t, getErr) + assert.Equal(t, rcommon.StatusFailed, mustRelease(t, last).Info.Status) + assert.Equal(t, rcommon.StatusFailed, mustRelease(t, rel).Info.Status) +} + +func TestInstall_Sequenced_AtomicRollback(t *testing.T) { + client := newRecordingKubeClient() + // Install-then-rollback needs cluster-like GET behavior: 404 before the + // resources are created (conflict check), owned afterwards (rollback + // deletion ownership verification). + client.buildRESTClient = statefulOwnedRESTClient(client, "test-install-release", "spaced") + client.waitErrorOnCall = 1 + client.waitError = errors.New("timed out waiting for batch") + + install := newSequencedInstallAction(t, client) + install.RollbackOnFailure = true + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }) + + _, err := install.Run(ch, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "rollback-on-failure") + assert.NotEmpty(t, client.deleteCalls) +} + +// TestInstall_Sequenced_FailureCleanupIsSequenced locks beads ll8/0vr: the +// Sequenced flag must reach the STORED release record before deployment +// starts, so the rollback-on-failure uninstall, which re-reads the release +// from storage, tears the partial install down in reverse batch order with a +// delete-wait per batch, not as one unordered bulk delete. +func TestInstall_Sequenced_FailureCleanupIsSequenced(t *testing.T) { + client := newRecordingKubeClient() + client.buildRESTClient = statefulOwnedRESTClient(client, "test-install-release", "spaced") + client.waitErrorOnCall = 2 + client.waitError = errors.New("batch two never became ready") + + install := newSequencedInstallAction(t, client) + install.RollbackOnFailure = true + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/a.yaml", "a", map[string]string{ + releaseutil.AnnotationResourceGroup: "a", + }), + makeConfigMapTemplate("templates/b.yaml", "b", map[string]string{ + releaseutil.AnnotationResourceGroup: "b", + releaseutil.AnnotationDependsOnResourceGroups: `["a"]`, + }), + makeConfigMapTemplate("templates/c.yaml", "c", map[string]string{ + releaseutil.AnnotationResourceGroup: "c", + releaseutil.AnnotationDependsOnResourceGroups: `["b"]`, + }), + }) + + rel, err := install.Run(ch, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "rollback-on-failure") + assert.True(t, mustRelease(t, rel).Sequenced, "failed release must persist sequencing intent via the Sequenced flag") + + // a and b were created (c's batch never started; its manifests read as + // already-deleted). Cleanup must delete in reverse group order, waiting + // for each batch's deletion before the next. + assert.Equal(t, [][]string{{"ConfigMap/b"}, {"ConfigMap/a"}}, client.deleteCalls) + assert.Equal(t, [][]string{{"ConfigMap/b"}, {"ConfigMap/a"}}, client.deleteWaitCalls) +} + +// TestInstall_Sequenced_TakeOwnershipAdoptsExisting locks bead 9ui (install): +// with --take-ownership, a resource that already exists in the cluster must be +// adopted via Update inside its plan batch, not blindly Created. +func TestInstall_Sequenced_TakeOwnershipAdoptsExisting(t *testing.T) { + client := newRecordingKubeClient() + client.buildRESTClient = statefulOwnedRESTClient(client, "test-install-release", "spaced") + // database pre-exists in the cluster before the install begins. + client.createdObjects["ConfigMap/database"] = true + + install := newSequencedInstallAction(t, client) + install.TakeOwnership = true + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }) + + mustRunInstall(t, install, ch) + + // database's batch adopts (Update with the pre-existing object as current); + // app's batch has nothing to adopt and is Created. + require.Len(t, client.updateCalls, 1) + assert.Equal(t, []string{"ConfigMap/database"}, client.updateCalls[0].current) + assert.Equal(t, []string{"ConfigMap/database"}, client.updateCalls[0].target) + assert.Equal(t, [][]string{{"ConfigMap/app"}}, client.createCalls) +} + +// TestInstall_Sequenced_SameNameDifferentKindWaited locks beads xnf/p1m: +// readiness gating must never key on bare metadata.name. A ConfigMap and a +// Secret share the name "shared"; previously the leaf-suppression map keyed by +// name suppressed the wait for BOTH, including the depended-on ConfigMap. +func TestInstall_Sequenced_SameNameDifferentKindWaited(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + secretManifest := "apiVersion: v1\nkind: Secret\nmetadata:\n name: shared\n annotations:\n" + + " " + releaseutil.AnnotationResourceGroup + ": \"app\"\n" + + " " + releaseutil.AnnotationDependsOnResourceGroups + ": '[\"base\"]'\n" + + "stringData:\n key: value\n" + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/base.yaml", "shared", map[string]string{ + releaseutil.AnnotationResourceGroup: "base", + }), + releaseFile("templates/app.yaml", secretManifest), + }) + + mustRunInstall(t, install, ch) + + assert.Equal(t, [][]string{{"ConfigMap/shared"}, {"Secret/shared"}}, client.createCalls) + assert.Equal(t, [][]string{{"ConfigMap/shared"}, {"Secret/shared"}}, client.waitCalls) +} + +func TestInstall_Sequenced_DryRun(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + install.DryRunStrategy = DryRunServer + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }) + + rel := mustRelease(t, mustRunInstall(t, install, ch)) + assert.Empty(t, client.createCalls) + assert.Empty(t, client.waitCalls) + assert.Equal(t, "Dry run complete", rel.Info.Description) +} + +func TestInstall_Sequenced_CyclicDependency(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/a.yaml", "a", map[string]string{ + releaseutil.AnnotationResourceGroup: "a", + releaseutil.AnnotationDependsOnResourceGroups: `["b"]`, + }), + makeConfigMapTemplate("templates/b.yaml", "b", map[string]string{ + releaseutil.AnnotationResourceGroup: "b", + releaseutil.AnnotationDependsOnResourceGroups: `["a"]`, + }), + }) + + _, err := install.Run(ch, nil) + require.Error(t, err) + assert.True(t, strings.Contains(err.Error(), "cycle") || strings.Contains(err.Error(), "circular")) + assert.Empty(t, client.createCalls) + _, lastErr := install.cfg.Releases.Last(install.ReleaseName) + assert.Error(t, lastErr, "cycle must be detected before any release record is stored") +} + +func TestInstall_Sequenced_SequencedFlagStored(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/config.yaml", "config", nil), + }) + + rel := mustRelease(t, mustRunInstall(t, install, ch)) + + assert.True(t, rel.Sequenced) + assert.Nil(t, rel.SequencingInfo) + stored, err := install.cfg.Releases.Get(rel.Name, rel.Version) + require.NoError(t, err) + storedRel := mustRelease(t, stored) + assert.True(t, storedRel.Sequenced) + assert.True(t, storedRel.IsSequenced()) + assert.Nil(t, storedRel.SequencingInfo) +} + +func TestInstall_NonSequenced_Unchanged(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + install.WaitStrategy = kube.StatusWatcherStrategy + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }) + + rel := mustRelease(t, mustRunInstall(t, install, ch)) + + require.Len(t, client.createCalls, 1) + assert.ElementsMatch(t, []string{"ConfigMap/database", "ConfigMap/app"}, client.createCalls[0]) + assert.Nil(t, rel.SequencingInfo) +} + +func TestInstall_Sequenced_HookResourcesExcludedFromDAG(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + ch := buildChartWithTemplates([]*common.File{ + makeHookTemplate("templates/pre-hook.yaml", "pre-hook", release.HookPreInstall.String(), map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + }) + + mustRunInstall(t, install, ch) + + assert.Equal(t, [][]string{{"ConfigMap/pre-hook"}, {"ConfigMap/database"}}, client.createCalls) + assert.Equal(t, [][]string{{"ConfigMap/pre-hook"}}, client.watchUntilReadyCalls) +} + +func TestInstall_Sequenced_HooksAtStandardPositions(t *testing.T) { + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + ch := buildChartWithTemplates([]*common.File{ + makeHookTemplate("templates/pre-hook.yaml", "pre-hook", release.HookPreInstall.String(), map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeHookTemplate("templates/post-hook.yaml", "post-hook", release.HookPostInstall.String(), map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + }) + + mustRunInstall(t, install, ch) + + assert.Equal(t, [][]string{{"ConfigMap/pre-hook"}, {"ConfigMap/database"}, {"ConfigMap/post-hook"}}, client.createCalls) + assert.Equal(t, [][]string{{"ConfigMap/pre-hook"}, {"ConfigMap/post-hook"}}, client.watchUntilReadyCalls) +} + +func singleManifestBatch(m releaseutil.Manifest) sequence.Batch { + return sequence.Batch{ + Groups: []sequence.Group{{Manifests: []releaseutil.Manifest{m}}}, + Wait: true, + } +} + +func TestSequencedDeployment_ApplyBatch_Create_RespectsContextCancellation(t *testing.T) { + newSequencedDeploymentForTest := func(client kube.Interface) *sequencedDeployment { + t.Helper() + + cfg := actionConfigFixture(t) + cfg.KubeClient = client + + return &sequencedDeployment{ + cfg: cfg, + releaseName: "demo", + releaseNamespace: "spaced", + waitStrategy: kube.OrderedWaitStrategy, + readinessTimeout: time.Minute, + } + } + + t.Run("context canceled before build", func(t *testing.T) { + client := newRecordingKubeClient() + buildCalled := false + client.onBuild = func() { buildCalled = true } + + sd := newSequencedDeploymentForTest(client) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := sd.applyBatch(ctx, singleManifestBatch(makeTestManifest("cm", "chart/templates/cm.yaml", nil))) + require.ErrorIs(t, err, context.Canceled) + assert.False(t, buildCalled) + assert.Empty(t, client.createCalls) + assert.Empty(t, client.waitCalls) + }) + + t.Run("context canceled after build before create", func(t *testing.T) { + client := newRecordingKubeClient() + ctx, cancel := context.WithCancel(context.Background()) + client.onBuild = cancel + + sd := newSequencedDeploymentForTest(client) + + err := sd.applyBatch(ctx, singleManifestBatch(makeTestManifest("cm", "chart/templates/cm.yaml", nil))) + require.ErrorIs(t, err, context.Canceled) + assert.Empty(t, client.createCalls) + assert.Empty(t, client.waitCalls) + }) + + t.Run("context canceled after create before wait", func(t *testing.T) { + client := newRecordingKubeClient() + ctx, cancel := context.WithCancel(context.Background()) + client.onCreate = cancel + + sd := newSequencedDeploymentForTest(client) + + err := sd.applyBatch(ctx, singleManifestBatch(makeTestManifest("cm", "chart/templates/cm.yaml", nil))) + require.ErrorIs(t, err, context.Canceled) + require.Len(t, client.createCalls, 1) + assert.Empty(t, client.waitCalls) + }) +} + +func TestSequencedDeployment_ApplyBatch_Update_RespectsContextCancellation(t *testing.T) { + newSequencedDeploymentForTest := func(client kube.Interface) *sequencedDeployment { + t.Helper() + + cfg := actionConfigFixture(t) + cfg.KubeClient = client + + return &sequencedDeployment{ + cfg: cfg, + releaseName: "demo", + releaseNamespace: "spaced", + waitStrategy: kube.OrderedWaitStrategy, + readinessTimeout: time.Minute, + upgradeMode: true, + } + } + + t.Run("context canceled before build", func(t *testing.T) { + client := newRecordingKubeClient() + buildCalled := false + client.onBuild = func() { buildCalled = true } + + sd := newSequencedDeploymentForTest(client) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := sd.applyBatch(ctx, singleManifestBatch(makeTestManifest("cm", "chart/templates/cm.yaml", nil))) + require.ErrorIs(t, err, context.Canceled) + assert.False(t, buildCalled) + assert.Empty(t, client.updateCalls) + assert.Empty(t, client.createCalls) + assert.Empty(t, client.waitCalls) + }) + + t.Run("context canceled after build before update", func(t *testing.T) { + client := newRecordingKubeClient() + ctx, cancel := context.WithCancel(context.Background()) + client.onBuild = cancel + + sd := newSequencedDeploymentForTest(client) + + err := sd.applyBatch(ctx, singleManifestBatch(makeTestManifest("cm", "chart/templates/cm.yaml", nil))) + require.ErrorIs(t, err, context.Canceled) + assert.Empty(t, client.updateCalls) + assert.Empty(t, client.createCalls) + assert.Empty(t, client.waitCalls) + }) + + t.Run("context canceled after update before wait", func(t *testing.T) { + client := newRecordingKubeClient() + ctx, cancel := context.WithCancel(context.Background()) + client.onUpdate = cancel + + sd := newSequencedDeploymentForTest(client) + + err := sd.applyBatch(ctx, singleManifestBatch(makeTestManifest("cm", "chart/templates/cm.yaml", nil))) + require.ErrorIs(t, err, context.Canceled) + require.Len(t, client.updateCalls, 1) + assert.Empty(t, client.createCalls) + assert.Empty(t, client.waitCalls) + }) +} + +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, + // so Name + Object are sufficient — no Mapping/Client required. + makeInfo := func(name string, annotations map[string]string) *resource.Info { + u := &unstructured.Unstructured{} + u.SetAPIVersion("v1") + u.SetKind("ConfigMap") + u.SetName(name) + if annotations != nil { + u.SetAnnotations(annotations) + } + return &resource.Info{Name: name, Object: u} + } + + annotationsOf := func(info *resource.Info) map[string]string { + acc, err := meta.Accessor(info.Object) + require.NoError(t, err) + return acc.GetAnnotations() + } + + t.Run("strips depends-on annotation, preserves siblings", func(t *testing.T) { + info := makeInfo("cm", map[string]string{ + releaseutil.AnnotationDependsOnResourceGroups: `["foo"]`, + releaseutil.AnnotationResourceGroup: "app", + "other.example.com/keep": "yes", + }) + resources := kube.ResourceList{info} + + require.NoError(t, stripSequencingAnnotations(resources)) + + ann := annotationsOf(info) + assert.NotContains(t, ann, releaseutil.AnnotationDependsOnResourceGroups, + "helm-internal depends-on annotation must be removed") + assert.Equal(t, "app", ann[releaseutil.AnnotationResourceGroup], + "single-slash helm.sh/resource-group is a valid k8s key and must be preserved") + assert.Equal(t, "yes", ann["other.example.com/keep"], + "unrelated annotations must be preserved") + }) + + t.Run("no-op when resource has no annotations", func(t *testing.T) { + info := makeInfo("cm", nil) + resources := kube.ResourceList{info} + + require.NoError(t, stripSequencingAnnotations(resources)) + assert.Empty(t, annotationsOf(info)) + }) + + t.Run("no-op when resource has only non-sequencing annotations", func(t *testing.T) { + info := makeInfo("cm", map[string]string{ + "keep.example.com/one": "1", + "keep.example.com/two": "2", + }) + resources := kube.ResourceList{info} + + require.NoError(t, stripSequencingAnnotations(resources)) + + ann := annotationsOf(info) + assert.Equal(t, "1", ann["keep.example.com/one"]) + assert.Equal(t, "2", ann["keep.example.com/two"]) + }) + + t.Run("strips across a multi-resource batch", func(t *testing.T) { + a := makeInfo("a", map[string]string{ + releaseutil.AnnotationDependsOnResourceGroups: `["x"]`, + releaseutil.AnnotationResourceGroup: "g1", + }) + b := makeInfo("b", map[string]string{ + releaseutil.AnnotationDependsOnResourceGroups: `["y"]`, + releaseutil.AnnotationResourceGroup: "g2", + }) + c := makeInfo("c", map[string]string{ + "unrelated": "value", + }) + resources := kube.ResourceList{a, b, c} + + require.NoError(t, stripSequencingAnnotations(resources)) + + for _, info := range []*resource.Info{a, b} { + ann := annotationsOf(info) + assert.NotContains(t, ann, releaseutil.AnnotationDependsOnResourceGroups, + "resource %s should have depends-on stripped", info.Name) + assert.Contains(t, ann, releaseutil.AnnotationResourceGroup, + "resource %s should retain resource-group", info.Name) + } + assert.Equal(t, "value", annotationsOf(c)["unrelated"], + "resources without sequencing annotations are untouched") + }) + + t.Run("strips every key listed in HelmInternalSequencingAnnotations", func(t *testing.T) { + // Lock the invariant: the function must remove EVERY key in the + // HelmInternalSequencingAnnotations list, not just depends-on. + ann := map[string]string{"keep": "yes"} + for _, k := range releaseutil.HelmInternalSequencingAnnotations() { + ann[k] = "internal" + } + info := makeInfo("cm", ann) + + require.NoError(t, stripSequencingAnnotations(kube.ResourceList{info})) + + got := annotationsOf(info) + for _, k := range releaseutil.HelmInternalSequencingAnnotations() { + assert.NotContains(t, got, k, "internal key %q must be stripped", k) + } + assert.Equal(t, "yes", got["keep"]) + }) +} + +func makeTestManifest(name, sourcePath string, annotations map[string]string) releaseutil.Manifest { + content := configMapManifest(name, annotations) + head := &releaseutil.SimpleHead{ + Version: v1.SchemeGroupVersion.String(), + Kind: "ConfigMap", + } + head.Metadata = &struct { + Name string `json:"name"` + Namespace string `json:"namespace,omitempty"` + Annotations map[string]string `json:"annotations"` + }{ + Name: name, + Annotations: annotations, + } + return releaseutil.Manifest{Name: sourcePath, Content: content, Head: head} +} + +func mustRunInstall(t *testing.T, install *Install, ch *chart.Chart) ri.Releaser { + t.Helper() + rel, err := install.RunWithContext(context.Background(), ch, map[string]any{}) + require.NoError(t, err) + return rel +} diff --git a/pkg/action/uninstall.go b/pkg/action/uninstall.go index b14305112..6a0827b2c 100644 --- a/pkg/action/uninstall.go +++ b/pkg/action/uninstall.go @@ -28,8 +28,9 @@ import ( chartutil "helm.sh/helm/v4/pkg/chart/v2/util" "helm.sh/helm/v4/pkg/kube" releasei "helm.sh/helm/v4/pkg/release" - "helm.sh/helm/v4/pkg/release/common" + releasecommon "helm.sh/helm/v4/pkg/release/common" 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" "helm.sh/helm/v4/pkg/storage/driver" ) @@ -183,7 +184,7 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) // TODO: Are there any cases where we want to force a delete even if it's // already marked deleted? - if rel.Info.Status == common.StatusUninstalled { + if rel.Info.Status == releasecommon.StatusUninstalled { if !u.KeepHistory { if err := u.purgeReleases(rels...); err != nil { return nil, fmt.Errorf("uninstall: Failed to purge the release: %w", err) @@ -193,8 +194,17 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) return nil, fmt.Errorf("the release named %q is already deleted", name) } + gatingWaiter := waiter + if rel.IsSequenced() && u.WaitStrategy == kube.HookOnlyStrategy { + u.cfg.Logger().Info("release was installed with ordered sequencing; using the status watcher to gate deletion batches (hooks keep the hook-only strategy)", "release", rel.Name) + gatingWaiter, err = getWaiterFor(u.cfg.KubeClient, kube.StatusWatcherStrategy, u.WaitOptions...) + if err != nil { + return nil, err + } + } + u.cfg.Logger().Debug("uninstall: deleting release", "name", name) - rel.Info.Status = common.StatusUninstalling + rel.Info.Status = releasecommon.StatusUninstalling rel.Info.Deleted = time.Now() rel.Info.Description = "Deletion in progress (or silently failed)" res := &releasei.UninstallReleaseResponse{Release: rel} @@ -214,7 +224,7 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) u.cfg.Logger().Debug("uninstall: Failed to store updated release", slog.Any("error", err)) } - deletedResources, kept, errs := u.deleteRelease(rel) + deletedResources, kept, errs := u.deleteRelease(rel, gatingWaiter) if errs != nil { u.cfg.Logger().Debug("uninstall: Failed to delete release", slog.Any("error", errs)) return nil, fmt.Errorf("failed to delete release: %s", name) @@ -222,8 +232,10 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) res.Info = kept - if err := waiter.WaitForDelete(deletedResources, u.Timeout); err != nil { - errs = append(errs, err) + if !rel.IsSequenced() { + if err := waiter.WaitForDelete(deletedResources, u.Timeout); err != nil { + errs = append(errs, err) + } } if !u.DisableHooks { @@ -233,7 +245,7 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) } } - rel.Info.Status = common.StatusUninstalled + rel.Info.Status = releasecommon.StatusUninstalled if len(u.Description) > 0 { rel.Info.Description = u.Description } else { @@ -272,7 +284,7 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) } u.cfg.Logger().Debug("superseding previous deployment", "version", rel.Version) - rel.Info.Status = common.StatusSuperseded + rel.Info.Status = releasecommon.StatusSuperseded if err := u.cfg.Releases.Update(rel); err != nil { u.cfg.Logger().Debug("uninstall: Failed to store updated release", slog.Any("error", err)) } @@ -317,8 +329,12 @@ func (e *joinedErrors) Unwrap() []error { return e.errs } -// 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) (kube.ResourceList, string, []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) + } + var errs []error manifests := releaseutil.SplitManifests(rel.Manifest) @@ -340,75 +356,180 @@ func (u *Uninstall) deleteRelease(rel *release.Release) (kube.ResourceList, stri } } + resources, err := u.buildDeleteResources(filesToDelete) + if err != nil { + return nil, "", []error{err} + } + + ownedResources, skipped, err := u.verifyOwnedForDelete(resources, rel.Name, rel.Namespace) + if err != nil { + return nil, "", []error{err} + } + if skipped != "" { + if kept.Len() > 0 { + kept.WriteString("\n") + } + kept.WriteString(skipped) + } + + // Delete only owned resources + if len(ownedResources) > 0 { + _, errs = u.cfg.KubeClient.Delete(ownedResources, parseCascadingFlag(u.DeletionPropagation, u.cfg.Logger())) + } + return ownedResources, kept.String(), errs +} + +// deleteReleaseSequenced deletes a sequenced release's resources in exact +// reverse deployment order. The forward plan is rebuilt from the STORED +// manifest via its "# Source:" comments (no chart re-render), so it covers +// every stored resource, including vendored subcharts absent from Chart.yaml +// 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) { + 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)} + } + + filesToKeep, filesToDelete := filterManifestsToKeep(manifests) + var kept strings.Builder + if len(filesToKeep) > 0 { + kept.WriteString("These resources were kept due to the resource policy:\n") + for _, f := range filesToKeep { + fmt.Fprintf(&kept, "[%s] %s\n", f.Head.Kind, f.Head.Metadata.Name) + } + } + + plan, err := sequence.Build(rel.Chart, filesToDelete) + if err != nil { + return nil, kept.String(), []error{fmt.Errorf("building sequencing plan for uninstall: %w", err)} + } + logPlanWarnings(u.cfg.Logger(), plan) + + deadline := computeDeadline(u.Timeout) + var allDeleted kube.ResourceList + for _, batch := range plan.Reverse().Batches { + deleted, errs := u.deleteManifestBatch(batch.Manifests(), waiter, deadline, rel.Name, rel.Namespace) + allDeleted = append(allDeleted, deleted...) + if len(errs) > 0 { + return allDeleted, kept.String(), errs + } + } + return allDeleted, kept.String(), nil +} + +// verifyOwnedForDelete verifies which of the built resources are owned by the +// release. It logs (and thereby skips) resources that are not owned by the +// release or whose ownership cannot be verified, and returns the owned subset +// together with a human-readable summary of what was skipped (empty when nothing +// was skipped). It performs no deletion; callers delete the returned owned +// resources. Both the sequenced and non-sequenced uninstall paths route through +// it so neither deletes resources that do not belong to the release. +func (u *Uninstall) verifyOwnedForDelete(resources kube.ResourceList, releaseName, releaseNamespace string) (kube.ResourceList, string, error) { + if len(resources) == 0 { + return nil, "", nil + } + + ownedResources, unownedResources, unverifiableResources, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + if err != nil { + return nil, "", fmt.Errorf("unable to verify resource ownership: %w", err) + } + + var skipped strings.Builder + + // Log and report resources that are not owned by this release. + if len(unownedResources) > 0 { + for _, info := range unownedResources { + u.cfg.Logger().Warn("skipping delete of resource not owned by this release", + "kind", info.Mapping.GroupVersionKind.Kind, + "name", info.Name, + "namespace", info.Namespace, + "release", releaseName) + } + fmt.Fprintf(&skipped, "%d resource(s) were not deleted because they are not owned by this release:\n", len(unownedResources)) + for _, info := range unownedResources { + fmt.Fprintf(&skipped, "[%s] %s\n", info.Mapping.GroupVersionKind.Kind, info.Name) + } + } + + // Log and report resources whose ownership could not be verified. + if len(unverifiableResources) > 0 { + for _, ur := range unverifiableResources { + u.cfg.Logger().Warn("skipping delete of resource because ownership could not be verified", + "kind", ur.Info.Mapping.GroupVersionKind.Kind, + "name", ur.Info.Name, + "namespace", ur.Info.Namespace, + "release", releaseName, + "error", ur.Err) + } + if skipped.Len() > 0 { + skipped.WriteString("\n") + } + fmt.Fprintf(&skipped, "%d resource(s) were not deleted because their ownership could not be verified:\n", len(unverifiableResources)) + for _, ur := range unverifiableResources { + fmt.Fprintf(&skipped, "[%s] %s: %s\n", ur.Info.Mapping.GroupVersionKind.Kind, ur.Info.Name, ur.Err) + } + } + + for _, info := range ownedResources { + u.cfg.Logger().Debug("deleting resource owned by this release", + "kind", info.Mapping.GroupVersionKind.Kind, + "name", info.Name, + "namespace", info.Namespace, + "release", releaseName) + } + + return ownedResources, skipped.String(), nil +} + +func (u *Uninstall) buildDeleteResources(manifests []releaseutil.Manifest) (kube.ResourceList, error) { var builder strings.Builder - for _, file := range filesToDelete { + for _, file := range manifests { builder.WriteString("\n---\n" + file.Content) } resources, err := u.cfg.KubeClient.Build(strings.NewReader(builder.String()), false) if err != nil { - return nil, "", []error{fmt.Errorf("unable to build kubernetes objects for delete: %w", err)} + return nil, fmt.Errorf("unable to build kubernetes objects for delete: %w", err) } + return resources, nil +} - // Verify ownership before deleting resources - var ownedResources, unownedResources kube.ResourceList - var unverifiableResources []unverifiableResource - if len(resources) > 0 { - ownedResources, unownedResources, unverifiableResources, err = verifyOwnershipBeforeDelete(resources, rel.Name, rel.Namespace) +func (u *Uninstall) deleteManifestBatch(manifests []releaseutil.Manifest, waiter kube.Waiter, deadline time.Time, releaseName, releaseNamespace string) (kube.ResourceList, []error) { + resources, err := u.buildDeleteResources(manifests) + if err != nil || len(resources) == 0 { if err != nil { - return nil, "", []error{fmt.Errorf("unable to verify resource ownership: %w", err)} + return nil, []error{err} } + return nil, nil + } - // Log warnings for unowned resources - if len(unownedResources) > 0 { - for _, info := range unownedResources { - u.cfg.Logger().Warn("skipping delete of resource not owned by this release", - "kind", info.Mapping.GroupVersionKind.Kind, - "name", info.Name, - "namespace", info.Namespace, - "release", rel.Name) - } - if kept.Len() > 0 { - kept.WriteString("\n") - } - fmt.Fprintf(&kept, "%d resource(s) were not deleted because they are not owned by this release:\n", len(unownedResources)) - for _, info := range unownedResources { - fmt.Fprintf(&kept, "[%s] %s\n", info.Mapping.GroupVersionKind.Kind, info.Name) - } - } + // Verify ownership before deleting so a sequenced uninstall, like the + // non-sequenced path, never deletes resources it does not own. + ownedResources, _, err := u.verifyOwnedForDelete(resources, releaseName, releaseNamespace) + if err != nil { + return nil, []error{err} + } + if len(ownedResources) == 0 { + return nil, nil + } - // Log warnings for resources whose ownership could not be verified - if len(unverifiableResources) > 0 { - for _, ur := range unverifiableResources { - u.cfg.Logger().Warn("skipping delete of resource because ownership could not be verified", - "kind", ur.Info.Mapping.GroupVersionKind.Kind, - "name", ur.Info.Name, - "namespace", ur.Info.Namespace, - "release", rel.Name, - "error", ur.Err) - } - if kept.Len() > 0 { - kept.WriteString("\n") - } - fmt.Fprintf(&kept, "%d resource(s) were not deleted because their ownership could not be verified:\n", len(unverifiableResources)) - for _, ur := range unverifiableResources { - fmt.Fprintf(&kept, "[%s] %s: %s\n", ur.Info.Mapping.GroupVersionKind.Kind, ur.Info.Name, ur.Err) - } - } + _, errs := u.cfg.KubeClient.Delete(ownedResources, parseCascadingFlag(u.DeletionPropagation, u.cfg.Logger())) + if len(errs) > 0 { + return ownedResources, errs + } - // Delete only owned resources - if len(ownedResources) > 0 { - for _, info := range ownedResources { - u.cfg.Logger().Debug("deleting resource owned by this release", - "kind", info.Mapping.GroupVersionKind.Kind, - "name", info.Name, - "namespace", info.Namespace, - "release", rel.Name) - } - _, errs = u.cfg.KubeClient.Delete(ownedResources, parseCascadingFlag(u.DeletionPropagation, u.cfg.Logger())) + timeout := u.Timeout + if !deadline.IsZero() { + if remaining := time.Until(deadline); remaining < timeout { + timeout = remaining } } - return ownedResources, kept.String(), errs + if err := waiter.WaitForDelete(ownedResources, timeout); err != nil { + errs = append(errs, err) + } + return ownedResources, errs } func parseCascadingFlag(cascadingFlag string, logger *slog.Logger) v1.DeletionPropagation { diff --git a/pkg/action/uninstall_sequenced_test.go b/pkg/action/uninstall_sequenced_test.go new file mode 100644 index 000000000..9f01d7ff6 --- /dev/null +++ b/pkg/action/uninstall_sequenced_test.go @@ -0,0 +1,648 @@ +/* +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 action + +import ( + "bytes" + "fmt" + "log/slog" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "helm.sh/helm/v4/pkg/chart/common" + chart "helm.sh/helm/v4/pkg/chart/v2" + "helm.sh/helm/v4/pkg/kube" + kubefake "helm.sh/helm/v4/pkg/kube/fake" + rcommon "helm.sh/helm/v4/pkg/release/common" + release "helm.sh/helm/v4/pkg/release/v1" + releaseutil "helm.sh/helm/v4/pkg/release/v1/util" +) + +func newSequencedUninstallAction(t *testing.T, kubeClient kube.Interface) *Uninstall { + t.Helper() + + cfg := actionConfigFixture(t) + cfg.KubeClient = kubeClient + + uninstall := NewUninstall(cfg) + uninstall.DisableHooks = true + uninstall.Timeout = 5 * time.Minute + uninstall.WaitStrategy = kube.OrderedWaitStrategy + + return uninstall +} + +func seedUninstallRelease(t *testing.T, uninstall *Uninstall, rel *release.Release) { + t.Helper() + require.NoError(t, uninstall.cfg.Releases.Create(rel)) +} + +func newUninstallRelease(name string, ch *chart.Chart, manifest string, sequencingInfo *release.SequencingInfo, hooks ...*release.Hook) *release.Release { + now := time.Now() + return &release.Release{ + Name: name, + Namespace: "spaced", + Chart: ch, + Config: map[string]any{}, + Manifest: manifest, + Info: &release.Info{ + FirstDeployed: now, + LastDeployed: now, + Status: rcommon.StatusDeployed, + Description: "Seeded release", + }, + Version: 1, + Hooks: hooks, + SequencingInfo: sequencingInfo, + } +} + +// sourcedManifest prefixes a manifest document with the "# Source:" comment the +// real render pipeline stores in rel.Manifest (action.go renderResourcesWithFiles). +func sourcedManifest(path, content string) string { + return "# Source: " + path + "\n" + content +} + +type commentStrippingPostRenderer struct{} + +func (commentStrippingPostRenderer) Run(in *bytes.Buffer) (*bytes.Buffer, error) { + var out bytes.Buffer + for line := range strings.SplitSeq(in.String(), "\n") { + if strings.HasPrefix(strings.TrimSpace(line), "#") { + continue + } + out.WriteString(line + "\n") + } + return &out, nil +} + +func secretManifest(name string) string { + return fmt.Sprintf(`apiVersion: v1 +kind: Secret +metadata: + name: %s +type: Opaque +data: + password: cGFzc3dvcmQ= +`, name) +} + +func serviceAccountManifest(name string) string { + return fmt.Sprintf(`apiVersion: v1 +kind: ServiceAccount +metadata: + name: %s +`, name) +} + +func expectedUninstallIDs(t *testing.T, manifest string) []string { + t.Helper() + + _, files, err := releaseutil.SortManifests(releaseutil.SplitManifests(manifest), nil, releaseutil.UninstallOrder) + require.NoError(t, err) + + ids := make([]string, 0, len(files)) + for _, file := range files { + ids = append(ids, fmt.Sprintf("%s/%s", file.Head.Kind, file.Head.Metadata.Name)) + } + return ids +} + +func filterDeleteCalls(calls [][]string, wanted ...string) [][]string { + want := make(map[string]struct{}, len(wanted)) + for _, id := range wanted { + want[id] = struct{}{} + } + + var filtered [][]string + for _, call := range calls { + if len(call) != 1 { + continue + } + if _, ok := want[call[0]]; ok { + filtered = append(filtered, call) + } + } + return filtered +} + +func operationIndex(operations []string, exact string) int { + for i, operation := range operations { + if operation == exact { + return i + } + } + return -1 +} + +func newDeleteHook(name string, event release.HookEvent) *release.Hook { + return &release.Hook{ + Name: name, + Kind: "ConfigMap", + Path: name, + Manifest: configMapManifest(name, nil), + Events: []release.HookEvent{event}, + DeletePolicies: []release.HookDeletePolicy{release.HookSucceeded}, + } +} + +func TestUninstall_Sequenced_ReverseOrder(t *testing.T) { + client := newRecordingKubeClient().withoutBuildClients() + uninstall := newSequencedUninstallAction(t, client) + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("sequenced-uninstall")) + + rel := newUninstallRelease( + "sequenced-uninstall", + ch, + joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), + &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/app"}, {"ConfigMap/database"}}, client.deleteCalls) + assert.Equal(t, client.deleteCalls, client.deleteWaitCalls) +} + +func TestUninstall_NonSequenced_Unchanged(t *testing.T) { + client := newRecordingKubeClient().withoutBuildClients() + uninstall := newSequencedUninstallAction(t, client) + + manifest := joinManifestDocs( + configMapManifest("config", nil), + secretManifest("secret"), + serviceAccountManifest("service-account"), + ) + rel := newUninstallRelease("standard-uninstall", buildChartWithTemplates(nil, withName("standard-uninstall")), manifest, nil) + seedUninstallRelease(t, uninstall, rel) + + _, err := uninstall.Run(rel.Name) + require.NoError(t, err) + + require.Len(t, client.deleteCalls, 1) + assert.Equal(t, expectedUninstallIDs(t, manifest), client.deleteCalls[0]) + require.Len(t, client.deleteWaitCalls, 1) + assert.Equal(t, client.deleteCalls[0], client.deleteWaitCalls[0]) +} + +func TestUninstall_Sequenced_WithSubcharts(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")) + + bar.AddDependency(nginx) + bar.Metadata.Dependencies = []*chart.Dependency{{Name: "nginx"}} + parent.AddDependency(bar) + parent.Metadata.Dependencies = []*chart.Dependency{{Name: "bar"}} + + rel := newUninstallRelease( + "subchart-uninstall", + 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) + + // Reverse-DAG uninstall: parent first, then bar (its own resources before + // recursing into bar's subchart batch), then nginx (innermost). With the + // chart-path threading fix, nested subcharts route through their proper + // subtree level instead of flattening. + assert.Equal(t, [][]string{{"ConfigMap/parent"}, {"ConfigMap/bar"}, {"ConfigMap/nginx"}}, client.deleteCalls) + assert.Equal(t, client.deleteCalls, client.deleteWaitCalls) +} + +// TestUninstall_Sequenced_DeletesVendoredSubchart locks bead i42: a subchart +// vendored into charts/ but absent from Chart.yaml dependencies deploys on +// install (TestInstall_Sequenced_UndeclaredVendoredSubchartDeployed) but the +// DAG-walking deleter never visited it. The plan's completeness invariant +// guarantees it is now deleted, after the parent's own resources and before +// declared subcharts (exact reverse of the forward plan). +func TestUninstall_Sequenced_DeletesVendoredSubchart(t *testing.T) { + client := newRecordingKubeClient().withoutBuildClients() + uninstall := newSequencedUninstallAction(t, client) + + parent := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/parent.yaml", "parent", nil), + }, withName("parent")) + declared := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + }, withName("database")) + vendored := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/vendored.yaml", "vendored", nil), + }, withName("vendored")) + + parent.AddDependency(declared, vendored) + parent.Metadata.Dependencies = []*chart.Dependency{{Name: "database"}} + + rel := newUninstallRelease( + "vendored-uninstall", + 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_DefaultWaitStrategyGatesBatches locks bead 4sj / +// master-plan D4: ordering intent is a property of the stored release, so a +// sequenced release uninstalled with the DEFAULT wait strategy (hookOnly, whose +// WaitForDelete is a no-op) must upgrade its batch-gating waiter to the status +// watcher. Hooks keep the user's strategy; this test disables hooks. +func TestUninstall_Sequenced_DefaultWaitStrategyGatesBatches(t *testing.T) { + client := newRecordingKubeClient().withoutBuildClients() + cfg := actionConfigFixture(t) + cfg.KubeClient = client + + uninstall := NewUninstall(cfg) + uninstall.DisableHooks = true + uninstall.Timeout = 5 * time.Minute + uninstall.WaitStrategy = kube.HookOnlyStrategy + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("default-wait-uninstall")) + + rel := newUninstallRelease( + "default-wait-uninstall", + ch, + joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), + nil, + ) + rel.Sequenced = true + seedUninstallRelease(t, uninstall, rel) + + _, err := uninstall.Run(rel.Name) + require.NoError(t, err) + + assert.Equal(t, [][]string{{"ConfigMap/app"}, {"ConfigMap/database"}}, client.deleteCalls) + assert.Equal(t, client.deleteCalls, client.deleteWaitCalls) + assert.Contains(t, client.waiterStrategies, kube.StatusWatcherStrategy) +} + +// TestUninstall_Sequenced_UsesStoredSourcePaths pins the D5 mechanism: the +// uninstall plan is built purely from the stored manifest's "# Source:" +// comments. The chart in this fixture would RE-RENDER to different resource +// names (values drift), which used to break the fuzzy content/identity +// matcher; the stored paths make re-rendering irrelevant. +func TestUninstall_Sequenced_UsesStoredSourcePaths(t *testing.T) { + client := newRecordingKubeClient().withoutBuildClients() + uninstall := newSequencedUninstallAction(t, client) + + parent := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/parent.yaml", "parent-renamed", nil), + }, withName("parent")) + bar := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/bar.yaml", "bar-renamed", nil), + }, withName("bar")) + + parent.AddDependency(bar) + parent.Metadata.Dependencies = []*chart.Dependency{{Name: "bar"}} + + rel := newUninstallRelease( + "stored-source-uninstall", + 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)}, + ) + seedUninstallRelease(t, uninstall, rel) + + _, err := uninstall.Run(rel.Name) + require.NoError(t, err) + + assert.Equal(t, [][]string{{"ConfigMap/parent"}, {"ConfigMap/bar"}}, client.deleteCalls) + assert.Equal(t, client.deleteCalls, client.deleteWaitCalls) +} + +// TestUninstall_Sequenced_AfterPostRenderedInstall pins the epic's D5 premise +// end-to-end: "# Source:" comments are appended to the stored manifest AFTER +// post-rendering (from the filename annotation), so even a comment-stripping +// post-renderer (kustomize-style) leaves the stored record fully +// plan-recoverable. NOTE: this test also passes before the Stage-C rewrite +// (the old path recovered order by re-rendering); it exists so any future +// regression of the premise fails loudly here rather than in production. +func TestUninstall_Sequenced_AfterPostRenderedInstall(t *testing.T) { + client := newRecordingKubeClient() + client.buildRESTClient = statefulOwnedRESTClient(client, "test-install-release", "spaced") + + install := newSequencedInstallAction(t, client) + install.PostRenderer = commentStrippingPostRenderer{} + + 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"}} + + rel := mustRelease(t, mustRunInstall(t, install, parent)) + assert.Contains(t, rel.Manifest, "# Source: parent/charts/bar/templates/") + assert.Contains(t, rel.Manifest, "# Source: parent/templates/") + + uninstall := NewUninstall(install.cfg) + uninstall.DisableHooks = true + uninstall.Timeout = 5 * time.Minute + uninstall.WaitStrategy = kube.OrderedWaitStrategy + + _, err := uninstall.Run(rel.Name) + require.NoError(t, err) + + assert.Equal(t, [][]string{{"ConfigMap/parent"}, {"ConfigMap/bar"}}, client.deleteCalls) + assert.Equal(t, client.deleteCalls, client.deleteWaitCalls) +} + +func TestUninstall_Sequenced_KeepPolicy(t *testing.T) { + client := newRecordingKubeClient().withoutBuildClients() + uninstall := newSequencedUninstallAction(t, client) + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database-live.yaml", "database-live", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/database-keep.yaml", "database-keep", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + kube.ResourcePolicyAnno: kube.KeepPolicy, + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("keep-policy")) + + rel := newUninstallRelease( + "keep-policy", + ch, + joinManifestDocs( + configMapManifest("database-live", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("database-keep", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + kube.ResourcePolicyAnno: kube.KeepPolicy, + }), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), + &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}, + ) + seedUninstallRelease(t, uninstall, rel) + + res, err := uninstall.Run(rel.Name) + require.NoError(t, err) + + assert.Equal(t, [][]string{{"ConfigMap/app"}, {"ConfigMap/database-live"}}, client.deleteCalls) + assert.Equal(t, client.deleteCalls, client.deleteWaitCalls) + assert.NotContains(t, client.deleteCalls, []string{"ConfigMap/database-keep"}) + assert.Contains(t, res.Info, "[ConfigMap] database-keep") +} + +func TestUninstall_Sequenced_KeepHistory(t *testing.T) { + client := newRecordingKubeClient().withoutBuildClients() + uninstall := newSequencedUninstallAction(t, client) + uninstall.KeepHistory = true + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("keep-history")) + + rel := newUninstallRelease( + "keep-history", + ch, + joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), + &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}, + ) + seedUninstallRelease(t, uninstall, rel) + + _, err := uninstall.Run(rel.Name) + require.NoError(t, err) + + 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, [][]string{{"ConfigMap/app"}, {"ConfigMap/database"}}, client.deleteCalls) + assert.Equal(t, rcommon.StatusUninstalled, stored.Info.Status) +} + +func TestUninstall_Sequenced_DryRun(t *testing.T) { + client := newRecordingKubeClient().withoutBuildClients() + uninstall := newSequencedUninstallAction(t, client) + uninstall.DryRun = true + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("dry-run-uninstall")) + + rel := newUninstallRelease( + "dry-run-uninstall", + ch, + joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), + &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}, + ) + seedUninstallRelease(t, uninstall, rel) + + res, err := uninstall.Run(rel.Name) + require.NoError(t, err) + require.NotNil(t, res) + assert.Empty(t, client.deleteCalls) + assert.Empty(t, client.deleteWaitCalls) +} + +func TestUninstall_Sequenced_Hooks(t *testing.T) { + client := newRecordingKubeClient().withoutBuildClients() + uninstall := newSequencedUninstallAction(t, client) + uninstall.DisableHooks = false + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("hooked-uninstall")) + + rel := newUninstallRelease( + "hooked-uninstall", + ch, + joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), + &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}, + newDeleteHook("pre-hook", release.HookPreDelete), + newDeleteHook("post-hook", release.HookPostDelete), + ) + seedUninstallRelease(t, uninstall, rel) + + _, err := uninstall.Run(rel.Name) + require.NoError(t, err) + + assert.Equal(t, [][]string{{"ConfigMap/pre-hook"}, {"ConfigMap/post-hook"}}, client.createCalls) + assert.Equal(t, [][]string{{"ConfigMap/pre-hook"}, {"ConfigMap/post-hook"}}, client.watchUntilReadyCalls) + assert.Equal(t, [][]string{{"ConfigMap/app"}, {"ConfigMap/database"}}, filterDeleteCalls(client.deleteCalls, "ConfigMap/app", "ConfigMap/database")) + + preHookCreate := operationIndex(client.operations, "create:ConfigMap/pre-hook") + appDelete := operationIndex(client.operations, "delete:ConfigMap/app") + databaseWaitDelete := operationIndex(client.operations, "wait-delete:ConfigMap/database") + postHookCreate := operationIndex(client.operations, "create:ConfigMap/post-hook") + + require.NotEqual(t, -1, preHookCreate) + require.NotEqual(t, -1, appDelete) + require.NotEqual(t, -1, databaseWaitDelete) + require.NotEqual(t, -1, postHookCreate) + assert.Less(t, preHookCreate, appDelete) + assert.Greater(t, postHookCreate, databaseWaitDelete) +} + +// TestUninstall_Sequenced_SkipsUnownedResources locks the fix for the sequenced +// uninstall ownership gap: the sequenced delete path must verify ownership and +// skip resources it does not own, matching the non-sequenced path, rather than +// deleting them. +func TestUninstall_Sequenced_SkipsUnownedResources(t *testing.T) { + is := assert.New(t) + + logBuffer := &bytes.Buffer{} + config := actionConfigFixture(t) + config.SetLogger(slog.NewTextHandler(logBuffer, &slog.HandlerOptions{Level: slog.LevelWarn})) + + uninstall := NewUninstall(config) + uninstall.DisableHooks = true + uninstall.KeepHistory = true + uninstall.WaitStrategy = kube.OrderedWaitStrategy + + ch := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + }), + }, withName("sequenced-ownership")) + + rel := newUninstallRelease( + "sequenced-ownership", + ch, + configMapManifest("app", map[string]string{releaseutil.AnnotationResourceGroup: "app"}), + &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}, + ) + seedUninstallRelease(t, uninstall, rel) + + // Build resolves every manifest to a resource that is NOT owned by this + // release (no Helm ownership metadata). The sequenced path must skip it. + failer := config.KubeClient.(*kubefake.FailingKubeClient) + failer.DummyResources = kube.ResourceList{ + newDeploymentWithOwner("unowned-deploy", rel.Namespace, nil, nil), + } + + _, err := uninstall.Run(rel.Name) + is.NoError(err) + + logOutput := logBuffer.String() + is.Contains(logOutput, "skipping delete of resource not owned by this release") + is.Contains(logOutput, "unowned-deploy") +} diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 81d51164f..1b85d87d1 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -40,6 +40,7 @@ import ( ri "helm.sh/helm/v4/pkg/release" rcommon "helm.sh/helm/v4/pkg/release/common" 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" "helm.sh/helm/v4/pkg/storage/driver" ) @@ -70,6 +71,10 @@ type Upgrade struct { SkipCRDs bool // Timeout is the timeout for this operation Timeout time.Duration + // ReadinessTimeout is the per-batch timeout when --wait=ordered is used. + // Each ordered batch waits at most this long for resources to become ready. + // Must not exceed Timeout. Defaults to 1 minute when zero. + ReadinessTimeout time.Duration // WaitStrategy determines what type of waiting should be done WaitStrategy kube.WaitStrategy // WaitOptions are additional options for waiting on resources @@ -188,12 +193,16 @@ func (u *Upgrade) RunWithContext(ctx context.Context, name string, ch chart.Char u.WaitStrategy = kube.StatusWatcherStrategy } + if u.ReadinessTimeout > 0 && u.Timeout > 0 && u.ReadinessTimeout > u.Timeout { + return nil, fmt.Errorf("--readiness-timeout (%s) must not exceed --timeout (%s)", u.ReadinessTimeout, u.Timeout) + } + if err := chartutil.ValidateReleaseName(name); err != nil { return nil, fmt.Errorf("release name is invalid: %s", name) } u.cfg.Logger().Debug("preparing upgrade", "name", name) - currentRelease, upgradedRelease, serverSideApply, err := u.prepareUpgrade(ctx, name, chrt, vals) + currentRelease, upgradedRelease, serverSideApply, sortedManifests, err := u.prepareUpgrade(ctx, name, chrt, vals) if err != nil { return nil, err } @@ -201,7 +210,12 @@ func (u *Upgrade) RunWithContext(ctx context.Context, name string, ch chart.Char u.cfg.Releases.MaxHistory = u.MaxHistory u.cfg.Logger().Debug("performing update", "name", name) - res, err := u.performUpgrade(ctx, currentRelease, upgradedRelease, serverSideApply) + var res *release.Release + if u.WaitStrategy == kube.OrderedWaitStrategy { + res, err = u.performSequencedUpgrade(ctx, chrt, currentRelease, upgradedRelease, sortedManifests, serverSideApply) + } else { + res, err = u.performUpgrade(ctx, currentRelease, upgradedRelease, serverSideApply) + } if err != nil { return res, err } @@ -218,14 +232,16 @@ func (u *Upgrade) RunWithContext(ctx context.Context, name string, ch chart.Char } // prepareUpgrade builds an upgraded release for an upgrade operation. -func (u *Upgrade) prepareUpgrade(ctx context.Context, name string, chart *chartv2.Chart, vals map[string]any) (*release.Release, *release.Release, bool, error) { +// When WaitStrategy is OrderedWaitStrategy, sortedManifests contains the parsed +// manifests with annotation data needed for DAG sequencing; otherwise it is nil. +func (u *Upgrade) prepareUpgrade(ctx context.Context, name string, chart *chartv2.Chart, vals map[string]any) (*release.Release, *release.Release, bool, []releaseutil.Manifest, error) { if chart == nil { - return nil, nil, false, errMissingChart + return nil, nil, false, nil, errMissingChart } // HideSecret must be used with dry run. Otherwise, return an error. if !isDryRun(u.DryRunStrategy) && u.HideSecret { - return nil, nil, false, errors.New("hiding Kubernetes secrets requires a dry-run mode") + return nil, nil, false, nil, errors.New("hiding Kubernetes secrets requires a dry-run mode") } // finds the last non-deleted release with the given name @@ -233,19 +249,19 @@ func (u *Upgrade) prepareUpgrade(ctx context.Context, name string, chart *chartv if err != nil { // to keep existing behavior of returning the "%q has no deployed releases" error when an existing release does not exist if errors.Is(err, driver.ErrReleaseNotFound) { - return nil, nil, false, driver.NewErrNoDeployedReleases(name) + return nil, nil, false, nil, driver.NewErrNoDeployedReleases(name) } - return nil, nil, false, err + return nil, nil, false, nil, err } lastRelease, err := releaserToV1Release(lastReleasei) if err != nil { - return nil, nil, false, err + return nil, nil, false, nil, err } // Concurrent `helm upgrade`s will either fail here with `errPending` or when creating the release with "already exists". This should act as a pessimistic lock. if lastRelease.Info.Status.IsPending() { - return nil, nil, false, errPending + return nil, nil, false, nil, errPending } var currentRelease *release.Release @@ -258,14 +274,14 @@ func (u *Upgrade) prepareUpgrade(ctx context.Context, name string, chart *chartv var cerr error currentRelease, cerr = releaserToV1Release(currentReleasei) if cerr != nil { - return nil, nil, false, cerr + return nil, nil, false, nil, cerr } if err != nil { if errors.Is(err, driver.ErrNoDeployedReleases) && (lastRelease.Info.Status == rcommon.StatusFailed || lastRelease.Info.Status == rcommon.StatusSuperseded) { currentRelease = lastRelease } else { - return nil, nil, false, err + return nil, nil, false, nil, err } } } @@ -273,11 +289,11 @@ func (u *Upgrade) prepareUpgrade(ctx context.Context, name string, chart *chartv // determine if values will be reused vals, err = u.reuseValues(chart, currentRelease, vals) if err != nil { - return nil, nil, false, err + return nil, nil, false, nil, err } if err := chartutil.ProcessDependencies(chart, vals); err != nil { - return nil, nil, false, err + return nil, nil, false, nil, err } // Increment revision count. This is passed to templates, and also stored on @@ -293,25 +309,33 @@ func (u *Upgrade) prepareUpgrade(ctx context.Context, name string, chart *chartv caps, err := u.cfg.getCapabilities() if err != nil { - return nil, nil, false, err + return nil, nil, false, nil, err } valuesToRender, err := util.ToRenderValuesWithSchemaValidation(chart, vals, options, caps, u.SkipSchemaValidation) if err != nil { - return nil, nil, false, err + return nil, nil, false, nil, err } - hooks, manifestDoc, notesTxt, err := u.cfg.renderResources(ctx, chart, valuesToRender, "", "", u.SubNotes, false, false, u.PostRenderer, interactWithServer(u.DryRunStrategy), u.EnableDNS, u.HideSecret, u.PostRenderStrategy) + var sortedManifests []releaseutil.Manifest + var hooks []*release.Hook + var manifestDoc *bytes.Buffer + var notesTxt string + if u.WaitStrategy == kube.OrderedWaitStrategy { + hooks, manifestDoc, notesTxt, sortedManifests, err = u.cfg.renderResourcesWithFiles(ctx, chart, valuesToRender, "", "", u.SubNotes, false, false, u.PostRenderer, interactWithServer(u.DryRunStrategy), u.EnableDNS, u.HideSecret, u.PostRenderStrategy) + } else { + hooks, manifestDoc, notesTxt, err = u.cfg.renderResources(ctx, chart, valuesToRender, "", "", u.SubNotes, false, false, u.PostRenderer, interactWithServer(u.DryRunStrategy), u.EnableDNS, u.HideSecret, u.PostRenderStrategy) + } if err != nil { - return nil, nil, false, err + return nil, nil, false, nil, err } if driver.ContainsSystemLabels(u.Labels) { - return nil, nil, false, fmt.Errorf("user supplied labels contains system reserved label name. System labels: %+v", driver.GetSystemLabels()) + return nil, nil, false, nil, fmt.Errorf("user supplied labels contains system reserved label name. System labels: %+v", driver.GetSystemLabels()) } serverSideApply, err := getUpgradeServerSideValue(u.ServerSideApply, lastRelease.ApplyMethod) if err != nil { - return nil, nil, false, err + return nil, nil, false, nil, err } u.cfg.Logger().Debug("determined release apply method", slog.Bool("server_side_apply", serverSideApply), slog.String("previous_release_apply_method", lastRelease.ApplyMethod)) @@ -333,13 +357,14 @@ func (u *Upgrade) prepareUpgrade(ctx context.Context, name string, chart *chartv Hooks: hooks, Labels: mergeCustomLabels(lastRelease.Labels, u.Labels), ApplyMethod: string(determineReleaseSSApplyMethod(serverSideApply)), + Sequenced: u.WaitStrategy == kube.OrderedWaitStrategy, } if len(notesTxt) > 0 { upgradedRelease.Info.Notes = notesTxt } err = validateManifest(u.cfg.KubeClient, manifestDoc.Bytes(), !u.DisableOpenAPIValidation) - return currentRelease, upgradedRelease, serverSideApply, err + return currentRelease, upgradedRelease, serverSideApply, sortedManifests, err } func (u *Upgrade) performUpgrade(ctx context.Context, originalRelease, upgradedRelease *release.Release, serverSideApply bool) (*release.Release, error) { @@ -414,7 +439,7 @@ func (u *Upgrade) performUpgrade(ctx context.Context, originalRelease, upgradedR ctxChan := make(chan resultMessage) doneChan := make(chan any) defer close(doneChan) - go u.releasingUpgrade(rChan, upgradedRelease, current, target, originalRelease, serverSideApply) + go u.releasingUpgrade(ctx, rChan, upgradedRelease, current, target, originalRelease, serverSideApply) go u.handleContext(ctx, doneChan, ctxChan, upgradedRelease) select { @@ -428,10 +453,10 @@ func (u *Upgrade) performUpgrade(ctx context.Context, originalRelease, upgradedR // Function used to lock the Mutex, this is important for the case when RollbackOnFailure is set. // In that case the upgrade will finish before the rollback is finished so it is necessary to wait for the rollback to finish. // The rollback will be trigger by the function failRelease -func (u *Upgrade) reportToPerformUpgrade(c chan<- resultMessage, rel *release.Release, created kube.ResourceList, err error) { +func (u *Upgrade) reportToPerformUpgrade(ctx context.Context, c chan<- resultMessage, rel *release.Release, created kube.ResourceList, err error) { u.Lock.Lock() if err != nil { - rel, err = u.failRelease(rel, created, err) + rel, err = u.failRelease(ctx, rel, created, err) } c <- resultMessage{r: rel, e: err} u.Lock.Unlock() @@ -444,7 +469,7 @@ func (u *Upgrade) handleContext(ctx context.Context, done chan any, c chan<- res err := ctx.Err() // when RollbackOnFailure is set, the ongoing release finish first and doesn't give time for the rollback happens. - u.reportToPerformUpgrade(c, upgradedRelease, kube.ResourceList{}, err) + u.reportToPerformUpgrade(ctx, c, upgradedRelease, kube.ResourceList{}, err) case <-done: return } @@ -454,18 +479,24 @@ func isReleaseApplyMethodClientSideApply(applyMethod string) bool { return applyMethod == "" || applyMethod == string(release.ApplyMethodClientSideApply) } -func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *release.Release, current kube.ResourceList, target kube.ResourceList, originalRelease *release.Release, serverSideApply bool) { +func (u *Upgrade) releasingUpgrade(ctx context.Context, c chan<- resultMessage, upgradedRelease *release.Release, current kube.ResourceList, target kube.ResourceList, originalRelease *release.Release, serverSideApply bool) { // pre-upgrade hooks if !u.DisableHooks { if err := u.cfg.execHook(upgradedRelease, release.HookPreUpgrade, u.WaitStrategy, u.WaitOptions, u.Timeout, serverSideApply); err != nil { - u.reportToPerformUpgrade(c, upgradedRelease, kube.ResourceList{}, fmt.Errorf("pre-upgrade hooks failed: %w", err)) + u.reportToPerformUpgrade(ctx, c, upgradedRelease, kube.ResourceList{}, fmt.Errorf("pre-upgrade hooks failed: %w", err)) return } } else { u.cfg.Logger().Debug("upgrade hooks disabled", "name", upgradedRelease.Name) } + // Strip Helm-internal sequencing annotations before applying to K8s. + if err := stripSequencingAnnotations(target); err != nil { + u.reportToPerformUpgrade(ctx, c, upgradedRelease, kube.ResourceList{}, fmt.Errorf("stripping sequencing annotations: %w", err)) + return + } + upgradeClientSideFieldManager := isReleaseApplyMethodClientSideApply(originalRelease.ApplyMethod) && serverSideApply // Update client-side field manager if transitioning from client-side to server-side apply results, err := u.cfg.KubeClient.Update( current, @@ -475,7 +506,7 @@ func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *rele kube.ClientUpdateOptionUpgradeClientSideFieldManager(upgradeClientSideFieldManager)) if err != nil { u.cfg.recordRelease(originalRelease) - u.reportToPerformUpgrade(c, upgradedRelease, results.Created, err) + u.reportToPerformUpgrade(ctx, c, upgradedRelease, results.Created, err) return } @@ -487,19 +518,19 @@ func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *rele } if err != nil { u.cfg.recordRelease(originalRelease) - u.reportToPerformUpgrade(c, upgradedRelease, results.Created, err) + u.reportToPerformUpgrade(ctx, c, upgradedRelease, results.Created, err) return } if u.WaitForJobs { if err := waiter.WaitWithJobs(target, u.Timeout); err != nil { u.cfg.recordRelease(originalRelease) - u.reportToPerformUpgrade(c, upgradedRelease, results.Created, err) + u.reportToPerformUpgrade(ctx, c, upgradedRelease, results.Created, err) return } } else { if err := waiter.Wait(target, u.Timeout); err != nil { u.cfg.recordRelease(originalRelease) - u.reportToPerformUpgrade(c, upgradedRelease, results.Created, err) + u.reportToPerformUpgrade(ctx, c, upgradedRelease, results.Created, err) return } } @@ -507,7 +538,7 @@ func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *rele // post-upgrade hooks if !u.DisableHooks { if err := u.cfg.execHook(upgradedRelease, release.HookPostUpgrade, u.WaitStrategy, u.WaitOptions, u.Timeout, serverSideApply); err != nil { - u.reportToPerformUpgrade(c, upgradedRelease, results.Created, fmt.Errorf("post-upgrade hooks failed: %w", err)) + u.reportToPerformUpgrade(ctx, c, upgradedRelease, results.Created, fmt.Errorf("post-upgrade hooks failed: %w", err)) return } } @@ -521,10 +552,175 @@ func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *rele } else { upgradedRelease.Info.Description = "Upgrade complete" } - u.reportToPerformUpgrade(c, upgradedRelease, nil, nil) + u.reportToPerformUpgrade(ctx, c, upgradedRelease, nil, nil) +} + +// performSequencedUpgrade deploys chart resources in DAG-ordered batches when +// --wait=ordered is used. It mirrors releasingUpgrade but uses sequencedDeployment +// to apply each batch and wait for readiness before proceeding to the next batch. +func (u *Upgrade) performSequencedUpgrade(ctx context.Context, chrt *chartv2.Chart, currentRelease, upgradedRelease *release.Release, manifests []releaseutil.Manifest, serverSideApply bool) (*release.Release, error) { + // Build the full set of current (old) resources for diff matching. + current, err := u.cfg.KubeClient.Build(bytes.NewBufferString(currentRelease.Manifest), false) + if err != nil { + if strings.Contains(err.Error(), "unable to recognize \"\": no matches for kind") { + return upgradedRelease, fmt.Errorf("current release manifest contains removed kubernetes api(s) for this "+ + "kubernetes version and it is therefore unable to build the kubernetes "+ + "objects for performing the diff. error from kubernetes: %w", err) + } + return upgradedRelease, fmt.Errorf("unable to build kubernetes objects from current release manifest: %w", err) + } + + // Build target to compute the set of resources to delete (those removed in the new release). + target, err := u.cfg.KubeClient.Build(bytes.NewBufferString(upgradedRelease.Manifest), !u.DisableOpenAPIValidation) + if err != nil { + return upgradedRelease, fmt.Errorf("unable to build kubernetes objects from new release manifest: %w", err) + } + + // It is safe to use force only on target because these are resources currently rendered by the chart. + if err := target.Visit(setMetadataVisitor(upgradedRelease.Name, upgradedRelease.Namespace, true)); err != nil { + return upgradedRelease, err + } + + // Do a basic diff using gvk + name to figure out what new resources are being + // created so we can validate they don't already exist (or adopt them). + existingResources := make(map[string]bool, len(current)) + for _, r := range current { + existingResources[objectKey(r)] = true + } + var toBeCreated kube.ResourceList + for _, r := range target { + if !existingResources[objectKey(r)] { + toBeCreated = append(toBeCreated, r) + } + } + + var toBeUpdated kube.ResourceList + if u.TakeOwnership { + toBeUpdated, err = requireAdoption(toBeCreated) + } else { + toBeUpdated, err = existingResourceConflict(toBeCreated, upgradedRelease.Name, upgradedRelease.Namespace) + } + if err != nil { + return nil, fmt.Errorf("unable to continue with update: %w", err) + } + + // Adopted resources join current so their plan batches update them in place. + toBeUpdated.Visit(func(r *resource.Info, err error) error { + if err != nil { + return err + } + current.Append(r) + return nil + }) + + if isDryRun(u.DryRunStrategy) { + u.cfg.Logger().Debug("dry run for release", "name", upgradedRelease.Name) + if len(u.Description) > 0 { + upgradedRelease.Info.Description = u.Description + } else { + upgradedRelease.Info.Description = "Dry run complete" + } + return upgradedRelease, nil + } + + plan, err := sequence.Build(chrt, manifests) + if err != nil { + return upgradedRelease, fmt.Errorf("building sequencing plan: %w", err) + } + + u.cfg.Logger().Debug("creating upgraded release", "name", upgradedRelease.Name) + if err := u.cfg.Releases.Create(upgradedRelease); err != nil { + return nil, err + } + + // pre-upgrade hooks + if !u.DisableHooks { + if err := u.cfg.execHook(upgradedRelease, release.HookPreUpgrade, u.WaitStrategy, u.WaitOptions, u.Timeout, serverSideApply); err != nil { + return u.failRelease(ctx, upgradedRelease, nil, fmt.Errorf("pre-upgrade hooks failed: %w", err)) + } + } else { + u.cfg.Logger().Debug("upgrade hooks disabled", "name", upgradedRelease.Name) + } + + upgradeCSAFieldManager := isReleaseApplyMethodClientSideApply(currentRelease.ApplyMethod) && serverSideApply + + readinessTimeout := u.ReadinessTimeout + if readinessTimeout <= 0 { + readinessTimeout = time.Minute + } + + sd := &sequencedDeployment{ + cfg: u.cfg, + releaseName: upgradedRelease.Name, + releaseNamespace: upgradedRelease.Namespace, + disableOpenAPI: u.DisableOpenAPIValidation, + serverSideApply: serverSideApply, + forceConflicts: u.ForceConflicts, + forceReplace: u.ForceReplace, + waitStrategy: u.WaitStrategy, + waitOptions: u.WaitOptions, + waitForJobs: u.WaitForJobs, + timeout: u.Timeout, + readinessTimeout: readinessTimeout, + deadline: computeDeadline(u.Timeout), + upgradeMode: true, + currentResources: current, + upgradeCSAFieldManager: upgradeCSAFieldManager, + } + + logPlanWarnings(u.cfg.Logger(), plan) + if err := sd.apply(ctx, plan); err != nil { + return u.failRelease(ctx, upgradedRelease, sd.createdResources, err) + } + + // Delete resources present in the old release but absent from the new one, + // in exact reverse deployment order of the old release's plan (7yi). + allNewKeys := make(map[string]bool, len(target)) + for _, r := range target { + allNewKeys[objectKey(r)] = true + } + removedKeys := make(map[string]bool) + var toBeDeleted kube.ResourceList + for _, r := range current { + if !allNewKeys[objectKey(r)] { + removedKeys[objectKey(r)] = true + toBeDeleted = append(toBeDeleted, r) + } + } + if len(toBeDeleted) > 0 { + if err := u.deleteRemovedSequenced(currentRelease, sd, removedKeys, toBeDeleted); err != nil { + return u.failRelease(ctx, upgradedRelease, sd.createdResources, err) + } + } + + // post-upgrade hooks + if !u.DisableHooks { + if err := u.cfg.execHook(upgradedRelease, release.HookPostUpgrade, u.WaitStrategy, u.WaitOptions, u.Timeout, serverSideApply); err != nil { + return u.failRelease(ctx, upgradedRelease, sd.createdResources, fmt.Errorf("post-upgrade hooks failed: %w", err)) + } + } + + currentRelease.Info.Status = rcommon.StatusSuperseded + u.cfg.recordRelease(currentRelease) + upgradedRelease.Info.Status = rcommon.StatusDeployed + if len(u.Description) > 0 { + upgradedRelease.Info.Description = u.Description + } else { + upgradedRelease.Info.Description = "Upgrade complete" + } + u.cfg.recordRelease(upgradedRelease) + return upgradedRelease, nil +} + +func (u *Upgrade) deleteRemovedSequenced(currentRelease *release.Release, sd *sequencedDeployment, removedKeys map[string]bool, toBeDeleted kube.ResourceList) error { + waiter, err := getWaiterFor(u.cfg.KubeClient, u.WaitStrategy, u.WaitOptions...) + if err != nil { + return fmt.Errorf("getting waiter for removed-resource deletion: %w", err) + } + return sd.deleteRemovedFromOldRelease(currentRelease, removedKeys, toBeDeleted, waiter) } -func (u *Upgrade) failRelease(rel *release.Release, created kube.ResourceList, err error) (*release.Release, error) { +func (u *Upgrade) failRelease(ctx context.Context, rel *release.Release, created kube.ResourceList, err error) (*release.Release, error) { msg := fmt.Sprintf("Upgrade %q failed: %s", rel.Name, err) u.cfg.Logger().Warn( "upgrade failed", @@ -588,7 +784,7 @@ func (u *Upgrade) failRelease(rel *release.Release, created kube.ResourceList, e rollin.ForceConflicts = u.ForceConflicts rollin.ServerSideApply = u.ServerSideApply rollin.Timeout = u.Timeout - if rollErr := rollin.Run(rel.Name); rollErr != nil { + if rollErr := rollin.RunWithContext(ctx, rel.Name); rollErr != nil { return rel, fmt.Errorf("an error occurred while rolling back the release. original upgrade error: %w: %w", err, rollErr) } return rel, fmt.Errorf("release %s failed, and has been rolled back due to rollback-on-failure being set: %w", rel.Name, err) diff --git a/pkg/action/upgrade_sequenced_test.go b/pkg/action/upgrade_sequenced_test.go new file mode 100644 index 000000000..ec493711c --- /dev/null +++ b/pkg/action/upgrade_sequenced_test.go @@ -0,0 +1,559 @@ +/* +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 action + +import ( + "errors" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "helm.sh/helm/v4/pkg/chart/common" + chart "helm.sh/helm/v4/pkg/chart/v2" + "helm.sh/helm/v4/pkg/kube" + ri "helm.sh/helm/v4/pkg/release" + rcommon "helm.sh/helm/v4/pkg/release/common" + release "helm.sh/helm/v4/pkg/release/v1" + releaseutil "helm.sh/helm/v4/pkg/release/v1/util" +) + +func newSequencedUpgradeAction(t *testing.T, kubeClient kube.Interface) *Upgrade { + t.Helper() + cfg := actionConfigFixture(t) + cfg.KubeClient = kubeClient + + upgrade := NewUpgrade(cfg) + upgrade.Namespace = "spaced" + upgrade.Timeout = 5 * time.Minute + upgrade.ReadinessTimeout = time.Minute + upgrade.WaitStrategy = kube.OrderedWaitStrategy + return upgrade +} + +func seedDeployedRelease(t *testing.T, upgrade *Upgrade, name string, ch *chart.Chart, manifest string, sequencingInfo *release.SequencingInfo) { + t.Helper() + + now := time.Now() + rel := &release.Release{ + Name: name, + Namespace: "spaced", + Chart: ch, + Config: map[string]any{}, + Manifest: manifest, + Info: &release.Info{ + FirstDeployed: now, + LastDeployed: now, + Status: rcommon.StatusDeployed, + Description: "Seeded release", + }, + Version: 1, + SequencingInfo: sequencingInfo, + } + + require.NoError(t, upgrade.cfg.Releases.Create(rel)) +} + +func joinManifestDocs(docs ...string) string { + return strings.Join(docs, "\n---\n") +} + +func updateTargets(calls []updateCall) [][]string { + targets := make([][]string, 0, len(calls)) + for _, call := range calls { + targets = append(targets, call.target) + } + return targets +} + +func TestUpgrade_Sequenced_Basic(t *testing.T) { + client := newRecordingKubeClient() + upgrade := newSequencedUpgradeAction(t, client) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("sequenced-upgrade")) + seedDeployedRelease(t, upgrade, "sequenced-upgrade", currentChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), nil) + + upgradedChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("sequenced-upgrade")) + + rel := mustRelease(t, mustRunUpgrade(t, upgrade, "sequenced-upgrade", upgradedChart)) + + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/app"}}, updateTargets(client.updateCalls)) + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/app"}}, client.waitCalls) + require.Len(t, client.updateCalls, 2) + assert.Equal(t, []string{"ConfigMap/database"}, client.updateCalls[0].current) + assert.Equal(t, []string{"ConfigMap/app"}, client.updateCalls[1].current) + assert.Equal(t, rcommon.StatusDeployed, rel.Info.Status) + assert.True(t, rel.Sequenced) + assert.True(t, rel.IsSequenced()) + assert.Nil(t, rel.SequencingInfo) +} + +func TestUpgrade_Sequenced_NewResourceGroup(t *testing.T) { + client := newRecordingKubeClient() + upgrade := newSequencedUpgradeAction(t, client) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("new-group")) + seedDeployedRelease(t, upgrade, "new-group", currentChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), nil) + + upgradedChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/queue.yaml", "queue", map[string]string{ + releaseutil.AnnotationResourceGroup: "queue", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["queue"]`, + }), + }, withName("new-group")) + + mustRunUpgrade(t, upgrade, "new-group", upgradedChart) + + require.Len(t, client.updateCalls, 3) + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/queue"}, {"ConfigMap/app"}}, updateTargets(client.updateCalls)) + assert.Empty(t, client.updateCalls[1].current) + assert.Equal(t, []string{"ConfigMap/queue"}, client.updateCalls[1].created) + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/queue"}, {"ConfigMap/app"}}, client.waitCalls) +} + +func TestUpgrade_Sequenced_RemovedResourceGroup(t *testing.T) { + client := newRecordingKubeClient() + upgrade := newSequencedUpgradeAction(t, client) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/queue.yaml", "queue", map[string]string{ + releaseutil.AnnotationResourceGroup: "queue", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["queue"]`, + }), + }, withName("removed-group")) + seedDeployedRelease(t, upgrade, "removed-group", currentChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("queue", map[string]string{ + releaseutil.AnnotationResourceGroup: "queue", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["queue"]`, + }), + ), nil) + + upgradedChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("removed-group")) + + mustRunUpgrade(t, upgrade, "removed-group", upgradedChart) + + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/app"}}, updateTargets(client.updateCalls)) + assert.Equal(t, [][]string{{"ConfigMap/queue"}}, client.deleteCalls) + assert.Equal(t, []string{ + "update:ConfigMap/database", + "wait:ConfigMap/database", + "update:ConfigMap/app", + "wait:ConfigMap/app", + "delete:ConfigMap/queue", + "wait-delete:ConfigMap/queue", + }, client.operations) +} + +// TestUpgrade_Sequenced_NewResourceConflictAndAdoption locks bead 9ui +// (upgrade): a sequenced upgrade must run the same existing-resource +// conflict / adoption step over newly added resources as the default path +// (performUpgrade) does. +func TestUpgrade_Sequenced_NewResourceConflictAndAdoption(t *testing.T) { + currentChartFiles := []*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + } + currentManifest := configMapManifest("database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }) + upgradedChartFiles := []*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/queue.yaml", "queue", map[string]string{ + releaseutil.AnnotationResourceGroup: "queue", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + } + + t.Run("unowned existing resource conflicts before anything is applied", func(t *testing.T) { + client := newRecordingKubeClient() + // queue already exists in the cluster, owned by another release. + client.buildRESTClient = statefulOwnedRESTClient(client, "some-other-release", "spaced") + client.createdObjects["ConfigMap/queue"] = true + + upgrade := newSequencedUpgradeAction(t, client) + seedDeployedRelease(t, upgrade, "adopt-upgrade", buildChartWithTemplates(currentChartFiles, withName("adopt-upgrade")), currentManifest, nil) + + _, err := upgrade.Run("adopt-upgrade", buildChartWithTemplates(upgradedChartFiles, withName("adopt-upgrade")), map[string]any{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "unable to continue with update") + assert.Empty(t, client.updateCalls, "no batch may be applied after a conflict") + }) + + t.Run("take-ownership adopts the existing resource in its batch", func(t *testing.T) { + client := newRecordingKubeClient() + client.buildRESTClient = statefulOwnedRESTClient(client, "some-other-release", "spaced") + client.createdObjects["ConfigMap/queue"] = true + + upgrade := newSequencedUpgradeAction(t, client) + upgrade.TakeOwnership = true + seedDeployedRelease(t, upgrade, "adopt-upgrade-to", buildChartWithTemplates(currentChartFiles, withName("adopt-upgrade-to")), currentManifest, nil) + + mustRunUpgrade(t, upgrade, "adopt-upgrade-to", buildChartWithTemplates(upgradedChartFiles, withName("adopt-upgrade-to"))) + + require.Len(t, client.updateCalls, 2) + assert.Equal(t, []string{"ConfigMap/queue"}, client.updateCalls[1].current, + "pre-existing queue must be adopted (updated against a current object), not treated as new") + assert.Empty(t, client.updateCalls[1].created) + }) +} + +// TestUpgrade_Sequenced_RemovedGroupsDeletedInReverseOrder locks bead 7yi +// (upgrade): resources removed by an upgrade are deleted in exact reverse +// deployment order of the OLD release's plan, with a delete-wait gating each +// batch, not as one unordered bulk delete. +func TestUpgrade_Sequenced_RemovedGroupsDeletedInReverseOrder(t *testing.T) { + client := newRecordingKubeClient() + upgrade := newSequencedUpgradeAction(t, client) + + group := func(name string, deps string) map[string]string { + ann := map[string]string{releaseutil.AnnotationResourceGroup: name} + if deps != "" { + ann[releaseutil.AnnotationDependsOnResourceGroups] = deps + } + return ann + } + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", group("database", "")), + makeConfigMapTemplate("templates/queue.yaml", "queue", group("queue", `["database"]`)), + makeConfigMapTemplate("templates/worker.yaml", "worker", group("worker", `["queue"]`)), + makeConfigMapTemplate("templates/app.yaml", "app", group("app", `["worker"]`)), + }, withName("reverse-removed")) + seedDeployedRelease(t, upgrade, "reverse-removed", currentChart, joinManifestDocs( + configMapManifest("database", group("database", "")), + configMapManifest("queue", group("queue", `["database"]`)), + configMapManifest("worker", group("worker", `["queue"]`)), + configMapManifest("app", group("app", `["worker"]`)), + ), nil) + + upgradedChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", group("database", "")), + makeConfigMapTemplate("templates/app.yaml", "app", group("app", `["database"]`)), + }, withName("reverse-removed")) + + mustRunUpgrade(t, upgrade, "reverse-removed", upgradedChart) + + // Old plan order: database, queue, worker, app. Reverse: app, worker, + // queue, database. Removed set: {queue, worker} -> worker deleted (and + // delete-waited) strictly before queue. + assert.Equal(t, [][]string{{"ConfigMap/worker"}, {"ConfigMap/queue"}}, client.deleteCalls) + assert.Equal(t, [][]string{{"ConfigMap/worker"}, {"ConfigMap/queue"}}, client.deleteWaitCalls) +} + +func TestUpgrade_NonSequencedToSequenced(t *testing.T) { + client := newRecordingKubeClient() + upgrade := newSequencedUpgradeAction(t, client) + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("transition-to-sequenced")) + seedDeployedRelease(t, upgrade, "transition-to-sequenced", currentChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), nil) + + rel := mustRelease(t, mustRunUpgrade(t, upgrade, "transition-to-sequenced", currentChart)) + + require.Len(t, client.updateCalls, 2) + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/app"}}, updateTargets(client.updateCalls)) + assert.True(t, rel.Sequenced) + assert.True(t, rel.IsSequenced()) +} + +func TestUpgrade_SequencedToNonSequenced(t *testing.T) { + client := newRecordingKubeClient() + upgrade := newSequencedUpgradeAction(t, client) + upgrade.WaitStrategy = kube.StatusWatcherStrategy + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("transition-to-standard")) + seedDeployedRelease(t, upgrade, "transition-to-standard", currentChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), &release.SequencingInfo{Enabled: true, Strategy: string(kube.OrderedWaitStrategy)}) + + rel := mustRelease(t, mustRunUpgrade(t, upgrade, "transition-to-standard", currentChart)) + + require.Len(t, client.updateCalls, 1) + assert.ElementsMatch(t, []string{"ConfigMap/database", "ConfigMap/app"}, client.updateCalls[0].target) + require.Len(t, client.waitCalls, 1) + assert.ElementsMatch(t, []string{"ConfigMap/database", "ConfigMap/app"}, client.waitCalls[0]) + assert.Nil(t, rel.SequencingInfo) +} + +func TestUpgrade_Sequenced_FailureRollback(t *testing.T) { + client := newRecordingKubeClient() + client.waitErrorOnCall = 2 + client.waitError = assert.AnError + + upgrade := newSequencedUpgradeAction(t, client) + upgrade.RollbackOnFailure = true + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("failure-rollback")) + seedDeployedRelease(t, upgrade, "failure-rollback", currentChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), nil) + + upgradedChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/queue.yaml", "queue", map[string]string{ + releaseutil.AnnotationResourceGroup: "queue", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["queue"]`, + }), + }, withName("failure-rollback")) + + rel, err := upgrade.Run("failure-rollback", upgradedChart, map[string]any{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "rolled back due to rollback-on-failure") + + failed := mustRelease(t, rel) + assert.Equal(t, rcommon.StatusFailed, failed.Info.Status) + + last, getErr := upgrade.cfg.Releases.Last("failure-rollback") + require.NoError(t, getErr) + assert.Equal(t, rcommon.StatusDeployed, mustRelease(t, last).Info.Status) +} + +func TestUpgrade_Sequenced_SubchartAdded(t *testing.T) { + client := newRecordingKubeClient() + upgrade := newSequencedUpgradeAction(t, client) + + currentParent := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/parent.yaml", "parent", nil), + }, withName("parent")) + currentDatabase := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + }, withName("database")) + currentParent.AddDependency(currentDatabase) + currentParent.Metadata.Dependencies = []*chart.Dependency{ + {Name: "database"}, + } + seedDeployedRelease(t, upgrade, "subchart-added", currentParent, joinManifestDocs( + configMapManifest("database", nil), + configMapManifest("parent", nil), + ), nil) + + upgradedParent := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/parent.yaml", "parent", nil), + }, withName("parent")) + upgradedDatabase := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", nil), + }, withName("database")) + upgradedAPI := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/api.yaml", "api", nil), + }, withName("api")) + upgradedParent.AddDependency(upgradedDatabase, upgradedAPI) + upgradedParent.Metadata.Dependencies = []*chart.Dependency{ + {Name: "database"}, + {Name: "api", DependsOn: []string{"database"}}, + } + + mustRunUpgrade(t, upgrade, "subchart-added", upgradedParent) + + require.Len(t, client.updateCalls, 3) + assert.Equal(t, [][]string{{"ConfigMap/database"}, {"ConfigMap/api"}, {"ConfigMap/parent"}}, updateTargets(client.updateCalls)) + assert.Empty(t, client.updateCalls[1].current) + assert.Equal(t, []string{"ConfigMap/api"}, client.updateCalls[1].created) +} + +func TestUpgrade_Sequenced_DryRun(t *testing.T) { + client := newRecordingKubeClient() + upgrade := newSequencedUpgradeAction(t, client) + upgrade.DryRunStrategy = DryRunClient + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("dry-run")) + seedDeployedRelease(t, upgrade, "dry-run", currentChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), nil) + + rel := mustRelease(t, mustRunUpgrade(t, upgrade, "dry-run", currentChart)) + + assert.Empty(t, client.updateCalls) + assert.Empty(t, client.waitCalls) + assert.Empty(t, client.deleteCalls) + assert.Equal(t, "Dry run complete", rel.Info.Description) +} + +func TestUpgrade_Sequenced_CleanupOnFail(t *testing.T) { + client := newRecordingKubeClient() + client.waitErrorOnCall = 2 + client.waitError = errors.New("timed out waiting for batch") + + upgrade := newSequencedUpgradeAction(t, client) + upgrade.CleanupOnFail = true + + currentChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + }, withName("cleanup-on-fail")) + seedDeployedRelease(t, upgrade, "cleanup-on-fail", currentChart, joinManifestDocs( + configMapManifest("database", map[string]string{releaseutil.AnnotationResourceGroup: "database"}), + configMapManifest("app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ), nil) + + upgradedChart := buildChartWithTemplates([]*common.File{ + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/queue.yaml", "queue", map[string]string{ + releaseutil.AnnotationResourceGroup: "queue", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["queue"]`, + }), + }, withName("cleanup-on-fail")) + + rel, err := upgrade.Run("cleanup-on-fail", upgradedChart, map[string]any{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "timed out waiting for batch") + assert.Equal(t, [][]string{{"ConfigMap/queue"}}, client.deleteCalls) + assert.Equal(t, rcommon.StatusFailed, mustRelease(t, rel).Info.Status) +} + +func mustRunUpgrade(t *testing.T, upgrade *Upgrade, name string, ch *chart.Chart) ri.Releaser { + t.Helper() + rel, err := upgrade.Run(name, ch, map[string]any{}) + require.NoError(t, err) + return rel +} diff --git a/pkg/action/validate.go b/pkg/action/validate.go index 948005521..0d790c274 100644 --- a/pkg/action/validate.go +++ b/pkg/action/validate.go @@ -82,6 +82,13 @@ func existingResourceConflict(resources kube.ResourceList, releaseName, releaseN return err } + if info.Client == nil { + // Fail fast: without a REST client we cannot verify whether the + // resource already exists or is owned by another release, and + // assuming "no conflict" would skip adoption/ownership checks. + return fmt.Errorf("unable to verify whether resource %s already exists: no REST client available", resourceString(info)) + } + helper := resource.NewHelper(info.Client, info.Mapping) existing, err := helper.Get(info.Namespace, info.Name) if err != nil { @@ -130,7 +137,8 @@ func verifyOwnershipBeforeDelete(resources kube.ResourceList, releaseName, relea return err } - // If client is not available, skip verification (test scenario or build failure) + // If client is not available, skip verification (test scenario or build failure). + // Deliberately unchanged by HIP-0025 Stage C: shared with the non-sequenced path; tightening it is tracked separately. if info.Client == nil { infoCopy := *info owned.Append(&infoCopy) diff --git a/pkg/action/validate_test.go b/pkg/action/validate_test.go index 3624d3676..b1a4c9036 100644 --- a/pkg/action/validate_test.go +++ b/pkg/action/validate_test.go @@ -166,6 +166,18 @@ func TestExistingResourceConflict(t *testing.T) { assert.Error(t, err) } +// Regression for PR #32038 review (validate.go nil-client thread): a missing +// REST client must fail fast rather than silently skipping the conflict and +// adoption checks. +func TestExistingResourceConflictNilClientFailsFast(t *testing.T) { + info := newMissingDeployment("clientless", "ns-a") + info.Client = nil + + _, err := existingResourceConflict(kube.ResourceList{info}, "rel-name", "rel-namespace") + assert.Error(t, err) + assert.Contains(t, err.Error(), "no REST client available") +} + func TestCheckOwnership(t *testing.T) { deployFoo := newDeploymentResource("foo", "ns-a", "") diff --git a/pkg/action/warning_system_test.go b/pkg/action/warning_system_test.go new file mode 100644 index 000000000..918524d3d --- /dev/null +++ b/pkg/action/warning_system_test.go @@ -0,0 +1,109 @@ +/* +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 action + +import ( + "bytes" + "log/slog" + "testing" + + "github.com/stretchr/testify/assert" + + "helm.sh/helm/v4/pkg/chart/common" + "helm.sh/helm/v4/pkg/kube" + releaseutil "helm.sh/helm/v4/pkg/release/v1/util" +) + +func captureWarningOutput(t *testing.T) *bytes.Buffer { + t.Helper() + + var buf bytes.Buffer + handler := slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn}) + oldLogger := slog.Default() + slog.SetDefault(slog.New(handler)) + t.Cleanup(func() { slog.SetDefault(oldLogger) }) + + return &buf +} + +func runSequencedInstallWithWarnings(t *testing.T, templates ...*common.File) string { + t.Helper() + + client := newRecordingKubeClient() + install := newSequencedInstallAction(t, client) + + var buf bytes.Buffer + handler := slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn}) + install.cfg.SetLogger(handler) + + mustRunInstall(t, install, buildChartWithTemplates(templates)) + + return buf.String() +} + +func TestWarning_PartialReadinessAnnotations(t *testing.T) { + output := runSequencedInstallWithWarnings(t, + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + kube.AnnotationReadinessSuccess: `["{.ready} == true"]`, + }), + ) + + assert.Contains(t, output, "sequencing: resource \\\"app\\\" has only one of") + assert.Contains(t, output, releaseutil.AnnotationReadinessSuccess) + assert.Contains(t, output, releaseutil.AnnotationReadinessFailure) +} + +func TestWarning_IsolatedResourceGroups(t *testing.T) { + output := runSequencedInstallWithWarnings(t, + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + }), + makeConfigMapTemplate("templates/cache.yaml", "cache", map[string]string{ + releaseutil.AnnotationResourceGroup: "cache", + }), + ) + + assert.Contains(t, output, "resource-group \\\"database\\\" is isolated") + assert.Contains(t, output, "resource-group \\\"cache\\\" is isolated") +} + +func TestWarning_NonExistentGroupReference(t *testing.T) { + output := runSequencedInstallWithWarnings(t, + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["missing"]`, + }), + ) + + assert.Contains(t, output, "sequencing: group \\\"app\\\" depends-on non-existent group \\\"missing\\\"") +} + +func TestWarning_WellFormedSequencing(t *testing.T) { + output := runSequencedInstallWithWarnings(t, + makeConfigMapTemplate("templates/database.yaml", "database", map[string]string{ + releaseutil.AnnotationResourceGroup: "database", + kube.AnnotationReadinessSuccess: `["{.ready} == true"]`, + kube.AnnotationReadinessFailure: `["{.failed} == true"]`, + }), + makeConfigMapTemplate("templates/app.yaml", "app", map[string]string{ + releaseutil.AnnotationResourceGroup: "app", + releaseutil.AnnotationDependsOnResourceGroups: `["database"]`, + }), + ) + + assert.Empty(t, output) +} diff --git a/pkg/chart/v2/lint/rules/sequencing.go b/pkg/chart/v2/lint/rules/sequencing.go index e24f1e6a9..74235c399 100644 --- a/pkg/chart/v2/lint/rules/sequencing.go +++ b/pkg/chart/v2/lint/rules/sequencing.go @@ -17,6 +17,7 @@ limitations under the License. package rules // import "helm.sh/helm/v4/pkg/chart/v2/lint/rules" import ( + "context" "fmt" "path" "strings" @@ -102,7 +103,7 @@ func validateRenderedSequencingAnnotations(linter *support.Linter, c *chart.Char var renderEngine engine.Engine renderEngine.LintMode = true - renderedContentMap, err := renderEngine.Render(c, valuesToRender) + renderedContentMap, err := renderEngine.RenderWithContext(context.Background(), c, valuesToRender) if err != nil { // Template rendering errors are already reported by the Templates lint rule. return diff --git a/pkg/release/v1/util/manifest.go b/pkg/release/v1/util/manifest.go index 31babdbb5..e6d90bbd7 100644 --- a/pkg/release/v1/util/manifest.go +++ b/pkg/release/v1/util/manifest.go @@ -83,15 +83,15 @@ 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: +// 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 lazily -// from HelmInternalSequencingAnnotations so that adding a new helm-internal +// 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 := 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?`) diff --git a/pkg/release/v1/util/resource_group.go b/pkg/release/v1/util/resource_group.go index e5d96ccbf..f65674331 100644 --- a/pkg/release/v1/util/resource_group.go +++ b/pkg/release/v1/util/resource_group.go @@ -19,6 +19,7 @@ package util import ( "encoding/json" "fmt" + "slices" chartutil "helm.sh/helm/v4/pkg/chart/v2/util" ) @@ -48,16 +49,29 @@ const ( AnnotationReadinessFailure = "helm.sh/readiness-failure" ) -// HelmInternalSequencingAnnotations lists annotation keys used by Helm for +// helmInternalSequencingAnnotations lists annotation keys used by Helm for // resource sequencing that are NOT valid Kubernetes annotation keys (their // names contain multiple `/` separators). Helm strips these before applying // resources to the API server, and before printing manifests via // `helm template` so downstream tooling like `kubectl apply` accepts the // output unmodified. -var HelmInternalSequencingAnnotations = []string{ +// +// It is unexported, and the regex in manifest.go is compiled from it once at +// init. External callers read it through the HelmInternalSequencingAnnotations +// accessor (which returns a copy) so the canonical list — and the regex derived +// from it — cannot be mutated, avoiding the silent-no-op and data-race footguns +// of an exported mutable slice. +var helmInternalSequencingAnnotations = []string{ AnnotationDependsOnResourceGroups, } +// HelmInternalSequencingAnnotations returns a copy of the helm-internal +// sequencing annotation keys that Helm strips from rendered output. A copy is +// returned so callers cannot mutate the canonical list. +func HelmInternalSequencingAnnotations() []string { + return slices.Clone(helmInternalSequencingAnnotations) +} + // ResourceGroupResult holds the output of ParseResourceGroups. type ResourceGroupResult struct { // Groups maps group name to manifests assigned to that group.