dennishuo commented on PR #3699: URL: https://github.com/apache/polaris/pull/3699#issuecomment-4228266712
Sorry for the delay responding to this! I've been digesting this more and I think there's a bigger convergence we can align with. I agree with what you said that if we only reintroduce PolarisCredentialVendor in the proposed way, it's a bit confusing whether a `StorageIntegration` itself is also fundamentally a CredentialVendor interface (with partially-bound request-scoped state). I'm mulling over whether what you describe is topologically equivalent to the other long-term direction we wanted to go, just with different class names. **The TL;DR:** We should incrementally try to converge with how the analogous ConnectionConfig flow works for federation, especially since the sigv4-to-Glue federation credentials are pretty much exactly the same as sigv4-to-s3 storage credentials. Your proposal does seem to align somewhat, so I'd say go ahead and try to have StorageIntegration be the first-class entrypoint. We should consider accordingly moving the items related to request scope (`realmConfig`, `storageConfig`) out of the `getSubscopedCreds` arguments and force them to be bound at construction-time, if `StorageIntegrationProvider.getStorageIntegration(resolvedEntityPath)` is intended to be the stateful lookup. Extended analysis: Very abstractly, there are these four things: 1. "User-Requested Config" -- Basically the StorageConfingInfo that comes in from a CreateCatalog request initially 2. "Full config with system-assigned internal references" -- This is currently a confusing "hidden" ghost entity but only hinted at due to `createStorageIntegration` and `persistStorageIntegrationIfNeeded` and the fact that `PolarisCredentialVendor` takes the `PolarisEntity` when getting subscoped credentials. The `StorageIntegration` object itself kind of doubles as an effective "Data Access Object" (DAO) that wraps this "ghost entity" 3. "Handle into the entity-specific credential vending layer" - The `StorageIntegration`, by virtue of being the DAO into the hidden layer, basically fills this role. Importantly, the `IntegrationPersistence.loadPolarisStorageIntegration` method is implied to interact with persistence because it takes `PolarisBaseEntity` as an argument. 4. "Factory for the credential-vending handle, that requires full config with system-assigned state" - This is the `IntegrationPersistence.loadPolarisStorageIntegration` method basically The fact that StorageIntegrationProvider at `HEAD` only takes what amounts to (1) - the user-requested config - and serves the purpose of (4) to provide (3) is what causes confusion. What I remembered is that we actually already did try to redesign the flow of such credential-handling to accommodate this same concept in a more explicit way. See: 1. Secrets management for Catalog Federation: https://docs.google.com/document/d/1JPNx5vL4vM8DqwRwnBIPiQxwN4MXOdGx4Ki0j7vgwSM/edit?tab=t.0 2. The sigv4 support for that secrets management that is really the same flow as storage-credential vending, but for a different set of IAM actions: https://github.com/apache/polaris/pull/2190 Reposting @XJDKC 's diagram here: <img width="5209" height="3351" alt="polaris_connection_config_sigv4" src="https://github.com/user-attachments/assets/2565fb1f-087c-4470-bcef-6c5f407bcf2b"; /> In this world: 1. User-Requested config is the `ConnectionConfigInfoModel` 2. Config with System-assigned references is the `ConnectionConfigInfoDpo` that contains an "identityReference" 3. Handle into credential-vending - This is actually multi-step - [AwsIamServiceIdentityCredential](https://github.com/apache/polaris/blob/main/polaris-core/src/main/java/org/apache/polaris/core/identity/credential/AwsIamServiceIdentityCredential.java) (concrete `ServiceIdentityCredential` class) is basically the handle into the *service identity* that can be used to further mint short-lived access, and [SigV4ConnectionCredentialVendor](https://github.com/apache/polaris/blob/main/runtime/service/src/main/java/org/apache/polaris/service/credentials/connection/SigV4ConnectionCredentialVendor.java) (concrete `ConnectionCredentialVendor ` class) is the thing that actually gets the short-lived credentials, as evidenced by it holding an `StsClient`. 4. Factory that takes internal config/reference to get the handle - `ServiceIdentityProvider` I guess the rough equivalences are: - `ServiceIdentityProvider` is kind of like `StorageIntegrationProvider` but it's also kind of like `IntegrationPersistence` since it also defines the `allocate` method - `ServiceIdentityCredential` is like `StorageIntegration` - `ConnectionCredentialVendor ` is like a combination of `StorageIntegration`, `PolarisCredentialVendor`, `StorageCredentialsVendor`, and `StorageAccessConfigProvider` One key difference in behavior is in the ConnectionConfig path, I guess we ended up putting the following all the way in `PolarisAdminService`: Optional<ServiceIdentityInfoDpo> serviceIdentityInfoDpoOptional = serviceIdentityProvider.allocateServiceIdentity(connectionConfigInfo); entity = new CatalogEntity.Builder(entity) .setConnectionConfigInfoDpoWithSecrets( connectionConfigInfo, processedSecretReferences, serviceIdentityInfoDpoOptional.orElse(null)) .build(); This is in contrast to putting the "allocate" hook into the inner transaction-aware block in PolarisMetaStoreManager. So it doesn't support as good of transactional semantics as how we handle StorageConfig, but on the plus side, it forces the referential service-identity info to be first-class in the `CatalogEntity` itself, so we don't have fully invisible "ghost entities" representing the underlying credential allocation. Overall, fully merging the Storage credential flow into the Connection credential flow is probably too big of an undertaking to do here, and maybe we also want to adjust the Connection credential code at the same time, but I think any incremental refactoring to make it structurally compatible is probably the right direction. All this to say, yes, I think your proposal is somewhat structurally compatible, with a couple key points: 1. StorageIntegrationProvider changes from being semantically `allocateForConfig(userSpecifiedConfig)` to being `fetchForEntity(entityPath)` 2. StorageIntegration *instances* should be thought of as fully representing the resolved entityPath already, rather than being application/type-scoped -- this might mean we want `realmConfig` and `storageConfig` to be constructor-time bindings rather than arguments to `getSubscopedCreds` (where the `storageConfig` will actually be derived from `entityPath` during construction by `StorageIntegrationProvider`) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
