Infrastructure changes for token timeouts#17614
Infrastructure changes for token timeouts#17614openshift-merge-robot merged 4 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
size/XL
simo5
left a comment
simo5
left a comment
There was a problem hiding this comment.
I like this (except the last commit really does not belong in the same PR).
Only minor nits to fix
pkg/auth/authenticator/interfaces.go
Outdated
| AuthenticateClient(client api.Client) (user.Info, bool, error) | ||
| } | ||
|
|
||
| type OAuthTokenUserValidator interface { |
There was a problem hiding this comment.
Should be called OAuthTokenValidator
The fact one of the validators checks users does not change the generic nature of the validator.
| return authenticator.OAuthTokenUserValidatorFunc( | ||
| func(token *oauth.OAuthAccessToken, _ *user.User) error { | ||
| if token.ExpiresIn > 0 { | ||
| if token.CreationTimestamp.Time.Add(time.Duration(token.ExpiresIn) * time.Second).Before(time.Now()) { |
There was a problem hiding this comment.
It be more readbale if the conversion of token.ExpiresIn to a duration would be doen via a helper function
| fakeOAuthClient := oauthfake.NewSimpleClientset( | ||
| &oapi.OAuthAccessToken{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "token", CreationTimestamp: metav1.Time{Time: time.Now().Add(-1 * time.Hour)}}, | ||
| ObjectMeta: metav1.ObjectMeta{Name: "token1", CreationTimestamp: metav1.Time{Time: time.Now().Add(-1 * time.Hour)}}, // expired |
There was a problem hiding this comment.
these comments are so far they look silly, what about adding just one explanatory comment on the line before &oapi.OAuthAccessToken{ ?
Like:
// Add expired token with 10 minutes expiration time
| tokens oauthclient.OAuthAccessTokenInterface | ||
| users userclient.UserResourceInterface | ||
| groupMapper identitymapper.UserToGroupMapper | ||
| validator authenticator.OAuthTokenUserValidator |
There was a problem hiding this comment.
should probably be plural: validators
pkg/util/sortedset/sortedset.go
Outdated
| @@ -0,0 +1,182 @@ | |||
| package sortedset | |||
There was a problem hiding this comment.
this should be called something like rankedset, a soretedset is a different thing
Also prese move this whole commit to a different PR, as it has nothing to do with the rest of infrastrcture changes
This change removes the duplicate authenticator interfaces that are no longer needed. The exact same interfaces exist upstream, so this change is purely mechanical. Signed-off-by: Monis Khan <mkhan@redhat.com>
This change adds the OAuthTokenValidator interface for generically validating an OAuthAccessToken and User. The expiration and UID validation was pulled out from the tokenAuthenticator. tokenAuthenticator simply takes OAuthTokenValidators as input, and delegates validation to them. This allows all future validation to simply append itself to the list of validators without requiring any changes to tokenAuthenticator. Signed-off-by: Monis Khan <mkhan@redhat.com>
This change wires up the OAuth informer to allow performant lookups to be made for the OAuth resources. Signed-off-by: Monis Khan <mkhan@redhat.com>
This change allows the authenticator to return post start hooks that can be used to set up any infrastructure the authenticator needs. It also makes sure that these resources are properly cleaned up when the post start hooks are stopped. Signed-off-by: Monis Khan <mkhan@redhat.com>
enj
force-pushed
the
enj/f/token_timeout_foundation
branch
from
804c8d6 to
f27d947
Compare
openshift-ci-robot
added
approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj, simo5 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 17480, 17614). |
4 participants
Remove duplicate Origin authenticator interfaces
This change removes the duplicate authenticator interfaces that are
no longer needed. The exact same interfaces exist upstream, so this
change is purely mechanical.
Signed-off-by: Monis Khan mkhan@redhat.com
Add OAuth token and user validator interface
This change adds the OAuthTokenValidator interface for generically
validating an OAuthAccessToken and User. The expiration and UID
validation was pulled out from the tokenAuthenticator.
tokenAuthenticator simply takes OAuthTokenValidators as input, and
delegates validation to them. This allows all future validation to
simply append itself to the list of validators without requiring any
changes to tokenAuthenticator.
Signed-off-by: Monis Khan mkhan@redhat.com
Wire up OAuth shared informer
This change wires up the OAuth informer to allow performant lookups
to be made for the OAuth resources.
Signed-off-by: Monis Khan mkhan@redhat.com
Allow authenticator to return post start hooks
This change allows the authenticator to return post start hooks that
can be used to set up any infrastructure the authenticator needs.
It also makes sure that these resources are properly cleaned up when
the post start hooks are stopped.
Signed-off-by: Monis Khan mkhan@redhat.com
/assign @simo5