Improve KMS model by giograno · Pull Request #13558 · localstack/localstack

giograno · 2025-12-22T08:13:09Z

Motivation

This PR improves typing annotations for the KSM store. This is an ongoing effort needed for the new persistence serialization, heavily based on type annotations.

Changes

Fixed a few unsubscribed annotations (e.g., dict);
Remove the grant_names field from the store. This field was problematic as it was using a tuple as a dictionary key. I noticed that the data contained in this field is actually redundant (grants contains the same information), and it was used for O(1) lookup on grant names in the CreateGrant operation. With the current changes, the lookup is O(n).

Tests

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 4m 39s ⏱️ - 1h 49m 34s
767 tests - 4 386 759 ✅ - 3 997 8 💤 - 389 0 ❌ ±0
769 runs - 4 386 759 ✅ - 3 997 10 💤 - 389 0 ❌ ±0

Results for commit 83a28e4. ± Comparison against base commit 056da83.

This pull request removes 4386 tests.

tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

github-actions · 2025-12-22T08:24:09Z

Test Results - Preflight, Unit

23 046 tests +64 21 201 ✅ +61 6m 37s ⏱️ +23s
1 suites ± 0 1 845 💤 + 3
1 files ± 0 0 ❌ ± 0

Results for commit 83a28e4. ± Comparison against base commit 056da83.

♻️ This comment has been updated with latest results.

github-actions · 2025-12-22T08:32:33Z

Test Results (amd64) - Acceptance

7 tests ±0 5 ✅ ±0 3m 0s ⏱️ -1s
1 suites ±0 2 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 83a28e4. ± Comparison against base commit 056da83.

♻️ This comment has been updated with latest results.

github-actions · 2025-12-22T08:37:31Z

Test Results (amd64) - Integration, Bootstrap

5 files ± 0 5 suites ±0 17m 4s ⏱️ - 2h 16m 5s
791 tests - 4 768 783 ✅ - 4 216 8 💤 - 552 0 ❌ ±0
797 runs - 4 768 783 ✅ - 4 216 14 💤 - 552 0 ❌ ±0

Results for commit 83a28e4. ± Comparison against base commit 056da83.

This pull request removes 4768 tests.

tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

bentsku

LGTM! Interesting changes, we see the history of things and why they were made that way 😄 just put a comment about the data model of KMS, but this is out of scope of this PR! Nice work 💯

bentsku · 2025-12-22T11:21:44Z

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

    is_key_rotation_enabled: bool
    rotation_period_in_days: int
    next_rotation_date: datetime.datetime
-    previous_keys = [str]


bentsku · 2025-12-22T11:29:26Z

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

+        grant: KmsGrant | None = None
+        if grant_name:
+            for existing_grant in store.grants.values():
+                if existing_grant.metadata.get("Name") != grant_name:
+                    continue
+                if existing_grant.metadata.get("KeyId") == key_id:
+                    grant = existing_grant
+                    break
+
+        if grant is None:


interesting, I mean the change is okay, but ultimately I believe the grants should be part of the KMS Key metadata (a Grant seems to be always linked to a Key), so if it was stored there we wouldn't need to do all of this.

Maybe we could leave a TODO to change the data model such that the KmsKey would hold the grants?
At least from a quick look that's how it feels, that the Grant should be nested inside the Key itself.
Only exception when a Grant operation is spanning multiple keys is list_retirable_grants but I think that'd be fine. Anyway, I was just thinking why we got into this situation in the first place 😄
\cc @k-a-il

Make sense. I added a todo. I implemented it this way since I believe the change should not break pickle persistence (as the data is already replicated across the two fields). Is my assumption correct? The refactoring you mentioned might be a good use case to test migrations :D

I implemented it this way since I believe the change should not break pickle persistence (as the data is already replicated across the two fields). Is my assumption correct?

Yes, yes, totally! This is the correct change right now, it just made me think about why it was necessary in the first place 😄 and definitely for the migration, it would be a pretty good test case!

Thanks @bentsku and @giograno , I have created a ticket FLC-265 to address this issue with grants. I checked KMS docs and grant indeed can be connected only to one key, so, it makes sense to change the model 👍

giograno added 3 commits December 22, 2025 08:17

wip

9a7bc14

remove grant_names property

23ae1df

simplification

d1db44f

giograno requested a review from bentsku December 22, 2025 09:44

giograno marked this pull request as ready for review December 22, 2025 09:44

giograno requested a review from k-a-il as a code owner December 22, 2025 09:44

bentsku approved these changes Dec 22, 2025

View reviewed changes

adding a todo

83a28e4

giograno merged commit 64e325c into main Dec 22, 2025
42 checks passed

giograno deleted the kms-store branch December 22, 2025 19:06

giograno mentioned this pull request Dec 23, 2025

Fix KMS model annotations #13563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve KMS model#13558

Improve KMS model#13558
giograno merged 4 commits intomainfrom
kms-store

giograno commented Dec 22, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Dec 22, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Dec 22, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Dec 22, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Dec 22, 2025 •

edited

Loading

Uh oh!

bentsku left a comment

Uh oh!

bentsku Dec 22, 2025

Uh oh!

bentsku Dec 22, 2025

Uh oh!

giograno Dec 22, 2025

Uh oh!

bentsku Dec 22, 2025

Uh oh!

k-a-il Dec 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

giograno commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Tests

Related

Uh oh!

github-actions bot commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

github-actions bot commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results - Preflight, Unit

Uh oh!

github-actions bot commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results (amd64) - Acceptance

Uh oh!

github-actions bot commented Dec 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results (amd64) - Integration, Bootstrap

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

bentsku Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

bentsku Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

giograno Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

bentsku Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

k-a-il Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

giograno commented Dec 22, 2025 •

edited

Loading

github-actions bot commented Dec 22, 2025 •

edited

Loading

github-actions bot commented Dec 22, 2025 •

edited

Loading

github-actions bot commented Dec 22, 2025 •

edited

Loading

github-actions bot commented Dec 22, 2025 •

edited

Loading