Validate apiGroup names are valid in federation_domain_watcher.go

internal/controller/supervisorconfig/federation_domain_watcher.go

@@ -41,6 +41,7 @@ const (
 	typeIssuerIsUnique                = "IssuerIsUnique"
 	typeIdentityProvidersFound        = "IdentityProvidersFound"
 	typeDisplayNamesUnique            = "DisplayNamesUnique"
+	typeAPIGroupSuffixValid           = "APIGroupSuffixValid"
 
 	reasonSuccess                                     = "Success"
 	reasonNotReady                                    = "NotReady"
@@ -53,6 +54,7 @@ const (
 	reasonIdentityProvidersObjectRefsNotFound         = "IdentityProvidersObjectRefsNotFound"
 	reasonIdentityProviderNotSpecified                = "IdentityProviderNotSpecified"
 	reasonDuplicateDisplayNames                       = "DuplicateDisplayNames"
+	reasonAPIGroupNameUnrecognized                    = "APIGroupNameUnrecognized"
 
 	celTransformerMaxExpressionRuntime = 5 * time.Second
 )
@@ -66,6 +68,7 @@ type FederationDomainsSetter interface {
 
 type federationDomainWatcherController struct {
 	federationDomainsSetter FederationDomainsSetter
+	apiGroup                string
 	clock                   clock.Clock
 	client                  pinnipedclientset.Interface
 
@@ -81,6 +84,7 @@ type federationDomainWatcherController struct {
 // FederationDomain objects and notifies a callback object of the collection of provider configs.
 func NewFederationDomainWatcherController(
 	federationDomainsSetter FederationDomainsSetter,
+	apiGroupSuffix string,
 	clock clock.Clock,
 	client pinnipedclientset.Interface,
 	federationDomainInformer configinformers.FederationDomainInformer,
@@ -94,6 +98,7 @@ func NewFederationDomainWatcherController(
 			Name: "FederationDomainWatcherController",
 			Syncer: &federationDomainWatcherController{
 				federationDomainsSetter:                 federationDomainsSetter,
+				apiGroup:                                fmt.Sprintf("idp.supervisor.%s", apiGroupSuffix),
 				clock:                                   clock,
 				client:                                  client,
 				federationDomainInformer:                federationDomainInformer,
@@ -297,12 +302,13 @@ func (c *federationDomainWatcherController) makeLegacyFederationDomainIssuer(
 		})
 	}
 
-	conditions = c.addDuplicateDisplayNamesCondition(sets.Set[string]{}, conditions)
-
 	// This is the constructor for the backwards compatibility mode.
 	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)
+
 	return federationDomainIssuer, conditions, nil
 }
 
@@ -314,6 +320,7 @@ func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplic
 	idpNotFoundIndices := []int{}
 	displayNames := sets.Set[string]{}
 	duplicateDisplayNames := sets.Set[string]{}
+	badAPIGroupNames := sets.Set[string]{}
 
 	for index, idp := range federationDomain.Spec.IdentityProviders {
 		if displayNames.Has(idp.DisplayName) {
@@ -321,7 +328,13 @@ func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplic
 		}
 		displayNames.Insert(idp.DisplayName)
 
-		// TODO: Validate that idp.ObjectRef.APIGroup is the expected APIGroup for IDP CRs "idp.supervisor.pinniped.dev" where .pinniped.dev is the configurable suffix
+		apiGroup := "nil"
+		if idp.ObjectRef.APIGroup != nil {
+			apiGroup = *idp.ObjectRef.APIGroup
+		}
+		if apiGroup != c.apiGroup {
+			badAPIGroupNames.Insert(apiGroup)
+		}
 
 		// 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
@@ -374,34 +387,14 @@ func (c *federationDomainWatcherController) makeFederationDomainIssuerWithExplic
 		})
 	}
 
-	conditions = c.addDuplicateDisplayNamesCondition(duplicateDisplayNames, conditions)
-
 	// This is the constructor for any case other than the legacy case, including when there is an empty list of IDPs.
 	federationDomainIssuer, err := federationdomainproviders.NewFederationDomainIssuer(federationDomain.Spec.Issuer, federationDomainIdentityProviders)
 	conditions = appendIssuerURLValidCondition(err, conditions)
-	return federationDomainIssuer, conditions, nil
-}
 
