resolve some todos about error handling search base discovery results

internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go

@@ -1347,7 +1347,8 @@ 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.
-			//  it could potentially succeed but return something unexpected...
+			//  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 = ""
@@ -1377,6 +1378,122 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
 					LDAPConnectionProtocol: upstreamldap.TLS,
 					UserSearchBase:         testUserSearchBase}},
 		},
+		{
+			name: "when query for defaultNamingContext returns empty string",
+			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) {
+				upstream.Spec.UserSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderUserSearchAttributes{}
+				upstream.Spec.GroupSearch.Base = ""
+			})},
+			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
+			setupMocks: func(conn *mockldapconn.MockConn) {
+				// Should perform a test dial and bind.
+				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2)
+				conn.EXPECT().Close().Times(2)
+				conn.EXPECT().Search(expectedDefaultNamingContextSearch()).Return(&ldap.SearchResult{
+					Entries: []*ldap.Entry{
+						{
+							DN: "",
+							Attributes: []*ldap.EntryAttribute{
+								ldap.NewEntryAttribute("defaultNamingContext", []string{""}),
+							},
+						},
+					}}, nil).Times(1)
+			},
+			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
+			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						bindSecretValidTrueCondition(1234),
+						activeDirectoryConnectionValidTrueCondition(1234, "4242"),
+						searchBaseFoundErrorCondition(1234, "Error finding search base: error querying RootDSE for defaultNamingContext: empty search base DN found"),
+						tlsConfigurationValidLoadedTrueCondition(1234),
+					},
+				},
+			}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{
+				testName: {BindSecretResourceVersion: "4242",
+					LDAPConnectionProtocol: upstreamldap.TLS,
+					UserSearchBase:         testUserSearchBase}},
+		},
+		{
+			name: "when query for defaultNamingContext returns multiple entries",
+			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) {
+				upstream.Spec.UserSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderUserSearchAttributes{}
+				upstream.Spec.GroupSearch.Base = ""
+			})},
+			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
+			setupMocks: func(conn *mockldapconn.MockConn) {
+				// Should perform a test dial and bind.
+				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2)
+				conn.EXPECT().Close().Times(2)
+				conn.EXPECT().Search(expectedDefaultNamingContextSearch()).Return(&ldap.SearchResult{
+					Entries: []*ldap.Entry{
+						{
+							DN: "",
+							Attributes: []*ldap.EntryAttribute{
+								ldap.NewEntryAttribute("defaultNamingContext", []string{""}),
+							},
+						},
+						{
+							DN: "",
+							Attributes: []*ldap.EntryAttribute{
+								ldap.NewEntryAttribute("defaultNamingContext", []string{""}),
+							},
+						},
+					}}, nil).Times(1)
+			},
+			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
+			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						bindSecretValidTrueCondition(1234),
+						activeDirectoryConnectionValidTrueCondition(1234, "4242"),
+						searchBaseFoundErrorCondition(1234, "Error finding search base: error querying RootDSE for defaultNamingContext: expected to find 1 entry but found 2"),
+						tlsConfigurationValidLoadedTrueCondition(1234),
+					},
+				},
+			}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{
+				testName: {BindSecretResourceVersion: "4242",
+					LDAPConnectionProtocol: upstreamldap.TLS,
+					UserSearchBase:         testUserSearchBase}},
+		},
+		{
+			name: "when query for defaultNamingContext returns no entries",
+			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) {
+				upstream.Spec.UserSearch.Attributes = v1alpha1.ActiveDirectoryIdentityProviderUserSearchAttributes{}
+				upstream.Spec.GroupSearch.Base = ""
+			})},
+			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
+			setupMocks: func(conn *mockldapconn.MockConn) {
+				// Should perform a test dial and bind.
+				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2)
+				conn.EXPECT().Close().Times(2)
+				conn.EXPECT().Search(expectedDefaultNamingContextSearch()).Return(&ldap.SearchResult{
+					Entries: []*ldap.Entry{}}, nil).Times(1)
+			},
+			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
+			wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						bindSecretValidTrueCondition(1234),
+						activeDirectoryConnectionValidTrueCondition(1234, "4242"),
+						searchBaseFoundErrorCondition(1234, "Error finding search base: error querying RootDSE for defaultNamingContext: expected to find 1 entry but found 0"),
+						tlsConfigurationValidLoadedTrueCondition(1234),
+					},
+				},
+			}},
+			wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{
+				testName: {BindSecretResourceVersion: "4242",
+					LDAPConnectionProtocol: upstreamldap.TLS,
+					UserSearchBase:         testUserSearchBase}},
+		},
 	}
 
 	for _, tt := range tests {

internal/upstreamldap/upstreamldap.go

@@ -413,12 +413,16 @@ func (p *Provider) SearchForDefaultNamingContext(ctx context.Context) (string, e
 	if err != nil {
 		return "", fmt.Errorf(`error querying RootDSE for defaultNamingContext: %w`, err)
 	}
-	// TODO handle getting empty entry back-- I think this is possible but we might want to
+
-	//  treat it as an error
+	if len(searchResult.Entries) != 1 {
-	// TODO handle getting no entries back
+		return "", fmt.Errorf(`error querying RootDSE for defaultNamingContext: expected to find 1 entry but found %d`, len(searchResult.Entries))
-	// TODO handle getting more than 1 result back
+	}
-	// TODO handle getting no values for defaultNamingContext attribute back in entry
+	searchBase := searchResult.Entries[0].GetAttributeValue("defaultNamingContext")
-	return searchResult.Entries[0].GetAttributeValue("defaultNamingContext"), nil
+	if searchBase == "" {
+		// if we get an empty search base back, treat it like an error. Otherwise we might make too broad of a search.
+		return "", fmt.Errorf(`error querying RootDSE for defaultNamingContext: empty search base DN found`)
+	}
+	return searchBase, nil
 }
 
 func (p *Provider) searchAndBindUser(conn Conn, username string, bindFunc func(conn Conn, foundUserDN string) error) (string, string, []string, error) {