From 46887fb25ebc715a9d7a551093e0816403409596 Mon Sep 17 00:00:00 2001
From: Lee Verberne <verb@google.com>
Date: Wed, 14 Apr 2021 18:40:42 +0200
Subject: [PATCH] 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 28d34237c..7d44b7dbf 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