lasdf1234 commented on PR #11294: URL: https://github.com/apache/gravitino/pull/11294#issuecomment-4619815215
> Hi, thanks for the detailed PR! I noticed a potential gap in the REST catalog proxy path. > > The current fix in `loadTableInternal` / `createTableInternal` / `stageTableCreateInternal` extracts credential config via `retrieveFileIOProperties(table.io())` → `fileIO.properties()`, then calls `buildRefreshCredentialEndpoints` based on those properties. > > However, in Iceberg 1.11, when the upstream REST catalog returns credentials via `LoadTableResponse.credentials()` (the standard structured field), `RESTSessionCatalog` injects them through `SupportsStorageCredentials#setCredentials()` — **not** into `fileIO.properties()`. These two are separate fields in `S3FileIO` / `ResolvingFileIO` / `GCSFileIO`: `setCredentials()` writes to `storageCredentials`, while `properties()` only returns what was passed to `initialize()`. > > So if the upstream follows the Iceberg 1.11 spec and uses `storage-credentials: [...]` in its load-table response, `retrieveFileIOProperties(table.io())` will return empty credential keys, `filterCredentialProperties` will filter them out, and `buildRefreshCredentialEndpoints` will return nothing. The downstream client's load-table response ends up with neither initial credentials nor a refresh endpoint. > > The existing test `testLoadTableInjectsRefreshEndpoint` only covers the compat path (mocking `fileIO.properties()` with `s3.session-token`), not the standard `credentials` field path. > > Could you share your thoughts on this? One possible fix direction would be to also read `((SupportsStorageCredentials) table.io()).credentials()` and either forward those credentials (with endpoint rewriting) to the downstream `LoadTableResponse.credentials()` field, or at least use them to drive `buildRefreshCredentialEndpoints`. Would be happy to discuss the best approach. @FANNG1 Thanks your valuable suggestions and agreed on the gap. Kept the existing fileIO.properties() path and added an Iceberg 1.11 path that reads upstream credentials from SupportsStorageCredentials#credentials() -- 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]
