Populate internal GitHub IDP Config from CRD

internal/controller/authenticator/authenticator.go

@@ -1,19 +1,9 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package authenticator contains helper code for dealing with *Authenticator CRDs.
 package authenticator
 
-import (
-	"crypto/x509"
-	"encoding/base64"
-	"fmt"
-
-	"k8s.io/client-go/util/cert"
-
-	auth1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1"
-)
-
 // Closer is a type that can be closed idempotently.
 //
 // This type is slightly different from io.Closer, because io.Closer can return an error and is not
@@ -21,24 +11,3 @@ import (
 type Closer interface {
 	Close()
 }
-
-// CABundle returns a PEM-encoded CA bundle from the provided spec. If the provided spec is nil, a
-// nil CA bundle will be returned. If the provided spec contains a CA bundle that is not properly
-// encoded, an error will be returned.
-func CABundle(spec *auth1alpha1.TLSSpec) (*x509.CertPool, []byte, error) {
-	if spec == nil || len(spec.CertificateAuthorityData) == 0 {
-		return nil, nil, nil
-	}
-
-	pem, err := base64.StdEncoding.DecodeString(spec.CertificateAuthorityData)
-	if err != nil {
-		return nil, nil, err
-	}
-
-	rootCAs, err := cert.NewPoolFromBytes(pem)
-	if err != nil {
-		return nil, nil, fmt.Errorf("certificateAuthorityData is not valid PEM: %w", err)
-	}
-
-	return rootCAs, pem, nil
-}

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go

@@ -240,7 +240,7 @@ func (c *jwtCacheFillerController) extractValueAsJWTAuthenticator(value authncac
 }
 
 func (c *jwtCacheFillerController) validateTLS(tlsSpec *auth1alpha1.TLSSpec, conditions []*metav1.Condition) (*x509.CertPool, []*metav1.Condition, bool) {
-	rootCAs, _, err := pinnipedauthenticator.CABundle(tlsSpec)
+	rootCAs, _, err := pinnipedcontroller.BuildCertPoolAuth(tlsSpec)
 	if err != nil {
 		msg := fmt.Sprintf("%s: %s", "invalid TLS configuration", err.Error())
 		conditions = append(conditions, &metav1.Condition{
@@ -594,7 +594,7 @@ func (c *jwtCacheFillerController) updateStatus(
 		})
 	}
 
-	_ = conditionsutil.MergeConfigConditions(
+	_ = conditionsutil.MergeConditions(
 		conditions,
 		original.Generation,
 		&updated.Status.Conditions,

internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go

@@ -30,7 +30,6 @@ import (
 	conciergeclientset "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned"
 	authinformers "go.pinniped.dev/generated/latest/client/concierge/informers/externalversions/authentication/v1alpha1"
 	pinnipedcontroller "go.pinniped.dev/internal/controller"
-	pinnipedauthenticator "go.pinniped.dev/internal/controller/authenticator"
 	"go.pinniped.dev/internal/controller/authenticator/authncache"
 	"go.pinniped.dev/internal/controller/conditionsutil"
 	"go.pinniped.dev/internal/controllerlib"
@@ -316,7 +315,7 @@ func (c *webhookCacheFillerController) validateConnection(certPool *x509.CertPoo
 }
 
 func (c *webhookCacheFillerController) validateTLSBundle(tlsSpec *auth1alpha1.TLSSpec, conditions []*metav1.Condition) (*x509.CertPool, []byte, []*metav1.Condition, bool) {
-	rootCAs, pemBytes, err := pinnipedauthenticator.CABundle(tlsSpec)
+	rootCAs, pemBytes, err := pinnipedcontroller.BuildCertPoolAuth(tlsSpec)
 	if err != nil {
 		msg := fmt.Sprintf("%s: %s", "invalid TLS configuration", err.Error())
 		conditions = append(conditions, &metav1.Condition{
@@ -411,7 +410,7 @@ func (c *webhookCacheFillerController) updateStatus(
 		})
 	}
 
-	_ = conditionsutil.MergeConfigConditions(
+	_ = conditionsutil.MergeConditions(
 		conditions,
 		original.Generation,
 		&updated.Status.Conditions,

internal/controller/conditionsutil/conditions_util.go

@@ -12,29 +12,34 @@ import (
 	"go.pinniped.dev/internal/plog"
 )
 
-// MergeIDPConditions merges conditions into conditionsToUpdate. If returns true if it merged any error conditions.
-func MergeIDPConditions(conditions []*metav1.Condition, observedGeneration int64, conditionsToUpdate *[]metav1.Condition, log plog.MinLogger) bool {
-	hadErrorCondition := false
+// MergeConditions merges conditions into conditionsToUpdate.
+// Note that LastTransitionTime refers to the time when the status changed,
+// but ObservedGeneration should be the current generation for all conditions, since Pinniped should always check every condition.
+// It returns true if any resulting condition has non-true status.
+func MergeConditions(
+	conditions []*metav1.Condition,
+	observedGeneration int64,
+	conditionsToUpdate *[]metav1.Condition,
+	log plog.MinLogger,
+	lastTransitionTime metav1.Time,
+) bool {
 	for i := range conditions {
 		cond := conditions[i].DeepCopy()
-		cond.LastTransitionTime = metav1.Now()
+		cond.LastTransitionTime = lastTransitionTime
 		cond.ObservedGeneration = observedGeneration
-		if mergeIDPCondition(conditionsToUpdate, cond) {
+		if mergeCondition(conditionsToUpdate, cond) {
 			log.Info("updated condition", "type", cond.Type, "status", cond.Status, "reason", cond.Reason, "message", cond.Message)
 		}
-		if cond.Status == metav1.ConditionFalse {
-			hadErrorCondition = true
-		}
 	}
 	sort.SliceStable(*conditionsToUpdate, func(i, j int) bool {
 		return (*conditionsToUpdate)[i].Type < (*conditionsToUpdate)[j].Type
 	})
-	return hadErrorCondition
+	return HadErrorCondition(conditions)
 }
 
-// mergeIDPCondition merges a new metav1.Condition into a slice of existing conditions. It returns true
+// mergeCondition merges a new metav1.Condition into a slice of existing conditions. It returns true
 // if the condition has meaningfully changed.
-func mergeIDPCondition(existing *[]metav1.Condition, new *metav1.Condition) bool {
+func mergeCondition(existing *[]metav1.Condition, new *metav1.Condition) bool {
 	// Find any existing condition with a matching type.
 	var old *metav1.Condition
 	for i := range *existing {
@@ -62,61 +67,7 @@ func mergeIDPCondition(existing *[]metav1.Condition, new *metav1.Condition) bool
 		return true
 	}
 
-	// Otherwise the entry is already up to date.
-	return false
-}
-
-// MergeConfigConditions merges conditions into conditionsToUpdate. It returns true if it merged any error conditions.
-func MergeConfigConditions(conditions []*metav1.Condition, observedGeneration int64, conditionsToUpdate *[]metav1.Condition, log plog.MinLogger, now metav1.Time) bool {
-	hadErrorCondition := false
-	for i := range conditions {
-		cond := conditions[i].DeepCopy()
-		cond.LastTransitionTime = now
-		cond.ObservedGeneration = observedGeneration
-		if mergeConfigCondition(conditionsToUpdate, cond) {
-			log.Info("updated condition", "type", cond.Type, "status", cond.Status, "reason", cond.Reason, "message", cond.Message)
-		}
-		if cond.Status == metav1.ConditionFalse {
-			hadErrorCondition = true
-		}
-	}
-	sort.SliceStable(*conditionsToUpdate, func(i, j int) bool {
-		return (*conditionsToUpdate)[i].Type < (*conditionsToUpdate)[j].Type
-	})
-	return hadErrorCondition
-}
-
-// mergeConfigCondition merges a new metav1.Condition into a slice of existing conditions. It returns true
-// if the condition has meaningfully changed.
-func mergeConfigCondition(existing *[]metav1.Condition, new *metav1.Condition) bool {
-	// Find any existing condition with a matching type.
-	var old *metav1.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.
+	// Otherwise the entry is already up-to-date.
 	return false
 }
 

internal/controller/conditionsutil/conditions_util_test.go

@@ -0,0 +1,158 @@
+// Copyright 2024 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package conditionsutil
+
+import (
+	"bytes"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"go.pinniped.dev/internal/plog"
+)
+
+func TestMergeIDPConditions(t *testing.T) {
+	twoHoursAgo := metav1.Time{Time: time.Now().Add(-2 * time.Hour)}
+	oneHourAgo := metav1.Time{Time: time.Now().Add(-1 * time.Hour)}
+	testTime := metav1.Now()
+
+	tests := []struct {
+		name               string
+		newConditions      []*metav1.Condition
+		conditionsToUpdate *[]metav1.Condition
+		observedGeneration int64
+		wantResult         bool
+		wantLogSnippets    []string
+		wantConditions     []metav1.Condition
+	}{
+		{
+			name: "True -> False returns true",
+			newConditions: []*metav1.Condition{
+				{
+					Type:    "UnchangedType",
+					Status:  metav1.ConditionTrue,
+					Reason:  "unchanged reason",
+					Message: "unchanged message",
+				},
+				{
+					Type:    "FalseToTrueType",
+					Status:  metav1.ConditionFalse,
+					Reason:  "new reason",
+					Message: "new message",
+				},
+				{
+					Type:    "NewType",
+					Status:  metav1.ConditionTrue,
+					Reason:  "new reason",
+					Message: "new message",
+				},
+			},
+			conditionsToUpdate: &[]metav1.Condition{
+				{
+					Type:               "UnchangedType",
+					Status:             metav1.ConditionTrue,
+					ObservedGeneration: int64(10),
+					LastTransitionTime: twoHoursAgo,
+					Reason:             "unchanged reason",
+					Message:            "unchanged message",
+				},
+				{
+					Type:               "FalseToTrueType",
+					Status:             metav1.ConditionTrue,
+					ObservedGeneration: int64(5),
+					LastTransitionTime: oneHourAgo,
+					Reason:             "old reason",
+					Message:            "old message",
+				},
+			},
+			observedGeneration: int64(100),
+			wantLogSnippets: []string{
+				`"message":"updated condition","type":"UnchangedType","status":"True"`,
+				`"message":"updated condition","type":"NewType","status":"True"`,
+				`"message":"updated condition","type":"FalseToTrueType","status":"False"`,
+			},
+			wantConditions: []metav1.Condition{
+				{
+					Type:               "FalseToTrueType",
+					Status:             metav1.ConditionFalse,
+					ObservedGeneration: int64(100),
+					LastTransitionTime: testTime,
+					Reason:             "new reason",
+					Message:            "new message",
+				},
+				{
+					Type:               "NewType",
+					Status:             metav1.ConditionTrue,
+					ObservedGeneration: int64(100),
+					LastTransitionTime: testTime,
+					Reason:             "new reason",
+					Message:            "new message",
+				},
+				{
+					Type:               "UnchangedType",
+					Status:             metav1.ConditionTrue,
+					ObservedGeneration: int64(100),
+					LastTransitionTime: twoHoursAgo,
+					Reason:             "unchanged reason",
+					Message:            "unchanged message",
+				},
+			},
+			wantResult: true,
+		},
+		{
+			name: "No logs when ObservedGeneration is unchanged",
+			newConditions: []*metav1.Condition{
+				{
+					Type:    "UnchangedType",
+					Status:  metav1.ConditionFalse,
+					Reason:  "unchanged reason",
+					Message: "unchanged message",
+				},
+			},
+			conditionsToUpdate: &[]metav1.Condition{
+				{
+					Type:               "UnchangedType",
+					Status:             metav1.ConditionFalse,
+					ObservedGeneration: int64(10),
+					LastTransitionTime: twoHoursAgo,
+					Reason:             "unchanged reason",
+					Message:            "unchanged message",
+				},
+			},
+			observedGeneration: int64(10),
+			wantConditions: []metav1.Condition{
+				{
+					Type:               "UnchangedType",
+					Status:             metav1.ConditionFalse,
+					ObservedGeneration: int64(10),
+					LastTransitionTime: twoHoursAgo,
+					Reason:             "unchanged reason",
+					Message:            "unchanged message",
+				},
+			},
+			wantResult: true,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			var log bytes.Buffer
+			logger := plog.TestLogger(t, &log)
+
+			result := MergeConditions(tt.newConditions, tt.observedGeneration, tt.conditionsToUpdate, logger, testTime)
+
+			logString := log.String()
+			require.Equal(t, len(tt.wantLogSnippets), strings.Count(logString, "\n"))
+			for _, wantLog := range tt.wantLogSnippets {
+				require.Contains(t, logString, wantLog)
+			}
+			require.Equal(t, tt.wantResult, result)
+			require.Equal(t, tt.wantConditions, *tt.conditionsToUpdate)
+		})
+	}
+}

internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go

@@ -1,4 +1,4 @@
-// Copyright 2021-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package activedirectoryupstreamwatcher implements a controller which watches ActiveDirectoryIdentityProviders.
@@ -368,7 +368,7 @@ func (c *activeDirectoryWatcherController) updateStatus(ctx context.Context, ups
 	log := plog.WithValues("namespace", upstream.Namespace, "name", upstream.Name)
 	updated := upstream.DeepCopy()
 
-	hadErrorCondition := conditionsutil.MergeIDPConditions(conditions, upstream.Generation, &updated.Status.Conditions, log)
+	hadErrorCondition := conditionsutil.MergeConditions(conditions, upstream.Generation, &updated.Status.Conditions, log, metav1.Now())
 
 	updated.Status.Phase = v1alpha1.ActiveDirectoryPhaseReady
 	if hadErrorCondition {

internal/controller/supervisorconfig/federation_domain_watcher.go

@@ -813,7 +813,7 @@ func (c *federationDomainWatcherController) updateStatus(
 		})
 	}
 
-	_ = conditionsutil.MergeConfigConditions(conditions,
+	_ = conditionsutil.MergeConditions(conditions,
 		federationDomain.Generation, &updated.Status.Conditions, plog.New().WithName(controllerName), metav1.NewTime(c.clock.Now()))
 
 	if equality.Semantic.DeepEqual(federationDomain, updated) {

internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go

@@ -6,39 +6,54 @@ package githubupstreamwatcher
 
 import (
 	"context"
+	"crypto/tls"
+	"crypto/x509"
+	"errors"
 	"fmt"
+	"net"
+	"net/http"
+	"slices"
+	"strings"
 
+	"golang.org/x/oauth2"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
+	k8sapierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	errorsutil "k8s.io/apimachinery/pkg/util/errors"
+	k8sutilerrors "k8s.io/apimachinery/pkg/util/errors"
 	corev1informers "k8s.io/client-go/informers/core/v1"
+	"k8s.io/utils/clock"
 
 	"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/controller/supervisorconfig/upstreamwatchers"
 	"go.pinniped.dev/internal/controllerlib"
+	"go.pinniped.dev/internal/crypto/ptls"
+	"go.pinniped.dev/internal/endpointaddr"
 	"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"
+	gitHubClientSecretType               corev1.SecretType = "secrets.pinniped.dev/github-client"
+	clientIDDataKey, clientSecretDataKey string            = "clientID", "clientSecret"
 
-	//
-	// clientIDDataKey     = "clientID"
-	// clientSecretDataKey = "clientSecret"
-	//
-	// // Constants related to conditions.
-	// typeClientCredentialsValid = "ClientCredentialsValid" //nolint:gosec // this is not a credential.
+	HostValid                string = "HostValid"
+	TLSConfigurationValid    string = "TLSConfigurationValid"
+	OrganizationsPolicyValid string = "OrganizationsPolicyValid"
+	// ClientCredentialsObtained is different from other status conditions because it only checks that the client credentials
+	// have been obtained. The controller has no way to verify whether they are valid.
+	ClientCredentialsObtained string = "ClientCredentialsObtained" //nolint:gosec // this is not a credential
+	GitHubConnectionValid     string = "GitHubConnectionValid"
 )
 
 // UpstreamGitHubIdentityProviderICache is a thread safe cache that holds a list of validated upstream GitHub IDP configurations.
@@ -47,40 +62,52 @@ type UpstreamGitHubIdentityProviderICache interface {
 }
 
 type gitHubWatcherController struct {
+	namespace                      string
 	cache                          UpstreamGitHubIdentityProviderICache
 	log                            plog.Logger
 	client                         supervisorclientset.Interface
 	gitHubIdentityProviderInformer idpinformers.GitHubIdentityProviderInformer
 	secretInformer                 corev1informers.SecretInformer
+	httpClientBuilder              func(rootCAs *x509.CertPool) *http.Client
+	clock                          clock.Clock
 }
 
 // New instantiates a new controllerlib.Controller which will populate the provided UpstreamGitHubIdentityProviderICache.
 func New(
+	namespace string,
 	idpCache UpstreamGitHubIdentityProviderICache,
 	client supervisorclientset.Interface,
 	gitHubIdentityProviderInformer idpinformers.GitHubIdentityProviderInformer,
 	secretInformer corev1informers.SecretInformer,
 	log plog.Logger,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
+	clock clock.Clock,
+	httpClientBuilder func(rootCAs *x509.CertPool) *http.Client,
 ) controllerlib.Controller {
 	c := gitHubWatcherController{
+		namespace:                      namespace,
 		cache:                          idpCache,
 		client:                         client,
 		log:                            log.WithName(controllerName),
 		gitHubIdentityProviderInformer: gitHubIdentityProviderInformer,
 		secretInformer:                 secretInformer,
+		httpClientBuilder:              httpClientBuilder,
+		clock:                          clock,
 	}
 
 	return controllerlib.New(
 		controllerlib.Config{Name: controllerName, Syncer: &c},
 		withInformer(
 			gitHubIdentityProviderInformer,
-			pinnipedcontroller.MatchAnythingFilter(pinnipedcontroller.SingletonQueue()),
+			pinnipedcontroller.SimpleFilter(func(obj metav1.Object) bool {
+				gitHubIDP, ok := obj.(*v1alpha1.GitHubIdentityProvider)
+				return ok && gitHubIDP.Namespace == namespace
+			}, pinnipedcontroller.SingletonQueue()),
 			controllerlib.InformerOption{},
 		),
 		withInformer(
 			secretInformer,
-			pinnipedcontroller.MatchAnySecretOfTypeFilter(gitHubClientSecretType, pinnipedcontroller.SingletonQueue()),
+			pinnipedcontroller.MatchAnySecretOfTypeFilter(gitHubClientSecretType, pinnipedcontroller.SingletonQueue(), namespace),
 			controllerlib.InformerOption{},
 		),
 	)
@@ -89,79 +116,358 @@ func New(
 // Sync implements controllerlib.Syncer.
 func (c *gitHubWatcherController) Sync(ctx controllerlib.Context) error {
 	actualUpstreams, err := c.gitHubIdentityProviderInformer.Lister().List(labels.Everything())
-	if err != nil {
+	if err != nil { // untested
 		return fmt.Errorf("failed to list GitHubIdentityProviders: %w", err)
 	}
 
-	var errs []error
+	// Sort them by name just so that the logs output is consistent
+	slices.SortStableFunc(actualUpstreams, func(a, b *v1alpha1.GitHubIdentityProvider) int {
+		return strings.Compare(a.Name, b.Name)
+	})
 
-	requeue := false
+	var applicationErrors []error
 	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))
+		validatedUpstream, applicationErr := c.validateUpstreamAndUpdateConditions(ctx, upstream)
+		if applicationErr != nil {
+			applicationErrors = append(applicationErrors, applicationErr)
+		} else if validatedUpstream != nil {
+			validatedUpstreams = append(validatedUpstreams, validatedUpstream)
 		}
+		// Else:
+		// If both validatedUpstream and applicationErr are nil, this must be because the upstream had configuration errors.
+		// This controller should take no action until the user has reconfigured the upstream.
 	}
 	c.cache.SetGitHubIdentityProviders(validatedUpstreams)
-	if requeue {
-		return controllerlib.ErrSyntheticRequeue
+
+	// If we have recoverable application errors, let's do a requeue and capture all the applicationErrors too
+	if len(applicationErrors) > 0 {
+		applicationErrors = append([]error{controllerlib.ErrSyntheticRequeue}, applicationErrors...)
 	}
 
-	// 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)
+	return errorsutil.NewAggregate(applicationErrors)
 }
 
-func (c *gitHubWatcherController) validateUpstream(ctx controllerlib.Context, upstream *v1alpha1.GitHubIdentityProvider) (*upstreamgithub.ProviderConfig, error) {
-	result := upstreamgithub.ProviderConfig{
-		Name: upstream.Name,
+func (c *gitHubWatcherController) validateClientSecret(secretName string) (*metav1.Condition, string, string, error) {
+	secret, unableToRetrieveSecretErr := c.secretInformer.Lister().Secrets(c.namespace).Get(secretName)
+
+	// This error requires user interaction, so ignore it.
+	if k8sapierrors.IsNotFound(unableToRetrieveSecretErr) {
+		unableToRetrieveSecretErr = nil
 	}
 
-	// TODO: once we firm up the proposal doc & merge, then firm up the CRD & merge, we can fill out these validations.
+	buildFalseCondition := func(prefix string) (*metav1.Condition, string, string, error) {
+		return &metav1.Condition{
+			Type:   ClientCredentialsObtained,
+			Status: metav1.ConditionFalse,
+			Reason: upstreamwatchers.ReasonNotFound,
+			Message: fmt.Sprintf("%s: secret from spec.client.SecretName (%q) must be found in namespace %q with type %q and keys %q and %q",
+				prefix,
+				secretName,
+				c.namespace,
+				gitHubClientSecretType,
+				clientIDDataKey,
+				clientSecretDataKey),
+		}, "", "", unableToRetrieveSecretErr
+	}
+
+	if unableToRetrieveSecretErr != nil || secret == nil {
+		return buildFalseCondition(fmt.Sprintf("secret %q not found", secretName))
+	}
+
+	if secret.Type != gitHubClientSecretType {
+		return buildFalseCondition(fmt.Sprintf("wrong secret type %q", secret.Type))
+	}
+
+	clientID := string(secret.Data[clientIDDataKey])
+	if len(clientID) < 1 {
+		return buildFalseCondition(fmt.Sprintf("missing key %q", clientIDDataKey))
+	}
+
+	clientSecret := string(secret.Data[clientSecretDataKey])
+	if len(clientSecret) < 1 {
+		return buildFalseCondition(fmt.Sprintf("missing key %q", clientSecretDataKey))
+	}
+
+	if len(secret.Data) != 2 {
+		return buildFalseCondition("extra keys found")
+	}
+
+	return &metav1.Condition{
+		Type:    ClientCredentialsObtained,
+		Status:  metav1.ConditionTrue,
+		Reason:  upstreamwatchers.ReasonSuccess,
+		Message: fmt.Sprintf("clientID and clientSecret have been read from spec.client.SecretName (%q)", secretName),
+	}, clientID, clientSecret, nil
+}
+
+func validateOrganizationsPolicy(organizationsSpec *v1alpha1.GitHubOrganizationsSpec) (*metav1.Condition, v1alpha1.GitHubAllowedAuthOrganizationsPolicy) {
+	var policy v1alpha1.GitHubAllowedAuthOrganizationsPolicy
+	if organizationsSpec.Policy != nil {
+		policy = *organizationsSpec.Policy
+	}
+
+	// Should not happen due to CRD defaulting, enum validation, and CEL validation (for recent versions of K8s only!)
+	// That is why the message here is very minimal
+	if (policy == v1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers && len(organizationsSpec.Allowed) == 0) ||
+		(policy == v1alpha1.GitHubAllowedAuthOrganizationsPolicyOnlyUsersFromAllowedOrganizations && len(organizationsSpec.Allowed) > 0) {
+		return &metav1.Condition{
+			Type:    OrganizationsPolicyValid,
+			Status:  metav1.ConditionTrue,
+			Reason:  upstreamwatchers.ReasonSuccess,
+			Message: fmt.Sprintf("spec.allowAuthentication.organizations.policy (%q) is valid", policy),
+		}, policy
+	}
+
+	if len(organizationsSpec.Allowed) > 0 {
+		return &metav1.Condition{
+			Type:    OrganizationsPolicyValid,
+			Status:  metav1.ConditionFalse,
+			Reason:  "Invalid",
+			Message: "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed",
+		}, policy
+	}
+
+	return &metav1.Condition{
+		Type:    OrganizationsPolicyValid,
+		Status:  metav1.ConditionFalse,
+		Reason:  "Invalid",
+		Message: "spec.allowAuthentication.organizations.policy must be 'AllGitHubUsers' when spec.allowAuthentication.organizations.allowed is empty",
+	}, policy
+}
+
+func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx controllerlib.Context, upstream *v1alpha1.GitHubIdentityProvider) (
+	*upstreamgithub.ProviderConfig, // If validated, returns the config
+	error, // This error will only refer to programmatic errors such as inability to perform a Dial or dereference a pointer, not configuration errors
+) {
+	conditions := make([]*metav1.Condition, 0)
+	applicationErrors := make([]error, 0)
+
+	clientSecretCondition, clientID, clientSecret, clientSecretErr := c.validateClientSecret(upstream.Spec.Client.SecretName)
+	conditions = append(conditions, clientSecretCondition)
+	if clientSecretErr != nil { // untested
+		applicationErrors = append(applicationErrors, clientSecretErr)
+	}
+
+	// Should there be some sort of catch-all condition to capture this?
+	// This does not actually prevent a GitHub IDP from being added to the cache.
+	// CRD defaulting and validation should eliminate the possibility of an error here.
+	groupNameAttribute, usernameAttribute, userAndGroupErr := validateUserAndGroupAttributes(upstream)
+	if userAndGroupErr != nil {
+		applicationErrors = append(applicationErrors, userAndGroupErr)
+	}
+
+	organizationPolicyCondition, policy := validateOrganizationsPolicy(&upstream.Spec.AllowAuthentication.Organizations)
+	conditions = append(conditions, organizationPolicyCondition)
+
+	hostCondition, hostPort := validateHost(upstream.Spec.GitHubAPI)
+	conditions = append(conditions, hostCondition)
+
+	tlsConfigCondition, certPool := c.validateTLSConfiguration(upstream.Spec.GitHubAPI.TLS)
+	conditions = append(conditions, tlsConfigCondition)
+
+	githubConnectionCondition, hostURL, httpClient, githubConnectionErr := c.validateGitHubConnection(
+		hostPort,
+		certPool,
+		hostCondition.Status == metav1.ConditionTrue && tlsConfigCondition.Status == metav1.ConditionTrue,
+	)
+	if githubConnectionErr != nil {
+		applicationErrors = append(applicationErrors, githubConnectionErr)
+	}
+	conditions = append(conditions, githubConnectionCondition)
+
 	// 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(),
+	// same set of conditions.  Conditions depending on other conditions should get Status: metav1.ConditionUnknown, or
+	// Status: metav1.ConditionFalse, never be omitted.
+	if len(conditions) != 5 { // untested since all code paths result in 5 conditions
+		applicationErrors = append(applicationErrors, fmt.Errorf("expected %d conditions but found %d conditions", 5, len(conditions)))
+		return nil, k8sutilerrors.NewAggregate(applicationErrors)
+	}
+	hadErrorCondition, updateStatusErr := c.updateStatus(ctx.Context, upstream, conditions)
+	if updateStatusErr != nil { // untested
+		applicationErrors = append(applicationErrors, updateStatusErr)
+	}
+	// Any error condition means we will not add the IDP to the cache, so just return nil here
+	if hadErrorCondition {
+		return nil, k8sutilerrors.NewAggregate(applicationErrors)
 	}
 
-	err := c.updateStatus(ctx.Context, upstream, conditions)
-	return &result, err
+	providerConfig := &upstreamgithub.ProviderConfig{
+		Name:               upstream.Name,
+		ResourceUID:        upstream.UID,
+		Host:               hostURL,
+		GroupNameAttribute: groupNameAttribute,
+		UsernameAttribute:  usernameAttribute,
+		OAuth2Config: &oauth2.Config{
+			ClientID:     clientID,
+			ClientSecret: clientSecret,
+		},
+		AllowedOrganizations:    upstream.Spec.AllowAuthentication.Organizations.Allowed,
+		OrganizationLoginPolicy: policy,
+		AuthorizationURL:        fmt.Sprintf("%s/login/oauth/authorize", hostURL),
+		HttpClient:              httpClient,
+	}
+	return providerConfig, k8sutilerrors.NewAggregate(applicationErrors)
+}
+
+func validateHost(gitHubAPIConfig v1alpha1.GitHubAPIConfig) (*metav1.Condition, *endpointaddr.HostPort) {
+	buildInvalidHost := func(host, reason string) *metav1.Condition {
+		return &metav1.Condition{
+			Type:    HostValid,
+			Status:  metav1.ConditionFalse,
+			Reason:  "InvalidHost",
+			Message: fmt.Sprintf("spec.githubAPI.host (%q) is not valid: %s", host, reason),
+		}
+	}
+
+	// Should not happen due to CRD defaulting
+	if gitHubAPIConfig.Host == nil || len(*gitHubAPIConfig.Host) < 1 {
+		return buildInvalidHost("", "must not be empty"), nil
+	}
+
+	host := *gitHubAPIConfig.Host
+	hostPort, addressParseErr := endpointaddr.Parse(host, 443)
+	if addressParseErr != nil {
+		// addressParseErr is not recoverable. It requires user interaction, so do not return the error.
+		return buildInvalidHost(host, addressParseErr.Error()), nil
+	}
+
+	return &metav1.Condition{
+		Type:    HostValid,
+		Status:  metav1.ConditionTrue,
+		Reason:  upstreamwatchers.ReasonSuccess,
+		Message: fmt.Sprintf("spec.githubAPI.host (%q) is valid", host),
+	}, &hostPort
+}
+
+func (c *gitHubWatcherController) validateTLSConfiguration(tlsSpec *v1alpha1.TLSSpec) (*metav1.Condition, *x509.CertPool) {
+	certPool, _, buildCertPoolErr := pinnipedcontroller.BuildCertPoolIDP(tlsSpec)
+	if buildCertPoolErr != nil {
+		// buildCertPoolErr is not recoverable with a resync.
+		// It requires user interaction, so do not return the error.
+		return &metav1.Condition{
+			Type:    TLSConfigurationValid,
+			Status:  metav1.ConditionFalse,
+			Reason:  "InvalidTLSConfig",
+			Message: fmt.Sprintf("spec.githubAPI.tls.certificateAuthorityData is not valid: %s", buildCertPoolErr),
+		}, nil
+	}
+
+	return &metav1.Condition{
+		Type:    TLSConfigurationValid,
+		Status:  metav1.ConditionTrue,
+		Reason:  upstreamwatchers.ReasonSuccess,
+		Message: "spec.githubAPI.tls.certificateAuthorityData is valid",
+	}, certPool
+}
+
+func (c *gitHubWatcherController) validateGitHubConnection(
+	hostPort *endpointaddr.HostPort,
+	certPool *x509.CertPool,
+	validSoFar bool,
+) (*metav1.Condition, string, *http.Client, error) {
+	if !validSoFar {
+		return &metav1.Condition{
+			Type:    GitHubConnectionValid,
+			Status:  metav1.ConditionUnknown,
+			Reason:  "UnableToValidate",
+			Message: "unable to validate; see other conditions for details",
+		}, "", nil, nil
+	}
+
+	conn, tlsDialErr := tls.Dial("tcp", hostPort.Endpoint(), ptls.Default(certPool))
+	if tlsDialErr != nil {
+		return &metav1.Condition{
+			Type:    GitHubConnectionValid,
+			Status:  metav1.ConditionFalse,
+			Reason:  "UnableToDialServer",
+			Message: fmt.Sprintf("cannot dial server spec.githubAPI.host (%q): %s", hostPort.Endpoint(), buildDialErrorMessage(tlsDialErr)),
+		}, "", nil, tlsDialErr
+	}
+
+	return &metav1.Condition{
+		Type:    GitHubConnectionValid,
+		Status:  metav1.ConditionTrue,
+		Reason:  upstreamwatchers.ReasonSuccess,
+		Message: fmt.Sprintf("spec.githubAPI.host (%q) is reachable and TLS verification succeeds", hostPort.Endpoint()),
+	}, fmt.Sprintf("https://%s", hostPort.Endpoint()), c.httpClientBuilder(certPool), conn.Close()
+}
+
+// buildDialErrorMessage standardizes DNS error messages that appear differently on different platforms, so that tests and log grepping is uniform.
+func buildDialErrorMessage(tlsDialErr error) string {
+	reason := tlsDialErr.Error()
+
+	var opError *net.OpError
+	var dnsError *net.DNSError
+	if errors.As(tlsDialErr, &opError) && errors.As(tlsDialErr, &dnsError) {
+		dnsError.Server = ""
+		opError.Err = dnsError
+		return opError.Error()
+	}
+
+	return reason
+}
+
+func validateUserAndGroupAttributes(upstream *v1alpha1.GitHubIdentityProvider) (v1alpha1.GitHubGroupNameAttribute, v1alpha1.GitHubUsernameAttribute, error) {
+	var groupNameAttribute v1alpha1.GitHubGroupNameAttribute
+	var usernameAttribute v1alpha1.GitHubUsernameAttribute
+
+	if upstream.Spec.Claims.Groups == nil || upstream.Spec.Claims.Username == nil {
+		return "", "", fmt.Errorf("spec.claims.groups and spec.claims.username are required")
+	} else {
+		groupNameAttribute = *upstream.Spec.Claims.Groups
+		usernameAttribute = *upstream.Spec.Claims.Username
+	}
+
+	switch usernameAttribute {
+	case v1alpha1.GitHubUsernameLoginAndID:
+	case v1alpha1.GitHubUsernameLogin:
+	case v1alpha1.GitHubUsernameID:
+	default:
+		// Should not happen due to CRD enum validation
+		return "", "", fmt.Errorf("invalid spec.claims.username (%q)", usernameAttribute)
+	}
+
+	switch groupNameAttribute {
+	case v1alpha1.GitHubUseTeamNameForGroupName:
+	case v1alpha1.GitHubUseTeamSlugForGroupName:
+	default:
+		// Should not happen due to CRD enum validation
+		return "", "", fmt.Errorf("invalid spec.claims.groups (%q)", groupNameAttribute)
+	}
+
+	return groupNameAttribute, usernameAttribute, nil
 }
 
 func (c *gitHubWatcherController) updateStatus(
 	ctx context.Context,
 	upstream *v1alpha1.GitHubIdentityProvider,
-	conditions []*metav1.Condition) error {
+	conditions []*metav1.Condition) (bool, error) {
 	log := c.log.WithValues("namespace", upstream.Namespace, "name", upstream.Name)
 	updated := upstream.DeepCopy()
 
-	hadErrorCondition := conditionsutil.MergeIDPConditions(conditions, upstream.Generation, &updated.Status.Conditions, log)
+	hadErrorCondition := conditionsutil.MergeConditions(
+		conditions,
+		upstream.Generation,
+		&updated.Status.Conditions,
+		log,
+		metav1.NewTime(c.clock.Now()),
+	)
 
 	updated.Status.Phase = v1alpha1.GitHubPhaseReady
 	if hadErrorCondition {
 		updated.Status.Phase = v1alpha1.GitHubPhaseError
 	}
 
-	if equality.Semantic.DeepEqual(upstream, updated) {
-		return nil
+	if equality.Semantic.DeepEqual(upstream, updated) { // untested
+		return hadErrorCondition, nil
 	}
 
-	_, err := c.client.
+	log.Info("updating GitHubIdentityProvider status", "phase", updated.Status.Phase)
+
+	_, updateStatusError := c.client.
 		IDPV1alpha1().
 		GitHubIdentityProviders(upstream.Namespace).
 		UpdateStatus(ctx, updated, metav1.UpdateOptions{})
-	return err
+	return hadErrorCondition, updateStatusError
 }

internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go

@@ -260,7 +260,7 @@ func (c *ldapWatcherController) updateStatus(ctx context.Context, upstream *v1al
 	log := plog.WithValues("namespace", upstream.Namespace, "name", upstream.Name)
 	updated := upstream.DeepCopy()
 
-	hadErrorCondition := conditionsutil.MergeIDPConditions(conditions, upstream.Generation, &updated.Status.Conditions, log)
+	hadErrorCondition := conditionsutil.MergeConditions(conditions, upstream.Generation, &updated.Status.Conditions, log, metav1.Now())
 
 	updated.Status.Phase = v1alpha1.LDAPPhaseReady
 	if hadErrorCondition {

internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go

@@ -1,4 +1,4 @@
-// Copyright 2022-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2022-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package oidcclientwatcher
@@ -133,7 +133,7 @@ func (c *oidcClientWatcherController) updateStatus(
 ) error {
 	updated := upstream.DeepCopy()
 
-	hadErrorCondition := conditionsutil.MergeConfigConditions(conditions,
+	hadErrorCondition := conditionsutil.MergeConditions(conditions,
 		upstream.Generation, &updated.Status.Conditions, plog.New(), metav1.Now())
 
 	updated.Status.Phase = v1alpha1.OIDCClientPhaseReady

internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go

@@ -412,7 +412,7 @@ func (c *oidcWatcherController) updateStatus(ctx context.Context, upstream *v1al
 	log := c.log.WithValues("namespace", upstream.Namespace, "name", upstream.Name)
 	updated := upstream.DeepCopy()
 
-	hadErrorCondition := conditionsutil.MergeIDPConditions(conditions, upstream.Generation, &updated.Status.Conditions, log)
+	hadErrorCondition := conditionsutil.MergeConditions(conditions, upstream.Generation, &updated.Status.Conditions, log, metav1.Now())
 
 	updated.Status.Phase = v1alpha1.PhaseReady
 	if hadErrorCondition {

internal/controller/utils.go

@@ -1,12 +1,20 @@
-// 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 controller
 
 import (
+	"crypto/x509"
+	"encoding/base64"
+	"fmt"
+	"slices"
+
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/util/cert"
 
+	authv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1"
+	idpv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1"
 	"go.pinniped.dev/internal/controllerlib"
 )
 
@@ -43,12 +51,16 @@ func SimpleFilter(match func(metav1.Object) bool, parentFunc controllerlib.Paren
 	}
 }
 
-func MatchAnySecretOfTypeFilter(secretType corev1.SecretType, parentFunc controllerlib.ParentFunc) controllerlib.Filter {
+func MatchAnySecretOfTypeFilter(secretType corev1.SecretType, parentFunc controllerlib.ParentFunc, namespaces ...string) controllerlib.Filter {
 	isSecretOfType := func(obj metav1.Object) bool {
 		secret, ok := obj.(*corev1.Secret)
 		if !ok {
 			return false
 		}
+		// Only match on namespace if namespaces are provided
+		if len(namespaces) > 0 && !slices.Contains(namespaces, secret.Namespace) {
+			return false
+		}
 		return secret.Type == secretType
 	}
 	return SimpleFilter(isSecretOfType, parentFunc)
@@ -87,3 +99,43 @@ type WithInformerOptionFunc func(
 
 // Same signature as controllerlib.WithInitialEvent().
 type WithInitialEventOptionFunc func(key controllerlib.Key) controllerlib.Option
+
+// BuildCertPoolAuth returns a PEM-encoded CA bundle from the provided spec. If the provided spec is nil, a
+// nil CA bundle will be returned. If the provided spec contains a CA bundle that is not properly
+// encoded, an error will be returned.
+func BuildCertPoolAuth(spec *authv1alpha1.TLSSpec) (*x509.CertPool, []byte, error) {
+	if spec == nil {
+		return nil, nil, nil
+	}
+
+	return buildCertPool(spec.CertificateAuthorityData)
+}
+
+// BuildCertPoolIDP returns a PEM-encoded CA bundle from the provided spec. If the provided spec is nil, a
+// nil CA bundle will be returned. If the provided spec contains a CA bundle that is not properly
+// encoded, an error will be returned.
+func BuildCertPoolIDP(spec *idpv1alpha1.TLSSpec) (*x509.CertPool, []byte, error) {
+	if spec == nil {
+		return nil, nil, nil
+	}
+
+	return buildCertPool(spec.CertificateAuthorityData)
+}
+
+func buildCertPool(certificateAuthorityData string) (*x509.CertPool, []byte, error) {
+	if len(certificateAuthorityData) == 0 {
+		return nil, nil, nil
+	}
+
+	pem, err := base64.StdEncoding.DecodeString(certificateAuthorityData)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	rootCAs, err := cert.NewPoolFromBytes(pem)
+	if err != nil {
+		return nil, nil, fmt.Errorf("certificateAuthorityData is not valid PEM: %w", err)
+	}
+
+	return rootCAs, pem, nil
+}