From c267dfecb7ec673cb8410801c094d706a141e8b2 Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Wed, 5 Dec 2018 13:40:26 -0800 Subject: [PATCH] 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 | 16 +++++++++++++++- apis/duck/patch_test.go | 6 +++--- testing/resource.go | 19 ++++++++++++++++--- webhook/webhook.go | 12 +++++++++--- webhook/webhook_test.go | 30 ++++++++++++++++++++++++++++++ 5 files changed, 73 insertions(+), 10 deletions(-) diff --git a/apis/duck/patch.go b/apis/duck/patch.go index 386aa1f32..762479e24 100644 --- a/apis/duck/patch.go +++ b/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 diff --git a/apis/duck/patch_test.go b/apis/duck/patch_test.go index d63cbd5fe..546136bf7 100644 --- a/apis/duck/patch_test.go +++ b/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", diff --git a/testing/resource.go b/testing/resource.go index 8eb51f2c5..590a17ee7 100644 --- a/testing/resource.go +++ b/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 } diff --git a/webhook/webhook.go b/webhook/webhook.go index 6ca418214..69fa2af9b 100644 --- a/webhook/webhook.go +++ b/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 } diff --git a/webhook/webhook_test.go b/webhook/webhook_test.go index 51c120a20..3ff6af050 100644 --- a/webhook/webhook_test.go +++ b/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)