dennishuo opened a new pull request, #208: URL: https://github.com/apache/polaris/pull/208
# Description The previous default initialization of a `catalogFileIO` that doesn't take into consideration any kind of subscoping is fundamentally a bug if it ever kicks in for real isolated-server scenarios where "application defaults" aren't appropriate for any storage interactions. Some unittest scenarios relied on this "default" initialization but this masked bugs where methods like `registerTable` really do need to go through the full subscoping flow, but silently "succeeded" in test cases just because the application default credentials leaked into the test environment. Refactoring of FileIO initialization in `BasePolarisCatalog` now makes it more explicit as to when we expect to use defaults or when we require going through subscoping flows. Conveniently, this also provides a clean way to configure a server to skip all subscoping flows entirely and always allow application defaults and/or table-config inheritance to define the credentials used to interact with storage. (E.g. provides a way to achieve https://github.com/apache/polaris/pull/77/files just by setting SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION=true) Overview: - Add SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION configuration option for integration tests that want to use "fake" s3 paths or server deployments that don't need to perform all the assumeRole subscoping and instead just use application defaults - Refactor to properly inherit tableProperties in FileIO initialization and remove redundant callsites, ensure consistent usage of `refreshIOWithCredentials` - No longer initialize `catalogFileIO` by default; only set it in `BasePolarisCatalogTest` where client/server behaviors are mixed - Override `registerTable` to correctly use the subscoping flow to initialize a FileIO Fixes # (issue) ## Type of change Please delete options that are not relevant. - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update # How Has This Been Tested? Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration - [ ] Test A - [ ] Test B **Test Configuration**: * Firmware version: * Hardware: * Toolchain: * SDK: # Checklist: Please delete options that are not relevant. - [ ] I have performed a self-review of my code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules - [ ] If adding new functionality, I have discussed my implementation with the community using the linked GitHub issue -- 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]
