feat: storage-scoped aws credentials#3409
Conversation
| Optional<String> awsSecretKey(); | ||
|
|
||
| @WithName("aws") | ||
| Map<String, StorageConfig> storages(); |
There was a problem hiding this comment.
@WithDefaults and/or @WithParentName might be helpful here... WDYT? e.g.
There was a problem hiding this comment.
See, replacing the awsAccessKey() and awsSecretKey() properties.
@WithParentName
Optional<StorageConfig> defaultSecrets();There was a problem hiding this comment.
@tokoko : WDYT about @WithParentName for config handling? I hope it might be a bit clearer.
snazy
left a comment
snazy
left a comment
There was a problem hiding this comment.
Can we safely assume that a bucket name is "globally unique" across all realms? Mean, this approach is probably good enough to serve most users' needs. Just wondering whether there might be situations in multi-realm setups that would require a more fine-grained approach. We can certainly defer that to when we have a "secrets provider" API.
| Optional<String> awsSecretKey(); | ||
|
|
||
| @WithName("aws") | ||
| Map<String, StorageConfig> storages(); |
There was a problem hiding this comment.
See, replacing the awsAccessKey() and awsSecretKey() properties.
@WithParentName
Optional<StorageConfig> defaultSecrets();
tokoko
force-pushed
the
aws-storage-credentials
branch
from
a70fa75 to
7eaf070
Compare
|
@snazy from the discussion in the issue, I think the point is that it's pointless to try to come up with a way to infer a good storage name that will also be guaranteed to be unique. There might be more than one allowed buckets, the same bucket name could be referring to different buckets in s3 and minio and so on. So the PR tried to infer a "good enough" name from the first entry in allowed locations. Whenever that's impossible or incorrect, the user is free to set it's own name. |
There was a problem hiding this comment.
Thanks for the update, @tokoko !
It looks like we still need to make storage location parsing more robust, but otherwise this approach LGTM.
This PR enables Polaris deployments to be configured with a finite set of per-location credentials known ahead of time, which is a nice feature for Polaris Administrators, I guess.
There are on-going discussions about leveraging (developing) a Secrets Manager for loading credentials, so this part of the Polaris codebase may need to change soon, but I hope we can still preserver the basic feature of allowing per-bucket credentials.
All in all, I think this PR is worth merging as an intermediate solution since it helps some users while the bigger Secrets Manager is under development.
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
As required for API and (major) config changes, I started this Let's way a couple of days and see if other people have comments. |
| this.realmConfig = realmConfig; | ||
| } | ||
|
|
||
| public PolarisStorageIntegrationProviderImpl( |
There was a problem hiding this comment.
Some further cleanup (in a follow-up PR) would be nice here, as this constructor is now only used in 2 tests but it complicates the logic for resolving the creds quite a bit.
There was a problem hiding this comment.
yeah, that's for backwards-compat only
collado-mike
left a comment
collado-mike
left a comment
There was a problem hiding this comment.
It would be nice if the credentials could be realm-scoped, but I suppose the original default credentials were never realm-scoped, so this isn't strictly worse. However, I think that falling back to the default credentials if a storage name is incorrectly specified is bad for security. Given the catalog admins ought to know the name of the storage credential set they want to use, I think falling back to default credentials allows for a potential security hole.
| default AwsCredentialsProvider stsCredentials(String storageName) { | ||
| if (storageName != null && aws().storages().containsKey(storageName)) { | ||
| StorageConfig storageConfig = aws().storages().get(storageName); | ||
| return StaticCredentialsProvider.create( | ||
| AwsBasicCredentials.create(storageConfig.accessKey(), storageConfig.secretKey())); | ||
| } | ||
| return stsCredentials(); |
There was a problem hiding this comment.
This seems wrong - if the storage name was passed explicitly, why do we fall back on the default credentials? Seems we should throw here.
There was a problem hiding this comment.
fixed. nice catch, thanks. missed this one on the latest change. now that storageName is always explicitly set, it no longer makes sense to fall back on the defaults.
github-project-automation
Bot
moved this from Ready to merge
to PRs In Progress
in Basic Kanban Board
|
@collado-mike : my understanding is that this feature is generally usable only in situations when the admin owns Polaris deployment (otherwise it is not possible to configure per-name credentials at all, and the system falls back to current behaviour). Additionally, the new behaviour is controlled by a feature flag, which is off by default. So existing users are not affected even when they upgrade to this code. In self-hosted environments, the admin user is, of course, expected to be aware of how has permissions for creating catalogs and should act accordingly when electing to use this feature. That said, defaulting to the global credential or not is probably the admin user's choice. Would covering that behaviour with another feature flag address your concern? @tokoko : Please share your perspective too. |
|
@collado-mike I agree it should ideally be realm-scoped, but that probably also applies to gcp and azure configs as well and I would like to avoid blowing up the scope of this PR. @adutra the same reason, I guess. we don't use gcs and didn't want to unnecessary blow up the scope. plus, I think (!) the issue is not as relevant with gcs as there's not a lot of gcs-compatible products out there. if this mechanism ends up being used for table-level configs, that will probably change, though. |
My concern isn't for existing users. It's for people who try to use the feature and accidentally or maliciously take advantage of this fallback. E.g., if an admin configures a credential called |
|
@collado-mike that was already fixed in the last commit in case you missed it, just making sure.. :) |
@tokoko , I had not seen the revision. Thanks for pointing that out |
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
If there are no fresh comments / concerns in GH or on the |
sfc-gh-ahemani
left a comment
sfc-gh-ahemani
left a comment
There was a problem hiding this comment.
Sorry, I'm late to the party :) But I think this is useful and we should merge this! Can see many future extensions including adding tags/etc. to the storage configurations and expanding this to other cloud providers potentially. Great addition!
github-project-automation
Bot
moved this from Ready to merge
to Done
in Basic Kanban Board
…ark) - HierarchicalStorageConfigResolutionTest: add two unit tests verifying namespace storageName takes precedence over catalog, and that a null storageName at namespace level stops the hierarchy walk - StorageNameHierarchyCredentialTest: new service-layer tests verifying hierarchy resolution reaches the credential provider when the flag is enabled, and that resolution is independent of the feature flag - PolarisStorageConfigIntegrationTest: add three HTTP integration tests for namespace storageName roundtrip, table override, and delete-reverts-to-namespace behaviour - PolarisSparkStorageNameHierarchyIntegrationTest: new Spark integration test class covering 13 named/unnamed storageName permutations across catalog, namespace, and table levels - SparkStorageNameHierarchyIT: Quarkus integration test runner for the Spark hierarchy scenarios Add storage-config integration and Spark credential-vending tests runtime-service: harden storage-config resolution/conflict semantics This commit completes the remaining Step 8 hardening scope for inline storage configuration after the apache#3409 rebase, focused on correctness and deterministic API behavior for namespace/table storage-config endpoints. What changed: - Added full table-path resolution helper in PolarisAdminService via resolveTablePath(...) and reused it from resolveTableEntity(...) - Updated table effective GET logic in PolarisServiceImpl to resolve storage config from full ancestry (table -> namespace chain -> catalog) using FileIOUtil.findStorageInfoFromHierarchy(...) - Added deterministic error mapping for namespace/table storage-config mutations: - optimistic-lock/path-race update outcomes mapped to CommitConflictException in PolarisAdminService.updateEntity(...) - endpoint handlers map CommitConflictException to HTTP 409 - endpoint handlers map ForbiddenException to HTTP 403 - Kept storageName compatibility intact across API/internal model conversion paths Tests added/updated: - PolarisStorageConfigIntegrationTest - storageName roundtrip assertions for namespace/table - effective fallback chain assertions (table -> namespace -> catalog) - nested namespace ancestor fallback regression (ns1\u001Fns2\u001Fns3.table with config on ns1) - PolarisServiceImplTest - 409 contract test for namespace set on concurrent update - 403 contract tests for table get/set/delete unauthorized behavior - PolarisAdminServiceTest - updateEntity conflict mapping test (TARGET_ENTITY_CONCURRENTLY_MODIFIED -> CommitConflictException) - updateEntity entity-not-found mapping test Validation run: - ./gradlew :polaris-runtime-service:compileJava - ./gradlew :polaris-runtime-service:spotlessApply - ./gradlew :polaris-runtime-service:test --tests "org.apache.polaris.service.catalog.io.PolarisStorageConfigIntegrationTest" --tests "org.apache.polaris.service.admin.PolarisServiceImplTest" --tests "org.apache.polaris.service.admin.PolarisAdminServiceTest" fix(storage-config): preserve storageName and prevent task payload serialization regression Context:\n- Branch was rebased on main after apache#3409 (storage-scoped AWS credentials via storageName).\n- Two regressions were observed during validation:\n 1) storageName was dropped in namespace/table management API conversion helpers.\n 2) new helper getters were serialized into task payload JSON, breaking TableCleanupTaskHandlerTest.\n\nWhat changed:\n1) runtime/service/.../PolarisServiceImpl.java\n - Preserve storageName in toInternalModel() for AWS/Azure/GCS/FILE.\n - Preserve storageName in toApiModel() for AWS/Azure/GCS/FILE.\n - Use FileStorageConfigInfo constructor variant that includes storageName.\n\n2) polaris-core/.../NamespaceEntity.java\n3) polaris-core/.../table/TableLikeEntity.java\n - Add @JsonIgnore to getStorageConfigurationInfo() to keep it as a helper accessor only\n and prevent Jackson from serializing it into generic entity JSON payloads (including task data).\n\nWhy:\n- apache#3409 credential lookup can resolve by storageName when RESOLVE_CREDENTIALS_BY_STORAGE_NAME is enabled.\n Dropping storageName causes silent fallback to default credentials, which is incorrect behavior.\n- Serializing helper getters introduced an unexpected field (storageConfigurationInfo) into task payloads,\n causing deserialization failures in TableCleanupTaskHandlerTest.\n\nValidation:\n- ./gradlew :polaris-runtime-service:compileJava\n- ./gradlew :polaris-runtime-service:test --tests org.apache.polaris.service.task.TableCleanupTaskHandlerTest\n\nBoth checks pass after this change. feat: Implement Management API for inline storage configuration Implement Step 8 of the inline storage configuration feature, adding complete CRUD operations for namespace and table storage configs via the Management API. Changes: - Implemented 6 storage config handlers in PolarisServiceImpl: * GET/PUT/DELETE namespace storage config * GET/PUT/DELETE table storage config - Added conversion helpers between API and internal storage config models - Added entity resolution helpers in PolarisAdminService - Updated runtime/service tests to validate all CRUD operations (7/7 passing) - Updated integration tests to verify end-to-end functionality Key Features: - Storage config hierarchy resolution (table → namespace → catalog) - Support for all storage types (S3, Azure, GCS, FILE) - Multipart namespace handling with unit separator encoding - Proper error handling for non-existent entities (404 responses) - Entity versioning and optimistic locking support All tests passing with no compilation errors. feat: Add Management API endpoints for namespace/table storage configuration Implements Step 6 of inline storage configuration feature - Management API design and endpoint exposure with test coverage. Changes: - Add 6 new REST endpoints to Management API specification: * GET /catalogs/{catalog}/namespaces/{namespace}/storage-config * PUT /catalogs/{catalog}/namespaces/{namespace}/storage-config * DELETE /catalogs/{catalog}/namespaces/{namespace}/storage-config * GET /catalogs/{catalog}/namespaces/{namespace}/tables/{table}/storage-config * PUT /catalogs/{catalog}/namespaces/{namespace}/tables/{table}/storage-config * DELETE /catalogs/{catalog}/namespaces/{namespace}/tables/{table}/storage-config - Define API request/response models: * NamespaceStorageConfigResponse * TableStorageConfigResponse - Implement handler stubs in PolarisServiceImpl: * All handlers return 501 NOT_IMPLEMENTED (placeholder for Step 8) * Proper method signatures with RealmContext and SecurityContext * Support for multipart namespace paths - Add comprehensive test coverage (7 tests, all passing): * Test GET/PUT/DELETE for namespace storage configs * Test GET/PUT/DELETE for table storage configs * Test multipart namespace handling (e.g., "level1.level2.level3") * Test multiple cloud providers (AWS S3, Azure Blob, GCP GCS) - Add helper methods to ManagementApi test utility: * getNamespaceStorageConfig() * setNamespaceStorageConfig() * deleteNamespaceStorageConfig() * getTableStorageConfig() * setTableStorageConfig() * deleteTableStorageConfig() Technical Details: - OpenAPI spec: spec/polaris-management-service.yml (+170 lines) - Service implementation: PolarisServiceImpl.java (+138 lines) - Test coverage: PolarisStorageConfigIntegrationTest.java (7/7 tests passing) - Code generation: Regenerated API interfaces from OpenAPI spec Test Results: Total: 7 tests Passed: 7 ✅ Failed: 0 Time: 0.822s Next Steps: - Step 7: Add authorization framework (privileges and access control) - Step 8: Implement full CRUD logic with entity persistence Related: DESIGN.md, STEP-06-MANAGEMENT-API.md feat(storage): Add hierarchical storage config verification and comprehensive tests Step 5: Verification and Documentation - Inline Storage Configuration Feature This commit completes the verification phase of the hierarchical storage configuration feature by adding comprehensive documentation and integration tests. Changes: - Enhanced javadoc in IcebergCatalog.loadFileIOForTableLike() to document hierarchical storage config resolution behavior (table > namespace > catalog) - Enhanced javadoc in StorageAccessConfigProvider.getStorageAccessConfig() to explicitly document entity path search order and hierarchical resolution - Added HierarchicalStorageConfigResolutionTest.java with 8 comprehensive integration test scenarios covering: * Table-level config overrides * Namespace-level config inheritance * Nested namespace hierarchies (4+ levels deep) * Catalog fallback behavior * Edge cases (empty paths, no configs anywhere) * Multiple configs at different levels with correct priority Verification Results: - All 8 new integration tests pass ✓ - FileIOUtilTest (8 tests from Step 4) pass ✓ - Entity storage config tests (from Step 2) pass ✓ - No regressions in existing tests - Code formatted with spotlessApply The hierarchical storage configuration feature is now fully verified at the backend level. The resolution logic correctly walks the entity hierarchy (table → namespace(s) → catalog) to find storage configuration, enabling tables and namespaces to override storage settings inherited from parents. Next: Step 6 will design the Management API to expose this feature to users. Related: Inline Storage Configuration Feature Implementation feat: Add hierarchical storage configuration support for tables and namespaces This commit implements Step 4 of the inline storage configuration feature, enabling tables and namespaces to override catalog-level storage settings. Changes: - Enhanced FileIOUtil.findStorageInfoFromHierarchy() with improved docs and debug logging for hierarchical resolution (table → namespace → catalog) - Added comprehensive FileIOUtilTest with 8 test cases covering all resolution scenarios including nested namespaces - Removed duplicate PolarisEntityUtil and PolarisEntityUtilTest that were created in Step 3 but are not needed in production code Key insight: The hierarchical resolution was already working through the existing StorageAccessConfigProvider → FileIOUtil production code path. The credential cache requires the entity (not just config) for cache keys, which is why FileIOUtil returns Optional<PolarisEntity>. Testing: - All 8 FileIOUtilTest tests pass - Verifies table override, namespace fallback, catalog fallback - Tests nested namespace scenarios (4 levels deep) - Tests edge cases (no config, partial hierarchies) Related: Step 2 added getStorageConfigurationInfo() to NamespaceEntity and TableLikeEntity, which enabled this hierarchical resolution. feat: Add storage configuration support to TableLikeEntity and NamespaceEntity Add storage configuration getter methods to TableLikeEntity and NamespaceEntity, enabling table-level and namespace-level storage configuration overrides. This follows the existing pattern established by CatalogEntity and maintains consistency across the codebase. Changes: - Add getStorageConfigurationInfo() to TableLikeEntity base class - Applies to all table types: IcebergTable, IcebergView, GenericTable - Retrieves config from entity's internal properties - Returns null if no configuration is set (backward compatible) - Add getStorageConfigurationInfo() to NamespaceEntity - Enables namespace-level storage configuration overrides - Supports both single and nested namespaces - Follows same pattern as CatalogEntity - Add comprehensive unit tests - TableLikeEntityStorageConfigTest: 5 tests covering IcebergTable, IcebergView, and GenericTable entities - NamespaceEntityStorageConfigTest: 6 tests covering single/nested namespaces, AWS/Azure configs, and base location integration Design rationale: - Methods added to specific entity classes (TableLikeEntity, NamespaceEntity) rather than base PolarisEntity class - Maintains consistency with CatalogEntity implementation - Keeps administrative entity types (Principal, Role, etc.) clean - Uses existing serialization mechanism via PolarisEntityConstants - No changes needed to FileIOUtil.findStorageInfoFromHierarchy() - it already detects storage configs in internalProperties This implementation enables hierarchical storage configuration resolution: Table → Namespace → Catalog, with existing FileIOUtil automatically supporting the new entity-level configurations. Related to: Inline Storage Configuration feature
Add effective-source metadata for namespace/table storage-config GET responses to remove ambiguity during hierarchy fallback and post-DELETE behavior. This complements PR apache#3409 (storageName-based credentials): once hierarchy resolution picks the effective config, callers can now observe where it came from while storageName continues to drive named-credential lookup when enabled. - add X-Polaris-Storage-Config-Source response header for GET namespace/table - introduce source-aware effective resolution helpers in PolarisServiceImpl - keep existing storageName conversion/fallback behavior unchanged - extend PolarisStorageConfigIntegrationTest to assert source across fallback paths
Namespace GET effective storage config skipped intermediate parent namespaces: for ns1.ns2.ns3 with a config only on ns1, the resolver jumped directly to the catalog instead of stopping at ns1. The root cause was resolveEffectiveNamespaceStorageConfig performing a single entity lookup then a catalog fallback, rather than using the same FileIOUtil.findStorageInfoFromHierarchy leaf-to-root walk that the table GET path already uses correctly. This interacts with PR apache#3409 (storageName): if a parent namespace carries a storageName for named-credential isolation, and the leaf namespace has no config, the PR apache#3409 credential dispatch would silently use the catalog's storageName instead of the ancestor's. The fix closes that hole for namespace-level hierarchy resolution. Changes: - PolarisAdminService: add resolveNamespacePath() returning the full PolarisResolvedPathWrapper (catalog root → leaf namespace); refactor resolveNamespaceEntity() to delegate to it, eliminating duplicated manifest-resolution logic. - PolarisServiceImpl.resolveEffectiveNamespaceStorageConfig: replace direct entity + catalog lookup with resolveNamespacePath() + findStorageInfoFromHierarchy(), aligning namespace GET with table GET. - PolarisServiceImpl.resolveEffectiveTableStorageConfigWithSource: remove redundant pre-calls to resolveNamespaceEntity() and resolveTableEntity(); resolveTablePath() already validates existence and throws NotFoundException, so the extra round-trips were wasteful. - Add Javadoc to STORAGE_CONFIG_SOURCE_HEADER, StorageConfigSource, ResolvedStorageConfig, toInternalModel/toApiModel (document storageName invariant), storageConfigFromEntity, sourceForEntityType, and both resolveEffective* helpers. - PolarisStorageConfigIntegrationTest: add testGetNamespaceStorageConfigFallsBackToAncestorNamespace — creates ns1 → ns1.ns2 → ns1.ns2.ns3, places config on ns1 only, asserts namespace GET for ns1.ns2.ns3 returns source=NAMESPACE (not CATALOG).
7 participants
Introduces 2 new configurations:
polaris.storage.aws.<storage-name>.access-keypolaris.storage.aws.<storage-name>.secret-keyUsers can set these to override aws credentials per catalog. Each catalog looks up it's keys by using host of the first uri in the list of AllowedLocations or it's own configured storageName if available.
Fixes #2970