Only test the server connection when the spec has changed

This early version of the controller is not intended to act as an
ongoing health check for your upstream LDAP server. It will connect
to the LDAP server to essentially "lint" your configuration once.
It will do it again only when you change your configuration. To account
for transient errors, it will keep trying to connect to the server
until it succeeds once.

This commit does not include looking for changes in the associated bind
user username/password Secret.

internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go

@@ -129,7 +129,11 @@ func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream *
 
 	// No point in trying to connect to the server if the config was already determined to be invalid.
 	if secretValidCondition.Status == v1alpha1.ConditionTrue && tlsValidCondition.Status == v1alpha1.ConditionTrue {
-		conditions = append(conditions, c.validateFinishedConfig(ctx, config))
+		finishedConfigCondition := c.validateFinishedConfig(ctx, upstream, config)
+		// nil when there is no need to update this condition.
+		if finishedConfigCondition != nil {
+			conditions = append(conditions, finishedConfigCondition)
+		}
 	}
 
 	hadErrorCondition := c.updateStatus(ctx, upstream, conditions)
@@ -164,10 +168,14 @@ func (c *ldapWatcherController) validateTLSConfig(upstream *v1alpha1.LDAPIdentit
 	return c.validTLSCondition(loadedTLSConfigurationMessage)
 }
 
-func (c *ldapWatcherController) validateFinishedConfig(ctx context.Context, config *upstreamldap.ProviderConfig) *v1alpha1.Condition {
+func (c *ldapWatcherController) validateFinishedConfig(ctx context.Context, upstream *v1alpha1.LDAPIdentityProvider, config *upstreamldap.ProviderConfig) *v1alpha1.Condition {
 	ldapProvider := upstreamldap.New(*config)
 
-	testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, 60*time.Second)
+	if alreadyValidatedFinishedConfigForThisSpecGeneration(upstream) {
+		return nil
+	}
+
+	testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, 90*time.Second)
 	defer cancelFunc()
 
 	err := ldapProvider.TestConnection(testConnectionTimeout)
@@ -188,6 +196,16 @@ func (c *ldapWatcherController) validateFinishedConfig(ctx context.Context, conf
 	}
 }
 
+func alreadyValidatedFinishedConfigForThisSpecGeneration(upstream *v1alpha1.LDAPIdentityProvider) bool {
+	currentGeneration := upstream.Generation
+	for _, c := range upstream.Status.Conditions {
+		if c.Type == typeLDAPConnectionValid && c.Status == v1alpha1.ConditionTrue && c.ObservedGeneration == currentGeneration {
+			return true
+		}
+	}
+	return false
+}
+
 func (c *ldapWatcherController) validTLSCondition(message string) *v1alpha1.Condition {
 	return &v1alpha1.Condition{
 		Type:    typeTLSConfigurationValid,

internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go

@@ -184,7 +184,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 			},
 		},
 	}
-	modifiedCopyOfValidUpstream := func(editFunc func(*v1alpha1.LDAPIdentityProvider)) *v1alpha1.LDAPIdentityProvider {
+	editedValidUpstream := func(editFunc func(*v1alpha1.LDAPIdentityProvider)) *v1alpha1.LDAPIdentityProvider {
 		deepCopy := validUpstream.DeepCopy()
 		editFunc(deepCopy)
 		return deepCopy
@@ -204,6 +204,44 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 		},
 	}
 
