From c13d5ce36a47c71fc2becb4a01e44ec55e57a348 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Mon, 9 Oct 2017 14:44:02 +0200 Subject: [PATCH 1/4] apiextensions-apiserver: stop cacher on CRD update --- .../pkg/apiserver/customresource_handler.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go index 50b2f4fe007..845e66e22e3 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go @@ -245,7 +245,7 @@ func (r *crdHandler) removeDeadStorage() { return } - for uid := range storageMap { + for uid, s := range storageMap { found := false for _, crd := range allCustomResourceDefinitions { if crd.UID == uid { @@ -254,6 +254,7 @@ func (r *crdHandler) removeDeadStorage() { } } if !found { + s.storage.DestroyFunc() delete(storageMap, uid) } } @@ -423,10 +424,13 @@ func (c *crdHandler) updateCustomResourceDefinition(oldObj, _ interface{}) { // Copy because we cannot write to storageMap without a race // as it is used without locking elsewhere for k, v := range storageMap { + if k == oldCRD.UID { + v.storage.DestroyFunc() + continue + } storageMap2[k] = v } - delete(storageMap2, oldCRD.UID) c.customStorage.Store(storageMap2) } From b1253887675dc21c62348c51d34eb4fb9639c4b1 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Tue, 10 Oct 2017 16:57:04 +0200 Subject: [PATCH 2/4] apiextensions: create storage with accepted, not spec'ed names --- .../pkg/apiserver/customresource_handler.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go index 845e66e22e3..6e0c078980a 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go @@ -302,7 +302,7 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti parameterScheme.AddGeneratedDeepCopyFuncs(metav1.GetGeneratedDeepCopyFuncs()...) parameterCodec := runtime.NewParameterCodec(parameterScheme) - kind := schema.GroupVersionKind{Group: crd.Spec.Group, Version: crd.Spec.Version, Kind: crd.Spec.Names.Kind} + kind := schema.GroupVersionKind{Group: crd.Spec.Group, Version: crd.Spec.Version, Kind: crd.Status.AcceptedNames.Kind} typer := unstructuredObjectTyper{ delegate: parameterScheme, unstructuredTyper: discovery.NewUnstructuredObjectTyper(nil), @@ -320,8 +320,8 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti validator := validate.NewSchemaValidator(openapiSchema, nil, "", strfmt.Default) storage := customresource.NewREST( - schema.GroupResource{Group: crd.Spec.Group, Resource: crd.Spec.Names.Plural}, - schema.GroupVersionKind{Group: crd.Spec.Group, Version: crd.Spec.Version, Kind: crd.Spec.Names.ListKind}, + schema.GroupResource{Group: crd.Spec.Group, Resource: crd.Status.AcceptedNames.Plural}, + schema.GroupVersionKind{Group: crd.Spec.Group, Version: crd.Spec.Version, Kind: crd.Status.AcceptedNames.ListKind}, customresource.NewStrategy( typer, crd.Spec.Scope == apiextensions.NamespaceScoped, @@ -368,7 +368,7 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti Typer: typer, UnsafeConvertor: unstructured.UnstructuredObjectConverter{}, - Resource: schema.GroupVersionResource{Group: crd.Spec.Group, Version: crd.Spec.Version, Resource: crd.Spec.Names.Plural}, + Resource: schema.GroupVersionResource{Group: crd.Spec.Group, Version: crd.Spec.Version, Resource: crd.Status.AcceptedNames.Plural}, Kind: kind, Subresource: "", From e23983189d6812cac02766fd6ff62d988532aa3c Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Tue, 10 Oct 2017 17:08:57 +0200 Subject: [PATCH 3/4] apiextensions: keep CRD storage for updates outside of spec and accepted names --- .../pkg/apiserver/BUILD | 1 + .../pkg/apiserver/customresource_handler.go | 29 +++++++++++++++++-- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD index be54b00ccbf..0619b7cc609 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/BUILD @@ -33,6 +33,7 @@ go_library( "//vendor/k8s.io/apiextensions-apiserver/pkg/controller/status:go_default_library", "//vendor/k8s.io/apiextensions-apiserver/pkg/registry/customresource:go_default_library", "//vendor/k8s.io/apiextensions-apiserver/pkg/registry/customresourcedefinition:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apimachinery/announced:go_default_library", diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go index 6e0c078980a..4470684447f 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go @@ -30,6 +30,7 @@ import ( "github.com/go-openapi/validate" "github.com/golang/glog" + apiequality "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -79,6 +80,11 @@ type crdHandler struct { // crdInfo stores enough information to serve the storage for the custom resource type crdInfo struct { + // spec and acceptedNames are used to compare against if a change is made on a CRD. We only update + // the storage if one of these changes. + spec *apiextensions.CustomResourceDefinitionSpec + acceptedNames *apiextensions.CustomResourceDefinitionNames + storage *customresource.REST requestScope handlers.RequestScope } @@ -108,6 +114,9 @@ func NewCustomResourceDefinitionHandler( crdInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ UpdateFunc: ret.updateCustomResourceDefinition, + DeleteFunc: func(obj interface{}) { + ret.removeDeadStorage() + }, }) ret.customStorage.Store(crdStorageMap{}) @@ -254,6 +263,7 @@ func (r *crdHandler) removeDeadStorage() { } } if !found { + glog.V(4).Infof("Removing dead CRD storage for %v", s.requestScope.Resource) s.storage.DestroyFunc() delete(storageMap, uid) } @@ -376,6 +386,9 @@ func (r *crdHandler) getServingInfoFor(crd *apiextensions.CustomResourceDefiniti } ret = &crdInfo{ + spec: &crd.Spec, + acceptedNames: &crd.Status.AcceptedNames, + storage: storage, requestScope: requestScope, } @@ -411,14 +424,24 @@ func (c crdObjectConverter) ConvertFieldLabel(version, kind, label, value string } } -func (c *crdHandler) updateCustomResourceDefinition(oldObj, _ interface{}) { +func (c *crdHandler) updateCustomResourceDefinition(oldObj, newObj interface{}) { oldCRD := oldObj.(*apiextensions.CustomResourceDefinition) - glog.V(4).Infof("Updating customresourcedefinition %s", oldCRD.Name) + newCRD := newObj.(*apiextensions.CustomResourceDefinition) c.customStorageLock.Lock() defer c.customStorageLock.Unlock() - storageMap := c.customStorage.Load().(crdStorageMap) + + oldInfo, found := storageMap[newCRD.UID] + if !found { + return + } + if apiequality.Semantic.DeepEqual(&newCRD.Spec, oldInfo.spec) && apiequality.Semantic.DeepEqual(&newCRD.Status.AcceptedNames, oldInfo.acceptedNames) { + glog.V(6).Infof("Ignoring customresourcedefinition %s update because neither spec, nor accepted names changed", oldCRD.Name) + return + } + + glog.V(4).Infof("Updating customresourcedefinition %s", oldCRD.Name) storageMap2 := make(crdStorageMap, len(storageMap)) // Copy because we cannot write to storageMap without a race From 333f49f5d1dc7bb2f644ee57a93a38303d277dcf Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 13 Oct 2017 12:15:17 +0200 Subject: [PATCH 4/4] apiextensions: fix test loop for CRD validation --- .../test/integration/validation_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go index 3c771ef1825..d540fa59b88 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go @@ -21,6 +21,7 @@ import ( "testing" "time" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/wait" @@ -379,15 +380,18 @@ func TestCRValidationOnCRDUpdate(t *testing.T) { // update the CRD to a less stricter schema gottenCRD.Spec.Validation.OpenAPIV3Schema.Required = []string{"alpha", "beta"} - - updatedCRD, err := apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(gottenCRD) - if err != nil { + if _, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(gottenCRD); err != nil { t.Fatal(err) } // CR is now accepted err = wait.Poll(500*time.Millisecond, wait.ForeverTestTimeout, func() (bool, error) { - _, err = instantiateCustomResource(t, newNoxuValidationInstance(ns, "foo"), noxuResourceClient, updatedCRD) + _, err := noxuResourceClient.Create(newNoxuValidationInstance(ns, "foo")) + if statusError, isStatus := err.(*apierrors.StatusError); isStatus { + if strings.Contains(statusError.Error(), "is invalid") { + return false, nil + } + } if err != nil { return false, err }