Add GCP service account impersonation for credentials. #3246
Conversation
adnanhemani
left a comment
adnanhemani
left a comment
There was a problem hiding this comment.
Looks great, @talatuyarer!! Thank you for this :) One small nit - not a requirement before merging (we can refactor after), but if you are making any changes, would be nice to take care of this in the same go.
| .generateAccessToken( | ||
| Mockito.argThat( | ||
| request -> | ||
| request.getName().equals("projects/-/serviceAccounts/" + serviceAccount) |
There was a problem hiding this comment.
nit: Would recommend moving these variables out to the main class as public variables and then referencing them here from there so that we don't have magic variables and it will be cleaner to maintain if there are any changes in the future!
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @talatuyarer !
| * is configured, it impersonates that account first. | ||
| */ | ||
| private GoogleCredentials getBaseCredentials() { | ||
| if (config().getGcpServiceAccount() != null) { |
There was a problem hiding this comment.
Interesting that this config value was defined before, but not used 🤔
Does anyone have insight into why it was this way?
Technically, now that GcpServiceAccount config values start affecting Polaris behaviour, this could be a breaking change in existing deployments that may have (possibly accidental and/or incorrect) values in that config property.
I do believe that such a situation is unlikely, so it should be fine to proceed with this PR. However, it probably deserves mentioning as a Change in CHANGELOG.md
There was a problem hiding this comment.
Looks like this is in the CLI doc for a while, https://polaris.apache.org/in-dev/unreleased/command-line-interface/#catalogs
--service-account (Only for GCS) The service account to use when connecting to GCS
Agreed with @dimas-b, not particularly a big concern, but +1 on adding this to the CHANGELOG.md.
flyrain
left a comment
flyrain
left a comment
There was a problem hiding this comment.
Thanks a lot for working on it, @talatuyarer! Looks great to me overall. I think it's ready once we add an item in changelog.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
|
Thanks a lot for adding this, @talatuyarer! Thanks @dimas-b @adnanhemani for the review! |
4 participants
This will close this bug: #550
cc @flyrain