Add dry_run support to GenerateDataKeyPair/GenerateDataKeyPairWithoutPlaintext. #12505

macpak · 2025-04-09T06:01:20Z

Motivation

I am adding support for the dry_run parameter to the GenerateDataKeyPair and GenerateDataKeyPairWithoutPlaintext operations to align with AWS behavior and enhance validation capabilities.

Reference: AWS KMS Dry Run Documentation

Changes

Before these changes, the GenerateDataKeyPair and GenerateDataKeyPairWithoutPlaintext operations in KMS did not take the dry_run parameter into account. This PR updates both operations to properly respect the dry_run flag, aligning their behavior with AWS expectations.

Since IAM permission checks are not yet implemented in KMS, the primary purpose of this functionality is to determine whether a request would succeed (in which case a DryRunOperationException is raised when dry_run=True) or fail due to validation errors, rather than to enforce or simulate IAM permissions

Testing

You can verify the behavior by executing the following CLI commands:

awslocal kms generate-data-key-pair \
  --key-id <key_id> \
  --key-pair-spec RSA_2048 \
  --dry-run

awslocal kms generate-data-key-pair-without-plaintext \
  --key-id <key_id> \
  --key-pair-spec RSA_2048 \
  --dry-run

When the request is valid, these commands should return a DryRunOperationException. If the input is invalid, a validation error will be returned instead.

localstack-bot

Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.

localstack-bot · 2025-04-09T06:01:33Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

macpak · 2025-04-09T06:35:53Z

I have read the CLA Document and I hereby sign the CLA

alexrashed

Thanks a lot for the great contribution! In principal the code is looking great! I just found an issue (only parts of the validations are considered for the dry run), and I posted some more nitpicks and questions.
Once they have been addressed we are good to go! 🚀 💯

localstack-core/localstack/services/kms/exceptions.py

localstack-core/localstack/services/kms/utils.py

tests/aws/services/kms/test_kms.py

localstack-core/localstack/services/kms/models.py

localstack-core/localstack/services/kms/utils.py

tests/aws/services/kms/test_kms.py

tests/unit/services/kms/test_kms.py

macpak

Addressed the review comments and applied the suggested fixes.

localstack-core/localstack/services/kms/models.py

tests/aws/services/kms/test_kms.py

localstack-core/localstack/services/kms/utils.py

localstack-core/localstack/services/kms/exceptions.py

tests/unit/services/kms/test_kms.py

alexrashed

Thanks a lot for addressing the comments! This is already looking great! 🤩
I only have a few more small comments / questions. We need to make sure that the test is validated, and I would like to quickly discuss the UnsupportedOperationException, the rest is just nitpicks / not blocking.
It seems that there is a small conflict though, so maybe you could quickly rebase this PR on the latest changes on master?

tests/unit/services/kms/test_kms.py

tests/aws/services/kms/test_kms.validation.json

localstack-core/localstack/services/kms/models.py

…ry run capability in data key pair generation

… generation with dry run support

…hout plaintext

… dry run operations

…pair generation methods

…r function

…orts

…idation logic

…in KMS generate data key pair tests

…_capable function

…e all valid key specs

…intext

alexrashed

Thanks a lot for addressing the comments once more! The validation is looking good, and the reasoning on the KmsCryptoKey is sound. I really only want to check that we don't introduce an issue with the switch from the UnsupportedOperationException to the ValidationException in some cases. ;)

localstack-core/localstack/services/kms/models.py

…andling in data key pair generation tests

… function for improved encapsulation

… of valid specs for clarity

macpak

The remaining comment has been addressed.

localstack-core/localstack/services/kms/models.py

alexrashed

Thanks a lot for addressing the comments, really following up on each and every one! I'll go ahead and directly merge this one! 🤩
Thanks for the contribution! 💯

tests/unit/services/kms/test_kms.py

localstack-core/localstack/services/kms/models.py

localstack-bot reviewed Apr 9, 2025

View reviewed changes

macpak force-pushed the feature/kms-dry-run-generate-key-pair branch 2 times, most recently from 3d91e00 to 2fee7cd Compare April 9, 2025 06:32

localstack-bot added a commit that referenced this pull request Apr 9, 2025

