LDAP upstream watcher controller tries using both TLS and StartTLS

- Automatically try to fall back to using StartTLS when using TLS
  doesn't work. Only complain when both don't work.
- Remember (in-memory) which one worked and keeping using that one
  in the future (unless the pod restarts).

internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go

@@ -58,13 +58,18 @@ type ldapWatcherController struct {
 }
 
 // An in-memory cache with an entry for each LDAPIdentityProvider, to keep track of which ResourceVersion
-// of the bind Secret was used during the most recent successful validation.
+// of the bind Secret and which TLS/StartTLS setting was used during the most recent successful validation.
 type secretVersionCache struct {
-	ResourceVersionsByName map[string]string
+	ValidatedSettingsByName map[string]validatedSettings
+}
+
+type validatedSettings struct {
+	BindSecretResourceVersion string
+	LDAPConnectionProtocol    upstreamldap.LDAPConnectionProtocol
 }
 
 func newSecretVersionCache() *secretVersionCache {
-	return &secretVersionCache{ResourceVersionsByName: map[string]string{}}
+	return &secretVersionCache{ValidatedSettingsByName: map[string]validatedSettings{}}
 }
 
 // New instantiates a new controllerlib.Controller which will populate the provided UpstreamLDAPIdentityProviderICache.
@@ -154,7 +159,6 @@ func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream *
 	config := &upstreamldap.ProviderConfig{
 		Name: upstream.Name,
 		Host: spec.Host,
-		ConnectionProtocol: upstreamldap.TLS,
 		UserSearch: upstreamldap.UserSearchConfig{
 			Base:              spec.UserSearch.Base,
 			Filter:            spec.UserSearch.Filter,
@@ -229,22 +233,23 @@ func (c *ldapWatcherController) validateTLSConfig(upstream *v1alpha1.LDAPIdentit
 }
 
 func (c *ldapWatcherController) validateFinishedConfig(ctx context.Context, upstream *v1alpha1.LDAPIdentityProvider, config *upstreamldap.ProviderConfig, currentSecretVersion string) *v1alpha1.Condition {
-	ldapProvider := upstreamldap.New(*config)
+	if c.hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream, currentSecretVersion, config) {
-
-	if c.hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream, currentSecretVersion) {
 		return nil
 	}
 
 	testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, testLDAPConnectionTimeout)
 	defer cancelFunc()
 
-	condition := c.testConnection(testConnectionTimeout, upstream, config, ldapProvider, currentSecretVersion)
+	condition := c.testConnection(testConnectionTimeout, upstream, config, currentSecretVersion)
 
 	if condition.Status == v1alpha1.ConditionTrue {
 		// Remember (in-memory for this pod) that the controller has successfully validated the LDAP 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.
-		c.validatedSecretVersionsCache.ResourceVersionsByName[upstream.GetName()] = currentSecretVersion
+		c.validatedSecretVersionsCache.ValidatedSettingsByName[upstream.GetName()] = validatedSettings{
+			BindSecretResourceVersion: currentSecretVersion,
+			LDAPConnectionProtocol:    config.ConnectionProtocol,
+		}
 	}
 
 	return condition
@@ -254,10 +259,28 @@ func (c *ldapWatcherController) testConnection(
 	ctx context.Context,
 	upstream *v1alpha1.LDAPIdentityProvider,
 	config *upstreamldap.ProviderConfig,
-	ldapProvider *upstreamldap.Provider,
 	currentSecretVersion string,
 ) *v1alpha1.Condition {
-	err := ldapProvider.TestConnection(ctx)
+	// First try using TLS.
+	config.ConnectionProtocol = upstreamldap.TLS
+	tlsLDAPProvider := upstreamldap.New(*config)
+	err := tlsLDAPProvider.TestConnection(ctx)
+	if err != nil {
+		// If there was any error, try again with StartTLS instead.
+		config.ConnectionProtocol = upstreamldap.StartTLS
+		startTLSLDAPProvider := upstreamldap.New(*config)
+		startTLSErr := startTLSLDAPProvider.TestConnection(ctx)
+		if startTLSErr == nil {
+			// Successfully able to fall back to using StartTLS, so clear the original
+			// error and consider the connection test to be successful.
+			err = nil
+		} else {
+			// Falling back to StartTLS also failed, so put TLS back into the config
+			// and consider the connection test to be failed.
+			config.ConnectionProtocol = upstreamldap.TLS
+		}
+	}
+
 	if err != nil {
 		return &v1alpha1.Condition{
 			Type:   typeLDAPConnectionValid,
@@ -277,14 +300,16 @@ func (c *ldapWatcherController) testConnection(
 	}
 }
 
-func (c *ldapWatcherController) hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream *v1alpha1.LDAPIdentityProvider, currentSecretVersion string) bool {
+func (c *ldapWatcherController) hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream *v1alpha1.LDAPIdentityProvider, currentSecretVersion string, config *upstreamldap.ProviderConfig) bool {
 	currentGeneration := upstream.Generation
 	for _, cond := range upstream.Status.Conditions {
 		if cond.Type == typeLDAPConnectionValid && cond.Status == v1alpha1.ConditionTrue && cond.ObservedGeneration == currentGeneration {
 			// 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.
-			validatedSecretVersion := c.validatedSecretVersionsCache.ResourceVersionsByName[upstream.GetName()]
+			validatedSecretVersion := c.validatedSecretVersionsCache.ValidatedSettingsByName[upstream.GetName()]
-			if validatedSecretVersion == currentSecretVersion {
+			if validatedSecretVersion.BindSecretResourceVersion == currentSecretVersion {
+				// Reload the TLS vs StartTLS setting that was previously validated.
+				config.ConnectionProtocol = validatedSecretVersion.LDAPConnectionProtocol
 				return true
 			}
 		}

internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go

@@ -12,6 +12,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/go-ldap/ldap/v3"
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
@@ -196,7 +197,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 		return deepCopy
 	}
 
-	providerConfigForValidUpstream := &upstreamldap.ProviderConfig{
+	providerConfigForValidUpstreamWithTLS := &upstreamldap.ProviderConfig{
 		Name:               testName,
 		Host:               testHost,
 		ConnectionProtocol: upstreamldap.TLS,
@@ -216,6 +217,11 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 		},
 	}
 
+	// Make a copy with targeted changes.
+	copyOfProviderConfigForValidUpstreamWithTLS := *providerConfigForValidUpstreamWithTLS
+	providerConfigForValidUpstreamWithStartTLS := &copyOfProviderConfigForValidUpstreamWithTLS
+	providerConfigForValidUpstreamWithStartTLS.ConnectionProtocol = upstreamldap.StartTLS
+
 	bindSecretValidTrueCondition := func(gen int64) v1alpha1.Condition {
 		return v1alpha1.Condition{
 			Type:               "BindSecretValid",
@@ -266,15 +272,15 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 
 	tests := []struct {
 		name                     string
-		initialValidatedSecretVersions map[string]string
+		initialValidatedSettings map[string]validatedSettings
 		inputUpstreams           []runtime.Object
 		inputSecrets             []runtime.Object
 		setupMocks               func(conn *mockldapconn.MockConn)
-		dialError                      error
+		dialErrors               map[string]error
 		wantErr                  string
 		wantResultingCache       []*upstreamldap.ProviderConfig
 		wantResultingUpstreams   []v1alpha1.LDAPIdentityProvider
-		wantValidatedSecretVersions    map[string]string
+		wantValidatedSettings    map[string]validatedSettings
 	}{
 		{
 			name:               "no LDAPIdentityProvider upstreams clears the cache",
@@ -289,7 +295,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
 				conn.EXPECT().Close().Times(1)
 			},
-			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream},
+			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS},
 			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
 				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
 				Status: v1alpha1.LDAPIdentityProviderStatus{
@@ -297,7 +303,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 					Conditions: allConditionsTrue(1234, "4242"),
 				},
 			}},
-			wantValidatedSecretVersions: map[string]string{testName: "4242"},
+			wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}},
 		},
 		{
 			name:               "missing secret",
@@ -480,7 +486,121 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 					},
 				},
 			}},
