Revanth14 commented on PR #2163: URL: https://github.com/apache/iceberg-go/pull/2163#issuecomment-6101023912
> The `adls.token` path itself looks good — the factory branch sits in the right precedence slot, the static one-hour-cache credential matches Java and PyIceberg, and the Azure-side test matrix (precedence, empty-token fallthrough, rejected-token no-fallback, header/SAS assertions) is thorough. > > What I'd want resolved before this merges is the stale-credential merge refactor it's bundled with. The ADLS clearing deletes every account's SAS, connection string and shared key, not just the ones for the account the vended token covers, and the comment justifies that as safe because props is a per-location copy — which is only true inside `propertiesForLocation`. At CreateTable/RegisterTable/loadTableWithMode and RefreshTableCredentials, `config` is the whole table/catalog map, so a vended token for one account can silently clear a working credential for another and that read falls through to the default chain or 403s. > > The same move also widens the existing S3/GCS clear-then-copy to those table-wide sites, which is a behavior change for non-Azure users — a server that returns a static key pair in config and vends only a session token now has the pair wiped — and none of the three new sites have a test. I'd lean toward landing the Azure feature on its own and splitting the merge-helper change into a second PR described as a semantics change, or at minimum scoping the ADLS clearing to the matched account, correcting the comment, and adding coverage through the real Create/Register/Load handlers. Details inline. Thanks for the review. Addrssed changes. @laskoviymishka -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
