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)