From afa3aa22324b24da18e60ed10005aeffad95ee37 Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Tue, 6 Aug 2024 13:03:00 -0500 Subject: [PATCH] LDAP and AD IDPs now always report condition with type LDAPConnectionValid, even if the status is unknown Co-authored-by: Ryan Richard --- .../jwtcachefiller/jwtcachefiller.go | 10 ++++------ .../webhookcachefiller/webhookcachefiller.go | 6 ++---- .../conditionsutil/conditions_util.go | 4 +++- .../active_directory_upstream_watcher.go | 8 -------- .../active_directory_upstream_watcher_test.go | 17 +++++++++++++++++ .../github_upstream_watcher.go | 2 +- .../ldap_upstream_watcher.go | 4 ---- .../ldap_upstream_watcher_test.go | 18 ++++++++++++++++++ .../oidc_upstream_watcher.go | 6 +----- .../upstreamwatchers/upstream_watchers.go | 15 ++++++++++++--- 10 files changed, 58 insertions(+), 32 deletions(-) diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index 297c0fbd1..64ec4ffc1 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go @@ -64,8 +64,6 @@ const ( reasonInvalidAuthenticator = "InvalidAuthenticator" reasonInvalidCouldNotFetchJWKS = "InvalidCouldNotFetchJWKS" - msgUnableToValidate = "unable to validate; see other conditions for details" - // These default values come from the way that the Supervisor issues and signs tokens. We make these // the defaults for a JWTAuthenticator so that they can easily integrate with the Supervisor. defaultUsernameClaim = oidcapi.IDTokenClaimUsername @@ -462,7 +460,7 @@ func (c *jwtCacheFillerController) validateProviderDiscovery(ctx context.Context Type: typeDiscoveryValid, Status: metav1.ConditionUnknown, Reason: conditionsutil.ReasonUnableToValidate, - Message: msgUnableToValidate, + Message: conditionsutil.MessageUnableToValidate, }) return nil, nil, conditions, nil } @@ -500,7 +498,7 @@ func (c *jwtCacheFillerController) validateProviderJWKSURL(provider *coreosoidc. Type: typeJWKSURLValid, Status: metav1.ConditionUnknown, Reason: conditionsutil.ReasonUnableToValidate, - Message: msgUnableToValidate, + Message: conditionsutil.MessageUnableToValidate, }) return "", conditions, nil } @@ -567,7 +565,7 @@ func (c *jwtCacheFillerController) validateJWKSFetch(ctx context.Context, jwksUR Type: typeJWKSFetchValid, Status: metav1.ConditionUnknown, Reason: conditionsutil.ReasonUnableToValidate, - Message: msgUnableToValidate, + Message: conditionsutil.MessageUnableToValidate, }) return nil, conditions, nil } @@ -646,7 +644,7 @@ func (c *jwtCacheFillerController) newCachedJWTAuthenticator( Type: typeAuthenticatorValid, Status: metav1.ConditionUnknown, Reason: conditionsutil.ReasonUnableToValidate, - Message: msgUnableToValidate, + Message: conditionsutil.MessageUnableToValidate, }) return nil, conditions, nil } diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index 2361381cc..f80dad113 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go @@ -54,8 +54,6 @@ const ( reasonUnableToInstantiateWebhook = "UnableToInstantiateWebhook" reasonInvalidEndpointURL = "InvalidEndpointURL" reasonInvalidEndpointURLScheme = "InvalidEndpointURLScheme" - - msgUnableToValidate = "unable to validate; see other conditions for details" ) type cachedWebhookAuthenticator struct { @@ -344,7 +342,7 @@ func newWebhookAuthenticator( Type: typeAuthenticatorValid, Status: metav1.ConditionUnknown, Reason: conditionsutil.ReasonUnableToValidate, - Message: msgUnableToValidate, + Message: conditionsutil.MessageUnableToValidate, }) return nil, conditions, nil } @@ -425,7 +423,7 @@ func (c *webhookCacheFillerController) validateConnection( Type: typeWebhookConnectionValid, Status: metav1.ConditionUnknown, Reason: conditionsutil.ReasonUnableToValidate, - Message: msgUnableToValidate, + Message: conditionsutil.MessageUnableToValidate, }) return conditions, nil } diff --git a/internal/controller/conditionsutil/conditions_util.go b/internal/controller/conditionsutil/conditions_util.go index 91bc24c63..fc90c3986 100644 --- a/internal/controller/conditionsutil/conditions_util.go +++ b/internal/controller/conditionsutil/conditions_util.go @@ -12,13 +12,15 @@ import ( "go.pinniped.dev/internal/plog" ) -// Some common reasons shared by conditions of various resources. +// Some common reasons and messages shared by conditions of various resources. const ( ReasonSuccess = "Success" ReasonNotReady = "NotReady" ReasonUnableToValidate = "UnableToValidate" ReasonUnableToDialServer = "UnableToDialServer" ReasonInvalidIssuerURL = "InvalidIssuerURL" + + MessageUnableToValidate = "unable to validate; see other conditions for details" ) // MergeConditions merges conditions into conditionsToUpdate. diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index cba7d0bd0..2c5dfffec 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go @@ -212,14 +212,6 @@ func (g *activeDirectoryUpstreamGenericLDAPGroupSearch) GroupNameAttribute() str return g.groupSearch.Attributes.GroupName } -type activeDirectoryUpstreamGenericLDAPStatus struct { - activeDirectoryIdentityProvider idpv1alpha1.ActiveDirectoryIdentityProvider -} - -func (s *activeDirectoryUpstreamGenericLDAPStatus) Conditions() []metav1.Condition { - return s.activeDirectoryIdentityProvider.Status.Conditions -} - // UpstreamActiveDirectoryIdentityProviderICache is a thread safe cache that holds a list of validated upstream LDAP IDP configurations. type UpstreamActiveDirectoryIdentityProviderICache interface { SetActiveDirectoryIdentityProviders([]upstreamprovider.UpstreamLDAPIdentityProviderI) 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 42cee8d20..f9537f034 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go @@ -402,6 +402,17 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { } } + ldapConnectionValidUnknown := func(gen int64) metav1.Condition { + return metav1.Condition{ + Type: "LDAPConnectionValid", + Status: "Unknown", + LastTransitionTime: now, + Reason: "UnableToValidate", + Message: "unable to validate; see other conditions for details", + ObservedGeneration: gen, + } + } + searchBaseFoundInRootDSECondition := func(gen int64) metav1.Condition { return metav1.Condition{ Type: "SearchBaseFound", @@ -663,6 +674,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`secret "%s" not found`, testBindSecretName), ObservedGeneration: 1234, }, + ldapConnectionValidUnknown(1234), tlsConfigurationValidLoadedTrueCondition(1234, "using configured CA bundle"), }, }, @@ -691,6 +703,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`referenced Secret "%s" has wrong type "some-other-type" (should be "kubernetes.io/basic-auth")`, testBindSecretName), ObservedGeneration: 1234, }, + ldapConnectionValidUnknown(1234), tlsConfigurationValidLoadedTrueCondition(1234, "using configured CA bundle"), }, }, @@ -718,6 +731,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`referenced Secret "%s" is missing required keys ["username" "password"]`, testBindSecretName), ObservedGeneration: 1234, }, + ldapConnectionValidUnknown(1234), tlsConfigurationValidLoadedTrueCondition(1234, "using configured CA bundle"), }, }, @@ -737,6 +751,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Phase: "Error", Conditions: []metav1.Condition{ bindSecretValidTrueCondition(1234), + ldapConnectionValidUnknown(1234), { Type: "TLSConfigurationValid", Status: "False", @@ -763,6 +778,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Phase: "Error", Conditions: []metav1.Condition{ bindSecretValidTrueCondition(1234), + ldapConnectionValidUnknown(1234), { Type: "TLSConfigurationValid", Status: "False", @@ -1158,6 +1174,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`secret "%s" not found`, "non-existent-secret"), ObservedGeneration: 42, }, + ldapConnectionValidUnknown(42), tlsConfigurationValidLoadedTrueCondition(42, "using configured CA bundle"), }, }, diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go index c5b1a4ead..1f8236cac 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go @@ -464,7 +464,7 @@ func (c *gitHubWatcherController) validateGitHubConnection( Type: GitHubConnectionValid, Status: metav1.ConditionUnknown, Reason: conditionsutil.ReasonUnableToValidate, - Message: "unable to validate; see other conditions for details", + Message: conditionsutil.MessageUnableToValidate, }, nil, nil } diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go index c56f1f76f..213816ab5 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go @@ -120,10 +120,6 @@ func (g *ldapUpstreamGenericLDAPGroupSearch) GroupNameAttribute() string { return g.groupSearch.Attributes.GroupName } -type ldapUpstreamGenericLDAPStatus struct { - ldapIdentityProvider idpv1alpha1.LDAPIdentityProvider -} - // UpstreamLDAPIdentityProviderICache is a thread safe cache that holds a list of validated upstream LDAP IDP configurations. type UpstreamLDAPIdentityProviderICache interface { SetLDAPIdentityProviders([]upstreamprovider.UpstreamLDAPIdentityProviderI) diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go index f529e9182..31647db6d 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go @@ -393,6 +393,18 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { ObservedGeneration: gen, } } + + ldapConnectionValidUnknown := func(gen int64) metav1.Condition { + return metav1.Condition{ + Type: "LDAPConnectionValid", + Status: "Unknown", + LastTransitionTime: now, + Reason: "UnableToValidate", + Message: "unable to validate; see other conditions for details", + ObservedGeneration: gen, + } + } + allConditionsTrue := func(gen int64, secretVersion string) []metav1.Condition { return []metav1.Condition{ bindSecretValidTrueCondition(gen), @@ -588,6 +600,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`secret "%s" not found`, testBindSecretName), ObservedGeneration: 1234, }, + ldapConnectionValidUnknown(1234), tlsConfigurationValidLoadedTrueCondition(1234, "using configured CA bundle"), }, }, @@ -616,6 +629,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`referenced Secret "%s" has wrong type "some-other-type" (should be "kubernetes.io/basic-auth")`, testBindSecretName), ObservedGeneration: 1234, }, + ldapConnectionValidUnknown(1234), tlsConfigurationValidLoadedTrueCondition(1234, "using configured CA bundle"), }, }, @@ -643,6 +657,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`referenced Secret "%s" is missing required keys ["username" "password"]`, testBindSecretName), ObservedGeneration: 1234, }, + ldapConnectionValidUnknown(1234), tlsConfigurationValidLoadedTrueCondition(1234, "using configured CA bundle"), }, }, @@ -662,6 +677,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Phase: "Error", Conditions: []metav1.Condition{ bindSecretValidTrueCondition(1234), + ldapConnectionValidUnknown(1234), { Type: "TLSConfigurationValid", Status: "False", @@ -688,6 +704,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Phase: "Error", Conditions: []metav1.Condition{ bindSecretValidTrueCondition(1234), + ldapConnectionValidUnknown(1234), { Type: "TLSConfigurationValid", Status: "False", @@ -981,6 +998,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) { Message: fmt.Sprintf(`secret "%s" not found`, "non-existent-secret"), ObservedGeneration: 42, }, + ldapConnectionValidUnknown(42), tlsConfigurationValidLoadedTrueCondition(42, "using configured CA bundle"), }, }, diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index 4ebcda244..3c01188e8 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go @@ -27,7 +27,6 @@ import ( oidcapi "go.pinniped.dev/generated/latest/apis/supervisor/oidc" supervisorclientset "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned" idpinformers "go.pinniped.dev/generated/latest/client/supervisor/informers/externalversions/idp/v1alpha1" - "go.pinniped.dev/internal/constable" pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controller/conditionsutil" "go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatchers" @@ -61,9 +60,6 @@ const ( reasonInvalidResponse = "InvalidResponse" reasonDisallowedParameterName = "DisallowedParameterName" allParamNamesAllowedMsg = "additionalAuthorizeParameters parameter names are allowed" - - // Errors that are generated by our reconcile process. - errOIDCFailureStatus = constable.Error("OIDCIdentityProvider has a failing condition") ) var ( @@ -335,7 +331,7 @@ func (c *oidcWatcherController) validateIssuer(ctx context.Context, upstream *id Type: typeOIDCDiscoverySucceeded, Status: metav1.ConditionUnknown, Reason: conditionsutil.ReasonUnableToValidate, - Message: "unable to validate; see other conditions for details", + Message: conditionsutil.MessageUnableToValidate, }, tlsCondition, } diff --git a/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go b/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go index 32c42dfa6..c9ff9fbc4 100644 --- a/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go +++ b/internal/controller/supervisorconfig/upstreamwatchers/upstream_watchers.go @@ -229,9 +229,9 @@ type GradatedConditions struct { } func (g *GradatedConditions) Conditions() []*metav1.Condition { - conditions := []*metav1.Condition{} - for _, gc := range g.gradatedConditions { - conditions = append(conditions, gc.condition) + conditions := make([]*metav1.Condition, len(g.gradatedConditions)) + for i, gc := range g.gradatedConditions { + conditions[i] = gc.condition } return conditions } @@ -263,9 +263,18 @@ func ValidateGenericLDAP( if secretValidCondition.Status == metav1.ConditionTrue && tlsValidCondition.Status == metav1.ConditionTrue { ldapConnectionValidCondition, searchBaseFoundCondition = validateAndSetLDAPServerConnectivityAndSearchBase(ctx, validatedSettingsCache, upstream, config, currentSecretVersion) conditions.Append(ldapConnectionValidCondition, false) + // TODO: For AD, hould we add a condition of type SearchBaseFoundCondition when we can't validate the bind secret or TLS config??? if searchBaseFoundCondition != nil { // currently, only used for AD, so may be nil conditions.Append(searchBaseFoundCondition, true) } + } else { + connectionUnknownCondition := &metav1.Condition{ + Type: typeLDAPConnectionValid, + Status: metav1.ConditionUnknown, + Reason: conditionsutil.ReasonUnableToValidate, + Message: conditionsutil.MessageUnableToValidate, + } + conditions.Append(connectionUnknownCondition, true) } return conditions }