@macpak has signed the CLA in #12505

a142bbe

alexrashed self-assigned this Apr 9, 2025

alexrashed added the semver: patch Non-breaking changes which can be included in patch releases label Apr 9, 2025

alexrashed self-requested a review April 9, 2025 07:30

macpak marked this pull request as ready for review April 9, 2025 07:46

macpak requested a review from sannya-singal as a code owner April 9, 2025 07:46

macpak changed the title ~~Feature/kms dry run generate key pair~~ Add dry_run support to GenerateDataKeyPair/GenerateDataKeyPairWithoutPlaintext. Apr 9, 2025

alexrashed requested changes Apr 9, 2025

View reviewed changes

macpak commented Apr 10, 2025

View reviewed changes

alexrashed reviewed Apr 10, 2025

View reviewed changes

macpak added 15 commits April 13, 2025 13:34

feat(kms): add dry run capability for data key pair generation

0dc2b13

feat(kms): update DryRunOperationException message and add test for d…

7cbb85d

…ry run capability in data key pair generation

feat(kms): add key specification validation and enhance data key pair…

b47395a

… generation with dry run support

test(kms): add validation for dry run in data key pair generation wit…

4f65f68

…hout plaintext

feat(kms): add pydoc for execute_dry_run_capable function

dadaa9a

fix(kms): clean up code formatting and improve exception handling for…

68048cf

… dry run operations

test(kms): add tests for execute_dry_run_capable function behavior

2cda28a

fix(kms): remove TODO comments regarding dry run support in data key …

5127e4f

…pair generation methods

fix(tests): remove unnecessary blank line in test_alias_name_validato…

c7ff721

…r function

refactor(kms): standardize docstrings using reST format

975446b

refactor(kms): use DryRunOperationException from api and clean up imp…

998e6f1

…orts

fix(kms): add support for SYMMETRIC_DEFAULT key spec and clean up val…

638c081

…idation logic

fix(tests): remove redundant assertions for DryRunOperationException …

e86cc9d

…in KMS generate data key pair tests

fix(kms): remove unused UnsupportedOperationException import

cf4c9a1

fix(kms): enhance DryRunOperationException message in execute_dry_run…

698bf93

…_capable function

fix(kms): update key specification validation error message to includ…

32c20c0

…e all valid key specs

macpak force-pushed the feature/kms-dry-run-generate-key-pair branch from 2e96c89 to 32c20c0 Compare April 13, 2025 11:38

fix(tests): add validation date for new dry run test case without pla…

7a42bb3

…intext

alexrashed reviewed Apr 14, 2025

View reviewed changes

localstack-core/localstack/services/kms/models.py Show resolved Hide resolved

localstack-core/localstack/services/kms/models.py Show resolved Hide resolved

macpak added 3 commits April 18, 2025 11:06

fix(kms): improve key specification validation and update exception h…

6dc51a6

…andling in data key pair generation tests

fix(kms): move all valid key specifications into the raise_validation…

f83d007

… function for improved encapsulation

fix(kms): update key specification validation to use a hardcoded list…

1768909

… of valid specs for clarity

macpak commented Apr 18, 2025

View reviewed changes

localstack-core/localstack/services/kms/models.py Show resolved Hide resolved

alexrashed approved these changes Apr 23, 2025

View reviewed changes

tests/unit/services/kms/test_kms.py Show resolved Hide resolved

localstack-core/localstack/services/kms/models.py Show resolved Hide resolved

alexrashed merged commit 77f30dc into localstack:master Apr 23, 2025
29 checks passed

Uh oh!

Add dry_run support to GenerateDataKeyPair/GenerateDataKeyPairWithoutPlaintext. #12505

Add dry_run support to GenerateDataKeyPair/GenerateDataKeyPairWithoutPlaintext. #12505

Uh oh!

Conversation

macpak commented Apr 9, 2025

Motivation

Changes

Testing

Uh oh!

localstack-bot left a comment

Choose a reason for hiding this comment

Uh oh!

localstack-bot commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

macpak commented Apr 9, 2025

Uh oh!

alexrashed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

macpak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexrashed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexrashed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

macpak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexrashed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

localstack-bot commented Apr 9, 2025 •

edited

Loading