-			wantValidatedSecretVersions: map[string]string{testName: "4242"},
+			wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}},
+		},
+		{
+			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) {
+				upstream.Spec.Host = "ldap.example.com" // when the port is not specified, automatically switch ports for StartTLS
+			})},
+			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
+			setupMocks: func(conn *mockldapconn.MockConn) {
+				// Should perform a test dial and bind.
+				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
+				conn.EXPECT().Close().Times(1)
+			},
+			dialErrors: map[string]error{
+				"ldap.example.com:" + ldap.DefaultLdapsPort: fmt.Errorf("some ldaps dial error"),
+				"ldap.example.com:" + ldap.DefaultLdapPort:  nil, // no error on the regular ldap:// port
+			},
+			wantResultingCache: []*upstreamldap.ProviderConfig{
+				{
+					Name:               testName,
+					Host:               "ldap.example.com",
+					ConnectionProtocol: upstreamldap.StartTLS, // successfully fell back to using StartTLS
+					CABundle:           testCABundle,
+					BindUsername:       testBindUsername,
+					BindPassword:       testBindPassword,
+					UserSearch: upstreamldap.UserSearchConfig{
+						Base:              testUserSearchBase,
+						Filter:            testUserSearchFilter,
+						UsernameAttribute: testUsernameAttrName,
+						UIDAttribute:      testUIDAttrName,
+					},
+					GroupSearch: upstreamldap.GroupSearchConfig{
+						Base:               testGroupSearchBase,
+						Filter:             testGroupSearchFilter,
+						GroupNameAttribute: testGroupNameAttrName,
+					},
+				},
+			},
+			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Status: v1alpha1.LDAPIdentityProviderStatus{
+					Phase: "Ready",
+					Conditions: []v1alpha1.Condition{
+						bindSecretValidTrueCondition(1234),
+						{
+							Type:               "LDAPConnectionValid",
+							Status:             "True",
+							LastTransitionTime: now,
+							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"),
+							ObservedGeneration: 1234,
+						},
+						tlsConfigurationValidLoadedTrueCondition(1234),
+					},
+				},
+			}},
+			wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}},
+		},
+		{
+			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) {
+				upstream.Spec.Host = "ldap.example.com:5678" // when the port is specified, do not automatically switch ports for StartTLS
+			})},
+			inputSecrets: []runtime.Object{validBindUserSecret("4242")},
+			setupMocks: func(conn *mockldapconn.MockConn) {
+				// Both dials fail, so there should be no bind.
+			},
+			dialErrors: map[string]error{
+				"ldap.example.com:5678": fmt.Errorf("some dial error"), // both TLS and StartTLS should try the same port and both fail
+			},
+			wantResultingCache: []*upstreamldap.ProviderConfig{
+				// even though the connection test failed, still loads into the cache because it is treated like a warning
+				{
+					Name:               testName,
+					Host:               "ldap.example.com:5678",
+					ConnectionProtocol: upstreamldap.TLS, // need to pick TLS or StartTLS to load into the cache when both fail, so choose TLS
+					CABundle:           testCABundle,
+					BindUsername:       testBindUsername,
+					BindPassword:       testBindPassword,
+					UserSearch: upstreamldap.UserSearchConfig{
+						Base:              testUserSearchBase,
+						Filter:            testUserSearchFilter,
+						UsernameAttribute: testUsernameAttrName,
+						UIDAttribute:      testUIDAttrName,
+					},
+					GroupSearch: upstreamldap.GroupSearchConfig{
+						Base:               testGroupSearchBase,
+						Filter:             testGroupSearchFilter,
+						GroupNameAttribute: testGroupNameAttrName,
+					},
+				},
+			},
+			wantErr: controllerlib.ErrSyntheticRequeue.Error(),
+			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Status: v1alpha1.LDAPIdentityProviderStatus{
+					Phase: "Error",
+					Conditions: []v1alpha1.Condition{
+						bindSecretValidTrueCondition(1234),
+						{
+							Type:               "LDAPConnectionValid",
+							Status:             "False",
+							LastTransitionTime: now,
+							Reason:             "LDAPConnectionError",
+							Message: fmt.Sprintf(
+								`could not successfully connect to "%s" and bind as user "%s": error dialing host "%s": some dial error`,
+								"ldap.example.com:5678", testBindUsername, "ldap.example.com:5678"),
+							ObservedGeneration: 1234,
+						},
+						tlsConfigurationValidLoadedTrueCondition(1234),
+					},
+				},
+			}},
 		},
 		{
 			name: "non-nil TLS configuration with empty CertificateAuthorityData is valid",
@@ -521,7 +641,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 					Conditions: allConditionsTrue(1234, "4242"),
 				},
 			}},
