Skip to content

Conversation

@dimas-b
Copy link
Contributor

@dimas-b dimas-b commented Dec 24, 2025

Following up on #2802

Checklist

  • 🛡️ Don't disclose security issues! (contact [email protected])
  • 🔗 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
Copy link
Contributor Author

dimas-b commented Dec 24, 2025

Copy link
Contributor

@adnanhemani adnanhemani 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 for this @dimas-b !

Copy link
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

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

Thanks a ton for penning it down @dimas-b !

Thank you @fivetran-ashokborra @fabio-rizzo-01 for working on these and the polaris community members for making this happen !

AWS [Key Management Service](https://docs.aws.amazon.com/kms/latest/developerguide/overview.html) (KMS) provides
a way to encrypt S3 data in AWS without exposing raw key material outside AWS services.

Apache Polaris supports using KMS in its catalogs backed by AWS S3 storage.
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: do we need (incubating) here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess not... the statement below already mentions a specific incubating version, plus the site's front page has the appropriate "incubating" designation.

This can be achieved by using the `--allowed-kms-key` CLI option to add zero or more extra KMS key ARNs to the
catalog's storage configuration.

Note: if the key material is rotated without introducing a new key ARN, no catalog changes are necessary.
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't fully get this part, can you please elaborate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rephrased

MonkeyCanCode
MonkeyCanCode previously approved these changes Dec 26, 2025
@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Dec 26, 2025
singhpk234
singhpk234 previously approved these changes Dec 29, 2025
Copy link
Contributor

@singhpk234 singhpk234 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 @dimas-b !

Added some questions on the timeline for the cli feature.

Apache Polaris supports using KMS in its catalogs backed by AWS S3 storage.

The core functionality is available via Polaris REST API since the `1.2.0-incubating` release.
CLI support will be made available in the release following `1.3.0-incubating`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we planning to CP #3330 this tp 1.3x ? and have a new RC ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess not. Let's not expand scope of 1.3.0

Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying to understand more what the above statement meant, 1.3.1 could also be a following release.
i think we mean 1.4 ideally and we are not sure if will be 2.0 ?

## Using Multiple KMS Keys

If the bucket used by the catalog has had multiple different KMS key ARNs associated with it over time,
Polaris needs to know all related key ARNs in order to properly form policies for accessing old and new data.
Copy link
Contributor

Choose a reason for hiding this comment

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

optional :

Suggested change
Polaris needs to know all related key ARNs in order to properly form policies for accessing old and new data.
Polaris needs to know all related key ARNs in order to properly form policies used for vending creds for accessing old and new data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx - updated

Comment on lines +88 to +89
This can be achieved by using the `--allowed-kms-key` CLI option to add zero or more extra KMS key ARNs to the
catalog's storage configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we plan to merge this blog post cli pr gets merged ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, CLI needs to go first

@fivetran-ashokborra
Copy link
Contributor

CC: @fivetran-ashokborra , @fabio-rizzo-01

LGTM, Thanks for the acknowledgement 🙏 @fabio-rizzo-01 thanks for working on this feature

@dimas-b
Copy link
Contributor Author

dimas-b commented Dec 30, 2025

I'm going to merge this PR in its current form. Willing to take feedback and make adjustments after merging.

@dimas-b dimas-b merged commit 118e342 into apache:main Dec 30, 2025
15 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Dec 30, 2025
@dimas-b dimas-b deleted the kms-blog branch December 30, 2025 20:39
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.

5 participants