From 0183bf9cdc7349e3c76c26e51f84689b411fccea Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Thu, 14 Feb 2019 11:29:45 -0500 Subject: [PATCH] Drop spec.generation support (#234) * Drop webhook logic to increment spec.generation With Kubernetes 1.11+ metadata.generation now increments properly when the status subresource is enabled on CRDs For more details see: https://github.com/knative/serving/issues/643 * Drop the generational duck type --- apis/duck/typed_test.go | 34 +++--- apis/duck/v1alpha1/generational_types.go | 82 -------------- apis/duck/v1alpha1/implements_test.go | 2 - apis/duck/v1alpha1/register.go | 2 - apis/duck/v1alpha1/zz_generated.deepcopy.go | 76 ------------- testing/resource.go | 3 - testing/resource_test.go | 31 ------ webhook/webhook.go | 110 ------------------ webhook/webhook_integration_test.go | 17 +-- webhook/webhook_test.go | 117 +++----------------- 10 files changed, 37 insertions(+), 437 deletions(-) delete mode 100644 apis/duck/v1alpha1/generational_types.go delete mode 100644 testing/resource_test.go diff --git a/apis/duck/typed_test.go b/apis/duck/typed_test.go index c7f879c2cb..38ac5a8b9b 100644 --- a/apis/duck/typed_test.go +++ b/apis/duck/typed_test.go @@ -40,8 +40,8 @@ func TestSimpleList(t *testing.T) { AddToScheme(scheme) duckv1alpha1.AddToScheme(scheme) - namespace, name := "foo", "bar" - var want int64 = 1234 + namespace, name, want := "foo", "bar", "my_hostname" + // Despite the signature allowing `...runtime.Object`, this method // will not work properly unless the passed objects are `unstructured.Unstructured` client := fake.NewSimpleDynamicClient(scheme, &unstructured.Unstructured{ @@ -52,8 +52,10 @@ func TestSimpleList(t *testing.T) { "namespace": namespace, "name": name, }, - "spec": map[string]interface{}{ - "generation": want, + "status": map[string]interface{}{ + "address": map[string]interface{}{ + "hostname": want, + }, }, }, }) @@ -63,7 +65,7 @@ func TestSimpleList(t *testing.T) { tif := &duck.TypedInformerFactory{ Client: client, - Type: &duckv1alpha1.Generational{}, + Type: &duckv1alpha1.AddressableType{}, ResyncPeriod: 1 * time.Second, StopChannel: stopCh, } @@ -80,13 +82,13 @@ func TestSimpleList(t *testing.T) { t.Fatalf("Get() = %v", err) } - got, ok := elt.(*duckv1alpha1.Generational) + got, ok := elt.(*duckv1alpha1.AddressableType) if !ok { - t.Fatalf("Get() = %T, wanted *duckv1alpha1.Generational", elt) + t.Fatalf("Get() = %T, wanted *duckv1alpha1.AddressableType", elt) } - if want != int64(got.Spec.Generation) { - t.Errorf("Get().Spec.Generation = %v, wanted %v", got.Spec.Generation, want) + if gotHostname := got.Status.Address.Hostname; gotHostname != want { + t.Errorf("Get().Status.Address.Hostname = %v, wanted %v", gotHostname, want) } // TODO(mattmoor): Access through informer @@ -98,7 +100,7 @@ func TestAsStructuredWatcherNestedError(t *testing.T) { return nil, want } - wf := duck.AsStructuredWatcher(nwf, &duckv1alpha1.Generational{}) + wf := duck.AsStructuredWatcher(nwf, &duckv1alpha1.AddressableType{}) _, got := wf(metav1.ListOptions{}) if got != want { @@ -111,7 +113,7 @@ func TestAsStructuredWatcherClosedChannel(t *testing.T) { return watch.NewEmptyWatch(), nil } - wf := duck.AsStructuredWatcher(nwf, &duckv1alpha1.Generational{}) + wf := duck.AsStructuredWatcher(nwf, &duckv1alpha1.AddressableType{}) wi, err := wf(metav1.ListOptions{}) if err != nil { @@ -132,7 +134,7 @@ func TestAsStructuredWatcherPassThru(t *testing.T) { return duck.NewProxyWatcher(unstructuredCh), nil } - wf := duck.AsStructuredWatcher(nwf, &duckv1alpha1.Generational{}) + wf := duck.AsStructuredWatcher(nwf, &duckv1alpha1.AddressableType{}) wi, err := wf(metav1.ListOptions{}) if err != nil { @@ -159,13 +161,13 @@ func TestAsStructuredWatcherPassThru(t *testing.T) { select { case x, ok := <-ch: if !ok { - t.Fatal("<-ch = closed, wanted *duckv1alpha1.Generational{}") + t.Fatal("<-ch = closed, wanted *duckv1alpha1.AddressableType{}") } if got := x.Type; got != want { t.Errorf("x.Type = %v, wanted %v", got, want) } - if _, ok := x.Object.(*duckv1alpha1.Generational); !ok { - t.Errorf("<-ch = %T, wanted %T", x, &duckv1alpha1.Generational{}) + if _, ok := x.Object.(*duckv1alpha1.AddressableType); !ok { + t.Errorf("<-ch = %T, wanted %T", x, &duckv1alpha1.AddressableType{}) } case <-time.After(100 * time.Millisecond): t.Error("Didn't see expected message on channel.") @@ -178,7 +180,7 @@ func TestAsStructuredWatcherPassThruErrors(t *testing.T) { return duck.NewProxyWatcher(unstructuredCh), nil } - wf := duck.AsStructuredWatcher(nwf, &duckv1alpha1.Generational{}) + wf := duck.AsStructuredWatcher(nwf, &duckv1alpha1.AddressableType{}) wi, err := wf(metav1.ListOptions{}) if err != nil { diff --git a/apis/duck/v1alpha1/generational_types.go b/apis/duck/v1alpha1/generational_types.go deleted file mode 100644 index 13ef018038..0000000000 --- a/apis/duck/v1alpha1/generational_types.go +++ /dev/null @@ -1,82 +0,0 @@ -/* -Copyright 2018 The Knative 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 v1alpha1 - -import ( - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - - "github.com/knative/pkg/apis" - "github.com/knative/pkg/apis/duck" -) - -// Generation is the schema for the generational portion of the payload -type Generation int64 - -// Generation is an Implementable "duck type". -var _ duck.Implementable = (*Generation)(nil) - -// +genclient -// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object - -// Generational is a skeleton type wrapping Generation in the manner we expect -// resource writers defining compatible resources to embed it. We will -// typically use this type to deserialize Generation ObjectReferences and -// access the Generation data. This is not a real resource. -type Generational struct { - metav1.TypeMeta `json:",inline"` - metav1.ObjectMeta `json:"metadata,omitempty"` - - Spec GenerationalSpec `json:"spec"` -} - -// GenerationalSpec shows how we expect folks to embed Generation in -// their Spec field. -type GenerationalSpec struct { - Generation Generation `json:"generation,omitempty"` -} - -// In order for Generation to be Implementable, Generational must be Populatable. -var _ duck.Populatable = (*Generational)(nil) - -// Ensure Generational satisfies apis.Listable -var _ apis.Listable = (*Generational)(nil) - -// GetFullType implements duck.Implementable -func (_ *Generation) GetFullType() duck.Populatable { - return &Generational{} -} - -// Populate implements duck.Populatable -func (t *Generational) Populate() { - t.Spec.Generation = 1234 -} - -// GetListType implements apis.Listable -func (r *Generational) GetListType() runtime.Object { - return &GenerationalList{} -} - -// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object - -// GenerationalList is a list of Generational resources -type GenerationalList struct { - metav1.TypeMeta `json:",inline"` - metav1.ListMeta `json:"metadata"` - - Items []Generational `json:"items"` -} diff --git a/apis/duck/v1alpha1/implements_test.go b/apis/duck/v1alpha1/implements_test.go index 759b2fc580..1e9baad255 100644 --- a/apis/duck/v1alpha1/implements_test.go +++ b/apis/duck/v1alpha1/implements_test.go @@ -23,14 +23,12 @@ import ( ) func TestTypesImplements(t *testing.T) { - var emptyGen Generation testCases := []struct { instance interface{} iface duck.Implementable }{ {instance: &AddressableType{}, iface: &Addressable{}}, {instance: &KResource{}, iface: &Conditions{}}, - {instance: &Generational{}, iface: &emptyGen}, {instance: &LegacyTarget{}, iface: &LegacyTargetable{}}, {instance: &Target{}, iface: &Targetable{}}, } diff --git a/apis/duck/v1alpha1/register.go b/apis/duck/v1alpha1/register.go index a0264e576e..4bb344f2ac 100644 --- a/apis/duck/v1alpha1/register.go +++ b/apis/duck/v1alpha1/register.go @@ -47,8 +47,6 @@ func addKnownTypes(scheme *runtime.Scheme) error { SchemeGroupVersion, &KResource{}, (&KResource{}).GetListType(), - &Generational{}, - (&Generational{}).GetListType(), &AddressableType{}, (&AddressableType{}).GetListType(), &Target{}, diff --git a/apis/duck/v1alpha1/zz_generated.deepcopy.go b/apis/duck/v1alpha1/zz_generated.deepcopy.go index 731c7059dc..45f285d7ea 100644 --- a/apis/duck/v1alpha1/zz_generated.deepcopy.go +++ b/apis/duck/v1alpha1/zz_generated.deepcopy.go @@ -160,82 +160,6 @@ func (in Conditions) DeepCopy() Conditions { return *out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *Generational) DeepCopyInto(out *Generational) { - *out = *in - out.TypeMeta = in.TypeMeta - in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) - out.Spec = in.Spec - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Generational. -func (in *Generational) DeepCopy() *Generational { - if in == nil { - return nil - } - out := new(Generational) - in.DeepCopyInto(out) - return out -} - -// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. -func (in *Generational) DeepCopyObject() runtime.Object { - if c := in.DeepCopy(); c != nil { - return c - } - return nil -} - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *GenerationalList) DeepCopyInto(out *GenerationalList) { - *out = *in - out.TypeMeta = in.TypeMeta - out.ListMeta = in.ListMeta - if in.Items != nil { - in, out := &in.Items, &out.Items - *out = make([]Generational, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } - } - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GenerationalList. -func (in *GenerationalList) DeepCopy() *GenerationalList { - if in == nil { - return nil - } - out := new(GenerationalList) - in.DeepCopyInto(out) - return out -} - -// DeepCopyObject is an autogenerated deepcopy function, copying the receiver, creating a new runtime.Object. -func (in *GenerationalList) DeepCopyObject() runtime.Object { - if c := in.DeepCopy(); c != nil { - return c - } - return nil -} - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *GenerationalSpec) DeepCopyInto(out *GenerationalSpec) { - *out = *in - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GenerationalSpec. -func (in *GenerationalSpec) DeepCopy() *GenerationalSpec { - if in == nil { - return nil - } - out := new(GenerationalSpec) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *KResource) DeepCopyInto(out *KResource) { *out = *in diff --git a/testing/resource.go b/testing/resource.go index 816374c8c4..e7b6ec5b6d 100644 --- a/testing/resource.go +++ b/testing/resource.go @@ -52,10 +52,7 @@ var _ apis.Annotatable = (*Resource)(nil) var _ apis.Listable = (*Resource)(nil) // ResourceSpec represents test resource spec. -// TODO: Check that we implement the Generation duck type. type ResourceSpec struct { - Generation int64 `json:"generation,omitempty"` - FieldWithDefault string `json:"fieldWithDefault,omitempty"` FieldWithValidation string `json:"fieldWithValidation,omitempty"` FieldThatsImmutable string `json:"fieldThatsImmutable,omitempty"` diff --git a/testing/resource_test.go b/testing/resource_test.go deleted file mode 100644 index 4f829b0e45..0000000000 --- a/testing/resource_test.go +++ /dev/null @@ -1,31 +0,0 @@ -/* -Copyright 2018 The Knative 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 testing - -import ( - "testing" - - "github.com/knative/pkg/apis/duck" - duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" -) - -func TestResourceImplementsGenerational(t *testing.T) { - var emptyGen duckv1alpha1.Generation - if err := duck.VerifyType(&Resource{}, &emptyGen); err != nil { - t.Error(err) - } -} diff --git a/webhook/webhook.go b/webhook/webhook.go index 93ac4fb2ac..26d2961966 100644 --- a/webhook/webhook.go +++ b/webhook/webhook.go @@ -33,7 +33,6 @@ import ( "github.com/knative/pkg/apis" "github.com/knative/pkg/apis/duck" - duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" "github.com/knative/pkg/kmp" "github.com/knative/pkg/logging" "github.com/knative/pkg/logging/logkey" @@ -304,15 +303,6 @@ func (ac *AdmissionController) Run(stop <-chan struct{}) error { logger.Infof("Delaying admission webhook registration for %v", ac.Options.RegistrationDelay) } - // Verify that each of the types we are given implements the Generation duck type. - for _, crd := range ac.Handlers { - cp := crd.DeepCopyObject() - var emptyGen duckv1alpha1.Generation - if err := duck.VerifyType(cp, &emptyGen); err != nil { - return err - } - } - select { case <-time.After(ac.Options.RegistrationDelay): cl := ac.Client.AdmissionregistrationV1beta1().MutatingWebhookConfigurations() @@ -553,11 +543,6 @@ func (ac *AdmissionController) mutate(ctx context.Context, req *admissionv1beta1 } patches = append(patches, rtp...) - if patches, err = updateGeneration(ctx, patches, oldObj, newObj); err != nil { - logger.Errorw("Failed to update generation", zap.Error(err)) - return nil, perrors.Wrap(err, "failed to update generation") - } - if patches, err = setDefaults(patches, newObj); err != nil { logger.Errorw("Failed the resource specific defaulter", zap.Error(err)) // Return the error message as-is to give the defaulter callback @@ -583,39 +568,6 @@ func (ac *AdmissionController) mutate(ctx context.Context, req *admissionv1beta1 return json.Marshal(patches) } -// updateGeneration sets the generation by following this logic: -// if there's no old object, it's create, set generation to 1 -// if there's an old object and spec has changed, set generation to oldGeneration + 1 -// appends the patch to patches if changes are necessary. -// TODO: Generation does not work correctly with CRD. They are scrubbed -// by the APIserver (https://github.com/kubernetes/kubernetes/issues/58778) -// So, we add Generation here. Once that gets fixed, remove this and use -// ObjectMeta.Generation instead. -func updateGeneration(ctx context.Context, patches duck.JSONPatch, old GenericCRD, new GenericCRD) (duck.JSONPatch, error) { - logger := logging.FromContext(ctx) - - if chg, err := hasChanged(ctx, old, new); err != nil { - return nil, err - } else if !chg { - logger.Info("No changes in the spec, not bumping generation") - return patches, nil - } - - // Leverage Spec duck typing to bump the Generation of the resource. - before, err := asGenerational(new) - if err != nil { - return nil, err - } - after := before.DeepCopyObject().(*duckv1alpha1.Generational) - after.Spec.Generation = after.Spec.Generation + 1 - - genBump, err := duck.CreatePatch(before, after) - if err != nil { - return nil, err - } - return append(patches, genBump...), nil -} - // roundTripPatch generates the JSONPatch that corresponds to round tripping the given bytes through // the Golang type (JSON -> Golang type -> JSON). Because it is not always true that // bytes == json.Marshal(json.Unmarshal(bytes)). @@ -634,68 +586,6 @@ func roundTripPatch(bytes []byte, unmarshalled interface{}) (duck.JSONPatch, err return jsonpatch.CreatePatch(bytes, marshaledBytes) } -// Not worth fully duck typing since there's no shared schema. -type hasSpec struct { - Spec json.RawMessage `json:"spec"` -} - -func getSpecJSON(crd GenericCRD) ([]byte, error) { - b, err := json.Marshal(crd) - if err != nil { - return nil, err - } - hs := hasSpec{} - if err := json.Unmarshal(b, &hs); err != nil { - return nil, err - } - return []byte(hs.Spec), nil -} - -func hasChanged(ctx context.Context, old, new GenericCRD) (bool, error) { - if old == nil { - return true, nil - } - logger := logging.FromContext(ctx) - - oldSpecJSON, err := getSpecJSON(old) - if err != nil { - logger.Errorw("Failed to get Spec JSON for old", zap.Error(err)) - return false, err - } - newSpecJSON, err := getSpecJSON(new) - if err != nil { - logger.Errorw("Failed to get Spec JSON for new", zap.Error(err)) - return false, err - } - - specPatches, err := jsonpatch.CreatePatch(oldSpecJSON, newSpecJSON) - if err != nil { - return false, err - } - if len(specPatches) == 0 { - return false, nil - } - specPatchesJSON, err := json.Marshal(specPatches) - if err != nil { - logger.Errorw("Failed to marshal spec patches", zap.Error(err)) - return false, err - } - logger.Infof("Specs differ:\n%s\n", string(specPatchesJSON)) - return true, nil -} - -func asGenerational(crd GenericCRD) (*duckv1alpha1.Generational, error) { - raw, err := json.Marshal(crd) - if err != nil { - return nil, err - } - kr := &duckv1alpha1.Generational{} - if err := json.Unmarshal(raw, kr); err != nil { - return nil, err - } - return kr, nil -} - func generateSecret(ctx context.Context, options *ControllerOptions) (*corev1.Secret, error) { serverKey, serverCert, caCert, err := CreateCerts(ctx, options.ServiceName, options.Namespace) if err != nil { diff --git a/webhook/webhook_integration_test.go b/webhook/webhook_integration_test.go index 7276a6f6f6..f9315fe233 100644 --- a/webhook/webhook_integration_test.go +++ b/webhook/webhook_integration_test.go @@ -27,7 +27,6 @@ import ( "testing" "time" - "github.com/google/go-cmp/cmp" "github.com/mattbaird/jsonpatch" admissionv1beta1 "k8s.io/api/admission/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -171,7 +170,7 @@ func TestValidResponseForResource(t *testing.T) { Kind: "Resource", }, } - testRev := createResource(1234, "testrev") + testRev := createResource("testrev") marshaled, err := json.Marshal(testRev) if err != nil { t.Fatalf("Failed to marshal resource: %s", err) @@ -215,18 +214,6 @@ func TestValidResponseForResource(t *testing.T) { if err != nil { t.Fatalf("Failed to decode response: %v", err) } - - expectJsonPatch := incrementGenerationPatch(testRev.Spec.Generation) - - var respPatch []jsonpatch.JsonPatchOperation - err = json.Unmarshal(reviewResponse.Response.Patch, &respPatch) - if err != nil { - t.Fatalf("Failed to unmarshal json patch %v", err) - } - - if diff := cmp.Diff(respPatch[0], expectJsonPatch); diff != "" { - t.Errorf("Unexpected patch (-want, +got): %v", diff) - } } func TestInvalidResponseForResource(t *testing.T) { @@ -254,7 +241,7 @@ func TestInvalidResponseForResource(t *testing.T) { t.Fatalf("createSecureTLSClient() = %v", err) } - resource := createResource(1, testResourceName) + resource := createResource(testResourceName) resource.Spec.FieldWithValidation = "not the right value" marshaled, err := json.Marshal(resource) diff --git a/webhook/webhook_test.go b/webhook/webhook_test.go index 00706d034e..317e82b8de 100644 --- a/webhook/webhook_test.go +++ b/webhook/webhook_test.go @@ -17,7 +17,6 @@ limitations under the License. package webhook import ( - "context" "crypto/tls" "encoding/json" "encoding/pem" @@ -136,7 +135,7 @@ func TestUnknownVersionFails(t *testing.T) { } func TestValidCreateResourceSucceeds(t *testing.T) { - r := createResource(1234, "a name") + r := createResource("a name") for _, v := range []string{"v1alpha1", "v1beta1"} { r.TypeMeta.APIVersion = v r.SetDefaults() // Fill in defaults to check that there are no patches. @@ -144,19 +143,17 @@ func TestValidCreateResourceSucceeds(t *testing.T) { resp := ac.admit(TestContextWithLogger(t), createCreateResource(r)) expectAllowed(t, resp) expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{ - incrementGenerationPatch(r.Spec.Generation), setUserAnnotation(user1, user1), }) } } func TestValidCreateResourceSucceedsWithDefaultPatch(t *testing.T) { - r := createResource(1234, "a name") + r := createResource("a name") _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) resp := ac.admit(TestContextWithLogger(t), createCreateResource(r)) expectAllowed(t, resp) expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{ - incrementGenerationPatch(r.Spec.Generation), { Operation: "add", Path: "/spec/fieldThatsImmutableWithDefault", @@ -190,13 +187,6 @@ func TestValidCreateResourceSucceedsWithRoundTripAndDefaultPatch(t *testing.T) { Path: "/spec", Value: map[string]interface{}{}, }, - // This is almost identical to incrementGenerationPatch(0), but uses 'add', rather than - // 'replace' because `spec` is empty to begin with. - { - Operation: "add", - Path: "/spec/generation", - Value: float64(1), - }, { Operation: "add", Path: "/spec/fieldWithDefault", @@ -232,7 +222,7 @@ func createInnerDefaultResourceWithoutSpec(t *testing.T) []byte { } func TestInvalidCreateResourceFails(t *testing.T) { - r := createResource(1234, "a name") + r := createResource("a name") // Put a bad value in. r.Spec.FieldWithValidation = "not what's expected" @@ -243,7 +233,7 @@ func TestInvalidCreateResourceFails(t *testing.T) { } func TestNopUpdateResourceSucceeds(t *testing.T) { - r := createResource(1234, "a name") + r := createResource("a name") r.SetDefaults() // Fill in defaults to check that there are no patches. nr := r.DeepCopyObject().(*Resource) r.AnnotateUserInfo(nil, &authenticationv1.UserInfo{Username: user1}) @@ -253,66 +243,11 @@ func TestNopUpdateResourceSucceeds(t *testing.T) { expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{}) } -func TestUpdateGeneration(t *testing.T) { - ctx := context.Background() - r := createResource(1988, "beautiful") - rc := createResource(1988, "beautiful") - rc.Spec.FieldWithDefault = "lily" - - tests := []struct { - name string - in duck.JSONPatch - old *Resource - new *Resource - want duck.JSONPatch - }{{ - "nil in, no change", - nil, - r, r, - nil, - }, { - "empty in, no change", - []jsonpatch.JsonPatchOperation{}, - r, r, - []jsonpatch.JsonPatchOperation{}, - }, { - "nil in, change", - []jsonpatch.JsonPatchOperation{}, - r, rc, - []jsonpatch.JsonPatchOperation{ - {Operation: "replace", Path: "/spec/generation", Value: 1989.0}, - }, - }, { - "non-nil in, change", - []jsonpatch.JsonPatchOperation{ - {Operation: "replace", Path: "/spec/fieldWithDefault", Value: "Zero"}, - }, - r, rc, - []jsonpatch.JsonPatchOperation{ - {Operation: "replace", Path: "/spec/fieldWithDefault", Value: "Zero"}, - {Operation: "replace", Path: "/spec/generation", Value: 1989.0}, - }, - }} - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - got, err := updateGeneration(ctx, test.in, test.old, test.new) - if err != nil { - t.Fatalf("Error in updateGeneration: %v", err) - } - if got, want := got, test.want; !cmp.Equal(got, want) { - t.Errorf("JSONPatch diff (+got, -want): %s", cmp.Diff(got, want)) - } - }) - } -} - func TestValidUpdateResourcePreserveAnnotations(t *testing.T) { - old := createResource(1234, "a name") + old := createResource("a name") old.SetDefaults() // Fill in defaults to check that there are no patches. old.AnnotateUserInfo(nil, &authenticationv1.UserInfo{Username: user1}) - new := createResource(1234, "a name") + new := createResource("a name") new.SetDefaults() // User set annotations on the resource. new.ObjectMeta.SetAnnotations(map[string]string{ @@ -327,20 +262,16 @@ func TestValidUpdateResourcePreserveAnnotations(t *testing.T) { } func TestValidBigChangeResourceSucceeds(t *testing.T) { - old := createResource(1234, "a name") + old := createResource("a name") old.SetDefaults() // Fill in defaults to check that there are no patches. old.AnnotateUserInfo(nil, &authenticationv1.UserInfo{Username: user1}) - new := createResource(1234, "a name") + new := createResource("a name") new.Spec.FieldWithDefault = "melon collie and the infinite sadness" _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) resp := ac.admit(TestContextWithLogger(t), createUpdateResource(old, new)) expectAllowed(t, resp) expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{{ - Operation: "replace", - Path: "/spec/generation", - Value: float64(1235), - }, { Operation: "add", Path: "/spec/fieldThatsImmutableWithDefault", Value: "this is another default value", @@ -349,20 +280,15 @@ func TestValidBigChangeResourceSucceeds(t *testing.T) { } func TestValidUpdateResourceSucceeds(t *testing.T) { - old := createResource(1234, "a name") + old := createResource("a name") old.SetDefaults() // Fill in defaults to check that there are no patches. old.AnnotateUserInfo(nil, &authenticationv1.UserInfo{Username: user1}) - new := createResource(1234, "a name") - // We clear the field that has a default. + new := createResource("a name") _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) resp := ac.admit(TestContextWithLogger(t), createUpdateResource(old, new)) expectAllowed(t, resp) expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{{ - Operation: "replace", - Path: "/spec/generation", - Value: 1235.0, - }, { Operation: "add", Path: "/spec/fieldThatsImmutableWithDefault", Value: "this is another default value", @@ -374,8 +300,8 @@ func TestValidUpdateResourceSucceeds(t *testing.T) { } func TestInvalidUpdateResourceFailsValidation(t *testing.T) { - old := createResource(1234, "a name") - new := createResource(1234, "a name") + old := createResource("a name") + new := createResource("a name") // Try to update to a bad value. new.Spec.FieldWithValidation = "not what's expected" @@ -386,8 +312,8 @@ func TestInvalidUpdateResourceFailsValidation(t *testing.T) { } func TestInvalidUpdateResourceFailsImmutability(t *testing.T) { - old := createResource(1234, "a name") - new := createResource(1234, "a name") + old := createResource("a name") + new := createResource("a name") // Try to change the value new.Spec.FieldThatsImmutable = "a different value" @@ -399,9 +325,9 @@ func TestInvalidUpdateResourceFailsImmutability(t *testing.T) { } func TestDefaultingImmutableFields(t *testing.T) { - old := createResource(1234, "a name") + old := createResource("a name") old.AnnotateUserInfo(nil, &authenticationv1.UserInfo{Username: user1}) - new := createResource(1234, "a name") + new := createResource("a name") // If we don't specify the new, but immutable field, we default it, // and it is not rejected. @@ -622,14 +548,13 @@ func createDeployment(ac *AdmissionController) { ac.Client.ExtensionsV1beta1().Deployments("knative-something").Create(deployment) } -func createResource(generation int64, name string) *Resource { +func createResource(name string) *Resource { return &Resource{ ObjectMeta: metav1.ObjectMeta{ Namespace: testNamespace, Name: name, }, Spec: ResourceSpec{ - Generation: generation, FieldWithValidation: "magic value", }, } @@ -747,14 +672,6 @@ func expectPatches(t *testing.T, a []byte, e []jsonpatch.JsonPatchOperation) { } } -func incrementGenerationPatch(old int64) jsonpatch.JsonPatchOperation { - return jsonpatch.JsonPatchOperation{ - Operation: "replace", - Path: "/spec/generation", - Value: float64(old) + 1.0, - } -} - func updateAnnotationsWithUser(userC, userU string) []jsonpatch.JsonPatchOperation { // Just keys is being updated, so format is iffy. return []jsonpatch.JsonPatchOperation{{