Skip to content

Commit

Permalink
Merge pull request #2034 from vmware-tanzu/jtc/older-idps-should-use-…
Browse files Browse the repository at this point in the history 
…unknown-condition-status

OIDC/LDAP/AD IDPs should use unknown condition status
  • Loading branch information
@cfryanr
cfryanr authored Aug 7, 2024
2 parents fbbec50 + c1328d9 commit b377040
Show file tree
Hide file tree
Showing 16 changed files with 591 additions and 120 deletions.
10 changes: 4 additions & 6 deletions internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
6 changes: 2 additions & 4 deletions internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,6 @@ const (
reasonUnableToInstantiateWebhook = "UnableToInstantiateWebhook"
reasonInvalidEndpointURL = "InvalidEndpointURL"
reasonInvalidEndpointURLScheme = "InvalidEndpointURLScheme"

msgUnableToValidate = "unable to validate; see other conditions for details"
)

type cachedWebhookAuthenticator struct {
Expand Down Expand Up @@ -344,7 +342,7 @@ func newWebhookAuthenticator(
Type: typeAuthenticatorValid,
Status: metav1.ConditionUnknown,
Reason: conditionsutil.ReasonUnableToValidate,
Message: msgUnableToValidate,
Message: conditionsutil.MessageUnableToValidate,
})
return nil, conditions, nil
}
Expand Down Expand Up @@ -425,7 +423,7 @@ func (c *webhookCacheFillerController) validateConnection(
Type: typeWebhookConnectionValid,
Status: metav1.ConditionUnknown,
Reason: conditionsutil.ReasonUnableToValidate,
Message: msgUnableToValidate,
Message: conditionsutil.MessageUnableToValidate,
})
return conditions, nil
}
Expand Down
4 changes: 3 additions & 1 deletion internal/controller/conditionsutil/conditions_util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
21 changes: 9 additions & 12 deletions ...ller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,6 @@ func (g *activeDirectoryUpstreamGenericLDAPImpl) Generation() int64 {
return g.activeDirectoryIdentityProvider.Generation
}

func (g *activeDirectoryUpstreamGenericLDAPImpl) Status() upstreamwatchers.UpstreamGenericLDAPStatus {
return &activeDirectoryUpstreamGenericLDAPStatus{g.activeDirectoryIdentityProvider}
}

type activeDirectoryUpstreamGenericLDAPSpec struct {
activeDirectoryIdentityProvider idpv1alpha1.ActiveDirectoryIdentityProvider
}
Expand All @@ -122,6 +118,15 @@ func (s *activeDirectoryUpstreamGenericLDAPSpec) GroupSearch() upstreamwatchers.
return &activeDirectoryUpstreamGenericLDAPGroupSearch{s.activeDirectoryIdentityProvider.Spec.GroupSearch}
}

func (s *activeDirectoryUpstreamGenericLDAPSpec) UnknownSearchBaseCondition() *metav1.Condition {
return &metav1.Condition{
Type: upstreamwatchers.TypeSearchBaseFound,
Status: metav1.ConditionUnknown,
Reason: conditionsutil.ReasonUnableToValidate,
Message: conditionsutil.MessageUnableToValidate,
}
}

func (s *activeDirectoryUpstreamGenericLDAPSpec) DetectAndSetSearchBase(ctx context.Context, config *upstreamldap.ProviderConfig) *metav1.Condition {
config.GroupSearch.Base = s.activeDirectoryIdentityProvider.Spec.GroupSearch.Base
config.UserSearch.Base = s.activeDirectoryIdentityProvider.Spec.UserSearch.Base
Expand Down Expand Up @@ -216,14 +221,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)
Expand Down
36 changes: 36 additions & 0 deletions ...supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
ObservedGeneration: gen,
}
}

activeDirectoryConnectionValidTrueCondition := func(gen int64, secretVersion string) metav1.Condition {
return metav1.Condition{
Type: "LDAPConnectionValid",
Expand All @@ -383,6 +384,17 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
c.LastTransitionTime = metav1.Time{}
return c
}
ldapConnectionValidUnknownCondition := 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,
}
}

