This exempts defaulting from the rules governing field immutability. (#191)

Immutable fields with default values may now be changed iff they change is to populate their default value.  This is to support defaulting in the scenario where an object was created long ago and a new field (with a default!) is added.  When controllers attempt to mutate the object status today, this would create a webhook rejection!  With this change, we compare against a freshly defaulted "old" object to exclude newly defaulted fields from the immutability check.

We saw this in knative/serving for the newly added TimeoutSeconds field in Revision (otherwise immutable), which I believe it leading to upgrade testing flakes since post-upgrade Revision status updates will fail.

apis/duck/patch.go

@@ -18,6 +18,7 @@ package duck
 
 import (
 	"encoding/json"
+	"sort"
 
 	jsonmergepatch "github.com/evanphx/json-patch"
 	"github.com/mattbaird/jsonpatch"
@@ -50,7 +51,20 @@ func CreatePatch(before, after interface{}) (JSONPatch, error) {
 	if err != nil {
 		return nil, err
 	}
-	return jsonpatch.CreatePatch(rawBefore, rawAfter)
+	patch, err := jsonpatch.CreatePatch(rawBefore, rawAfter)
+	if err != nil {
+		return nil, err
+	}
+
+	// Give the patch a deterministic ordering.
+	sort.Slice(patch, func(i, j int) bool {
+		lhs, rhs := patch[i], patch[j]
+		if lhs.Operation != rhs.Operation {
+			return lhs.Operation < rhs.Operation
+		}
+		return lhs.Path < rhs.Path
+	})
+	return patch, nil
 }
 
 type JSONPatch []jsonpatch.JsonPatchOperation

apis/duck/patch_test.go

@@ -163,12 +163,12 @@ func TestCreatePatch(t *testing.T) {
 			},
 		},
 		want: JSONPatch{{
+			Operation: "remove",
+			Path:      "/status/patchable/field2",
+		}, {
 			Operation: "replace",
 			Path:      "/status/patchable/field1",
 			Value:     42.0,
-		}, {
-			Operation: "remove",
-			Path:      "/status/patchable/field2",
 		}},
 	}, {
 		name: "patch array",

testing/resource.go

@@ -45,9 +45,10 @@ var _ apis.Listable = (*Resource)(nil)
 type ResourceSpec struct {
 	Generation int64 `json:"generation,omitempty"`
 
-	FieldWithDefault    string `json:"fieldWithDefault,omitempty"`
-	FieldWithValidation string `json:"fieldWithValidation,omitempty"`
-	FieldThatsImmutable string `json:"fieldThatsImmutable,omitempty"`
+	FieldWithDefault               string `json:"fieldWithDefault,omitempty"`
+	FieldWithValidation            string `json:"fieldWithValidation,omitempty"`
+	FieldThatsImmutable            string `json:"fieldThatsImmutable,omitempty"`
+	FieldThatsImmutableWithDefault string `json:"fieldThatsImmutableWithDefault,omitempty"`
 }
 
 func (c *Resource) SetDefaults() {
@@ -58,6 +59,9 @@ func (cs *ResourceSpec) SetDefaults() {
 	if cs.FieldWithDefault == "" {
 		cs.FieldWithDefault = "I'm a default."
 	}
+	if cs.FieldThatsImmutableWithDefault == "" {
+		cs.FieldThatsImmutableWithDefault = "this is another default value"
+	}
 }
 
 func (c *Resource) Validate() *apis.FieldError {
@@ -85,6 +89,15 @@ func (current *Resource) CheckImmutableFields(og apis.Immutable) *apis.FieldErro
 				original.Spec.FieldThatsImmutable),
 		}
 	}
+
+	if original.Spec.FieldThatsImmutableWithDefault != current.Spec.FieldThatsImmutableWithDefault {
+		return &apis.FieldError{
+			Message: "Immutable field changed",
+			Paths:   []string{"spec.fieldThatsImmutableWithDefault"},
+			Details: fmt.Sprintf("got: %v, want: %v", current.Spec.FieldThatsImmutableWithDefault,
+				original.Spec.FieldThatsImmutableWithDefault),
+		}
+	}
 	return nil
 }
 

