SQS: Improve update support for CloudFormation handlers. #13477
Conversation
Test Results - Preflight, Unit23 001 tests 21 158 ✅ 6m 34s ⏱️ Results for commit 6de7fa0. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Acceptance7 tests 5 ✅ 3m 3s ⏱️ Results for commit 6de7fa0. ♻️ This comment has been updated with latest results. |
Test Results - Alternative Providers583 tests 325 ✅ 18m 12s ⏱️ Results for commit 6de7fa0. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 34m 38s ⏱️ Results for commit 6de7fa0. ♻️ This comment has been updated with latest results. |
LocalStack Community integration with Pro 2 files 2 suites 1h 57m 59s ⏱️ Results for commit 6de7fa0. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Awesome PR. Thank you for the improvements!!
From the CFn side the PR looks great, but it also include changes to the SQS provider so the service owners should take a look too before merging.
|
|
||
| # Values used when a property is removed from a template and needs to be set to its default. | ||
| # If AWS changes their defaults in the future, our parity tests should break. | ||
| DEFAULT_ATTRIBUTE_VALUES = { |
There was a problem hiding this comment.
Praise: We should add this type of constant in more resource providers 🙌
There was a problem hiding this comment.
I think I agree, but @peter-smith-phd 's comment below "Note: CloudFormation sets this to 256KB on update, but 1MB on create" makes me rethink my position.
Context: in the previous CFn resource provider implementation we had the GenericBaseModel subclasses, which tried to abstract to much from the process, and were difficult to maintain. This default values dictionary is starting to become like this.
As an example, I was about to write something like
perhaps we should add an additional layer of structure to this dictionary, e.g.
DEFAULT_ATTRIBUTE_VALUES: dict[str, str | dict[str, str]] = {
"MaximumMessageSize": {"CREATE": "1048576", "UPDATE": 262144},
}but the type signature alone reminds me of the old way where we would define these deploy templates which was an attempt to make this API declarative, but as you can see by the number of closures in that AWS::IAM::User method, it was not straightforward.
I don't think this approach is as bad since it's only the defaults, but I personally don't like having very dynamic types in my type signatures, which is a code smell for how we have to add conditional logic down the line when checking.
| if k not in properties: | ||
| properties[k] = v | ||
|
|
||
| def _tags_to_remove_or_update( |
There was a problem hiding this comment.
Idea: We should add this to the general utilities for resource providers.
There was a problem hiding this comment.
That's a good idea. Which source file do you recommend I move it to?
There was a problem hiding this comment.
localstack-core/localstack/services/cloudformation/provider_utils.py
| "$..Attributes.ReceiveMessageWaitTimeSeconds", | ||
| ] | ||
| ) | ||
| @markers.snapshot.skip_snapshot_verify(paths=["$..PhysicalResourceId"]) |
| if k in sqs_constants.DELETE_IF_DEFAULT and v == sqs_constants.DELETE_IF_DEFAULT[k]: | ||
| if k in queue.attributes: | ||
| del queue.attributes[k] | ||
| else: | ||
| queue.attributes[k] = v |
There was a problem hiding this comment.
Nit: Have you considered hide the values in the response for the get_attributes operation?
There was a problem hiding this comment.
No, I hadn't thought of that. Is there a benefit to hiding it in the output of get_queue_attributes? I had done it this way because I wasn't sure if there's internal code in the service that might check if it exists.
There was a problem hiding this comment.
@thrau, @gregfurman or @baermat should know what's the best approach here.
There was a problem hiding this comment.
@thrau @gregfurman or @baermat, do any of you want to review this PR. Particularly for the couple of files within localstack-core/localstack/services/sqs that I've updated?
There was a problem hiding this comment.
I think deleting the attributes is the better approach right now, because we do not include KmsMasterKeyId or KmsDataKeyReusePeriodSeconds in our current default values. I don't know if this is simply a discrepancy between the cloudformation and the "regular" logic, or if this is something we should handle at some point. For now, deleting them seems like the strategy with less friction to me.
We probably also want to test this behaviour in our provider tests with something like this.
@markers.aws.validated
def test_set_queue_attributes_default_values(self, sqs_create_queue, snapshot, aws_sqs_client):
attributes = {
"KmsMasterKeyId": "",
"KmsDataKeyReusePeriodSeconds": "300",
}
queue_url = sqs_create_queue()
response = aws_sqs_client.get_queue_attributes(QueueUrl=queue_url, AttributeNames=["All"])
snapshot.match("get-queue-attributes-before-set", response)
aws_sqs_client.set_queue_attributes(QueueUrl=queue_url, Attributes=attributes)
response = aws_sqs_client.get_queue_attributes(QueueUrl=queue_url, AttributeNames=["All"])
snapshot.match("get-queue-attributes-after-set", response)
|
This test is failing now with the old engine. Please fix this or consider using |
That was one of my open questions - what is the recommended approach here? Are we still supporting the legacy engine, or is it acceptable to no longer support backward compatibility? |
|
The test deploys the same SQS CFn template twice but with different QueueName. That should trigger a resource replacement not an update. Your changes make the SQS::Queue resource provider no longer be in charge of reviewing the attribute changes and triggering a replacement. IMO, it's fine since it's more parity in to what the actual update should behave by leaving the replacement handling to the engine. Let's skip the test in the previous engine with cc/ @simonrw |
62afc7e to
2af204c
Compare
There was a problem hiding this comment.
We are good to go from my side once the comments are addressed 🎉
| if request.desired_state.get("FifoQueue"): | ||
| queue_name = f"{queue_name[:-5]}.fifo" |
There was a problem hiding this comment.
To be on the safe side, please double check that we actually get a boolean here, not a string (because False and "False" will react differently here) :)
There was a problem hiding this comment.
Oh good catch! I tried this out and there are several combinations:
- Completely omitting the property gives
None(expected) - Providing
Trueortruegives a boolean True (expected) - Providing
Falseorfalsegives a boolean False (expected) - Providing
FasleorFaLsEgives a string value (NOT expected)
I'll add a == True to make this work, but it does feel like a bug in the CloudFormation engine. It should be rejecting anything that's not a simple True/False value.
In AWS, we see Model validation failed (#/FifoQueue: expected type: Boolean, found: String) when I provide anything other than True or False.
| (model["QueueName"], model["FifoQueue"]) = ( | ||
| prev_model.get("QueueName"), | ||
| prev_model.get("FifoQueue", False), | ||
| ) |
There was a problem hiding this comment.
nit: this is probably a bit easier to read if we use two assignments instead of one tuple unpacking with assignment
| if k not in properties: | ||
| properties[k] = v |
There was a problem hiding this comment.
nit: you can do this with properties.setdefault(k, v) instead
| if k not in properties: | |
| properties[k] = v | |
| properties.setdefault(k, v) |
| # | ||
| # If these attributes are set to their default values, they are effectively | ||
| # deleted from the queue attributes and not returned in future calls to get_queue_attributes() | ||
| # | ||
| DELETE_IF_DEFAULT = {"KmsMasterKeyId": "", "KmsDataKeyReusePeriodSeconds": "300"} |
There was a problem hiding this comment.
extra nitty nit: changing this to a list of keys and then accessing the new DEFAULT_ATTRIBUTE_VALUES avoids duplicating those values. Just in case they ever change the KmsDataKeyReusePeriodSeconds 🙃
There was a problem hiding this comment.
Interesting thought. I had deliberately not done this, because DEFAULT_ATTRIBUTE_VALUE is defined in the resource providers, but DELETE_IF_DEFAULT is defined inside the service provider. I guess there's nothing stopping this from working, but it feels odd to have a service -> resource provider dependency in the code.
(not to mention, this would be impossible in a real AWS resource provider where they execute in completely separate domains).
Thoughts?
There was a problem hiding this comment.
I see the dependency problem, I was more referring to this in a general manner, which could maybe solved by moving this to constants? In any case, it's absolutely non-blocking :)
| if k in sqs_constants.DELETE_IF_DEFAULT and v == sqs_constants.DELETE_IF_DEFAULT[k]: | ||
| if k in queue.attributes: | ||
| del queue.attributes[k] | ||
| else: | ||
| queue.attributes[k] = v |
There was a problem hiding this comment.
I think deleting the attributes is the better approach right now, because we do not include KmsMasterKeyId or KmsDataKeyReusePeriodSeconds in our current default values. I don't know if this is simply a discrepancy between the cloudformation and the "regular" logic, or if this is something we should handle at some point. For now, deleting them seems like the strategy with less friction to me.
We probably also want to test this behaviour in our provider tests with something like this.
@markers.aws.validated
def test_set_queue_attributes_default_values(self, sqs_create_queue, snapshot, aws_sqs_client):
attributes = {
"KmsMasterKeyId": "",
"KmsDataKeyReusePeriodSeconds": "300",
}
queue_url = sqs_create_queue()
response = aws_sqs_client.get_queue_attributes(QueueUrl=queue_url, AttributeNames=["All"])
snapshot.match("get-queue-attributes-before-set", response)
aws_sqs_client.set_queue_attributes(QueueUrl=queue_url, Attributes=attributes)
response = aws_sqs_client.get_queue_attributes(QueueUrl=queue_url, AttributeNames=["All"])
snapshot.match("get-queue-attributes-after-set", response)
955ad81 to
e6f1abf
Compare
e6f1abf to
6de7fa0
Compare
There was a problem hiding this comment.
Awesome stuff, thank you for greatly expanding on the test coverage for this resource provider!
I am a bit concerned about the number of additional templates this PR has added. Perhaps with a little CFn engine magic we could trim that down a bit (e.g. using properties/conditions etc.)
I also think that testing these resource providers should get much easier once we have CloudControl implemented. Then we wouldn't need to maintain these templates 👍
|
|
||
| # Values used when a property is removed from a template and needs to be set to its default. | ||
| # If AWS changes their defaults in the future, our parity tests should break. | ||
| DEFAULT_ATTRIBUTE_VALUES = { |
There was a problem hiding this comment.
I think I agree, but @peter-smith-phd 's comment below "Note: CloudFormation sets this to 256KB on update, but 1MB on create" makes me rethink my position.
Context: in the previous CFn resource provider implementation we had the GenericBaseModel subclasses, which tried to abstract to much from the process, and were difficult to maintain. This default values dictionary is starting to become like this.
As an example, I was about to write something like
perhaps we should add an additional layer of structure to this dictionary, e.g.
DEFAULT_ATTRIBUTE_VALUES: dict[str, str | dict[str, str]] = {
"MaximumMessageSize": {"CREATE": "1048576", "UPDATE": 262144},
}but the type signature alone reminds me of the old way where we would define these deploy templates which was an attempt to make this API declarative, but as you can see by the number of closures in that AWS::IAM::User method, it was not straightforward.
I don't think this approach is as bad since it's only the defaults, but I personally don't like having very dynamic types in my type signatures, which is a code smell for how we have to add conditional logic down the line when checking.
| # Private method for creating a unique queue name, if none is specified. | ||
| def _autogenerated_queue_name(self, request: ResourceRequest[SQSQueueProperties]) -> str: |
There was a problem hiding this comment.
nit(minor): I tend to prefer methods that perform work be named as such
| # Private method for creating a unique queue name, if none is specified. | |
| def _autogenerated_queue_name(self, request: ResourceRequest[SQSQueueProperties]) -> str: | |
| # Private method for creating a unique queue name, if none is specified. | |
| def _generate_queue_name(self, request: ResourceRequest[SQSQueueProperties]) -> str: |
| ) | ||
|
|
||
| @markers.aws.validated | ||
| @skip_if_legacy_engine() |
There was a problem hiding this comment.
🎉 it's great to see more skips for the old engine meaning that the new engine is a higher parity!
| ) | ||
| assert converted_dict == target_dict | ||
|
|
||
| def test_resource_tags_to_remove_or_update(self): |
There was a problem hiding this comment.
praise: thank you for adding this test for your new helper function!
| def deploy_stack(deploy_cfn_template, template_filename, **kwargs): | ||
| """ | ||
| Helper function to deploy a CloudFormation stack using a template file. This exists to reduce | ||
| boilerplate in the test cases. | ||
| """ | ||
| template_path = os.path.join(os.path.dirname(__file__), "templates", template_filename) | ||
| return deploy_cfn_template(template_path=template_path, **kwargs) |
There was a problem hiding this comment.
suggestion: perhaps make this a fixture making it an even easier API in the test
| def deploy_stack(deploy_cfn_template, template_filename, **kwargs): | |
| """ | |
| Helper function to deploy a CloudFormation stack using a template file. This exists to reduce | |
| boilerplate in the test cases. | |
| """ | |
| template_path = os.path.join(os.path.dirname(__file__), "templates", template_filename) | |
| return deploy_cfn_template(template_path=template_path, **kwargs) | |
| @pytest.fixture | |
| def deploy_stack(deploy_cfn_template): | |
| def inner(template_filename: str, **kwargs): | |
| """ | |
| Helper function to deploy a CloudFormation stack using a template file. This exists to reduce | |
| boilerplate in the test cases. | |
| """ | |
| template_path = os.path.join(os.path.dirname(__file__), "templates", template_filename) | |
| return deploy_cfn_template(template_path=template_path, **kwargs) | |
| return inner |
That way the test only needs to include
def test_foo(deploy_stack):
stack = deploy_stack("template_name.yaml")Alternatively, we could change deploy_cfn_template to walk up the directory structure looking for the template matching the file stub. E.g. in this case
# in tests/a/b/c/test_d.py
# with tests/a/templates/foo.yaml
deploy_cfn_template("foo.yaml")
# - does tests/a/b/c/templates/foo.yaml exist? no
# - does tests/a/b/templates/foo.yaml exist? no
# - does tests/a/templates/foo.yaml exist? yes => deploy this template
| @@ -0,0 +1,334 @@ | |||
| { | |||
| "tests/aws/services/sqs/resource_providers/test_aws_sqs_queue.py::test_create_standard_queue_with_required_properties": { | |||
| "recorded-date": "07-12-2025, 19:18:58", | |||
There was a problem hiding this comment.
praise: thank you for revalidating!
Motivation
Improve support for CloudFormation updates using the
AWS::SQS::Queueresource provider. The current resource providers do not execute updates correctly.Changes
updatehandler forAWS::SQS::QueueAWS::SQS::Queuemoved into thetests/aws/services/sqs/resource_providersdirectory to associated them more closely with the core SQS tests.set_queue_attributesto effectively remove attributes that are set to their default values.AWS::SQS::QueuePolicyinto a separate test file (the TODOs will be implemented in a later PR)Tests
Extensive parity tests added for the
AWS::SQS::Queuescenarios.Related