condPtr := func(c metav1.Condition) *metav1.Condition {
return &c
}
Expand All @@ -391,6 +403,7 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
c.LastTransitionTime = metav1.Time{}
return c
}

tlsConfigurationValidLoadedTrueCondition := func(gen int64, msg string) metav1.Condition {
return metav1.Condition{
Type: "TLSConfigurationValid",
Expand Down Expand Up @@ -435,6 +448,17 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
}
}

searchBaseFoundUnknownCondition := func(gen int64) metav1.Condition {
return metav1.Condition{
Type: "SearchBaseFound",
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),
Expand Down Expand Up @@ -663,6 +687,8 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
Message: fmt.Sprintf(`secret "%s" not found`, testBindSecretName),
ObservedGeneration: 1234,
},
ldapConnectionValidUnknownCondition(1234),
searchBaseFoundUnknownCondition(1234),
tlsConfigurationValidLoadedTrueCondition(1234, "using configured CA bundle"),
},
},
Expand Down Expand Up @@ -691,6 +717,8 @@ 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,
},
ldapConnectionValidUnknownCondition(1234),
searchBaseFoundUnknownCondition(1234),
tlsConfigurationValidLoadedTrueCondition(1234, "using configured CA bundle"),
},
},
Expand Down Expand Up @@ -718,6 +746,8 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
Message: fmt.Sprintf(`referenced Secret "%s" is missing required keys ["username" "password"]`, testBindSecretName),
ObservedGeneration: 1234,
},
ldapConnectionValidUnknownCondition(1234),
searchBaseFoundUnknownCondition(1234),
tlsConfigurationValidLoadedTrueCondition(1234, "using configured CA bundle"),
},
},
Expand All @@ -737,6 +767,8 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
Phase: "Error",
Conditions: []metav1.Condition{
bindSecretValidTrueCondition(1234),
ldapConnectionValidUnknownCondition(1234),
searchBaseFoundUnknownCondition(1234),
{
Type: "TLSConfigurationValid",
Status: "False",
Expand All @@ -763,6 +795,8 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
Phase: "Error",
Conditions: []metav1.Condition{
bindSecretValidTrueCondition(1234),
ldapConnectionValidUnknownCondition(1234),
searchBaseFoundUnknownCondition(1234),
{
Type: "TLSConfigurationValid",
Status: "False",
Expand Down Expand Up @@ -1158,6 +1192,8 @@ func TestActiveDirectoryUpstreamWatcherControllerSync(t *testing.T) {
Message: fmt.Sprintf(`secret "%s" not found`, "non-existent-secret"),
ObservedGeneration: 42,
},
ldapConnectionValidUnknownCondition(42),
searchBaseFoundUnknownCondition(42),
tlsConfigurationValidLoadedTrueCondition(42, "using configured CA bundle"),
},
},
Expand Down
2 changes: 1 addition & 1 deletion internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
18 changes: 5 additions & 13 deletions internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,6 @@ func (g *ldapUpstreamGenericLDAPImpl) Generation() int64 {
return g.ldapIdentityProvider.Generation
}

func (g *ldapUpstreamGenericLDAPImpl) Status() upstreamwatchers.UpstreamGenericLDAPStatus {
return &ldapUpstreamGenericLDAPStatus{g.ldapIdentityProvider}
}

type ldapUpstreamGenericLDAPSpec struct {
ldapIdentityProvider idpv1alpha1.LDAPIdentityProvider
}
Expand All @@ -78,10 +74,14 @@ func (s *ldapUpstreamGenericLDAPSpec) GroupSearch() upstreamwatchers.UpstreamGen
return &ldapUpstreamGenericLDAPGroupSearch{s.ldapIdentityProvider.Spec.GroupSearch}
}

func (s *ldapUpstreamGenericLDAPSpec) UnknownSearchBaseCondition() *metav1.Condition {
return nil // currently, only AD returns a condition for this
}

func (s *ldapUpstreamGenericLDAPSpec) DetectAndSetSearchBase(_ context.Context, config *upstreamldap.ProviderConfig) *metav1.Condition {
config.GroupSearch.Base = s.ldapIdentityProvider.Spec.GroupSearch.Base
config.UserSearch.Base = s.ldapIdentityProvider.Spec.UserSearch.Base
return nil
return nil // currently, only AD returns a condition for this
}

type ldapUpstreamGenericLDAPUserSearch struct {
Expand Down Expand Up @@ -124,14 +124,6 @@ func (g *ldapUpstreamGenericLDAPGroupSearch) GroupNameAttribute() string {
return g.groupSearch.Attributes.GroupName
}

type ldapUpstreamGenericLDAPStatus struct {
ldapIdentityProvider idpv1alpha1.LDAPIdentityProvider
}

func (s *ldapUpstreamGenericLDAPStatus) Conditions() []metav1.Condition {
return s.ldapIdentityProvider.Status.Conditions
}

// UpstreamLDAPIdentityProviderICache is a thread safe cache that holds a list of validated upstream LDAP IDP configurations.
type UpstreamLDAPIdentityProviderICache interface {
SetLDAPIdentityProviders([]upstreamprovider.UpstreamLDAPIdentityProviderI)
Expand Down
20 changes: 20 additions & 0 deletions internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
ObservedGeneration: gen,
}
}

ldapConnectionValidTrueCondition := func(gen int64, secretVersion string) metav1.Condition {
return metav1.Condition{
Type: "LDAPConnectionValid",
Expand All @@ -380,9 +381,21 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
c.LastTransitionTime = metav1.Time{}
return c
}
ldapConnectionValidUnknownCondition := 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,
}
}

