Merge pull request #1903 from vmware-tanzu/ben/github/identity-provider/controller

WIP: Add GitHub upstream observer controller

internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go

@@ -0,0 +1,167 @@
+// 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"
+
+	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"
+	errorsutil "k8s.io/apimachinery/pkg/util/errors"
+	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/plog"
+	"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"
+
+	//
+	// 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                            plog.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 plog.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)
+	}
+
+	var errs []error
+
+	requeue := false
+	validatedUpstreams := make([]upstreamprovider.UpstreamGithubIdentityProviderI, 0, len(actualUpstreams))
+	for _, upstream := range actualUpstreams {
+		valid, err := c.validateUpstream(ctx, upstream)
+		if valid == nil {
+			requeue = true
+			errs = append(errs, err)
+		} else {
+			validatedUpstreams = append(validatedUpstreams, upstreamprovider.UpstreamGithubIdentityProviderI(valid))
+		}
+	}
+	c.cache.SetGitHubIdentityProviders(validatedUpstreams)
+	if requeue {
+		return controllerlib.ErrSyntheticRequeue
+	}
+
+	// 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, error) {
+	result := upstreamgithub.ProviderConfig{
+		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{
+		// 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(),
+	}
+
+	err := c.updateStatus(ctx.Context, upstream, conditions)
+	return &result, err
+}
+
+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()
+
+	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 nil
+	}
+
+	_, err := c.client.
+		IDPV1alpha1().
+		GitHubIdentityProviders(upstream.Namespace).
+		UpdateStatus(ctx, updated, metav1.UpdateOptions{})
+	return err
+}

internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go

@@ -0,0 +1,128 @@
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package githubupstreamwatcher
+
+import (
+	"bytes"
+	"context"
+	"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/githubtestutil"
+	"go.pinniped.dev/internal/upstreamgithub"
+)
+
+func TestController(t *testing.T) {
+	t.Parallel()
+
+	testNamespace := "foo"
+	testName := "bar"
+
+	tests := []struct {
+		name                    string
+		githubIdentityProviders []runtime.Object
+		inputSecrets            []runtime.Object
+		configClient            func(*pinnipedfake.Clientset)
+		wantErr                 string
+		wantLogs                []string
+		wantResultingCache      []*githubtestutil.TestUpstreamGithubIdentityProvider
+		wantResultingUpstreams  []v1alpha1.GitHubIdentityProvider
+	}{
+		{
+			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",
+			}},
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			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)
+
+			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(
+				cache,
+				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)
+			}
+
+			actualIDPList := cache.GetGitHubIdentityProviders()
+			require.Equal(t, len(tt.wantResultingCache), len(actualIDPList))
+		})
+	}
+}

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
 }

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
+}

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,16 @@ func prepareControllers(
 				controllerlib.WithInformer,
 			),
 			singletonWorker).
+		WithController(
+			githubupstreamwatcher.New(
+				dynamicUpstreamIDPProvider,
+				pinnipedClient,
+				pinnipedInformers.IDP().V1alpha1().GitHubIdentityProviders(),
+				secretInformer,
+				plog.New(),
+				controllerlib.WithInformer,
+			),
+			singletonWorker).
 		WithController(
 			apicerts.NewCertsManagerController(
 				podInfo.Namespace,

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
+}

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
+}

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