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

pkg/registry/core/pod/strategy.go

@@ -205,15 +205,22 @@ type podEphemeralContainersStrategy struct {
 // EphemeralContainersStrategy wraps and exports the used podStrategy for the storage package.
 var EphemeralContainersStrategy = podEphemeralContainersStrategy{Strategy}
 
+// dropNonEphemeralContainerUpdates discards all changes except for pod.Spec.EphemeralContainers and certain metadata
+func dropNonEphemeralContainerUpdates(newPod, oldPod *api.Pod) *api.Pod {
+	pod := oldPod.DeepCopy()
+	pod.Name = newPod.Name
+	pod.Namespace = newPod.Namespace
+	pod.ResourceVersion = newPod.ResourceVersion
+	pod.UID = newPod.UID
+	pod.Spec.EphemeralContainers = newPod.Spec.EphemeralContainers
+	return pod
+}
+
 func (podEphemeralContainersStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) {
 	newPod := obj.(*api.Pod)
 	oldPod := old.(*api.Pod)
 
-	// Drop all changes except for pod.Spec.EphemeralContainers
+	*newPod = *dropNonEphemeralContainerUpdates(newPod, oldPod)
-	ec := newPod.Spec.EphemeralContainers
-	*newPod = *oldPod
-	newPod.Spec.EphemeralContainers = ec
-
 	podutil.DropDisabledPodFields(newPod, oldPod)
 }
 

pkg/registry/core/pod/strategy_test.go

@@ -24,6 +24,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/require"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -1381,3 +1382,221 @@ func TestPodStrategyValidateUpdate(t *testing.T) {
 		})
 	}
 }
+
+func TestDropNonEphemeralContainerUpdates(t *testing.T) {
+	tests := []struct {
+		name                    string
+		oldPod, newPod, wantPod *api.Pod
+	}{
+		{
+			name: "simple ephemeral container append",
+			oldPod: &api.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "test-pod",
+					Namespace:       "test-ns",
+					ResourceVersion: "1",
+				},
+				Spec: api.PodSpec{
+					Containers: []api.Container{
+						{
+							Name:  "container",
+							Image: "image",
+						},
+					},
+				},
+			},
+			newPod: &api.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "test-pod",
+					Namespace:       "test-ns",
+					ResourceVersion: "1",
+				},
+				Spec: api.PodSpec{
+					Containers: []api.Container{
+						{
+							Name:  "container",
+							Image: "image",
+						},
+					},
+					EphemeralContainers: []api.EphemeralContainer{
+						{
+							EphemeralContainerCommon: api.EphemeralContainerCommon{
+								Name:  "container",
+								Image: "image",
+							},
+						},
+					},
+				},
+			},
+			wantPod: &api.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "test-pod",
+					Namespace:       "test-ns",
+					ResourceVersion: "1",
+				},
+				Spec: api.PodSpec{
+					Containers: []api.Container{
+						{
+							Name:  "container",
+							Image: "image",
+						},
+					},
+					EphemeralContainers: []api.EphemeralContainer{
+						{
+							EphemeralContainerCommon: api.EphemeralContainerCommon{
+								Name:  "container",
+								Image: "image",
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "whoops wrong pod",
+			oldPod: &api.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "test-pod",
+					Namespace:       "test-ns",
+					ResourceVersion: "1",
+					UID:             "blue",
+				},
+			},
+			newPod: &api.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "new-pod",
+					Namespace:       "new-ns",
+					ResourceVersion: "1",
+					UID:             "green",
+				},
+			},
+			wantPod: &api.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "new-pod",
+					Namespace:       "new-ns",
+					ResourceVersion: "1",
+					UID:             "green",
+				},
+			},
+		},
+		{
+			name: "resource conflict during update",
+			oldPod: &api.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "test-pod",
+					Namespace:       "test-ns",
+					ResourceVersion: "2",
+					UID:             "blue",
+				},
+			},
+			newPod: &api.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "test-pod",
+					Namespace:       "test-ns",
+					ResourceVersion: "1",
+					UID:             "blue",
+				},
+			},
+			wantPod: &api.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "test-pod",
+					Namespace:       "test-ns",
+					ResourceVersion: "1",
+					UID:             "blue",
+				},
+			},
+		},
+		{
+			name: "drop non-ephemeral container changes",
+			oldPod: &api.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "test-pod",
+					Namespace:       "test-ns",
+					ResourceVersion: "1",
+					Annotations:     map[string]string{"foo": "bar"},
+				},
+				Spec: api.PodSpec{
+					Containers: []api.Container{
+						{
+							Name:  "container",
+							Image: "image",
+						},
+					},
+				},
+			},
+			newPod: &api.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "test-pod",
+					Namespace:       "test-ns",
+					ResourceVersion: "1",
+					Annotations:     map[string]string{"foo": "bar", "whiz": "pop"},
+					ClusterName:     "milo",
+				},
+				Spec: api.PodSpec{
+					Containers: []api.Container{
+						{
+							Name:  "container",
+							Image: "newimage",
+						},
+					},
+					EphemeralContainers: []api.EphemeralContainer{
+						{
+							EphemeralContainerCommon: api.EphemeralContainerCommon{
+								Name:  "container1",
+								Image: "image",
+							},
+						},
+						{
+							EphemeralContainerCommon: api.EphemeralContainerCommon{
+								Name:  "container2",
+								Image: "image",
+							},
+						},
+					},
+				},
+				Status: api.PodStatus{
+					Message: "hi.",
+				},
+			},
+			wantPod: &api.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "test-pod",
+					Namespace:       "test-ns",
+					ResourceVersion: "1",
+					Annotations:     map[string]string{"foo": "bar"},
+				},
+				Spec: api.PodSpec{
+					Containers: []api.Container{
+						{
+							Name:  "container",
+							Image: "image",
+						},
+					},
+					EphemeralContainers: []api.EphemeralContainer{
+						{
+							EphemeralContainerCommon: api.EphemeralContainerCommon{
+								Name:  "container1",
+								Image: "image",
+							},
+						},
+						{
+							EphemeralContainerCommon: api.EphemeralContainerCommon{
+								Name:  "container2",
+								Image: "image",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			gotPod := dropNonEphemeralContainerUpdates(tc.newPod, tc.oldPod)
+			if diff := cmp.Diff(tc.wantPod, gotPod); diff != "" {
+				t.Errorf("unexpected diff when dropping fields (-want, +got):\n%s", diff)
+			}
+		})
+	}
+}