+	bindSecretValidTrueCondition := func(gen int64) v1alpha1.Condition {
+		return v1alpha1.Condition{
+			Type:               "BindSecretValid",
+			Status:             "True",
+			LastTransitionTime: now,
+			Reason:             "Success",
+			Message:            "loaded bind secret",
+			ObservedGeneration: gen,
+		}
+	}
+	ldapConnectionValidTrueCondition := func(gen int64) v1alpha1.Condition {
+		return v1alpha1.Condition{
+			Type:               "LDAPConnectionValid",
+			Status:             "True",
+			LastTransitionTime: now,
+			Reason:             "Success",
+			Message:            fmt.Sprintf(`successfully able to connect to "%s" and bind as user "%s"`, testHost, testBindUsername),
+			ObservedGeneration: gen,
+		}
+	}
+	tlsConfigurationValidLoadedTrueCondition := func(gen int64) v1alpha1.Condition {
+		return v1alpha1.Condition{
+			Type:               "TLSConfigurationValid",
+			Status:             "True",
+			LastTransitionTime: now,
+			Reason:             "Success",
+			Message:            "loaded TLS configuration",
+			ObservedGeneration: gen,
+		}
+	}
+	allConditionsTrue := func(gen int64) []v1alpha1.Condition {
+		return []v1alpha1.Condition{
+			bindSecretValidTrueCondition(gen),
+			ldapConnectionValidTrueCondition(gen),
+			tlsConfigurationValidLoadedTrueCondition(gen),
+		}
+	}
+
 	tests := []struct {
 		name                   string
 		inputUpstreams         []runtime.Object
@@ -235,33 +273,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
 				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
 				Status: v1alpha1.LDAPIdentityProviderStatus{
-					Phase: "Ready",
+					Phase:      "Ready",
-					Conditions: []v1alpha1.Condition{
+					Conditions: allConditionsTrue(1234),
-						{
-							Type:               "BindSecretValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            "loaded bind secret",
-							ObservedGeneration: 1234,
-						},
-						{
-							Type:               "LDAPConnectionValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            fmt.Sprintf(`successfully able to connect to "%s" and bind as user "%s"`, testHost, testBindUsername),
-							ObservedGeneration: 1234,
-						},
-						{
-							Type:               "TLSConfigurationValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            "loaded TLS configuration",
-							ObservedGeneration: 1234,
-						},
-					},
 				},
 			}},
 		},
@@ -284,14 +297,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 							Message:            fmt.Sprintf(`secret "%s" not found`, testSecretName),
 							ObservedGeneration: 1234,
 						},
-						{
+						tlsConfigurationValidLoadedTrueCondition(1234),
-							Type:               "TLSConfigurationValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            "loaded TLS configuration",
-							ObservedGeneration: 1234,
-						},
 					},
 				},
 			}},
@@ -319,14 +325,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 							Message:            fmt.Sprintf(`referenced Secret "%s" has wrong type "some-other-type" (should be "kubernetes.io/basic-auth")`, testSecretName),
 							ObservedGeneration: 1234,
 						},
-						{
+						tlsConfigurationValidLoadedTrueCondition(1234),
-							Type:               "TLSConfigurationValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            "loaded TLS configuration",
-							ObservedGeneration: 1234,
-						},
 					},
 				},
 			}},
@@ -353,21 +352,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 							Message:            fmt.Sprintf(`referenced Secret "%s" is missing required keys ["username" "password"]`, testSecretName),
 							ObservedGeneration: 1234,
 						},
