Merge pull request #103284 from kebe7jun/fix/kubectl-logs-error-for-multiple-pods

Fix the code is leaking the defaulting between unrelated pod instances

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 {
+			if annotations := t.GetAnnotations(); annotations != nil && currOpts.Container == "" {
-				var containerName string
+				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 len(defaultContainer) > 0 {
-					if exists, _ := podcmd.FindContainerByName(t, containerName); exists != nil {
+					if exists, _ := podcmd.FindContainerByName(t, defaultContainer); exists == nil {
-						opts.Container = containerName
+						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 currOpts.Container == "" {
-			if opts == nil || len(opts.Container) == 0 {
 				// 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
+				currOpts.Container = t.Spec.Containers[0].Name
-			} else {
-				containerName = opts.Container
 			}
 
-			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
 		}
 

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,