Merge 4a3faf8c13 into 37cc2fa5ce

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)

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
 		}

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 {

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")
+}

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
 		}
 	}

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)
 		}
 	}

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
 		}