From a11e1527f09a157955258471a68deb2f995633cb Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Tue, 26 Mar 2024 12:51:33 -0400 Subject: [PATCH 1/3] Add github-upstream-observer Controller --- .../github_upstream_watcher.go | 154 ++++++++++++++++++ .../github_upstream_watcher_test.go | 4 + .../dynamic_upstream_idp_provider.go | 18 +- .../upstreamprovider/upsteam_provider.go | 4 + internal/supervisor/server/server.go | 13 ++ internal/upstreamgithub/upstreamgithub.go | 29 ++++ .../upstreamgithub/upstreamgithub_test.go | 4 + 7 files changed, 225 insertions(+), 1 deletion(-) create mode 100644 internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go create mode 100644 internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go create mode 100644 internal/upstreamgithub/upstreamgithub.go create mode 100644 internal/upstreamgithub/upstreamgithub_test.go diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go new file mode 100644 index 000000000..7a333c4a3 --- /dev/null +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go @@ -0,0 +1,154 @@ +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package githubupstreamwatcher implements a controller which watches GitHubIdentityProviders. +package githubupstreamwatcher + +import ( + "context" + "fmt" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + corev1informers "k8s.io/client-go/informers/core/v1" + + "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" + supervisorclientset "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned" + idpinformers "go.pinniped.dev/generated/latest/client/supervisor/informers/externalversions/idp/v1alpha1" + pinnipedcontroller "go.pinniped.dev/internal/controller" + "go.pinniped.dev/internal/controller/conditionsutil" + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/federationdomain/upstreamprovider" + "go.pinniped.dev/internal/upstreamgithub" +) + +const ( + // Setup for the name of our controller in logs. + controllerName = "github-upstream-observer" + + // Constants related to the client credentials Secret. + gitHubClientSecretType corev1.SecretType = "secrets.pinniped.dev/github-client" + + // // fixes lint to split from above group where const has explicit type + // clientIDDataKey = "clientID" + // clientSecretDataKey = "clientSecret" + // + // // Constants related to conditions. + // typeClientCredentialsValid = "ClientCredentialsValid" //nolint:gosec // this is not a credential. +) + +// UpstreamGitHubIdentityProviderICache is a thread safe cache that holds a list of validated upstream GitHub IDP configurations. +type UpstreamGitHubIdentityProviderICache interface { + SetGitHubIdentityProviders([]upstreamprovider.UpstreamGithubIdentityProviderI) +} + +type gitHubWatcherController struct { + cache UpstreamGitHubIdentityProviderICache + log logr.Logger + client supervisorclientset.Interface + gitHubIdentityProviderInformer idpinformers.GitHubIdentityProviderInformer + secretInformer corev1informers.SecretInformer +} + +// New instantiates a new controllerlib.Controller which will populate the provided UpstreamGitHubIdentityProviderICache. +func New( + idpCache UpstreamGitHubIdentityProviderICache, + client supervisorclientset.Interface, + gitHubIdentityProviderInformer idpinformers.GitHubIdentityProviderInformer, + secretInformer corev1informers.SecretInformer, + log logr.Logger, + withInformer pinnipedcontroller.WithInformerOptionFunc, +) controllerlib.Controller { + c := gitHubWatcherController{ + cache: idpCache, + client: client, + log: log.WithName(controllerName), + gitHubIdentityProviderInformer: gitHubIdentityProviderInformer, + secretInformer: secretInformer, + } + + return controllerlib.New( + controllerlib.Config{Name: controllerName, Syncer: &c}, + withInformer( + gitHubIdentityProviderInformer, + pinnipedcontroller.MatchAnythingFilter(pinnipedcontroller.SingletonQueue()), + controllerlib.InformerOption{}, + ), + withInformer( + secretInformer, + pinnipedcontroller.MatchAnySecretOfTypeFilter(gitHubClientSecretType, pinnipedcontroller.SingletonQueue()), + controllerlib.InformerOption{}, + ), + ) +} + +// Sync implements controllerlib.Syncer. +func (c *gitHubWatcherController) Sync(ctx controllerlib.Context) error { + actualUpstreams, err := c.gitHubIdentityProviderInformer.Lister().List(labels.Everything()) + if err != nil { + return fmt.Errorf("failed to list GitHubIdentityProviders: %w", err) + } + + requeue := false + validatedUpstreams := make([]upstreamprovider.UpstreamGithubIdentityProviderI, 0, len(actualUpstreams)) + for _, upstream := range actualUpstreams { + valid := c.validateUpstream(ctx, upstream) + if valid == nil { + requeue = true + } else { + validatedUpstreams = append(validatedUpstreams, upstreamprovider.UpstreamGithubIdentityProviderI(valid)) + } + } + c.cache.SetGitHubIdentityProviders(validatedUpstreams) + if requeue { + return controllerlib.ErrSyntheticRequeue + } + return nil +} + +func (c *gitHubWatcherController) validateUpstream(ctx controllerlib.Context, upstream *v1alpha1.GitHubIdentityProvider) *upstreamgithub.ProviderConfig { + result := upstreamgithub.ProviderConfig{ + Name: upstream.Name, + } + + conditions := []*metav1.Condition{ + // TODO: once we firm up the proposal doc & merge, then firm up the CRD & merge, we can + // fill out these validations. + // c.validateHost(), + // c.validateTLS(), + // c.validateAllowedOrganizations(), + // c.validateOrganizationLoginPolicy(), + // c.validateClient(), + } + + c.updateStatus(ctx.Context, upstream, conditions) + + return &result +} + +func (c *gitHubWatcherController) updateStatus(ctx context.Context, upstream *v1alpha1.GitHubIdentityProvider, conditions []*metav1.Condition) { + log := c.log.WithValues("namespace", upstream.Namespace, "name", upstream.Name) + updated := upstream.DeepCopy() + + hadErrorCondition := conditionsutil.MergeIDPConditions(conditions, upstream.Generation, &updated.Status.Conditions, log) + + updated.Status.Phase = v1alpha1.GitHubPhaseReady + if hadErrorCondition { + updated.Status.Phase = v1alpha1.GitHubPhaseError + } + + if equality.Semantic.DeepEqual(upstream, updated) { + return + } + + _, err := c.client. + IDPV1alpha1(). + GitHubIdentityProviders(upstream.Namespace). + UpdateStatus(ctx, updated, metav1.UpdateOptions{}) + if err != nil { + log.Error(err, "failed to update status") + } +} diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go new file mode 100644 index 000000000..a95ee4abb --- /dev/null +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go @@ -0,0 +1,4 @@ +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package githubupstreamwatcher diff --git a/internal/federationdomain/dynamicupstreamprovider/dynamic_upstream_idp_provider.go b/internal/federationdomain/dynamicupstreamprovider/dynamic_upstream_idp_provider.go index bb92e6105..ea0cd50e8 100644 --- a/internal/federationdomain/dynamicupstreamprovider/dynamic_upstream_idp_provider.go +++ b/internal/federationdomain/dynamicupstreamprovider/dynamic_upstream_idp_provider.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package dynamicupstreamprovider @@ -17,12 +17,15 @@ type DynamicUpstreamIDPProvider interface { GetLDAPIdentityProviders() []upstreamprovider.UpstreamLDAPIdentityProviderI SetActiveDirectoryIdentityProviders(adIDPs []upstreamprovider.UpstreamLDAPIdentityProviderI) GetActiveDirectoryIdentityProviders() []upstreamprovider.UpstreamLDAPIdentityProviderI + SetGitHubIdentityProviders(gitHubIDPs []upstreamprovider.UpstreamGithubIdentityProviderI) + GetGitHubIdentityProviders() []upstreamprovider.UpstreamGithubIdentityProviderI } type dynamicUpstreamIDPProvider struct { oidcUpstreams []upstreamprovider.UpstreamOIDCIdentityProviderI ldapUpstreams []upstreamprovider.UpstreamLDAPIdentityProviderI activeDirectoryUpstreams []upstreamprovider.UpstreamLDAPIdentityProviderI + gitHubUpstreams []upstreamprovider.UpstreamGithubIdentityProviderI mutex sync.RWMutex } @@ -31,6 +34,7 @@ func NewDynamicUpstreamIDPProvider() DynamicUpstreamIDPProvider { oidcUpstreams: []upstreamprovider.UpstreamOIDCIdentityProviderI{}, ldapUpstreams: []upstreamprovider.UpstreamLDAPIdentityProviderI{}, activeDirectoryUpstreams: []upstreamprovider.UpstreamLDAPIdentityProviderI{}, + gitHubUpstreams: []upstreamprovider.UpstreamGithubIdentityProviderI{}, } } @@ -70,6 +74,18 @@ func (p *dynamicUpstreamIDPProvider) GetActiveDirectoryIdentityProviders() []ups return p.activeDirectoryUpstreams } +func (p *dynamicUpstreamIDPProvider) SetGitHubIdentityProviders(gitHubIDPs []upstreamprovider.UpstreamGithubIdentityProviderI) { + p.mutex.Lock() // acquire a write lock + defer p.mutex.Unlock() + p.gitHubUpstreams = gitHubIDPs +} + +func (p *dynamicUpstreamIDPProvider) GetGitHubIdentityProviders() []upstreamprovider.UpstreamGithubIdentityProviderI { + p.mutex.RLock() // acquire a read lock + defer p.mutex.RUnlock() + return p.gitHubUpstreams +} + type RetryableRevocationError struct { wrapped error } diff --git a/internal/federationdomain/upstreamprovider/upsteam_provider.go b/internal/federationdomain/upstreamprovider/upsteam_provider.go index 40162c52a..5a7c59fa4 100644 --- a/internal/federationdomain/upstreamprovider/upsteam_provider.go +++ b/internal/federationdomain/upstreamprovider/upsteam_provider.go @@ -125,3 +125,7 @@ type UpstreamLDAPIdentityProviderI interface { // PerformRefresh performs a refresh against the upstream LDAP identity provider PerformRefresh(ctx context.Context, storedRefreshAttributes RefreshAttributes, idpDisplayName string) (groups []string, err error) } + +type UpstreamGithubIdentityProviderI interface { + UpstreamIdentityProviderI +} diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 0b17f0d6e..175852f2a 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -50,6 +50,7 @@ import ( "go.pinniped.dev/internal/controller/supervisorconfig" "go.pinniped.dev/internal/controller/supervisorconfig/activedirectoryupstreamwatcher" "go.pinniped.dev/internal/controller/supervisorconfig/generator" + "go.pinniped.dev/internal/controller/supervisorconfig/githubupstreamwatcher" "go.pinniped.dev/internal/controller/supervisorconfig/ldapupstreamwatcher" "go.pinniped.dev/internal/controller/supervisorconfig/oidcclientwatcher" "go.pinniped.dev/internal/controller/supervisorconfig/oidcupstreamwatcher" @@ -322,6 +323,18 @@ func prepareControllers( controllerlib.WithInformer, ), singletonWorker). + WithController( + githubupstreamwatcher.New( + dynamicUpstreamIDPProvider, + pinnipedClient, + pinnipedInformers.IDP().V1alpha1().GitHubIdentityProviders(), + secretInformer, + // TODO: need to swap this out for plog.New() + plog.Logr(), //nolint:staticcheck // old controller with lots of log statements + // plog.New(), + controllerlib.WithInformer, + ), + singletonWorker). WithController( apicerts.NewCertsManagerController( podInfo.Namespace, diff --git a/internal/upstreamgithub/upstreamgithub.go b/internal/upstreamgithub/upstreamgithub.go new file mode 100644 index 000000000..4bcddf568 --- /dev/null +++ b/internal/upstreamgithub/upstreamgithub.go @@ -0,0 +1,29 @@ +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package upstreamoidc implements an abstraction of upstream GitHub provider interactions. +package upstreamgithub + +import ( + "k8s.io/apimachinery/pkg/types" + + "go.pinniped.dev/internal/federationdomain/upstreamprovider" +) + +// ProviderConfig holds the active configuration of an upstream GitHub provider. +type ProviderConfig struct { + Name string + ResourceUID types.UID + UsernameClaim string + GroupsClaim string +} + +var _ upstreamprovider.UpstreamGithubIdentityProviderI = (*ProviderConfig)(nil) + +func (p *ProviderConfig) GetResourceUID() types.UID { + return p.ResourceUID +} + +func (p *ProviderConfig) GetName() string { + return p.Name +} diff --git a/internal/upstreamgithub/upstreamgithub_test.go b/internal/upstreamgithub/upstreamgithub_test.go new file mode 100644 index 000000000..37e13266f --- /dev/null +++ b/internal/upstreamgithub/upstreamgithub_test.go @@ -0,0 +1,4 @@ +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package upstreamgithub From 5c490e999d56311f68f5aa4363292774d984c6d0 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Tue, 2 Apr 2024 12:38:06 -0400 Subject: [PATCH 2/3] Stub in unit tests for github_upstream_watcher --- .../github_upstream_watcher.go | 34 ++++---- .../github_upstream_watcher_test.go | 80 +++++++++++++++++++ internal/supervisor/server/server.go | 4 +- 3 files changed, 100 insertions(+), 18 deletions(-) diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go index 7a333c4a3..537c34516 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go @@ -8,7 +8,6 @@ import ( "context" "fmt" - "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -22,6 +21,7 @@ import ( "go.pinniped.dev/internal/controller/conditionsutil" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/federationdomain/upstreamprovider" + "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/upstreamgithub" ) @@ -32,7 +32,7 @@ const ( // Constants related to the client credentials Secret. gitHubClientSecretType corev1.SecretType = "secrets.pinniped.dev/github-client" - // // fixes lint to split from above group where const has explicit type + // // clientIDDataKey = "clientID" // clientSecretDataKey = "clientSecret" // @@ -47,7 +47,7 @@ type UpstreamGitHubIdentityProviderICache interface { type gitHubWatcherController struct { cache UpstreamGitHubIdentityProviderICache - log logr.Logger + log plog.Logger client supervisorclientset.Interface gitHubIdentityProviderInformer idpinformers.GitHubIdentityProviderInformer secretInformer corev1informers.SecretInformer @@ -59,7 +59,7 @@ func New( client supervisorclientset.Interface, gitHubIdentityProviderInformer idpinformers.GitHubIdentityProviderInformer, secretInformer corev1informers.SecretInformer, - log logr.Logger, + log plog.Logger, withInformer pinnipedcontroller.WithInformerOptionFunc, ) controllerlib.Controller { c := gitHubWatcherController{ @@ -114,13 +114,16 @@ func (c *gitHubWatcherController) validateUpstream(ctx controllerlib.Context, up Name: upstream.Name, } + // TODO: once we firm up the proposal doc & merge, then firm up the CRD & merge, we can fill out these validations. + // The critical pattern to maintain is that every run of the sync loop will populate the exact number of the exact + // same set of conditions. Conditions depending on other conditions should get Status: metav1.ConditionUnknown, or + // Status: metav1.ConditionFalse, never be omitted. conditions := []*metav1.Condition{ - // TODO: once we firm up the proposal doc & merge, then firm up the CRD & merge, we can - // fill out these validations. - // c.validateHost(), - // c.validateTLS(), - // c.validateAllowedOrganizations(), - // c.validateOrganizationLoginPolicy(), + // we may opt to split this up into smaller validation functions. + // Each function should be responsible for validating one logical unit and setting one condition. + // c.validateGitHubAPI(), + // c.validateClaims(), + // c.validateAllowedAuthentication(), // c.validateClient(), } @@ -129,7 +132,10 @@ func (c *gitHubWatcherController) validateUpstream(ctx controllerlib.Context, up return &result } -func (c *gitHubWatcherController) updateStatus(ctx context.Context, upstream *v1alpha1.GitHubIdentityProvider, conditions []*metav1.Condition) { +func (c *gitHubWatcherController) updateStatus( + ctx context.Context, + upstream *v1alpha1.GitHubIdentityProvider, + conditions []*metav1.Condition) error { log := c.log.WithValues("namespace", upstream.Namespace, "name", upstream.Name) updated := upstream.DeepCopy() @@ -141,14 +147,12 @@ func (c *gitHubWatcherController) updateStatus(ctx context.Context, upstream *v1 } if equality.Semantic.DeepEqual(upstream, updated) { - return + return nil } _, err := c.client. IDPV1alpha1(). GitHubIdentityProviders(upstream.Namespace). UpdateStatus(ctx, updated, metav1.UpdateOptions{}) - if err != nil { - log.Error(err, "failed to update status") - } + return err } diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go index a95ee4abb..29497deb8 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go @@ -2,3 +2,83 @@ // SPDX-License-Identifier: Apache-2.0 package githubupstreamwatcher + +import ( + "bytes" + "context" + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime" + k8sinformers "k8s.io/client-go/informers" + kubernetesfake "k8s.io/client-go/kubernetes/fake" + + "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" + pinnipedfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" + pinnipedinformers "go.pinniped.dev/generated/latest/client/supervisor/informers/externalversions" + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/federationdomain/dynamicupstreamprovider" + "go.pinniped.dev/internal/plog" + "go.pinniped.dev/internal/testutil/oidctestutil" +) + +func TestController(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + githubIdentityProviders []runtime.Object + inputSecrets []runtime.Object + configClient func(*pinnipedfake.Clientset) + wantErr string + wantLogs []string + wantResultingCache []*oidctestutil.TestUpstreamOIDCIdentityProvider + wantResultingUpstreams []v1alpha1.GitHubIdentityProvider + }{ + { + name: "The controller runs.... and should be further tested.", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + dynamicUpstreamProvider := dynamicupstreamprovider.NewDynamicUpstreamIDPProvider() + pinnipedAPIClient := pinnipedfake.NewSimpleClientset(tt.githubIdentityProviders...) + fakePinnipedClientForInformers := pinnipedfake.NewSimpleClientset(tt.githubIdentityProviders...) + pinnipedInformers := pinnipedinformers.NewSharedInformerFactory(fakePinnipedClientForInformers, 0) + + fakeKubeClient := kubernetesfake.NewSimpleClientset(tt.inputSecrets...) + kubeInformers := k8sinformers.NewSharedInformerFactoryWithOptions(fakeKubeClient, 0) + + var log bytes.Buffer + logger := plog.TestLogger(t, &log) + + controller := New( + dynamicUpstreamProvider, + pinnipedAPIClient, + pinnipedInformers.IDP().V1alpha1().GitHubIdentityProviders(), + kubeInformers.Core().V1().Secrets(), + logger, + controllerlib.WithInformer, + ) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + pinnipedInformers.Start(ctx.Done()) + kubeInformers.Start(ctx.Done()) + controllerlib.TestRunSynchronously(t, controller) + + syncCtx := controllerlib.Context{Context: ctx, Key: controllerlib.Key{}} + + if err := controllerlib.TestSync(t, controller, syncCtx); tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 175852f2a..724aba760 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -329,9 +329,7 @@ func prepareControllers( pinnipedClient, pinnipedInformers.IDP().V1alpha1().GitHubIdentityProviders(), secretInformer, - // TODO: need to swap this out for plog.New() - plog.Logr(), //nolint:staticcheck // old controller with lots of log statements - // plog.New(), + plog.New(), controllerlib.WithInformer, ), singletonWorker). From e2db152c6c60486d7dfd5b8b55674e57744406bb Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Tue, 2 Apr 2024 13:16:04 -0400 Subject: [PATCH 3/3] Stub in TestUpstreamGithubIdentityProvider for unit tests --- .../github_upstream_watcher.go | 21 +++++--- .../github_upstream_watcher_test.go | 54 +++++++++++++++++-- .../githubtestutil/testgithubprovider.go | 8 +++ 3 files changed, 72 insertions(+), 11 deletions(-) create mode 100644 internal/testutil/githubtestutil/testgithubprovider.go diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go index 537c34516..d1cd61bf1 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go @@ -12,6 +12,7 @@ import ( "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + errorsutil "k8s.io/apimachinery/pkg/util/errors" corev1informers "k8s.io/client-go/informers/core/v1" "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" @@ -92,12 +93,15 @@ func (c *gitHubWatcherController) Sync(ctx controllerlib.Context) error { return fmt.Errorf("failed to list GitHubIdentityProviders: %w", err) } + var errs []error + requeue := false validatedUpstreams := make([]upstreamprovider.UpstreamGithubIdentityProviderI, 0, len(actualUpstreams)) for _, upstream := range actualUpstreams { - valid := c.validateUpstream(ctx, upstream) + valid, err := c.validateUpstream(ctx, upstream) if valid == nil { requeue = true + errs = append(errs, err) } else { validatedUpstreams = append(validatedUpstreams, upstreamprovider.UpstreamGithubIdentityProviderI(valid)) } @@ -106,10 +110,16 @@ func (c *gitHubWatcherController) Sync(ctx controllerlib.Context) error { if requeue { return controllerlib.ErrSyntheticRequeue } - return nil + + // Sync loop errors: + // - Should not be configuration errors. Config errors a user must correct belong on the .Status + // object. The controller simply must wait for a user to correct before running again. + // - Other errors, such as networking errors, etc. are the types of errors that should return here + // and signal the controller to retry the sync loop. These may be corrected by machines. + return errorsutil.NewAggregate(errs) } -func (c *gitHubWatcherController) validateUpstream(ctx controllerlib.Context, upstream *v1alpha1.GitHubIdentityProvider) *upstreamgithub.ProviderConfig { +func (c *gitHubWatcherController) validateUpstream(ctx controllerlib.Context, upstream *v1alpha1.GitHubIdentityProvider) (*upstreamgithub.ProviderConfig, error) { result := upstreamgithub.ProviderConfig{ Name: upstream.Name, } @@ -127,9 +137,8 @@ func (c *gitHubWatcherController) validateUpstream(ctx controllerlib.Context, up // c.validateClient(), } - c.updateStatus(ctx.Context, upstream, conditions) - - return &result + err := c.updateStatus(ctx.Context, upstream, conditions) + return &result, err } func (c *gitHubWatcherController) updateStatus( diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go index 29497deb8..c9cc9e4cd 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go @@ -9,22 +9,29 @@ import ( "testing" "github.com/stretchr/testify/require" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" k8sinformers "k8s.io/client-go/informers" kubernetesfake "k8s.io/client-go/kubernetes/fake" + "k8s.io/utils/ptr" "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" pinnipedfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake" pinnipedinformers "go.pinniped.dev/generated/latest/client/supervisor/informers/externalversions" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/federationdomain/dynamicupstreamprovider" + "go.pinniped.dev/internal/federationdomain/upstreamprovider" "go.pinniped.dev/internal/plog" - "go.pinniped.dev/internal/testutil/oidctestutil" + "go.pinniped.dev/internal/testutil/githubtestutil" + "go.pinniped.dev/internal/upstreamgithub" ) func TestController(t *testing.T) { t.Parallel() + testNamespace := "foo" + testName := "bar" + tests := []struct { name string githubIdentityProviders []runtime.Object @@ -32,11 +39,41 @@ func TestController(t *testing.T) { configClient func(*pinnipedfake.Clientset) wantErr string wantLogs []string - wantResultingCache []*oidctestutil.TestUpstreamOIDCIdentityProvider + wantResultingCache []*githubtestutil.TestUpstreamGithubIdentityProvider wantResultingUpstreams []v1alpha1.GitHubIdentityProvider }{ { - name: "The controller runs.... and should be further tested.", + name: "no upstreams", + }, { + name: "found github idp is cached", + inputSecrets: []runtime.Object{}, + githubIdentityProviders: []runtime.Object{&v1alpha1.GitHubIdentityProvider{ + ObjectMeta: v1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Spec: v1alpha1.GitHubIdentityProviderSpec{ + GitHubAPI: v1alpha1.GitHubAPIConfig{ + Host: ptr.To("127.0.0.1"), + TLS: &v1alpha1.TLSSpec{ + CertificateAuthorityData: "irrelevant", + }, + }, + Claims: v1alpha1.GitHubClaims{ + Username: ptr.To(v1alpha1.GitHubUsernameID), + Groups: ptr.To(v1alpha1.GitHubUseTeamNameForGroupName), + }, + AllowAuthentication: v1alpha1.GitHubAllowAuthenticationSpec{ + Organizations: v1alpha1.GitHubOrganizationsSpec{ + Policy: ptr.To(v1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers), + Allowed: []string{"foo", "bar"}, + }, + }, + Client: v1alpha1.GitHubClientSpec{ + SecretName: "some-secret", + }, + }, + }}, + wantResultingCache: []*githubtestutil.TestUpstreamGithubIdentityProvider{{ + Name: "test-github-idp-to-flesh-out", + }}, }, } @@ -45,7 +82,6 @@ func TestController(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - dynamicUpstreamProvider := dynamicupstreamprovider.NewDynamicUpstreamIDPProvider() pinnipedAPIClient := pinnipedfake.NewSimpleClientset(tt.githubIdentityProviders...) fakePinnipedClientForInformers := pinnipedfake.NewSimpleClientset(tt.githubIdentityProviders...) pinnipedInformers := pinnipedinformers.NewSharedInformerFactory(fakePinnipedClientForInformers, 0) @@ -53,11 +89,16 @@ func TestController(t *testing.T) { fakeKubeClient := kubernetesfake.NewSimpleClientset(tt.inputSecrets...) kubeInformers := k8sinformers.NewSharedInformerFactoryWithOptions(fakeKubeClient, 0) + cache := dynamicupstreamprovider.NewDynamicUpstreamIDPProvider() + cache.SetGitHubIdentityProviders([]upstreamprovider.UpstreamGithubIdentityProviderI{ + &upstreamgithub.ProviderConfig{Name: "initial-entry-to-remove"}, + }) + var log bytes.Buffer logger := plog.TestLogger(t, &log) controller := New( - dynamicUpstreamProvider, + cache, pinnipedAPIClient, pinnipedInformers.IDP().V1alpha1().GitHubIdentityProviders(), kubeInformers.Core().V1().Secrets(), @@ -79,6 +120,9 @@ func TestController(t *testing.T) { } else { require.NoError(t, err) } + + actualIDPList := cache.GetGitHubIdentityProviders() + require.Equal(t, len(tt.wantResultingCache), len(actualIDPList)) }) } } diff --git a/internal/testutil/githubtestutil/testgithubprovider.go b/internal/testutil/githubtestutil/testgithubprovider.go new file mode 100644 index 000000000..532b9b8a0 --- /dev/null +++ b/internal/testutil/githubtestutil/testgithubprovider.go @@ -0,0 +1,8 @@ +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package githubtestutil + +type TestUpstreamGithubIdentityProvider struct { + Name string +}