-						{
+						tlsConfigurationValidLoadedTrueCondition(1234),
-							Type:               "TLSConfigurationValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            "loaded TLS configuration",
-							ObservedGeneration: 1234,
-						},
 					},
 				},
 			}},
 		},
 		{
 			name: "CertificateAuthorityData is not base64 encoded",
-			inputUpstreams: []runtime.Object{modifiedCopyOfValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
+			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
 				upstream.Spec.TLS.CertificateAuthorityData = "this-is-not-base64-encoded"
 			})},
 			inputSecrets: []runtime.Object{&corev1.Secret{
@@ -382,14 +374,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				Status: v1alpha1.LDAPIdentityProviderStatus{
 					Phase: "Error",
 					Conditions: []v1alpha1.Condition{
-						{
+						bindSecretValidTrueCondition(1234),
-							Type:               "BindSecretValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            "loaded bind secret",
-							ObservedGeneration: 1234,
-						},
 						{
 							Type:               "TLSConfigurationValid",
 							Status:             "False",
@@ -404,7 +389,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 		},
 		{
 			name: "CertificateAuthorityData is not valid pem data",
-			inputUpstreams: []runtime.Object{modifiedCopyOfValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
+			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
 				upstream.Spec.TLS.CertificateAuthorityData = base64.StdEncoding.EncodeToString([]byte("this is not pem data"))
 			})},
 			inputSecrets: []runtime.Object{&corev1.Secret{
@@ -419,14 +404,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				Status: v1alpha1.LDAPIdentityProviderStatus{
 					Phase: "Error",
 					Conditions: []v1alpha1.Condition{
-						{
+						bindSecretValidTrueCondition(1234),
-							Type:               "BindSecretValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            "loaded bind secret",
-							ObservedGeneration: 1234,
-						},
 						{
 							Type:               "TLSConfigurationValid",
 							Status:             "False",
@@ -441,7 +419,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 		},
 		{
 			name: "nil TLS configuration is valid",
-			inputUpstreams: []runtime.Object{modifiedCopyOfValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
+			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
 				upstream.Spec.TLS = nil
 			})},
 			inputSecrets: []runtime.Object{&corev1.Secret{
@@ -474,22 +452,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				Status: v1alpha1.LDAPIdentityProviderStatus{
 					Phase: "Ready",
 					Conditions: []v1alpha1.Condition{
-						{
+						bindSecretValidTrueCondition(1234),
-							Type:               "BindSecretValid",
+						ldapConnectionValidTrueCondition(1234),
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            "loaded bind secret",
-							ObservedGeneration: 1234,
-						},
-						{
-							Type:               "LDAPConnectionValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            fmt.Sprintf(`successfully able to connect to "%s" and bind as user "%s"`, testHost, testBindUsername),
-							ObservedGeneration: 1234,
-						},
 						{
 							Type:               "TLSConfigurationValid",
 							Status:             "True",
@@ -504,7 +468,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 		},
 		{
 			name: "non-nil TLS configuration with empty CertificateAuthorityData is valid",
-			inputUpstreams: []runtime.Object{modifiedCopyOfValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
+			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
 				upstream.Spec.TLS.CertificateAuthorityData = ""
 			})},
 			inputSecrets: []runtime.Object{&corev1.Secret{
@@ -535,39 +499,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
 				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
 				Status: v1alpha1.LDAPIdentityProviderStatus{
-					Phase: "Ready",
+					Phase:      "Ready",
-					Conditions: []v1alpha1.Condition{
+					Conditions: allConditionsTrue(1234),
-						{
-							Type:               "BindSecretValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            "loaded bind secret",
-							ObservedGeneration: 1234,
-						},
-						{
-							Type:               "LDAPConnectionValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            fmt.Sprintf(`successfully able to connect to "%s" and bind as user "%s"`, testHost, testBindUsername),
-							ObservedGeneration: 1234,
-						},
-						{
-							Type:               "TLSConfigurationValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            "loaded TLS configuration",
-							ObservedGeneration: 1234,
-						},
-					},
 				},
 			}},
 		},
 		{
 			name: "one valid upstream and one invalid upstream updates the cache to include only the valid upstream",
-			inputUpstreams: []runtime.Object{validUpstream, modifiedCopyOfValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
+			inputUpstreams: []runtime.Object{validUpstream, editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
 				upstream.Name = "other-upstream"
 				upstream.Generation = 42
 				upstream.Spec.Bind.SecretName = "non-existent-secret"
@@ -598,47 +537,15 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 								Message:            fmt.Sprintf(`secret "%s" not found`, "non-existent-secret"),
 								ObservedGeneration: 42,
 							},
-							{
+							tlsConfigurationValidLoadedTrueCondition(42),
-								Type:               "TLSConfigurationValid",
-								Status:             "True",
-								LastTransitionTime: now,
-								Reason:             "Success",
-								Message:            "loaded TLS configuration",
-								ObservedGeneration: 42,
-							},
 						},
 					},
 				},
 				{
 					ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
 					Status: v1alpha1.LDAPIdentityProviderStatus{
-						Phase: "Ready",
+						Phase:      "Ready",
-						Conditions: []v1alpha1.Condition{
+						Conditions: allConditionsTrue(1234),
-							{
-								Type:               "BindSecretValid",
-								Status:             "True",
-								LastTransitionTime: now,
-								Reason:             "Success",
-								Message:            "loaded bind secret",
-								ObservedGeneration: 1234,
-							},
-							{
-								Type:               "LDAPConnectionValid",
-								Status:             "True",
-								LastTransitionTime: now,
-								Reason:             "Success",
-								Message:            fmt.Sprintf(`successfully able to connect to "%s" and bind as user "%s"`, testHost, testBindUsername),
-								ObservedGeneration: 1234,
-							},
-							{
-								Type:               "TLSConfigurationValid",
-								Status:             "True",
-								LastTransitionTime: now,
-								Reason:             "Success",
-								Message:            "loaded TLS configuration",
-								ObservedGeneration: 1234,
-							},
-						},
 					},
 				},
 			},
@@ -663,14 +570,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				Status: v1alpha1.LDAPIdentityProviderStatus{
 					Phase: "Error",
 					Conditions: []v1alpha1.Condition{
-						{
+						bindSecretValidTrueCondition(1234),
-							Type:               "BindSecretValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            "loaded bind secret",
-							ObservedGeneration: 1234,
-						},
 						{
 							Type:               "LDAPConnectionValid",
 							Status:             "False",
@@ -681,18 +581,104 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 								testHost, testBindUsername, testBindUsername),
 							ObservedGeneration: 1234,
 						},
-						{
+						tlsConfigurationValidLoadedTrueCondition(1234),
-							Type:               "TLSConfigurationValid",
-							Status:             "True",
-							LastTransitionTime: now,
-							Reason:             "Success",
-							Message:            "loaded TLS configuration",
-							ObservedGeneration: 1234,
-						},
 					},
 				},
 			}},
 		},
+		{
+			name: "when the LDAP server connection was already validated for this resource generation, then do not validate it again",
+			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
+				upstream.Generation = 1234
+				upstream.Status.Conditions = []v1alpha1.Condition{
+					ldapConnectionValidTrueCondition(1234),
+				}
+			})},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace},
+				Type:       corev1.SecretTypeBasicAuth,
+				Data:       testValidSecretData,
+			}},
+			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.
+			},
+			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream},
+			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Status: v1alpha1.LDAPIdentityProviderStatus{
+					Phase:      "Ready",
+					Conditions: allConditionsTrue(1234),
+				},
+			}},
+		},
+		{
+			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) {
+				upstream.Generation = 1234 // current generation
+				upstream.Status.Conditions = []v1alpha1.Condition{
+					{
+						Type:               "LDAPConnectionValid",
+						Status:             "False",
+						LastTransitionTime: now,
+						Reason:             "LDAPConnectionError",
+						Message:            "some-error-message",
+						ObservedGeneration: 1233, // older generation!
+					},
+				}
+			})},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace},
+				Type:       corev1.SecretTypeBasicAuth,
+				Data:       testValidSecretData,
+			}},
+			setupMocks: func(conn *mockldapconn.MockConn) {
+				// Should perform a test dial and bind.
+				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
+				conn.EXPECT().Close().Times(1)
+			},
+			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream},
+			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Status: v1alpha1.LDAPIdentityProviderStatus{
+					Phase:      "Ready",
+					Conditions: allConditionsTrue(1234),
+				},
+			}},
+		},
+		{
+			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) {
+				upstream.Generation = 1234
+				upstream.Status.Conditions = []v1alpha1.Condition{
+					{
+						Type:               "LDAPConnectionValid",
+						Status:             "False", // failure!
+						LastTransitionTime: now,
+						Reason:             "LDAPConnectionError",
+						Message:            "some-error-message",
+						ObservedGeneration: 1234, // same (current) generation!
+					},
+				}
+			})},
+			inputSecrets: []runtime.Object{&corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace},
+				Type:       corev1.SecretTypeBasicAuth,
+				Data:       testValidSecretData,
+			}},
+			setupMocks: func(conn *mockldapconn.MockConn) {
+				// Should perform a test dial and bind.
+				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
+				conn.EXPECT().Close().Times(1)
+			},
+			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream},
+			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Status: v1alpha1.LDAPIdentityProviderStatus{
+					Phase:      "Ready",
+					Conditions: allConditionsTrue(1234),
+				},
+			}},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt