diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index f571209ae..d40f7f85f 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/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) diff --git a/internal/controller/supervisorconfig/federation_domain_watcher_test.go b/internal/controller/supervisorconfig/federation_domain_watcher_test.go index 1f8876a6f..d4c6c800a 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher_test.go +++ b/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, ) diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 1afe994cf..d66694c07 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -177,6 +177,7 @@ func prepareControllers( pinnipedInformers.IDP().V1alpha1().OIDCIdentityProviders(), pinnipedInformers.IDP().V1alpha1().LDAPIdentityProviders(), pinnipedInformers.IDP().V1alpha1().ActiveDirectoryIdentityProviders(), + pinnipedInformers.IDP().V1alpha1().GitHubIdentityProviders(), controllerlib.WithInformer, ), singletonWorker,