From 8ceaecb07d4f11786967bcd94a9f61636f3b60da Mon Sep 17 00:00:00 2001 From: Kebe Date: Fri, 9 Jul 2021 09:57:05 +0800 Subject: [PATCH] Fix the code is leaking the defaulting between unrelated pod instances --- .../pkg/polymorphichelpers/logsforobject.go | 33 ++++---- .../polymorphichelpers/logsforobject_test.go | 78 +++++++++++++++++-- 2 files changed, 88 insertions(+), 23 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go index 5395596fc3439..7f5a275b01a02 100644 --- a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go +++ b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go @@ -76,31 +76,34 @@ func logsForObjectWithClient(clientset corev1client.CoreV1Interface, object, opt case *corev1.Pod: // if allContainers is true, then we're going to locate all containers and then iterate through them. At that point, "allContainers" is false if !allContainers { + currOpts := new(corev1.PodLogOptions) + if opts != nil { + opts.DeepCopyInto(currOpts) + } // in case the "kubectl.kubernetes.io/default-container" annotation is present, we preset the opts.Containers to default to selected // container. This gives users ability to preselect the most interesting container in pod. - if annotations := t.GetAnnotations(); annotations != nil && len(opts.Container) == 0 { - var containerName string + if annotations := t.GetAnnotations(); annotations != nil && currOpts.Container == "" { + var defaultContainer string if len(annotations[podcmd.DefaultContainerAnnotationName]) > 0 { - containerName = annotations[podcmd.DefaultContainerAnnotationName] + defaultContainer = annotations[podcmd.DefaultContainerAnnotationName] } else if len(annotations[defaultLogsContainerAnnotationName]) > 0 { // Only log deprecation if we have only the old annotation. This allows users to // set both to support multiple versions of kubectl; if they are setting both // they must already know it is deprecated, so we don't need to add noisy // warnings. - containerName = annotations[defaultLogsContainerAnnotationName] + defaultContainer = annotations[defaultLogsContainerAnnotationName] fmt.Fprintf(os.Stderr, "Using deprecated annotation `kubectl.kubernetes.io/default-logs-container` in pod/%v. Please use `kubectl.kubernetes.io/default-container` instead\n", t.Name) } - if len(containerName) > 0 { - if exists, _ := podcmd.FindContainerByName(t, containerName); exists != nil { - opts.Container = containerName + if len(defaultContainer) > 0 { + if exists, _ := podcmd.FindContainerByName(t, defaultContainer); exists == nil { + fmt.Fprintf(os.Stderr, "Default container name %q not found in pod %s\n", defaultContainer, t.Name) } else { - fmt.Fprintf(os.Stderr, "Default container name %q not found in a pod\n", containerName) + currOpts.Container = defaultContainer } } } - var containerName string - if opts == nil || len(opts.Container) == 0 { + if currOpts.Container == "" { // We don't know container name. In this case we expect only one container to be present in the pod (ignoring InitContainers). // If there is more than one container, we should return an error showing all container names. if len(t.Spec.Containers) != 1 { @@ -117,14 +120,12 @@ func logsForObjectWithClient(clientset corev1client.CoreV1Interface, object, opt return nil, errors.New(err) } - containerName = t.Spec.Containers[0].Name - } else { - containerName = opts.Container + currOpts.Container = t.Spec.Containers[0].Name } - container, fieldPath := podcmd.FindContainerByName(t, containerName) + container, fieldPath := podcmd.FindContainerByName(t, currOpts.Container) if container == nil { - return nil, fmt.Errorf("container %s is not valid for pod %s", opts.Container, t.Name) + return nil, fmt.Errorf("container %s is not valid for pod %s", currOpts.Container, t.Name) } ref, err := reference.GetPartialReference(scheme.Scheme, t, fieldPath) if err != nil { @@ -132,7 +133,7 @@ func logsForObjectWithClient(clientset corev1client.CoreV1Interface, object, opt } ret := make(map[corev1.ObjectReference]rest.ResponseWrapper, 1) - ret[*ref] = clientset.Pods(t.Namespace).GetLogs(t.Name, opts) + ret[*ref] = clientset.Pods(t.Namespace).GetLogs(t.Name, currOpts) return ret, nil } diff --git a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject_test.go b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject_test.go index 928df2480d0c5..65a0c75d4022b 100644 --- a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject_test.go +++ b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject_test.go @@ -56,7 +56,7 @@ func TestLogsForObject(t *testing.T) { name: "pod logs", obj: testPodWithOneContainers(), actions: []testclient.Action{ - getLogsAction("test", nil), + getLogsAction("test", &corev1.PodLogOptions{Container: "c1"}), }, expectedSources: []corev1.ObjectReference{ { @@ -129,7 +129,7 @@ func TestLogsForObject(t *testing.T) { Items: []corev1.Pod{*testPodWithOneContainers()}, }, actions: []testclient.Action{ - getLogsAction("test", nil), + getLogsAction("test", &corev1.PodLogOptions{Container: "c1"}), }, expectedSources: []corev1.ObjectReference{{ Kind: testPodWithOneContainers().Kind, @@ -139,6 +139,70 @@ func TestLogsForObject(t *testing.T) { FieldPath: fmt.Sprintf("spec.containers{%s}", testPodWithOneContainers().Spec.Containers[0].Name), }}, }, + { + name: "pods list logs: default container should not leak across pods", + obj: &corev1.PodList{ + Items: []corev1.Pod{ + { + TypeMeta: metav1.TypeMeta{ + Kind: "pod", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "test", + Labels: map[string]string{"test": "logs"}, + Annotations: map[string]string{ + "kubectl.kubernetes.io/default-container": "c1", + }, + }, + Spec: corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyAlways, + DNSPolicy: corev1.DNSClusterFirst, + Containers: []corev1.Container{ + {Name: "c1"}, + {Name: "c2"}, + }, + }, + }, + { + TypeMeta: metav1.TypeMeta{ + Kind: "pod", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + Namespace: "test", + Labels: map[string]string{"test": "logs"}, + }, + Spec: corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyAlways, + DNSPolicy: corev1.DNSClusterFirst, + Containers: []corev1.Container{ + {Name: "c2"}, + }, + }, + }, + }, + }, + actions: []testclient.Action{ + getLogsAction("test", &corev1.PodLogOptions{Container: "c1"}), + getLogsAction("test", &corev1.PodLogOptions{Container: "c2"}), + }, + expectedSources: []corev1.ObjectReference{{ + Kind: "pod", + APIVersion: "v1", + Name: "foo", + Namespace: "test", + FieldPath: fmt.Sprintf("spec.containers{%s}", "c1"), + }, { + Kind: "pod", + APIVersion: "v1", + Name: "bar", + Namespace: "test", + FieldPath: fmt.Sprintf("spec.containers{%s}", "c2"), + }}, + }, { name: "pods list logs: all containers", obj: &corev1.PodList{ @@ -201,7 +265,7 @@ func TestLogsForObject(t *testing.T) { clientsetPods: []runtime.Object{testPodWithOneContainers()}, actions: []testclient.Action{ testclient.NewListAction(podsResource, podsKind, "test", metav1.ListOptions{LabelSelector: "foo=bar"}), - getLogsAction("test", nil), + getLogsAction("test", &corev1.PodLogOptions{Container: "c1"}), }, expectedSources: []corev1.ObjectReference{{ Kind: testPodWithOneContainers().Kind, @@ -222,7 +286,7 @@ func TestLogsForObject(t *testing.T) { clientsetPods: []runtime.Object{testPodWithOneContainers()}, actions: []testclient.Action{ testclient.NewListAction(podsResource, podsKind, "test", metav1.ListOptions{LabelSelector: "foo=bar"}), - getLogsAction("test", nil), + getLogsAction("test", &corev1.PodLogOptions{Container: "c1"}), }, expectedSources: []corev1.ObjectReference{{ Kind: testPodWithOneContainers().Kind, @@ -243,7 +307,7 @@ func TestLogsForObject(t *testing.T) { clientsetPods: []runtime.Object{testPodWithOneContainers()}, actions: []testclient.Action{ testclient.NewListAction(podsResource, podsKind, "test", metav1.ListOptions{LabelSelector: "foo=bar"}), - getLogsAction("test", nil), + getLogsAction("test", &corev1.PodLogOptions{Container: "c1"}), }, expectedSources: []corev1.ObjectReference{{ Kind: testPodWithOneContainers().Kind, @@ -264,7 +328,7 @@ func TestLogsForObject(t *testing.T) { clientsetPods: []runtime.Object{testPodWithOneContainers()}, actions: []testclient.Action{ testclient.NewListAction(podsResource, podsKind, "test", metav1.ListOptions{LabelSelector: "foo=bar"}), - getLogsAction("test", nil), + getLogsAction("test", &corev1.PodLogOptions{Container: "c1"}), }, expectedSources: []corev1.ObjectReference{{ Kind: testPodWithOneContainers().Kind, @@ -285,7 +349,7 @@ func TestLogsForObject(t *testing.T) { clientsetPods: []runtime.Object{testPodWithOneContainers()}, actions: []testclient.Action{ testclient.NewListAction(podsResource, podsKind, "test", metav1.ListOptions{LabelSelector: "foo=bar"}), - getLogsAction("test", nil), + getLogsAction("test", &corev1.PodLogOptions{Container: "c1"}), }, expectedSources: []corev1.ObjectReference{{ Kind: testPodWithOneContainers().Kind,