mj006648 commented on PR #4451: URL: https://github.com/apache/polaris/pull/4451#issuecomment-4466176105
Thanks for the thorough review, @dimas-b! Pushed `f5eae4e` addressing your comments: **S3 page** - L62: removed the "if you skip external ID" guidance and recommended `externalId` as default. - L67/79: dropped `userArn` from both the JSON example and the surrounding prose since `AwsStorageConfigurationInfo` does not use it. - L71: added a one-line note about the bearer token being the Polaris admin OAuth2 token, with a relref to the parent production-config page. - L102 (KMS): clarified that `currentKmsKey` and `allowedKmsKeys` are processed independently in `AwsCredentialsStorageIntegration`, so the write key has to be repeated in `allowedKmsKeys`. Fixed the IAM action wording accordingly. - L120: retitled the page to "Configuring S3 Storage". - L130: reconciled the STS subsection — STS-unsupported backends turn off vended credentials entirely and the client must drop the `X-Iceberg-Access-Delegation` header, instead of "falling back to long-lived credentials inside the vending flow". - L187: added a short paragraph noting the page covers native Polaris auth and external IdPs are supported but documented elsewhere. - L217: softened the client-side `s3.*` block — Polaris returns `endpoint`/`path-style-access`/`region` in the catalog config response, and the explicit block is now framed as "only needed where Trino requires it to be explicit (e.g. S3-compatible endpoints)". Please double-check that wording matches what you had in mind; if Polaris's response is enough on its own for the AWS S3 case, I am happy to drop the explicit block from the AWS path entirely. **Azure page** - L53: recommended Kubernetes Secret refs rather than plain env for `AZURE_CLIENT_SECRET` (and `POLARIS_BOOTSTRAP_CREDENTIALS`). - L63/64 (HNS): rewrote the bullet — HNS is not required by Polaris/Iceberg for table operations themselves; the main effect is SAS-token downscoping (directory vs. container). Atomic-commit framing removed. - L102 (`multiTenantAppName` / `consentUrl`): marked as informational only since current Apache Polaris code does not use them when communicating with Azure APIs. - L118: added a bullet that the `hierarchical` field must match the actual HNS state, otherwise runtime access errors occur. Re: L217 (Trino S3 block) — would be glad to drop more of it if Polaris's catalog config response is sufficient end-to-end on the Trino side. I have only verified the Trino path against MinIO with the explicit block in place; I have not tested removing it. -- 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]
