From baed6240468250c855a5ec235e47a64e078bd042 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Mon, 24 Jul 2023 15:28:21 +0300 Subject: [PATCH 1/3] kubectl debug: Introduce customizable AttachFunc instead static one Currently, kubectl debug statically relies on handleAttachPod function in order to attach to the pod. However, external tools would want to set their own customized attach function and this commit introduces generic `AttachFunc` function interface which can also override by external tools. From the point of kubectl debug, there is no functionality change. --- .../src/k8s.io/kubectl/pkg/cmd/debug/debug.go | 46 ++++++++++--------- .../kubectl/pkg/cmd/debug/debug_test.go | 2 +- 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go b/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go index 852e3807014..1066a86b8b8 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go @@ -111,6 +111,7 @@ type DebugOptions struct { Args []string ArgsOnly bool Attach bool + AttachFunc func(ctx context.Context, restClientGetter genericclioptions.RESTClientGetter, cmdPath string, ns, podName, containerName string) error Container string CopyTo string Replace bool @@ -212,6 +213,9 @@ func (o *DebugOptions) Complete(restClientGetter genericclioptions.RESTClientGet attachFlag := cmd.Flags().Lookup("attach") if !attachFlag.Changed && o.Interactive { o.Attach = true + if o.AttachFunc == nil { + o.AttachFunc = o.handleAttachPod + } } // Environment @@ -377,26 +381,8 @@ func (o *DebugOptions) Run(restClientGetter genericclioptions.RESTClientGetter, return visitErr } - if o.Attach && len(containerName) > 0 { - opts := &attach.AttachOptions{ - StreamOptions: exec.StreamOptions{ - IOStreams: o.IOStreams, - Stdin: o.Interactive, - TTY: o.TTY, - Quiet: o.Quiet, - }, - CommandName: cmd.Parent().CommandPath() + " attach", - - Attach: &attach.DefaultRemoteAttach{}, - } - config, err := restClientGetter.ToRESTConfig() - if err != nil { - return err - } - opts.Config = config - opts.AttachFunc = attach.DefaultAttachFunc - - if err := o.handleAttachPod(ctx, restClientGetter, debugPod.Namespace, debugPod.Name, containerName, opts); err != nil { + if o.Attach && len(containerName) > 0 && o.AttachFunc != nil { + if err := o.AttachFunc(ctx, restClientGetter, cmd.Parent().CommandPath(), debugPod.Namespace, debugPod.Name, containerName); err != nil { return err } } @@ -795,7 +781,25 @@ func (o *DebugOptions) waitForContainer(ctx context.Context, ns, podName, contai return result, err } -func (o *DebugOptions) handleAttachPod(ctx context.Context, restClientGetter genericclioptions.RESTClientGetter, ns, podName, containerName string, opts *attach.AttachOptions) error { +func (o *DebugOptions) handleAttachPod(ctx context.Context, restClientGetter genericclioptions.RESTClientGetter, cmdPath string, ns, podName, containerName string) error { + opts := &attach.AttachOptions{ + StreamOptions: exec.StreamOptions{ + IOStreams: o.IOStreams, + Stdin: o.Interactive, + TTY: o.TTY, + Quiet: o.Quiet, + }, + CommandName: cmdPath + " attach", + + Attach: &attach.DefaultRemoteAttach{}, + } + config, err := restClientGetter.ToRESTConfig() + if err != nil { + return err + } + opts.Config = config + opts.AttachFunc = attach.DefaultAttachFunc + pod, err := o.waitForContainer(ctx, ns, podName, containerName) if err != nil { return err diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug_test.go index 3febf5210f3..17a04803225 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug_test.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug_test.go @@ -2104,7 +2104,7 @@ func TestCompleteAndValidate(t *testing.T) { } if diff := cmp.Diff(tc.wantOpts, opts, cmpFilter, cmpopts.IgnoreFields(DebugOptions{}, - "attachChanged", "shareProcessedChanged", "podClient", "WarningPrinter", "Applier", "explicitNamespace", "Builder")); diff != "" { + "attachChanged", "shareProcessedChanged", "podClient", "WarningPrinter", "Applier", "explicitNamespace", "Builder", "AttachFunc")); diff != "" { t.Error("CompleteAndValidate unexpected diff in generated object: (-want +got):\n", diff) } }) From fdea6ad17d74c7f39e55d4ae2b57f2430c12eaba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Mon, 24 Jul 2023 15:32:26 +0300 Subject: [PATCH 2/3] kubectl debug: Remove legacy server support Legacy server support for ephemeral containers were added in kubetl debug in 1.22. Since now we are in 1.29, we can safely remove ephemeral container legacy server support because 1.22 is already far away from supported version skew boundary. --- .../src/k8s.io/kubectl/pkg/cmd/debug/debug.go | 41 ------------------- 1 file changed, 41 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go b/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go index 1066a86b8b8..72926150b89 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go @@ -455,53 +455,12 @@ func (o *DebugOptions) debugByEphemeralContainer(ctx context.Context, pod *corev return nil, "", fmt.Errorf("ephemeral containers are disabled for this cluster (error from server: %q)", err) } - // The Kind used for the /ephemeralcontainers subresource changed in 1.22. When presented with an unexpected - // Kind the api server will respond with a not-registered error. When this happens we can optimistically try - // using the old API. - if runtime.IsNotRegisteredError(err) { - klog.V(1).Infof("Falling back to legacy API because server returned error: %v", err) - return o.debugByEphemeralContainerLegacy(ctx, pod, debugContainer) - } - return nil, "", err } return result, debugContainer.Name, nil } -// debugByEphemeralContainerLegacy adds debugContainer as an ephemeral container using the pre-1.22 /ephemeralcontainers API -// This may be removed when we no longer wish to support releases prior to 1.22. -func (o *DebugOptions) debugByEphemeralContainerLegacy(ctx context.Context, pod *corev1.Pod, debugContainer *corev1.EphemeralContainer) (*corev1.Pod, string, error) { - // We no longer have the v1.EphemeralContainers Kind since it was removed in 1.22, but - // we can present a JSON 6902 patch that the api server will apply. - patch, err := json.Marshal([]map[string]interface{}{{ - "op": "add", - "path": "/ephemeralContainers/-", - "value": debugContainer, - }}) - if err != nil { - return nil, "", fmt.Errorf("error creating JSON 6902 patch for old /ephemeralcontainers API: %s", err) - } - - result := o.podClient.RESTClient().Patch(types.JSONPatchType). - Namespace(pod.Namespace). - Resource("pods"). - Name(pod.Name). - SubResource("ephemeralcontainers"). - Body(patch). - Do(ctx) - if err := result.Error(); err != nil { - return nil, "", err - } - - newPod, err := o.podClient.Pods(pod.Namespace).Get(ctx, pod.Name, metav1.GetOptions{}) - if err != nil { - return nil, "", err - } - - return newPod, debugContainer.Name, nil -} - // debugByCopy runs a copy of the target Pod with a debug container added or an original container modified func (o *DebugOptions) debugByCopy(ctx context.Context, pod *corev1.Pod) (*corev1.Pod, string, error) { copied, dc, err := o.generatePodCopyWithDebugContainer(pod) From fa37bebcd4b239cc5ad8eb3899d9868b015b34de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arda=20G=C3=BC=C3=A7l=C3=BC?= Date: Mon, 28 Aug 2023 08:36:32 +0300 Subject: [PATCH 3/3] Add comment about why AttachFunc be overridable --- staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go b/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go index 72926150b89..7f6a1d194fd 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go @@ -213,6 +213,11 @@ func (o *DebugOptions) Complete(restClientGetter genericclioptions.RESTClientGet attachFlag := cmd.Flags().Lookup("attach") if !attachFlag.Changed && o.Interactive { o.Attach = true + // Downstream tools may want to use their own customized + // attach function to do extra work or use attach command + // with different flags instead of the static one defined in + // handleAttachPod. But if this function is not set explicitly, + // we fall back to default. if o.AttachFunc == nil { o.AttachFunc = o.handleAttachPod }