diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 9d0bb390b..37e134c48 100644 --- a/pkg/action/hooks.go +++ b/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 { diff --git a/pkg/action/hooks_test.go b/pkg/action/hooks_test.go index 38f25d9ab..68379add8 100644 --- a/pkg/action/hooks_test.go +++ b/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.") + } + }) + } +}