Allow GitHubIdentityProvider IDP type by FederationDomainWatcher

internal/controller/supervisorconfig/federation_domain_watcher.go

@@ -67,6 +67,7 @@ const (
 	kindLDAPIdentityProvider            = "LDAPIdentityProvider"
 	kindOIDCIdentityProvider            = "OIDCIdentityProvider"
 	kindActiveDirectoryIdentityProvider = "ActiveDirectoryIdentityProvider"
+	kindGitHubIdentityProvider          = "GitHubIdentityProvider"
 
 	celTransformerMaxExpressionRuntime = 5 * time.Second
 )
@@ -88,9 +89,9 @@ type federationDomainWatcherController struct {
 	oidcIdentityProviderInformer            idpinformers.OIDCIdentityProviderInformer
 	ldapIdentityProviderInformer            idpinformers.LDAPIdentityProviderInformer
 	activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer
-
-	celTransformer *celtransformer.CELTransformer
-	allowedKinds   sets.Set[string]
+	githubIdentityProviderInformer          idpinformers.GitHubIdentityProviderInformer
+	celTransformer                          *celtransformer.CELTransformer
+	allowedKinds                            sets.Set[string]
 }
 
 // NewFederationDomainWatcherController creates a controllerlib.Controller that watches
@@ -104,9 +105,10 @@ func NewFederationDomainWatcherController(
 	oidcIdentityProviderInformer idpinformers.OIDCIdentityProviderInformer,
 	ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer,
 	activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer,
+	githubProviderInformer idpinformers.GitHubIdentityProviderInformer,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
 ) controllerlib.Controller {
-	allowedKinds := sets.New(kindActiveDirectoryIdentityProvider, kindLDAPIdentityProvider, kindOIDCIdentityProvider)
+	allowedKinds := sets.New(kindActiveDirectoryIdentityProvider, kindLDAPIdentityProvider, kindOIDCIdentityProvider, kindGitHubIdentityProvider)
 	return controllerlib.New(
 		controllerlib.Config{
 			Name: controllerName,
@@ -119,6 +121,7 @@ func NewFederationDomainWatcherController(
 				oidcIdentityProviderInformer:            oidcIdentityProviderInformer,
 				ldapIdentityProviderInformer:            ldapIdentityProviderInformer,
 				activeDirectoryIdentityProviderInformer: activeDirectoryIdentityProviderInformer,
+				githubIdentityProviderInformer:          githubProviderInformer,
 				allowedKinds:                            allowedKinds,
 			},
 		},
@@ -148,6 +151,13 @@ func NewFederationDomainWatcherController(
 			pinnipedcontroller.MatchAnythingIgnoringUpdatesFilter(pinnipedcontroller.SingletonQueue()),
 			controllerlib.InformerOption{},
 		),
+		withInformer(
+			githubProviderInformer,
+			// Since this controller only cares about IDP metadata names and UIDs (immutable fields),
+			// we only need to trigger Sync on creates and deletes.
+			pinnipedcontroller.MatchAnythingIgnoringUpdatesFilter(pinnipedcontroller.SingletonQueue()),
+			controllerlib.InformerOption{},
+		),
 	)
 }
 
