PR feedback

internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go

@@ -100,9 +100,9 @@ func (s *activeDirectoryUpstreamGenericLDAPSpec) DetectAndSetSearchBase(ctx cont
 	if config.GroupSearch.Base != "" && config.UserSearch.Base != "" {
 		// Both were already set in spec so just return; no need to query the RootDSE
 		return &v1alpha1.Condition{
-			Type:    "SearchBaseFound",
+			Type:    upstreamwatchers.TypeSearchBaseFound,
 			Status:  v1alpha1.ConditionTrue,
-			Reason:  "Success",
+			Reason:  upstreamwatchers.ReasonUsingConfigurationFromSpec,
 			Message: "Using search base from ActiveDirectoryIdentityProvider config.",
 		}
 	}
@@ -115,7 +115,7 @@ func (s *activeDirectoryUpstreamGenericLDAPSpec) DetectAndSetSearchBase(ctx cont
 		return &v1alpha1.Condition{
 			Type:    upstreamwatchers.TypeSearchBaseFound,
 			Status:  v1alpha1.ConditionFalse,
-			Reason:  "Error",
+			Reason:  upstreamwatchers.ReasonErrorFetchingSearchBase,
 			Message: fmt.Sprintf(`Error finding search base: %s`, err.Error()),
 		}
 	}

internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go

@@ -273,7 +273,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 			Type:               "SearchBaseFound",
 			Status:             "True",
 			LastTransitionTime: now,
-			Reason:             "Success",
+			Reason:             "UsingConfigurationFromSpec",
 			Message:            "Using search base from ActiveDirectoryIdentityProvider config.",
 			ObservedGeneration: gen,
 		}
@@ -284,7 +284,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 			Type:               "SearchBaseFound",
 			Status:             "False",
 			LastTransitionTime: now,
-			Reason:             "Error",
+			Reason:             "ErrorFetchingSearchBase",
 			Message:            message,
 			ObservedGeneration: gen,
 		}
@@ -878,7 +878,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 								testHost, testBindUsername, testBindUsername),
 							ObservedGeneration: 1234,
 						},
-						searchBaseFoundErrorCondition(1234, "Error finding search base: error binding as \"test-bind-username\" before user search: some bind error"),
+						searchBaseFoundErrorCondition(1234, "Error finding search base: error binding as \"test-bind-username\" before querying for defaultNamingContext: some bind error"),
 						tlsConfigurationValidLoadedTrueCondition(1234),
 					},
 				},
@@ -1344,9 +1344,6 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 		},
 		{
 			name: "when the input activedirectoryidentityprovider leaves group search base blank and query for defaultNamingContext fails",
-			// TODO is this a fatal error? I think so because leaving the search base blank and trying anyway does not seem expected.
-			//  queries with an empty search base could potentially succeed but return something unexpected, like if you were
-			//  pointing at global catalog but not intending to use the GC functionality...
 			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) {
 				upstream.Spec.UserSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderUserSearchAttributes{}
 				upstream.Spec.GroupSearch.Base = ""

internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go

@@ -33,13 +33,15 @@ const (
 	TestLDAPConnectionTimeout = 90 * time.Second
 
 	// Constants related to conditions.
-	typeBindSecretValid           = "BindSecretValid"
-	typeTLSConfigurationValid     = "TLSConfigurationValid"
-	typeLDAPConnectionValid       = "LDAPConnectionValid"
-	TypeSearchBaseFound           = "SearchBaseFound"
-	reasonLDAPConnectionError     = "LDAPConnectionError"
-	noTLSConfigurationMessage     = "no TLS configuration provided"
-	loadedTLSConfigurationMessage = "loaded TLS configuration"
+	typeBindSecretValid              = "BindSecretValid"
+	typeTLSConfigurationValid        = "TLSConfigurationValid"
+	typeLDAPConnectionValid          = "LDAPConnectionValid"
+	TypeSearchBaseFound              = "SearchBaseFound"
+	reasonLDAPConnectionError        = "LDAPConnectionError"
+	noTLSConfigurationMessage        = "no TLS configuration provided"
+	loadedTLSConfigurationMessage    = "loaded TLS configuration"
+	ReasonUsingConfigurationFromSpec = "UsingConfigurationFromSpec"
+	ReasonErrorFetchingSearchBase    = "ErrorFetchingSearchBase"
 )
 
 // An in-memory cache with an entry for each ActiveDirectoryIdentityProvider, to keep track of which ResourceVersion
@@ -187,7 +189,7 @@ func HasPreviousSuccessfulSearchBaseConditionForCurrentGeneration(secretVersionC
 			// Found a previously successful condition for the current spec generation.
 			// Now figure out which version of the bind Secret was used during that previous validation, if any.
 			validatedSettings := secretVersionCache.ValidatedSettingsByName[upstreamName]
-			// Reload the TLS vs StartTLS setting that was previously validated.
+			// Reload the user search and group search base settings that were previously validated.
 			config.UserSearch.Base = validatedSettings.UserSearchBase
 			config.GroupSearch.Base = validatedSettings.GroupSearchBase
 			return true