From 5c490e999d56311f68f5aa4363292774d984c6d0 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Tue, 2 Apr 2024 12:38:06 -0400 Subject: [PATCH] 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).