diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index 91780b92a..81e438005 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher.go @@ -35,13 +35,14 @@ import ( ) const ( - typeReady = "Ready" - typeIssuerURLValid = "IssuerURLValid" - typeOneTLSSecretPerIssuerHostname = "OneTLSSecretPerIssuerHostname" - typeIssuerIsUnique = "IssuerIsUnique" - typeIdentityProvidersFound = "IdentityProvidersFound" - typeDisplayNamesUnique = "DisplayNamesUnique" - typeAPIGroupSuffixValid = "APIGroupSuffixValid" + typeReady = "Ready" + typeIssuerURLValid = "IssuerURLValid" + typeOneTLSSecretPerIssuerHostname = "OneTLSSecretPerIssuerHostname" + typeIssuerIsUnique = "IssuerIsUnique" + typeIdentityProvidersFound = "IdentityProvidersFound" + typeIdentityProvidersDisplayNamesUnique = "IdentityProvidersDisplayNamesUnique" + typeIdentityProvidersAPIGroupSuffixValid = "IdentityProvidersObjectRefAPIGroupSuffixValid" + typeIdentityProvidersObjectRefKindValid = "IdentityProvidersObjectRefKindValid" reasonSuccess = "Success" reasonNotReady = "NotReady" @@ -54,7 +55,12 @@ const ( reasonIdentityProvidersObjectRefsNotFound = "IdentityProvidersObjectRefsNotFound" reasonIdentityProviderNotSpecified = "IdentityProviderNotSpecified" reasonDuplicateDisplayNames = "DuplicateDisplayNames" - reasonAPIGroupNameUnrecognized = "APIGroupNameUnrecognized" + reasonAPIGroupNameUnrecognized = "APIGroupUnrecognized" + reasonKindUnrecognized = "KindUnrecognized" + + kindLDAPIdentityProvider = "LDAPIdentityProvider" + kindOIDCIdentityProvider = "OIDCIdentityProvider" + kindActiveDirectoryIdentityProvider = "ActiveDirectoryIdentityProvider" celTransformerMaxExpressionRuntime = 5 * time.Second ) @@ -78,6 +84,7 @@ type federationDomainWatcherController struct { activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer celTransformer *celtransformer.CELTransformer + allowedKinds sets.Set[string] } // NewFederationDomainWatcherController creates a controllerlib.Controller that watches @@ -93,6 +100,7 @@ func NewFederationDomainWatcherController( activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, ) controllerlib.Controller { + allowedKinds := sets.New(kindActiveDirectoryIdentityProvider, kindLDAPIdentityProvider, kindOIDCIdentityProvider) return controllerlib.New( controllerlib.Config{ Name: "FederationDomainWatcherController", @@ -105,6 +113,7 @@ func NewFederationDomainWatcherController( oidcIdentityProviderInformer: oidcIdentityProviderInformer, ldapIdentityProviderInformer: ldapIdentityProviderInformer, activeDirectoryIdentityProviderInformer: activeDirectoryIdentityProviderInformer, + allowedKinds: allowedKinds, }, }, withInformer( @@ -306,8 +315,9 @@ func (c *federationDomainWatcherController) makeLegacyFederationDomainIssuer( federationDomainIssuer, err := federationdomainproviders.NewFederationDomainIssuerWithDefaultIDP(federationDomain.Spec.Issuer, defaultFederationDomainIdentityProvider) conditions = appendIssuerURLValidCondition(err, conditions) - conditions = appendDuplicateDisplayNamesCondition(sets.Set[string]{}, conditions) - conditions = appendAPIGroupSuffixCondition(c.apiGroup, sets.Set[string]{}, conditions) + conditions = appendIdentityProviderDuplicateDisplayNamesCondition(sets.Set[string]{}, conditions) + conditions = appendIdentityProviderObjectRefAPIGroupSuffixCondition(c.apiGroup, []string{}, conditions) + conditions = appendIdentityProviderObjectRefKindCondition(c.sortedAllowedKinds(), []string{}, conditions) return federationDomainIssuer, conditions, nil } @@ -320,30 +330,47 @@ func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplic idpNotFoundIndices := []int{} displayNames := sets.Set[string]{} duplicateDisplayNames := sets.Set[string]{} - badAPIGroupNames := sets.Set[string]{} + badAPIGroupNames := []string{} + badKinds := []string{} for index, idp := range federationDomain.Spec.IdentityProviders { + // The CRD requires the displayName field, and validates that it has at least one character, + // so here we only need to validate that they are unique. if displayNames.Has(idp.DisplayName) { duplicateDisplayNames.Insert(idp.DisplayName) } displayNames.Insert(idp.DisplayName) - apiGroup := "nil" + // The objectRef is a required field in the CRD, so it will always exist in practice. + // objectRef.name and objectRef.kind are required, but may be empty strings. + // objectRef.apiGroup is not required, however, so it may be nil or empty string. + canTryToFindIDP := true + apiGroup := "" if idp.ObjectRef.APIGroup != nil { apiGroup = *idp.ObjectRef.APIGroup } if apiGroup != c.apiGroup { - badAPIGroupNames.Insert(apiGroup) + badAPIGroupNames = append(badAPIGroupNames, apiGroup) + canTryToFindIDP = false + } + if !c.allowedKinds.Has(idp.ObjectRef.Kind) { + badKinds = append(badKinds, idp.ObjectRef.Kind) + canTryToFindIDP = false } - // Validate that each objectRef resolves to an existing IDP. It does not matter if the IDP itself - // is phase=Ready, because it will not be loaded into the cache if not ready. For each objectRef - // that does not resolve, put an error on the FederationDomain status. - idpResourceUID, idpWasFound, err := c.findIDPsUIDByObjectRef(idp.ObjectRef, federationDomain.Namespace) - if err != nil { - return nil, nil, err + var idpResourceUID types.UID + idpWasFound := false + if canTryToFindIDP { + var err error + // Validate that each objectRef resolves to an existing IDP. It does not matter if the IDP itself + // is phase=Ready, because it will not be loaded into the cache if not ready. For each objectRef + // that does not resolve, put an error on the FederationDomain status. + idpResourceUID, idpWasFound, err = c.findIDPsUIDByObjectRef(idp.ObjectRef, federationDomain.Namespace) + if err != nil { + return nil, nil, err + } } - if !idpWasFound { + if !canTryToFindIDP || !idpWasFound { idpNotFoundIndices = append(idpNotFoundIndices, index) } @@ -391,26 +418,27 @@ func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplic federationDomainIssuer, err := federationdomainproviders.NewFederationDomainIssuer(federationDomain.Spec.Issuer, federationDomainIdentityProviders) conditions = appendIssuerURLValidCondition(err, conditions) - conditions = appendDuplicateDisplayNamesCondition(duplicateDisplayNames, conditions) - conditions = appendAPIGroupSuffixCondition(c.apiGroup, badAPIGroupNames, conditions) + conditions = appendIdentityProviderDuplicateDisplayNamesCondition(duplicateDisplayNames, conditions) + conditions = appendIdentityProviderObjectRefAPIGroupSuffixCondition(c.apiGroup, badAPIGroupNames, conditions) + conditions = appendIdentityProviderObjectRefKindCondition(c.sortedAllowedKinds(), badKinds, conditions) return federationDomainIssuer, conditions, nil } - func (c *federationDomainWatcherController) findIDPsUIDByObjectRef(objectRef corev1.TypedLocalObjectReference, namespace string) (types.UID, bool, error) { var idpResourceUID types.UID var foundIDP metav1.Object var err error switch objectRef.Kind { - case "LDAPIdentityProvider": + case kindLDAPIdentityProvider: foundIDP, err = c.ldapIdentityProviderInformer.Lister().LDAPIdentityProviders(namespace).Get(objectRef.Name) - case "ActiveDirectoryIdentityProvider": + case kindActiveDirectoryIdentityProvider: foundIDP, err = c.activeDirectoryIdentityProviderInformer.Lister().ActiveDirectoryIdentityProviders(namespace).Get(objectRef.Name) - case "OIDCIdentityProvider": + case kindOIDCIdentityProvider: foundIDP, err = c.oidcIdentityProviderInformer.Lister().OIDCIdentityProviders(namespace).Get(objectRef.Name) default: - // TODO: handle an IDP type that we do not understand by writing a status condition + // This shouldn't happen because this helper function is not called when the kind is invalid. + return "", false, fmt.Errorf("unexpected kind: %s", objectRef.Kind) } switch { @@ -419,7 +447,7 @@ func (c *federationDomainWatcherController) findIDPsUIDByObjectRef(objectRef cor case errors.IsNotFound(err): return "", false, nil default: - return "", false, err // unexpected error + return "", false, err // unexpected error from the informer } return idpResourceUID, true, nil } @@ -555,20 +583,42 @@ func (c *federationDomainWatcherController) makeTransformationPipelineForIdentit return pipeline, nil } -func appendAPIGroupSuffixCondition(expectedSuffixName string, badSuffixNames sets.Set[string], conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition { - if badSuffixNames.Len() > 0 { - badNames := badSuffixNames.UnsortedList() - sort.Strings(badNames) +func (c *federationDomainWatcherController) sortedAllowedKinds() []string { + return sortAndQuote(c.allowedKinds.UnsortedList()) +} + +func appendIdentityProviderObjectRefKindCondition(expectedKinds []string, badSuffixNames []string, conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition { + if len(badSuffixNames) > 0 { conditions = append(conditions, &configv1alpha1.Condition{ - Type: typeAPIGroupSuffixValid, + Type: typeIdentityProvidersObjectRefKindValid, Status: configv1alpha1.ConditionFalse, - Reason: reasonAPIGroupNameUnrecognized, - Message: fmt.Sprintf("the API groups specified by .spec.identityProviders[].objectRef.apiGroup are not recognized (should be %q): %s", - expectedSuffixName, strings.Join(badNames, ", ")), + Reason: reasonKindUnrecognized, + Message: fmt.Sprintf("the kinds specified by .spec.identityProviders[].objectRef.kind are not recognized (should be one of %s): %s", + strings.Join(expectedKinds, ", "), strings.Join(sortAndQuote(badSuffixNames), ", ")), }) } else { conditions = append(conditions, &configv1alpha1.Condition{ - Type: typeAPIGroupSuffixValid, + Type: typeIdentityProvidersObjectRefKindValid, + Status: configv1alpha1.ConditionTrue, + Reason: reasonSuccess, + Message: "the kinds specified by .spec.identityProviders[].objectRef.kind are recognized", + }) + } + return conditions +} + +func appendIdentityProviderObjectRefAPIGroupSuffixCondition(expectedSuffixName string, badSuffixNames []string, conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition { + if len(badSuffixNames) > 0 { + conditions = append(conditions, &configv1alpha1.Condition{ + Type: typeIdentityProvidersAPIGroupSuffixValid, + Status: configv1alpha1.ConditionFalse, + Reason: reasonAPIGroupNameUnrecognized, + Message: fmt.Sprintf("the API groups specified by .spec.identityProviders[].objectRef.apiGroup are not recognized (should be %q): %s", + expectedSuffixName, strings.Join(sortAndQuote(badSuffixNames), ", ")), + }) + } else { + conditions = append(conditions, &configv1alpha1.Condition{ + Type: typeIdentityProvidersAPIGroupSuffixValid, Status: configv1alpha1.ConditionTrue, Reason: reasonSuccess, Message: "the API groups specified by .spec.identityProviders[].objectRef.apiGroup are recognized", @@ -577,20 +627,18 @@ func appendAPIGroupSuffixCondition(expectedSuffixName string, badSuffixNames set return conditions } -func appendDuplicateDisplayNamesCondition(duplicateDisplayNames sets.Set[string], conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition { +func appendIdentityProviderDuplicateDisplayNamesCondition(duplicateDisplayNames sets.Set[string], conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition { if duplicateDisplayNames.Len() > 0 { - duplicates := duplicateDisplayNames.UnsortedList() - sort.Strings(duplicates) conditions = append(conditions, &configv1alpha1.Condition{ - Type: typeDisplayNamesUnique, + Type: typeIdentityProvidersDisplayNamesUnique, Status: configv1alpha1.ConditionFalse, Reason: reasonDuplicateDisplayNames, Message: fmt.Sprintf("the names specified by .spec.identityProviders[].displayName contain duplicates: %s", - strings.Join(duplicates, ", ")), + strings.Join(sortAndQuote(duplicateDisplayNames.UnsortedList()), ", ")), }) } else { conditions = append(conditions, &configv1alpha1.Condition{ - Type: typeDisplayNamesUnique, + Type: typeIdentityProvidersDisplayNamesUnique, Status: configv1alpha1.ConditionTrue, Reason: reasonSuccess, Message: "the names specified by .spec.identityProviders[].displayName are unique", @@ -620,6 +668,15 @@ func appendIssuerURLValidCondition(err error, conditions []*configv1alpha1.Condi return conditions } +func sortAndQuote(strs []string) []string { + quoted := make([]string, 0, len(strs)) + for _, s := range strs { + quoted = append(quoted, fmt.Sprintf("%q", s)) + } + sort.Strings(quoted) + return quoted +} + func (c *federationDomainWatcherController) updateStatus( ctx context.Context, federationDomain *configv1alpha1.FederationDomain, diff --git a/internal/controller/supervisorconfig/federation_domain_watcher_test.go b/internal/controller/supervisorconfig/federation_domain_watcher_test.go index 10884cc78..8cad14e11 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher_test.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher_test.go @@ -364,20 +364,20 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { } } - sadIdentityProvidersFoundConditionIdentityProvidersObjectRefsNotFound := func(msg string, time metav1.Time, observedGeneration int64) configv1alpha1.Condition { + sadIdentityProvidersFoundConditionIdentityProvidersObjectRefsNotFound := func(idpsNotFound string, time metav1.Time, observedGeneration int64) configv1alpha1.Condition { return configv1alpha1.Condition{ Type: "IdentityProvidersFound", Status: "False", ObservedGeneration: observedGeneration, LastTransitionTime: time, Reason: "IdentityProvidersObjectRefsNotFound", - Message: msg, + Message: fmt.Sprintf(".spec.identityProviders[].objectRef identifies resource(s) that cannot be found: %s", idpsNotFound), } } happyDisplayNamesUniqueCondition := func(time metav1.Time, observedGeneration int64) configv1alpha1.Condition { return configv1alpha1.Condition{ - Type: "DisplayNamesUnique", + Type: "IdentityProvidersDisplayNamesUnique", Status: "True", ObservedGeneration: observedGeneration, LastTransitionTime: time, @@ -388,7 +388,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { sadDisplayNamesUniqueCondition := func(duplicateNames string, time metav1.Time, observedGeneration int64) configv1alpha1.Condition { return configv1alpha1.Condition{ - Type: "DisplayNamesUnique", + Type: "IdentityProvidersDisplayNamesUnique", Status: "False", ObservedGeneration: observedGeneration, LastTransitionTime: time, @@ -399,7 +399,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { happyAPIGroupSuffixCondition := func(time metav1.Time, observedGeneration int64) configv1alpha1.Condition { return configv1alpha1.Condition{ - Type: "APIGroupSuffixValid", + Type: "IdentityProvidersObjectRefAPIGroupSuffixValid", Status: "True", ObservedGeneration: observedGeneration, LastTransitionTime: time, @@ -408,20 +408,53 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { } } - sadAPIGroupSuffixCondition := func(badNames string, time metav1.Time, observedGeneration int64) configv1alpha1.Condition { + sadAPIGroupSuffixCondition := func(badApiGroups string, time metav1.Time, observedGeneration int64) configv1alpha1.Condition { return configv1alpha1.Condition{ - Type: "APIGroupSuffixValid", + Type: "IdentityProvidersObjectRefAPIGroupSuffixValid", Status: "False", ObservedGeneration: observedGeneration, LastTransitionTime: time, - Reason: "APIGroupNameUnrecognized", - Message: fmt.Sprintf("the API groups specified by .spec.identityProviders[].objectRef.apiGroup are not recognized (should be \"idp.supervisor.%s\"): %s", apiGroupSuffix, badNames), + Reason: "APIGroupUnrecognized", + Message: fmt.Sprintf("the API groups specified by .spec.identityProviders[].objectRef.apiGroup "+ + "are not recognized (should be \"idp.supervisor.%s\"): %s", apiGroupSuffix, badApiGroups), } } + happyKindCondition := func(time metav1.Time, observedGeneration int64) configv1alpha1.Condition { + return configv1alpha1.Condition{ + Type: "IdentityProvidersObjectRefKindValid", + Status: "True", + ObservedGeneration: observedGeneration, + LastTransitionTime: time, + Reason: "Success", + Message: "the kinds specified by .spec.identityProviders[].objectRef.kind are recognized", + } + } + + sadKindCondition := func(badKinds string, time metav1.Time, observedGeneration int64) configv1alpha1.Condition { + return configv1alpha1.Condition{ + Type: "IdentityProvidersObjectRefKindValid", + Status: "False", + ObservedGeneration: observedGeneration, + LastTransitionTime: time, + Reason: "KindUnrecognized", + Message: fmt.Sprintf(`the kinds specified by .spec.identityProviders[].objectRef.kind are `+ + `not recognized (should be one of "ActiveDirectoryIdentityProvider", "LDAPIdentityProvider", "OIDCIdentityProvider"): %s`, badKinds), + } + } + + sortConditionsByType := func(c []configv1alpha1.Condition) []configv1alpha1.Condition { + cp := make([]configv1alpha1.Condition, len(c)) + copy(cp, c) + sort.SliceStable(cp, func(i, j int) bool { + return cp[i].Type < cp[j].Type + }) + return cp + } + allHappyConditionsLegacyConfigurationSuccess := func(issuer string, idpName string, time metav1.Time, observedGeneration int64) []configv1alpha1.Condition { - return []configv1alpha1.Condition{ - // expect them to be sorted alphabetically by type + return sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(idpName, time, observedGeneration), @@ -429,12 +462,12 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { happyIssuerURLValidCondition(time, observedGeneration), happyOneTLSSecretPerIssuerHostnameCondition(time, observedGeneration), happyReadyCondition(issuer, time, observedGeneration), - } + }) } allHappyConditionsSuccess := func(issuer string, time metav1.Time, observedGeneration int64) []configv1alpha1.Condition { - return []configv1alpha1.Condition{ - // expect them to be sorted alphabetically by type + return sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionSuccess(frozenMetav1Now, 123), @@ -442,7 +475,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { happyIssuerURLValidCondition(frozenMetav1Now, 123), happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), happyReadyCondition(issuer, frozenMetav1Now, 123), - } + }) } invalidIssuerURL := ":/host//path" @@ -564,7 +597,8 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { wantStatusUpdates: []*configv1alpha1.FederationDomain{ expectedFederationDomainStatusUpdate(invalidIssuerURLFederationDomain, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), @@ -572,7 +606,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { sadIssuerURLValidConditionCannotHaveQuery(frozenMetav1Now, 123), happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }, + }), ), expectedFederationDomainStatusUpdate(federationDomain2, configv1alpha1.FederationDomainPhaseReady, @@ -609,7 +643,8 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { wantStatusUpdates: []*configv1alpha1.FederationDomain{ expectedFederationDomainStatusUpdate(invalidIssuerURLFederationDomain, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), @@ -617,7 +652,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { sadIssuerURLValidConditionCannotHaveQuery(frozenMetav1Now, 123), happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }, + }), ), expectedFederationDomainStatusUpdate(federationDomain2, configv1alpha1.FederationDomainPhaseReady, @@ -652,7 +687,8 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "duplicate1", Namespace: namespace, Generation: 123}, }, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), @@ -660,14 +696,15 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { happyIssuerURLValidCondition(frozenMetav1Now, 123), happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }, + }), ), expectedFederationDomainStatusUpdate( &configv1alpha1.FederationDomain{ ObjectMeta: metav1.ObjectMeta{Name: "duplicate2", Namespace: namespace, Generation: 123}, }, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), @@ -675,7 +712,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { happyIssuerURLValidCondition(frozenMetav1Now, 123), happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }, + }), ), expectedFederationDomainStatusUpdate( &configv1alpha1.FederationDomain{ @@ -731,7 +768,8 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "fd1", Namespace: namespace, Generation: 123}, }, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), @@ -739,14 +777,15 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { happyIssuerURLValidCondition(frozenMetav1Now, 123), sadOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }, + }), ), expectedFederationDomainStatusUpdate( &configv1alpha1.FederationDomain{ ObjectMeta: metav1.ObjectMeta{Name: "fd2", Namespace: namespace, Generation: 123}, }, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), @@ -754,14 +793,15 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { happyIssuerURLValidCondition(frozenMetav1Now, 123), sadOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }, + }), ), expectedFederationDomainStatusUpdate( &configv1alpha1.FederationDomain{ ObjectMeta: metav1.ObjectMeta{Name: "invalidIssuerURLFederationDomain", Namespace: namespace, Generation: 123}, }, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123), @@ -769,7 +809,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { sadIssuerURLValidConditionCannotParse(frozenMetav1Now, 123), unknownOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }, + }), ), expectedFederationDomainStatusUpdate( &configv1alpha1.FederationDomain{ @@ -790,7 +830,8 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { wantStatusUpdates: []*configv1alpha1.FederationDomain{ expectedFederationDomainStatusUpdate(federationDomain1, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), sadIdentityProvidersFoundConditionLegacyConfigurationIdentityProviderNotFound(frozenMetav1Now, 123), @@ -798,11 +839,12 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { happyIssuerURLValidCondition(frozenMetav1Now, 123), happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }, + }), ), expectedFederationDomainStatusUpdate(federationDomain2, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), sadIdentityProvidersFoundConditionLegacyConfigurationIdentityProviderNotFound(frozenMetav1Now, 123), @@ -810,7 +852,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { happyIssuerURLValidCondition(frozenMetav1Now, 123), happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }, + }), ), }, }, @@ -826,7 +868,8 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { wantStatusUpdates: []*configv1alpha1.FederationDomain{ expectedFederationDomainStatusUpdate(federationDomain1, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), sadIdentityProvidersFoundConditionIdentityProviderNotSpecified(3, frozenMetav1Now, 123), @@ -834,7 +877,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { happyIssuerURLValidCondition(frozenMetav1Now, 123), happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }, + }), ), }, }, @@ -881,12 +924,12 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123}, }, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), sadIdentityProvidersFoundConditionIdentityProvidersObjectRefsNotFound( - `.spec.identityProviders[].objectRef identifies resource(s) that cannot be found: `+ - `.spec.identityProviders[0] with displayName "cant-find-me", `+ + `.spec.identityProviders[0] with displayName "cant-find-me", `+ `.spec.identityProviders[1] with displayName "cant-find-me-either", `+ `.spec.identityProviders[2] with displayName "cant-find-me-still"`, frozenMetav1Now, 123), @@ -894,7 +937,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { happyIssuerURLValidCondition(frozenMetav1Now, 123), happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }, + }), ), }, }, @@ -1029,15 +1072,16 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123}, }, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), happyAPIGroupSuffixCondition(frozenMetav1Now, 123), - sadDisplayNamesUniqueCondition("duplicate1, duplicate2", frozenMetav1Now, 123), + sadDisplayNamesUniqueCondition(`"duplicate1", "duplicate2"`, frozenMetav1Now, 123), happyIdentityProvidersFoundConditionSuccess(frozenMetav1Now, 123), happyIssuerIsUniqueCondition(frozenMetav1Now, 123), happyIssuerURLValidCondition(frozenMetav1Now, 123), happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }), + })), }, }, { @@ -1062,7 +1106,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { { DisplayName: "name2", ObjectRef: corev1.TypedLocalObjectReference{ - APIGroup: pointer.String("also-wrong.example.com"), + APIGroup: pointer.String(""), // empty string is wrong Kind: "LDAPIdentityProvider", Name: ldapIdentityProvider.Name, }, @@ -1070,7 +1114,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { { DisplayName: "name3", ObjectRef: corev1.TypedLocalObjectReference{ - APIGroup: nil, // also wrong + APIGroup: nil, // nil is wrong, and gets treated like an empty string in the error condition Kind: "LDAPIdentityProvider", Name: ldapIdentityProvider.Name, }, @@ -1094,15 +1138,81 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123}, }, configv1alpha1.FederationDomainPhaseError, - []configv1alpha1.Condition{ - sadAPIGroupSuffixCondition("also-wrong.example.com, nil, wrong.example.com", frozenMetav1Now, 123), + sortConditionsByType([]configv1alpha1.Condition{ + happyKindCondition(frozenMetav1Now, 123), + sadAPIGroupSuffixCondition(`"", "", "wrong.example.com"`, frozenMetav1Now, 123), happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), - happyIdentityProvidersFoundConditionSuccess(frozenMetav1Now, 123), + sadIdentityProvidersFoundConditionIdentityProvidersObjectRefsNotFound( + `.spec.identityProviders[0] with displayName "name1", `+ + `.spec.identityProviders[1] with displayName "name2", `+ + `.spec.identityProviders[2] with displayName "name3"`, + frozenMetav1Now, 123), happyIssuerIsUniqueCondition(frozenMetav1Now, 123), happyIssuerURLValidCondition(frozenMetav1Now, 123), happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), - }), + })), + }, + }, + { + name: "the federation domain has unrecognized kind names in objectRefs", + inputObjects: []runtime.Object{ + oidcIdentityProvider, + ldapIdentityProvider, + adIdentityProvider, + &configv1alpha1.FederationDomain{ + ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123}, + Spec: configv1alpha1.FederationDomainSpec{ + Issuer: "https://issuer1.com", + IdentityProviders: []configv1alpha1.FederationDomainIdentityProvider{ + { + DisplayName: "name1", + ObjectRef: corev1.TypedLocalObjectReference{ + APIGroup: pointer.String(apiGroupSupervisor), + Kind: "OIDCIdentityProvider", // correct + Name: oidcIdentityProvider.Name, + }, + }, + { + DisplayName: "name2", + ObjectRef: corev1.TypedLocalObjectReference{ + APIGroup: pointer.String(apiGroupSupervisor), + Kind: "wrong", + Name: ldapIdentityProvider.Name, + }, + }, + { + DisplayName: "name3", + ObjectRef: corev1.TypedLocalObjectReference{ + APIGroup: pointer.String(apiGroupSupervisor), + Kind: "", // empty is also wrong + Name: ldapIdentityProvider.Name, + }, + }, + }, + }, + }, + }, + wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{}, + wantStatusUpdates: []*configv1alpha1.FederationDomain{ + expectedFederationDomainStatusUpdate( + &configv1alpha1.FederationDomain{ + ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123}, + }, + configv1alpha1.FederationDomainPhaseError, + sortConditionsByType([]configv1alpha1.Condition{ + sadKindCondition(`"", "wrong"`, frozenMetav1Now, 123), + happyAPIGroupSuffixCondition(frozenMetav1Now, 123), + happyDisplayNamesUniqueCondition(frozenMetav1Now, 123), + sadIdentityProvidersFoundConditionIdentityProvidersObjectRefsNotFound( + `.spec.identityProviders[1] with displayName "name2", `+ + `.spec.identityProviders[2] with displayName "name3"`, + frozenMetav1Now, 123), + happyIssuerIsUniqueCondition(frozenMetav1Now, 123), + happyIssuerURLValidCondition(frozenMetav1Now, 123), + happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123), + sadReadyCondition(frozenMetav1Now, 123), + })), }, }, { diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index c971584a3..9e2b7807e 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -133,18 +133,24 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) { config6Duplicate1, _ := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer6, client) config6Duplicate2 := testlib.CreateTestFederationDomain(ctx, t, v1alpha1.FederationDomainSpec{Issuer: issuer6}, v1alpha1.FederationDomainPhaseError) requireStatus(t, client, ns, config6Duplicate1.Name, v1alpha1.FederationDomainPhaseError, map[string]v1alpha1.ConditionStatus{ - "Ready": v1alpha1.ConditionFalse, - "IssuerIsUnique": v1alpha1.ConditionFalse, - "IdentityProvidersFound": v1alpha1.ConditionTrue, - "OneTLSSecretPerIssuerHostname": v1alpha1.ConditionTrue, - "IssuerURLValid": v1alpha1.ConditionTrue, + "Ready": v1alpha1.ConditionFalse, + "IssuerIsUnique": v1alpha1.ConditionFalse, + "IdentityProvidersFound": v1alpha1.ConditionTrue, + "OneTLSSecretPerIssuerHostname": v1alpha1.ConditionTrue, + "IssuerURLValid": v1alpha1.ConditionTrue, + "IdentityProvidersObjectRefKindValid": v1alpha1.ConditionTrue, + "IdentityProvidersObjectRefAPIGroupSuffixValid": v1alpha1.ConditionTrue, + "IdentityProvidersDisplayNamesUnique": v1alpha1.ConditionTrue, }) requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.FederationDomainPhaseError, map[string]v1alpha1.ConditionStatus{ - "Ready": v1alpha1.ConditionFalse, - "IssuerIsUnique": v1alpha1.ConditionFalse, - "IdentityProvidersFound": v1alpha1.ConditionTrue, - "OneTLSSecretPerIssuerHostname": v1alpha1.ConditionTrue, - "IssuerURLValid": v1alpha1.ConditionTrue, + "Ready": v1alpha1.ConditionFalse, + "IssuerIsUnique": v1alpha1.ConditionFalse, + "IdentityProvidersFound": v1alpha1.ConditionTrue, + "OneTLSSecretPerIssuerHostname": v1alpha1.ConditionTrue, + "IssuerURLValid": v1alpha1.ConditionTrue, + "IdentityProvidersObjectRefKindValid": v1alpha1.ConditionTrue, + "IdentityProvidersObjectRefAPIGroupSuffixValid": v1alpha1.ConditionTrue, + "IdentityProvidersDisplayNamesUnique": v1alpha1.ConditionTrue, }) requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer6) @@ -164,11 +170,14 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) { // When we create a provider with an invalid issuer, the status is set to invalid. badConfig := testlib.CreateTestFederationDomain(ctx, t, v1alpha1.FederationDomainSpec{Issuer: badIssuer}, v1alpha1.FederationDomainPhaseError) requireStatus(t, client, ns, badConfig.Name, v1alpha1.FederationDomainPhaseError, map[string]v1alpha1.ConditionStatus{ - "Ready": v1alpha1.ConditionFalse, - "IssuerIsUnique": v1alpha1.ConditionTrue, - "IdentityProvidersFound": v1alpha1.ConditionTrue, - "OneTLSSecretPerIssuerHostname": v1alpha1.ConditionTrue, - "IssuerURLValid": v1alpha1.ConditionFalse, + "Ready": v1alpha1.ConditionFalse, + "IssuerIsUnique": v1alpha1.ConditionTrue, + "IdentityProvidersFound": v1alpha1.ConditionTrue, + "OneTLSSecretPerIssuerHostname": v1alpha1.ConditionTrue, + "IssuerURLValid": v1alpha1.ConditionFalse, + "IdentityProvidersObjectRefKindValid": v1alpha1.ConditionTrue, + "IdentityProvidersObjectRefAPIGroupSuffixValid": v1alpha1.ConditionTrue, + "IdentityProvidersDisplayNamesUnique": v1alpha1.ConditionTrue, }) requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, badIssuer) requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, badConfig, client, ns, scheme, addr, caBundle, badIssuer) @@ -677,11 +686,14 @@ func requireDelete(t *testing.T, client pinnipedclientset.Interface, ns, name st func requireFullySuccessfulStatus(t *testing.T, client pinnipedclientset.Interface, ns, name string) { requireStatus(t, client, ns, name, v1alpha1.FederationDomainPhaseReady, map[string]v1alpha1.ConditionStatus{ - "Ready": v1alpha1.ConditionTrue, - "IssuerIsUnique": v1alpha1.ConditionTrue, - "IdentityProvidersFound": v1alpha1.ConditionTrue, - "OneTLSSecretPerIssuerHostname": v1alpha1.ConditionTrue, - "IssuerURLValid": v1alpha1.ConditionTrue, + "Ready": v1alpha1.ConditionTrue, + "IssuerIsUnique": v1alpha1.ConditionTrue, + "IdentityProvidersFound": v1alpha1.ConditionTrue, + "OneTLSSecretPerIssuerHostname": v1alpha1.ConditionTrue, + "IssuerURLValid": v1alpha1.ConditionTrue, + "IdentityProvidersObjectRefKindValid": v1alpha1.ConditionTrue, + "IdentityProvidersObjectRefAPIGroupSuffixValid": v1alpha1.ConditionTrue, + "IdentityProvidersDisplayNamesUnique": v1alpha1.ConditionTrue, }) }