webhook/webhook.go

@@ -204,12 +204,17 @@ func getOrGenerateKeyCertsFromSecret(ctx context.Context, client kubernetes.Inte
 // it then delegates validation to apis.Validatable on "new".
 func Validate(ctx context.Context) ResourceCallback {
 	return func(patches *[]jsonpatch.JsonPatchOperation, old GenericCRD, new GenericCRD) error {
-		if hifNew, ok := new.(apis.Immutable); ok && old != nil {
-			hifOld, ok := old.(apis.Immutable)
+		if immutableNew, ok := new.(apis.Immutable); ok && old != nil {
+			// Copy the old object and set defaults so that we don't reject our own
+			// defaulting done earlier in the webhook.
+			old = old.DeepCopyObject().(GenericCRD)
+			old.SetDefaults()
+
+			immutableOld, ok := old.(apis.Immutable)
 			if !ok {
 				return fmt.Errorf("unexpected type mismatch %T vs. %T", old, new)
 			}
-			if err := hifNew.CheckImmutableFields(hifOld); err != nil {
+			if err := immutableNew.CheckImmutableFields(immutableOld); err != nil {
 				return err
 			}
 		}
@@ -231,6 +236,7 @@ func SetDefaults(ctx context.Context) ResourceDefaulter {
 		if err != nil {
 			return err
 		}
+
 		*patches = append(*patches, patch...)
 		return nil
 	}

webhook/webhook_test.go

@@ -131,6 +131,10 @@ func TestValidCreateResourceSucceedsWithDefaultPatch(t *testing.T) {
 	expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{
 		incrementGenerationPatch(r.Spec.Generation),
 		{
+			Operation: "add",
+			Path:      "/spec/fieldThatsImmutableWithDefault",
+			Value:     "this is another default value",
+		}, {
 			Operation: "add",
 			Path:      "/spec/fieldWithDefault",
 			Value:     "I'm a default.",
@@ -171,6 +175,10 @@ func TestValidUpdateResourceSucceeds(t *testing.T) {
 		Operation: "replace",
 		Path:      "/spec/generation",
 		Value:     1235.0,
+	}, {
+		Operation: "add",
+		Path:      "/spec/fieldThatsImmutableWithDefault",
+		Value:     "this is another default value",
 	}, {
 		Operation: "add",
 		Path:      "/spec/fieldWithDefault",
@@ -196,12 +204,34 @@ func TestInvalidUpdateResourceFailsImmutability(t *testing.T) {
 
 	// Try to change the value
 	new.Spec.FieldThatsImmutable = "a different value"
+	new.Spec.FieldThatsImmutableWithDefault = "another different value"
 
 	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
 	resp := ac.admit(TestContextWithLogger(t), createUpdateResource(&old, &new))
 	expectFailsWith(t, resp, "Immutable field changed")
 }
 
+func TestDefaultingImmutableFields(t *testing.T) {
+	old := createResource(1234, "a name")
+	new := createResource(1234, "a name")
+
+	// If we don't specify the new, but immutable field, we default it,
+	// and it is not rejected.
+
+	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
+	resp := ac.admit(TestContextWithLogger(t), createUpdateResource(&old, &new))
+	expectAllowed(t, resp)
+	expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{{
+		Operation: "add",
+		Path:      "/spec/fieldThatsImmutableWithDefault",
+		Value:     "this is another default value",
+	}, {
+		Operation: "add",
+		Path:      "/spec/fieldWithDefault",
+		Value:     "I'm a default.",
+	}})
+}
+
 func TestValidWebhook(t *testing.T) {
 	_, ac := newNonRunningTestAdmissionController(t, newDefaultOptions())
 	createDeployment(ac)