Skip to content

Conversation

@MonkeyCanCode
Copy link
Contributor

@MonkeyCanCode MonkeyCanCode commented Jan 22, 2026

This PR addressed one of the issues reported in #3440 where when end-user is non-AWS S3-compatible backend and have region set on the catalog property. With current code, we are determining if a S3 backend if AWS or not based on checking if account id and region are both set which is a bit problematic IMO. The account id here is derived from IAM role and the region is be set implicitly. That being said, when an user is trying to use assume role to auth and interact with S3-compatible storage, it can cause good amount of confusion as our current code base will add wildcard KMS policy to it if the backend is "AWS" (in this case, if a region and account id are both present...and region itself is valid for MinIO AFAIK and it default to us-east-1).

As discussed in #3496, instead of fixing the check for isAwsS3, we will proceed with a new catalog config instead.

Sample outputs:

# setup
./polaris --profile dev catalogs create --storage-type s3 --role-arn "arn:aws:iam::123456789012:role/polaris-warehouse-access" --default-base-location "s3://analytics-bucket/warehouse/" quickstart_catalog
./polaris --profile dev catalogs create --storage-type s3 --role-arn "arn:aws:iam::123456789012:role/polaris-warehouse-access" --default-base-location "s3://analytics-bucket-no-kms/warehouse/" --no-kms quickstart_catalog_no_kms

# outputs
./polaris --profile dev catalogs get quickstart_catalog | jq
{
  "type": "INTERNAL",
  "name": "quickstart_catalog",
  "properties": {
    "default-base-location": "s3://analytics-bucket/warehouse/"
  },
  "createTimestamp": 1769052561792,
  "lastUpdateTimestamp": 1769052561792,
  "entityVersion": 1,
  "storageConfigInfo": {
    "storageType": "S3",
    "allowedLocations": [
      "s3://analytics-bucket/warehouse/"
    ],
    "roleArn": "arn:aws:iam::123456789012:role/polaris-warehouse-access",
    "allowedKmsKeys": [],
    "stsUnavailable": false,
    "pathStyleAccess": false,
    "kmsUnavailable": false
  }
}

./polaris --profile dev catalogs get quickstart_catalog_no_kms | jq
{
  "type": "INTERNAL",
  "name": "quickstart_catalog_no_kms",
  "properties": {
    "default-base-location": "s3://analytics-bucket-no-kms/warehouse/"
  },
  "createTimestamp": 1769052562086,
  "lastUpdateTimestamp": 1769052562086,
  "entityVersion": 1,
  "storageConfigInfo": {
    "storageType": "S3",
    "allowedLocations": [
      "s3://analytics-bucket-no-kms/warehouse/"
    ],
    "roleArn": "arn:aws:iam::123456789012:role/polaris-warehouse-access",
    "allowedKmsKeys": [],
    "stsUnavailable": false,
    "pathStyleAccess": false,
    "kmsUnavailable": true
  }
}

Mailing list: https://lists.apache.org/thread/pksdwpzl0243ny1rvr8w4p6yz1m0xssn

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

dimas-b
dimas-b previously approved these changes Jan 22, 2026
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thanks, @MonkeyCanCode !

Since this PR contains a REST API change, I believe we ought to open a dedicated dev ML discussion thread for it too (if only as a review notification) and have (lazy) consensus there before merging.

I'm approving in GH proactively.

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jan 22, 2026
@MonkeyCanCode MonkeyCanCode changed the title Add support for no KMS with s3-compatible backend (feat) Add support for no KMS with s3-compatible backend Jan 22, 2026
@MonkeyCanCode
Copy link
Contributor Author

MonkeyCanCode commented Jan 22, 2026

LGTM 👍 Thanks, @MonkeyCanCode !

Since this PR contains a REST API change, I believe we ought to open a dedicated dev ML discussion thread for it too (if only as a review notification) and have (lazy) consensus there before merging.

I'm approving in GH proactively.

Sounds good. ML had being added to the description.

@MonkeyCanCode
Copy link
Contributor Author

Thanks for review @dimas-b . I will have this up until next Monday as we collecting more feedback from mailing list as well as allow more review time as it is API change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants