Merge branch 'main' into upstream-oidc-refresh-groups

ROADMAP.md

@@ -40,7 +40,9 @@ Last Updated: Sept 2021
 |Improving Security Posture|Supervisor token refresh fails when the upstream user is in an invalid state for LDAP/AD |Jan 2022|
 |Improving Security Posture|Set stricter default TLS versions and Ciphers |Jan 2022|
 |Improving Security Posture|Support FIPS compliant Boring crypto libraries |Feb 2022|
-|Improving Security Posture|Support Audit logging of security events related to Authentication |March 2022|
+|Multiple IDP support|Support multiple IDPs configured on a single Supervisor|March/April 2022|
+|Improving Security Posture|TLS hardening |March/April 2022|
+|Improving Security Posture|Support Audit logging of security events related to Authentication |April/May 2022|
 |Improving Security Posture|mTLS for Supervisor sessions |Exploring/Ongoing|
 |Improving Security Posture|Key management/rotation for Pinniped components with minimal downtime |Exploring/Ongoing|
 |Improving Security Posture|Support for Session Logout |Exploring/Ongoing|
@@ -52,7 +54,6 @@ Last Updated: Sept 2021
 |Improving Security Posture|Client side use of Secure Enclaves for Session data |Exploring/Ongoing|
 |Improving Security Posture|Enforce the use of HTTP Strict Transport (HSTS) |Exploring/Ongoing|
 |Improving Security Posture|Assert that Pinniped runs under the restricted PSP version2 levels  |Exploring/Ongoing|
-|Multiple IDP support|Support multiple IDPs configured on a single Supervisor|Exploring/Ongoing|
 |Wider Concierge cluster support|Support for OpenShift cluster types in the Concierge|Exploring/Ongoing|
 |Identity transforms|Support prefixing, filtering, or performing coarse-grained checks on upstream users and groups|Exploring/Ongoing|
 |CLI SSO|Support Kerberos based authentication on CLI |Exploring/Ongoing|

internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go

@@ -226,7 +226,7 @@ type UpstreamActiveDirectoryIdentityProviderICache interface {
 
 type activeDirectoryWatcherController struct {
 	cache                                   UpstreamActiveDirectoryIdentityProviderICache
-	validatedSecretVersionsCache            upstreamwatchers.SecretVersionCacheI
+	validatedSettingsCache                  upstreamwatchers.ValidatedSettingsCacheI
 	ldapDialer                              upstreamldap.LDAPDialer
 	client                                  pinnipedclientset.Interface
 	activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer
@@ -243,8 +243,8 @@ func New(
 ) controllerlib.Controller {
 	return newInternal(
 		idpCache,
-		// start with an empty secretVersionCache
+		// start with an empty cache
-		upstreamwatchers.NewSecretVersionCache(),
+		upstreamwatchers.NewValidatedSettingsCache(),
 		// nil means to use a real production dialer when creating objects to add to the cache
 		nil,
 		client,
@@ -257,7 +257,7 @@ func New(
 // For test dependency injection purposes.
 func newInternal(
 	idpCache UpstreamActiveDirectoryIdentityProviderICache,
-	validatedSecretVersionsCache upstreamwatchers.SecretVersionCacheI,
+	validatedSettingsCache upstreamwatchers.ValidatedSettingsCacheI,
 	ldapDialer upstreamldap.LDAPDialer,
 	client pinnipedclientset.Interface,
 	activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer,
@@ -266,7 +266,7 @@ func newInternal(
 ) controllerlib.Controller {
 	c := activeDirectoryWatcherController{
 		cache:                                   idpCache,
-		validatedSecretVersionsCache:            validatedSecretVersionsCache,
+		validatedSettingsCache:                  validatedSettingsCache,
 		ldapDialer:                              ldapDialer,
 		client:                                  client,
 		activeDirectoryIdentityProviderInformer: activeDirectoryIdentityProviderInformer,
@@ -351,7 +351,7 @@ func (c *activeDirectoryWatcherController) validateUpstream(ctx context.Context,
 		}
 	}
 
-	conditions := upstreamwatchers.ValidateGenericLDAP(ctx, adUpstreamImpl, c.secretInformer, c.validatedSecretVersionsCache, config)
+	conditions := upstreamwatchers.ValidateGenericLDAP(ctx, adUpstreamImpl, c.secretInformer, c.validatedSettingsCache, config)
 
 	c.updateStatus(ctx, upstream, conditions.Conditions())
 

internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go

@@ -150,8 +150,8 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 
 	const (
 		testNamespace         = "test-namespace"
-		testResourceUID       = "test-uid"
 		testName              = "test-name"
+		testResourceUID       = "test-uid"
 		testSecretName        = "test-bind-secret"
 		testBindUsername      = "test-bind-username"
 		testBindPassword      = "test-bind-password"
@@ -255,6 +255,19 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 			ObservedGeneration: gen,
 		}
 	}
+	activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration := func(secretVersion string) v1alpha1.Condition {
+		c := activeDirectoryConnectionValidTrueCondition(0, secretVersion)
+		c.LastTransitionTime = metav1.Time{}
+		return c
+	}
+	condPtr := func(c v1alpha1.Condition) *v1alpha1.Condition {
+		return &c
+	}
+	withoutTime := func(c v1alpha1.Condition) v1alpha1.Condition {
+		c = *c.DeepCopy()
+		c.LastTransitionTime = metav1.Time{}
+		return c
+	}
 	tlsConfigurationValidLoadedTrueCondition := func(gen int64) v1alpha1.Condition {
 		return v1alpha1.Condition{
 			Type:               "TLSConfigurationValid",
@@ -377,7 +390,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					Conditions: allConditionsTrue(1234, "4242"),
 				},
 			}},
-			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
+			}},
 		},
 		{
 			name:               "missing secret",
@@ -568,7 +589,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					},
 				},
 			}},
-			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
+			}},
 		},
 		{
 			name: "sAMAccountName explicitly provided as group name attribute does not add an override",
@@ -629,7 +658,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					},
 				},
 			}},
-			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
+			}},
 		},
 		{
 			name: "when TLS connection fails it tries to use StartTLS instead: without a specified port it automatically switches ports",
@@ -695,7 +732,22 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					},
 				},
 			}},
-			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.StartTLS,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition: &v1alpha1.Condition{
+					Type:   "LDAPConnectionValid",
+					Status: "True",
+					Reason: "Success",
+					Message: fmt.Sprintf(
+						`successfully able to connect to "%s" and bind as user "%s" [validated with Secret "%s" at version "%s"]`,
+						"ldap.example.com", testBindUsername, testSecretName, "4242"),
+				},
+				SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
+			}},
 		},
 		{
 			name: "when TLS connection fails it tries to use StartTLS instead: with a specified port it does not automatically switch ports",
@@ -808,7 +860,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					Conditions: allConditionsTrue(1234, "4242"),
 				},
 			}},
-			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
+			}},
 		},
 		{
 			name: "one valid upstream and one invalid upstream updates the cache to include only the valid upstream",
@@ -852,7 +912,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					},
 				},
 			},
-			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
+			}},
 		},
 		{
 			name: "when testing the connection to the LDAP server fails then the upstream is still added to the cache anyway but not to validatedsettings (treated like a warning)",
@@ -1004,7 +1072,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				}
 			})},
 			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
-			initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}},
+			initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
+			}},
 			setupMocks: func(conn *mockldapconn.MockConn) {
 				// Should not perform a test dial and bind. No mocking here means the test will fail if Bind() or Close() are called.
 			},
@@ -1016,7 +1092,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					Conditions: allConditionsTrue(1234, "4242"),
 				},
 			}},
-			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
+			}},
 		},
 		{
 			name: "when the validated cache contains LDAP server info but the search base is empty, reload everything",
@@ -1030,7 +1114,11 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				upstream.Spec.UserSearch.Base = ""
 			})},
 			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
-			initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, Generation: 1234}},
+			initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				IDPSpecGeneration:         1234,
+			}},
 			setupMocks: func(conn *mockldapconn.MockConn) {
 				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2)
 				conn.EXPECT().Close().Times(2)
@@ -1075,7 +1163,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					},
 				},
 			}},
-			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase, Generation: 1234}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            exampleDefaultNamingContext,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))),
+			}},
 		},
 		{
 			name: "when the LDAP server connection was already validated using TLS, and the search base was found, load TLS and search base info into the cache",
@@ -1088,7 +1184,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				upstream.Spec.UserSearch.Base = ""
 			})},
 			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
-			initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase, Generation: 1234}},
+			initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            exampleDefaultNamingContext,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))),
+			}},
 			setupMocks: func(conn *mockldapconn.MockConn) {
 			},
 			wantResultingCache: []*upstreamldap.ProviderConfig{
@@ -1136,7 +1240,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            exampleDefaultNamingContext,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))),
 			}},
 		},
 		{
@@ -1149,7 +1255,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				}
 			})},
 			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
-			initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS, Generation: 1234, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}},
+			initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.StartTLS,
+				IDPSpecGeneration:         1234,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
+			}},
 			setupMocks: func(conn *mockldapconn.MockConn) {
 				// Should not perform a test dial and bind. No mocking here means the test will fail if Bind() or Close() are called.
 			},
@@ -1166,7 +1280,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.StartTLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
 			}},
 		},
 		{
@@ -1183,7 +1299,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1233,
+				IDPSpecGeneration:         1233,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
 			}},
 			setupMocks: func(conn *mockldapconn.MockConn) {
 				// Should perform a test dial and bind.
@@ -1203,7 +1321,49 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
+			}},
+		},
+		{
+			name: "when the LDAP server connection condition failed to update previously, then write the cached condition from the previous connection validation",
+			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) {
+				upstream.Generation = 1234 // current generation
+				upstream.Status.Conditions = []v1alpha1.Condition{
+					activeDirectoryConnectionValidTrueCondition(1234, "4200"), // old version of the condition, as if the previous update of conditions had failed
+				}
+			})},
+			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
+			initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				IDPSpecGeneration:         1234,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), // already previously validated with version 4242
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
+			}},
+			setupMocks: func(conn *mockldapconn.MockConn) {
+				// The connection had already been validated previously and the result was cached, so don't probe the server again.
+				// Should not perform a test dial and bind. No mocking here means the test will fail if Bind() or Close() are called.
+			},
+			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS},
+			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, UID: testResourceUID, Generation: 1234},
+				Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{
+					Phase:      "Ready",
+					Conditions: allConditionsTrue(1234, "4242"), // updated version of the condition using the cached condition value
+				},
+			}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
 			}},
 		},
 		{
@@ -1222,7 +1382,6 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				}
 			})},
 			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
-			initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "1", LDAPConnectionProtocol: upstreamldap.TLS, Generation: 1234}},
 			setupMocks: func(conn *mockldapconn.MockConn) {
 				// Should perform a test dial and bind.
 				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
@@ -1241,7 +1400,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
 			}},
 		},
 		{
@@ -1258,7 +1419,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4241")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
 			}}, // old version was validated
 			setupMocks: func(conn *mockldapconn.MockConn) {
 				// Should perform a test dial and bind.
@@ -1273,12 +1436,14 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					Conditions: allConditionsTrue(1234, "4242"),
 				},
 			}},
-			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
-				testName: {BindSecretResourceVersion: "4242",
+				BindSecretResourceVersion: "4242",
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-					Generation:             1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
 			}},
 		},
 		{
@@ -1336,7 +1501,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInConfigCondition(0))),
 			}},
 		},
 		{
@@ -1398,7 +1565,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            exampleDefaultNamingContext,
 				GroupSearchBase:           exampleDefaultNamingContext,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))),
 			}},
 		},
 		{
@@ -1454,7 +1623,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					},
 				},
 			}},
-			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase, Generation: 1234}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            exampleDefaultNamingContext,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))),
+			}},
 		},
 		{
 			name: "when the input activedirectoryidentityprovider leaves group search base blank but provides user search base, query for defaultNamingContext",
@@ -1509,7 +1686,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					},
 				},
 			}},
-			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: exampleDefaultNamingContext, Generation: 1234}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           exampleDefaultNamingContext,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))),
+			}},
 		},
 		{
 			name: "when the input activedirectoryidentityprovider leaves group search base blank and query for defaultNamingContext fails",
@@ -1662,7 +1847,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4241")),
+				SearchBaseFoundCondition:  condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))),
 			}},
 			setupMocks: func(conn *mockldapconn.MockConn) {
 				// Should perform a test dial and bind.
@@ -1715,7 +1902,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					LDAPConnectionProtocol:   upstreamldap.TLS,
 					GroupSearchBase:          exampleDefaultNamingContext,
 					UserSearchBase:           testUserSearchBase,
-					Generation:             1234,
+					IDPSpecGeneration:        1234,
+					ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+					SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))),
 				}},
 		},
 	}
@@ -1752,20 +1941,20 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 				return conn, nil
 			})}
 
-			var validatedSecretVersionCache *upstreamwatchers.SecretVersionCache
+			var validatedSettingsCache *upstreamwatchers.ValidatedSettingsCache
 			if tt.initialValidatedSettings != nil {
-				validatedSecretVersionCache = &upstreamwatchers.SecretVersionCache{
+				validatedSettingsCache = &upstreamwatchers.ValidatedSettingsCache{
 					ValidatedSettingsByName: tt.initialValidatedSettings,
 				}
 			} else {
-				validatedSecretVersionCache = &upstreamwatchers.SecretVersionCache{
+				validatedSettingsCache = &upstreamwatchers.ValidatedSettingsCache{
 					ValidatedSettingsByName: map[string]upstreamwatchers.ValidatedSettings{},
 				}
 			}
 
 			controller := newInternal(
 				cache,
-				validatedSecretVersionCache,
+				validatedSettingsCache,
 				dialer,
 				fakePinnipedClient,
 				pinnipedInformers.IDP().V1alpha1().ActiveDirectoryIdentityProviders(),
@@ -1850,7 +2039,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 			if tt.wantValidatedSettings == nil {
 				tt.wantValidatedSettings = map[string]upstreamwatchers.ValidatedSettings{}
 			}
-			require.Equal(t, tt.wantValidatedSettings, validatedSecretVersionCache.ValidatedSettingsByName)
+			require.Equal(t, tt.wantValidatedSettings, validatedSettingsCache.ValidatedSettingsByName)
 		})
 	}
 }

internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package ldapupstreamwatcher implements a controller which watches LDAPIdentityProviders.
@@ -134,7 +134,7 @@ type UpstreamLDAPIdentityProviderICache interface {
 
 type ldapWatcherController struct {
 	cache                        UpstreamLDAPIdentityProviderICache
-	validatedSecretVersionsCache upstreamwatchers.SecretVersionCacheI
+	validatedSettingsCache       upstreamwatchers.ValidatedSettingsCacheI
 	ldapDialer                   upstreamldap.LDAPDialer
 	client                       pinnipedclientset.Interface
 	ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer
@@ -151,8 +151,8 @@ func New(
 ) controllerlib.Controller {
 	return newInternal(
 		idpCache,
-		// start with an empty secretVersionCache
+		// start with an empty cache
-		upstreamwatchers.NewSecretVersionCache(),
+		upstreamwatchers.NewValidatedSettingsCache(),
 		// nil means to use a real production dialer when creating objects to add to the cache
 		nil,
 		client,
@@ -165,7 +165,7 @@ func New(
 // For test dependency injection purposes.
 func newInternal(
 	idpCache UpstreamLDAPIdentityProviderICache,
-	validatedSecretVersionsCache upstreamwatchers.SecretVersionCacheI,
+	validatedSettingsCache upstreamwatchers.ValidatedSettingsCacheI,
 	ldapDialer upstreamldap.LDAPDialer,
 	client pinnipedclientset.Interface,
 	ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer,
@@ -174,7 +174,7 @@ func newInternal(
 ) controllerlib.Controller {
 	c := ldapWatcherController{
 		cache:                        idpCache,
-		validatedSecretVersionsCache: validatedSecretVersionsCache,
+		validatedSettingsCache:       validatedSettingsCache,
 		ldapDialer:                   ldapDialer,
 		client:                       client,
 		ldapIdentityProviderInformer: ldapIdentityProviderInformer,
@@ -243,7 +243,7 @@ func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream *
 		Dialer: c.ldapDialer,
 	}
 
-	conditions := upstreamwatchers.ValidateGenericLDAP(ctx, &ldapUpstreamGenericLDAPImpl{*upstream}, c.secretInformer, c.validatedSecretVersionsCache, config)
+	conditions := upstreamwatchers.ValidateGenericLDAP(ctx, &ldapUpstreamGenericLDAPImpl{*upstream}, c.secretInformer, c.validatedSettingsCache, config)
 
 	c.updateStatus(ctx, upstream, conditions.Conditions())
 

internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package ldapupstreamwatcher
@@ -253,6 +253,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 			ObservedGeneration: gen,
 		}
 	}
+	ldapConnectionValidTrueConditionWithoutTimeOrGeneration := func(secretVersion string) v1alpha1.Condition {
+		c := ldapConnectionValidTrueCondition(0, secretVersion)
+		c.LastTransitionTime = metav1.Time{}
+		return c
+	}
+	condPtr := func(c v1alpha1.Condition) *v1alpha1.Condition {
+		return &c
+	}
 	tlsConfigurationValidLoadedTrueCondition := func(gen int64) v1alpha1.Condition {
 		return v1alpha1.Condition{
 			Type:               "TLSConfigurationValid",
@@ -317,7 +325,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
 			}},
 		},
 		{
@@ -507,8 +516,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
-			}}},
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+			}},
+		},
 		{
 			name: "when TLS connection fails it tries to use StartTLS instead: without a specified port it automatically switches ports",
 			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
@@ -571,8 +582,17 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.StartTLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
-			}}},
+				ConnectionValidCondition: &v1alpha1.Condition{
+					Type:   "LDAPConnectionValid",
+					Status: "True",
+					Reason: "Success",
+					Message: fmt.Sprintf(
+						`successfully able to connect to "%s" and bind as user "%s" [validated with Secret "%s" at version "%s"]`,
+						"ldap.example.com", testBindUsername, testSecretName, "4242"),
+				},
+			}},
+		},
 		{
 			name: "when TLS connection fails it tries to use StartTLS instead: with a specified port it does not automatically switch ports",
 			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
@@ -676,7 +696,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
 			}},
 		},
 		{
@@ -726,8 +747,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
-			}}},
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+			}},
+		},
 		{
 			name:           "when testing the connection to the LDAP server fails then the upstream is still added to the cache anyway (treated like a warning) but not the validated settings cache",
 			inputUpstreams: []runtime.Object{validUpstream},
@@ -776,7 +799,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 					LDAPConnectionProtocol:   upstreamldap.TLS,
 					UserSearchBase:           testUserSearchBase,
 					GroupSearchBase:          testGroupSearchBase,
-					Generation:             1234,
+					IDPSpecGeneration:        1234,
+					ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
 				}},
 			setupMocks: func(conn *mockldapconn.MockConn) {
 				// Should not perform a test dial and bind. No mocking here means the test will fail if Bind() or Close() are called.
@@ -794,8 +818,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
-			}}},
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+			}},
+		},
 		{
 			name: "when the LDAP server connection was already validated using StartTLS for the current resource generation and secret version, then do not validate it again and keep using StartTLS",
 			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
@@ -810,7 +836,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.StartTLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
 			}},
 			setupMocks: func(conn *mockldapconn.MockConn) {
 				// Should not perform a test dial and bind. No mocking here means the test will fail if Bind() or Close() are called.
@@ -828,8 +855,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.StartTLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
-			}}},
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+			}},
+		},
 		{
 			name: "when the LDAP server connection was validated for an older resource generation, then try to validate it again",
 			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
@@ -842,7 +871,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 			initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
 				BindSecretResourceVersion: "4242",
 				LDAPConnectionProtocol:    upstreamldap.TLS,
-				Generation:                1233,
+				IDPSpecGeneration:         1233,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
 			}},
@@ -864,8 +893,48 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
-			}}},
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+			}},
+		},
+		{
+			name: "when the LDAP server connection condition failed to update previously, then write the cached condition from the previous connection validation",
+			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
+				upstream.Generation = 1234 // current generation
+				upstream.Status.Conditions = []v1alpha1.Condition{
+					ldapConnectionValidTrueCondition(1234, "4200"), // old version of the condition, as if the previous update of conditions had failed
+				}
+			})},
+			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
+			initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				IDPSpecGeneration:         1234,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), // already previously validated with version 4242
+			}},
+			setupMocks: func(conn *mockldapconn.MockConn) {
+				// The connection had already been validated previously and the result was cached, so don't probe the server again.
+				// Should not perform a test dial and bind. No mocking here means the test will fail if Bind() or Close() are called.
+			},
+			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS},
+			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testResourceUID},
+				Status: v1alpha1.LDAPIdentityProviderStatus{
+					Phase:      "Ready",
+					Conditions: allConditionsTrue(1234, "4242"), // updated version of the condition using the cached condition value
+				},
+			}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {
+				BindSecretResourceVersion: "4242",
+				LDAPConnectionProtocol:    upstreamldap.TLS,
+				UserSearchBase:            testUserSearchBase,
+				GroupSearchBase:           testGroupSearchBase,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+			}},
+		},
 		{
 			name: "when the LDAP server connection validation previously failed for this resource generation, then try to validate it again",
 			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
@@ -900,8 +969,11 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
-			}}}, {
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
+			}},
+		},
+		{
 			name: "when the validated settings cache is incomplete, then try to validate it again",
 			// this shouldn't happen, but if it does, just throw it out and try again.
 			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
@@ -940,7 +1012,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
 			}},
 		},
 		{
@@ -957,7 +1030,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
 			}}, // old version was validated
 			setupMocks: func(conn *mockldapconn.MockConn) {
 				// Should perform a test dial and bind.
@@ -977,7 +1050,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				LDAPConnectionProtocol:    upstreamldap.TLS,
 				UserSearchBase:            testUserSearchBase,
 				GroupSearchBase:           testGroupSearchBase,
-				Generation:                1234,
+				IDPSpecGeneration:         1234,
+				ConnectionValidCondition:  condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")),
 			}}},
 	}
 
@@ -1013,20 +1087,20 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				return conn, nil
 			})}
 
-			var validatedSecretVersionCache *upstreamwatchers.SecretVersionCache
+			var validatedSettingsCache *upstreamwatchers.ValidatedSettingsCache
 			if tt.initialValidatedSettings != nil {
-				validatedSecretVersionCache = &upstreamwatchers.SecretVersionCache{
+				validatedSettingsCache = &upstreamwatchers.ValidatedSettingsCache{
 					ValidatedSettingsByName: tt.initialValidatedSettings,
 				}
 			} else {
-				validatedSecretVersionCache = &upstreamwatchers.SecretVersionCache{
+				validatedSettingsCache = &upstreamwatchers.ValidatedSettingsCache{
 					ValidatedSettingsByName: map[string]upstreamwatchers.ValidatedSettings{},
 				}
 			}
 
 			controller := newInternal(
 				cache,
-				validatedSecretVersionCache,
+				validatedSettingsCache,
 				dialer,
 				fakePinnipedClient,
 				pinnipedInformers.IDP().V1alpha1().LDAPIdentityProviders(),
@@ -1075,7 +1149,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 			if tt.wantValidatedSettings == nil {
 				tt.wantValidatedSettings = map[string]upstreamwatchers.ValidatedSettings{}
 			}
-			require.Equal(t, tt.wantValidatedSettings, validatedSecretVersionCache.ValidatedSettingsByName)
+			require.Equal(t, tt.wantValidatedSettings, validatedSettingsCache.ValidatedSettingsByName)
 		})
 	}
 }

internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package upstreamwatchers
@@ -44,45 +44,61 @@ const (
 	ReasonErrorFetchingSearchBase    = "ErrorFetchingSearchBase"
 )
 
-// An in-memory cache with an entry for each ActiveDirectoryIdentityProvider, to keep track of which ResourceVersion
+// ValidatedSettings is the struct which is cached by the ValidatedSettingsCacheI interface.
-// of the bind Secret, which TLS/StartTLS setting was used and which search base was found during the most recent successful validation.
+type ValidatedSettings struct {
-type SecretVersionCacheI interface {
+	IDPSpecGeneration         int64  // which IDP spec was used during the validation
-	Get(upstreamName, resourceVersion string, generation int64) (ValidatedSettings, bool)
+	BindSecretResourceVersion string // which bind secret was used during the validation
-	Set(upstreamName, resourceVersion string, generation int64, settings ValidatedSettings)
+
+	// Cache the setting for TLS vs StartTLS. This is always auto-discovered by probing the server.
+	LDAPConnectionProtocol upstreamldap.LDAPConnectionProtocol
+
+	// Cache the settings for search bases. These could be configured by the IDP spec, or in the
+	// case of AD they can also be auto-discovered by probing the server.
+	UserSearchBase, GroupSearchBase string
+
+	// Cache copies of the conditions that were computed when the above settings were cached, so we
+	// can keep writing them to the status in the future. This matters most when the first attempt
+	// to write them to the IDP's status fails. In this case, future Syncs calls will be able to
+	// use these cached values to try writing them again.
+	ConnectionValidCondition, SearchBaseFoundCondition *v1alpha1.Condition
 }
 
-type SecretVersionCache struct {
+// ValidatedSettingsCacheI is an interface for an in-memory cache with an entry for each upstream
+// provider. It keeps track of settings that were already validated for a given IDP spec and bind
+// secret for that upstream.
+type ValidatedSettingsCacheI interface {
+	// Get the cached settings for a given upstream at a given generation which was previously
+	// validated using a given bind secret version. If no settings have been cached for the
+	// upstream, or if the settings were cached at a different generation of the upstream or
+	// using a different version of the bind secret, then return false to indicate that the
+	// desired settings were not cached yet for that combination of spec generation and secret version.
+	Get(upstreamName, resourceVersion string, idpSpecGeneration int64) (ValidatedSettings, bool)
+
+	// Set some settings into the cache for a given upstream.
+	Set(upstreamName string, settings ValidatedSettings)
+}
+
+type ValidatedSettingsCache struct {
 	ValidatedSettingsByName map[string]ValidatedSettings
 }
 
-func (s *SecretVersionCache) Get(upstreamName, resourceVersion string, generation int64) (ValidatedSettings, bool) {
+func NewValidatedSettingsCache() ValidatedSettingsCacheI {
-	validatedSettings := s.ValidatedSettingsByName[upstreamName]
+	return &ValidatedSettingsCache{ValidatedSettingsByName: map[string]ValidatedSettings{}}
-	if validatedSettings.BindSecretResourceVersion == resourceVersion &&
+}
-		validatedSettings.Generation == generation && validatedSettings.UserSearchBase != "" &&
+
-		validatedSettings.GroupSearchBase != "" && validatedSettings.LDAPConnectionProtocol != "" {
+func (s *ValidatedSettingsCache) Get(upstreamName, resourceVersion string, idpSpecGeneration int64) (ValidatedSettings, bool) {
+	validatedSettings, found := s.ValidatedSettingsByName[upstreamName]
+	if found && validatedSettings.BindSecretResourceVersion == resourceVersion && validatedSettings.IDPSpecGeneration == idpSpecGeneration {
 		return validatedSettings, true
 	}
 	return ValidatedSettings{}, false
 }
 
-func (s *SecretVersionCache) Set(upstreamName, resourceVersion string, generation int64, settings ValidatedSettings) {
+func (s *ValidatedSettingsCache) Set(upstreamName string, settings ValidatedSettings) {
 	s.ValidatedSettingsByName[upstreamName] = settings
 }
 
-type ValidatedSettings struct {
+// UpstreamGenericLDAPIDP is a read-only interface for abstracting the differences between LDAP and Active Directory IDP types.
-	Generation                int64
-	BindSecretResourceVersion string
-	LDAPConnectionProtocol    upstreamldap.LDAPConnectionProtocol
-	UserSearchBase            string
-	GroupSearchBase           string
-}
-
-func NewSecretVersionCache() SecretVersionCacheI {
-	cache := SecretVersionCache{ValidatedSettingsByName: map[string]ValidatedSettings{}}
-	return &cache
-}
-
-// read only interface for sharing between ldap and active directory.
 type UpstreamGenericLDAPIDP interface {
 	Spec() UpstreamGenericLDAPSpec
 	Name() string
@@ -247,8 +263,15 @@ func ValidateSecret(secretInformer corev1informers.SecretInformer, secretName st
 	}, secret.ResourceVersion
 }
 
+// gradatedCondition is a condition and a boolean that tells you whether the condition is fatal or just a warning.
+type gradatedCondition struct {
+	condition *v1alpha1.Condition
+	isFatal   bool
+}
+
+// GradatedConditions is a list of conditions, where each condition can additionally be considered fatal or non-fatal.
 type GradatedConditions struct {
-	gradatedConditions []GradatedCondition
+	gradatedConditions []gradatedCondition
 }
 
 func (g *GradatedConditions) Conditions() []*v1alpha1.Condition {
@@ -260,76 +283,82 @@ func (g *GradatedConditions) Conditions() []*v1alpha1.Condition {
 }
 
 func (g *GradatedConditions) Append(condition *v1alpha1.Condition, isFatal bool) {
-	g.gradatedConditions = append(g.gradatedConditions, GradatedCondition{condition: condition, isFatal: isFatal})
+	g.gradatedConditions = append(g.gradatedConditions, gradatedCondition{condition: condition, isFatal: isFatal})
 }
 
-// A condition and a boolean that tells you whether it's fatal or just a warning.
+func ValidateGenericLDAP(
-type GradatedCondition struct {
+	ctx context.Context,
-	condition *v1alpha1.Condition
+	upstream UpstreamGenericLDAPIDP,
-	isFatal   bool
+	secretInformer corev1informers.SecretInformer,
-}
+	validatedSettingsCache ValidatedSettingsCacheI,
-
+	config *upstreamldap.ProviderConfig,
-func ValidateGenericLDAP(ctx context.Context, upstream UpstreamGenericLDAPIDP, secretInformer corev1informers.SecretInformer, validatedSecretVersionsCache SecretVersionCacheI, config *upstreamldap.ProviderConfig) GradatedConditions {
+) GradatedConditions {
 	conditions := GradatedConditions{}
+
 	secretValidCondition, currentSecretVersion := ValidateSecret(secretInformer, upstream.Spec().BindSecretName(), upstream.Namespace(), config)
 	conditions.Append(secretValidCondition, true)
+
 	tlsValidCondition := ValidateTLSConfig(upstream.Spec().TLSSpec(), config)
 	conditions.Append(tlsValidCondition, true)
 
+	var ldapConnectionValidCondition, searchBaseFoundCondition *v1alpha1.Condition
 	// No point in trying to connect to the server if the config was already determined to be invalid.
-	var ldapConnectionValidCondition *v1alpha1.Condition
-	var searchBaseFoundCondition *v1alpha1.Condition
 	if secretValidCondition.Status == v1alpha1.ConditionTrue && tlsValidCondition.Status == v1alpha1.ConditionTrue {
-		ldapConnectionValidCondition, searchBaseFoundCondition = validateAndSetLDAPServerConnectivityAndSearchBase(ctx, validatedSecretVersionsCache, upstream, config, currentSecretVersion)
+		ldapConnectionValidCondition, searchBaseFoundCondition = validateAndSetLDAPServerConnectivityAndSearchBase(ctx, validatedSettingsCache, upstream, config, currentSecretVersion)
-		if ldapConnectionValidCondition != nil {
 		conditions.Append(ldapConnectionValidCondition, false)
-		}
+		if searchBaseFoundCondition != nil { // currently, only used for AD, so may be nil
-		if searchBaseFoundCondition != nil {
 			conditions.Append(searchBaseFoundCondition, true)
 		}
 	}
 	return conditions
 }
 
-func validateAndSetLDAPServerConnectivityAndSearchBase(ctx context.Context, validatedSecretVersionsCache SecretVersionCacheI, upstream UpstreamGenericLDAPIDP, config *upstreamldap.ProviderConfig, currentSecretVersion string) (*v1alpha1.Condition, *v1alpha1.Condition) {
+func validateAndSetLDAPServerConnectivityAndSearchBase(
-	// previouslyValidatedSecretVersion := validatedSecretVersionsCache.ValidatedSettingsByName[upstream.Name()].BindSecretResourceVersion
+	ctx context.Context,
-	// doesn't have an existing entry for ValidatedSettingsByName with this secret version ->
+	validatedSettingsCache ValidatedSettingsCacheI,
-	// lets double check tls connection
+	upstream UpstreamGenericLDAPIDP,
-	// if we can connect, put it in the secret cache
+	config *upstreamldap.ProviderConfig,
-	// also we KNOW we need to recheck the search base stuff too... so they should all be one function?
+	currentSecretVersion string,
-	// but if tls validation fails no need to also try to get search base stuff?
+) (*v1alpha1.Condition, *v1alpha1.Condition) {
-
+	validatedSettings, hasPreviousValidatedSettings := validatedSettingsCache.Get(upstream.Name(), currentSecretVersion, upstream.Generation())
-	validatedSettings, hasPreviousValidatedSettings := validatedSecretVersionsCache.Get(upstream.Name(), currentSecretVersion, upstream.Generation())
 	var ldapConnectionValidCondition, searchBaseFoundCondition *v1alpha1.Condition
-	if !hasPreviousValidatedSettings {
+
+	if hasPreviousValidatedSettings && validatedSettings.UserSearchBase != "" && validatedSettings.GroupSearchBase != "" {
+		// Found previously validated settings in the cache (which is also not missing search base fields), so use them.
+		config.ConnectionProtocol = validatedSettings.LDAPConnectionProtocol
+		config.UserSearch.Base = validatedSettings.UserSearchBase
+		config.GroupSearch.Base = validatedSettings.GroupSearchBase
+		ldapConnectionValidCondition = validatedSettings.ConnectionValidCondition.DeepCopy()
+		searchBaseFoundCondition = validatedSettings.SearchBaseFoundCondition.DeepCopy()
+	} else {
+		// Did not find previously validated settings in the cache, so probe the LDAP server.
 		testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, probeLDAPTimeout)
 		defer cancelFunc()
-
 		ldapConnectionValidCondition = TestConnection(testConnectionTimeout, upstream.Spec().BindSecretName(), config, currentSecretVersion)
 
 		searchBaseTimeout, cancelFunc := context.WithTimeout(ctx, probeLDAPTimeout)
 		defer cancelFunc()
 		searchBaseFoundCondition = upstream.Spec().DetectAndSetSearchBase(searchBaseTimeout, config)
 
-		if ldapConnectionValidCondition.Status == v1alpha1.ConditionTrue {
+		// When there were no failures, write the newly validated settings to the cache.
-			// if it's nil, don't worry about the search base condition. But if it exists make sure the status is true.
+		// It's okay for the search base condition to be nil, since it's only used by Active Directory providers,
-			if searchBaseFoundCondition == nil || (searchBaseFoundCondition.Status == v1alpha1.ConditionTrue) {
+		// but if it exists make sure it was not a failure.
-				// Remember (in-memory for this pod) that the controller has successfully validated the LDAP provider
+		if ldapConnectionValidCondition.Status == v1alpha1.ConditionTrue &&
+			(searchBaseFoundCondition == nil || (searchBaseFoundCondition.Status == v1alpha1.ConditionTrue)) {
+			// Remember (in-memory for this pod) that the controller has successfully validated the LDAP or AD provider
 			// using this version of the Secret. This is for performance reasons, to avoid attempting to connect to
 			// the LDAP server more than is needed. If the pod restarts, it will attempt this validation again.
-				validatedSettings.LDAPConnectionProtocol = config.ConnectionProtocol
+			validatedSettingsCache.Set(upstream.Name(), ValidatedSettings{
-				validatedSettings.BindSecretResourceVersion = currentSecretVersion
+				IDPSpecGeneration:         upstream.Generation(),
-				validatedSettings.Generation = upstream.Generation()
+				BindSecretResourceVersion: currentSecretVersion,
-				validatedSettings.UserSearchBase = config.UserSearch.Base
+				LDAPConnectionProtocol:    config.ConnectionProtocol,
-				validatedSettings.GroupSearchBase = config.GroupSearch.Base
+				UserSearchBase:            config.UserSearch.Base,
-				validatedSecretVersionsCache.Set(upstream.Name(), currentSecretVersion, upstream.Generation(), validatedSettings)
+				GroupSearchBase:           config.GroupSearch.Base,
+				ConnectionValidCondition:  ldapConnectionValidCondition.DeepCopy(),
+				SearchBaseFoundCondition:  searchBaseFoundCondition.DeepCopy(), // currently, only used for AD, so may be nil
+			})
 		}
 	}
-	} else {
-		config.ConnectionProtocol = validatedSettings.LDAPConnectionProtocol
-		config.UserSearch.Base = validatedSettings.UserSearchBase
-		config.GroupSearch.Base = validatedSettings.GroupSearchBase
-	}
 
 	return ldapConnectionValidCondition, searchBaseFoundCondition
 }