-func (c *federationDomainWatcherController) addDuplicateDisplayNamesCondition(duplicateDisplayNames sets.Set[string], conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition {
+	conditions = appendDuplicateDisplayNamesCondition(duplicateDisplayNames, conditions)
-	if duplicateDisplayNames.Len() > 0 {
+	conditions = appendAPIGroupSuffixCondition(c.apiGroup, badAPIGroupNames, conditions)
-		duplicates := duplicateDisplayNames.UnsortedList()
+
-		sort.Strings(duplicates)
+	return federationDomainIssuer, conditions, nil
-		conditions = append(conditions, &configv1alpha1.Condition{
-			Type:   typeDisplayNamesUnique,
-			Status: configv1alpha1.ConditionFalse,
-			Reason: reasonDuplicateDisplayNames,
-			Message: fmt.Sprintf("the names specified by .spec.identityProviders[].displayName contain duplicates: %s",
-				strings.Join(duplicates, ", ")),
-		})
-	} else {
-		conditions = append(conditions, &configv1alpha1.Condition{
-			Type:    typeDisplayNamesUnique,
-			Status:  configv1alpha1.ConditionTrue,
-			Reason:  reasonSuccess,
-			Message: "the names specified by .spec.identityProviders[].displayName are unique",
-		})
-	}
-	return conditions
 }
 
 func (c *federationDomainWatcherController) findIDPsUIDByObjectRef(objectRef corev1.TypedLocalObjectReference, namespace string) (types.UID, bool, error) {
@@ -562,6 +555,50 @@ 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)
+		conditions = append(conditions, &configv1alpha1.Condition{
+			Type:   typeAPIGroupSuffixValid,
+			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, ", ")),
+		})
+	} else {
+		conditions = append(conditions, &configv1alpha1.Condition{
+			Type:    typeAPIGroupSuffixValid,
+			Status:  configv1alpha1.ConditionTrue,
+			Reason:  reasonSuccess,
+			Message: "the API groups specified by .spec.identityProviders[].objectRef.apiGroup are recognized",
+		})
+	}
+	return conditions
+}
+
+func appendDuplicateDisplayNamesCondition(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,
+			Status: configv1alpha1.ConditionFalse,
+			Reason: reasonDuplicateDisplayNames,
+			Message: fmt.Sprintf("the names specified by .spec.identityProviders[].displayName contain duplicates: %s",
+				strings.Join(duplicates, ", ")),
+		})
+	} else {
+		conditions = append(conditions, &configv1alpha1.Condition{
+			Type:    typeDisplayNamesUnique,
+			Status:  configv1alpha1.ConditionTrue,
+			Reason:  reasonSuccess,
+			Message: "the names specified by .spec.identityProviders[].displayName are unique",
+		})
+	}
+	return conditions
+}
+
 func appendIssuerURLValidCondition(err error, conditions []*configv1alpha1.Condition) []*configv1alpha1.Condition {
 	if err != nil {
 		// Note that the FederationDomainIssuer constructors only validate the Issuer URL,

internal/controller/supervisorconfig/federation_domain_watcher_test.go

@@ -89,6 +89,7 @@ func TestFederationDomainWatcherControllerInformerFilters(t *testing.T) {
 
 			NewFederationDomainWatcherController(
 				nil,
+				"",
 				nil,
 				nil,
 				federationDomainInformer,
@@ -128,7 +129,8 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 	t.Parallel()
 
 	const namespace = "some-namespace"
-	const apiGroupSupervisor = "idp.supervisor.pinniped.dev"
+	const apiGroupSuffix = "custom.suffix.pinniped.dev"
+	const apiGroupSupervisor = "idp.supervisor." + apiGroupSuffix
 
 	frozenNow := time.Date(2020, time.September, 23, 7, 42, 0, 0, time.Local)
 	frozenMetav1Now := metav1.NewTime(frozenNow)
@@ -395,9 +397,32 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 		}
 	}
 
+	happyAPIGroupSuffixCondition := func(time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
+		return configv1alpha1.Condition{
+			Type:               "APIGroupSuffixValid",
+			Status:             "True",
+			ObservedGeneration: observedGeneration,
+			LastTransitionTime: time,
+			Reason:             "Success",
+			Message:            "the API groups specified by .spec.identityProviders[].objectRef.apiGroup are recognized",
+		}
+	}
+
+	sadAPIGroupSuffixCondition := func(badNames string, time metav1.Time, observedGeneration int64) configv1alpha1.Condition {
+		return configv1alpha1.Condition{
+			Type:               "APIGroupSuffixValid",
+			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),
+		}
+	}
+
 	allHappyConditionsLegacyConfigurationSuccess := func(issuer string, idpName string, time metav1.Time, observedGeneration int64) []configv1alpha1.Condition {
 		return []configv1alpha1.Condition{
 			// expect them to be sorted alphabetically by type
+			happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 			happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 			happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(idpName, time, observedGeneration),
 			happyIssuerIsUniqueCondition(time, observedGeneration),
@@ -410,6 +435,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 	allHappyConditionsSuccess := func(issuer string, time metav1.Time, observedGeneration int64) []configv1alpha1.Condition {
 		return []configv1alpha1.Condition{
 			// expect them to be sorted alphabetically by type
+			happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 			happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 			happyIdentityProvidersFoundConditionSuccess(frozenMetav1Now, 123),
 			happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
@@ -539,6 +565,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				expectedFederationDomainStatusUpdate(invalidIssuerURLFederationDomain,
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
@@ -583,6 +610,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				expectedFederationDomainStatusUpdate(invalidIssuerURLFederationDomain,
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
@@ -625,6 +653,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						sadIssuerIsUniqueCondition(frozenMetav1Now, 123),
@@ -639,6 +668,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						sadIssuerIsUniqueCondition(frozenMetav1Now, 123),
@@ -702,6 +732,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
@@ -716,6 +747,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
@@ -730,6 +762,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionLegacyConfigurationSuccess(oidcIdentityProvider.Name, frozenMetav1Now, 123),
 						unknownIssuerIsUniqueCondition(frozenMetav1Now, 123),
@@ -758,6 +791,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				expectedFederationDomainStatusUpdate(federationDomain1,
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						sadIdentityProvidersFoundConditionLegacyConfigurationIdentityProviderNotFound(frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
@@ -769,6 +803,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				expectedFederationDomainStatusUpdate(federationDomain2,
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						sadIdentityProvidersFoundConditionLegacyConfigurationIdentityProviderNotFound(frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
@@ -792,6 +827,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				expectedFederationDomainStatusUpdate(federationDomain1,
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						sadIdentityProvidersFoundConditionIdentityProviderNotSpecified(3, frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
@@ -846,6 +882,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
 						sadIdentityProvidersFoundConditionIdentityProvidersObjectRefsNotFound(
 							`.spec.identityProviders[].objectRef identifies resource(s) that cannot be found: `+
@@ -993,6 +1030,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					},
 					configv1alpha1.FederationDomainPhaseError,
 					[]configv1alpha1.Condition{
+						happyAPIGroupSuffixCondition(frozenMetav1Now, 123),
 						sadDisplayNamesUniqueCondition("duplicate1, duplicate2", frozenMetav1Now, 123),
 						happyIdentityProvidersFoundConditionSuccess(frozenMetav1Now, 123),
 						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
@@ -1002,6 +1040,71 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					}),
 			},
 		},
+		{
+			name: "the federation domain has unrecognized api group 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("wrong.example.com"),
+									Kind:     "OIDCIdentityProvider",
+									Name:     oidcIdentityProvider.Name,
+								},
+							},
+							{
+								DisplayName: "name2",
+								ObjectRef: corev1.TypedLocalObjectReference{
+									APIGroup: pointer.String("also-wrong.example.com"),
+									Kind:     "LDAPIdentityProvider",
+									Name:     ldapIdentityProvider.Name,
+								},
+							},
+							{
+								DisplayName: "name3",
+								ObjectRef: corev1.TypedLocalObjectReference{
+									APIGroup: nil, // also wrong
+									Kind:     "LDAPIdentityProvider",
+									Name:     ldapIdentityProvider.Name,
+								},
+							},
+							{
+								DisplayName: "name4",
+								ObjectRef: corev1.TypedLocalObjectReference{
+									APIGroup: pointer.String(apiGroupSupervisor), // correct
+									Kind:     "ActiveDirectoryIdentityProvider",
+									Name:     adIdentityProvider.Name,
+								},
+							},
+						},
+					},
+				},
+			},
+			wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{},
+			wantStatusUpdates: []*configv1alpha1.FederationDomain{
+				expectedFederationDomainStatusUpdate(
+					&configv1alpha1.FederationDomain{
+						ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
+					},
+					configv1alpha1.FederationDomainPhaseError,
+					[]configv1alpha1.Condition{
+						sadAPIGroupSuffixCondition("also-wrong.example.com, nil, wrong.example.com", frozenMetav1Now, 123),
+						happyDisplayNamesUniqueCondition(frozenMetav1Now, 123),
+						happyIdentityProvidersFoundConditionSuccess(frozenMetav1Now, 123),
+						happyIssuerIsUniqueCondition(frozenMetav1Now, 123),
+						happyIssuerURLValidCondition(frozenMetav1Now, 123),
+						happyOneTLSSecretPerIssuerHostnameCondition(frozenMetav1Now, 123),
+						sadReadyCondition(frozenMetav1Now, 123),
+					}),
+			},
+		},
 		{
 			name: "the federation domain specifies illegal const type, which shouldn't really happen since the CRD validates it",
 			inputObjects: []runtime.Object{
@@ -1083,6 +1186,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 
 			controller := NewFederationDomainWatcherController(
 				federationDomainsSetter,
+				apiGroupSuffix,
 				clocktesting.NewFakeClock(frozenNow),
 				pinnipedAPIClient,
 				pinnipedInformers.Config().V1alpha1().FederationDomains(),

internal/supervisor/server/server.go

@@ -167,6 +167,7 @@ func prepareControllers(
 		WithController(
 			supervisorconfig.NewFederationDomainWatcherController(
 				issuerManager,
+				*cfg.APIGroupSuffix,
 				clock.RealClock{},
 				pinnipedClient,
 				federationDomainInformer,