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..55c3bfad46d 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,8 +70,13 @@ const ( type CustomResourceDefinitionConditionType string const ( - // NameConflict means the names chosen for this CustomResourceDefinition conflict with others in the group. - NameConflict CustomResourceDefinitionConditionType = "NameConflict" + // 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 NamesAccepted due to changed names. Note that not all names can be changed. + Established CustomResourceDefinitionConditionType = "Established" + // NamesAccepted means the names chosen for this CustomResourceDefinition do not conflict with others in + // the group and are therefore accepted. + NamesAccepted CustomResourceDefinitionConditionType = "NamesAccepted" // Terminating means that the CustomResourceDefinition has been deleted and is cleaning up. Terminating CustomResourceDefinitionConditionType = "Terminating" ) 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..0d6fb7e027d 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,8 +70,13 @@ const ( type CustomResourceDefinitionConditionType string const ( - // NameConflict means the names chosen for this CustomResourceDefinition conflict with others in the group. - NameConflict CustomResourceDefinitionConditionType = "NameConflict" + // 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 NamesAccepted due to changed names. Note that not all names can be changed. + Established CustomResourceDefinitionConditionType = "Established" + // NamesAccepted means the names chosen for this CustomResourceDefinition do not conflict with others in + // the group and are therefore accepted. + NamesAccepted CustomResourceDefinitionConditionType = "NamesAccepted" // Terminating means that the CustomResourceDefinition has been deleted and is cleaning up. Terminating CustomResourceDefinitionConditionType = "Terminating" ) diff --git a/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/validation/validation.go b/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/validation/validation.go index 360b6ece4f7..e0ef856bc97 100644 --- a/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/validation/validation.go +++ b/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/validation/validation.go @@ -46,7 +46,7 @@ func ValidateCustomResourceDefinition(obj *apiextensions.CustomResourceDefinitio // ValidateCustomResourceDefinitionUpdate statically validates func ValidateCustomResourceDefinitionUpdate(obj, oldObj *apiextensions.CustomResourceDefinition) field.ErrorList { allErrs := genericvalidation.ValidateObjectMetaUpdate(&obj.ObjectMeta, &oldObj.ObjectMeta, field.NewPath("metadata")) - allErrs = append(allErrs, ValidateCustomResourceDefinitionSpecUpdate(&obj.Spec, &oldObj.Spec, field.NewPath("spec"))...) + allErrs = append(allErrs, ValidateCustomResourceDefinitionSpecUpdate(&obj.Spec, &oldObj.Spec, apiextensions.IsCRDConditionTrue(oldObj, apiextensions.Established), field.NewPath("spec"))...) allErrs = append(allErrs, ValidateCustomResourceDefinitionStatus(&obj.Status, field.NewPath("status"))...) return allErrs } @@ -64,22 +64,21 @@ func ValidateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi if len(spec.Group) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("group"), "")) - } - if errs := validationutil.IsDNS1123Subdomain(spec.Group); len(errs) > 0 { + } else if errs := validationutil.IsDNS1123Subdomain(spec.Group); len(errs) > 0 { allErrs = append(allErrs, field.Invalid(fldPath.Child("group"), spec.Group, strings.Join(errs, ","))) - } - if len(strings.Split(spec.Group, ".")) < 2 { + } else if len(strings.Split(spec.Group, ".")) < 2 { allErrs = append(allErrs, field.Invalid(fldPath.Child("group"), spec.Group, "should be a domain with at least one dot")) } if len(spec.Version) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("version"), "")) - } - if errs := validationutil.IsDNS1035Label(spec.Version); len(errs) > 0 { + } else if errs := validationutil.IsDNS1035Label(spec.Version); len(errs) > 0 { allErrs = append(allErrs, field.Invalid(fldPath.Child("version"), spec.Version, strings.Join(errs, ","))) } switch spec.Scope { + case "": + allErrs = append(allErrs, field.Required(fldPath.Child("scope"), "")) case apiextensions.ClusterScoped, apiextensions.NamespaceScoped: default: allErrs = append(allErrs, field.NotSupported(fldPath.Child("scope"), spec.Scope, []string{string(apiextensions.ClusterScoped), string(apiextensions.NamespaceScoped)})) @@ -105,17 +104,19 @@ func ValidateCustomResourceDefinitionSpec(spec *apiextensions.CustomResourceDefi } // ValidateCustomResourceDefinitionSpecUpdate statically validates -func ValidateCustomResourceDefinitionSpecUpdate(spec, oldSpec *apiextensions.CustomResourceDefinitionSpec, fldPath *field.Path) field.ErrorList { +func ValidateCustomResourceDefinitionSpecUpdate(spec, oldSpec *apiextensions.CustomResourceDefinitionSpec, established bool, fldPath *field.Path) field.ErrorList { allErrs := ValidateCustomResourceDefinitionSpec(spec, fldPath) - // these all affect the storage, so you can't change them - genericvalidation.ValidateImmutableField(spec.Group, oldSpec.Group, fldPath.Child("group")) - genericvalidation.ValidateImmutableField(spec.Version, oldSpec.Version, fldPath.Child("version")) - genericvalidation.ValidateImmutableField(spec.Scope, oldSpec.Scope, fldPath.Child("scope")) - genericvalidation.ValidateImmutableField(spec.Names.Kind, oldSpec.Names.Kind, fldPath.Child("names", "kind")) + if established { + // these effect the storage and cannot be changed therefore + allErrs = append(allErrs, genericvalidation.ValidateImmutableField(spec.Version, oldSpec.Version, fldPath.Child("version"))...) + allErrs = append(allErrs, genericvalidation.ValidateImmutableField(spec.Scope, oldSpec.Scope, fldPath.Child("scope"))...) + allErrs = append(allErrs, genericvalidation.ValidateImmutableField(spec.Names.Kind, oldSpec.Names.Kind, fldPath.Child("names", "kind"))...) + } - // this affects the expected resource name, so you can't change it either - genericvalidation.ValidateImmutableField(spec.Names.Plural, oldSpec.Names.Plural, fldPath.Child("names", "plural")) + // these affects the resource name, which is always immutable, so this can't be updated. + allErrs = append(allErrs, genericvalidation.ValidateImmutableField(spec.Group, oldSpec.Group, fldPath.Child("group"))...) + allErrs = append(allErrs, genericvalidation.ValidateImmutableField(spec.Names.Plural, oldSpec.Names.Plural, fldPath.Child("names", "plural"))...) return allErrs } diff --git a/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/validation/validation_test.go b/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/validation/validation_test.go index bc1b2ff0add..642d0a87138 100644 --- a/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/validation/validation_test.go +++ b/staging/src/k8s.io/kube-apiextensions-server/pkg/apis/apiextensions/validation/validation_test.go @@ -29,11 +29,17 @@ type validationMatch struct { errorType field.ErrorType } -func required(path *field.Path) validationMatch { - return validationMatch{path: path, errorType: field.ErrorTypeRequired} +func required(path ...string) validationMatch { + return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeRequired} } -func invalid(path *field.Path) validationMatch { - return validationMatch{path: path, errorType: field.ErrorTypeInvalid} +func invalid(path ...string) validationMatch { + return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeInvalid} +} +func unsupported(path ...string) validationMatch { + return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeNotSupported} +} +func immutable(path ...string) validationMatch { + return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeInvalid} } func (v validationMatch) matches(err *field.Error) bool { @@ -58,7 +64,12 @@ func TestValidateCustomResourceDefinition(t *testing.T) { }, }, errors: []validationMatch{ - invalid(field.NewPath("metadata", "name")), + invalid("metadata", "name"), + required("spec", "version"), + required("spec", "scope"), + required("spec", "names", "singular"), + required("spec", "names", "kind"), + required("spec", "names", "listKind"), }, }, { @@ -67,13 +78,14 @@ func TestValidateCustomResourceDefinition(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com"}, }, errors: []validationMatch{ - required(field.NewPath("spec", "group")), - required(field.NewPath("spec", "version")), - {path: field.NewPath("spec", "scope"), errorType: field.ErrorTypeNotSupported}, - required(field.NewPath("spec", "names", "plural")), - required(field.NewPath("spec", "names", "singular")), - required(field.NewPath("spec", "names", "kind")), - required(field.NewPath("spec", "names", "listKind")), + invalid("metadata", "name"), + required("spec", "group"), + required("spec", "version"), + required("spec", "scope"), + required("spec", "names", "plural"), + required("spec", "names", "singular"), + required("spec", "names", "kind"), + required("spec", "names", "listKind"), }, }, { @@ -101,17 +113,20 @@ func TestValidateCustomResourceDefinition(t *testing.T) { }, }, errors: []validationMatch{ - invalid(field.NewPath("spec", "group")), - invalid(field.NewPath("spec", "version")), - {path: field.NewPath("spec", "scope"), errorType: field.ErrorTypeNotSupported}, - invalid(field.NewPath("spec", "names", "plural")), - invalid(field.NewPath("spec", "names", "singular")), - invalid(field.NewPath("spec", "names", "kind")), - invalid(field.NewPath("spec", "names", "listKind")), - invalid(field.NewPath("status", "acceptedNames", "plural")), - invalid(field.NewPath("status", "acceptedNames", "singular")), - invalid(field.NewPath("status", "acceptedNames", "kind")), - invalid(field.NewPath("status", "acceptedNames", "listKind")), + invalid("metadata", "name"), + invalid("spec", "group"), + invalid("spec", "version"), + unsupported("spec", "scope"), + invalid("spec", "names", "plural"), + invalid("spec", "names", "singular"), + invalid("spec", "names", "kind"), + invalid("spec", "names", "listKind"), // invalid format + invalid("spec", "names", "listKind"), // kind == listKind + invalid("status", "acceptedNames", "plural"), + invalid("status", "acceptedNames", "singular"), + invalid("status", "acceptedNames", "kind"), + invalid("status", "acceptedNames", "listKind"), // invalid format + invalid("status", "acceptedNames", "listKind"), // kind == listKind }, }, { @@ -138,21 +153,25 @@ func TestValidateCustomResourceDefinition(t *testing.T) { }, }, errors: []validationMatch{ - invalid(field.NewPath("spec", "group")), - invalid(field.NewPath("spec", "names", "listKind")), - invalid(field.NewPath("status", "acceptedNames", "listKind")), + invalid("metadata", "name"), + invalid("spec", "group"), + required("spec", "scope"), + invalid("spec", "names", "listKind"), + invalid("status", "acceptedNames", "listKind"), }, }, } for _, tc := range tests { errs := ValidateCustomResourceDefinition(tc.resource) + seenErrs := make([]bool, len(errs)) for _, expectedError := range tc.errors { found := false - for _, err := range errs { - if expectedError.matches(err) { + for i, err := range errs { + if expectedError.matches(err) && !seenErrs[i] { found = true + seenErrs[i] = true break } } @@ -161,5 +180,281 @@ func TestValidateCustomResourceDefinition(t *testing.T) { t.Errorf("%s: expected %v at %v, got %v", tc.name, expectedError.errorType, expectedError.path.String(), errs) } } + + for i, seen := range seenErrs { + if !seen { + t.Errorf("%s: unexpected error: %v", tc.name, errs[i]) + } + } + } +} + +func TestValidateCustomResourceDefinitionUpdate(t *testing.T) { + tests := []struct { + name string + old *apiextensions.CustomResourceDefinition + resource *apiextensions.CustomResourceDefinition + errors []validationMatch + }{ + { + name: "unchanged", + old: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "plural.group.com", + ResourceVersion: "42", + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Version: "version", + Scope: apiextensions.ResourceScope("Cluster"), + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "kind", + ListKind: "listkind", + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + AcceptedNames: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "kind", + ListKind: "listkind", + }, + }, + }, + resource: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "plural.group.com", + ResourceVersion: "42", + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Version: "version", + Scope: apiextensions.ResourceScope("Cluster"), + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "kind", + ListKind: "listkind", + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + AcceptedNames: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "kind", + ListKind: "listkind", + }, + }, + }, + errors: []validationMatch{}, + }, + { + name: "unchanged-established", + old: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "plural.group.com", + ResourceVersion: "42", + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Version: "version", + Scope: apiextensions.ResourceScope("Cluster"), + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "kind", + ListKind: "listkind", + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + AcceptedNames: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "kind", + ListKind: "listkind", + }, + Conditions: []apiextensions.CustomResourceDefinitionCondition{ + {Type: apiextensions.Established, Status: apiextensions.ConditionTrue}, + }, + }, + }, + resource: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "plural.group.com", + ResourceVersion: "42", + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Version: "version", + Scope: apiextensions.ResourceScope("Cluster"), + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "kind", + ListKind: "listkind", + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + AcceptedNames: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "kind", + ListKind: "listkind", + }, + }, + }, + errors: []validationMatch{}, + }, + { + name: "changes", + old: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "plural.group.com", + ResourceVersion: "42", + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Version: "version", + Scope: apiextensions.ResourceScope("Cluster"), + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "kind", + ListKind: "listkind", + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + AcceptedNames: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "kind", + ListKind: "listkind", + }, + Conditions: []apiextensions.CustomResourceDefinitionCondition{ + {Type: apiextensions.Established, Status: apiextensions.ConditionFalse}, + }, + }, + }, + resource: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "plural.group.com", + ResourceVersion: "42", + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "abc.com", + Version: "version2", + Scope: apiextensions.ResourceScope("Namespaced"), + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural2", + Singular: "singular2", + Kind: "kind2", + ListKind: "listkind2", + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + AcceptedNames: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural2", + Singular: "singular2", + Kind: "kind2", + ListKind: "listkind2", + }, + }, + }, + errors: []validationMatch{ + immutable("spec", "group"), + immutable("spec", "names", "plural"), + }, + }, + { + name: "changes-established", + old: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "plural.group.com", + ResourceVersion: "42", + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "group.com", + Version: "version", + Scope: apiextensions.ResourceScope("Cluster"), + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "kind", + ListKind: "listkind", + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + AcceptedNames: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural", + Singular: "singular", + Kind: "kind", + ListKind: "listkind", + }, + Conditions: []apiextensions.CustomResourceDefinitionCondition{ + {Type: apiextensions.Established, Status: apiextensions.ConditionTrue}, + }, + }, + }, + resource: &apiextensions.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "plural.group.com", + ResourceVersion: "42", + }, + Spec: apiextensions.CustomResourceDefinitionSpec{ + Group: "abc.com", + Version: "version2", + Scope: apiextensions.ResourceScope("Namespaced"), + Names: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural2", + Singular: "singular2", + Kind: "kind2", + ListKind: "listkind2", + }, + }, + Status: apiextensions.CustomResourceDefinitionStatus{ + AcceptedNames: apiextensions.CustomResourceDefinitionNames{ + Plural: "plural2", + Singular: "singular2", + Kind: "kind2", + ListKind: "listkind2", + }, + }, + }, + errors: []validationMatch{ + immutable("spec", "group"), + immutable("spec", "version"), + immutable("spec", "scope"), + immutable("spec", "names", "kind"), + immutable("spec", "names", "plural"), + }, + }, + } + + for _, tc := range tests { + errs := ValidateCustomResourceDefinitionUpdate(tc.resource, tc.old) + seenErrs := make([]bool, len(errs)) + + for _, expectedError := range tc.errors { + found := false + for i, err := range errs { + if expectedError.matches(err) && !seenErrs[i] { + found = true + seenErrs[i] = true + break + } + } + + if !found { + t.Errorf("%s: expected %v at %v, got %v", tc.name, expectedError.errorType, expectedError.path.String(), errs) + } + } + + for i, seen := range seenErrs { + if !seen { + t.Errorf("%s: unexpected error: %v", tc.name, errs[i]) + } + } } } 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..b788ec7b0c9 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,12 +119,12 @@ 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{ - Type: apiextensions.NameConflict, + namesAcceptedCondition := apiextensions.CustomResourceDefinitionCondition{ + Type: apiextensions.NamesAccepted, 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() + namesAcceptedCondition.Status = apiextensions.ConditionFalse + namesAcceptedCondition.Reason = "PluralConflict" + namesAcceptedCondition.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() + namesAcceptedCondition.Status = apiextensions.ConditionFalse + namesAcceptedCondition.Reason = "SingularConflict" + namesAcceptedCondition.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() + namesAcceptedCondition.Status = apiextensions.ConditionFalse + namesAcceptedCondition.Reason = "ShortNamesConflict" + namesAcceptedCondition.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() + namesAcceptedCondition.Status = apiextensions.ConditionFalse + namesAcceptedCondition.Reason = "KindConflict" + namesAcceptedCondition.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() + namesAcceptedCondition.Status = apiextensions.ConditionFalse + namesAcceptedCondition.Reason = "ListKindConflict" + namesAcceptedCondition.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 namesAcceptedCondition.Status == apiextensions.ConditionUnknown { + namesAcceptedCondition.Status = apiextensions.ConditionTrue + namesAcceptedCondition.Reason = "NoConflicts" + namesAcceptedCondition.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 && namesAcceptedCondition.Status == apiextensions.ConditionTrue { + 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, namesAcceptedCondition, 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) - // nothing to do if accepted names and NameConflict condition didn't change + acceptedNames, namingCondition, establishedCondition := c.calculateNamesAndConditions(inCustomResourceDefinition) + + // nothing to do if accepted names and NamesAccepted 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.NamesAccepted)) && + 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..e9a7627758f 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,30 +88,45 @@ func (b *crdBuilder) NewOrDie() *apiextensions.CustomResourceDefinition { return &b.curr } -var goodCondition = apiextensions.CustomResourceDefinitionCondition{ - Type: apiextensions.NameConflict, - Status: apiextensions.ConditionFalse, +var acceptedCondition = apiextensions.CustomResourceDefinitionCondition{ + Type: apiextensions.NamesAccepted, + Status: apiextensions.ConditionTrue, 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, + Type: apiextensions.NamesAccepted, + Status: apiextensions.ConditionFalse, Reason: reason, Message: message, } } +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("PluralConflict", `"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("SingularConflict", `"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("ShortNamesConflict", `"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("KindConflict", `"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("ListKindConflict", `"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("ListKindConflict", `"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("ListKindConflict", `"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("PluralConflict", `"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("PluralConflict", `"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) } }