From e2db152c6c60486d7dfd5b8b55674e57744406bb Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Tue, 2 Apr 2024 13:16:04 -0400 Subject: [PATCH] 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 +}