[bug] mutating webhook admission failed because of wrong json patch

[FillZpp]

Take an example

1. Define a CRD, spec like this:

type MyResourceSpec struct {
        Foo string `json:"foo,omitempty"`
        Bar MyResourceBar `json:"myResourceBar,omitempty"`
}

type MyResourceBar struct {
        Baz string `json:"baz,omitempty"`
}

2. Generate a mutating webhook, the handler like this:

func (h *MyResourceCreateUpdateHandler) Handle(ctx context.Context, req types.Request) types.Response {
	obj := &appsv1beta1.MyResource{}

	err := h.Decoder.Decode(req, obj)
	if err != nil {
		return admission.ErrorResponse(http.StatusBadRequest, err)
	}
	copy := obj.DeepCopy()

	err = h.mutatingMyResourceFn(ctx, copy)
	if err != nil {
		return admission.ErrorResponse(http.StatusInternalServerError, err)
	}
	return admission.PatchResponse(obj, copy)
}

3. In mutatingMyResourceFn, i try to set value for baz:

func (h *MyResourceCreateUpdateHandler) mutatingMyResourceFn(ctx context.Context, obj *appsv1beta1.MyResource) error {
	obj.Spec.Bar.Baz = "test"

	return nil
}

4. After installing and deploying, i try to create a CR:

apiVersion: apps.fillzpp.org/v1beta1
kind: MyResource
metadata:
  labels:
    controller-tools.k8s.io: "1.0"
  name: test-myresource
spec:
  foo: foo1

5. Create failed, error like this:

Error from server (InternalError): error when creating "test.yaml":
 Internal error occurred: Internal error occurred: 
jsonpatch add operation does not apply: doc is missing path: /spec/bar/baz

This is because my test.yaml has no bar in spec, but admission ask to add /spec/bar/baz.

My opinion

admission.PatchResponse should use req.AdmissionRequest.Object.Raw as original object to prepare with current object.

The problem is, the obj in handler contains some fields/objects no existing in original object raw. So when we compare obj with current object, we might get the wrong json patch, and apiserver will report such error message.

The correct code like this

controller-runtime

// PatchResponse returns a new response with json patch.
func PatchResponseWithRaw(originalRaw []byte, original, current runtime.Object) types.Response {
	patches, err := patch.newJSONPatchWithRaw(originalRaw, original, current)
	if err != nil {
		return admission.ErrorResponse(http.StatusInternalServerError, err)
	}
	return types.Response{
		Patches: patches,
		Response: &admissionv1beta1.AdmissionResponse{
			Allowed:   true,
			PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(),
		},
	}
}

// NewJSONPatch calculates the JSON patch between original and current objects.
func NewJSONPatchWithRaw(originalRaw []byte, original, current runtime.Object) ([]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)
	}
	cur, err := json.Marshal(current)
	if err != nil {
		return nil, err
	}
	return jsonpatch.CreatePatch(originalRaw, cur)
}

mutating admission handler use new admission.PatchResponseWithRaw

func (h *MyResourceCreateUpdateHandler) Handle(ctx context.Context, req types.Request) types.Response {
	obj := &appsv1beta1.MyResource{}

	err := h.Decoder.Decode(req, obj)
	if err != nil {
		return admission.ErrorResponse(http.StatusBadRequest, err)
	}
	copy := obj.DeepCopy()

	err = h.mutatingMyResourceFn(ctx, copy)
	if err != nil {
		return admission.ErrorResponse(http.StatusInternalServerError, err)
	}
	return admission.PatchResponseWithRaw(req.AdmissionRequest.Object.Raw, obj, copy)
}

[mengqiy]

@FillZpp Thanks for your suggestion.
It looks very likely your suggested approach will work.