condPtr := func(c metav1.Condition) *metav1.Condition {
return &c
}

tlsConfigurationValidLoadedTrueCondition := func(gen int64, msg string) metav1.Condition {
return metav1.Condition{
Type: "TLSConfigurationValid",
Expand All @@ -393,6 +406,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
ObservedGeneration: gen,
}
}

allConditionsTrue := func(gen int64, secretVersion string) []metav1.Condition {
return []metav1.Condition{
bindSecretValidTrueCondition(gen),
Expand Down Expand Up @@ -588,6 +602,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
Message: fmt.Sprintf(`secret "%s" not found`, testBindSecretName),
ObservedGeneration: 1234,
},
ldapConnectionValidUnknownCondition(1234),
tlsConfigurationValidLoadedTrueCondition(1234, "using configured CA bundle"),
},
},
Expand Down Expand Up @@ -616,6 +631,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,
},
ldapConnectionValidUnknownCondition(1234),
tlsConfigurationValidLoadedTrueCondition(1234, "using configured CA bundle"),
},
},
Expand Down Expand Up @@ -643,6 +659,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
Message: fmt.Sprintf(`referenced Secret "%s" is missing required keys ["username" "password"]`, testBindSecretName),
ObservedGeneration: 1234,
},
ldapConnectionValidUnknownCondition(1234),
tlsConfigurationValidLoadedTrueCondition(1234, "using configured CA bundle"),
},
},
Expand All @@ -662,6 +679,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
Phase: "Error",
Conditions: []metav1.Condition{
bindSecretValidTrueCondition(1234),
ldapConnectionValidUnknownCondition(1234),
{
Type: "TLSConfigurationValid",
Status: "False",
Expand All @@ -688,6 +706,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
Phase: "Error",
Conditions: []metav1.Condition{
bindSecretValidTrueCondition(1234),
ldapConnectionValidUnknownCondition(1234),
{
Type: "TLSConfigurationValid",
Status: "False",
Expand Down Expand Up @@ -981,6 +1000,7 @@ func TestLDAPUpstreamWatcherControllerSync(t *testing.T) {
Message: fmt.Sprintf(`secret "%s" not found`, "non-existent-secret"),
ObservedGeneration: 42,
},
ldapConnectionValidUnknownCondition(42),
tlsConfigurationValidLoadedTrueCondition(42, "using configured CA bundle"),
},
},
Expand Down
Loading

0 comments on commit b377040

Please sign in to comment.