From 671a422a410c96995cc97d807b5abf24c86b24c0 Mon Sep 17 00:00:00 2001 From: Fabiano Franz Date: Fri, 27 Nov 2015 16:19:36 -0200 Subject: [PATCH] Pod log location must validate container if provided --- pkg/registry/pod/strategy.go | 27 +++++++-- pkg/registry/pod/strategy_test.go | 91 +++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 4 deletions(-) diff --git a/pkg/registry/pod/strategy.go b/pkg/registry/pod/strategy.go index fbb977360d29..64d7e538ea97 100644 --- a/pkg/registry/pod/strategy.go +++ b/pkg/registry/pod/strategy.go @@ -235,13 +235,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 { @@ -277,12 +282,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: @@ -339,12 +353,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 { @@ -362,7 +381,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 @@ -387,7 +406,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 } diff --git a/pkg/registry/pod/strategy_test.go b/pkg/registry/pod/strategy_test.go index fec29d108175..60f31531e92c 100644 --- a/pkg/registry/pod/strategy_test.go +++ b/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) + } + } +}