staging/src/k8s.io/kubectl/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)
-
+	debugPod := pod.DeepCopy()
-	pod.Spec.EphemeralContainers = append(pod.Spec.EphemeralContainers, *debugContainer)
+	debugPod.Spec.EphemeralContainers = append(debugPod.Spec.EphemeralContainers, *debugContainer)
-	result, err := pods.UpdateEphemeralContainers(ctx, pod.Name, pod, metav1.UpdateOptions{})
+	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.
+		return nil, "", fmt.Errorf("error creating JSON for debug container: %v", err)
-		if serr, ok := err.(*errors.StatusError); ok && serr.Status().Reason == metav1.StatusReasonNotFound {
+	}
+
+	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

test/integration/pods/pods_test.go

@@ -709,8 +709,9 @@ func TestPodEphemeralContainersDisabled(t *testing.T) {
 	}
 	pod, err := setUpEphemeralContainers(client.CoreV1().Pods(ns.Name), pod, nil)
 	if err != nil {
-		t.Error(err)
+		t.Fatal(err)
 	}
+	defer integration.DeletePodOrErrorf(t, client, ns.Name, pod.Name)
 
 	pod.Spec.EphemeralContainers = append(pod.Spec.EphemeralContainers, v1.EphemeralContainer{
 		EphemeralContainerCommon: v1.EphemeralContainerCommon{
@@ -721,11 +722,18 @@ func TestPodEphemeralContainersDisabled(t *testing.T) {
 		},
 	})
 
-	if _, err := client.CoreV1().Pods(ns.Name).UpdateEphemeralContainers(context.TODO(), pod.Name, pod, metav1.UpdateOptions{}); err == nil {
+	if _, err = client.CoreV1().Pods(ns.Name).UpdateEphemeralContainers(context.TODO(), pod.Name, pod, metav1.UpdateOptions{}); err == nil {
-		t.Errorf("got nil error when updating ephemeral containers with feature disabled, wanted %q", metav1.StatusReasonNotFound)
+		t.Fatalf("got nil error when updating ephemeral containers with feature disabled, wanted %q", metav1.StatusReasonNotFound)
-	} else if se := err.(*errors.StatusError); se.ErrStatus.Reason != metav1.StatusReasonNotFound {
-		t.Errorf("got error reason %q when updating ephemeral containers with feature disabled, want %q: %#v", se.ErrStatus.Reason, metav1.StatusReasonNotFound, se)
 	}
 
-	integration.DeletePodOrErrorf(t, client, ns.Name, pod.Name)
+	se, ok := err.(*errors.StatusError)
+	if !ok {
+		t.Fatalf("got error %#v, expected StatusError", err)
+	}
+	if se.ErrStatus.Reason != metav1.StatusReasonNotFound {
+		t.Errorf("got error reason %q when updating ephemeral containers with feature disabled, want %q: %#v", se.ErrStatus.Reason, metav1.StatusReasonNotFound, se)
+	}
+	if se.ErrStatus.Details.Name != "" {
+		t.Errorf("got error details with name %q, want %q: %#v", se.ErrStatus.Details.Name, "", se)
+	}
 }