Hello, I think the PR is ok to merge as is, however, I anticipate more cases like this will appear as Polaris integrates with more storage systems. So I think we need to start thinking proactively about how to address such cases. One way I can think of is to establish a feature flag hierarchy; that will be used to determine how to apply future storage features. I have not yet had time to look but it is also possible that a similar concern will exist in some other parts of the application as more integrations are supported.
On Fri, Jan 23, 2026 at 9:58 AM Dmitri Bourlatchkov <[email protected]> wrote: > Thanks for debugging and fixing this issue, Yong! > > I believe the proposed change is backward-compatible for existing Polaris > deployments that are not affected by the issue. The change allows affected > users to achieve correct behaviour by updating Storage Config in the > affected catalogs. > > +1 to merge PR #3501. > > Cheers, > Dmitri. > > On Thu, Jan 22, 2026 at 5:05 PM Yong Zheng <[email protected]> wrote: > > > Hello, > > > > While debugging https://github.com/apache/polaris/issues/3440, we found > > there are couple issues when user is trying to use assume role with STS > on > > a S3-compatible storage but doesn't use any KMS key. One of the issue if > > our current code does following will add wildcard KMS policies regardless > > if KMS keys are defined or not as long as it thinks you are using AWS SS > > (PR https://github.com/apache/polaris/pull/3493 is created to handle the > > case when it is non-AWS S3). > > > > The determination of AWS S3 here is defined by checking if region and > > account id (account id is derived from IAM role) are being set ( > > > https://github.com/MonkeyCanCode/polaris/blob/main/polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java#L320 > ). > > This check itself is a bit problematic as both are valid for some non-AWS > > S3-compatible storage such as localstack and MinIO (e.g. MinIO does > > supports IAM role and default region itself is us-east-1). Here is a > > different discussion I created to discuss if we want to refine the logic > > for how to check if user is on AWS or not ( > > https://lists.apache.org/thread/l2ts4czz2bh4qf1swy4n63f2xrznnss5) and > > here is the sample implementation ( > > https://github.com/apache/polaris/pull/3496). For the sample > > implementation, it will work for the most of the case but it can cause FP > > for localstack and potentially AWS S3 Outpost as they will be using > non-AWS > > domains but compatible with IAM role and KMS. > > > > While going over the existed code, this is the only place where we are > > using "isAwsS3" logic to control a workflow. Thus, we all believed > > introducing a new catalog feature flag to disable KMS completely may be > > cleaner until we refactor AwsStorageConfigurationInfo.java to better > handle > > AWS vs non-AWS S3 backends (detail discussion can be found in the sample > > implementation above for refined "isAwsS3" check). > > > > With the new purposed change ( > https://github.com/apache/polaris/pull/3501), > > anything that is not being implicitly set will be default to AWS S3. The > > existed "isAwsS3" will still honor the users who may be using this flawed > > check when the flag is not being set to maintain the backward compatible. > > > > Thanks, > > Yong Zheng > > >
