-
Notifications
You must be signed in to change notification settings - Fork 338
feat: pass principal name as part of aws subscoped credentials session #3224
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
base: main
Are you sure you want to change the base?
Conversation
dimas-b
left a comment
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.
Thanks for your contribution, @tokoko !
Overall the PR looks pretty good to me with some comments as noted below.
Given that it affects credential vending, I'd propose to also send a dev email about it for visibility (a custom for major changes in Polaris). Also, a CHANGELOG entry would be good to have for this.
...core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java
Outdated
Show resolved
Hide resolved
...service/src/main/java/org/apache/polaris/service/catalog/io/StorageAccessConfigProvider.java
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java
Outdated
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java
Show resolved
Hide resolved
dimas-b
left a comment
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.
Hi @tokoko : I think this PR is moving closer to completion. I have some more comments, but conceptually this PR LGTM 👍
...core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java
Outdated
Show resolved
Hide resolved
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java
Outdated
Show resolved
Hide resolved
...c/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/task/TaskExecutorImpl.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/polaris/service/catalog/generic/PolarisGenericTableCatalogRelationalTest.java
Outdated
Show resolved
Hide resolved
|
CI appears to be stuck 🤷 |
dimas-b
left a comment
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.
Thanks, @tokoko ! From my POV the PR looks good, but let's collect some more reviews as it's a pretty fundamental change.
| .put("polaris.features.\"ALLOW_TABLE_LOCATION_OVERLAP\"", "true") | ||
| .put("polaris.features.\"LIST_PAGINATION_ENABLED\"", "true") | ||
| .put("polaris.behavior-changes.\"ALLOW_NAMESPACE_CUSTOM_LOCATION\"", "true") | ||
| .put("polaris.test.rootAugmentor.enabled", "true") |
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.
@adutra : do you have any comment on this? As for me, I'm ok with this approach, as I do not have a better alternative off the top of my head 😅
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 think we can achieve the same by simply producing this bean from this profile class:
public static class Profile extends Profiles.DefaultProfile {
@Override public Map<String, String> getConfigOverrides() {...}
@Produces @RequestScoped produceTestAugmentor() { return new RootPrincipalAugmentor(...); }
}But the current approach is fine too.
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.
The idea was to change it later from a simple toggle to something that will hold more information. Something like polaris.test.augmentor.identity: test_principal:test_principal_role. I ended up not requiring it, but it could still come in handy later...
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.
Sounds good 👍
pinging @XN137 for awareness since I vaguely recall you were working on turning a bunch of manual setup code into proper Quarkus tests with CDI enabled.
...service/src/main/java/org/apache/polaris/service/context/catalog/PolarisPrincipalHolder.java
Show resolved
Hide resolved
| .put("polaris.features.\"ALLOW_TABLE_LOCATION_OVERLAP\"", "true") | ||
| .put("polaris.features.\"LIST_PAGINATION_ENABLED\"", "true") | ||
| .put("polaris.behavior-changes.\"ALLOW_NAMESPACE_CUSTOM_LOCATION\"", "true") | ||
| .put("polaris.test.rootAugmentor.enabled", "true") |
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 think we can achieve the same by simply producing this bean from this profile class:
public static class Profile extends Profiles.DefaultProfile {
@Override public Map<String, String> getConfigOverrides() {...}
@Produces @RequestScoped produceTestAugmentor() { return new RootPrincipalAugmentor(...); }
}But the current approach is fine too.
adnanhemani
left a comment
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, two nits that don't require a code change but would be quick and easy to do if there is another revision :)
...ris-core/src/test/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheTest.java
Show resolved
Hide resolved
...c/test/java/org/apache/polaris/service/storage/aws/AwsCredentialsStorageIntegrationTest.java
Show resolved
Hide resolved
adnanhemani
left a comment
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 for this great contribution!!
PR changes role session name of temporary credentials generated for s3 to contain principal name. The goal is to simplify audit of storage access with credentials generated by Polaris. PolarisPrincipal is injected in
StorageAccessConfigProvider, used as part of a cache key and then value propagated through the call chain. Azure and Gcp integration classes also accept PolarisPrincipal, but the values are ignored for now.This will probably also result in relatively increased amount of sts calls as credential requests for the same table by different principals will no longer hit the same cache.
Fixes #3196
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)