From 6d9004dc62de890c0dcbdfaefe990804870120b5 Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Thu, 17 Feb 2022 15:24:56 +0800 Subject: [PATCH] [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 --- changelogs/unreleased/4661-jxun | 1 + pkg/podexec/pod_command_executor.go | 36 +++++++++++++----------- pkg/podexec/pod_command_executor_test.go | 23 +++++++++++++++ 3 files changed, 43 insertions(+), 17 deletions(-) create mode 100644 changelogs/unreleased/4661-jxun diff --git a/changelogs/unreleased/4661-jxun b/changelogs/unreleased/4661-jxun new file mode 100644 index 000000000..3c08a0e3c --- /dev/null +++ b/changelogs/unreleased/4661-jxun @@ -0,0 +1 @@ +Avoid overwritten hook's exec.container parameter when running pod command executor. \ No newline at end of file diff --git a/pkg/podexec/pod_command_executor.go b/pkg/podexec/pod_command_executor.go index a44554820..569fca4e5 100644 --- a/pkg/podexec/pod_command_executor.go +++ b/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()) diff --git a/pkg/podexec/pod_command_executor_test.go b/pkg/podexec/pod_command_executor_test.go index 39b46d11d..d4637bf4a 100644 --- a/pkg/podexec/pod_command_executor_test.go +++ b/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) }) }