-
Notifications
You must be signed in to change notification settings - Fork 25
Add auth support for UAA IDP #802
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
Conversation
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.
Nice refactoring @vuil !
I am still continuing the review, but sharing few comments i have so far. Thanks
5d333dd
to
8b7729a
Compare
f048304
to
963d20c
Compare
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.
Changes LGTM, just couple more comments and then good to go.
A fair bit of code refactoring to support multiple IDPs, include new support for UAA. IDP used in CLI context creation is stored in the Context's additionalMetadata map. `context get-token` is updated to refreshed token based on the IDP associated with said context tanzu context create / tanzu login are update to use UAA IDP unless the CLI recognizes the endpoint as a public Tanzu Platform service endpoint or if CSP idp is explicitly requested with the --force-csp hidden flag.
} | ||
|
||
// UAA-based authentication does not provide org id or name yet | ||
orgName := "" |
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.
OrgName which gets configured in the context
as part of additionalMetadata is used by all the plugins to show log messages. Keeping this empty might make the log message incomplete. Does it make sense to configure dummy value as part of additionalMetadata? like SelfManaged
or something like that?
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.
We can do this as part of follow up as well.
Co-Authored-by: Anuj Chaudhari <[email protected]> Signed-off-by: Vui Lam <[email protected]>
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.
LGTM. Thanks!
A fair bit of code refactoring to support multiple IDPs, include new support for UAA.
IDP used in CLI context creation is stored in the Context's additionalMetadata map.
context get-token
is updated to refresh token based on the IDP associated with said contexttanzu context create / tanzu login
are updated to use UAA IDP unless the CLI recognizes the endpoint as a public Tanzu Platform service endpoint or if CSP idp is explicitly requested with the
--force-csp
hidden flag.What this PR does / why we need it
Which issue(s) this PR fixes
Fixes #
Describe testing done for PR
Ran unit tests
manual testings of following commands:
Release note
Additional information
Special notes for your reviewer