From 8e438e22e959ae6be1fef6d0f003fe26b09a346f Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 15 Apr 2021 16:46:27 -0700 Subject: [PATCH] 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. --- .../upstreamwatcher/ldap_upstream_watcher.go | 24 +- .../ldap_upstream_watcher_test.go | 320 +++++++++--------- 2 files changed, 174 insertions(+), 170 deletions(-) diff --git a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go index 353267c9..fd76095a 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go +++ b/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, diff --git a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go index 0a2bdbe0..c33ceebd 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go +++ b/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", - Conditions: []v1alpha1.Condition{ - { - 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, - }, - }, + Phase: "Ready", + Conditions: allConditionsTrue(1234), }, }}, }, @@ -284,14 +297,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`secret "%s" not found`, testSecretName), ObservedGeneration: 1234, }, - { - Type: "TLSConfigurationValid", - Status: "True", - LastTransitionTime: now, - Reason: "Success", - Message: "loaded TLS configuration", - ObservedGeneration: 1234, - }, + tlsConfigurationValidLoadedTrueCondition(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, }, - { - Type: "TLSConfigurationValid", - Status: "True", - LastTransitionTime: now, - Reason: "Success", - Message: "loaded TLS configuration", - ObservedGeneration: 1234, - }, + tlsConfigurationValidLoadedTrueCondition(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, }, - { - Type: "TLSConfigurationValid", - Status: "True", - LastTransitionTime: now, - Reason: "Success", - Message: "loaded TLS configuration", - ObservedGeneration: 1234, - }, + tlsConfigurationValidLoadedTrueCondition(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{ - { - Type: "BindSecretValid", - Status: "True", - LastTransitionTime: now, - Reason: "Success", - Message: "loaded bind secret", - ObservedGeneration: 1234, - }, + bindSecretValidTrueCondition(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{ - { - Type: "BindSecretValid", - Status: "True", - LastTransitionTime: now, - Reason: "Success", - Message: "loaded bind secret", - ObservedGeneration: 1234, - }, + bindSecretValidTrueCondition(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{ - { - 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, - }, + bindSecretValidTrueCondition(1234), + ldapConnectionValidTrueCondition(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", - Conditions: []v1alpha1.Condition{ - { - 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, - }, - }, + Phase: "Ready", + Conditions: allConditionsTrue(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, }, - { - Type: "TLSConfigurationValid", - Status: "True", - LastTransitionTime: now, - Reason: "Success", - Message: "loaded TLS configuration", - ObservedGeneration: 42, - }, + tlsConfigurationValidLoadedTrueCondition(42), }, }, }, { ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ - Phase: "Ready", - Conditions: []v1alpha1.Condition{ - { - 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, - }, - }, + Phase: "Ready", + Conditions: allConditionsTrue(1234), }, }, }, @@ -663,14 +570,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Status: v1alpha1.LDAPIdentityProviderStatus{ Phase: "Error", Conditions: []v1alpha1.Condition{ - { - Type: "BindSecretValid", - Status: "True", - LastTransitionTime: now, - Reason: "Success", - Message: "loaded bind secret", - ObservedGeneration: 1234, - }, + bindSecretValidTrueCondition(1234), { Type: "LDAPConnectionValid", Status: "False", @@ -681,18 +581,104 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { testHost, testBindUsername, testBindUsername), ObservedGeneration: 1234, }, - { - Type: "TLSConfigurationValid", - Status: "True", - LastTransitionTime: now, - Reason: "Success", - Message: "loaded TLS configuration", - ObservedGeneration: 1234, - }, + tlsConfigurationValidLoadedTrueCondition(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