✨ Fix roundtripping issue when genearting JSON patch

example/mutatingwebhook.go

@@ -50,7 +50,7 @@ func (a *podAnnotator) Handle(ctx context.Context, req types.Request) types.Resp
 	if err != nil {
 		return admission.ErrorResponse(http.StatusInternalServerError, err)
 	}
-	return admission.PatchResponse(pod, copy)
+	return admission.PatchResponse(pod, copy, req.AdmissionRequest.Object.Raw...)
 }
 
 // mutatePodsFn add an annotation to the given pod

pkg/patch/patch.go

@@ -26,16 +26,24 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
-// NewJSONPatch calculates the JSON patch between original and current objects.
-func NewJSONPatch(original, current runtime.Object) ([]jsonpatch.JsonPatchOperation, error) {
+// NewJSONPatch calculates the JSON patch between original and current objects and
+// an optional original object in raw bytes format.
+// If the original raw object is provided, it will be used to calculate json patch.
+// It is STRONGLY recommended to use the original raw object to avoid the roundtripping
+// issue for non-pointer fields.
+func NewJSONPatch(original, current runtime.Object, originalRaw ...byte) ([]jsonpatch.JsonPatchOperation, error) {
 	originalGVK := original.GetObjectKind().GroupVersionKind()
 	currentGVK := current.GetObjectKind().GroupVersionKind()
 	if !reflect.DeepEqual(originalGVK, currentGVK) {
 		return nil, fmt.Errorf("GroupVersionKind %#v is expected to match %#v", originalGVK, currentGVK)
 	}
-	ori, err := json.Marshal(original)
-	if err != nil {
-		return nil, err
+	ori := originalRaw
+	if len(ori) == 0 {
+		var err error
+		ori, err = json.Marshal(original)
+		if err != nil {
+			return nil, err
+		}
 	}
 	cur, err := json.Marshal(current)
 	if err != nil {

pkg/patch/patch_test.go

@@ -0,0 +1,192 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package patch_test
+
+import (
+	"encoding/json"
+	"fmt"
+	"reflect"
+	"testing"
+
+	jpatch "github.com/evanphx/json-patch"
+	"github.com/mattbaird/jsonpatch"
+
+	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/patch"
+)
+
+type testcase struct {
+	original    runtime.Object
+	current     runtime.Object
+	originalRaw []byte
+	// expectedPatchesFromRaw is the expected json patches when using originalRaw
+	expectedPatchesFromRaw []jsonpatch.JsonPatchOperation
+	// expectedPatchesFromRaw is the expected json patches when NOT using originalRaw
+	expectedPatches []jsonpatch.JsonPatchOperation
+}
+
+var testcases = []testcase{
+	{
+		original: &appsv1.Deployment{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: "apps/v1",
+				Kind:       "Deployment",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "test-deployment",
+			},
+		},
+		current: &appsv1.Deployment{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: "apps/v1",
+				Kind:       "Deployment",
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "test-deployment",
+			},
+			Spec: appsv1.DeploymentSpec{
+				Replicas: func(x int32) *int32 { return &x }(1),
+				Selector: &metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"foo": "bar",
+					},
+				},
+				Template: corev1.PodTemplateSpec{},
+				Strategy: appsv1.DeploymentStrategy{
+					Type: appsv1.RecreateDeploymentStrategyType,
+				},
+			},
+		},
+		originalRaw: []byte(`{"apiVersion":"apps/v1", "kind":"Deployment", "metadata": {"creationTimestamp":null, "name": "test-deployment"}, "status": {}}`),
+		expectedPatchesFromRaw: []jsonpatch.JsonPatchOperation{
+			{
+				Operation: "add",
+				Path:      "/spec",
+				Value: map[string]interface{}{
+					"replicas": 1.0,
+					"selector": map[string]interface{}{
+						"matchLabels": map[string]interface{}{
+							"foo": "bar",
+						},
+					},
+					"strategy": map[string]interface{}{
+						"type": "Recreate",
+					},
+					"template": map[string]interface{}{
+						"metadata": map[string]interface{}{
+							"creationTimestamp": nil,
+						},
+						"spec": map[string]interface{}{
+							"containers": nil,
+						},
+					},
+				},
+			},
+		},
+		expectedPatches: []jsonpatch.JsonPatchOperation{
+			{
+				Operation: "add",
+				Path:      "/spec/replicas",
+				// float64 will be used by default in the generated JSON patch
+				Value: 1.0,
+			},
+			{
+				Operation: "replace",
+				Path:      "/spec/selector",
+				Value: map[string]interface{}{
+					"matchLabels": map[string]interface{}{
+						"foo": "bar",
+					},
+				},
+			},
+			{
+				Operation: "add",
+				Path:      "/spec/strategy/type",
+				Value:     "Recreate",
+			},
+		},
+	},
+}
+
+func TestJSONPatchFromRaw(t *testing.T) {
+	for _, tc := range testcases {
+		patches, err := patch.NewJSONPatch(tc.original, tc.current, tc.originalRaw...)
+		assertNoError(err, t)
+
+		err = arrayEqual(tc.expectedPatchesFromRaw, patches)
+		assertNoError(err, t)
+
+		// Applying the patch to the original raw object should yield the desired object
+		patchByte, err := json.Marshal(patches)
+		assertNoError(err, t)
+
+		p, err := jpatch.DecodePatch(patchByte)
+		assertNoError(err, t)
+
+		modified, err := p.Apply(tc.originalRaw)
+		assertNoError(err, t)
+
+		var appliedObject *appsv1.Deployment
+		err = json.Unmarshal(modified, &appliedObject)
+		assertNoError(err, t)
+
+		if !reflect.DeepEqual(tc.current, appliedObject) {
+			t.Fatalf("expect object:\n%#v,\nbut got:\n%#v", tc.current, appliedObject)
+		}
+	}
+}
+
+func TestJSONPatch(t *testing.T) {
+	for _, tc := range testcases {
+		patches, err := patch.NewJSONPatch(tc.original, tc.current)
+		assertNoError(err, t)
+
+		err = arrayEqual(tc.expectedPatches, patches)
+		assertNoError(err, t)
+	}
+}
+
+func arrayEqual(expected, got []jsonpatch.JsonPatchOperation) error {
+	if len(expected) != len(got) {
+		return fmt.Errorf("expected to have length %d, but got length %d", len(expected), len(got))
+	}
+	for _, e := range expected {
+		found := false
+		for _, g := range got {
+			if e.Path == g.Path {
+				if !reflect.DeepEqual(e, g) {
+					return fmt.Errorf("expected: %#v,\nbut got: %#v", e, g)
+				}
+				found = true
+				break
+			}
+		}
+		if !found {
+			return fmt.Errorf("expected entry: %#v but didn't get it", e)
+		}
+	}
+	return nil
+}
+
+func assertNoError(err error, t *testing.T) {
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}

pkg/webhook/admission/response.go

@@ -55,8 +55,10 @@ func ValidationResponse(allowed bool, reason string) types.Response {
 }
 
 // PatchResponse returns a new response with json patch.
-func PatchResponse(original, current runtime.Object) types.Response {
-	patches, err := patch.NewJSONPatch(original, current)
+// originalRaw is optional. If provided, it will be used to calculate json patch.
+// It is STRONGLY recommended to use it to avoid the roundtripping issue for non-pointer fields.
+func PatchResponse(original, current runtime.Object, originalRaw ...byte) types.Response {
+	patches, err := patch.NewJSONPatch(original, current, originalRaw...)
 	if err != nil {
 		return ErrorResponse(http.StatusInternalServerError, err)
 	}