[fix] Avoid overwritten hook's exec.container parameter when running pod command executor

Fix #4499
When hook influnce multiple pods, current logic's first pod's container will overwrite the hook's exec.container parameter. That will cause the other pod fail on the hook executing.

Signed-off-by: Xun Jiang <jxun@vmware.com>

changelogs/unreleased/4661-jxun

@@ -0,0 +1 @@
+Avoid overwritten hook's exec.container parameter when running pod command executor.

pkg/podexec/pod_command_executor.go

@@ -83,42 +83,44 @@ func (e *defaultPodCommandExecutor) ExecutePodCommand(log logrus.FieldLogger, it
 		return errors.New("hook is required")
 	}
 
+	localHook := *hook
+
 	pod := new(corev1api.Pod)
 	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(item, pod); err != nil {
 		return errors.WithStack(err)
 	}
 
-	if hook.Container == "" {
-		if err := setDefaultHookContainer(pod, hook); err != nil {
+	if localHook.Container == "" {
+		if err := setDefaultHookContainer(pod, &localHook); err != nil {
 			return err
 		}
-	} else if err := ensureContainerExists(pod, hook.Container); err != nil {
+	} else if err := ensureContainerExists(pod, localHook.Container); err != nil {
 		return err
 	}
 
-	if len(hook.Command) == 0 {
+	if len(localHook.Command) == 0 {
 		return errors.New("command is required")
 	}
 
-	switch hook.OnError {
+	switch localHook.OnError {
 	case api.HookErrorModeFail, api.HookErrorModeContinue:
 		// use the specified value
 	default:
 		// default to fail
-		hook.OnError = api.HookErrorModeFail
+		localHook.OnError = api.HookErrorModeFail
 	}
 
-	if hook.Timeout.Duration == 0 {
-		hook.Timeout.Duration = defaultTimeout
+	if localHook.Timeout.Duration == 0 {
+		localHook.Timeout.Duration = defaultTimeout
 	}
 
 	hookLog := log.WithFields(
 		logrus.Fields{
 			"hookName":      hookName,
-			"hookContainer": hook.Container,
-			"hookCommand":   hook.Command,
-			"hookOnError":   hook.OnError,
-			"hookTimeout":   hook.Timeout,
+			"hookContainer": localHook.Container,
+			"hookCommand":   localHook.Command,
+			"hookOnError":   localHook.OnError,
+			"hookTimeout":   localHook.Timeout,
 		},
 	)
 	hookLog.Info("running exec hook")
@@ -130,8 +132,8 @@ func (e *defaultPodCommandExecutor) ExecutePodCommand(log logrus.FieldLogger, it
 		SubResource("exec")
 
 	req.VersionedParams(&corev1api.PodExecOptions{
-		Container: hook.Container,
-		Command:   hook.Command,
+		Container: localHook.Container,
+		Command:   localHook.Command,
 		Stdout:    true,
 		Stderr:    true,
 	}, kscheme.ParameterCodec)
@@ -156,8 +158,8 @@ func (e *defaultPodCommandExecutor) ExecutePodCommand(log logrus.FieldLogger, it
 	}()
 
 	var timeoutCh <-chan time.Time
-	if hook.Timeout.Duration > 0 {
-		timer := time.NewTimer(hook.Timeout.Duration)
+	if localHook.Timeout.Duration > 0 {
+		timer := time.NewTimer(localHook.Timeout.Duration)
 		defer timer.Stop()
 		timeoutCh = timer.C
 	}
@@ -165,7 +167,7 @@ func (e *defaultPodCommandExecutor) ExecutePodCommand(log logrus.FieldLogger, it
 	select {
 	case err = <-errCh:
 	case <-timeoutCh:
-		return errors.Errorf("timed out after %v", hook.Timeout.Duration)
+		return errors.Errorf("timed out after %v", localHook.Timeout.Duration)
 	}
 
 	hookLog.Infof("stdout: %s", stdout.String())

pkg/podexec/pod_command_executor_test.go

@@ -30,6 +30,7 @@ import (
 	"github.com/stretchr/testify/require"
 	corev1api "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/remotecommand"
@@ -101,12 +102,34 @@ func TestExecutePodCommandMissingInputs(t *testing.T) {
 				Container: "foo",
 			},
 		},
+		{
+			name:         "hook's container is not overwritten by pod",
+			item:         velerotest.UnstructuredOrDie(`{"kind":"Pod","spec":{"containers":[{"name":"foo"}]}}`).Object,
+			podNamespace: "ns",
+			podName:      "pod",
+			hookName:     "hook",
+			hook: &v1.ExecHook{
+				Container: "bar",
+			},
+		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
+			pod := new(corev1api.Pod)
+			hookPodContainerNotSame := false
+			if err := runtime.DefaultUnstructuredConverter.FromUnstructured(test.item, pod); err != nil {
+				assert.Error(t, err)
+			}
+			if (len(pod.Spec.Containers) > 0) && (pod.Spec.Containers[0].Name != test.hook.Container) {
+				hookPodContainerNotSame = true
+			}
+
 			e := &defaultPodCommandExecutor{}
 			err := e.ExecutePodCommand(velerotest.NewLogger(), test.item, test.podNamespace, test.podName, test.hookName, test.hook)
+			if hookPodContainerNotSame && test.hook.Container == pod.Spec.Containers[0].Name {
+				assert.Error(t, fmt.Errorf("hook exec container is overwritten"))
+			}
 			assert.Error(t, err)
 		})
 	}