diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index bee96024122..0f411af7128 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/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 - ec := newPod.Spec.EphemeralContainers - *newPod = *oldPod - newPod.Spec.EphemeralContainers = ec - + *newPod = *dropNonEphemeralContainerUpdates(newPod, oldPod) podutil.DropDisabledPodFields(newPod, oldPod) } diff --git a/pkg/registry/core/pod/strategy_test.go b/pkg/registry/core/pod/strategy_test.go index bc2bf1b3528..cd3b1bbe55e 100644 --- a/pkg/registry/core/pod/strategy_test.go +++ b/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) + } + }) + } +} 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 28d34237c03..7d44b7dbf75 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/debug/debug.go +++ b/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) - - 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 diff --git a/test/integration/pods/pods_test.go b/test/integration/pods/pods_test.go index 1ed8a0384c4..404994cde90 100644 --- a/test/integration/pods/pods_test.go +++ b/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 { - t.Errorf("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) + if _, err = client.CoreV1().Pods(ns.Name).UpdateEphemeralContainers(context.TODO(), pod.Name, pod, metav1.UpdateOptions{}); err == nil { + t.Fatalf("got nil error when updating ephemeral containers with feature disabled, wanted %q", metav1.StatusReasonNotFound) } - 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) + } }