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 11fdc4374..06e3d73c0 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -406,7 +406,7 @@ func (i *Install) performInstall(c chan<- resultMessage, rel *release.Release, t // 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 { i.reportToRun(c, rel, fmt.Errorf("failed pre-install: %s", err)) return } @@ -442,7 +442,7 @@ func (i *Install) performInstall(c chan<- resultMessage, rel *release.Release, t } 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 { i.reportToRun(c, rel, fmt.Errorf("failed post-install: %s", err)) return } 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 new file mode 100644 index 000000000..ae63d786c --- /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_PositiveFilter(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, hook.Name) + case "finding-nemo": + is.Empty(hook.LastRun.Phase, hook.Name) + case "finding-dory": + is.Empty(hook.LastRun.Phase, hook.Name) + 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") +} diff --git a/pkg/action/rollback.go b/pkg/action/rollback.go index dda8c700b..e6e0b38d8 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 { @@ -224,7 +224,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 801498544..d897308d8 100644 --- a/pkg/action/uninstall.go +++ b/pkg/action/uninstall.go @@ -101,7 +101,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 { @@ -134,7 +134,7 @@ func (u *Uninstall) Run(name string) (*release.UninstallReleaseResponse, error) } 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 ebe3dd2ee..38ac9fcd7 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -391,7 +391,7 @@ func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *rele // 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 { u.reportToPerformUpgrade(c, upgradedRelease, kube.ResourceList{}, fmt.Errorf("pre-upgrade hooks failed: %s", err)) return } @@ -437,7 +437,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.Timeout); err != nil { + if err := u.cfg.execHook(upgradedRelease, release.HookPostUpgrade, nil, u.Timeout); err != nil { u.reportToPerformUpgrade(c, upgradedRelease, results.Created, fmt.Errorf("post-upgrade hooks failed: %s", err)) return }