-			wantValidatedSecretVersions: map[string]string{testName: "4242"},
+			wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}},
 		},
 		{
 			name: "one valid upstream and one invalid upstream updates the cache to include only the valid upstream",
@@ -537,7 +657,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				conn.EXPECT().Close().Times(1)
 			},
 			wantErr:            controllerlib.ErrSyntheticRequeue.Error(),
-			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream},
+			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS},
 			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{
 				{
 					ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: "other-upstream", Generation: 42},
@@ -564,7 +684,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 					},
 				},
 			},
-			wantValidatedSecretVersions: map[string]string{testName: "4242"},
+			wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}},
 		},
 		{
 			name:           "when testing the connection to the LDAP server fails then the upstream is still added to the cache anyway (treated like a warning)",
@@ -572,11 +692,12 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 			inputSecrets:   []runtime.Object{validBindUserSecret("")},
 			setupMocks: func(conn *mockldapconn.MockConn) {
 				// Should perform a test dial and bind.
-				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1).Return(errors.New("some bind error"))
+				// Expect two calls to each of these: once for trying TLS and once for trying StartTLS.
-				conn.EXPECT().Close().Times(1)
+				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2).Return(errors.New("some bind error"))
+				conn.EXPECT().Close().Times(2)
 			},
 			wantErr:            controllerlib.ErrSyntheticRequeue.Error(),
