lasdf1234 commented on PR #11294: URL: https://github.com/apache/gravitino/pull/11294#issuecomment-4603795407
> I found a few issues after checking this PR against the local Iceberg 1.11 code under `~/opensource/iceberg`: > > 1. `getRESTTableCredentials()` silently converts upstream REST failures into an empty credential response. > > `getRESTTableCredentials()` catches all non-`NoSuchTableException` exceptions and returns an empty `LoadCredentialsResponse`. This differs from `getLocalTableCredentials()`, which only catches local credential-generation `ServiceUnavailableException`. If the upstream REST catalog returns auth/403/5xx, Gravitino hides the real error and the Iceberg client later fails inside `VendedCredentialsProvider.refreshCredential()` with `Invalid S3 Credentials: empty`, which is misleading and makes the original upstream failure hard to diagnose. > > Please let upstream auth/403/5xx errors propagate to the REST response, and only downgrade truly expected/no-credential cases. > > 2. `AwsIrsaCredential` can emit a non-positive S3 token expiry. > > The new `CredentialPropertyUtils.toIcebergProperties()` path treats `AwsIrsaCredential` like a temporary S3 credential and writes `s3.session-token-expires-at-ms`. Iceberg 1.11 parses this with `Long.parseLong` and skips the initial cached credential when the expiry is already before the prefetch time. Unlike `S3TokenCredential`, `AwsIrsaCredential` currently does not validate `expireTimeInMs > 0`, so `0` can cause the client to refresh immediately and repeatedly. > > Please either validate `AwsIrsaCredential.expireTimeInMs() > 0` or reject non-positive expiry before adding the Iceberg expiry property. > > 3. ADLS refresh expiry key does not match the Iceberg expected host suffix. > > Gravitino emits the SAS token key as `adls.sas-token.<account>.dfs.core.windows.net`, but the expiry key as `adls.sas-token-expires-at-ms.<account>`. Iceberg ADLS refresh uses `ADLSLocation.host()` for both keys, for example `account.dfs.core.windows.net`, and `VendedAdlsCredentialProvider` requires `adls.sas-token-expires-at-ms.<host>`. With a normal `abfss://[email protected]/...` table location, refresh will fail because the expiry key is missing. > > Please use the same full host suffix for both ADLS SAS token and expiry keys. > > 4. OSS refresh endpoint is advertised, but the local Iceberg OSS module has no refresh provider. > > `IcebergRESTUtils.toRESTCredential()` adds `client.refresh-credentials-endpoint` for `OSSTokenCredential`. In the local Iceberg code, `AliyunProperties` only reads `client.access-key-id`, `client.access-key-secret`, and `client.security-token`; there is no OSS vended refresh endpoint/provider. So this currently provides initial temporary OSS credentials, but does not actually enable refresh. > > Please remove the OSS refresh endpoint until Iceberg OSS supports it, or document/prove the supported client path. 1.Now the upstream exception will be thrown and the code has been modified. 2.The expiration logic of AwsIrsaCredential has been made consistent with that of S3 token. 3.The issue of ADLS refresh expiry key has also been resolved. 4.The oss module of iceberg does not support the "vended refresh". This issue has been noted in the comments of the relevant methods in gravitino, and the corresponding code has also been modified. -- 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]
