diff --git a/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/upstreamwatcher/ldap_upstream_watcher.go index fd76095a..6a0796eb 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 c33ceebd..b8af6d0c 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"), }, }}, },