Skip to content

Add validation for tags in kms service #12142

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Jan 22, 2025
Merged

Add validation for tags in kms service #12142

merged 6 commits into from
Jan 22, 2025

Conversation

k-a-il
Copy link
Contributor

@k-a-il k-a-il commented Jan 17, 2025 •
edited
Loading

Motivation

Implement validation for tags in the KMS service with the following rules:

  • Duplicate tag keys are not permitted
  • A maximum of 50 tags can be applied
  • Enforce a limit on the length of tag keys and tag values
  • Tag keys cannot use the reserved aws: prefix

Changes

  • Added new test cases for invalid tag keys and values, excessive tag counts (over 50 tags), overly long tag keys and values, and duplicate tag keys.
  • Introduced a new method for tag validation and updated the add_tags method.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 17, 2025 •
edited
Loading

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   4m 5s ⏱️ - 1h 48m 49s
606 tests  - 3 377  595 ✅  - 3 072  11 💤  - 305  0 ❌ ±0 
608 runs   - 3 377  595 ✅  - 3 072  13 💤  - 305  0 ❌ ±0 

Results for commit afbcc62. ± Comparison against base commit ece83c8.

This pull request removes 3385 and adds 8 tests. Note that renamed tests count towards both. 
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]
…

tests.aws.services.kms.test_kms.TestKMS ‑ test_create_key_with_invalid_tag_key[lowercase_prefix]
tests.aws.services.kms.test_kms.TestKMS ‑ test_create_key_with_invalid_tag_key[too_long_key]
tests.aws.services.kms.test_kms.TestKMS ‑ test_create_key_with_invalid_tag_key[uppercase_prefix]
tests.aws.services.kms.test_kms.TestKMS ‑ test_create_key_with_too_many_tags_raises_error
tests.aws.services.kms.test_kms.TestKMS ‑ test_key_with_long_tag_value_raises_error
tests.aws.services.kms.test_kms.TestKMS ‑ test_tag_existing_key_with_invalid_tag_key
tests.aws.services.kms.test_kms.TestKMS ‑ test_tag_key_with_duplicate_tag_keys_raises_error
tests.aws.services.s3.test_s3.TestS3 ‑ test_s3_get_object_checksum[CRC64NVME]
This pull request removes 306 skipped tests and adds 1 skipped test. Note that renamed tests count towards both. 
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input4-FAILED]
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_deployed_infra_state
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_populate_data
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_user_clicks_are_stored
tests.aws.services.apigateway.test_apigateway_api.TestApiGatewayApiRestApi ‑ test_get_api_case_insensitive
tests.aws.services.apigateway.test_apigateway_api.TestApigatewayIntegration ‑ test_put_integration_request_parameter_bool_type
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_api_gateway_authorizer_crud
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_api_gateway_http_integration_with_path_request_parameter
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_api_gateway_lambda_proxy_integration[/lambda/foo1]
…

tests.aws.services.s3.test_s3.TestS3 ‑ test_s3_get_object_checksum[CRC64NVME]

♻️ This comment has been updated with latest results.

@k-a-il k-a-il self-assigned this Jan 20, 2025
@k-a-il k-a-il added the semver: patch Non-breaking changes which can be included in patch releases label Jan 20, 2025
@k-a-il k-a-il force-pushed the kms-tags-add-validation branch from 45fee2b to 2679fa5 Compare January 21, 2025 08:47
@k-a-il k-a-il requested a review from bentsku January 21, 2025 17:41
@k-a-il k-a-il force-pushed the kms-tags-add-validation branch from 0578756 to 81b5a0c Compare January 21, 2025 19:15
@k-a-il k-a-il force-pushed the kms-tags-add-validation branch from 81b5a0c to 37b6252 Compare January 21, 2025 19:23
@k-a-il k-a-il marked this pull request as ready for review January 21, 2025 19:55
@k-a-il k-a-il requested a review from sannya-singal as a code owner January 21, 2025 19:55
bentsku
bentsku approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! Very nice tests, and very nice validation that goes with it. This is really nice work!

I can clearly see and follow the logic from the tests to the code. 💯

I only 2 minor nits: one is related to using Python built-in to simplify one loop, and one about the parametrization of tests and the name of the test in the snapshot.

Nothing is blocking, but could be nice to have 😄

k-a-il and others added 3 commits January 22, 2025 14:21
@k-a-il @bentsku
Update tests/aws/services/kms/test_kms.py
6cf6a9f 
Co-authored-by: Ben Simon Hartung <42031100+bentsku@users.noreply.github.com>
@k-a-il @bentsku
Update localstack-core/localstack/services/kms/models.py
2fd4256 
Co-authored-by: Ben Simon Hartung <42031100+bentsku@users.noreply.github.com>
@k-a-il
Updated snapshots
62e0a58
@k-a-il k-a-il added this to the 4.1 milestone Jan 22, 2025
@k-a-il k-a-il merged commit 65647fc into master Jan 22, 2025
31 checks passed
@k-a-il k-a-il deleted the kms-tags-add-validation branch January 22, 2025 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentsku bentsku bentsku approved these changes

@sannya-singal sannya-singal Awaiting requested review from sannya-singal

Assignees

@k-a-il k-a-il

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
2 participants
@k-a-il @bentsku