-			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstream},
+			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS},
 			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
 				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
 				Status: v1alpha1.LDAPIdentityProviderStatus{
@@ -599,7 +720,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 			}},
 		},
 		{
-			name: "when the LDAP server connection was already validated for the current resource generation and secret version, then do not validate it again",
+			name: "when the LDAP server connection was already validated using TLS for the current resource generation and secret version, then do not validate it again and keep using TLS",
 			inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) {
 				upstream.Generation = 1234
 				upstream.Status.Conditions = []v1alpha1.Condition{
@@ -607,11 +728,11 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				}
 			})},
 			inputSecrets:             []runtime.Object{validBindUserSecret("4242")},
-			initialValidatedSecretVersions: map[string]string{testName: "4242"},
+			initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}},
 			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},
+			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS},
 			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
 				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
 				Status: v1alpha1.LDAPIdentityProviderStatus{
@@ -619,7 +740,30 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 					Conditions: allConditionsTrue(1234, "4242"),
 				},
 			}},
-			wantValidatedSecretVersions: map[string]string{testName: "4242"},
+			wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}},
+		},
+		{
+			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) {
+				upstream.Generation = 1234
+				upstream.Status.Conditions = []v1alpha1.Condition{
+					ldapConnectionValidTrueCondition(1234, "4242"),
+				}
+			})},
+			inputSecrets:             []runtime.Object{validBindUserSecret("4242")},
+			initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}},
+			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{providerConfigForValidUpstreamWithStartTLS},
+			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
+				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
+				Status: v1alpha1.LDAPIdentityProviderStatus{
+					Phase:      "Ready",
+					Conditions: allConditionsTrue(1234, "4242"),
+				},
+			}},
+			wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS}},
 		},
 		{
 			name: "when the LDAP server connection was validated for an older resource generation, then try to validate it again",
@@ -630,13 +774,13 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				}
 			})},
 			inputSecrets:             []runtime.Object{validBindUserSecret("4242")},
