Add "upstream-watcher" controller to supervisor.

Signed-off-by: Matt Moyer <moyerm@vmware.com>

Dockerfile

@@ -25,6 +25,7 @@ RUN mkdir out \
 
 # Use a runtime image based on Debian slim
 FROM debian:10.6-slim
+RUN apt-get update && apt-get install -y ca-certificates && rm -rf /var/lib/apt/lists/*
 
 # Copy the binaries from the build-env stage
 COPY --from=build-env /work/out/pinniped-concierge /usr/local/bin/pinniped-concierge

cmd/pinniped-supervisor/main.go

@@ -22,11 +22,13 @@ import (
 	restclient "k8s.io/client-go/rest"
 	"k8s.io/component-base/logs"
 	"k8s.io/klog/v2"
+	"k8s.io/klog/v2/klogr"
 
 	pinnipedclientset "go.pinniped.dev/generated/1.19/client/supervisor/clientset/versioned"
 	pinnipedinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions"
 	"go.pinniped.dev/internal/config/supervisor"
 	"go.pinniped.dev/internal/controller/supervisorconfig"
+	"go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatcher"
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/downward"
 	"go.pinniped.dev/internal/oidc/jwks"
@@ -73,6 +75,7 @@ func startControllers(
 	issuerManager *manager.Manager,
 	dynamicJWKSProvider jwks.DynamicJWKSProvider,
 	dynamicTLSCertProvider provider.DynamicTLSCertProvider,
+	dynamicUpstreamIDPProvider provider.DynamicUpstreamIDPProvider,
 	kubeClient kubernetes.Interface,
 	pinnipedClient pinnipedclientset.Interface,
 	kubeInformers kubeinformers.SharedInformerFactory,
@@ -120,7 +123,15 @@ func startControllers(
 				controllerlib.WithInformer,
 			),
 			singletonWorker,
-		)
+		).
+		WithController(
+			upstreamwatcher.New(
+				dynamicUpstreamIDPProvider,
+				pinnipedClient,
+				pinnipedInformers.IDP().V1alpha1().UpstreamOIDCProviders(),
+				kubeInformers.Core().V1().Secrets(),
+				klogr.New()),
+			singletonWorker)
 
 	kubeInformers.Start(ctx.Done())
 	pinnipedInformers.Start(ctx.Done())
@@ -193,6 +204,7 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error {
 		oidProvidersManager,
 		dynamicJWKSProvider,
 		dynamicTLSCertProvider,
+		dynamicUpstreamIDPProvider,
 		kubeClient,
 		pinnipedClient,
 		kubeInformers,

hack/lib/tilt/supervisor.Dockerfile

@@ -4,6 +4,8 @@
 # Use a runtime image based on Debian slim
 FROM debian:10.6-slim
 
+RUN apt-get update && apt-get install -y ca-certificates && rm -rf /var/lib/apt/lists/*
+
 # Copy the binary which was built outside the container.
 COPY build/pinniped-supervisor /usr/local/bin/pinniped-supervisor
 

internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go

@@ -0,0 +1,337 @@
+// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+// Package upstreamwatcher implements a controller that watches UpstreamOIDCProvider objects.
+package upstreamwatcher
+
+import (
+	"context"
+	"fmt"
+	"net/url"
+	"sort"
+	"strings"
+	"time"
+
+	"github.com/coreos/go-oidc"
+	"github.com/go-logr/logr"
+	"k8s.io/apimachinery/pkg/api/equality"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/util/cache"
+	corev1informers "k8s.io/client-go/informers/core/v1"
+
+	"go.pinniped.dev/generated/1.19/apis/supervisor/idp/v1alpha1"
+	pinnipedclientset "go.pinniped.dev/generated/1.19/client/supervisor/clientset/versioned"
+	idpinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions/idp/v1alpha1"
+	"go.pinniped.dev/internal/constable"
+	pinnipedcontroller "go.pinniped.dev/internal/controller"
+	"go.pinniped.dev/internal/controllerlib"
+	"go.pinniped.dev/internal/oidc/provider"
+)
+
+const (
+	// Setup for the name of our controller in logs.
+	controllerName = "upstream-observer"
+
+	// Constants related to the client credentials Secret.
+	oidcClientSecretType = "secrets.pinniped.dev/oidc-client"
+	clientIDDataKey      = "clientID"
+	clientSecretDataKey  = "clientSecret"
+
+	// Constants related to the OIDC provider discovery cache. These do not affect the cache of JWKS.
+	validatorCacheTTL = 15 * time.Minute
+
+	// Constants related to conditions.
+	typeClientCredsValid       = "ClientCredentialsValid"
+	typeOIDCDiscoverySucceeded = "OIDCDiscoverySucceeded"
+	reasonNotFound             = "SecretNotFound"
+	reasonWrongType            = "SecretWrongType"
+	reasonMissingKeys          = "SecretMissingKeys"
+	reasonSuccess              = "Success"
+	reasonUnreachable          = "Unreachable"
+	reasonInvalidResponse      = "InvalidResponse"
+
+	// Errors that are generated by our reconcile process.
+	errFailureStatus = constable.Error("UpstreamOIDCProvider has a failing condition")
+)
+
+// IDPCache is a thread safe cache that holds a list of validated upstream OIDC IDP configurations.
+type IDPCache interface {
+	SetIDPList([]provider.UpstreamOIDCIdentityProvider)
+}
+
+type controller struct {
+	cache          IDPCache
+	log            logr.Logger
+	client         pinnipedclientset.Interface
+	providers      idpinformers.UpstreamOIDCProviderInformer
+	secrets        corev1informers.SecretInformer
+	validatorCache *cache.Expiring
+}
+
+// New instantiates a new controllerlib.Controller which will populate the provided IDPCache.
+func New(
+	idpCache IDPCache,
+	client pinnipedclientset.Interface,
+	providers idpinformers.UpstreamOIDCProviderInformer,
+	secrets corev1informers.SecretInformer,
+	log logr.Logger,
+) controllerlib.Controller {
+	c := controller{
+		cache:          idpCache,
+		log:            log.WithName(controllerName),
+		client:         client,
+		providers:      providers,
+		secrets:        secrets,
+		validatorCache: cache.NewExpiring(),
+	}
+	filter := pinnipedcontroller.MatchAnythingFilter(pinnipedcontroller.SingletonQueue())
+	return controllerlib.New(
+		controllerlib.Config{Name: controllerName, Syncer: &c},
+		controllerlib.WithInformer(providers, filter, controllerlib.InformerOption{}),
+		controllerlib.WithInformer(secrets, filter, controllerlib.InformerOption{}),
+	)
+}
+
+// Sync implements controllerlib.Syncer.
+func (c *controller) Sync(ctx controllerlib.Context) error {
+	actualUpstreams, err := c.providers.Lister().List(labels.Everything())
+	if err != nil {
+		return fmt.Errorf("failed to list UpstreamOIDCProviders: %w", err)
+	}
+
+	requeue := false
+	validatedUpstreams := make([]provider.UpstreamOIDCIdentityProvider, 0, len(actualUpstreams))
+	for _, upstream := range actualUpstreams {
+		valid := c.validateUpstream(ctx, upstream)
+		if valid == nil {
+			requeue = true
+		} else {
+			validatedUpstreams = append(validatedUpstreams, *valid)
+		}
+	}
+	c.cache.SetIDPList(validatedUpstreams)
+	if requeue {
+		return controllerlib.ErrSyntheticRequeue
+	}
+	return nil
+}
+
+// validateUpstream validates the provided v1alpha1.UpstreamOIDCProvider and returns the validated configuration as a
+// provider.UpstreamOIDCIdentityProvider. As a side effect, it also updates the status of the v1alpha1.UpstreamOIDCProvider.
+func (c *controller) validateUpstream(ctx controllerlib.Context, upstream *v1alpha1.UpstreamOIDCProvider) *provider.UpstreamOIDCIdentityProvider {
+	result := provider.UpstreamOIDCIdentityProvider{
+		Name:   upstream.Name,
+		Scopes: computeScopes(upstream.Spec.AuthorizationConfig.AdditionalScopes),
+	}
+	conditions := []*v1alpha1.Condition{
+		c.validateSecret(upstream, &result),
+		c.validateIssuer(ctx.Context, upstream, &result),
+	}
+	c.updateStatus(ctx.Context, upstream, conditions)
+
+	valid := true
+	log := c.log.WithValues("namespace", upstream.Namespace, "name", upstream.Name)
+	for _, condition := range conditions {
+		if condition.Status == v1alpha1.ConditionFalse {
+			valid = false
+			log.WithValues(
+				"type", condition.Type,
+				"reason", condition.Reason,
+				"message", condition.Message,
+			).Error(errFailureStatus, "found failing condition")
+		}
+	}
+	if valid {
+		return &result
+	}
+	return nil
+}
+
+// validateSecret validates the .spec.client.secretName field and returns the appropriate ClientCredentialsValid condition.
+func (c *controller) validateSecret(upstream *v1alpha1.UpstreamOIDCProvider, result *provider.UpstreamOIDCIdentityProvider) *v1alpha1.Condition {
+	secretName := upstream.Spec.Client.SecretName
+
+	// Fetch the Secret from informer cache.
+	secret, err := c.secrets.Lister().Secrets(upstream.Namespace).Get(secretName)
+	if err != nil {
+		return &v1alpha1.Condition{
+			Type:    typeClientCredsValid,
+			Status:  v1alpha1.ConditionFalse,
+			Reason:  reasonNotFound,
+			Message: err.Error(),
+		}
+	}
+
+	// Validate the secret .type field.
+	if secret.Type != oidcClientSecretType {
+		return &v1alpha1.Condition{
+			Type:    typeClientCredsValid,
+			Status:  v1alpha1.ConditionFalse,
+			Reason:  reasonWrongType,
+			Message: fmt.Sprintf("referenced Secret %q has wrong type %q (should be %q)", secretName, secret.Type, oidcClientSecretType),
+		}
+	}
+
+	// Validate the secret .data field.
+	clientID := secret.Data[clientIDDataKey]
+	clientSecret := secret.Data[clientSecretDataKey]
+	if len(clientID) == 0 || len(clientSecret) == 0 {
+		return &v1alpha1.Condition{
+			Type:    typeClientCredsValid,
+			Status:  v1alpha1.ConditionFalse,
+			Reason:  reasonMissingKeys,
+			Message: fmt.Sprintf("referenced Secret %q is missing required keys %q", secretName, []string{clientIDDataKey, clientSecretDataKey}),
+		}
+	}
+
+	// If everything is valid, update the result and set the condition to true.
+	result.ClientID = string(clientID)
+	return &v1alpha1.Condition{
+		Type:    typeClientCredsValid,
+		Status:  v1alpha1.ConditionTrue,
+		Reason:  reasonSuccess,
+		Message: "loaded client credentials",
+	}
+}
+
+// validateIssuer validates the .spec.issuer field, performs OIDC discovery, and returns the appropriate OIDCDiscoverySucceeded condition.
+func (c *controller) validateIssuer(ctx context.Context, upstream *v1alpha1.UpstreamOIDCProvider, result *provider.UpstreamOIDCIdentityProvider) *v1alpha1.Condition {
+	// Get the provider (from cache if possible).
+	var discoveredProvider *oidc.Provider
+	if cached, ok := c.validatorCache.Get(upstream.Spec.Issuer); ok {
+		discoveredProvider = cached.(*oidc.Provider)
+	}
+
+	// If the provider does not exist in the cache, do a fresh discovery lookup and save to the cache.
+	if discoveredProvider == nil {
+		var err error
+		discoveredProvider, err = oidc.NewProvider(ctx, upstream.Spec.Issuer)
+		if err != nil {
+			err := fmt.Errorf("failed to perform OIDC discovery against %q: %w", upstream.Spec.Issuer, err)
+			return &v1alpha1.Condition{
+				Type:    typeOIDCDiscoverySucceeded,
+				Status:  v1alpha1.ConditionFalse,
+				Reason:  reasonUnreachable,
+				Message: strings.TrimSpace(err.Error()),
+			}
+		}
+
+		// Update the cache with the newly discovered value.
+		c.validatorCache.Set(upstream.Spec.Issuer, discoveredProvider, validatorCacheTTL)
+	}
+
+	// Parse out and validate the discovered authorize endpoint.
+	authURL, err := url.Parse(discoveredProvider.Endpoint().AuthURL)
+	if err != nil {
+		return &v1alpha1.Condition{
+			Type:    typeOIDCDiscoverySucceeded,
+			Status:  v1alpha1.ConditionFalse,
+			Reason:  reasonInvalidResponse,
+			Message: fmt.Sprintf("failed to parse authorization endpoint URL: %v", err),
+		}
+	}
+	if authURL.Scheme != "https" {
+		return &v1alpha1.Condition{
+			Type:    typeOIDCDiscoverySucceeded,
+			Status:  v1alpha1.ConditionFalse,
+			Reason:  reasonInvalidResponse,
+			Message: fmt.Sprintf(`authorization endpoint URL scheme must be "https", not %q`, authURL.Scheme),
+		}
+	}
+
+	// If everything is valid, update the result and set the condition to true.
+	result.AuthorizationURL = *authURL
+	return &v1alpha1.Condition{
+		Type:    typeOIDCDiscoverySucceeded,
+		Status:  v1alpha1.ConditionTrue,
+		Reason:  reasonSuccess,
+		Message: "discovered issuer configuration",
+	}
+}
+
+func (c *controller) updateStatus(ctx context.Context, upstream *v1alpha1.UpstreamOIDCProvider, conditions []*v1alpha1.Condition) {
+	log := c.log.WithValues("namespace", upstream.Namespace, "name", upstream.Name)
+	updated := upstream.DeepCopy()
+
+	updated.Status.Phase = v1alpha1.PhaseReady
+
+	for i := range conditions {
+		cond := conditions[i].DeepCopy()
+		cond.LastTransitionTime = metav1.Now()
+		cond.ObservedGeneration = upstream.Generation
+		if mergeCondition(&updated.Status.Conditions, cond) {
+			log.Info("updated condition", "type", cond.Type, "status", cond.Status, "reason", cond.Reason, "message", cond.Message)
+		}
+		if cond.Status == v1alpha1.ConditionFalse {
+			updated.Status.Phase = v1alpha1.PhaseError
+		}
+	}
+
+	sort.SliceStable(updated.Status.Conditions, func(i, j int) bool {
+		return updated.Status.Conditions[i].Type < updated.Status.Conditions[j].Type
+	})
+
+	if equality.Semantic.DeepEqual(upstream, updated) {
+		return
+	}
+
+	_, err := c.client.
+		IDPV1alpha1().
+		UpstreamOIDCProviders(upstream.Namespace).
+		UpdateStatus(ctx, updated, metav1.UpdateOptions{})
+	if err != nil {
+		log.Error(err, "failed to update status")
+	}
+}
+
+// mergeCondition merges a new v1alpha1.Condition into a slice of existing conditions. It returns true
+// if the condition has meaningfully changed.
+func mergeCondition(existing *[]v1alpha1.Condition, new *v1alpha1.Condition) bool {
+	// Find any existing condition with a matching type.
+	var old *v1alpha1.Condition
+	for i := range *existing {
+		if (*existing)[i].Type == new.Type {
+			old = &(*existing)[i]
+			continue
+		}
+	}
+
+	// If there is no existing condition of this type, append this one and we're done.
+	if old == nil {
+		*existing = append(*existing, *new)
+		return true
+	}
+
+	// Set the LastTransitionTime depending on whether the status has changed.
+	new = new.DeepCopy()
+	if old.Status == new.Status {
+		new.LastTransitionTime = old.LastTransitionTime
+	}
+
+	// If anything has actually changed, update the entry and return true.
+	if !equality.Semantic.DeepEqual(old, new) {
+		*old = *new
+		return true
+	}
+
+	// Otherwise the entry is already up to date.
+	return false
+}
+
+func computeScopes(additionalScopes []string) []string {
+	// First compute the unique set of scopes, including "openid" (de-duplicate).
+	set := make(map[string]bool, len(additionalScopes)+1)
+	set["openid"] = true
+	for _, s := range additionalScopes {
+		set[s] = true
+	}
+
+	// Then grab all the keys and sort them.
+	scopes := make([]string, 0, len(set))
+	for s := range set {
+		scopes = append(scopes, s)
+	}
+	sort.Strings(scopes)
+	return scopes
+}

internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go

@@ -0,0 +1,529 @@
+// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package upstreamwatcher
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/informers"
+	"k8s.io/client-go/kubernetes/fake"
+
+	"go.pinniped.dev/generated/1.19/apis/supervisor/idp/v1alpha1"
+	pinnipedfake "go.pinniped.dev/generated/1.19/client/supervisor/clientset/versioned/fake"
+	pinnipedinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions"
+	"go.pinniped.dev/internal/controllerlib"
+	"go.pinniped.dev/internal/oidc/provider"
+	"go.pinniped.dev/internal/testutil/testlogger"
+)
+
+func TestController(t *testing.T) {
+	t.Parallel()
+	now := metav1.NewTime(time.Now().UTC())
+	earlier := metav1.NewTime(now.Add(-1 * time.Hour).UTC())
+
+	// Start another test server that answers discovery successfully.
+	testIssuer := newTestIssuer(t)
+	testIssuerAuthorizeURL, err := url.Parse("https://example.com/authorize")
+	require.NoError(t, err)
+
+	var (
+		testNamespace        = "test-namespace"
+		testName             = "test-name"
+		testSecretName       = "test-client-secret"
+		testAdditionalScopes = []string{"scope1", "scope2", "scope3"}
+		testExpectedScopes   = []string{"openid", "scope1", "scope2", "scope3"}
+		testClientID         = "test-oidc-client-id"
+		testClientSecret     = "test-oidc-client-secret"
+		testValidSecretData  = map[string][]byte{"clientID": []byte(testClientID), "clientSecret": []byte(testClientSecret)}
+	)
+	tests := []struct {
+		name                   string
+		inputUpstreams         []runtime.Object
+		inputSecrets           []runtime.Object
+		wantErr                string
+		wantLogs               []string
+		wantResultingCache     []provider.UpstreamOIDCIdentityProvider
+		wantResultingUpstreams []v1alpha1.UpstreamOIDCProvider
+	}{
+		{
+			name: "no upstreams",
+		},
+		{
+			name: "missing secret",
+			inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Spec: v1alpha1.UpstreamOIDCProviderSpec{
+					Issuer:              testIssuer.URL,
+					Client:              v1alpha1.OIDCClient{SecretName: testSecretName},
+					AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes},
+				},
+			}},
+			inputSecrets: []runtime.Object{},
+			wantErr:      controllerlib.ErrSyntheticRequeue.Error(),
+			wantLogs: []string{
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="secret \"test-client-secret\" not found" "reason"="SecretNotFound" "status"="False" "type"="ClientCredentialsValid"`,
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`,
+				`upstream-observer "error"="UpstreamOIDCProvider has a failing condition" "msg"="found failing condition" "message"="secret \"test-client-secret\" not found" "name"="test-name" "namespace"="test-namespace" "reason"="SecretNotFound" "type"="ClientCredentialsValid"`,
+			},
+			wantResultingCache: []provider.UpstreamOIDCIdentityProvider{},
+			wantResultingUpstreams: []v1alpha1.UpstreamOIDCProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Status: v1alpha1.UpstreamOIDCProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						{
+							Type:               "ClientCredentialsValid",
+							Status:             "False",
+							LastTransitionTime: now,
+							Reason:             "SecretNotFound",
+							Message:            `secret "test-client-secret" not found`,
+						},
+						{
+							Type:               "OIDCDiscoverySucceeded",
+							Status:             "True",
+							LastTransitionTime: now,
+							Reason:             "Success",
+							Message:            "discovered issuer configuration",
+						},
+					},
+				},
+			}},
+		},
+		{
+			name: "secret has wrong type",
+			inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Spec: v1alpha1.UpstreamOIDCProviderSpec{
+					Issuer:              testIssuer.URL,
+					Client:              v1alpha1.OIDCClient{SecretName: testSecretName},
+					AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes},
+				},
+			}},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName},
+				Type:       "some-other-type",
+				Data:       testValidSecretData,
+			}},
+			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
+			wantLogs: []string{
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="referenced Secret \"test-client-secret\" has wrong type \"some-other-type\" (should be \"secrets.pinniped.dev/oidc-client\")" "reason"="SecretWrongType" "status"="False" "type"="ClientCredentialsValid"`,
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`,
+				`upstream-observer "error"="UpstreamOIDCProvider has a failing condition" "msg"="found failing condition" "message"="referenced Secret \"test-client-secret\" has wrong type \"some-other-type\" (should be \"secrets.pinniped.dev/oidc-client\")" "name"="test-name" "namespace"="test-namespace" "reason"="SecretWrongType" "type"="ClientCredentialsValid"`,
+			},
+			wantResultingCache: []provider.UpstreamOIDCIdentityProvider{},
+			wantResultingUpstreams: []v1alpha1.UpstreamOIDCProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Status: v1alpha1.UpstreamOIDCProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						{
+							Type:               "ClientCredentialsValid",
+							Status:             "False",
+							LastTransitionTime: now,
+							Reason:             "SecretWrongType",
+							Message:            `referenced Secret "test-client-secret" has wrong type "some-other-type" (should be "secrets.pinniped.dev/oidc-client")`,
+						},
+						{
+							Type:               "OIDCDiscoverySucceeded",
+							Status:             "True",
+							LastTransitionTime: now,
+							Reason:             "Success",
+							Message:            "discovered issuer configuration",
+						},
+					},
+				},
+			}},
+		},
+		{
+			name: "secret is missing key",
+			inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Spec: v1alpha1.UpstreamOIDCProviderSpec{
+					Issuer:              testIssuer.URL,
+					Client:              v1alpha1.OIDCClient{SecretName: testSecretName},
+					AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes},
+				},
+			}},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName},
+				Type:       "secrets.pinniped.dev/oidc-client",
+			}},
+			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
+			wantLogs: []string{
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="referenced Secret \"test-client-secret\" is missing required keys [\"clientID\" \"clientSecret\"]" "reason"="SecretMissingKeys" "status"="False" "type"="ClientCredentialsValid"`,
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`,
+				`upstream-observer "error"="UpstreamOIDCProvider has a failing condition" "msg"="found failing condition" "message"="referenced Secret \"test-client-secret\" is missing required keys [\"clientID\" \"clientSecret\"]" "name"="test-name" "namespace"="test-namespace" "reason"="SecretMissingKeys" "type"="ClientCredentialsValid"`,
+			},
+			wantResultingCache: []provider.UpstreamOIDCIdentityProvider{},
+			wantResultingUpstreams: []v1alpha1.UpstreamOIDCProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Status: v1alpha1.UpstreamOIDCProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						{
+							Type:               "ClientCredentialsValid",
+							Status:             "False",
+							LastTransitionTime: now,
+							Reason:             "SecretMissingKeys",
+							Message:            `referenced Secret "test-client-secret" is missing required keys ["clientID" "clientSecret"]`,
+						},
+						{
+							Type:               "OIDCDiscoverySucceeded",
+							Status:             "True",
+							LastTransitionTime: now,
+							Reason:             "Success",
+							Message:            "discovered issuer configuration",
+						},
+					},
+				},
+			}},
+		},
+		{
+			name: "issuer is invalid URL",
+			inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Spec: v1alpha1.UpstreamOIDCProviderSpec{
+					Issuer:              "invalid-url",
+					Client:              v1alpha1.OIDCClient{SecretName: testSecretName},
+					AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes},
+				},
+			}},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName},
+				Type:       "secrets.pinniped.dev/oidc-client",
+				Data:       testValidSecretData,
+			}},
+			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
+			wantLogs: []string{
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`,
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to perform OIDC discovery against \"invalid-url\": Get \"invalid-url/.well-known/openid-configuration\": unsupported protocol scheme \"\"" "reason"="Unreachable" "status"="False" "type"="OIDCDiscoverySucceeded"`,
+				`upstream-observer "error"="UpstreamOIDCProvider has a failing condition" "msg"="found failing condition" "message"="failed to perform OIDC discovery against \"invalid-url\": Get \"invalid-url/.well-known/openid-configuration\": unsupported protocol scheme \"\"" "name"="test-name" "namespace"="test-namespace" "reason"="Unreachable" "type"="OIDCDiscoverySucceeded"`,
+			},
+			wantResultingCache: []provider.UpstreamOIDCIdentityProvider{},
+			wantResultingUpstreams: []v1alpha1.UpstreamOIDCProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Status: v1alpha1.UpstreamOIDCProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						{
+							Type:               "ClientCredentialsValid",
+							Status:             "True",
+							LastTransitionTime: now,
+							Reason:             "Success",
+							Message:            "loaded client credentials",
+						},
+						{
+							Type:               "OIDCDiscoverySucceeded",
+							Status:             "False",
+							LastTransitionTime: now,
+							Reason:             "Unreachable",
+							Message:            `failed to perform OIDC discovery against "invalid-url": Get "invalid-url/.well-known/openid-configuration": unsupported protocol scheme ""`,
+						},
+					},
+				},
+			}},
+		},
+		{
+			name: "issuer returns invalid authorize URL",
+			inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Spec: v1alpha1.UpstreamOIDCProviderSpec{
+					Issuer:              testIssuer.URL + "/invalid",
+					Client:              v1alpha1.OIDCClient{SecretName: testSecretName},
+					AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes},
+				},
+			}},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName},
+				Type:       "secrets.pinniped.dev/oidc-client",
+				Data:       testValidSecretData,
+			}},
+			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
+			wantLogs: []string{
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`,
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="failed to parse authorization endpoint URL: parse \"%\": invalid URL escape \"%\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`,
+				`upstream-observer "error"="UpstreamOIDCProvider has a failing condition" "msg"="found failing condition" "message"="failed to parse authorization endpoint URL: parse \"%\": invalid URL escape \"%\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`,
+			},
+			wantResultingCache: []provider.UpstreamOIDCIdentityProvider{},
+			wantResultingUpstreams: []v1alpha1.UpstreamOIDCProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Status: v1alpha1.UpstreamOIDCProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						{
+							Type:               "ClientCredentialsValid",
+							Status:             "True",
+							LastTransitionTime: now,
+							Reason:             "Success",
+							Message:            "loaded client credentials",
+						},
+						{
+							Type:               "OIDCDiscoverySucceeded",
+							Status:             "False",
+							LastTransitionTime: now,
+							Reason:             "InvalidResponse",
+							Message:            `failed to parse authorization endpoint URL: parse "%": invalid URL escape "%"`,
+						},
+					},
+				},
+			}},
+		},
+		{
+			name: "issuer returns insecure authorize URL",
+			inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Spec: v1alpha1.UpstreamOIDCProviderSpec{
+					Issuer:              testIssuer.URL + "/insecure",
+					Client:              v1alpha1.OIDCClient{SecretName: testSecretName},
+					AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes},
+				},
+			}},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName},
+				Type:       "secrets.pinniped.dev/oidc-client",
+				Data:       testValidSecretData,
+			}},
+			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
+			wantLogs: []string{
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`,
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="authorization endpoint URL scheme must be \"https\", not \"http\"" "reason"="InvalidResponse" "status"="False" "type"="OIDCDiscoverySucceeded"`,
+				`upstream-observer "error"="UpstreamOIDCProvider has a failing condition" "msg"="found failing condition" "message"="authorization endpoint URL scheme must be \"https\", not \"http\"" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidResponse" "type"="OIDCDiscoverySucceeded"`,
+			},
+			wantResultingCache: []provider.UpstreamOIDCIdentityProvider{},
+			wantResultingUpstreams: []v1alpha1.UpstreamOIDCProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Status: v1alpha1.UpstreamOIDCProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						{
+							Type:               "ClientCredentialsValid",
+							Status:             "True",
+							LastTransitionTime: now,
+							Reason:             "Success",
+							Message:            "loaded client credentials",
+						},
+						{
+							Type:               "OIDCDiscoverySucceeded",
+							Status:             "False",
+							LastTransitionTime: now,
+							Reason:             "InvalidResponse",
+							Message:            `authorization endpoint URL scheme must be "https", not "http"`,
+						},
+					},
+				},
+			}},
+		},
+		{
+			name: "upstream becomes valid",
+			inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: "test-name"},
+				Spec: v1alpha1.UpstreamOIDCProviderSpec{
+					Issuer:              testIssuer.URL,
+					Client:              v1alpha1.OIDCClient{SecretName: testSecretName},
+					AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: append(testAdditionalScopes, "xyz", "openid")},
+				},
+				Status: v1alpha1.UpstreamOIDCProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						{Type: "ClientCredentialsValid", Status: "False", LastTransitionTime: earlier, Reason: "SomeError1", Message: "some previous error 1"},
+						{Type: "OIDCDiscoverySucceeded", Status: "False", LastTransitionTime: earlier, Reason: "SomeError2", Message: "some previous error 2"},
+					},
+				},
+			}},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName},
+				Type:       "secrets.pinniped.dev/oidc-client",
+				Data:       testValidSecretData,
+			}},
+			wantLogs: []string{
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`,
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`,
+			},
+			wantResultingCache: []provider.UpstreamOIDCIdentityProvider{{
+				Name:             testName,
+				ClientID:         testClientID,
+				AuthorizationURL: *testIssuerAuthorizeURL,
+				Scopes:           append(testExpectedScopes, "xyz"),
+			}},
+			wantResultingUpstreams: []v1alpha1.UpstreamOIDCProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName},
+				Status: v1alpha1.UpstreamOIDCProviderStatus{
+					Phase: "Ready",
+					Conditions: []v1alpha1.Condition{
+						{Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: now, Reason: "Success", Message: "loaded client credentials"},
+						{Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: now, Reason: "Success", Message: "discovered issuer configuration"},
+					},
+				},
+			}},
+		},
+		{
+			name: "existing valid upstream",
+			inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Spec: v1alpha1.UpstreamOIDCProviderSpec{
+					Issuer:              testIssuer.URL,
+					Client:              v1alpha1.OIDCClient{SecretName: testSecretName},
+					AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes},
+				},
+				Status: v1alpha1.UpstreamOIDCProviderStatus{
+					Phase: "Ready",
+					Conditions: []v1alpha1.Condition{
+						{Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials"},
+						{Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration"},
+					},
+				},
+			}},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName},
+				Type:       "secrets.pinniped.dev/oidc-client",
+				Data:       testValidSecretData,
+			}},
+			wantLogs: []string{
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`,
+				`upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="discovered issuer configuration" "reason"="Success" "status"="True" "type"="OIDCDiscoverySucceeded"`,
+			},
+			wantResultingCache: []provider.UpstreamOIDCIdentityProvider{{
+				Name:             testName,
+				ClientID:         testClientID,
+				AuthorizationURL: *testIssuerAuthorizeURL,
+				Scopes:           testExpectedScopes,
+			}},
+			wantResultingUpstreams: []v1alpha1.UpstreamOIDCProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Status: v1alpha1.UpstreamOIDCProviderStatus{
+					Phase: "Ready",
+					Conditions: []v1alpha1.Condition{
+						{Type: "ClientCredentialsValid", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "loaded client credentials", ObservedGeneration: 1234},
+						{Type: "OIDCDiscoverySucceeded", Status: "True", LastTransitionTime: earlier, Reason: "Success", Message: "discovered issuer configuration", ObservedGeneration: 1234},
+					},
+				},
+			}},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			fakePinnipedClient := pinnipedfake.NewSimpleClientset(tt.inputUpstreams...)
+			pinnipedInformers := pinnipedinformers.NewSharedInformerFactory(fakePinnipedClient, 0)
+			fakeKubeClient := fake.NewSimpleClientset(tt.inputSecrets...)
+			kubeInformers := informers.NewSharedInformerFactory(fakeKubeClient, 0)
+			testLog := testlogger.New(t)
+			cache := provider.NewDynamicUpstreamIDPProvider()
+			cache.SetIDPList([]provider.UpstreamOIDCIdentityProvider{{Name: "initial-entry"}})
+
+			controller := New(
+				cache,
+				fakePinnipedClient,
+				pinnipedInformers.IDP().V1alpha1().UpstreamOIDCProviders(),
+				kubeInformers.Core().V1().Secrets(),
+				testLog)
+
+			ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+			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)
+			}
+			require.Equal(t, strings.Join(tt.wantLogs, "\n"), strings.Join(testLog.Lines(), "\n"))
+			require.ElementsMatch(t, tt.wantResultingCache, cache.GetIDPList())
+
+			actualUpstreams, err := fakePinnipedClient.IDPV1alpha1().UpstreamOIDCProviders(testNamespace).List(ctx, metav1.ListOptions{})
+			require.NoError(t, err)
+
+			// Preprocess the set of upstreams a bit so that they're easier to assert against.
+			require.ElementsMatch(t, tt.wantResultingUpstreams, normalizeUpstreams(actualUpstreams.Items, now))
+
+			// Running the sync() a second time should be idempotent except for logs, and should return the same error.
+			// This also helps exercise code paths where the OIDC provider discovery hits cache.
+			if err := controllerlib.TestSync(t, controller, syncCtx); tt.wantErr != "" {
+				require.EqualError(t, err, tt.wantErr)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+func normalizeUpstreams(upstreams []v1alpha1.UpstreamOIDCProvider, now metav1.Time) []v1alpha1.UpstreamOIDCProvider {
+	result := make([]v1alpha1.UpstreamOIDCProvider, 0, len(upstreams))
+	for _, u := range upstreams {
+		normalized := u.DeepCopy()
+
+		// We're only interested in comparing the status, so zero out the spec.
+		normalized.Spec = v1alpha1.UpstreamOIDCProviderSpec{}
+
+		// Round down the LastTransitionTime values to `now` if they were just updated. This makes
+		// it much easier to encode assertions about the expected timestamps.
+		for i := range normalized.Status.Conditions {
+			if time.Since(normalized.Status.Conditions[i].LastTransitionTime.Time) < 5*time.Second {
+				normalized.Status.Conditions[i].LastTransitionTime = now
+			}
+		}
+		result = append(result, *normalized)
+	}
+
+	return result
+}
+
+func newTestIssuer(t *testing.T) *httptest.Server {
+	mux := http.NewServeMux()
+	testServer := httptest.NewServer(mux)
+	t.Cleanup(testServer.Close)
+
+	type providerJSON struct {
+		Issuer   string `json:"issuer"`
+		AuthURL  string `json:"authorization_endpoint"`
+		TokenURL string `json:"token_endpoint"`
+		JWKSURL  string `json:"jwks_uri"`
+	}
+
+	// At the root of the server, serve an issuer with a valid discovery response.
+	mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("content-type", "application/json")
+		_ = json.NewEncoder(w).Encode(&providerJSON{
+			Issuer:  testServer.URL,
+			AuthURL: "https://example.com/authorize",
+		})
+	})
+
+	// At "/invalid", serve an issuer that returns an invalid authorization URL (not parseable).
+	mux.HandleFunc("/invalid/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("content-type", "application/json")
+		_ = json.NewEncoder(w).Encode(&providerJSON{
+			Issuer:  testServer.URL + "/invalid",
+			AuthURL: "%",
+		})
+	})
+
+	// At "/insecure", serve an issuer that returns an insecure authorization URL (not https://).
+	mux.HandleFunc("/insecure/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("content-type", "application/json")
+		_ = json.NewEncoder(w).Encode(&providerJSON{
+			Issuer:  testServer.URL + "/insecure",
+			AuthURL: "http://example.com/authorize",
+		})
+	})
+
+	return testServer
+}