github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-07-25 12:43:23 +00:00
Code Issues Projects Releases Wiki Activity

kubectl: Inline the containers for the user in attach/exec

Browse Source 
The behavior of the container defaulting in attach/exec is inconsistent
and should be unified. As a user, when we default the vast majority of
pods will have a small number of containers and so printing the container
names inline (as kubectl logs did) is more appropriate. The debug message
we printed about using describe was already longer than 99% of all pod
container names, so we were wasting user time.

Unify container selection for exec and attach to be consistent with old
behavior. Properly handle the --quiet flag (should not print in that case)
for both commands. Remove EnableCmdSuggestion and the machinery it needs.

The message now prints:

> Defaulted container "etcdctl" out of: etcdctl, etcd, etcd-metrics, etcd-ensure-env-vars (init), etcd-resources-copy (init)
This commit is contained in:
Clayton Coleman 2021-02-11 11:40:25 -05:00
parent 75700d32bf
commit 43e8ebbbcd
No known key found for this signature in database
GPG Key ID: 3D16906B4F1C5CB3
9 changed files with 179 additions and 125 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
staging/src/k8s.io/kubectl/pkg/cmd/attach/attach.go
View File

@ -23,7 +23,6 @@ import (
"time"
"github.com/spf13/cobra"
"k8s.io/klog/v2"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime"
@ -33,6 +32,7 @@ import (
"k8s.io/client-go/tools/remotecommand"
"k8s.io/kubectl/pkg/cmd/exec"
cmdutil "k8s.io/kubectl/pkg/cmd/util"
"k8s.io/kubectl/pkg/cmd/util/podcmd"
"k8s.io/kubectl/pkg/polymorphichelpers"
"k8s.io/kubectl/pkg/scheme"
"k8s.io/kubectl/pkg/util/i18n"
@ -68,9 +68,7 @@ type AttachOptions struct {
// whether to disable use of standard error when streaming output from tty
DisableStderr bool
CommandName string
ParentCommandName string
EnableSuggestedCmdUsage bool
CommandName string
Pod *corev1.Pod
@ -185,14 +183,6 @@ func (o *AttachOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []s
o.Resources = args
o.restClientGetter = f
cmdParent := cmd.Parent()
if cmdParent != nil {
o.ParentCommandName = cmdParent.CommandPath()
}
if len(o.ParentCommandName) > 0 && cmdutil.IsSiblingCommandExists(cmd, "describe") {
o.EnableSuggestedCmdUsage = true
}
config, err := f.ToRESTConfig()
if err != nil {
return err
@ -312,32 +302,7 @@ func (o *AttachOptions) findAttachablePod(obj runtime.Object) (*corev1.Pod, erro
// containerToAttach returns a reference to the container to attach to, given
// by name or the first container if name is empty.
func (o *AttachOptions) containerToAttachTo(pod *corev1.Pod) (*corev1.Container, error) {
if len(o.ContainerName) > 0 {
for i := range pod.Spec.Containers {
if pod.Spec.Containers[i].Name == o.ContainerName {
return &pod.Spec.Containers[i], nil
}
}
for i := range pod.Spec.InitContainers {
if pod.Spec.InitContainers[i].Name == o.ContainerName {
return &pod.Spec.InitContainers[i], nil
}
}
for i := range pod.Spec.EphemeralContainers {
if pod.Spec.EphemeralContainers[i].Name == o.ContainerName {
return (*corev1.Container)(&pod.Spec.EphemeralContainers[i].EphemeralContainerCommon), nil
}
}
return nil, fmt.Errorf("container not found (%s)", o.ContainerName)
}
if o.EnableSuggestedCmdUsage {
fmt.Fprintf(o.ErrOut, "Defaulting container name to %s.\n", pod.Spec.Containers[0].Name)
fmt.Fprintf(o.ErrOut, "Use '%s describe pod/%s -n %s' to see all of the containers in this pod.\n", o.ParentCommandName, o.PodName, o.Namespace)
}
klog.V(4).Infof("defaulting container name to %s", pod.Spec.Containers[0].Name)
return &pod.Spec.Containers[0], nil
return podcmd.FindOrDefaultContainerByName(pod, o.ContainerName, o.Quiet, o.ErrOut)
}
// GetContainerName returns the name of the container to attach to, with a fallback.

55
staging/src/k8s.io/kubectl/pkg/cmd/attach/attach_test.go
View File

@ -17,6 +17,7 @@ limitations under the License.
package attach
import (
"bytes"
"fmt"
"io"
"net/http"
@ -35,6 +36,7 @@ import (
"k8s.io/client-go/tools/remotecommand"
"k8s.io/kubectl/pkg/cmd/exec"
cmdtesting "k8s.io/kubectl/pkg/cmd/testing"
"k8s.io/kubectl/pkg/cmd/util/podcmd"
"k8s.io/kubectl/pkg/polymorphichelpers"
"k8s.io/kubectl/pkg/scheme"
)
@ -65,6 +67,7 @@ func TestPodAndContainerAttach(t *testing.T) {
expectError string
expectedPodName string
expectedContainerName string
expectOut string
obj *corev1.Pod
}{
{
@ -85,6 +88,25 @@ func TestPodAndContainerAttach(t *testing.T) {
expectedPodName: "foo",
expectedContainerName: "bar",
obj: attachPod(),
expectOut: `Defaulted container "bar" out of: bar, debugger (ephem), initfoo (init)`,
},
{
name: "no container, no flags, sets default expected container as annotation",
options: &AttachOptions{GetPodTimeout: defaultPodLogsTimeout},
args: []string{"foo"},
expectedPodName: "foo",
expectedContainerName: "bar",
obj: setDefaultContainer(attachPod(), "initfoo"),
expectOut: ``,
},
{
name: "no container, no flags, sets default missing container as annotation",
options: &AttachOptions{GetPodTimeout: defaultPodLogsTimeout},
args: []string{"foo"},
expectedPodName: "foo",
expectedContainerName: "bar",
obj: setDefaultContainer(attachPod(), "does-not-exist"),
expectOut: `Defaulted container "bar" out of: bar, debugger (ephem), initfoo (init)`,
},
{
name: "container in flag",
@ -115,7 +137,7 @@ func TestPodAndContainerAttach(t *testing.T) {
options: &AttachOptions{StreamOptions: exec.StreamOptions{ContainerName: "wrong"}, GetPodTimeout: 10},
args: []string{"foo"},
expectedPodName: "foo",
expectError: "container not found",
expectError: "container wrong not found in pod foo",
obj: attachPod(),
},
{
@ -153,11 +175,23 @@ func TestPodAndContainerAttach(t *testing.T) {
pod, err := test.options.findAttachablePod(&corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: "test-pod", Namespace: "test"},
Spec: corev1.PodSpec{
InitContainers: []corev1.Container{
{
Name: "initfoo",
},
},
Containers: []corev1.Container{
{
Name: "foobar",
},
},
EphemeralContainers: []corev1.EphemeralContainer{
{
EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Name: "ephemfoo",
},
},
},
},
})
if err != nil {
@ -171,10 +205,17 @@ func TestPodAndContainerAttach(t *testing.T) {
t.Errorf("unexpected pod name: expected %q, got %q", test.expectedContainerName, pod.Name)
}
var buf bytes.Buffer
test.options.ErrOut = &buf
container, err := test.options.containerToAttachTo(attachPod())
if len(test.expectOut) > 0 && !strings.Contains(buf.String(), test.expectOut) {
t.Errorf("unexpected output: output did not contain %q\n---\n%s", test.expectOut, buf.String())
}
if err != nil {
if test.expectError == "" || !strings.Contains(err.Error(), test.expectError) {
t.Errorf("unexpected error: expected %q, got %q", err, test.expectError)
t.Errorf("unexpected error: expected %q, got %q", test.expectError, err)
}
return
}
@ -230,7 +271,7 @@ func TestAttach(t *testing.T) {
attachPath: "/api/" + version + "/namespaces/test/pods/foo/attach",
pod: attachPod(),
container: "foo",
expectedErr: "cannot attach to the container: container not found (foo)",
expectedErr: "cannot attach to the container: container foo not found in pod foo",
},
}
for _, test := range tests {
@ -435,3 +476,11 @@ func attachPod() *corev1.Pod {
},
}
}
func setDefaultContainer(pod *corev1.Pod, name string) *corev1.Pod {
if pod.Annotations == nil {
pod.Annotations = make(map[string]string)
}
pod.Annotations[podcmd.DefaultContainerAnnotationName] = name
return pod
}

30
staging/src/k8s.io/kubectl/pkg/cmd/exec/exec.go
View File

@ -34,6 +34,7 @@ import (
"k8s.io/client-go/tools/remotecommand"
cmdutil "k8s.io/kubectl/pkg/cmd/util"
"k8s.io/kubectl/pkg/cmd/util/podcmd"
"k8s.io/kubectl/pkg/polymorphichelpers"
"k8s.io/kubectl/pkg/scheme"
"k8s.io/kubectl/pkg/util/i18n"
@ -153,9 +154,6 @@ type ExecOptions struct {
Command []string
EnforceNamespace bool
ParentCommandName string
EnableSuggestedCmdUsage bool
Builder func() *resource.Builder
ExecutablePodFn polymorphichelpers.AttachablePodForObjectFunc
restClientGetter genericclioptions.RESTClientGetter
@ -203,14 +201,6 @@ func (p *ExecOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, argsIn []s
p.Builder = f.NewBuilder
p.restClientGetter = f
cmdParent := cmd.Parent()
if cmdParent != nil {
p.ParentCommandName = cmdParent.CommandPath()
}
if len(p.ParentCommandName) > 0 && cmdutil.IsSiblingCommandExists(cmd, "describe") {
p.EnableSuggestedCmdUsage = true
}
p.Config, err = f.ToRESTConfig()
if err != nil {
return err
@ -328,7 +318,14 @@ 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 := getDefaultContainerName(p, pod)
containerName := p.ContainerName
if len(containerName) == 0 {
container, err := podcmd.FindOrDefaultContainerByName(pod, containerName, p.Quiet, p.ErrOut)
if err != nil {
return err
}
containerName = container.Name
}
// ensure we can recover the terminal while attached
t := p.SetupTTY()
@ -373,12 +370,3 @@ 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)
}

27
staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go
View File

@ -30,7 +30,6 @@ 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"
@ -45,7 +44,6 @@ 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"
)
@ -746,28 +744,3 @@ 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[podutils.DefaultContainerAnnotationName]) > 0 {
containerName := annotations[podutils.DefaultContainerAnnotationName]
if exists, _ := podutils.FindContainerByName(pod, containerName); exists != nil {
fmt.Fprintf(w, "Defaulting container name to container %s.\n", containerName)
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
}

104
staging/src/k8s.io/kubectl/pkg/cmd/util/podcmd/podcmd.go Normal file
View File

@ -0,0 +1,104 @@
/*
Copyright 2021 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package podcmd
import (
"fmt"
"io"
"strings"
v1 "k8s.io/api/core/v1"
"k8s.io/klog/v2"
)
// 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"
// FindContainerByName selects the named container from the spec of
// the provided pod or return nil if no such container exists.
func FindContainerByName(pod *v1.Pod, name string) (*v1.Container, string) {
for i := range pod.Spec.Containers {
if pod.Spec.Containers[i].Name == name {
return &pod.Spec.Containers[i], fmt.Sprintf("spec.containers{%s}", name)
}
}
for i := range pod.Spec.InitContainers {
if pod.Spec.InitContainers[i].Name == name {
return &pod.Spec.InitContainers[i], fmt.Sprintf("spec.initContainers{%s}", name)
}
}
for i := range pod.Spec.EphemeralContainers {
if pod.Spec.EphemeralContainers[i].Name == name {
return (*v1.Container)(&pod.Spec.EphemeralContainers[i].EphemeralContainerCommon), fmt.Sprintf("spec.ephemeralContainers{%s}", name)
}
}
return nil, ""
}
// FindOrDefaultContainerByName defaults a container for a pod to the first container if any
// exists, or returns an error. It will print a message to the user indicating a default was
// selected if there was more than one container.
func FindOrDefaultContainerByName(pod *v1.Pod, name string, quiet bool, warn io.Writer) (*v1.Container, error) {
var container *v1.Container
if len(name) > 0 {
container, _ = FindContainerByName(pod, name)
if container == nil {
return nil, fmt.Errorf("container %s not found in pod %s", name, pod.Name)
}
return container, nil
}
// this should never happen, but just in case
if len(pod.Spec.Containers) == 0 {
return nil, fmt.Errorf("pod %s/%s does not have any containers", pod.Namespace, pod.Name)
}
// read the default container the annotation as per
// https://github.com/kubernetes/enhancements/tree/master/keps/sig-cli/2227-kubectl-default-container
if name := pod.Annotations[DefaultContainerAnnotationName]; len(name) > 0 {
if container, _ = FindContainerByName(pod, name); container != nil {
klog.V(4).Infof("Defaulting container name from annotation %s", container.Name)
return container, nil
}
klog.V(4).Infof("Default container name from annotation %s was not found in the pod", name)
}
// pick the first container as per existing behavior
container = &pod.Spec.Containers[0]
if !quiet && (len(pod.Spec.Containers) > 1 || len(pod.Spec.InitContainers) > 0 || len(pod.Spec.EphemeralContainers) > 0) {
fmt.Fprintf(warn, "Defaulted container %q out of: %s\n", container.Name, allContainerNames(pod))
}
klog.V(4).Infof("Defaulting container name to %s", container.Name)
return &pod.Spec.Containers[0], nil
}
func allContainerNames(pod *v1.Pod) string {
var containers []string
for _, container := range pod.Spec.Containers {
containers = append(containers, container.Name)
}
for _, container := range pod.Spec.EphemeralContainers {
containers = append(containers, fmt.Sprintf("%s (ephem)", container.Name))
}
for _, container := range pod.Spec.InitContainers {
containers = append(containers, fmt.Sprintf("%s (init)", container.Name))
}
return strings.Join(containers, ", ")
}

9
staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject.go
View File

@ -30,6 +30,7 @@ import (
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/reference"
"k8s.io/kubectl/pkg/cmd/util/podcmd"
"k8s.io/kubectl/pkg/scheme"
"k8s.io/kubectl/pkg/util/podutils"
)
@ -79,8 +80,8 @@ func logsForObjectWithClient(clientset corev1client.CoreV1Interface, object, opt
// container. This gives users ability to preselect the most interesting container in pod.
if annotations := t.GetAnnotations(); annotations != nil && len(opts.Container) == 0 {
var containerName string
if len(annotations[podutils.DefaultContainerAnnotationName]) > 0 {
containerName = annotations[podutils.DefaultContainerAnnotationName]
if len(annotations[podcmd.DefaultContainerAnnotationName]) > 0 {
containerName = 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
@ -90,7 +91,7 @@ func logsForObjectWithClient(clientset corev1client.CoreV1Interface, object, opt
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 exists, _ := podutils.FindContainerByName(t, containerName); exists != nil {
if exists, _ := podcmd.FindContainerByName(t, containerName); exists != nil {
opts.Container = containerName
} else {
fmt.Fprintf(os.Stderr, "Default container name %q not found in a pod\n", containerName)
@ -121,7 +122,7 @@ func logsForObjectWithClient(clientset corev1client.CoreV1Interface, object, opt
containerName = opts.Container
}
container, fieldPath := podutils.FindContainerByName(t, containerName)
container, fieldPath := podcmd.FindContainerByName(t, containerName)
if container == nil {
return nil, fmt.Errorf("container %s is not valid for pod %s", opts.Container, t.Name)
}

10
staging/src/k8s.io/kubectl/pkg/polymorphichelpers/logsforobject_test.go
View File

@ -32,7 +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"
"k8s.io/kubectl/pkg/cmd/util/podcmd"
)
var (
@ -430,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{podutils.DefaultContainerAnnotationName: "foo-2-c1"}
pod.Annotations = map[string]string{podcmd.DefaultContainerAnnotationName: "foo-2-c1"}
return pod
},
podLogOptions: &corev1.PodLogOptions{},
@ -440,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{podutils.DefaultContainerAnnotationName: "foo-2-c1"}
pod.Annotations = map[string]string{podcmd.DefaultContainerAnnotationName: "foo-2-c1"}
return pod
},
podLogOptions: &corev1.PodLogOptions{
@ -452,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{podutils.DefaultContainerAnnotationName: "non-existing"}
pod.Annotations = map[string]string{podcmd.DefaultContainerAnnotationName: "non-existing"}
return pod
},
podLogOptions: &corev1.PodLogOptions{},
@ -462,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{podutils.DefaultContainerAnnotationName: "foo-2-c1"}
pod.Annotations = map[string]string{podcmd.DefaultContainerAnnotationName: "foo-2-c1"}
return pod
},
allContainers: true,

27
staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go
View File

@ -17,7 +17,6 @@ limitations under the License.
package podutils
import (
"fmt"
"time"
corev1 "k8s.io/api/core/v1"
@ -25,10 +24,6 @@ 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:
@ -191,25 +186,3 @@ 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, ""
}

1
vendor/modules.txt vendored
View File

@ -2403,6 +2403,7 @@ k8s.io/kubectl/pkg/cmd/top
k8s.io/kubectl/pkg/cmd/util
k8s.io/kubectl/pkg/cmd/util/editor
k8s.io/kubectl/pkg/cmd/util/editor/crlf
k8s.io/kubectl/pkg/cmd/util/podcmd
k8s.io/kubectl/pkg/cmd/util/sanity
k8s.io/kubectl/pkg/cmd/version
k8s.io/kubectl/pkg/cmd/wait