-
Couldn't load subscription status.
- Fork 325
Remove credential flag from StorageAccessProperty.CLIENT_REGION
#2838
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
`CLIENT_REGION` is not a credential value, which is in line with Iceberg's `VendedCredentialsProvider` code. Cf. apache/iceberg#11389
1b98491 to
10204dc
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.
Thanks for the PR @dimas-b. To confirm the context, the s3.region was included in credentials for #342 because iceberg by that time does not support s3 cross-region access yet. That was later added in apache/iceberg@3d9fc1d and released with iceberg 1.7.0.
Would this be a breaking change for users since the s3.cross-region-access-enabled is still default to be false? I assume users who are relying on this information in vended credential to avoid cross-region call will encounter failure after this PR?
| - `s3.secret-access-key`: secret for credentials that provide access to data in S3 | ||
| - `s3.session-token | ||
| */ | ||
| StorageAccessProperty(Class valueType, String propertyName, String description) { |
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.
Is the idea here to make the enum declaration more explicit about the isCredential setting?
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
|
@HonahX : Is that sufficient for Iceberg clients? What's the best way to test it? For background: the reason for this PR is that the "nice error message" from #2711 does not happen if the region is set in storage config (but no credentials are vended). |
|
@dimas-b Thanks for the pointer and context. Iceberg's The RestCatalog client only use things in the If |
Would it be easier if consider the credential being "empty" if only CLIENT_REGION is there? |
|
@HonahX : I tested with Spark 3.5 + Iceberg 1.6.1 and AWS with the code in this PR... all seems to work well... WDYT? Note: Spark did not have any AWS FileIO config. It got everything from Polaris. Here's a sample load table response for reference: |
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.
@dimas-b Thanks for the test and examples. I also did one with Spark 3.5 + Iceberg 1.9.1. Apparently I missed some details when reading through the code. You are right! The client.region will still take effect in the config field. LGTM!
CLIENT_REGIONis not a credential value, which is in line with Iceberg'sVendedCredentialsProvidercode.Cf. apache/iceberg#11389