@@ -264,9 +274,12 @@ func (c *federationDomainWatcherController) makeLegacyFederationDomainIssuer(
 	if err != nil {
 		return nil, nil, err
 	}
-
+	githubIdentityProviders, err := c.githubIdentityProviderInformer.Lister().List(labels.Everything())
+	if err != nil {
+		return nil, nil, err
+	}
 	// Check if that there is exactly one IDP defined in the Supervisor namespace of any IDP CRD type.
-	idpCRsCount := len(oidcIdentityProviders) + len(ldapIdentityProviders) + len(activeDirectoryIdentityProviders)
+	idpCRsCount := len(oidcIdentityProviders) + len(ldapIdentityProviders) + len(activeDirectoryIdentityProviders) + len(githubIdentityProviders)
 
 	switch {
 	case idpCRsCount == 1:
@@ -286,6 +299,10 @@ func (c *federationDomainWatcherController) makeLegacyFederationDomainIssuer(
 			defaultFederationDomainIdentityProvider.DisplayName = activeDirectoryIdentityProviders[0].Name
 			defaultFederationDomainIdentityProvider.UID = activeDirectoryIdentityProviders[0].UID
 			foundIDPName = activeDirectoryIdentityProviders[0].Name
+		case len(githubIdentityProviders) == 1:
+			defaultFederationDomainIdentityProvider.DisplayName = githubIdentityProviders[0].Name
+			defaultFederationDomainIdentityProvider.UID = githubIdentityProviders[0].UID
+			foundIDPName = githubIdentityProviders[0].Name
 		}
 		// Backwards compatibility mode always uses an empty identity transformation pipeline since no
 		// transformations are defined on the FederationDomain.
@@ -446,6 +463,8 @@ func (c *federationDomainWatcherController) findIDPsUIDByObjectRef(objectRef cor
 		foundIDP, err = c.activeDirectoryIdentityProviderInformer.Lister().ActiveDirectoryIdentityProviders(namespace).Get(objectRef.Name)
 	case kindOIDCIdentityProvider:
 		foundIDP, err = c.oidcIdentityProviderInformer.Lister().OIDCIdentityProviders(namespace).Get(objectRef.Name)
+	case kindGitHubIdentityProvider:
+		foundIDP, err = c.githubIdentityProviderInformer.Lister().GitHubIdentityProviders(namespace).Get(objectRef.Name)
 	default:
 		// 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)

internal/controller/supervisorconfig/federation_domain_watcher_test.go

@@ -42,6 +42,7 @@ func TestFederationDomainWatcherControllerInformerFilters(t *testing.T) {
 	oidcIdentityProviderInformer := pinnipedinformers.NewSharedInformerFactoryWithOptions(nil, 0).IDP().V1alpha1().OIDCIdentityProviders()
 	ldapIdentityProviderInformer := pinnipedinformers.NewSharedInformerFactoryWithOptions(nil, 0).IDP().V1alpha1().LDAPIdentityProviders()
 	adIdentityProviderInformer := pinnipedinformers.NewSharedInformerFactoryWithOptions(nil, 0).IDP().V1alpha1().ActiveDirectoryIdentityProviders()
+	githubIdentityProviderInformer := pinnipedinformers.NewSharedInformerFactoryWithOptions(nil, 0).IDP().V1alpha1().GitHubIdentityProviders()
 
 	tests := []struct {
 		name       string
@@ -82,6 +83,13 @@ func TestFederationDomainWatcherControllerInformerFilters(t *testing.T) {
 			wantAdd:    true,
 			wantUpdate: false,
 			wantDelete: true,
+		}, {
+			name:       "any GitHubIdentityProvider adds or deletes, but updates are ignored",
+			obj:        &idpv1alpha1.GitHubIdentityProvider{},
+			informer:   githubIdentityProviderInformer,
+			wantAdd:    true,
+			wantUpdate: false,
+			wantDelete: true,
 		},
 	}
 	for _, test := range tests {
@@ -100,6 +108,7 @@ func TestFederationDomainWatcherControllerInformerFilters(t *testing.T) {
 				oidcIdentityProviderInformer,
 				ldapIdentityProviderInformer,
 				adIdentityProviderInformer,
+				githubIdentityProviderInformer,
 				withInformer.WithInformer, // make it possible to observe the behavior of the Filters
 			)
 
@@ -163,6 +172,14 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 		},
 	}
 
+	gitHubIdentityProvider := &idpv1alpha1.GitHubIdentityProvider{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "some-github-idp",
+			Namespace: namespace,
+			UID:       "some-github-idp",
+		},
+	}
+
 	federationDomain1 := &configv1alpha1.FederationDomain{
 		ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
 		Spec:       configv1alpha1.FederationDomainSpec{Issuer: "https://issuer1.com"},
@@ -487,7 +504,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 			LastTransitionTime: time,
 			Reason:             "KindUnrecognized",
 			Message: fmt.Sprintf(`some kinds specified by .spec.identityProviders[].objectRef.kind are `+
-				`not recognized (should be one of "ActiveDirectoryIdentityProvider", "LDAPIdentityProvider", "OIDCIdentityProvider"): %s`, badKinds),
+				`not recognized (should be one of "ActiveDirectoryIdentityProvider", "GitHubIdentityProvider", "LDAPIdentityProvider", "OIDCIdentityProvider"): %s`, badKinds),
 		}
 	}
 