-			initialValidatedSecretVersions: map[string]string{testName: "4242"},
+			initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}},
 			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},
+			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS},
 			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
 				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
 				Status: v1alpha1.LDAPIdentityProviderStatus{
@@ -644,7 +788,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 					Conditions: allConditionsTrue(1234, "4242"),
 				},
 			}},
-			wantValidatedSecretVersions: map[string]string{testName: "4242"},
+			wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}},
 		},
 		{
 			name: "when the LDAP server connection validation previously failed for this resource generation, then try to validate it again",
@@ -662,13 +806,13 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				}
 			})},
 			inputSecrets:             []runtime.Object{validBindUserSecret("4242")},
-			initialValidatedSecretVersions: map[string]string{testName: "1"},
+			initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "1", LDAPConnectionProtocol: upstreamldap.TLS}},
 			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},
+			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS},
 			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
 				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
 				Status: v1alpha1.LDAPIdentityProviderStatus{
@@ -676,7 +820,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 					Conditions: allConditionsTrue(1234, "4242"),
 				},
 			}},
-			wantValidatedSecretVersions: map[string]string{testName: "4242"},
+			wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}},
 		},
 		{
 			name: "when the LDAP server connection was already validated for this resource generation but the bind secret has changed, then try to validate it again",
@@ -687,13 +831,13 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				}
 			})},
 			inputSecrets:             []runtime.Object{validBindUserSecret("4242")},                                                                         // newer secret version!
-			initialValidatedSecretVersions: map[string]string{testName: "4241"},           // old version was validated
+			initialValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4241", LDAPConnectionProtocol: upstreamldap.TLS}}, // old version was validated
 			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},
+			wantResultingCache: []*upstreamldap.ProviderConfig{providerConfigForValidUpstreamWithTLS},
 			wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{
 				ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234},
 				Status: v1alpha1.LDAPIdentityProviderStatus{
@@ -701,7 +845,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 					Conditions: allConditionsTrue(1234, "4242"),
 				},
 			}},
-			wantValidatedSecretVersions: map[string]string{testName: "4242"},
+			wantValidatedSettings: map[string]validatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS}},
 		},
 	}
 
@@ -727,16 +871,19 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 				tt.setupMocks(conn)
 			}
 
