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.
This commit is contained in:
Ryan Richard 2021-04-15 17:45:15 -07:00
parent 8e438e22e9
commit 83085aa3d6
2 changed files with 105 additions and 100 deletions

View File

@ -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,14 +174,14 @@ 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)
@ -184,7 +190,8 @@ func (c *ldapWatcherController) validateFinishedConfig(ctx context.Context, upst
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()),
Message: fmt.Sprintf(`could not successfully connect to "%s" and bind as user "%s": %s`,
config.Host, config.BindUsername, err.Error()),
}
}
@ -192,17 +199,27 @@ func (c *ldapWatcherController) validateFinishedConfig(ctx context.Context, upst
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),
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 {
// 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,7 +251,7 @@ func (c *ldapWatcherController) validateSecret(upstream *v1alpha1.LDAPIdentityPr
Status: v1alpha1.ConditionFalse,
Reason: reasonNotFound,
Message: err.Error(),
}
}, ""
}
if secret.Type != corev1.SecretTypeBasicAuth {
@ -242,8 +259,9 @@ func (c *ldapWatcherController) validateSecret(upstream *v1alpha1.LDAPIdentityPr
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),
}
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])
@ -253,8 +271,9 @@ func (c *ldapWatcherController) validateSecret(upstream *v1alpha1.LDAPIdentityPr
Type: typeBindSecretValid,
Status: v1alpha1.ConditionFalse,
Reason: reasonMissingKeys,
Message: fmt.Sprintf("referenced Secret %q is missing required keys %q", secretName, []string{corev1.BasicAuthUsernameKey, corev1.BasicAuthPasswordKey}),
}
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 {

View File

@ -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"),
},
}},
},