From 7fd8647b0de2c592e2c93151d1b79b498fb54715 Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Fri, 9 Apr 2021 13:53:13 +0200 Subject: [PATCH 1/2] Switch ephemeralcontainers SR to Pod Kind This changes the `/ephemeralcontainers` subresource of `/pods` to use the `Pod` kind rather than `EphemeralContainers`. When designing this API initially it seemed preferable to create a new kind containing only the pod's ephemeral containers, similar to how binding and scaling work. It later became clear that this made admission control more difficult because the controller wouldn't be presented with the entire Pod, so we updated this to operate on the entire Pod, similar to how `/status` works. Kubernetes-commit: d22dc5cb72a627341f4004b5d58d275f3d8773b3 --- pkg/cmd/debug/debug.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/debug/debug.go b/pkg/cmd/debug/debug.go index abcb2530..28d34237 100644 --- a/pkg/cmd/debug/debug.go +++ b/pkg/cmd/debug/debug.go @@ -382,25 +382,22 @@ func (o *DebugOptions) visitPod(ctx context.Context, pod *corev1.Pod) (*corev1.P // debugByEphemeralContainer runs an EphemeralContainer in the target Pod for use as a debug container func (o *DebugOptions) debugByEphemeralContainer(ctx context.Context, pod *corev1.Pod) (*corev1.Pod, string, error) { pods := o.podClient.Pods(pod.Namespace) - ec, err := pods.GetEphemeralContainers(ctx, pod.Name, metav1.GetOptions{}) + klog.V(2).Infof("existing ephemeral containers: %v", pod.Spec.EphemeralContainers) + + debugContainer := o.generateDebugContainer(pod) + klog.V(2).Infof("new ephemeral container: %#v", debugContainer) + + pod.Spec.EphemeralContainers = append(pod.Spec.EphemeralContainers, *debugContainer) + result, err := pods.UpdateEphemeralContainers(ctx, pod.Name, pod, metav1.UpdateOptions{}) if err != nil { // The pod has already been fetched at this point, so a NotFound error indicates the ephemeralcontainers subresource wasn't found. if serr, ok := err.(*errors.StatusError); ok && serr.Status().Reason == metav1.StatusReasonNotFound { return nil, "", fmt.Errorf("ephemeral containers are disabled for this cluster (error from server: %q).", err) } - return nil, "", err - } - klog.V(2).Infof("existing ephemeral containers: %v", ec.EphemeralContainers) - - debugContainer := o.generateDebugContainer(pod) - klog.V(2).Infof("new ephemeral container: %#v", debugContainer) - ec.EphemeralContainers = append(ec.EphemeralContainers, *debugContainer) - _, err = pods.UpdateEphemeralContainers(ctx, pod.Name, ec, metav1.UpdateOptions{}) - if err != nil { return nil, "", fmt.Errorf("error updating ephemeral containers: %v", err) } - return pod, debugContainer.Name, nil + return result, debugContainer.Name, nil } // debugByCopy runs a copy of the target Pod with a debug container added or an original container modified From 46887fb25ebc715a9d7a551093e0816403409596 Mon Sep 17 00:00:00 2001 From: Lee Verberne Date: Wed, 14 Apr 2021 18:40:42 +0200 Subject: [PATCH 2/2] Address feedback for new /ephemeralcontainers API * Use deep copies in `PrepareForUpdate()` * Preserve select metadata from new pod * Use patch to add ephemeral container `kubectl debug` * Distinguish between pod vs /ephemeralcontainers NotFound Kubernetes-commit: 97726a50c138557522def7f753ec8581d00f0b02 --- pkg/cmd/debug/debug.go | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/debug/debug.go b/pkg/cmd/debug/debug.go index 28d34237..7d44b7db 100644 --- a/pkg/cmd/debug/debug.go +++ b/pkg/cmd/debug/debug.go @@ -18,6 +18,7 @@ package debug import ( "context" + "encoding/json" "fmt" "time" @@ -31,7 +32,9 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" utilrand "k8s.io/apimachinery/pkg/util/rand" + "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/apimachinery/pkg/watch" "k8s.io/cli-runtime/pkg/genericclioptions" "k8s.io/cli-runtime/pkg/resource" @@ -381,20 +384,36 @@ func (o *DebugOptions) visitPod(ctx context.Context, pod *corev1.Pod) (*corev1.P // debugByEphemeralContainer runs an EphemeralContainer in the target Pod for use as a debug container func (o *DebugOptions) debugByEphemeralContainer(ctx context.Context, pod *corev1.Pod) (*corev1.Pod, string, error) { - pods := o.podClient.Pods(pod.Namespace) klog.V(2).Infof("existing ephemeral containers: %v", pod.Spec.EphemeralContainers) + podJS, err := json.Marshal(pod) + if err != nil { + return nil, "", fmt.Errorf("error creating JSON for pod: %v", err) + } debugContainer := o.generateDebugContainer(pod) klog.V(2).Infof("new ephemeral container: %#v", debugContainer) - - pod.Spec.EphemeralContainers = append(pod.Spec.EphemeralContainers, *debugContainer) - result, err := pods.UpdateEphemeralContainers(ctx, pod.Name, pod, metav1.UpdateOptions{}) + debugPod := pod.DeepCopy() + debugPod.Spec.EphemeralContainers = append(debugPod.Spec.EphemeralContainers, *debugContainer) + debugJS, err := json.Marshal(debugPod) if err != nil { - // The pod has already been fetched at this point, so a NotFound error indicates the ephemeralcontainers subresource wasn't found. - if serr, ok := err.(*errors.StatusError); ok && serr.Status().Reason == metav1.StatusReasonNotFound { + return nil, "", fmt.Errorf("error creating JSON for debug container: %v", err) + } + + patch, err := strategicpatch.CreateTwoWayMergePatch(podJS, debugJS, pod) + if err != nil { + return nil, "", fmt.Errorf("error creating patch to add debug container: %v", err) + } + klog.V(2).Infof("generated strategic merge patch for debug container: %s", patch) + + pods := o.podClient.Pods(pod.Namespace) + result, err := pods.Patch(ctx, pod.Name, types.StrategicMergePatchType, patch, metav1.PatchOptions{}, "ephemeralcontainers") + if err != nil { + // The apiserver will return a 404 when the EphemeralContainers feature is disabled because the `/ephemeralcontainers` subresource + // is missing. Unlike the 404 returned by a missing pod, the status details will be empty. + if serr, ok := err.(*errors.StatusError); ok && serr.Status().Reason == metav1.StatusReasonNotFound && serr.ErrStatus.Details.Name == "" { return nil, "", fmt.Errorf("ephemeral containers are disabled for this cluster (error from server: %q).", err) } - return nil, "", fmt.Errorf("error updating ephemeral containers: %v", err) + return nil, "", err } return result, debugContainer.Name, nil