From 83085aa3d6e796a64dc13c9ba6499faa815130ab Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 15 Apr 2021 17:45:15 -0700 Subject: [PATCH] Retest the server connection when the bind Secret has changed Unfortunately, Secrets do not seem to have a Generation field, so we use the ResourceVersion field instead. This means that any change to the Secret will cause us to retry the connection to the LDAP server, even if the username and password fields in the Secret were not changed. Seems like an okay trade-off for this early draft of the controller compared to a more complex implementation. --- .../upstreamwatcher/ldap_upstream_watcher.go | 75 ++++++---- .../ldap_upstream_watcher_test.go | 130 ++++++++---------- 2 files changed, 105 insertions(+), 100 deletions(-) diff --git a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go index fd76095a6..6a0796ebe 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go @@ -8,6 +8,7 @@ import ( "crypto/x509" "encoding/base64" "fmt" + "regexp" "time" corev1 "k8s.io/api/core/v1" @@ -29,6 +30,7 @@ import ( const ( ldapControllerName = "ldap-upstream-observer" ldapBindAccountSecretType = corev1.SecretTypeBasicAuth + testLDAPConnectionTimeout = 90 * time.Second // Constants related to conditions. typeBindSecretValid = "BindSecretValid" @@ -39,6 +41,10 @@ const ( loadedTLSConfigurationMessage = "loaded TLS configuration" ) +var ( + secretVersionParser = regexp.MustCompile(` \[validated with Secret ".+" at version "(.+)"]`) +) + // UpstreamLDAPIdentityProviderICache is a thread safe cache that holds a list of validated upstream LDAP IDP configurations. type UpstreamLDAPIdentityProviderICache interface { SetLDAPIdentityProviders([]provider.UpstreamLDAPIdentityProviderI) @@ -123,13 +129,13 @@ func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream * } conditions := []*v1alpha1.Condition{} - secretValidCondition := c.validateSecret(upstream, config) + secretValidCondition, currentSecretVersion := c.validateSecret(upstream, config) tlsValidCondition := c.validateTLSConfig(upstream, config) conditions = append(conditions, secretValidCondition, tlsValidCondition) // 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 { - finishedConfigCondition := c.validateFinishedConfig(ctx, upstream, config) + finishedConfigCondition := c.validateFinishedConfig(ctx, upstream, config, currentSecretVersion) // nil when there is no need to update this condition. if finishedConfigCondition != nil { conditions = append(conditions, finishedConfigCondition) @@ -168,39 +174,50 @@ func (c *ldapWatcherController) validateTLSConfig(upstream *v1alpha1.LDAPIdentit return c.validTLSCondition(loadedTLSConfigurationMessage) } -func (c *ldapWatcherController) validateFinishedConfig(ctx context.Context, upstream *v1alpha1.LDAPIdentityProvider, config *upstreamldap.ProviderConfig) *v1alpha1.Condition { +func (c *ldapWatcherController) validateFinishedConfig(ctx context.Context, upstream *v1alpha1.LDAPIdentityProvider, config *upstreamldap.ProviderConfig, currentSecretVersion string) *v1alpha1.Condition { ldapProvider := upstreamldap.New(*config) - if alreadyValidatedFinishedConfigForThisSpecGeneration(upstream) { + if hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream, currentSecretVersion) { return nil } - testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, 90*time.Second) + testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, testLDAPConnectionTimeout) defer cancelFunc() err := ldapProvider.TestConnection(testConnectionTimeout) if err != nil { return &v1alpha1.Condition{ - Type: typeLDAPConnectionValid, - Status: v1alpha1.ConditionFalse, - Reason: reasonLDAPConnectionError, - Message: fmt.Sprintf(`could not successfully connect to "%s" and bind as user "%s: %s`, config.Host, config.BindUsername, err.Error()), + Type: typeLDAPConnectionValid, + Status: v1alpha1.ConditionFalse, + Reason: reasonLDAPConnectionError, + Message: fmt.Sprintf(`could not successfully connect to "%s" and bind as user "%s": %s`, + config.Host, config.BindUsername, err.Error()), } } return &v1alpha1.Condition{ - Type: typeLDAPConnectionValid, - Status: v1alpha1.ConditionTrue, - Reason: reasonSuccess, - Message: fmt.Sprintf(`successfully able to connect to "%s" and bind as user "%s"`, config.Host, config.BindUsername), + Type: typeLDAPConnectionValid, + Status: v1alpha1.ConditionTrue, + Reason: reasonSuccess, + Message: fmt.Sprintf(`successfully able to connect to "%s" and bind as user "%s" [validated with Secret "%s" at version "%s"]`, + config.Host, config.BindUsername, upstream.Spec.Bind.SecretName, currentSecretVersion), } } -func alreadyValidatedFinishedConfigForThisSpecGeneration(upstream *v1alpha1.LDAPIdentityProvider) bool { +func hasPreviousSuccessfulConditionForCurrentSpecGenerationAndSecretVersion(upstream *v1alpha1.LDAPIdentityProvider, currentSecretVersion string) bool { currentGeneration := upstream.Generation for _, c := range upstream.Status.Conditions { if c.Type == typeLDAPConnectionValid && c.Status == v1alpha1.ConditionTrue && c.ObservedGeneration == currentGeneration { - return true + // 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. + matches := secretVersionParser.FindStringSubmatch(c.Message) + if len(matches) != 2 { + continue + } + validatedSecretVersion := matches[1] + if validatedSecretVersion == currentSecretVersion { + return true + } } } return false @@ -224,7 +241,7 @@ func (c *ldapWatcherController) invalidTLSCondition(message string) *v1alpha1.Co } } -func (c *ldapWatcherController) validateSecret(upstream *v1alpha1.LDAPIdentityProvider, config *upstreamldap.ProviderConfig) *v1alpha1.Condition { +func (c *ldapWatcherController) validateSecret(upstream *v1alpha1.LDAPIdentityProvider, config *upstreamldap.ProviderConfig) (*v1alpha1.Condition, string) { secretName := upstream.Spec.Bind.SecretName secret, err := c.secretInformer.Lister().Secrets(upstream.Namespace).Get(secretName) @@ -234,27 +251,29 @@ func (c *ldapWatcherController) validateSecret(upstream *v1alpha1.LDAPIdentityPr Status: v1alpha1.ConditionFalse, Reason: reasonNotFound, Message: err.Error(), - } + }, "" } if secret.Type != corev1.SecretTypeBasicAuth { return &v1alpha1.Condition{ - Type: typeBindSecretValid, - Status: v1alpha1.ConditionFalse, - Reason: reasonWrongType, - Message: fmt.Sprintf("referenced Secret %q has wrong type %q (should be %q)", secretName, secret.Type, corev1.SecretTypeBasicAuth), - } + Type: typeBindSecretValid, + Status: v1alpha1.ConditionFalse, + Reason: reasonWrongType, + Message: fmt.Sprintf("referenced Secret %q has wrong type %q (should be %q)", + secretName, secret.Type, corev1.SecretTypeBasicAuth), + }, secret.ResourceVersion } config.BindUsername = string(secret.Data[corev1.BasicAuthUsernameKey]) config.BindPassword = string(secret.Data[corev1.BasicAuthPasswordKey]) if len(config.BindUsername) == 0 || len(config.BindPassword) == 0 { return &v1alpha1.Condition{ - Type: typeBindSecretValid, - Status: v1alpha1.ConditionFalse, - Reason: reasonMissingKeys, - Message: fmt.Sprintf("referenced Secret %q is missing required keys %q", secretName, []string{corev1.BasicAuthUsernameKey, corev1.BasicAuthPasswordKey}), - } + Type: typeBindSecretValid, + Status: v1alpha1.ConditionFalse, + Reason: reasonMissingKeys, + Message: fmt.Sprintf("referenced Secret %q is missing required keys %q", + secretName, []string{corev1.BasicAuthUsernameKey, corev1.BasicAuthPasswordKey}), + }, secret.ResourceVersion } return &v1alpha1.Condition{ @@ -262,7 +281,7 @@ func (c *ldapWatcherController) validateSecret(upstream *v1alpha1.LDAPIdentityPr Status: v1alpha1.ConditionTrue, Reason: reasonSuccess, Message: "loaded bind secret", - } + }, secret.ResourceVersion } func (c *ldapWatcherController) updateStatus(ctx context.Context, upstream *v1alpha1.LDAPIdentityProvider, conditions []*v1alpha1.Condition) bool { diff --git a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go index c33ceebdf..b8af6d0c5 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher_test.go @@ -214,13 +214,15 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ObservedGeneration: gen, } } - ldapConnectionValidTrueCondition := func(gen int64) v1alpha1.Condition { + ldapConnectionValidTrueCondition := func(gen int64, secretVersion string) 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), + Message: fmt.Sprintf( + `successfully able to connect to "%s" and bind as user "%s" [validated with Secret "%s" at version "%s"]`, + testHost, testBindUsername, testSecretName, secretVersion), ObservedGeneration: gen, } } @@ -234,14 +236,22 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ObservedGeneration: gen, } } - allConditionsTrue := func(gen int64) []v1alpha1.Condition { + allConditionsTrue := func(gen int64, secretVersion string) []v1alpha1.Condition { return []v1alpha1.Condition{ bindSecretValidTrueCondition(gen), - ldapConnectionValidTrueCondition(gen), + ldapConnectionValidTrueCondition(gen, secretVersion), tlsConfigurationValidLoadedTrueCondition(gen), } } + validBindUserSecret := func(secretVersion string) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace, ResourceVersion: secretVersion}, + Type: corev1.SecretTypeBasicAuth, + Data: testValidSecretData, + } + } + tests := []struct { name string inputUpstreams []runtime.Object @@ -259,11 +269,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { { name: "one valid upstream updates the cache to include only that upstream", inputUpstreams: []runtime.Object{validUpstream}, - inputSecrets: []runtime.Object{&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, - Type: corev1.SecretTypeBasicAuth, - Data: testValidSecretData, - }}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -274,7 +280,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ Phase: "Ready", - Conditions: allConditionsTrue(1234), + Conditions: allConditionsTrue(1234, "4242"), }, }}, }, @@ -362,11 +368,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { upstream.Spec.TLS.CertificateAuthorityData = "this-is-not-base64-encoded" })}, - inputSecrets: []runtime.Object{&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, - Type: corev1.SecretTypeBasicAuth, - Data: testValidSecretData, - }}, + inputSecrets: []runtime.Object{validBindUserSecret("")}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantResultingCache: []*upstreamldap.ProviderConfig{}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ @@ -392,11 +394,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { 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{ - ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, - Type: corev1.SecretTypeBasicAuth, - Data: testValidSecretData, - }}, + inputSecrets: []runtime.Object{validBindUserSecret("")}, wantErr: controllerlib.ErrSyntheticRequeue.Error(), wantResultingCache: []*upstreamldap.ProviderConfig{}, wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ @@ -422,11 +420,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { upstream.Spec.TLS = nil })}, - inputSecrets: []runtime.Object{&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, - Type: corev1.SecretTypeBasicAuth, - Data: testValidSecretData, - }}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -453,7 +447,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Phase: "Ready", Conditions: []v1alpha1.Condition{ bindSecretValidTrueCondition(1234), - ldapConnectionValidTrueCondition(1234), + ldapConnectionValidTrueCondition(1234, "4242"), { Type: "TLSConfigurationValid", Status: "True", @@ -471,11 +465,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { upstream.Spec.TLS.CertificateAuthorityData = "" })}, - inputSecrets: []runtime.Object{&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, - Type: corev1.SecretTypeBasicAuth, - Data: testValidSecretData, - }}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -500,7 +490,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ Phase: "Ready", - Conditions: allConditionsTrue(1234), + Conditions: allConditionsTrue(1234, "4242"), }, }}, }, @@ -511,11 +501,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { upstream.Generation = 42 upstream.Spec.Bind.SecretName = "non-existent-secret" })}, - inputSecrets: []runtime.Object{&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, - Type: corev1.SecretTypeBasicAuth, - Data: testValidSecretData, - }}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind for the one valid upstream configuration. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -545,7 +531,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ Phase: "Ready", - Conditions: allConditionsTrue(1234), + Conditions: allConditionsTrue(1234, "4242"), }, }, }, @@ -553,11 +539,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { { name: "when testing the connection to the LDAP server fails then the upstream is not added to the cache", inputUpstreams: []runtime.Object{validUpstream}, - inputSecrets: []runtime.Object{&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, - Type: corev1.SecretTypeBasicAuth, - Data: testValidSecretData, - }}, + 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")) @@ -577,7 +559,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LastTransitionTime: now, Reason: "LDAPConnectionError", Message: fmt.Sprintf( - `could not successfully connect to "%s" and bind as user "%s: error binding as "%s": some bind error`, + `could not successfully connect to "%s" and bind as user "%s": error binding as "%s": some bind error`, testHost, testBindUsername, testBindUsername), ObservedGeneration: 1234, }, @@ -587,18 +569,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }}, }, { - name: "when the LDAP server connection was already validated for this resource generation, then do not validate it again", + name: "when the LDAP server connection was already validated for the current resource generation and secret version, then do not validate it again", inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { upstream.Generation = 1234 upstream.Status.Conditions = []v1alpha1.Condition{ - ldapConnectionValidTrueCondition(1234), + ldapConnectionValidTrueCondition(1234, "4242"), } })}, - inputSecrets: []runtime.Object{&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, - Type: corev1.SecretTypeBasicAuth, - Data: testValidSecretData, - }}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, 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. }, @@ -607,7 +585,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ Phase: "Ready", - Conditions: allConditionsTrue(1234), + Conditions: allConditionsTrue(1234, "4242"), }, }}, }, @@ -616,21 +594,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { 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! - }, + ldapConnectionValidTrueCondition(1233, "4242"), // older spec generation! } })}, - inputSecrets: []runtime.Object{&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, - Type: corev1.SecretTypeBasicAuth, - Data: testValidSecretData, - }}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -641,7 +608,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ Phase: "Ready", - Conditions: allConditionsTrue(1234), + Conditions: allConditionsTrue(1234, "4242"), }, }}, }, @@ -660,11 +627,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { }, } })}, - inputSecrets: []runtime.Object{&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: testSecretName, Namespace: testNamespace}, - Type: corev1.SecretTypeBasicAuth, - Data: testValidSecretData, - }}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -675,7 +638,30 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Status: v1alpha1.LDAPIdentityProviderStatus{ Phase: "Ready", - Conditions: allConditionsTrue(1234), + Conditions: allConditionsTrue(1234, "4242"), + }, + }}, + }, + { + 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", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { + upstream.Generation = 1234 + upstream.Status.Conditions = []v1alpha1.Condition{ + ldapConnectionValidTrueCondition(1234, "4241"), // same spec generation, old secret version + } + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, // newer secret version! + 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, "4242"), }, }}, },