From 653258f1d521645646aea1db6b114d36e4da0643 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 19 May 2017 15:29:05 +0200 Subject: [PATCH] apiextensions: add Established condition --- .../pkg/apis/apiextensions/types.go | 4 + .../pkg/apis/apiextensions/v1alpha1/types.go | 4 + .../customresource_discovery_controller.go | 3 +- .../pkg/apiserver/customresource_handler.go | 3 +- .../pkg/controller/finalizer/crd_finalizer.go | 90 ++++++----- .../pkg/controller/status/BUILD | 1 + .../controller/status/naming_controller.go | 75 +++++---- .../status/naming_controller_test.go | 142 ++++++++++++++---- 8 files changed, 215 insertions(+), 107 deletions(-) diff --git a/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/types.go b/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/types.go index c2012df173a..e44ff8d71f4 100644 --- a/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/types.go +++ b/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/types.go @@ -70,6 +70,10 @@ const ( type CustomResourceDefinitionConditionType string const ( + // Established means that the resource has become active. A resource is established when all names are + // accepted without a conflict for the first time. A resource stays established until deleted, even during + // a later NameConflict due to changed names. Note that not all names can be changed. + Established CustomResourceDefinitionConditionType = "Established" // NameConflict means the names chosen for this CustomResourceDefinition conflict with others in the group. NameConflict CustomResourceDefinitionConditionType = "NameConflict" // Terminating means that the CustomResourceDefinition has been deleted and is cleaning up. diff --git a/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/v1alpha1/types.go b/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/v1alpha1/types.go index 1195df29bb7..4e004aa8199 100644 --- a/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/v1alpha1/types.go +++ b/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/v1alpha1/types.go @@ -70,6 +70,10 @@ const ( type CustomResourceDefinitionConditionType string const ( + // Established means that the resource has become active. A resource is established when all names are + // accepted without a conflict for the first time. A resource stays established until deleted, even during + // a later NameConflict due to changed names. Note that not all names can be changed. + Established CustomResourceDefinitionConditionType = "Established" // NameConflict means the names chosen for this CustomResourceDefinition conflict with others in the group. NameConflict CustomResourceDefinitionConditionType = "NameConflict" // Terminating means that the CustomResourceDefinition has been deleted and is cleaning up. diff --git a/staging/src/k8s.io/kube-apiextensions-server/pkg/apiserver/customresource_discovery_controller.go b/staging/src/k8s.io/kube-apiextensions-server/pkg/apiserver/customresource_discovery_controller.go index b385d69310b..51271fcba12 100644 --- a/staging/src/k8s.io/kube-apiextensions-server/pkg/apiserver/customresource_discovery_controller.go +++ b/staging/src/k8s.io/kube-apiextensions-server/pkg/apiserver/customresource_discovery_controller.go @@ -85,8 +85,7 @@ func (c *DiscoveryController) sync(version schema.GroupVersion) error { foundVersion := false foundGroup := false for _, crd := range crds { - // if we can't definitively determine that our names are good, don't serve it - if !apiextensions.IsCRDConditionFalse(crd, apiextensions.NameConflict) { + if !apiextensions.IsCRDConditionTrue(crd, apiextensions.Established) { continue } diff --git a/staging/src/k8s.io/kube-apiextensions-server/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/kube-apiextensions-server/pkg/apiserver/customresource_handler.go index 294ccb6c440..dcbfd84a4f4 100644 --- a/staging/src/k8s.io/kube-apiextensions-server/pkg/apiserver/customresource_handler.go +++ b/staging/src/k8s.io/kube-apiextensions-server/pkg/apiserver/customresource_handler.go @@ -143,8 +143,7 @@ func (r *crdHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { r.delegate.ServeHTTP(w, req) return } - // if we can't definitively determine that our names are good, delegate - if !apiextensions.IsCRDConditionFalse(crd, apiextensions.NameConflict) { + if !apiextensions.IsCRDConditionTrue(crd, apiextensions.Established) { r.delegate.ServeHTTP(w, req) } if len(requestInfo.Subresource) > 0 { diff --git a/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/finalizer/crd_finalizer.go b/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/finalizer/crd_finalizer.go index ab4fef4f6de..a9acb37b442 100644 --- a/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/finalizer/crd_finalizer.go +++ b/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/finalizer/crd_finalizer.go @@ -127,38 +127,55 @@ func (c *CRDFinalizer) sync(key string) error { return err } - // It's possible for a naming conflict to have removed this resource from the API after instances were created. - // For now we will cowardly stop finalizing. If we don't go through the REST API, weird things may happen: - // no audit trail, no admission checks or side effects, finalization would probably still work but defaulting - // would be missed. It would be a mess. - // This requires human intervention to solve, update status so they have a reason. - // TODO split coreNamesAccepted from extendedNamesAccepted. If coreNames were accepted, then we have something to cleanup - // and the endpoint is serviceable. if they aren't, then there's nothing to cleanup. - if !apiextensions.IsCRDConditionFalse(crd, apiextensions.NameConflict) { + // Now we can start deleting items. We should use the REST API to ensure that all normal admission runs. + // Since we control the endpoints, we know that delete collection works. No need to delete if not established. + if apiextensions.IsCRDConditionTrue(crd, apiextensions.Established) { + cond, deleteErr := c.deleteInstances(crd) + apiextensions.SetCRDCondition(crd, *cond) + if deleteErr != nil { + crd, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(crd) + if err != nil { + utilruntime.HandleError(err) + } + return deleteErr + } + } else { apiextensions.SetCRDCondition(crd, apiextensions.CustomResourceDefinitionCondition{ Type: apiextensions.Terminating, - Status: apiextensions.ConditionTrue, - Reason: "InstanceDeletionStuck", - Message: fmt.Sprintf("cannot proceed with deletion because of %v condition", apiextensions.NameConflict), + Status: apiextensions.ConditionFalse, + Reason: "NeverEstablished", + Message: "resource was never established", }) - crd, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(crd) - if err != nil { - return err - } - return fmt.Errorf("cannot proceed with deletion because of %v condition", apiextensions.NameConflict) } + apiextensions.CRDRemoveFinalizer(crd, apiextensions.CustomResourceCleanupFinalizer) + crd, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(crd) + if err != nil { + return err + } + + // and now issue another delete, which should clean it all up if no finalizers remain or no-op if they do + return c.crdClient.CustomResourceDefinitions().Delete(crd.Name, nil) +} + +func (c *CRDFinalizer) deleteInstances(crd *apiextensions.CustomResourceDefinition) (*apiextensions.CustomResourceDefinitionCondition, error) { // Now we can start deleting items. While it would be ideal to use a REST API client, doing so // could incorrectly delete a ThirdPartyResource with the same URL as the CustomResource, so we go // directly to the storage instead. Since we control the storage, we know that delete collection works. crClient := c.crClientGetter.GetCustomResourceListerCollectionDeleter(crd.UID) if crClient == nil { - return fmt.Errorf("unable to find a custom resource client for %s.%s", crd.Status.AcceptedNames.Plural, crd.Spec.Group) + return nil, fmt.Errorf("unable to find a custom resource client for %s.%s", crd.Status.AcceptedNames.Plural, crd.Spec.Group) } + ctx := genericapirequest.NewContext() allResources, err := crClient.List(ctx, nil) if err != nil { - return err + return &apiextensions.CustomResourceDefinitionCondition{ + Type: apiextensions.Terminating, + Status: apiextensions.ConditionTrue, + Reason: "InstanceDeletionFailed", + Message: fmt.Sprintf("could not list instances: %v", err), + }, err } deletedNamespaces := sets.String{} @@ -181,23 +198,18 @@ func (c *CRDFinalizer) sync(key string) error { } } if deleteError := utilerrors.NewAggregate(deleteErrors); deleteError != nil { - apiextensions.SetCRDCondition(crd, apiextensions.CustomResourceDefinitionCondition{ + return &apiextensions.CustomResourceDefinitionCondition{ Type: apiextensions.Terminating, Status: apiextensions.ConditionTrue, Reason: "InstanceDeletionFailed", Message: fmt.Sprintf("could not issue all deletes: %v", deleteError), - }) - crd, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(crd) - if err != nil { - utilruntime.HandleError(err) - } - return deleteError + }, deleteError } // now we need to wait until all the resources are deleted. Start with a simple poll before we do anything fancy. // TODO not all servers are synchronized on caches. It is possible for a stale one to still be creating things. // Once we have a mechanism for servers to indicate their states, we should check that for concurrence. - listErr := wait.PollImmediate(5*time.Second, 1*time.Minute, func() (bool, error) { + err = wait.PollImmediate(5*time.Second, 1*time.Minute, func() (bool, error) { listObj, err := crClient.List(ctx, nil) if err != nil { return false, err @@ -208,34 +220,20 @@ func (c *CRDFinalizer) sync(key string) error { glog.V(2).Infof("%s.%s waiting for %d items to be removed", crd.Status.AcceptedNames.Plural, crd.Spec.Group, len(listObj.(*unstructured.UnstructuredList).Items)) return false, nil }) - if listErr != nil { - apiextensions.SetCRDCondition(crd, apiextensions.CustomResourceDefinitionCondition{ + if err != nil { + return &apiextensions.CustomResourceDefinitionCondition{ Type: apiextensions.Terminating, Status: apiextensions.ConditionTrue, Reason: "InstanceDeletionCheck", - Message: fmt.Sprintf("could not confirm zero CustomResources remaining: %v", listErr), - }) - crd, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(crd) - if err != nil { - utilruntime.HandleError(err) - } - return listErr + Message: fmt.Sprintf("could not confirm zero CustomResources remaining: %v", err), + }, err } - - apiextensions.SetCRDCondition(crd, apiextensions.CustomResourceDefinitionCondition{ + return &apiextensions.CustomResourceDefinitionCondition{ Type: apiextensions.Terminating, Status: apiextensions.ConditionFalse, Reason: "InstanceDeletionCompleted", Message: "removed all instances", - }) - apiextensions.CRDRemoveFinalizer(crd, apiextensions.CustomResourceCleanupFinalizer) - crd, err = c.crdClient.CustomResourceDefinitions().UpdateStatus(crd) - if err != nil { - return err - } - - // and now issue another delete, which should clean it all up if no finalizers remain or no-op if they do - return c.crdClient.CustomResourceDefinitions().Delete(crd.Name, nil) + }, nil } func (c *CRDFinalizer) Run(workers int, stopCh <-chan struct{}) { diff --git a/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/BUILD b/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/BUILD index c1760d34d65..9123b5934e3 100644 --- a/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/BUILD +++ b/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/BUILD @@ -28,6 +28,7 @@ go_library( deps = [ "//vendor/github.com/golang/glog:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/conversion:go_default_library", "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/errors:go_default_library", diff --git a/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go b/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go index 9428cc7c0c0..b412a957da7 100644 --- a/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go +++ b/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller.go @@ -24,6 +24,7 @@ import ( "github.com/golang/glog" apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/conversion" "k8s.io/apimachinery/pkg/labels" utilerrors "k8s.io/apimachinery/pkg/util/errors" @@ -118,11 +119,11 @@ func (c *NamingConditionController) getAcceptedNamesForGroup(group string) (allR return allResources, allKinds } -func (c *NamingConditionController) calculateNames(in *apiextensions.CustomResourceDefinition) (apiextensions.CustomResourceDefinitionNames, apiextensions.CustomResourceDefinitionCondition) { +func (c *NamingConditionController) calculateNamesAndConditions(in *apiextensions.CustomResourceDefinition) (apiextensions.CustomResourceDefinitionNames, apiextensions.CustomResourceDefinitionCondition, apiextensions.CustomResourceDefinitionCondition) { // Get the names that have already been claimed allResources, allKinds := c.getAcceptedNamesForGroup(in.Spec.Group) - condition := apiextensions.CustomResourceDefinitionCondition{ + nameConflictCondition := apiextensions.CustomResourceDefinitionCondition{ Type: apiextensions.NameConflict, Status: apiextensions.ConditionUnknown, } @@ -134,16 +135,16 @@ func (c *NamingConditionController) calculateNames(in *apiextensions.CustomResou // Check each name for mismatches. If there's a mismatch between spec and status, then try to deconflict. // Continue on errors so that the status is the best match possible if err := equalToAcceptedOrFresh(requestedNames.Plural, acceptedNames.Plural, allResources); err != nil { - condition.Status = apiextensions.ConditionTrue - condition.Reason = "Plural" - condition.Message = err.Error() + nameConflictCondition.Status = apiextensions.ConditionTrue + nameConflictCondition.Reason = "Plural" + nameConflictCondition.Message = err.Error() } else { newNames.Plural = requestedNames.Plural } if err := equalToAcceptedOrFresh(requestedNames.Singular, acceptedNames.Singular, allResources); err != nil { - condition.Status = apiextensions.ConditionTrue - condition.Reason = "Singular" - condition.Message = err.Error() + nameConflictCondition.Status = apiextensions.ConditionTrue + nameConflictCondition.Reason = "Singular" + nameConflictCondition.Message = err.Error() } else { newNames.Singular = requestedNames.Singular } @@ -161,37 +162,58 @@ func (c *NamingConditionController) calculateNames(in *apiextensions.CustomResou } if err := utilerrors.NewAggregate(errs); err != nil { - condition.Status = apiextensions.ConditionTrue - condition.Reason = "ShortNames" - condition.Message = err.Error() + nameConflictCondition.Status = apiextensions.ConditionTrue + nameConflictCondition.Reason = "ShortNames" + nameConflictCondition.Message = err.Error() } else { newNames.ShortNames = requestedNames.ShortNames } } if err := equalToAcceptedOrFresh(requestedNames.Kind, acceptedNames.Kind, allKinds); err != nil { - condition.Status = apiextensions.ConditionTrue - condition.Reason = "Kind" - condition.Message = err.Error() + nameConflictCondition.Status = apiextensions.ConditionTrue + nameConflictCondition.Reason = "Kind" + nameConflictCondition.Message = err.Error() } else { newNames.Kind = requestedNames.Kind } if err := equalToAcceptedOrFresh(requestedNames.ListKind, acceptedNames.ListKind, allKinds); err != nil { - condition.Status = apiextensions.ConditionTrue - condition.Reason = "ListKind" - condition.Message = err.Error() + nameConflictCondition.Status = apiextensions.ConditionTrue + nameConflictCondition.Reason = "ListKind" + nameConflictCondition.Message = err.Error() } else { newNames.ListKind = requestedNames.ListKind } // if we haven't changed the condition, then our names must be good. - if condition.Status == apiextensions.ConditionUnknown { - condition.Status = apiextensions.ConditionFalse - condition.Reason = "NoConflicts" - condition.Message = "no conflicts found" + if nameConflictCondition.Status == apiextensions.ConditionUnknown { + nameConflictCondition.Status = apiextensions.ConditionFalse + nameConflictCondition.Reason = "NoConflicts" + nameConflictCondition.Message = "no conflicts found" } - return newNames, condition + // set EstablishedCondition to true if all names are accepted. Never set it back to false. + establishedCondition := apiextensions.CustomResourceDefinitionCondition{ + Type: apiextensions.Established, + Status: apiextensions.ConditionFalse, + Reason: "NotAccepted", + Message: "not all names are accepted", + LastTransitionTime: metav1.NewTime(time.Now()), + } + if old := apiextensions.FindCRDCondition(in, apiextensions.Established); old != nil { + establishedCondition = *old + } + if establishedCondition.Status != apiextensions.ConditionTrue && nameConflictCondition.Status == apiextensions.ConditionFalse { + establishedCondition = apiextensions.CustomResourceDefinitionCondition{ + Type: apiextensions.Established, + Status: apiextensions.ConditionTrue, + Reason: "InitialNamesAccepted", + Message: "the initial names have been accepted", + LastTransitionTime: metav1.NewTime(time.Now()), + } + } + + return newNames, nameConflictCondition, establishedCondition } func equalToAcceptedOrFresh(requestedName, acceptedName string, usedNames sets.String) error { @@ -214,12 +236,12 @@ func (c *NamingConditionController) sync(key string) error { return err } - acceptedNames, namingCondition := c.calculateNames(inCustomResourceDefinition) + acceptedNames, namingCondition, establishedCondition := c.calculateNamesAndConditions(inCustomResourceDefinition) + // nothing to do if accepted names and NameConflict condition didn't change if reflect.DeepEqual(inCustomResourceDefinition.Status.AcceptedNames, acceptedNames) && - apiextensions.IsCRDConditionEquivalent( - &namingCondition, - apiextensions.FindCRDCondition(inCustomResourceDefinition, apiextensions.NameConflict)) { + apiextensions.IsCRDConditionEquivalent(&namingCondition, apiextensions.FindCRDCondition(inCustomResourceDefinition, apiextensions.NameConflict)) && + apiextensions.IsCRDConditionEquivalent(&establishedCondition, apiextensions.FindCRDCondition(inCustomResourceDefinition, apiextensions.Established)) { return nil } @@ -230,6 +252,7 @@ func (c *NamingConditionController) sync(key string) error { crd.Status.AcceptedNames = acceptedNames apiextensions.SetCRDCondition(crd, namingCondition) + apiextensions.SetCRDCondition(crd, establishedCondition) updatedObj, err := c.crdClient.CustomResourceDefinitions().UpdateStatus(crd) if err != nil { diff --git a/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller_test.go b/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller_test.go index f79031a9644..619a20d75dd 100644 --- a/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller_test.go +++ b/staging/src/k8s.io/kube-apiextensions-server/pkg/controller/status/naming_controller_test.go @@ -67,6 +67,12 @@ func (b *crdBuilder) StatusNames(plural, singular, kind, listKind string, shortN return b } +func (b *crdBuilder) Condition(c apiextensions.CustomResourceDefinitionCondition) *crdBuilder { + b.curr.Status.Conditions = append(b.curr.Status.Conditions, c) + + return b +} + func names(plural, singular, kind, listKind string, shortNames ...string) apiextensions.CustomResourceDefinitionNames { ret := apiextensions.CustomResourceDefinitionNames{ Plural: plural, @@ -82,14 +88,14 @@ func (b *crdBuilder) NewOrDie() *apiextensions.CustomResourceDefinition { return &b.curr } -var goodCondition = apiextensions.CustomResourceDefinitionCondition{ +var acceptedCondition = apiextensions.CustomResourceDefinitionCondition{ Type: apiextensions.NameConflict, Status: apiextensions.ConditionFalse, Reason: "NoConflicts", Message: "no conflicts found", } -func badCondition(reason, message string) apiextensions.CustomResourceDefinitionCondition { +func nameConflictCondition(reason, message string) apiextensions.CustomResourceDefinitionCondition { return apiextensions.CustomResourceDefinitionCondition{ Type: apiextensions.NameConflict, Status: apiextensions.ConditionTrue, @@ -98,14 +104,29 @@ func badCondition(reason, message string) apiextensions.CustomResourceDefinition } } +var establishedCondition = apiextensions.CustomResourceDefinitionCondition{ + Type: apiextensions.Established, + Status: apiextensions.ConditionTrue, + Reason: "InitialNamesAccepted", + Message: "the initial names have been accepted", +} + +var notEstablishedCondition = apiextensions.CustomResourceDefinitionCondition{ + Type: apiextensions.Established, + Status: apiextensions.ConditionFalse, + Reason: "NotAccepted", + Message: "not all names are accepted", +} + func TestSync(t *testing.T) { tests := []struct { name string - in *apiextensions.CustomResourceDefinition - existing []*apiextensions.CustomResourceDefinition - expectedNames apiextensions.CustomResourceDefinitionNames - expectedCondition apiextensions.CustomResourceDefinitionCondition + in *apiextensions.CustomResourceDefinition + existing []*apiextensions.CustomResourceDefinition + expectedNames apiextensions.CustomResourceDefinitionNames + expectedNameConflictCondition apiextensions.CustomResourceDefinitionCondition + expectedEstablishedCondition apiextensions.CustomResourceDefinitionCondition }{ { name: "first resource", @@ -114,7 +135,8 @@ func TestSync(t *testing.T) { expectedNames: apiextensions.CustomResourceDefinitionNames{ Plural: "alfa", }, - expectedCondition: goodCondition, + expectedNameConflictCondition: acceptedCondition, + expectedEstablishedCondition: establishedCondition, }, { name: "different groups", @@ -122,8 +144,9 @@ func TestSync(t *testing.T) { existing: []*apiextensions.CustomResourceDefinition{ newCRD("alfa.charlie.com").StatusNames("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2").NewOrDie(), }, - expectedNames: names("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), - expectedCondition: goodCondition, + expectedNames: names("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), + expectedNameConflictCondition: acceptedCondition, + expectedEstablishedCondition: establishedCondition, }, { name: "conflict plural to singular", @@ -131,8 +154,9 @@ func TestSync(t *testing.T) { existing: []*apiextensions.CustomResourceDefinition{ newCRD("india.bravo.com").StatusNames("india", "alfa", "", "").NewOrDie(), }, - expectedNames: names("", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), - expectedCondition: badCondition("Plural", `"alfa" is already in use`), + expectedNames: names("", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), + expectedNameConflictCondition: nameConflictCondition("Plural", `"alfa" is already in use`), + expectedEstablishedCondition: notEstablishedCondition, }, { name: "conflict singular to shortName", @@ -140,8 +164,9 @@ func TestSync(t *testing.T) { existing: []*apiextensions.CustomResourceDefinition{ newCRD("india.bravo.com").StatusNames("india", "indias", "", "", "delta-singular").NewOrDie(), }, - expectedNames: names("alfa", "", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), - expectedCondition: badCondition("Singular", `"delta-singular" is already in use`), + expectedNames: names("alfa", "", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), + expectedNameConflictCondition: nameConflictCondition("Singular", `"delta-singular" is already in use`), + expectedEstablishedCondition: notEstablishedCondition, }, { name: "conflict on shortName to shortName", @@ -149,8 +174,9 @@ func TestSync(t *testing.T) { existing: []*apiextensions.CustomResourceDefinition{ newCRD("india.bravo.com").StatusNames("india", "indias", "", "", "hotel-shortname-2").NewOrDie(), }, - expectedNames: names("alfa", "delta-singular", "echo-kind", "foxtrot-listkind"), - expectedCondition: badCondition("ShortNames", `"hotel-shortname-2" is already in use`), + expectedNames: names("alfa", "delta-singular", "echo-kind", "foxtrot-listkind"), + expectedNameConflictCondition: nameConflictCondition("ShortNames", `"hotel-shortname-2" is already in use`), + expectedEstablishedCondition: notEstablishedCondition, }, { name: "conflict on kind to listkind", @@ -158,8 +184,9 @@ func TestSync(t *testing.T) { existing: []*apiextensions.CustomResourceDefinition{ newCRD("india.bravo.com").StatusNames("india", "indias", "", "echo-kind").NewOrDie(), }, - expectedNames: names("alfa", "delta-singular", "", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), - expectedCondition: badCondition("Kind", `"echo-kind" is already in use`), + expectedNames: names("alfa", "delta-singular", "", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), + expectedNameConflictCondition: nameConflictCondition("Kind", `"echo-kind" is already in use`), + expectedEstablishedCondition: notEstablishedCondition, }, { name: "conflict on listkind to kind", @@ -167,8 +194,9 @@ func TestSync(t *testing.T) { existing: []*apiextensions.CustomResourceDefinition{ newCRD("india.bravo.com").StatusNames("india", "indias", "foxtrot-listkind", "").NewOrDie(), }, - expectedNames: names("alfa", "delta-singular", "echo-kind", "", "golf-shortname-1", "hotel-shortname-2"), - expectedCondition: badCondition("ListKind", `"foxtrot-listkind" is already in use`), + expectedNames: names("alfa", "delta-singular", "echo-kind", "", "golf-shortname-1", "hotel-shortname-2"), + expectedNameConflictCondition: nameConflictCondition("ListKind", `"foxtrot-listkind" is already in use`), + expectedEstablishedCondition: notEstablishedCondition, }, { name: "no conflict on resource and kind", @@ -176,8 +204,9 @@ func TestSync(t *testing.T) { existing: []*apiextensions.CustomResourceDefinition{ newCRD("india.bravo.com").StatusNames("india", "echo-kind", "", "").NewOrDie(), }, - expectedNames: names("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), - expectedCondition: goodCondition, + expectedNames: names("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), + expectedNameConflictCondition: acceptedCondition, + expectedEstablishedCondition: establishedCondition, }, { name: "merge on conflicts", @@ -188,8 +217,9 @@ func TestSync(t *testing.T) { existing: []*apiextensions.CustomResourceDefinition{ newCRD("india.bravo.com").StatusNames("india", "indias", "foxtrot-listkind", "", "delta-singular").NewOrDie(), }, - expectedNames: names("alfa", "yankee-singular", "echo-kind", "whiskey-listkind", "golf-shortname-1", "hotel-shortname-2"), - expectedCondition: badCondition("ListKind", `"foxtrot-listkind" is already in use`), + expectedNames: names("alfa", "yankee-singular", "echo-kind", "whiskey-listkind", "golf-shortname-1", "hotel-shortname-2"), + expectedNameConflictCondition: nameConflictCondition("ListKind", `"foxtrot-listkind" is already in use`), + expectedEstablishedCondition: notEstablishedCondition, }, { name: "merge on conflicts shortNames as one", @@ -200,8 +230,9 @@ func TestSync(t *testing.T) { existing: []*apiextensions.CustomResourceDefinition{ newCRD("india.bravo.com").StatusNames("india", "indias", "foxtrot-listkind", "", "delta-singular", "golf-shortname-1").NewOrDie(), }, - expectedNames: names("alfa", "yankee-singular", "echo-kind", "whiskey-listkind", "victor-shortname-1", "uniform-shortname-2"), - expectedCondition: badCondition("ListKind", `"foxtrot-listkind" is already in use`), + expectedNames: names("alfa", "yankee-singular", "echo-kind", "whiskey-listkind", "victor-shortname-1", "uniform-shortname-2"), + expectedNameConflictCondition: nameConflictCondition("ListKind", `"foxtrot-listkind" is already in use`), + expectedEstablishedCondition: notEstablishedCondition, }, { name: "no conflicts on self", @@ -215,8 +246,9 @@ func TestSync(t *testing.T) { StatusNames("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"). NewOrDie(), }, - expectedNames: names("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), - expectedCondition: goodCondition, + expectedNames: names("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), + expectedNameConflictCondition: acceptedCondition, + expectedEstablishedCondition: establishedCondition, }, { name: "no conflicts on self, remove shortname", @@ -230,8 +262,53 @@ func TestSync(t *testing.T) { StatusNames("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"). NewOrDie(), }, - expectedNames: names("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1"), - expectedCondition: goodCondition, + expectedNames: names("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1"), + expectedNameConflictCondition: acceptedCondition, + expectedEstablishedCondition: establishedCondition, + }, + { + name: "established before with true condition", + in: newCRD("alfa.bravo.com").Condition(establishedCondition).NewOrDie(), + existing: []*apiextensions.CustomResourceDefinition{}, + expectedNames: apiextensions.CustomResourceDefinitionNames{ + Plural: "alfa", + }, + expectedNameConflictCondition: acceptedCondition, + expectedEstablishedCondition: establishedCondition, + }, + { + name: "not established before with false condition", + in: newCRD("alfa.bravo.com").Condition(notEstablishedCondition).NewOrDie(), + existing: []*apiextensions.CustomResourceDefinition{}, + expectedNames: apiextensions.CustomResourceDefinitionNames{ + Plural: "alfa", + }, + expectedNameConflictCondition: acceptedCondition, + expectedEstablishedCondition: establishedCondition, + }, + { + name: "conflicting, established before with true condition", + in: newCRD("alfa.bravo.com").SpecNames("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"). + Condition(establishedCondition). + NewOrDie(), + existing: []*apiextensions.CustomResourceDefinition{ + newCRD("india.bravo.com").StatusNames("india", "alfa", "", "").NewOrDie(), + }, + expectedNames: names("", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), + expectedNameConflictCondition: nameConflictCondition("Plural", `"alfa" is already in use`), + expectedEstablishedCondition: establishedCondition, + }, + { + name: "conflicting, not established before with false condition", + in: newCRD("alfa.bravo.com").SpecNames("alfa", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"). + Condition(notEstablishedCondition). + NewOrDie(), + existing: []*apiextensions.CustomResourceDefinition{ + newCRD("india.bravo.com").StatusNames("india", "alfa", "", "").NewOrDie(), + }, + expectedNames: names("", "delta-singular", "echo-kind", "foxtrot-listkind", "golf-shortname-1", "hotel-shortname-2"), + expectedNameConflictCondition: nameConflictCondition("Plural", `"alfa" is already in use`), + expectedEstablishedCondition: notEstablishedCondition, }, } @@ -245,12 +322,15 @@ func TestSync(t *testing.T) { crdLister: listers.NewCustomResourceDefinitionLister(crdIndexer), crdMutationCache: cache.NewIntegerResourceVersionMutationCache(crdIndexer, crdIndexer, 60*time.Second, false), } - actualNames, actualCondition := c.calculateNames(tc.in) + actualNames, actualNameConflictCondition, actualEstablishedCondition := c.calculateNamesAndConditions(tc.in) if e, a := tc.expectedNames, actualNames; !reflect.DeepEqual(e, a) { t.Errorf("%v expected %v, got %#v", tc.name, e, a) } - if e, a := tc.expectedCondition, actualCondition; !apiextensions.IsCRDConditionEquivalent(&e, &a) { + if e, a := tc.expectedNameConflictCondition, actualNameConflictCondition; !apiextensions.IsCRDConditionEquivalent(&e, &a) { + t.Errorf("%v expected %v, got %v", tc.name, e, a) + } + if e, a := tc.expectedEstablishedCondition, actualEstablishedCondition; !apiextensions.IsCRDConditionEquivalent(&e, &a) { t.Errorf("%v expected %v, got %v", tc.name, e, a) } }