From cc32702e8fa1c20346e5c6d9c2d349d10fc23c3a Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Tue, 20 Aug 2019 19:51:15 +0000 Subject: [PATCH] Add ephemeral containers to streamLocation name suggestions This combines container names into a single list because separating them into a long, variable length string isn't particularly useful in the context of an streaming error message. --- pkg/registry/core/pod/BUILD | 3 +++ pkg/registry/core/pod/strategy.go | 23 ++++++------------- pkg/registry/core/pod/strategy_test.go | 31 +++++++++++++++++++++++--- 3 files changed, 38 insertions(+), 19 deletions(-) diff --git a/pkg/registry/core/pod/BUILD b/pkg/registry/core/pod/BUILD index 277bb410e98..5d842a6780e 100644 --- a/pkg/registry/core/pod/BUILD +++ b/pkg/registry/core/pod/BUILD @@ -47,6 +47,7 @@ go_test( "//pkg/api/testing:go_default_library", "//pkg/apis/core:go_default_library", "//pkg/apis/core/install:go_default_library", + "//pkg/features:go_default_library", "//pkg/kubelet/client:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", @@ -56,7 +57,9 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/client-go/tools/cache:go_default_library", + "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", ], ) diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index afcf2113fff..917769e400c 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -312,15 +312,6 @@ func ResourceLocation(ctx context.Context, getter ResourceGetter, rt http.RoundT return loc, rt, nil } -// getContainerNames returns a formatted string containing the container names -func getContainerNames(containers []api.Container) string { - names := []string{} - for _, c := range containers { - names = append(names, c.Name) - } - return strings.Join(names, " ") -} - // LogLocation returns the log URL for a pod container. If opts.Container is blank // and only one container is present in the pod, that container is used. func LogLocation( @@ -553,13 +544,13 @@ func validateContainer(container string, pod *api.Pod) (string, error) { case 0: return "", errors.NewBadRequest(fmt.Sprintf("a container name must be specified for pod %s", pod.Name)) default: - containerNames := getContainerNames(pod.Spec.Containers) - initContainerNames := getContainerNames(pod.Spec.InitContainers) - err := fmt.Sprintf("a container name must be specified for pod %s, choose one of: [%s]", pod.Name, containerNames) - if len(initContainerNames) > 0 { - err += fmt.Sprintf(" or one of the init containers: [%s]", initContainerNames) - } - return "", errors.NewBadRequest(err) + var containerNames []string + podutil.VisitContainers(&pod.Spec, func(c *api.Container) bool { + containerNames = append(containerNames, c.Name) + return true + }) + errStr := fmt.Sprintf("a container name must be specified for pod %s, choose one of: %s", pod.Name, containerNames) + return "", errors.NewBadRequest(errStr) } } else { if !podHasContainerWithName(pod, container) { diff --git a/pkg/registry/core/pod/strategy_test.go b/pkg/registry/core/pod/strategy_test.go index 8c3cc48693c..a945c1c1af0 100644 --- a/pkg/registry/core/pod/strategy_test.go +++ b/pkg/registry/core/pod/strategy_test.go @@ -32,9 +32,12 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/tools/cache" + featuregatetesting "k8s.io/component-base/featuregate/testing" apitesting "k8s.io/kubernetes/pkg/api/testing" api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/kubelet/client" // ensure types are installed @@ -321,6 +324,7 @@ func (g mockPodGetter) Get(context.Context, string, *metav1.GetOptions) (runtime } func TestCheckLogLocation(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.EphemeralContainers, true)() ctx := genericapirequest.NewDefaultContext() fakePodName := "test" tcs := []struct { @@ -407,7 +411,28 @@ func TestCheckLogLocation(t *testing.T) { Status: api.PodStatus{}, }, opts: &api.PodLogOptions{}, - expectedErr: errors.NewBadRequest("a container name must be specified for pod test, choose one of: [container1 container2] or one of the init containers: [initcontainer1]"), + expectedErr: errors.NewBadRequest("a container name must be specified for pod test, choose one of: [initcontainer1 container1 container2]"), + expectedTransport: nil, + }, + { + in: &api.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: fakePodName}, + Spec: api.PodSpec{ + Containers: []api.Container{ + {Name: "container1"}, + {Name: "container2"}, + }, + InitContainers: []api.Container{ + {Name: "initcontainer1"}, + }, + EphemeralContainers: []api.EphemeralContainer{ + {EphemeralContainerCommon: api.EphemeralContainerCommon{Name: "debugger"}}, + }, + }, + Status: api.PodStatus{}, + }, + opts: &api.PodLogOptions{}, + expectedErr: errors.NewBadRequest("a container name must be specified for pod test, choose one of: [initcontainer1 container1 container2 debugger]"), expectedTransport: nil, }, { @@ -458,10 +483,10 @@ func TestCheckLogLocation(t *testing.T) { _, actualTransport, err := LogLocation(ctx, getter, connectionGetter, fakePodName, tc.opts) if !reflect.DeepEqual(err, tc.expectedErr) { - t.Errorf("expected %v, got %v", tc.expectedErr, err) + t.Errorf("expected %q, got %q", tc.expectedErr, err) } if actualTransport != tc.expectedTransport { - t.Errorf("expected %v, got %v", tc.expectedTransport, actualTransport) + t.Errorf("expected %q, got %q", tc.expectedTransport, actualTransport) } }) }