For sure. I was thinking about the same as if the backend does support KMS, they may not be using the same set of policy statement (e.g. MinIO does support but their policy statements are a bit different). Currently AWS storage config doesn't have the ability to distinguish if the backend is truly AWS or not and the code work well for AWS S3 but may ran into various edge cases when dealing with non-AWS S3-compatible storage.
Thanks, Yong Zheng On 2026/01/23 17:26:37 Innocent Djiofack wrote: > 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 > > > > > >