-			dialer := &comparableDialer{upstreamldap.LDAPDialerFunc(func(ctx context.Context, _ string) (upstreamldap.Conn, error) {
+			dialer := &comparableDialer{upstreamldap.LDAPDialerFunc(func(ctx context.Context, hostAndPort string) (upstreamldap.Conn, error) {
-				if tt.dialError != nil {
+				if tt.dialErrors != nil {
-					return nil, tt.dialError
+					dialErr := tt.dialErrors[hostAndPort]
+					if dialErr != nil {
+						return nil, dialErr
+					}
 				}
 				return conn, nil
 			})}
 
 			validatedSecretVersionCache := newSecretVersionCache()
-			if tt.initialValidatedSecretVersions != nil {
+			if tt.initialValidatedSettings != nil {
-				validatedSecretVersionCache.ResourceVersionsByName = tt.initialValidatedSecretVersions
+				validatedSecretVersionCache.ValidatedSettingsByName = tt.initialValidatedSettings
 			}
 
 			controller := newInternal(
@@ -768,11 +915,11 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 			require.Equal(t, len(tt.wantResultingCache), len(actualIDPList))
 			for i := range actualIDPList {
 				actualIDP := actualIDPList[i].(*upstreamldap.Provider)
-				copyOfExpectedValue := *tt.wantResultingCache[i] // copy before edit to avoid race because these tests are run in parallel
+				copyOfExpectedValueForResultingCache := *tt.wantResultingCache[i] // copy before edit to avoid race because these tests are run in parallel
 				// The dialer that was passed in to the controller's constructor should always have been
 				// passed through to the provider.
-				copyOfExpectedValue.Dialer = dialer
+				copyOfExpectedValueForResultingCache.Dialer = dialer
-				require.Equal(t, copyOfExpectedValue, actualIDP.GetConfig())
+				require.Equal(t, copyOfExpectedValueForResultingCache, actualIDP.GetConfig())
 			}
 
 			actualUpstreams, err := fakePinnipedClient.IDPV1alpha1().LDAPIdentityProviders(testNamespace).List(ctx, metav1.ListOptions{})
@@ -787,10 +934,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
 			}
 
 			// Check that the controller remembered which version of the secret it most recently validated successfully with.
-			if tt.wantValidatedSecretVersions == nil {
+			if tt.wantValidatedSettings == nil {
-				tt.wantValidatedSecretVersions = map[string]string{}
+				tt.wantValidatedSettings = map[string]validatedSettings{}
 			}
-			require.Equal(t, tt.wantValidatedSecretVersions, validatedSecretVersionCache.ResourceVersionsByName)
+			require.Equal(t, tt.wantValidatedSettings, validatedSecretVersionCache.ValidatedSettingsByName)
 		})
 	}
 }

internal/upstreamldap/upstreamldap.go

@@ -157,16 +157,26 @@ func (p *Provider) dial(ctx context.Context) (Conn, error) {
 		return nil, ldap.NewError(ldap.ErrorNetwork, err)
 	}
 
+	// Choose how and where to dial based on TLS vs. StartTLS config option.
+	var dialFunc LDAPDialerFunc
+	var hostAndPort string
 	switch {
-	case p.c.Dialer != nil:
-		return p.c.Dialer.Dial(ctx, tlsHostAndPort)
 	case p.c.ConnectionProtocol == TLS:
-		return p.dialTLS(ctx, tlsHostAndPort)
+		dialFunc = p.dialTLS
+		hostAndPort = tlsHostAndPort
 	case p.c.ConnectionProtocol == StartTLS:
-		return p.dialStartTLS(ctx, startTLSHostAndPort)
+		dialFunc = p.dialStartTLS
+		hostAndPort = startTLSHostAndPort
 	default:
 		return nil, ldap.NewError(ldap.ErrorNetwork, fmt.Errorf("did not specify valid ConnectionProtocol"))
 	}
+
+	// Override the real dialer for testing purposes sometimes.
+	if p.c.Dialer != nil {
+		dialFunc = p.c.Dialer.Dial
+	}
+
+	return dialFunc(ctx, hostAndPort)
 }
 
 // dialTLS is a default implementation of the Dialer, used when Dialer is nil and ConnectionProtocol is TLS.

internal/upstreamldap/upstreamldap_test.go

@@ -58,6 +58,7 @@ func TestEndUserAuthentication(t *testing.T) {
 			Name:               "some-provider-name",
 			Host:               testHost,
 			CABundle:           nil, // this field is only used by the production dialer, which is replaced by a mock for this test
+			ConnectionProtocol: TLS,
 			BindUsername:       testBindUsername,
 			BindPassword:       testBindPassword,
 			UserSearch: UserSearchConfig{
@@ -992,6 +993,7 @@ func TestTestConnection(t *testing.T) {
 			Name:               "some-provider-name",
 			Host:               testHost,
 			CABundle:           nil, // this field is only used by the production dialer, which is replaced by a mock for this test
+			ConnectionProtocol: TLS,
 			BindUsername:       testBindUsername,
 			BindPassword:       testBindPassword,
 			UserSearch:         UserSearchConfig{}, // not used by TestConnection