From 02996b230d65bbf5f920ace0b72548d6b9afedcc Mon Sep 17 00:00:00 2001 From: Lucas Heinlen Date: Wed, 24 Feb 2021 10:48:08 -0500 Subject: [PATCH 1/5] Add basic tests for ReleaseTesting action Signed-off-by: Lucas Heinlen --- pkg/action/release_testing_test.go | 243 +++++++++++++++++++++++++++++ 1 file changed, 243 insertions(+) create mode 100644 pkg/action/release_testing_test.go diff --git a/pkg/action/release_testing_test.go b/pkg/action/release_testing_test.go new file mode 100644 index 000000000..fe50ea867 --- /dev/null +++ b/pkg/action/release_testing_test.go @@ -0,0 +1,243 @@ +/* +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/v3/pkg/kube" + "helm.sh/helm/v3/pkg/release" +) + +func releaseTestingAction(t *testing.T) *ReleaseTesting { + config := actionConfigFixture(t) + rtAction := NewReleaseTesting(config) + rtAction.Namespace = "spaced" + return rtAction +} + +func TestReleaseTesting_NoFilters(t *testing.T) { + is := assert.New(t) + req := require.New(t) + + rtAction := releaseTestingAction(t) + rel := rtReleasStub() + rtAction.cfg.Releases.Create(rel) + + res, err := rtAction.Run(rel.Name) + req.NoError(err) + + // The filtering implementation will not necessarily maintain the hook order so we must find the hooks by name after verifying the length + is.Len(res.Hooks, 3) + for _, hook := range res.Hooks { + switch hook.Name { + case "test-cm": + is.Empty(hook.LastRun.Phase) + case "finding-nemo": + is.Equal(release.HookPhaseSucceeded, hook.LastRun.Phase) + case "finding-dory": + is.Equal(release.HookPhaseSucceeded, hook.LastRun.Phase) + default: + is.Fail("Unexpected hook: " + hook.Name) + } + } +} + +func TestReleaseTesting_PostiveFilter(t *testing.T) { + is := assert.New(t) + req := require.New(t) + + rtAction := releaseTestingAction(t) + rel := rtReleasStub() + rtAction.cfg.Releases.Create(rel) + rtAction.Filters = map[string][]string{"name": {"finding-dory"}} + + res, err := rtAction.Run(rel.Name) + req.NoError(err) + + // The filtering implementation will not necessarily maintain the hook order so we must find the hooks by name after verifying the length + is.Len(res.Hooks, 3) + for _, hook := range res.Hooks { + switch hook.Name { + case "test-cm": + is.Empty(hook.LastRun.Phase) + case "finding-nemo": + is.Empty(hook.LastRun.Phase) + case "finding-dory": + is.Equal(release.HookPhaseSucceeded, hook.LastRun.Phase) + default: + is.Fail("Unexpected hook: " + hook.Name) + } + } +} + +func TestReleaseTesting_NegativeFilter(t *testing.T) { + is := assert.New(t) + req := require.New(t) + + rtAction := releaseTestingAction(t) + rel := rtReleasStub() + rtAction.cfg.Releases.Create(rel) + rtAction.Filters = map[string][]string{"!name": {"finding-nemo"}} + + res, err := rtAction.Run(rel.Name) + req.NoError(err) + + // The filtering implementation will not necessarily maintain the hook order so we must find the hooks by name after verifying the length + is.Len(res.Hooks, 3) + for _, hook := range res.Hooks { + switch hook.Name { + case "test-cm": + is.Empty(hook.LastRun.Phase) + case "finding-nemo": + is.Empty(hook.LastRun.Phase) + case "finding-dory": + is.Equal(release.HookPhaseSucceeded, hook.LastRun.Phase) + default: + is.Fail("Unexpected hook: " + hook.Name) + } + } +} + +func TestReleaseTesting_BothFilters(t *testing.T) { + is := assert.New(t) + req := require.New(t) + + rtAction := releaseTestingAction(t) + rel := rtReleasStub() + rtAction.cfg.Releases.Create(rel) + rtAction.Filters = map[string][]string{ + "!name": {"finding-nemo"}, + "name": {"finding-dory"}, + } + + res, err := rtAction.Run(rel.Name) + req.NoError(err) + + // The filtering implementation will not necessarily maintain the hook order so we must find the hooks by name after verifying the length + is.Len(res.Hooks, 3) + for _, hook := range res.Hooks { + switch hook.Name { + case "test-cm": + is.Empty(hook.LastRun.Phase) + case "finding-nemo": + is.Empty(hook.LastRun.Phase) + case "finding-dory": + is.Equal(release.HookPhaseSucceeded, hook.LastRun.Phase) + default: + is.Fail("Unexpected hook: " + hook.Name) + } + } +} + +func TestReleaseTesting_ConflictingFilters(t *testing.T) { + is := assert.New(t) + req := require.New(t) + + rtAction := releaseTestingAction(t) + rel := rtReleasStub() + rtAction.cfg.Releases.Create(rel) + rtAction.Filters = map[string][]string{ + "!name": {"finding-nemo"}, + "name": {"finding-nemo"}, + } + + res, err := rtAction.Run(rel.Name) + req.NoError(err) + + // The filtering implementation will not necessarily maintain the hook order so we must find the hooks by name after verifying the length + is.Len(res.Hooks, 3) + for _, hook := range res.Hooks { + switch hook.Name { + case "test-cm": + is.Empty(hook.LastRun.Phase) + case "finding-nemo": + is.Empty(hook.LastRun.Phase) + case "finding-dory": + is.Empty(hook.LastRun.Phase) + default: + is.Fail("Unexpected hook: " + hook.Name) + } + } +} + +// TestReleaseTesting_CrashedWhileFiltering verifies that https://github.com/helm/helm/issues/9398 has been corrected +// To accomplish this, it uses a fake kube client which will panic on the Create call that occurs after the initial update +// of the release. Prior to the fix, this would have lost any helm hooks that did not pass the filter. +// Due to having to crash at a specific point in the code being tested, this test is somewhat fragile and could be broken +// by changes to the underlying implementation. +func TestReleaseTesting_CrashedWhileFiltering(t *testing.T) { + is := assert.New(t) + req := require.New(t) + + rtAction := releaseTestingAction(t) + rtAction.cfg.KubeClient = &panickingKubeClient{Interface: rtAction.cfg.KubeClient} + rel := rtReleasStub() + rtAction.cfg.Releases.Create(rel) + rtAction.Filters = map[string][]string{"name": {"finding-dory"}} + + defer func() { + if r := recover(); r != nil { + // Retrieving the release from storage to show that it was actually altered there and not just in memory + res, err := rtAction.cfg.Releases.Get(rel.Name, 1) + req.NoError(err) + + // The filtering implementation will not necessarily maintain the hook order so we must find the hooks by name after verifying the length + is.Len(res.Hooks, 3) + for _, hook := range res.Hooks { + switch hook.Name { + case "test-cm": + is.Empty(hook.LastRun.Phase) + case "finding-nemo": + is.Empty(hook.LastRun.Phase) + case "finding-dory": + is.Equal(release.HookPhaseUnknown, hook.LastRun.Phase) + default: + is.Fail("Unexpected hook: " + hook.Name) + } + } + } else { + is.Fail("Did not panic") + } + }() + + // this will panic so the return values aren't helpful + _, _ = rtAction.Run(rel.Name) +} + +func rtReleasStub() *release.Release { + stub := releaseStub() + stub.Hooks = append(stub.Hooks, &release.Hook{ + Name: "finding-dory", + Kind: "Pod", + Path: "finding-dory", + Manifest: manifestWithTestHook, + Events: []release.HookEvent{ + release.HookTest, + }, + }) + return stub +} + +type panickingKubeClient struct{ kube.Interface } + +func (f *panickingKubeClient) Create(resources kube.ResourceList) (*kube.Result, error) { + panic("yikes") +} From 15701c6736834d9d79299f801d13f28228d1b9fc Mon Sep 17 00:00:00 2001 From: Lucas Heinlen Date: Tue, 23 Feb 2021 14:39:27 -0500 Subject: [PATCH 2/5] Retain hooks when a filtered helm test is interrupted Signed-off-by: Lucas Heinlen --- pkg/action/hooks.go | 8 +++++--- pkg/action/release_testing.go | 17 +++-------------- pkg/release/hook.go | 2 ++ 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 40c1ffdb6..47c76bda3 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -31,9 +31,11 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, executingHooks := []*release.Hook{} for _, h := range rl.Hooks { - for _, e := range h.Events { - if e == hook { - executingHooks = append(executingHooks, h) + if !h.ExecutionDisabled { + for _, e := range h.Events { + if e == hook { + executingHooks = append(executingHooks, h) + } } } } diff --git a/pkg/action/release_testing.go b/pkg/action/release_testing.go index ecaeaf59f..ca0d9ced7 100644 --- a/pkg/action/release_testing.go +++ b/pkg/action/release_testing.go @@ -64,37 +64,26 @@ func (r *ReleaseTesting) Run(name string) (*release.Release, error) { return rel, err } - skippedHooks := []*release.Hook{} - executingHooks := []*release.Hook{} if len(r.Filters["!name"]) != 0 { for _, h := range rel.Hooks { if contains(r.Filters["!name"], h.Name) { - skippedHooks = append(skippedHooks, h) - } else { - executingHooks = append(executingHooks, h) + h.ExecutionDisabled = true } } - rel.Hooks = executingHooks } if len(r.Filters["name"]) != 0 { - executingHooks = nil for _, h := range rel.Hooks { - if contains(r.Filters["name"], h.Name) { - executingHooks = append(executingHooks, h) - } else { - skippedHooks = append(skippedHooks, h) + if !contains(r.Filters["name"], h.Name) { + h.ExecutionDisabled = true } } - rel.Hooks = executingHooks } if err := r.cfg.execHook(rel, release.HookTest, r.Timeout); err != nil { - rel.Hooks = append(skippedHooks, rel.Hooks...) r.cfg.Releases.Update(rel) return rel, err } - rel.Hooks = append(skippedHooks, rel.Hooks...) return rel, r.cfg.Releases.Update(rel) } diff --git a/pkg/release/hook.go b/pkg/release/hook.go index 662320f06..497c12cee 100644 --- a/pkg/release/hook.go +++ b/pkg/release/hook.go @@ -76,6 +76,8 @@ type Hook struct { Weight int `json:"weight,omitempty"` // DeletePolicies are the policies that indicate when to delete the hook DeletePolicies []HookDeletePolicy `json:"delete_policies,omitempty"` + // ExecutionDisabled is set to true when a hook has been filtered out and should not be executed + ExecutionDisabled bool `json:"-"` } // A HookExecution records the result for the last execution of a hook for a given release. From 5f5549abaa50d7ad5b21ca2bdabc610ef62060f8 Mon Sep 17 00:00:00 2001 From: Lucas Heinlen Date: Thu, 25 Feb 2021 09:35:56 -0500 Subject: [PATCH 3/5] Revert previous implementation Signed-off-by: Lucas Heinlen --- pkg/action/hooks.go | 8 +++----- pkg/action/release_testing.go | 17 ++++++++++++++--- pkg/release/hook.go | 2 -- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 47c76bda3..40c1ffdb6 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -31,11 +31,9 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, executingHooks := []*release.Hook{} for _, h := range rl.Hooks { - if !h.ExecutionDisabled { - for _, e := range h.Events { - if e == hook { - executingHooks = append(executingHooks, h) - } + for _, e := range h.Events { + if e == hook { + executingHooks = append(executingHooks, h) } } } diff --git a/pkg/action/release_testing.go b/pkg/action/release_testing.go index ca0d9ced7..ecaeaf59f 100644 --- a/pkg/action/release_testing.go +++ b/pkg/action/release_testing.go @@ -64,26 +64,37 @@ func (r *ReleaseTesting) Run(name string) (*release.Release, error) { return rel, err } + skippedHooks := []*release.Hook{} + executingHooks := []*release.Hook{} if len(r.Filters["!name"]) != 0 { for _, h := range rel.Hooks { if contains(r.Filters["!name"], h.Name) { - h.ExecutionDisabled = true + skippedHooks = append(skippedHooks, h) + } else { + executingHooks = append(executingHooks, h) } } + rel.Hooks = executingHooks } if len(r.Filters["name"]) != 0 { + executingHooks = nil for _, h := range rel.Hooks { - if !contains(r.Filters["name"], h.Name) { - h.ExecutionDisabled = true + if contains(r.Filters["name"], h.Name) { + executingHooks = append(executingHooks, h) + } else { + skippedHooks = append(skippedHooks, h) } } + rel.Hooks = executingHooks } if err := r.cfg.execHook(rel, release.HookTest, r.Timeout); err != nil { + rel.Hooks = append(skippedHooks, rel.Hooks...) r.cfg.Releases.Update(rel) return rel, err } + rel.Hooks = append(skippedHooks, rel.Hooks...) return rel, r.cfg.Releases.Update(rel) } diff --git a/pkg/release/hook.go b/pkg/release/hook.go index 497c12cee..662320f06 100644 --- a/pkg/release/hook.go +++ b/pkg/release/hook.go @@ -76,8 +76,6 @@ type Hook struct { Weight int `json:"weight,omitempty"` // DeletePolicies are the policies that indicate when to delete the hook DeletePolicies []HookDeletePolicy `json:"delete_policies,omitempty"` - // ExecutionDisabled is set to true when a hook has been filtered out and should not be executed - ExecutionDisabled bool `json:"-"` } // A HookExecution records the result for the last execution of a hook for a given release. From 129f04cf07df10b06a74715119fde5bb2a630f04 Mon Sep 17 00:00:00 2001 From: Lucas Heinlen Date: Thu, 25 Feb 2021 13:39:05 -0500 Subject: [PATCH 4/5] Retain hooks when a filtered helm test is interrupted Signed-off-by: Lucas Heinlen --- pkg/action/hooks.go | 7 ++++-- pkg/action/install.go | 4 ++-- pkg/action/release_testing.go | 38 +++++++++++++----------------- pkg/action/release_testing_test.go | 6 ++--- pkg/action/rollback.go | 4 ++-- pkg/action/uninstall.go | 4 ++-- pkg/action/upgrade.go | 4 ++-- 7 files changed, 32 insertions(+), 35 deletions(-) diff --git a/pkg/action/hooks.go b/pkg/action/hooks.go index 40c1ffdb6..a22712eee 100644 --- a/pkg/action/hooks.go +++ b/pkg/action/hooks.go @@ -27,10 +27,13 @@ import ( ) // execHook executes all of the hooks for the given hook event. -func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, timeout time.Duration) error { +func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent, activeHooks []*release.Hook, timeout time.Duration) error { executingHooks := []*release.Hook{} + if activeHooks == nil { + activeHooks = rl.Hooks + } - for _, h := range rl.Hooks { + for _, h := range activeHooks { for _, e := range h.Events { if e == hook { executingHooks = append(executingHooks, h) diff --git a/pkg/action/install.go b/pkg/action/install.go index 4de0b64e6..ef1927701 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -327,7 +327,7 @@ func (i *Install) Run(chrt *chart.Chart, vals map[string]interface{}) (*release. // pre-install hooks if !i.DisableHooks { - if err := i.cfg.execHook(rel, release.HookPreInstall, i.Timeout); err != nil { + if err := i.cfg.execHook(rel, release.HookPreInstall, nil, i.Timeout); err != nil { return i.failRelease(rel, fmt.Errorf("failed pre-install: %s", err)) } } @@ -358,7 +358,7 @@ func (i *Install) Run(chrt *chart.Chart, vals map[string]interface{}) (*release. } if !i.DisableHooks { - if err := i.cfg.execHook(rel, release.HookPostInstall, i.Timeout); err != nil { + if err := i.cfg.execHook(rel, release.HookPostInstall, nil, i.Timeout); err != nil { return i.failRelease(rel, fmt.Errorf("failed post-install: %s", err)) } } diff --git a/pkg/action/release_testing.go b/pkg/action/release_testing.go index ecaeaf59f..62e4e7791 100644 --- a/pkg/action/release_testing.go +++ b/pkg/action/release_testing.go @@ -64,37 +64,21 @@ func (r *ReleaseTesting) Run(name string) (*release.Release, error) { return rel, err } - skippedHooks := []*release.Hook{} - executingHooks := []*release.Hook{} - if len(r.Filters["!name"]) != 0 { + var activeHooks []*release.Hook + if len(r.Filters["!name"]) != 0 || len(r.Filters["name"]) != 0 { + activeHooks = []*release.Hook{} for _, h := range rel.Hooks { - if contains(r.Filters["!name"], h.Name) { - skippedHooks = append(skippedHooks, h) - } else { - executingHooks = append(executingHooks, h) + if r.passesFilters(h.Name) { + activeHooks = append(activeHooks, h) } } - rel.Hooks = executingHooks - } - if len(r.Filters["name"]) != 0 { - executingHooks = nil - for _, h := range rel.Hooks { - if contains(r.Filters["name"], h.Name) { - executingHooks = append(executingHooks, h) - } else { - skippedHooks = append(skippedHooks, h) - } - } - rel.Hooks = executingHooks } - if err := r.cfg.execHook(rel, release.HookTest, r.Timeout); err != nil { - rel.Hooks = append(skippedHooks, rel.Hooks...) + if err := r.cfg.execHook(rel, release.HookTest, activeHooks, r.Timeout); err != nil { r.cfg.Releases.Update(rel) return rel, err } - rel.Hooks = append(skippedHooks, rel.Hooks...) return rel, r.cfg.Releases.Update(rel) } @@ -128,6 +112,16 @@ func (r *ReleaseTesting) GetPodLogs(out io.Writer, rel *release.Release) error { return nil } +func (r *ReleaseTesting) passesFilters(name string) bool { + if len(r.Filters["!name"]) != 0 && contains(r.Filters["!name"], name) { + return false + } else if len(r.Filters["name"]) != 0 { + return contains(r.Filters["name"], name) + } + + return true +} + func contains(arr []string, value string) bool { for _, item := range arr { if item == value { diff --git a/pkg/action/release_testing_test.go b/pkg/action/release_testing_test.go index fe50ea867..db96934e0 100644 --- a/pkg/action/release_testing_test.go +++ b/pkg/action/release_testing_test.go @@ -167,11 +167,11 @@ func TestReleaseTesting_ConflictingFilters(t *testing.T) { for _, hook := range res.Hooks { switch hook.Name { case "test-cm": - is.Empty(hook.LastRun.Phase) + is.Empty(hook.LastRun.Phase, hook.Name) case "finding-nemo": - is.Empty(hook.LastRun.Phase) + is.Empty(hook.LastRun.Phase, hook.Name) case "finding-dory": - is.Empty(hook.LastRun.Phase) + is.Empty(hook.LastRun.Phase, hook.Name) default: is.Fail("Unexpected hook: " + hook.Name) } diff --git a/pkg/action/rollback.go b/pkg/action/rollback.go index f3f958f3d..f66ef85f9 100644 --- a/pkg/action/rollback.go +++ b/pkg/action/rollback.go @@ -157,7 +157,7 @@ func (r *Rollback) performRollback(currentRelease, targetRelease *release.Releas // pre-rollback hooks if !r.DisableHooks { - if err := r.cfg.execHook(targetRelease, release.HookPreRollback, r.Timeout); err != nil { + if err := r.cfg.execHook(targetRelease, release.HookPreRollback, nil, r.Timeout); err != nil { return targetRelease, err } } else { @@ -219,7 +219,7 @@ func (r *Rollback) performRollback(currentRelease, targetRelease *release.Releas // post-rollback hooks if !r.DisableHooks { - if err := r.cfg.execHook(targetRelease, release.HookPostRollback, r.Timeout); err != nil { + if err := r.cfg.execHook(targetRelease, release.HookPostRollback, nil, r.Timeout); err != nil { return targetRelease, err } } diff --git a/pkg/action/uninstall.go b/pkg/action/uninstall.go index c466c6ee2..ba0e24e2f 100644 --- a/pkg/action/uninstall.go +++ b/pkg/action/uninstall.go @@ -97,7 +97,7 @@ func (u *Uninstall) Run(name string) (*release.UninstallReleaseResponse, error) res := &release.UninstallReleaseResponse{Release: rel} if !u.DisableHooks { - if err := u.cfg.execHook(rel, release.HookPreDelete, u.Timeout); err != nil { + if err := u.cfg.execHook(rel, release.HookPreDelete, nil, u.Timeout); err != nil { return res, err } } else { @@ -114,7 +114,7 @@ func (u *Uninstall) Run(name string) (*release.UninstallReleaseResponse, error) res.Info = kept if !u.DisableHooks { - if err := u.cfg.execHook(rel, release.HookPostDelete, u.Timeout); err != nil { + if err := u.cfg.execHook(rel, release.HookPostDelete, nil, u.Timeout); err != nil { errs = append(errs, err) } } diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index b0f294cae..56feb5e9b 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -307,7 +307,7 @@ func (u *Upgrade) performUpgrade(originalRelease, upgradedRelease *release.Relea // pre-upgrade hooks if !u.DisableHooks { - if err := u.cfg.execHook(upgradedRelease, release.HookPreUpgrade, u.Timeout); err != nil { + if err := u.cfg.execHook(upgradedRelease, release.HookPreUpgrade, nil, u.Timeout); err != nil { return u.failRelease(upgradedRelease, kube.ResourceList{}, fmt.Errorf("pre-upgrade hooks failed: %s", err)) } } else { @@ -346,7 +346,7 @@ func (u *Upgrade) performUpgrade(originalRelease, upgradedRelease *release.Relea // post-upgrade hooks if !u.DisableHooks { - if err := u.cfg.execHook(upgradedRelease, release.HookPostUpgrade, u.Timeout); err != nil { + if err := u.cfg.execHook(upgradedRelease, release.HookPostUpgrade, nil, u.Timeout); err != nil { return u.failRelease(upgradedRelease, results.Created, fmt.Errorf("post-upgrade hooks failed: %s", err)) } } From 4a3faf8c133c8e08b18cc0e6646e2b81a94bc5cf Mon Sep 17 00:00:00 2001 From: Lucas Heinlen Date: Mon, 20 Dec 2021 16:47:07 -0500 Subject: [PATCH 5/5] Fix spelling error Signed-off-by: Lucas Heinlen --- pkg/action/release_testing_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/action/release_testing_test.go b/pkg/action/release_testing_test.go index db96934e0..ae63d786c 100644 --- a/pkg/action/release_testing_test.go +++ b/pkg/action/release_testing_test.go @@ -60,7 +60,7 @@ func TestReleaseTesting_NoFilters(t *testing.T) { } } -func TestReleaseTesting_PostiveFilter(t *testing.T) { +func TestReleaseTesting_PositiveFilter(t *testing.T) { is := assert.New(t) req := require.New(t)