From b54e823dbce08bff6fab979243663b0fea5a351f Mon Sep 17 00:00:00 2001 From: pacoxu Date: Mon, 7 Dec 2020 15:30:30 +0800 Subject: [PATCH 1/2] feature: use default container annotation for logs and exec - update according to KEP: move getContainerName to helper Signed-off-by: pacoxu --- .../api/core/v1/annotation_key_constants.go | 4 ++ .../src/k8s.io/kubectl/pkg/cmd/exec/exec.go | 20 ++++---- staging/src/k8s.io/kubectl/pkg/cmd/util/BUILD | 1 + .../k8s.io/kubectl/pkg/cmd/util/helpers.go | 26 ++++++++++ .../pkg/polymorphichelpers/logsforobject.go | 48 +++++++------------ .../polymorphichelpers/logsforobject_test.go | 8 ++-- .../kubectl/pkg/util/podutils/podutils.go | 23 +++++++++ 7 files changed, 85 insertions(+), 45 deletions(-) diff --git a/staging/src/k8s.io/api/core/v1/annotation_key_constants.go b/staging/src/k8s.io/api/core/v1/annotation_key_constants.go index d3ebf862836..e59e7bc2649 100644 --- a/staging/src/k8s.io/api/core/v1/annotation_key_constants.go +++ b/staging/src/k8s.io/api/core/v1/annotation_key_constants.go @@ -95,6 +95,10 @@ const ( // configuration of a resource for use in a three way diff by UpdateApplyAnnotation. LastAppliedConfigAnnotation = kubectlPrefix + "last-applied-configuration" + // DefaultContainerAnnotationName is an annotation name that can be used to preselect the interesting container + // from a pod when running kubectl. + DefaultContainerAnnotationName = kubectlPrefix + "default-container" + // AnnotationLoadBalancerSourceRangesKey is the key of the annotation on a service to set allowed ingress ranges on their LoadBalancers // // It should be a comma-separated list of CIDRs, e.g. `0.0.0.0/0` to diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/exec/exec.go b/staging/src/k8s.io/kubectl/pkg/cmd/exec/exec.go index bbca1ea2d76..6b874fc68bf 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/exec/exec.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/exec/exec.go @@ -323,16 +323,7 @@ func (p *ExecOptions) Run() error { return fmt.Errorf("cannot exec into a container in a completed pod; current phase is %s", pod.Status.Phase) } - containerName := p.ContainerName - if len(containerName) == 0 { - if len(pod.Spec.Containers) > 1 { - fmt.Fprintf(p.ErrOut, "Defaulting container name to %s.\n", pod.Spec.Containers[0].Name) - if p.EnableSuggestedCmdUsage { - fmt.Fprintf(p.ErrOut, "Use '%s describe pod/%s -n %s' to see all of the containers in this pod.\n", p.ParentCommandName, pod.Name, p.Namespace) - } - } - containerName = pod.Spec.Containers[0].Name - } + containerName := getDefaultContainerName(p, pod) // ensure we can recover the terminal while attached t := p.SetupTTY() @@ -377,3 +368,12 @@ func (p *ExecOptions) Run() error { return nil } + +func getDefaultContainerName(p *ExecOptions, pod *corev1.Pod) string { + containerName := p.ContainerName + if len(containerName) != 0 { + return containerName + } + + return cmdutil.GetDefaultContainerName(pod, p.EnableSuggestedCmdUsage, p.ErrOut) +} diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/util/BUILD b/staging/src/k8s.io/kubectl/pkg/cmd/util/BUILD index 626f6129448..ce685771013 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/util/BUILD +++ b/staging/src/k8s.io/kubectl/pkg/cmd/util/BUILD @@ -34,6 +34,7 @@ go_library( "//staging/src/k8s.io/kubectl/pkg/scheme:go_default_library", "//staging/src/k8s.io/kubectl/pkg/util/openapi:go_default_library", "//staging/src/k8s.io/kubectl/pkg/util/openapi/validation:go_default_library", + "//staging/src/k8s.io/kubectl/pkg/util/podutils:go_default_library", "//staging/src/k8s.io/kubectl/pkg/util/templates:go_default_library", "//staging/src/k8s.io/kubectl/pkg/validation:go_default_library", "//vendor/github.com/evanphx/json-patch:go_default_library", diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go b/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go index a6bfdd80afd..8e9340b4bd0 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go @@ -30,6 +30,7 @@ import ( jsonpatch "github.com/evanphx/json-patch" "github.com/spf13/cobra" "github.com/spf13/pflag" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -44,6 +45,7 @@ import ( "k8s.io/client-go/scale" "k8s.io/client-go/tools/clientcmd" "k8s.io/klog/v2" + "k8s.io/kubectl/pkg/util/podutils" utilexec "k8s.io/utils/exec" ) @@ -744,3 +746,27 @@ func Warning(cmdErr io.Writer, newGeneratorName, oldGeneratorName string) { oldGeneratorName, ) } + +// GetDefaultContainerName returns the default container name for a pod. +// it checks the annotation kubectl.kubernetes.io/default-container at first and then the first container name. +func GetDefaultContainerName(pod *corev1.Pod, enableSuggestedCmdUsage bool, w io.Writer) string { + if len(pod.Spec.Containers) > 1 { + // 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 := pod.Annotations; annotations != nil && len(annotations[corev1.DefaultContainerAnnotationName]) > 0 { + containerName := annotations[corev1.DefaultContainerAnnotationName] + if exists, _ := podutils.FindContainerByName(pod, containerName); exists != nil { + return containerName + } else { + fmt.Fprintf(w, "Default container name %q in annotation not found in a pod\n", containerName) + } + } + fmt.Fprintf(w, "Defaulting container name to first container %s.\n", pod.Spec.Containers[0].Name) + + if enableSuggestedCmdUsage { + fmt.Fprintf(w, "Use 'kubectl describe pod/%s -n %s' to see all of the containers in this pod.\n", pod.Name, pod.Namespace) + } + } + + return pod.Spec.Containers[0].Name +} diff --git a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go index b45243ba919..d9f76af65b7 100644 --- a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go +++ b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go @@ -35,7 +35,7 @@ import ( ) // defaultLogsContainerAnnotationName is an annotation name that can be used to preselect the interesting container -// from a pod when running kubectl logs. +// from a pod when running kubectl logs. It is deprecated and will be remove in 1.25. const defaultLogsContainerAnnotationName = "kubectl.kubernetes.io/default-logs-container" func logsForObject(restClientGetter genericclioptions.RESTClientGetter, object, options runtime.Object, timeout time.Duration, allContainers bool) (map[corev1.ObjectReference]rest.ResponseWrapper, error) { @@ -73,14 +73,22 @@ func logsForObjectWithClient(clientset corev1client.CoreV1Interface, object, opt return ret, nil case *corev1.Pod: - // in case the "kubectl.kubernetes.io/default-logs-container" annotation is present, we preset the opts.Containers to default to selected + // 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 && len(annotations[defaultLogsContainerAnnotationName]) > 0 { - containerName := annotations[defaultLogsContainerAnnotationName] - if exists, _ := findContainerByName(t, containerName); exists != nil { - opts.Container = containerName - } else { - fmt.Fprintf(os.Stderr, "Default container name %q not found in a pod\n", containerName) + if annotations := t.GetAnnotations(); annotations != nil && len(opts.Container) == 0 { + var containerName string + if len(annotations[defaultLogsContainerAnnotationName]) > 0 { + containerName = annotations[defaultLogsContainerAnnotationName] + fmt.Fprintf(os.Stderr, "Found deprecated `kubectl.kubernetes.io/default-logs-container` annotation %v in pod/%v\n", containerName, t.Name) + } else if len(annotations[corev1.DefaultContainerAnnotationName]) > 0 { + containerName = annotations[corev1.DefaultContainerAnnotationName] + } + if len(containerName) > 0 { + if exists, _ := podutils.FindContainerByName(t, containerName); exists != nil { + opts.Container = containerName + } else { + fmt.Fprintf(os.Stderr, "Default container name %q not found in a pod\n", containerName) + } } } // if allContainers is true, then we're going to locate all containers and then iterate through them. At that point, "allContainers" is false @@ -108,7 +116,7 @@ func logsForObjectWithClient(clientset corev1client.CoreV1Interface, object, opt containerName = opts.Container } - container, fieldPath := findContainerByName(t, containerName) + container, fieldPath := podutils.FindContainerByName(t, containerName) if container == nil { return nil, fmt.Errorf("container %s is not valid for pod %s", opts.Container, t.Name) } @@ -177,28 +185,6 @@ func logsForObjectWithClient(clientset corev1client.CoreV1Interface, object, opt return logsForObjectWithClient(clientset, pod, options, timeout, allContainers) } -// findContainerByName searches for a container by name amongst all containers in a pod. -// Returns a pointer to a container and a field path. -func findContainerByName(pod *corev1.Pod, name string) (container *corev1.Container, fieldPath string) { - for _, c := range pod.Spec.InitContainers { - if c.Name == name { - return &c, fmt.Sprintf("spec.initContainers{%s}", c.Name) - } - } - for _, c := range pod.Spec.Containers { - if c.Name == name { - return &c, fmt.Sprintf("spec.containers{%s}", c.Name) - } - } - for _, c := range pod.Spec.EphemeralContainers { - if c.Name == name { - containerCommon := corev1.Container(c.EphemeralContainerCommon) - return &containerCommon, fmt.Sprintf("spec.ephemeralContainers{%s}", containerCommon.Name) - } - } - return nil, "" -} - // getContainerNames returns a formatted string containing the container names func getContainerNames(containers []corev1.Container) string { names := []string{} 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 f8906941f35..23a239e3eb0 100644 --- a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject_test.go +++ b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject_test.go @@ -429,7 +429,7 @@ func TestLogsForObjectWithClient(t *testing.T) { name: "two container pod with default container selected", podFn: func() *corev1.Pod { pod := testPodWithTwoContainers() - pod.Annotations = map[string]string{defaultLogsContainerAnnotationName: "foo-2-c1"} + pod.Annotations = map[string]string{corev1.DefaultContainerAnnotationName: "foo-2-c1"} return pod }, podLogOptions: &corev1.PodLogOptions{}, @@ -439,7 +439,7 @@ func TestLogsForObjectWithClient(t *testing.T) { name: "two container pod with default container selected but also container set explicitly", podFn: func() *corev1.Pod { pod := testPodWithTwoContainers() - pod.Annotations = map[string]string{defaultLogsContainerAnnotationName: "foo-2-c1"} + pod.Annotations = map[string]string{corev1.DefaultContainerAnnotationName: "foo-2-c1"} return pod }, podLogOptions: &corev1.PodLogOptions{ @@ -451,7 +451,7 @@ func TestLogsForObjectWithClient(t *testing.T) { name: "two container pod with non-existing default container selected", podFn: func() *corev1.Pod { pod := testPodWithTwoContainers() - pod.Annotations = map[string]string{defaultLogsContainerAnnotationName: "non-existing"} + pod.Annotations = map[string]string{corev1.DefaultContainerAnnotationName: "non-existing"} return pod }, podLogOptions: &corev1.PodLogOptions{}, @@ -461,7 +461,7 @@ func TestLogsForObjectWithClient(t *testing.T) { name: "two container pod with default container set, but allContainers also set", podFn: func() *corev1.Pod { pod := testPodWithTwoContainers() - pod.Annotations = map[string]string{defaultLogsContainerAnnotationName: "foo-2-c1"} + pod.Annotations = map[string]string{corev1.DefaultContainerAnnotationName: "foo-2-c1"} return pod }, allContainers: true, diff --git a/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go b/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go index 847eb7e88b4..be8c50b05d0 100644 --- a/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go +++ b/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go @@ -17,6 +17,7 @@ limitations under the License. package podutils import ( + "fmt" "time" corev1 "k8s.io/api/core/v1" @@ -186,3 +187,25 @@ func maxContainerRestarts(pod *corev1.Pod) int { } return maxRestarts } + +// FindContainerByName searches for a container by name amongst all containers in a pod. +// Returns a pointer to a container and a field path. +func FindContainerByName(pod *corev1.Pod, name string) (container *corev1.Container, fieldPath string) { + for _, c := range pod.Spec.InitContainers { + if c.Name == name { + return &c, fmt.Sprintf("spec.initContainers{%s}", c.Name) + } + } + for _, c := range pod.Spec.Containers { + if c.Name == name { + return &c, fmt.Sprintf("spec.containers{%s}", c.Name) + } + } + for _, c := range pod.Spec.EphemeralContainers { + if c.Name == name { + containerCommon := corev1.Container(c.EphemeralContainerCommon) + return &containerCommon, fmt.Sprintf("spec.ephemeralContainers{%s}", containerCommon.Name) + } + } + return nil, "" +} From 27bd94e54d2f8a7676411b280ab9d0ac7f9e921b Mon Sep 17 00:00:00 2001 From: pacoxu Date: Fri, 26 Feb 2021 10:11:12 +0800 Subject: [PATCH 2/2] move default container annotation to kubectl pkg --- .../src/k8s.io/api/core/v1/annotation_key_constants.go | 4 ---- staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go | 4 ++-- .../kubectl/pkg/polymorphichelpers/logsforobject.go | 6 +++--- .../kubectl/pkg/polymorphichelpers/logsforobject_test.go | 9 +++++---- staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go | 4 ++++ 5 files changed, 14 insertions(+), 13 deletions(-) diff --git a/staging/src/k8s.io/api/core/v1/annotation_key_constants.go b/staging/src/k8s.io/api/core/v1/annotation_key_constants.go index e59e7bc2649..d3ebf862836 100644 --- a/staging/src/k8s.io/api/core/v1/annotation_key_constants.go +++ b/staging/src/k8s.io/api/core/v1/annotation_key_constants.go @@ -95,10 +95,6 @@ const ( // configuration of a resource for use in a three way diff by UpdateApplyAnnotation. LastAppliedConfigAnnotation = kubectlPrefix + "last-applied-configuration" - // DefaultContainerAnnotationName is an annotation name that can be used to preselect the interesting container - // from a pod when running kubectl. - DefaultContainerAnnotationName = kubectlPrefix + "default-container" - // AnnotationLoadBalancerSourceRangesKey is the key of the annotation on a service to set allowed ingress ranges on their LoadBalancers // // It should be a comma-separated list of CIDRs, e.g. `0.0.0.0/0` to diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go b/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go index 8e9340b4bd0..b7d056a0104 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go @@ -753,8 +753,8 @@ func GetDefaultContainerName(pod *corev1.Pod, enableSuggestedCmdUsage bool, w io if len(pod.Spec.Containers) > 1 { // 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 := pod.Annotations; annotations != nil && len(annotations[corev1.DefaultContainerAnnotationName]) > 0 { - containerName := annotations[corev1.DefaultContainerAnnotationName] + if annotations := pod.Annotations; annotations != nil && len(annotations[podutils.DefaultContainerAnnotationName]) > 0 { + containerName := annotations[podutils.DefaultContainerAnnotationName] if exists, _ := podutils.FindContainerByName(pod, containerName); exists != nil { return containerName } else { diff --git a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go index d9f76af65b7..3fff406909c 100644 --- a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go +++ b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go @@ -79,9 +79,9 @@ func logsForObjectWithClient(clientset corev1client.CoreV1Interface, object, opt var containerName string if len(annotations[defaultLogsContainerAnnotationName]) > 0 { containerName = annotations[defaultLogsContainerAnnotationName] - fmt.Fprintf(os.Stderr, "Found deprecated `kubectl.kubernetes.io/default-logs-container` annotation %v in pod/%v\n", containerName, t.Name) - } else if len(annotations[corev1.DefaultContainerAnnotationName]) > 0 { - containerName = annotations[corev1.DefaultContainerAnnotationName] + 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) + } else if len(annotations[podutils.DefaultContainerAnnotationName]) > 0 { + containerName = annotations[podutils.DefaultContainerAnnotationName] } if len(containerName) > 0 { if exists, _ := podutils.FindContainerByName(t, containerName); exists != 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 23a239e3eb0..60c340f73ca 100644 --- a/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject_test.go +++ b/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject_test.go @@ -32,6 +32,7 @@ import ( "k8s.io/apimachinery/pkg/util/diff" fakeexternal "k8s.io/client-go/kubernetes/fake" testclient "k8s.io/client-go/testing" + "k8s.io/kubectl/pkg/util/podutils" ) var ( @@ -429,7 +430,7 @@ func TestLogsForObjectWithClient(t *testing.T) { name: "two container pod with default container selected", podFn: func() *corev1.Pod { pod := testPodWithTwoContainers() - pod.Annotations = map[string]string{corev1.DefaultContainerAnnotationName: "foo-2-c1"} + pod.Annotations = map[string]string{podutils.DefaultContainerAnnotationName: "foo-2-c1"} return pod }, podLogOptions: &corev1.PodLogOptions{}, @@ -439,7 +440,7 @@ func TestLogsForObjectWithClient(t *testing.T) { name: "two container pod with default container selected but also container set explicitly", podFn: func() *corev1.Pod { pod := testPodWithTwoContainers() - pod.Annotations = map[string]string{corev1.DefaultContainerAnnotationName: "foo-2-c1"} + pod.Annotations = map[string]string{podutils.DefaultContainerAnnotationName: "foo-2-c1"} return pod }, podLogOptions: &corev1.PodLogOptions{ @@ -451,7 +452,7 @@ func TestLogsForObjectWithClient(t *testing.T) { name: "two container pod with non-existing default container selected", podFn: func() *corev1.Pod { pod := testPodWithTwoContainers() - pod.Annotations = map[string]string{corev1.DefaultContainerAnnotationName: "non-existing"} + pod.Annotations = map[string]string{podutils.DefaultContainerAnnotationName: "non-existing"} return pod }, podLogOptions: &corev1.PodLogOptions{}, @@ -461,7 +462,7 @@ func TestLogsForObjectWithClient(t *testing.T) { name: "two container pod with default container set, but allContainers also set", podFn: func() *corev1.Pod { pod := testPodWithTwoContainers() - pod.Annotations = map[string]string{corev1.DefaultContainerAnnotationName: "foo-2-c1"} + pod.Annotations = map[string]string{podutils.DefaultContainerAnnotationName: "foo-2-c1"} return pod }, allContainers: true, diff --git a/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go b/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go index be8c50b05d0..580f3706e49 100644 --- a/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go +++ b/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go @@ -25,6 +25,10 @@ import ( "k8s.io/utils/integer" ) +// DefaultContainerAnnotationName is an annotation name that can be used to preselect the interesting container +// from a pod when running kubectl. +const DefaultContainerAnnotationName = "kubectl.kubernetes.io/default-container" + // IsPodAvailable returns true if a pod is available; false otherwise. // Precondition for an available pod is that it must be ready. On top // of that, there are two cases when a pod can be considered available: