You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "jbonofre (via GitHub)" <gi...@apache.org> on 2026/06/11 09:01:58 UTC
[PR] Forward catalog static FileIO properties to server-side FileIO [polaris]
jbonofre opened a new pull request, #4696:
URL: https://github.com/apache/polaris/pull/4696
`IcebergCatalog.loadFileIOForTableLike` only passed `tableDefaultProperties` (the `table-default.*`-prefixed subset of catalog properties) to the `FileIOFactory`. For S3-compatible storage configured with `stsUnavailable: true`, catalogs typically carry static credentials (`s3.access-key-id` / `s3.secret-access-key`) as catalog properties. These were dropped before reaching `S3FileIO`, which then fell back to the AWS default credentials provider chain and failed when no chain credential was available.
Merge `catalogProperties` as the base layer under `tableProperties`. `DefaultFileIOFactory` still overlays `StorageAccessConfig` last, so STS-vended subscoped credentials continue to take precedence over static catalog credentials when STS is available.
## Checklist
- [x] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [x] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [x] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Forward catalog static FileIO properties to server-side FileIO [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #4696:
URL: https://github.com/apache/polaris/pull/4696#discussion_r3562671627
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2400,8 +2400,15 @@ private FileIO loadFileIOForTableLike(
StorageAccessConfig storageAccessConfig =
storageAccessConfigProvider.getStorageAccessConfig(
identifier, readLocations, storageActions, Optional.empty(), resolvedStorageEntity);
- // Reload fileIO based on table specific context
- FileIO fileIO = fileIOFactory.loadFileIO(storageAccessConfig, ioImplClassName, tableProperties);
+ // Catalog-level FileIO properties (e.g. s3.access-key-id / s3.secret-access-key for
+ // S3-compatible storage with stsUnavailable=true) form the base; the caller's
+ // tableProperties overlay on top, and DefaultFileIOFactory further overlays
+ // storageAccessConfig so STS-vended subscoped credentials always take precedence
+ // over static catalog credentials when STS is available.
+ Map<String, String> mergedProperties = new HashMap<>(catalogProperties);
Review Comment:
IIUC catalogProperties here is the full unfiltered map from initialize(), not scoped to S3 keys. Every catalog property rides into FileIO now, not just the two credential ones. Worth narrowing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Forward catalog static FileIO properties to server-side FileIO [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4696: Forward catalog static FileIO properties to server-side FileIO
URL: https://github.com/apache/polaris/pull/4696
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Forward catalog static FileIO properties to server-side FileIO [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4696:
URL: https://github.com/apache/polaris/pull/4696#issuecomment-5419565496
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Forward catalog static FileIO properties to server-side FileIO [polaris]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #4696:
URL: https://github.com/apache/polaris/pull/4696#discussion_r3623319415
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -2400,8 +2400,15 @@ private FileIO loadFileIOForTableLike(
StorageAccessConfig storageAccessConfig =
storageAccessConfigProvider.getStorageAccessConfig(
identifier, readLocations, storageActions, Optional.empty(), resolvedStorageEntity);
- // Reload fileIO based on table specific context
- FileIO fileIO = fileIOFactory.loadFileIO(storageAccessConfig, ioImplClassName, tableProperties);
+ // Catalog-level FileIO properties (e.g. s3.access-key-id / s3.secret-access-key for
+ // S3-compatible storage with stsUnavailable=true) form the base; the caller's
+ // tableProperties overlay on top, and DefaultFileIOFactory further overlays
+ // storageAccessConfig so STS-vended subscoped credentials always take precedence
+ // over static catalog credentials when STS is available.
+ Map<String, String> mergedProperties = new HashMap<>(catalogProperties);
Review Comment:
I changed the approach to not touch the properties as a whole but only related.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Forward catalog static FileIO properties to server-side FileIO [polaris]
Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #4696:
URL: https://github.com/apache/polaris/pull/4696#discussion_r3867353603
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogTest.java:
##########
@@ -1070,6 +1071,59 @@ public void testValidateNotificationFailToCreateFileIO() {
.hasMessageContaining("Fake failure applying downscoped credentials");
}
+ @Test
+ public void testCatalogStaticS3CredentialsAreForwardedToFileIO() {
+ Assumptions.assumeTrue(
+ requiresNamespaceCreate(),
+ "Only applicable if namespaces must be created before adding children");
+ Assumptions.assumeTrue(
+ supportsNestedNamespaces(), "Only applicable if nested namespaces are supported");
+ Assumptions.assumeTrue(
+ supportsNotifications(), "Only applicable if notifications are supported");
+
+ // Static S3 credentials set on the catalog (the stsUnavailable=true case for
+ // S3-compatible storage) must reach the server-side FileIO. Without this they
+ // would be dropped and S3FileIO would fall back to the AWS default credentials
+ // provider chain.
+ final String tableLocation = "s3://externally-owned-bucket/static_creds_table/";
+ final String tableMetadataLocation = tableLocation + "metadata/v1.metadata.json";
+ FileIOFactory spiedFileIOFactory = spy(this.fileIOFactory);
+ IcebergCatalog catalog =
+ newIcebergCatalog(catalog().name(), metaStoreManager, spiedFileIOFactory);
+ catalog.initialize(
+ CATALOG_NAME,
+ ImmutableMap.of(
+ CatalogProperties.FILE_IO_IMPL,
+ "org.apache.iceberg.inmemory.InMemoryFileIO",
+ "s3.access-key-id",
+ "static-access-key",
+ "s3.secret-access-key",
+ "static-secret-key"));
Review Comment:
Could this test construct an AWS storage config with stsUnavailable=true and a storage integration that returns no STS credentials? The shared fixture currently uses an STS-enabled config and returns session credentials, so this verifies only that the two catalog keys reach FileIOFactory, not that those static credentials drive server-side FileIO in the no-STS case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Forward catalog static FileIO properties to server-side FileIO [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #4696:
URL: https://github.com/apache/polaris/pull/4696#issuecomment-4681732751
> catalogs typically carry static credentials (`s3.access-key-id` / `s3.secret-access-key`) as catalog properties.
Isn't this a bad practice? In the [REST Federation](https://polaris.apache.org/releases/1.5.0/federation/iceberg-rest-federation/) doc page, we say:
> Catalog properties are returned to authenticated catalog clients as part of the Iceberg REST /config response. Only place non-sensitive client configuration in catalog properties. Do not put passwords, tokens, access keys, or other secrets into catalog properties.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Forward catalog static FileIO properties to server-side FileIO [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #4696:
URL: https://github.com/apache/polaris/pull/4696#issuecomment-4682487667
I agree that @adutra 's comment above that passwords should _not_ be stored in catalog properties.
I believe general-purpose Catalog properties should probably _not_ be forwarded to clients automatically. That will create scope confusion between the Server and the Client. If a particular property has a reason for being forwarded, it should be ok on a case-by-case basis (as for `table-default.*`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org