diff --git a/testing/resource.go b/testing/resource.go index 590a17ee7e..aec79b0a61 100644 --- a/testing/resource.go +++ b/testing/resource.go @@ -41,10 +41,8 @@ var _ apis.Defaultable = (*Resource)(nil) var _ apis.Immutable = (*Resource)(nil) var _ apis.Listable = (*Resource)(nil) -// Check that we implement the Generation duck type. +// ResourceSpec is structured with fields to test different webhook behaviour 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 69fa2af9b3..1f988f5892 100644 --- a/webhook/webhook.go +++ b/webhook/webhook.go @@ -34,7 +34,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/logging" "github.com/knative/pkg/logging/logkey" @@ -284,15 +283,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() @@ -527,12 +517,6 @@ func (ac *AdmissionController) mutate(ctx context.Context, kind metav1.GroupVers var patches []jsonpatch.JsonPatchOperation - err := updateGeneration(ctx, &patches, oldObj, newObj) - if err != nil { - logger.Error("Failed to update generation", zap.Error(err)) - return nil, fmt.Errorf("Failed to update generation: %s", err) - } - if defaulter := SetDefaults(ctx); defaulter != nil { if err := defaulter(&patches, newObj); err != nil { logger.Error("Failed the resource specific defaulter", zap.Error(err)) @@ -558,103 +542,6 @@ func (ac *AdmissionController) mutate(ctx context.Context, kind metav1.GroupVers 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 *[]jsonpatch.JsonPatchOperation, old GenericCRD, new GenericCRD) error { - logger := logging.FromContext(ctx) - - if chg, err := hasChanged(ctx, old, new); err != nil { - return err - } else if !chg { - logger.Info("No changes in the spec, not bumping generation") - return nil - } - - // Leverage Spec duck typing to bump the Generation of the resource. - before, err := asGenerational(ctx, new) - if err != nil { - return err - } - after := before.DeepCopyObject().(*duckv1alpha1.Generational) - after.Spec.Generation = after.Spec.Generation + 1 - - genBump, err := duck.CreatePatch(before, after) - if err != nil { - return err - } - *patches = append(*patches, genBump...) - return nil -} - -// 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.Error("Failed to get Spec JSON for old", zap.Error(err)) - return false, err - } - newSpecJSON, err := getSpecJSON(new) - if err != nil { - logger.Error("Failed to get Spec JSON for new", zap.Error(err)) - return false, err - } - - specPatches, err := jsonpatch.CreatePatch(oldSpecJSON, newSpecJSON) - if err != nil { - fmt.Printf("Error creating JSON patch:%v", err) - return false, err - } - if len(specPatches) == 0 { - return false, nil - } - specPatchesJSON, err := json.Marshal(specPatches) - if err != nil { - logger.Error("Failed to marshal spec patches", zap.Error(err)) - return false, err - } - logger.Infof("Specs differ:\n%+v\n", string(specPatchesJSON)) - return true, nil -} - -func asGenerational(ctx context.Context, 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 d1f0448614..3dac9bc41b 100644 --- a/webhook/webhook_test.go +++ b/webhook/webhook_test.go @@ -115,23 +115,19 @@ func TestUnknownKindFails(t *testing.T) { } func TestValidCreateResourceSucceeds(t *testing.T) { - r := createResource(1234, "a name") + r := createResource("a name") r.SetDefaults() // Fill in defaults to check that there are no patches. _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) resp := ac.admit(TestContextWithLogger(t), createCreateResource(&r)) expectAllowed(t, resp) - expectPatches(t, resp.Patch, []jsonpatch.JsonPatchOperation{ - incrementGenerationPatch(r.Spec.Generation), - }) } 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", @@ -145,7 +141,7 @@ func TestValidCreateResourceSucceedsWithDefaultPatch(t *testing.T) { } 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" @@ -156,7 +152,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. _, ac := newNonRunningTestAdmissionController(t, newDefaultOptions()) resp := ac.admit(TestContextWithLogger(t), createUpdateResource(&r, &r)) @@ -165,19 +161,15 @@ func TestNopUpdateResourceSucceeds(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. - new := createResource(1234, "a name") + new := createResource("a name") // We clear the field that has a default. _, 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", @@ -189,8 +181,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" @@ -201,8 +193,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" @@ -214,8 +206,8 @@ func TestInvalidUpdateResourceFailsImmutability(t *testing.T) { } func TestDefaultingImmutableFields(t *testing.T) { - old := createResource(1234, "a name") - new := createResource(1234, "a name") + old := createResource("a name") + new := createResource("a name") // If we don't specify the new, but immutable field, we default it, // and it is not rejected. @@ -396,14 +388,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", }, } @@ -511,14 +502,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 NewAdmissionController(client kubernetes.Interface, options ControllerOptions, logger *zap.SugaredLogger) (*AdmissionController, error) { return &AdmissionController{