Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: implementation of AWS OIDC #232
feat: implementation of AWS OIDC #232
Changes from 10 commits
29f5a51
9491bec
be6adeb
a732475
084c75d
6f9c479
4fbc279
5608c9b
5a81161
0193401
54a5c93
81dfcac
35504e9
f48ac02
fe13f69
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make the functions small and do one thing.
Having a function name with
Or
in it is a sign something isn't right ;)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect several of these functions do exist in the JWT or OIDC libraries - please take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which specific functions do you suspect? I might be missing something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming this seems to lose the readability of the code. is it necessary?
@mathetake is this a pattern used across the code base?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and this is the standard pattern and naming consistency is ensured by the linter in CI. Btw, this is not only here but also across the whole bunch of k8s related projects. you see the API package name
v1alpha1
exists across tons of projects so not renaming is terrible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not rename to egv1alpha1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider renaming function to something like
IdentityTokenAsByteArray
to make it more accurate for what the function does.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetIdentityToken
is the official function for IdentityTokenRetriever's interfaceBut I do agree it's not very clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since Handler has meaning in Go and becomes vague in this context. Consider renaming to be more specific, consider something descriptive like
TokenRefresher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TokenRefresher
makes sense if the "handler" only managed refreshing OIDC tokens. What do you think aboutCredentialRefresher
since it updates the OIDC token and updates/creates a secret file with new loginsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does both token exchange and refresh, we discussed to have a generic interface to support both aws and gcp which we are going to add very soon. The steps are the same to get the oidc token to exchange the cloud credential, then update the credential file. The token refresher go routine updates the credential before it expires.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an infite loop, could we spawn a process instead? If we spawn it as a process it can be terminated and should make the code cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming you're referring to this process?
Correct me if I'm misunderstanding you, but would this mean we create an executable for the OIDC credential update and have the process handle updating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple go routine based timer should work, we do not need a process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider making this a separate function that is called if the
backendSecurityPolicies
is set. putting this for loop a callable function instead will make it more testable and self contained.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider making this logic check a function, also consider making values like
-5*time.Minute
a constant somewhere, as the 5 minutes before expiry should be easily to semantically understand somewhere in the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a lot of casting in this function, which seems a bit excessive and unnecessary. Can we get an explanation of why this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for the two castings:
Claim
to be typeMapClaims
when parsing the access token viaParseUnverified
. We later cast it because we want to use its properties of being a map (we essentially are checking the type this way).