Pod log location must validate container if provided

Author: fabianofranz

pkg/registry/pod/strategy.go

@@ -240,13 +240,18 @@ func LogLocation(getter ResourceGetter, connInfo client.ConnectionInfoGetter, ct
 	}
 
 	// Try to figure out a container
+	// If a container was provided, it must be valid
 	container := opts.Container
-	if container == "" {
+	if len(container) == 0 {
 		if len(pod.Spec.Containers) == 1 {
 			container = pod.Spec.Containers[0].Name
 		} else {
 			return nil, nil, errors.NewBadRequest(fmt.Sprintf("a container name must be specified for pod %s", name))
 		}
+	} else {
+		if !podHasContainerWithName(pod, container) {
+			return nil, nil, errors.NewBadRequest(fmt.Sprintf("container %s is not valid for pod %s", container, name))
+		}
 	}
 	nodeHost := pod.Spec.NodeName
 	if len(nodeHost) == 0 {
@@ -282,12 +287,21 @@ func LogLocation(getter ResourceGetter, connInfo client.ConnectionInfoGetter, ct
 	loc := &url.URL{
 		Scheme:   nodeScheme,
 		Host:     fmt.Sprintf("%s:%d", nodeHost, nodePort),
-		Path:     fmt.Sprintf("/containerLogs/%s/%s/%s", pod.Namespace, name, container),
+		Path:     fmt.Sprintf("/containerLogs/%s/%s/%s", pod.Namespace, pod.Name, container),
 		RawQuery: params.Encode(),
 	}
 	return loc, nodeTransport, nil
 }
 
+func podHasContainerWithName(pod *api.Pod, containerName string) bool {
+	for _, c := range pod.Spec.Containers {
+		if c.Name == containerName {
+			return true
+		}
+	}
+	return false
+}
+
 func streamParams(params url.Values, opts runtime.Object) error {
 	switch opts := opts.(type) {
 	case *api.PodExecOptions:
@@ -344,12 +358,17 @@ func streamLocation(getter ResourceGetter, connInfo client.ConnectionInfoGetter,
 	}
 
 	// Try to figure out a container
+	// If a container was provided, it must be valid
 	if container == "" {
 		if len(pod.Spec.Containers) == 1 {
 			container = pod.Spec.Containers[0].Name
 		} else {
 			return nil, nil, errors.NewBadRequest(fmt.Sprintf("a container name must be specified for pod %s", name))
 		}
+	} else {
+		if !podHasContainerWithName(pod, container) {
+			return nil, nil, errors.NewBadRequest(fmt.Sprintf("container %s is not valid for pod %s", container, name))
+		}
 	}
 	nodeHost := pod.Spec.NodeName
 	if len(nodeHost) == 0 {
@@ -367,7 +386,7 @@ func streamLocation(getter ResourceGetter, connInfo client.ConnectionInfoGetter,
 	loc := &url.URL{
 		Scheme:   nodeScheme,
 		Host:     fmt.Sprintf("%s:%d", nodeHost, nodePort),
-		Path:     fmt.Sprintf("/%s/%s/%s/%s", path, pod.Namespace, name, container),
+		Path:     fmt.Sprintf("/%s/%s/%s/%s", path, pod.Namespace, pod.Name, container),
 		RawQuery: params.Encode(),
 	}
 	return loc, nodeTransport, nil
@@ -392,7 +411,7 @@ func PortForwardLocation(getter ResourceGetter, connInfo client.ConnectionInfoGe
 	loc := &url.URL{
 		Scheme: nodeScheme,
 		Host:   fmt.Sprintf("%s:%d", nodeHost, nodePort),
-		Path:   fmt.Sprintf("/portForward/%s/%s", pod.Namespace, name),
+		Path:   fmt.Sprintf("/portForward/%s/%s", pod.Namespace, pod.Name),
 	}
 	return loc, nodeTransport, nil
 }

pkg/registry/pod/strategy_test.go

@@ -17,9 +17,12 @@ limitations under the License.
 package pod
 
 import (
+	"reflect"
 	"testing"
 
 	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/errors"
+	"k8s.io/kubernetes/pkg/runtime"
 )
 
 func TestCheckGracefulDelete(t *testing.T) {
@@ -76,3 +79,91 @@ func TestCheckGracefulDelete(t *testing.T) {
 		}
 	}
 }
+
+type mockPodGetter struct {
+	pod *api.Pod
+}
+
+func (g mockPodGetter) Get(api.Context, string) (runtime.Object, error) {
+	return g.pod, nil
+}
+
+func TestCheckLogLocation(t *testing.T) {
+	ctx := api.NewDefaultContext()
+	tcs := []struct {
+		in          *api.Pod
+		opts        *api.PodLogOptions
+		expectedErr error
+	}{
+		{
+			in: &api.Pod{
+				Spec:   api.PodSpec{},
+				Status: api.PodStatus{},
+			},
+			opts:        &api.PodLogOptions{},
+			expectedErr: errors.NewBadRequest("a container name must be specified for pod test"),
+		},
+		{
+			in: &api.Pod{
+				Spec: api.PodSpec{
+					Containers: []api.Container{
+						{Name: "mycontainer"},
+					},
+				},
+				Status: api.PodStatus{},
+			},
+			opts:        &api.PodLogOptions{},
+			expectedErr: nil,
+		},
+		{
+			in: &api.Pod{
+				Spec: api.PodSpec{
+					Containers: []api.Container{
+						{Name: "container1"},
+						{Name: "container2"},
+					},
+				},
+				Status: api.PodStatus{},
+			},
+			opts:        &api.PodLogOptions{},
+			expectedErr: errors.NewBadRequest("a container name must be specified for pod test"),
+		},
+		{
+			in: &api.Pod{
+				Spec: api.PodSpec{
+					Containers: []api.Container{
+						{Name: "container1"},
+						{Name: "container2"},
+					},
+				},
+				Status: api.PodStatus{},
+			},
+			opts: &api.PodLogOptions{
+				Container: "unknown",
+			},
+			expectedErr: errors.NewBadRequest("container unknown is not valid for pod test"),
+		},
+		{
+			in: &api.Pod{
+				Spec: api.PodSpec{
+					Containers: []api.Container{
+						{Name: "container1"},
+						{Name: "container2"},
+					},
+				},
+				Status: api.PodStatus{},
+			},
+			opts: &api.PodLogOptions{
+				Container: "container2",
+			},
+			expectedErr: nil,
+		},
+	}
+	for _, tc := range tcs {
+		getter := &mockPodGetter{tc.in}
+		_, _, err := LogLocation(getter, nil, ctx, "test", tc.opts)
+		if !reflect.DeepEqual(err, tc.expectedErr) {
+			t.Errorf("expected %v, got %v", tc.expectedErr, err)
+		}
+	}
+}