@@ -604,6 +621,30 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				),
 			},
 		},
+		{
+			name: "legacy config: when no identity provider is specified on federation domains, but exactly one GitHub identity " +
+				"provider resource exists on cluster, the controller will set a default IDP on each federation domain " +
+				"matching the only identity provider found",
+			inputObjects: []runtime.Object{
+				federationDomain1,
+				federationDomain2,
+				gitHubIdentityProvider,
+			},
+			wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{
+				federationDomainIssuerWithDefaultIDP(t, federationDomain1.Spec.Issuer, gitHubIdentityProvider.ObjectMeta),
+				federationDomainIssuerWithDefaultIDP(t, federationDomain2.Spec.Issuer, gitHubIdentityProvider.ObjectMeta),
+			},
+			wantStatusUpdates: []*configv1alpha1.FederationDomain{
+				expectedFederationDomainStatusUpdate(federationDomain1,
+					configv1alpha1.FederationDomainPhaseReady,
+					allHappyConditionsLegacyConfigurationSuccess(federationDomain1.Spec.Issuer, gitHubIdentityProvider.Name, frozenMetav1Now, 123),
+				),
+				expectedFederationDomainStatusUpdate(federationDomain2,
+					configv1alpha1.FederationDomainPhaseReady,
+					allHappyConditionsLegacyConfigurationSuccess(federationDomain2.Spec.Issuer, gitHubIdentityProvider.Name, frozenMetav1Now, 123),
+				),
+			},
+		},
 		{
 			name: "when there are two valid FederationDomains, but one is already up to date, the sync loop only updates " +
 				"the out-of-date FederationDomain",
@@ -948,6 +989,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				oidcIdentityProvider,
 				ldapIdentityProvider,
 				adIdentityProvider,
+				gitHubIdentityProvider,
 			},
 			wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{},
 			wantStatusUpdates: []*configv1alpha1.FederationDomain{
@@ -956,7 +998,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 					conditionstestutil.Replace(
 						allHappyConditionsLegacyConfigurationSuccess(federationDomain1.Spec.Issuer, "", frozenMetav1Now, 123),
 						[]metav1.Condition{
-							sadIdentityProvidersFoundConditionIdentityProviderNotSpecified(3, frozenMetav1Now, 123),
+							sadIdentityProvidersFoundConditionIdentityProviderNotSpecified(4, frozenMetav1Now, 123),
 							sadReadyCondition(frozenMetav1Now, 123),
 						}),
 				),
@@ -994,6 +1036,14 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 									Name:     "cant-find-me-still-name",
 								},
 							},
+							{
+								DisplayName: "cant-find-me-again",
+								ObjectRef: corev1.TypedLocalObjectReference{
+									APIGroup: ptr.To(apiGroupSupervisor),
+									Kind:     "GitHubIdentityProvider",
+									Name:     "cant-find-me-again-name",
+								},
+							},
 						},
 					},
 				},
@@ -1013,7 +1063,9 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 
 								 cannot find resource specified by .spec.identityProviders[1].objectRef (with name "cant-find-me-either-name")
 
