Merge pull request #30673 from nvanthao/process-all-hook-deletions-on-failure

fix: Process all hook deletions on failure

pkg/action/hooks.go

@@ -49,7 +49,7 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent,
 	// hooke are pre-ordered by kind, so keep order stable
 	sort.Stable(hookByWeight(executingHooks))
 
-	for _, h := range executingHooks {
+	for i, h := range executingHooks {
 		// Set default delete policy to before-hook-creation
 		if len(h.DeletePolicies) == 0 {
 			// TODO(jlegrone): Only apply before-hook-creation delete policy to run to completion
@@ -109,6 +109,13 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent,
 				// We log the error here as we want to propagate the hook failure upwards to the release object.
 				log.Printf("error deleting the hook resource on hook failure: %v", errDeleting)
 			}
+
+			// If a hook is failed, check the annotation of the previous successful hooks to determine whether the hooks
+			// should be deleted under succeeded condition.
+			if err := cfg.deleteHooksByPolicy(executingHooks[0:i], release.HookSucceeded, timeout); err != nil {
+				return err
+			}
+
 			return err
 		}
 		h.LastRun.Phase = release.HookPhaseSucceeded
@@ -170,6 +177,17 @@ func (cfg *Configuration) deleteHookByPolicy(h *release.Hook, policy release.Hoo
 	return nil
 }
 
+// deleteHooksByPolicy deletes all hooks if the hook policy instructs it to
+func (cfg *Configuration) deleteHooksByPolicy(hooks []*release.Hook, policy release.HookDeletePolicy, timeout time.Duration) error {
+	for _, h := range hooks {
+		if err := cfg.deleteHookByPolicy(h, policy, timeout); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
 // hookHasDeletePolicy determines whether the defined hook deletion policy matches the hook deletion polices
 // supported by helm. If so, mark the hook as one should be deleted.
 func hookHasDeletePolicy(h *release.Hook, policy release.HookDeletePolicy) bool {

pkg/action/hooks_test.go

@@ -20,13 +20,22 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"reflect"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
+	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/yaml"
+	"k8s.io/cli-runtime/pkg/resource"
 
 	chart "helm.sh/helm/v4/pkg/chart/v2"
+	chartutil "helm.sh/helm/v4/pkg/chart/v2/util"
+	"helm.sh/helm/v4/pkg/kube"
 	kubefake "helm.sh/helm/v4/pkg/kube/fake"
 	release "helm.sh/helm/v4/pkg/release/v1"
+	"helm.sh/helm/v4/pkg/storage"
+	"helm.sh/helm/v4/pkg/storage/driver"
 )
 
 func podManifestWithOutputLogs(hookDefinitions []release.HookOutputLogPolicy) string {
@@ -206,3 +215,181 @@ func runInstallForHooksWithFailure(t *testing.T, manifest, expectedNamespace str
 	is.Equal(expectedOutput, outBuffer.String())
 	is.Equal(release.StatusFailed, res.Info.Status)
 }
+
+type HookFailedError struct{}
+
+func (e *HookFailedError) Error() string {
+	return "Hook failed!"
+}
+
+type HookFailingKubeClient struct {
+	kubefake.PrintingKubeClient
+	failOn       resource.Info
+	deleteRecord []resource.Info
+}
+
+func (*HookFailingKubeClient) Build(reader io.Reader, _ bool) (kube.ResourceList, error) {
+	configMap := &v1.ConfigMap{}
+
+	err := yaml.NewYAMLOrJSONDecoder(reader, 1000).Decode(configMap)
+
+	if err != nil {
+		return kube.ResourceList{}, err
+	}
+
+	return kube.ResourceList{{
+		Name:      configMap.Name,
+		Namespace: configMap.Namespace,
+	}}, nil
+}
+
+func (h *HookFailingKubeClient) WatchUntilReady(resources kube.ResourceList, duration time.Duration) error {
+	for _, res := range resources {
+		if res.Name == h.failOn.Name && res.Namespace == h.failOn.Namespace {
+			return &HookFailedError{}
+		}
+	}
+
+	return h.PrintingKubeClient.WatchUntilReady(resources, duration)
+}
+
+func (h *HookFailingKubeClient) Delete(resources kube.ResourceList) (*kube.Result, []error) {
+	for _, res := range resources {
+		h.deleteRecord = append(h.deleteRecord, resource.Info{
+			Name:      res.Name,
+			Namespace: res.Namespace,
+		})
+	}
+
+	return h.PrintingKubeClient.Delete(resources)
+}
+
+func TestHooksCleanUp(t *testing.T) {
+	hookEvent := release.HookPreInstall
+
+	testCases := []struct {
+		name                 string
+		inputRelease         release.Release
+		failOn               resource.Info
+		expectedDeleteRecord []resource.Info
+		expectError          bool
+	}{
+		{
+			"Deletion hook runs for previously successful hook on failure of a heavier weight hook",
+			release.Release{
+				Name:      "test-release",
+				Namespace: "test",
+				Hooks: []*release.Hook{
+					{
+						Name: "hook-1",
+						Kind: "ConfigMap",
+						Path: "templates/service_account.yaml",
+						Manifest: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: build-config-1
+  namespace: test
+data:
+  foo: bar
+`,
+						Weight: -5,
+						Events: []release.HookEvent{
+							hookEvent,
+						},
+						DeletePolicies: []release.HookDeletePolicy{
+							release.HookBeforeHookCreation,
+							release.HookSucceeded,
+							release.HookFailed,
+						},
+						LastRun: release.HookExecution{
+							Phase: release.HookPhaseSucceeded,
+						},
+					},
+					{
+						Name: "hook-2",
+						Kind: "ConfigMap",
+						Path: "templates/job.yaml",
+						Manifest: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: build-config-2
+  namespace: test
+data:
+  foo: bar
+`,
+						Weight: 0,
+						Events: []release.HookEvent{
+							hookEvent,
+						},
+						DeletePolicies: []release.HookDeletePolicy{
+							release.HookBeforeHookCreation,
+							release.HookSucceeded,
+							release.HookFailed,
+						},
+						LastRun: release.HookExecution{
+							Phase: release.HookPhaseFailed,
+						},
+					},
+				},
+			}, resource.Info{
+				Name:      "build-config-2",
+				Namespace: "test",
+			}, []resource.Info{
+				{
+					// This should be in the record for `before-hook-creation`
+					Name:      "build-config-1",
+					Namespace: "test",
+				},
+				{
+					// This should be in the record for `before-hook-creation`
+					Name:      "build-config-2",
+					Namespace: "test",
+				},
+				{
+					// This should be in the record for cleaning up (the failure first)
+					Name:      "build-config-2",
+					Namespace: "test",
+				},
+				{
+					// This should be in the record for cleaning up (then the previously successful)
+					Name:      "build-config-1",
+					Namespace: "test",
+				},
+			}, true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			kubeClient := &HookFailingKubeClient{
+				kubefake.PrintingKubeClient{Out: io.Discard}, tc.failOn, []resource.Info{},
+			}
+
+			configuration := &Configuration{
+				Releases:     storage.Init(driver.NewMemory()),
+				KubeClient:   kubeClient,
+				Capabilities: chartutil.DefaultCapabilities,
+				Log: func(format string, v ...interface{}) {
+					t.Helper()
+					if *verbose {
+						t.Logf(format, v...)
+					}
+				},
+			}
+
+			err := configuration.execHook(&tc.inputRelease, hookEvent, 600)
+
+			if !reflect.DeepEqual(kubeClient.deleteRecord, tc.expectedDeleteRecord) {
+				t.Fatalf("Got unexpected delete record, expected: %#v, but got: %#v", kubeClient.deleteRecord, tc.expectedDeleteRecord)
+			}
+
+			if err != nil && !tc.expectError {
+				t.Fatalf("Got an unexpected error.")
+			}
+
+			if err == nil && tc.expectError {
+				t.Fatalf("Expected and error but did not get it.")
+			}
+		})
+	}
+}