From 7f99d784622ed14fab5c857b4c4f5b7ccb118907 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 7 Jan 2022 17:19:13 -0800 Subject: [PATCH 1/3] Fix bug where LDAP or AD status conditions were not updated correctly When the LDAP and AD IDP watcher controllers encountered an update error while trying to update the status conditions of the IDP resources, then they would drop the computed desired new value of the condition on the ground. Next time the controller ran it would not try to update the condition again because it wants to use the cached settings and had already forgotten the desired new value of the condition computed during the previous run of the controller. This would leave the outdated value of the condition on the IDP resource. This bug would manifest in CI as random failures in which the expected condition message and the actual condition message would refer to different versions numbers of the bind secret. The actual condition message would refer to an older version of the bind secret because the update failed and then the new desired message got dropped on the ground. This commit changes the in-memory caching strategy to also cache the computed condition messages, allowing the conditions to be updated on the IDP resource during future calls to Sync() in the case of a failed update. --- .../active_directory_upstream_watcher.go | 10 +- .../active_directory_upstream_watcher_test.go | 283 +++++++++++++++--- .../ldap_upstream_watcher.go | 10 +- .../ldap_upstream_watcher_test.go | 136 +++++++-- .../upstreamwatchers/upstream_watchers.go | 154 +++++----- 5 files changed, 436 insertions(+), 157 deletions(-) diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index c96d20a4..0396ec0f 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package activedirectoryupstreamwatcher implements a controller which watches ActiveDirectoryIdentityProviders. @@ -226,7 +226,7 @@ type UpstreamActiveDirectoryIdentityProviderICache interface { type activeDirectoryWatcherController struct { cache UpstreamActiveDirectoryIdentityProviderICache - validatedSecretVersionsCache upstreamwatchers.SecretVersionCacheI + validatedSecretVersionsCache upstreamwatchers.ValidatedSettingsCacheI ldapDialer upstreamldap.LDAPDialer client pinnipedclientset.Interface activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer @@ -243,8 +243,8 @@ func New( ) controllerlib.Controller { return newInternal( idpCache, - // start with an empty secretVersionCache - upstreamwatchers.NewSecretVersionCache(), + // start with an empty cache + upstreamwatchers.NewValidatedSettingsCache(), // nil means to use a real production dialer when creating objects to add to the cache nil, client, @@ -257,7 +257,7 @@ func New( // For test dependency injection purposes. func newInternal( idpCache UpstreamActiveDirectoryIdentityProviderICache, - validatedSecretVersionsCache upstreamwatchers.SecretVersionCacheI, + validatedSecretVersionsCache upstreamwatchers.ValidatedSettingsCacheI, ldapDialer upstreamldap.LDAPDialer, client pinnipedclientset.Interface, activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer, diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go index 76a35ca2..7777bfec 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package activedirectoryupstreamwatcher @@ -150,8 +150,8 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { const ( testNamespace = "test-namespace" - testResourceUID = "test-uid" testName = "test-name" + testResourceUID = "test-uid" testSecretName = "test-bind-secret" testBindUsername = "test-bind-username" testBindPassword = "test-bind-password" @@ -255,6 +255,19 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { ObservedGeneration: gen, } } + activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration := func(secretVersion string) v1alpha1.Condition { + c := activeDirectoryConnectionValidTrueCondition(0, secretVersion) + c.LastTransitionTime = metav1.Time{} + return c + } + condPtr := func(c v1alpha1.Condition) *v1alpha1.Condition { + return &c + } + withoutTime := func(c v1alpha1.Condition) v1alpha1.Condition { + c = *c.DeepCopy() + c.LastTransitionTime = metav1.Time{} + return c + } tlsConfigurationValidLoadedTrueCondition := func(gen int64) v1alpha1.Condition { return v1alpha1.Condition{ Type: "TLSConfigurationValid", @@ -377,7 +390,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "missing secret", @@ -568,7 +589,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "sAMAccountName explicitly provided as group name attribute does not add an override", @@ -629,7 +658,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "when TLS connection fails it tries to use StartTLS instead: without a specified port it automatically switches ports", @@ -695,7 +732,22 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.StartTLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: &v1alpha1.Condition{ + Type: "LDAPConnectionValid", + Status: "True", + 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"), + }, + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "when TLS connection fails it tries to use StartTLS instead: with a specified port it does not automatically switch ports", @@ -808,7 +860,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "one valid upstream and one invalid upstream updates the cache to include only the valid upstream", @@ -852,7 +912,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "when testing the connection to the LDAP server fails then the upstream is still added to the cache anyway but not to validatedsettings (treated like a warning)", @@ -1003,8 +1071,16 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { searchBaseFoundInConfigCondition(1234), } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, 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. }, @@ -1016,7 +1092,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "when the validated cache contains LDAP server info but the search base is empty, reload everything", @@ -1029,8 +1113,12 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { } upstream.Spec.UserSearch.Base = "" })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, Generation: 1234}}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + IDPSpecGeneration: 1234, + }}, setupMocks: func(conn *mockldapconn.MockConn) { conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(2) conn.EXPECT().Close().Times(2) @@ -1075,7 +1163,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: exampleDefaultNamingContext, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), + }}, }, { name: "when the LDAP server connection was already validated using TLS, and the search base was found, load TLS and search base info into the cache", @@ -1087,8 +1183,16 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { } upstream.Spec.UserSearch.Base = "" })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: exampleDefaultNamingContext, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), + }}, setupMocks: func(conn *mockldapconn.MockConn) { }, wantResultingCache: []*upstreamldap.ProviderConfig{ @@ -1136,7 +1240,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), }}, }, { @@ -1148,8 +1254,16 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { searchBaseFoundInConfigCondition(1234), } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.StartTLS, Generation: 1234, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase}}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.StartTLS, + IDPSpecGeneration: 1234, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, 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. }, @@ -1166,7 +1280,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.StartTLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), }}, }, { @@ -1183,7 +1299,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1233, + IDPSpecGeneration: 1233, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), }}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. @@ -1203,7 +1321,49 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, + }, + { + name: "when the LDAP server connection condition failed to update previously, then write the cached condition from the previous connection validation", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.ActiveDirectoryIdentityProvider) { + upstream.Generation = 1234 // current generation + upstream.Status.Conditions = []v1alpha1.Condition{ + activeDirectoryConnectionValidTrueCondition(1234, "4200"), // old version of the condition, as if the previous update of conditions had failed + } + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + IDPSpecGeneration: 1234, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), // already previously validated with version 4242 + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, + setupMocks: func(conn *mockldapconn.MockConn) { + // The connection had already been validated previously and the result was cached, so don't probe the server again. + // 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{providerConfigForValidUpstreamWithTLS}, + wantResultingUpstreams: []v1alpha1.ActiveDirectoryIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, UID: testResourceUID, Generation: 1234}, + Status: v1alpha1.ActiveDirectoryIdentityProviderStatus{ + Phase: "Ready", + Conditions: allConditionsTrue(1234, "4242"), // updated version of the condition using the cached condition value + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), }}, }, { @@ -1221,8 +1381,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, } })}, - inputSecrets: []runtime.Object{validBindUserSecret("4242")}, - initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "1", LDAPConnectionProtocol: upstreamldap.TLS, Generation: 1234}}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1) @@ -1241,7 +1400,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), }}, }, { @@ -1258,7 +1419,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4241")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), }}, // old version was validated setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. @@ -1273,13 +1436,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Conditions: allConditionsTrue(1234, "4242"), }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{ - testName: {BindSecretResourceVersion: "4242", - LDAPConnectionProtocol: upstreamldap.TLS, - UserSearchBase: testUserSearchBase, - GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), + }}, }, { name: "when the input activedirectoryidentityprovider leaves user attributes blank, provide default values", @@ -1336,7 +1501,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInConfigCondition(0))), }}, }, { @@ -1398,7 +1565,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: exampleDefaultNamingContext, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), }}, }, { @@ -1454,7 +1623,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: exampleDefaultNamingContext, GroupSearchBase: testGroupSearchBase, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: exampleDefaultNamingContext, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), + }}, }, { name: "when the input activedirectoryidentityprovider leaves group search base blank but provides user search base, query for defaultNamingContext", @@ -1509,7 +1686,15 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }, }, }}, - wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: {BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: exampleDefaultNamingContext, Generation: 1234}}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: exampleDefaultNamingContext, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), + }}, }, { name: "when the input activedirectoryidentityprovider leaves group search base blank and query for defaultNamingContext fails", @@ -1662,7 +1847,9 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4241")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), }}, setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. @@ -1712,10 +1899,12 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { }}, wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{ testName: {BindSecretResourceVersion: "4242", - LDAPConnectionProtocol: upstreamldap.TLS, - GroupSearchBase: exampleDefaultNamingContext, - UserSearchBase: testUserSearchBase, - Generation: 1234, + LDAPConnectionProtocol: upstreamldap.TLS, + GroupSearchBase: exampleDefaultNamingContext, + UserSearchBase: testUserSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(activeDirectoryConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + SearchBaseFoundCondition: condPtr(withoutTime(searchBaseFoundInRootDSECondition(0))), }}, }, } @@ -1752,20 +1941,20 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { return conn, nil })} - var validatedSecretVersionCache *upstreamwatchers.SecretVersionCache + var validatedSettingsCache *upstreamwatchers.ValidatedSettingsCache if tt.initialValidatedSettings != nil { - validatedSecretVersionCache = &upstreamwatchers.SecretVersionCache{ + validatedSettingsCache = &upstreamwatchers.ValidatedSettingsCache{ ValidatedSettingsByName: tt.initialValidatedSettings, } } else { - validatedSecretVersionCache = &upstreamwatchers.SecretVersionCache{ + validatedSettingsCache = &upstreamwatchers.ValidatedSettingsCache{ ValidatedSettingsByName: map[string]upstreamwatchers.ValidatedSettings{}, } } controller := newInternal( cache, - validatedSecretVersionCache, + validatedSettingsCache, dialer, fakePinnipedClient, pinnipedInformers.IDP().V1alpha1().ActiveDirectoryIdentityProviders(), @@ -1850,7 +2039,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { if tt.wantValidatedSettings == nil { tt.wantValidatedSettings = map[string]upstreamwatchers.ValidatedSettings{} } - require.Equal(t, tt.wantValidatedSettings, validatedSecretVersionCache.ValidatedSettingsByName) + require.Equal(t, tt.wantValidatedSettings, validatedSettingsCache.ValidatedSettingsByName) }) } } diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go index bcbd6d91..ac7f0002 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package ldapupstreamwatcher implements a controller which watches LDAPIdentityProviders. @@ -134,7 +134,7 @@ type UpstreamLDAPIdentityProviderICache interface { type ldapWatcherController struct { cache UpstreamLDAPIdentityProviderICache - validatedSecretVersionsCache upstreamwatchers.SecretVersionCacheI + validatedSecretVersionsCache upstreamwatchers.ValidatedSettingsCacheI ldapDialer upstreamldap.LDAPDialer client pinnipedclientset.Interface ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer @@ -151,8 +151,8 @@ func New( ) controllerlib.Controller { return newInternal( idpCache, - // start with an empty secretVersionCache - upstreamwatchers.NewSecretVersionCache(), + // start with an empty cache + upstreamwatchers.NewValidatedSettingsCache(), // nil means to use a real production dialer when creating objects to add to the cache nil, client, @@ -165,7 +165,7 @@ func New( // For test dependency injection purposes. func newInternal( idpCache UpstreamLDAPIdentityProviderICache, - validatedSecretVersionsCache upstreamwatchers.SecretVersionCacheI, + validatedSecretVersionsCache upstreamwatchers.ValidatedSettingsCacheI, ldapDialer upstreamldap.LDAPDialer, client pinnipedclientset.Interface, ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer, diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go index c562d969..d7f82edf 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package ldapupstreamwatcher @@ -253,6 +253,14 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ObservedGeneration: gen, } } + ldapConnectionValidTrueConditionWithoutTimeOrGeneration := func(secretVersion string) v1alpha1.Condition { + c := ldapConnectionValidTrueCondition(0, secretVersion) + c.LastTransitionTime = metav1.Time{} + return c + } + condPtr := func(c v1alpha1.Condition) *v1alpha1.Condition { + return &c + } tlsConfigurationValidLoadedTrueCondition := func(gen int64) v1alpha1.Condition { return v1alpha1.Condition{ Type: "TLSConfigurationValid", @@ -317,7 +325,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), }}, }, { @@ -507,8 +516,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, { 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) { @@ -571,8 +582,17 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.StartTLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, + IDPSpecGeneration: 1234, + ConnectionValidCondition: &v1alpha1.Condition{ + Type: "LDAPConnectionValid", + Status: "True", + 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"), + }, + }}, + }, { 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) { @@ -676,7 +696,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), }}, }, { @@ -726,8 +747,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, { name: "when testing the connection to the LDAP server fails then the upstream is still added to the cache anyway (treated like a warning) but not the validated settings cache", inputUpstreams: []runtime.Object{validUpstream}, @@ -773,10 +796,11 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { inputSecrets: []runtime.Object{validBindUserSecret("4242")}, initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{ testName: {BindSecretResourceVersion: "4242", - LDAPConnectionProtocol: upstreamldap.TLS, - UserSearchBase: testUserSearchBase, - GroupSearchBase: testGroupSearchBase, - Generation: 1234, + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("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. @@ -794,8 +818,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, { 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) { @@ -810,7 +836,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.StartTLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("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. @@ -828,8 +855,10 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.StartTLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, { 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) { @@ -842,7 +871,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { BindSecretResourceVersion: "4242", LDAPConnectionProtocol: upstreamldap.TLS, - Generation: 1233, + IDPSpecGeneration: 1233, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, }}, @@ -864,8 +893,48 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, + { + name: "when the LDAP server connection condition failed to update previously, then write the cached condition from the previous connection validation", + inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { + upstream.Generation = 1234 // current generation + upstream.Status.Conditions = []v1alpha1.Condition{ + ldapConnectionValidTrueCondition(1234, "4200"), // old version of the condition, as if the previous update of conditions had failed + } + })}, + inputSecrets: []runtime.Object{validBindUserSecret("4242")}, + initialValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + IDPSpecGeneration: 1234, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), // already previously validated with version 4242 + }}, + setupMocks: func(conn *mockldapconn.MockConn) { + // The connection had already been validated previously and the result was cached, so don't probe the server again. + // 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{providerConfigForValidUpstreamWithTLS}, + wantResultingUpstreams: []v1alpha1.LDAPIdentityProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234, UID: testResourceUID}, + Status: v1alpha1.LDAPIdentityProviderStatus{ + Phase: "Ready", + Conditions: allConditionsTrue(1234, "4242"), // updated version of the condition using the cached condition value + }, + }}, + wantValidatedSettings: map[string]upstreamwatchers.ValidatedSettings{testName: { + BindSecretResourceVersion: "4242", + LDAPConnectionProtocol: upstreamldap.TLS, + UserSearchBase: testUserSearchBase, + GroupSearchBase: testGroupSearchBase, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, { 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) { @@ -900,8 +969,11 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, - }}}, { + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), + }}, + }, + { name: "when the validated settings cache is incomplete, then try to validate it again", // this shouldn't happen, but if it does, just throw it out and try again. inputUpstreams: []runtime.Object{editedValidUpstream(func(upstream *v1alpha1.LDAPIdentityProvider) { @@ -940,7 +1012,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), }}, }, { @@ -957,7 +1030,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, }}, // old version was validated setupMocks: func(conn *mockldapconn.MockConn) { // Should perform a test dial and bind. @@ -977,7 +1050,8 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { LDAPConnectionProtocol: upstreamldap.TLS, UserSearchBase: testUserSearchBase, GroupSearchBase: testGroupSearchBase, - Generation: 1234, + IDPSpecGeneration: 1234, + ConnectionValidCondition: condPtr(ldapConnectionValidTrueConditionWithoutTimeOrGeneration("4242")), }}}, } @@ -1013,20 +1087,20 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { return conn, nil })} - var validatedSecretVersionCache *upstreamwatchers.SecretVersionCache + var validatedSettingsCache *upstreamwatchers.ValidatedSettingsCache if tt.initialValidatedSettings != nil { - validatedSecretVersionCache = &upstreamwatchers.SecretVersionCache{ + validatedSettingsCache = &upstreamwatchers.ValidatedSettingsCache{ ValidatedSettingsByName: tt.initialValidatedSettings, } } else { - validatedSecretVersionCache = &upstreamwatchers.SecretVersionCache{ + validatedSettingsCache = &upstreamwatchers.ValidatedSettingsCache{ ValidatedSettingsByName: map[string]upstreamwatchers.ValidatedSettings{}, } } controller := newInternal( cache, - validatedSecretVersionCache, + validatedSettingsCache, dialer, fakePinnipedClient, pinnipedInformers.IDP().V1alpha1().LDAPIdentityProviders(), @@ -1075,7 +1149,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { if tt.wantValidatedSettings == nil { tt.wantValidatedSettings = map[string]upstreamwatchers.ValidatedSettings{} } - require.Equal(t, tt.wantValidatedSettings, validatedSecretVersionCache.ValidatedSettingsByName) + require.Equal(t, tt.wantValidatedSettings, validatedSettingsCache.ValidatedSettingsByName) }) } } diff --git a/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go b/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go index 60d09eba..73b7b575 100644 --- a/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go +++ b/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package upstreamwatchers @@ -44,45 +44,61 @@ const ( ReasonErrorFetchingSearchBase = "ErrorFetchingSearchBase" ) -// An in-memory cache with an entry for each ActiveDirectoryIdentityProvider, to keep track of which ResourceVersion -// of the bind Secret, which TLS/StartTLS setting was used and which search base was found during the most recent successful validation. -type SecretVersionCacheI interface { - Get(upstreamName, resourceVersion string, generation int64) (ValidatedSettings, bool) - Set(upstreamName, resourceVersion string, generation int64, settings ValidatedSettings) +// ValidatedSettings is the struct which is cached by the ValidatedSettingsCacheI interface. +type ValidatedSettings struct { + IDPSpecGeneration int64 // which IDP spec was used during the validation + BindSecretResourceVersion string // which bind secret was used during the validation + + // Cache the setting for TLS vs StartTLS. This is always auto-discovered by probing the server. + LDAPConnectionProtocol upstreamldap.LDAPConnectionProtocol + + // Cache the settings for search bases. These could be configured by the IDP spec, or in the + // case of AD they can also be auto-discovered by probing the server. + UserSearchBase, GroupSearchBase string + + // Cache copies of the conditions that were computed when the above settings were cached, so we + // can keep writing them to the status in the future. This matters most when the first attempt + // to write them to the IDP's status fails. In this case, future Syncs calls will be able to + // use these cached values to try writing them again. + ConnectionValidCondition, SearchBaseFoundCondition *v1alpha1.Condition } -type SecretVersionCache struct { +// ValidatedSettingsCacheI is an interface for an in-memory cache with an entry for each upstream +// provider. It keeps track of settings that were already validated for a given IDP spec and bind +// secret for that upstream. +type ValidatedSettingsCacheI interface { + // Get the cached settings for a given upstream at a given generation which was previously + // validated using a given bind secret version. If no settings have been cached for the + // upstream, or if the settings were cached at a different generation of the upstream or + // using a different version of the bind secret, then return false to indicate that the + // desired settings were not cached yet for that combination of spec generation and secret version. + Get(upstreamName, resourceVersion string, idpSpecGeneration int64) (ValidatedSettings, bool) + + // Set some settings into the cache for a given upstream. + Set(upstreamName string, settings ValidatedSettings) +} + +type ValidatedSettingsCache struct { ValidatedSettingsByName map[string]ValidatedSettings } -func (s *SecretVersionCache) Get(upstreamName, resourceVersion string, generation int64) (ValidatedSettings, bool) { - validatedSettings := s.ValidatedSettingsByName[upstreamName] - if validatedSettings.BindSecretResourceVersion == resourceVersion && - validatedSettings.Generation == generation && validatedSettings.UserSearchBase != "" && - validatedSettings.GroupSearchBase != "" && validatedSettings.LDAPConnectionProtocol != "" { +func NewValidatedSettingsCache() ValidatedSettingsCacheI { + return &ValidatedSettingsCache{ValidatedSettingsByName: map[string]ValidatedSettings{}} +} + +func (s *ValidatedSettingsCache) Get(upstreamName, resourceVersion string, idpSpecGeneration int64) (ValidatedSettings, bool) { + validatedSettings, found := s.ValidatedSettingsByName[upstreamName] + if found && validatedSettings.BindSecretResourceVersion == resourceVersion && validatedSettings.IDPSpecGeneration == idpSpecGeneration { return validatedSettings, true } return ValidatedSettings{}, false } -func (s *SecretVersionCache) Set(upstreamName, resourceVersion string, generation int64, settings ValidatedSettings) { +func (s *ValidatedSettingsCache) Set(upstreamName string, settings ValidatedSettings) { s.ValidatedSettingsByName[upstreamName] = settings } -type ValidatedSettings struct { - Generation int64 - BindSecretResourceVersion string - LDAPConnectionProtocol upstreamldap.LDAPConnectionProtocol - UserSearchBase string - GroupSearchBase string -} - -func NewSecretVersionCache() SecretVersionCacheI { - cache := SecretVersionCache{ValidatedSettingsByName: map[string]ValidatedSettings{}} - return &cache -} - -// read only interface for sharing between ldap and active directory. +// UpstreamGenericLDAPIDP is a read-only interface for abstracting the differences between LDAP and Active Directory IDP types. type UpstreamGenericLDAPIDP interface { Spec() UpstreamGenericLDAPSpec Name() string @@ -247,8 +263,15 @@ func ValidateSecret(secretInformer corev1informers.SecretInformer, secretName st }, secret.ResourceVersion } +// gradatedCondition is a condition and a boolean that tells you whether the condition is fatal or just a warning. +type gradatedCondition struct { + condition *v1alpha1.Condition + isFatal bool +} + +// GradatedConditions is a list of conditions, where each condition can additionally be considered fatal or non-fatal. type GradatedConditions struct { - gradatedConditions []GradatedCondition + gradatedConditions []gradatedCondition } func (g *GradatedConditions) Conditions() []*v1alpha1.Condition { @@ -260,75 +283,68 @@ func (g *GradatedConditions) Conditions() []*v1alpha1.Condition { } func (g *GradatedConditions) Append(condition *v1alpha1.Condition, isFatal bool) { - g.gradatedConditions = append(g.gradatedConditions, GradatedCondition{condition: condition, isFatal: isFatal}) + g.gradatedConditions = append(g.gradatedConditions, gradatedCondition{condition: condition, isFatal: isFatal}) } -// A condition and a boolean that tells you whether it's fatal or just a warning. -type GradatedCondition struct { - condition *v1alpha1.Condition - isFatal bool -} - -func ValidateGenericLDAP(ctx context.Context, upstream UpstreamGenericLDAPIDP, secretInformer corev1informers.SecretInformer, validatedSecretVersionsCache SecretVersionCacheI, config *upstreamldap.ProviderConfig) GradatedConditions { +func ValidateGenericLDAP(ctx context.Context, upstream UpstreamGenericLDAPIDP, secretInformer corev1informers.SecretInformer, validatedSecretVersionsCache ValidatedSettingsCacheI, config *upstreamldap.ProviderConfig) GradatedConditions { conditions := GradatedConditions{} + secretValidCondition, currentSecretVersion := ValidateSecret(secretInformer, upstream.Spec().BindSecretName(), upstream.Namespace(), config) conditions.Append(secretValidCondition, true) + tlsValidCondition := ValidateTLSConfig(upstream.Spec().TLSSpec(), config) conditions.Append(tlsValidCondition, true) + var ldapConnectionValidCondition, searchBaseFoundCondition *v1alpha1.Condition // No point in trying to connect to the server if the config was already determined to be invalid. - var ldapConnectionValidCondition *v1alpha1.Condition - var searchBaseFoundCondition *v1alpha1.Condition if secretValidCondition.Status == v1alpha1.ConditionTrue && tlsValidCondition.Status == v1alpha1.ConditionTrue { ldapConnectionValidCondition, searchBaseFoundCondition = validateAndSetLDAPServerConnectivityAndSearchBase(ctx, validatedSecretVersionsCache, upstream, config, currentSecretVersion) - if ldapConnectionValidCondition != nil { - conditions.Append(ldapConnectionValidCondition, false) - } - if searchBaseFoundCondition != nil { + conditions.Append(ldapConnectionValidCondition, false) + if searchBaseFoundCondition != nil { // currently, only used for AD, so may be nil conditions.Append(searchBaseFoundCondition, true) } } return conditions } -func validateAndSetLDAPServerConnectivityAndSearchBase(ctx context.Context, validatedSecretVersionsCache SecretVersionCacheI, upstream UpstreamGenericLDAPIDP, config *upstreamldap.ProviderConfig, currentSecretVersion string) (*v1alpha1.Condition, *v1alpha1.Condition) { - // previouslyValidatedSecretVersion := validatedSecretVersionsCache.ValidatedSettingsByName[upstream.Name()].BindSecretResourceVersion - // doesn't have an existing entry for ValidatedSettingsByName with this secret version -> - // lets double check tls connection - // if we can connect, put it in the secret cache - // also we KNOW we need to recheck the search base stuff too... so they should all be one function? - // but if tls validation fails no need to also try to get search base stuff? - +func validateAndSetLDAPServerConnectivityAndSearchBase(ctx context.Context, validatedSecretVersionsCache ValidatedSettingsCacheI, upstream UpstreamGenericLDAPIDP, config *upstreamldap.ProviderConfig, currentSecretVersion string) (*v1alpha1.Condition, *v1alpha1.Condition) { validatedSettings, hasPreviousValidatedSettings := validatedSecretVersionsCache.Get(upstream.Name(), currentSecretVersion, upstream.Generation()) var ldapConnectionValidCondition, searchBaseFoundCondition *v1alpha1.Condition - if !hasPreviousValidatedSettings { + + if hasPreviousValidatedSettings && validatedSettings.UserSearchBase != "" && validatedSettings.GroupSearchBase != "" { + // Found previously validated settings in the cache (which is also not missing search base fields), so use them. + config.ConnectionProtocol = validatedSettings.LDAPConnectionProtocol + config.UserSearch.Base = validatedSettings.UserSearchBase + config.GroupSearch.Base = validatedSettings.GroupSearchBase + ldapConnectionValidCondition = validatedSettings.ConnectionValidCondition.DeepCopy() + searchBaseFoundCondition = validatedSettings.SearchBaseFoundCondition.DeepCopy() + } else { + // Did not find previously validated settings in the cache, so probe the LDAP server. testConnectionTimeout, cancelFunc := context.WithTimeout(ctx, probeLDAPTimeout) defer cancelFunc() - ldapConnectionValidCondition = TestConnection(testConnectionTimeout, upstream.Spec().BindSecretName(), config, currentSecretVersion) searchBaseTimeout, cancelFunc := context.WithTimeout(ctx, probeLDAPTimeout) defer cancelFunc() searchBaseFoundCondition = upstream.Spec().DetectAndSetSearchBase(searchBaseTimeout, config) - if ldapConnectionValidCondition.Status == v1alpha1.ConditionTrue { - // if it's nil, don't worry about the search base condition. But if it exists make sure the status is true. - if searchBaseFoundCondition == nil || (searchBaseFoundCondition.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. - validatedSettings.LDAPConnectionProtocol = config.ConnectionProtocol - validatedSettings.BindSecretResourceVersion = currentSecretVersion - validatedSettings.Generation = upstream.Generation() - validatedSettings.UserSearchBase = config.UserSearch.Base - validatedSettings.GroupSearchBase = config.GroupSearch.Base - validatedSecretVersionsCache.Set(upstream.Name(), currentSecretVersion, upstream.Generation(), validatedSettings) - } + // When there were no failures, write the newly validated settings to the cache. + // It's okay for the search base condition to be nil, but if it exists make sure it was not a failure. + if ldapConnectionValidCondition.Status == v1alpha1.ConditionTrue && + (searchBaseFoundCondition == nil || (searchBaseFoundCondition.Status == v1alpha1.ConditionTrue)) { + // Remember (in-memory for this pod) that the controller has successfully validated the LDAP or AD 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. + validatedSecretVersionsCache.Set(upstream.Name(), ValidatedSettings{ + IDPSpecGeneration: upstream.Generation(), + BindSecretResourceVersion: currentSecretVersion, + LDAPConnectionProtocol: config.ConnectionProtocol, + UserSearchBase: config.UserSearch.Base, + GroupSearchBase: config.GroupSearch.Base, + ConnectionValidCondition: ldapConnectionValidCondition.DeepCopy(), + SearchBaseFoundCondition: searchBaseFoundCondition.DeepCopy(), // currently, only used for AD, so may be nil + }) } - } else { - config.ConnectionProtocol = validatedSettings.LDAPConnectionProtocol - config.UserSearch.Base = validatedSettings.UserSearchBase - config.GroupSearch.Base = validatedSettings.GroupSearchBase } return ldapConnectionValidCondition, searchBaseFoundCondition From 438b58193d74301feffda9a513cfd71d5d17668f Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 10 Jan 2022 13:47:13 -0800 Subject: [PATCH 2/3] Empty commit to trigger CI From 092a80f849ec39bfed3dc3590cbf8557a04b750c Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 14 Jan 2022 10:06:00 -0800 Subject: [PATCH 3/3] Refactor some variable names and update one comment Change variable names to match previously renamed interface name. --- .../active_directory_upstream_watcher.go | 8 +++--- .../ldap_upstream_watcher.go | 8 +++--- .../upstreamwatchers/upstream_watchers.go | 25 ++++++++++++++----- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index 0396ec0f..33d1557a 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -226,7 +226,7 @@ type UpstreamActiveDirectoryIdentityProviderICache interface { type activeDirectoryWatcherController struct { cache UpstreamActiveDirectoryIdentityProviderICache - validatedSecretVersionsCache upstreamwatchers.ValidatedSettingsCacheI + validatedSettingsCache upstreamwatchers.ValidatedSettingsCacheI ldapDialer upstreamldap.LDAPDialer client pinnipedclientset.Interface activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer @@ -257,7 +257,7 @@ func New( // For test dependency injection purposes. func newInternal( idpCache UpstreamActiveDirectoryIdentityProviderICache, - validatedSecretVersionsCache upstreamwatchers.ValidatedSettingsCacheI, + validatedSettingsCache upstreamwatchers.ValidatedSettingsCacheI, ldapDialer upstreamldap.LDAPDialer, client pinnipedclientset.Interface, activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer, @@ -266,7 +266,7 @@ func newInternal( ) controllerlib.Controller { c := activeDirectoryWatcherController{ cache: idpCache, - validatedSecretVersionsCache: validatedSecretVersionsCache, + validatedSettingsCache: validatedSettingsCache, ldapDialer: ldapDialer, client: client, activeDirectoryIdentityProviderInformer: activeDirectoryIdentityProviderInformer, @@ -351,7 +351,7 @@ func (c *activeDirectoryWatcherController) validateUpstream(ctx context.Context, } } - conditions := upstreamwatchers.ValidateGenericLDAP(ctx, adUpstreamImpl, c.secretInformer, c.validatedSecretVersionsCache, config) + conditions := upstreamwatchers.ValidateGenericLDAP(ctx, adUpstreamImpl, c.secretInformer, c.validatedSettingsCache, config) c.updateStatus(ctx, upstream, conditions.Conditions()) diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go index ac7f0002..82a9fdec 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go @@ -134,7 +134,7 @@ type UpstreamLDAPIdentityProviderICache interface { type ldapWatcherController struct { cache UpstreamLDAPIdentityProviderICache - validatedSecretVersionsCache upstreamwatchers.ValidatedSettingsCacheI + validatedSettingsCache upstreamwatchers.ValidatedSettingsCacheI ldapDialer upstreamldap.LDAPDialer client pinnipedclientset.Interface ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer @@ -165,7 +165,7 @@ func New( // For test dependency injection purposes. func newInternal( idpCache UpstreamLDAPIdentityProviderICache, - validatedSecretVersionsCache upstreamwatchers.ValidatedSettingsCacheI, + validatedSettingsCache upstreamwatchers.ValidatedSettingsCacheI, ldapDialer upstreamldap.LDAPDialer, client pinnipedclientset.Interface, ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer, @@ -174,7 +174,7 @@ func newInternal( ) controllerlib.Controller { c := ldapWatcherController{ cache: idpCache, - validatedSecretVersionsCache: validatedSecretVersionsCache, + validatedSettingsCache: validatedSettingsCache, ldapDialer: ldapDialer, client: client, ldapIdentityProviderInformer: ldapIdentityProviderInformer, @@ -243,7 +243,7 @@ func (c *ldapWatcherController) validateUpstream(ctx context.Context, upstream * Dialer: c.ldapDialer, } - conditions := upstreamwatchers.ValidateGenericLDAP(ctx, &ldapUpstreamGenericLDAPImpl{*upstream}, c.secretInformer, c.validatedSecretVersionsCache, config) + conditions := upstreamwatchers.ValidateGenericLDAP(ctx, &ldapUpstreamGenericLDAPImpl{*upstream}, c.secretInformer, c.validatedSettingsCache, config) c.updateStatus(ctx, upstream, conditions.Conditions()) diff --git a/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go b/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go index 73b7b575..5c33ea2d 100644 --- a/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go +++ b/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go @@ -286,7 +286,13 @@ func (g *GradatedConditions) Append(condition *v1alpha1.Condition, isFatal bool) g.gradatedConditions = append(g.gradatedConditions, gradatedCondition{condition: condition, isFatal: isFatal}) } -func ValidateGenericLDAP(ctx context.Context, upstream UpstreamGenericLDAPIDP, secretInformer corev1informers.SecretInformer, validatedSecretVersionsCache ValidatedSettingsCacheI, config *upstreamldap.ProviderConfig) GradatedConditions { +func ValidateGenericLDAP( + ctx context.Context, + upstream UpstreamGenericLDAPIDP, + secretInformer corev1informers.SecretInformer, + validatedSettingsCache ValidatedSettingsCacheI, + config *upstreamldap.ProviderConfig, +) GradatedConditions { conditions := GradatedConditions{} secretValidCondition, currentSecretVersion := ValidateSecret(secretInformer, upstream.Spec().BindSecretName(), upstream.Namespace(), config) @@ -298,7 +304,7 @@ func ValidateGenericLDAP(ctx context.Context, upstream UpstreamGenericLDAPIDP, s var ldapConnectionValidCondition, searchBaseFoundCondition *v1alpha1.Condition // 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 { - ldapConnectionValidCondition, searchBaseFoundCondition = validateAndSetLDAPServerConnectivityAndSearchBase(ctx, validatedSecretVersionsCache, upstream, config, currentSecretVersion) + ldapConnectionValidCondition, searchBaseFoundCondition = validateAndSetLDAPServerConnectivityAndSearchBase(ctx, validatedSettingsCache, upstream, config, currentSecretVersion) conditions.Append(ldapConnectionValidCondition, false) if searchBaseFoundCondition != nil { // currently, only used for AD, so may be nil conditions.Append(searchBaseFoundCondition, true) @@ -307,8 +313,14 @@ func ValidateGenericLDAP(ctx context.Context, upstream UpstreamGenericLDAPIDP, s return conditions } -func validateAndSetLDAPServerConnectivityAndSearchBase(ctx context.Context, validatedSecretVersionsCache ValidatedSettingsCacheI, upstream UpstreamGenericLDAPIDP, config *upstreamldap.ProviderConfig, currentSecretVersion string) (*v1alpha1.Condition, *v1alpha1.Condition) { - validatedSettings, hasPreviousValidatedSettings := validatedSecretVersionsCache.Get(upstream.Name(), currentSecretVersion, upstream.Generation()) +func validateAndSetLDAPServerConnectivityAndSearchBase( + ctx context.Context, + validatedSettingsCache ValidatedSettingsCacheI, + upstream UpstreamGenericLDAPIDP, + config *upstreamldap.ProviderConfig, + currentSecretVersion string, +) (*v1alpha1.Condition, *v1alpha1.Condition) { + validatedSettings, hasPreviousValidatedSettings := validatedSettingsCache.Get(upstream.Name(), currentSecretVersion, upstream.Generation()) var ldapConnectionValidCondition, searchBaseFoundCondition *v1alpha1.Condition if hasPreviousValidatedSettings && validatedSettings.UserSearchBase != "" && validatedSettings.GroupSearchBase != "" { @@ -329,13 +341,14 @@ func validateAndSetLDAPServerConnectivityAndSearchBase(ctx context.Context, vali searchBaseFoundCondition = upstream.Spec().DetectAndSetSearchBase(searchBaseTimeout, config) // When there were no failures, write the newly validated settings to the cache. - // It's okay for the search base condition to be nil, but if it exists make sure it was not a failure. + // It's okay for the search base condition to be nil, since it's only used by Active Directory providers, + // but if it exists make sure it was not a failure. if ldapConnectionValidCondition.Status == v1alpha1.ConditionTrue && (searchBaseFoundCondition == nil || (searchBaseFoundCondition.Status == v1alpha1.ConditionTrue)) { // Remember (in-memory for this pod) that the controller has successfully validated the LDAP or AD 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. - validatedSecretVersionsCache.Set(upstream.Name(), ValidatedSettings{ + validatedSettingsCache.Set(upstream.Name(), ValidatedSettings{ IDPSpecGeneration: upstream.Generation(), BindSecretResourceVersion: currentSecretVersion, LDAPConnectionProtocol: config.ConnectionProtocol,