-								 cannot find resource specified by .spec.identityProviders[2].objectRef (with name "cant-find-me-still-name")`,
+								 cannot find resource specified by .spec.identityProviders[2].objectRef (with name "cant-find-me-still-name")
+
+								 cannot find resource specified by .spec.identityProviders[3].objectRef (with name "cant-find-me-again-name")`,
 							), frozenMetav1Now, 123),
 							sadReadyCondition(frozenMetav1Now, 123),
 						}),
@@ -1026,6 +1078,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				oidcIdentityProvider,
 				ldapIdentityProvider,
 				adIdentityProvider,
+				gitHubIdentityProvider,
 				&configv1alpha1.FederationDomain{
 					ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
 					Spec: configv1alpha1.FederationDomainSpec{
@@ -1055,6 +1108,14 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 									Name:     adIdentityProvider.Name,
 								},
 							},
+							{
+								DisplayName: "can-find-me-four",
+								ObjectRef: corev1.TypedLocalObjectReference{
+									APIGroup: ptr.To(apiGroupSupervisor),
+									Kind:     "GitHubIdentityProvider",
+									Name:     gitHubIdentityProvider.Name,
+								},
+							},
 						},
 					},
 				},
@@ -1077,6 +1138,11 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 							UID:         adIdentityProvider.UID,
 							Transforms:  idtransform.NewTransformationPipeline(),
 						},
+						{
+							DisplayName: "can-find-me-four",
+							UID:         gitHubIdentityProvider.UID,
+							Transforms:  idtransform.NewTransformationPipeline(),
+						},
 					}),
 			},
 			wantStatusUpdates: []*configv1alpha1.FederationDomain{
@@ -1095,6 +1161,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				oidcIdentityProvider,
 				ldapIdentityProvider,
 				adIdentityProvider,
+				gitHubIdentityProvider,
 				&configv1alpha1.FederationDomain{
 					ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
 					Spec: configv1alpha1.FederationDomainSpec{
@@ -1148,6 +1215,14 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 									Name:     adIdentityProvider.Name,
 								},
 							},
+							{
+								DisplayName: "duplicate2",
+								ObjectRef: corev1.TypedLocalObjectReference{
+									APIGroup: ptr.To(apiGroupSupervisor),
+									Kind:     "GitHubIdentityProvider",
+									Name:     gitHubIdentityProvider.Name,
+								},
+							},
 						},
 					},
 				},
@@ -1174,6 +1249,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				oidcIdentityProvider,
 				ldapIdentityProvider,
 				adIdentityProvider,
+				gitHubIdentityProvider,
 				&configv1alpha1.FederationDomain{
 					ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
 					Spec: configv1alpha1.FederationDomainSpec{
@@ -1211,6 +1287,14 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 									Name:     adIdentityProvider.Name,
 								},
 							},
+							{
+								DisplayName: "name5",
+								ObjectRef: corev1.TypedLocalObjectReference{
+									APIGroup: ptr.To(apiGroupSupervisor), // correct
+									Kind:     "GitHubIdentityProvider",
+									Name:     gitHubIdentityProvider.Name,
+								},
+							},
 						},
 					},
 				},
@@ -1244,6 +1328,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				oidcIdentityProvider,
 				ldapIdentityProvider,
 				adIdentityProvider,
+				gitHubIdentityProvider,
 				&configv1alpha1.FederationDomain{
 					ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123},
 					Spec: configv1alpha1.FederationDomainSpec{
@@ -2024,6 +2109,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) {
 				pinnipedInformers.IDP().V1alpha1().OIDCIdentityProviders(),
 				pinnipedInformers.IDP().V1alpha1().LDAPIdentityProviders(),
 				pinnipedInformers.IDP().V1alpha1().ActiveDirectoryIdentityProviders(),
+				pinnipedInformers.IDP().V1alpha1().GitHubIdentityProviders(),
 				controllerlib.WithInformer,
 			)