-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Feature: filtering for ListFirewallRules in Route53Resolver #11742
base: master
Are you sure you want to change the base?
Feature: filtering for ListFirewallRules in Route53Resolver #11742
Conversation
…ration in Route 53 Resolver.
Listing a firewall-rule-group for which no rules were created resulted in a ResourceNotFoundException. This was because the store object for firewall-rules was only created when the 1st rule was added. This change lifts creating a store space for firewall-rules at the firewall-rule-group creation.
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this cool contribution @ecukalla! 🎉
This is a great addition for enhancing query flexibility, improving the robustness of the firewall rules and groups functionality in Localstack. Also kudos for adding some great use cases for the snapshot tests 🙌 I have added few improvements that could enhance readability and maintain consistency with existing code standards. I’ve also added some comments and nitpicks for some refinements. Once these are resolved, we should be good to go! 🚀
localstack-core/localstack/services/route53resolver/provider.py
Outdated
Show resolved
Hide resolved
localstack-core/localstack/services/route53resolver/provider.py
Outdated
Show resolved
Hide resolved
localstack-core/localstack/services/route53resolver/provider.py
Outdated
Show resolved
Hide resolved
localstack-core/localstack/services/route53resolver/provider.py
Outdated
Show resolved
Hide resolved
localstack-core/localstack/services/route53resolver/provider.py
Outdated
Show resolved
Hide resolved
rule_response_1 = aws_client.route53resolver.create_firewall_rule( | ||
FirewallRuleGroupId=rule_group_response["FirewallRuleGroup"]["Id"], | ||
FirewallDomainListId=domain_list_response_1["FirewallDomainList"]["Id"], | ||
Priority=1, | ||
Action=Action.ALLOW, | ||
Name=f"rule-name-{short_uid()}", | ||
) | ||
snapshot.match("create-firewall-rule-1", rule_response_1) | ||
cleanups.append( | ||
lambda: aws_client.route53resolver.delete_firewall_rule( | ||
FirewallRuleGroupId=rule_group_response["FirewallRuleGroup"]["Id"], | ||
FirewallDomainListId=domain_list_response_1["FirewallDomainList"]["Id"], | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this piece of code gets duplicated a few times, I would suggest to create a separate utility fixture to create the firewall rule and pass the arguments for Priority
and Action
which would additionally also cleanup the resources.
Let me know if you require more inputs here, happy to help (you can find several references for this in the core repository itself 🙂)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a new fixture for this. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome contribution, thanks for submitting this. I will leave it to @sannya-singal to have the final say, but 👏 🎉
localstack-core/localstack/services/route53resolver/provider.py
Outdated
Show resolved
Hide resolved
match = re.search("Trace Id: [\"'](.+)[\"']", error_msg) | ||
if match: | ||
trace_id = match.groups()[0] | ||
snapshot.add_transformer(snapshot.transform.regex(trace_id, "<trace-id>")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can this regex be used with the transformer directly?
Something like:
match = re.search("Trace Id: [\"'](.+)[\"']", error_msg) | |
if match: | |
trace_id = match.groups()[0] | |
snapshot.add_transformer(snapshot.transform.regex(trace_id, "<trace-id>")) | |
snapshot.add_transformer(snapshot.transform.regex(r"[\"'](.+)[\"']", "<trace-id>")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@markers.snapshot.skip_snapshot_verify(paths=["$..ManagedOwnerName"]) | ||
@markers.snapshot.skip_snapshot_verify(paths=["$..FirewallDomainRedirectionAction"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
@markers.snapshot.skip_snapshot_verify(paths=["$..ManagedOwnerName"]) | |
@markers.snapshot.skip_snapshot_verify(paths=["$..FirewallDomainRedirectionAction"]) | |
@markers.snapshot.skip_snapshot_verify( | |
paths=[ | |
"$..ManagedOwnerName", | |
"$..FirewallDomainRedirectionAction", | |
], | |
) |
snapshot.match("create-firewall-domain-list-1", domain_list_response_1) | ||
cleanups.append( | ||
lambda: aws_client.route53resolver.delete_firewall_domain_list( | ||
FirewallDomainListId=domain_list_response_1["FirewallDomainList"]["Id"] | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: As suggested above, the cleanup
s should be as close to the thing they are cleaning up as possible. In this case, not a lot will go wrong with the snapshot.match
function, however... 😂
snapshot.match("create-firewall-domain-list-1", domain_list_response_1) | |
cleanups.append( | |
lambda: aws_client.route53resolver.delete_firewall_domain_list( | |
FirewallDomainListId=domain_list_response_1["FirewallDomainList"]["Id"] | |
) | |
) | |
cleanups.append( | |
lambda: aws_client.route53resolver.delete_firewall_domain_list( | |
FirewallDomainListId=domain_list_response_1["FirewallDomainList"]["Id"] | |
) | |
) | |
snapshot.match("create-firewall-domain-list-1", domain_list_response_1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Done.
@markers.aws.validated | ||
@markers.snapshot.skip_snapshot_verify(paths=["$..ManagedOwnerName"]) | ||
@markers.snapshot.skip_snapshot_verify(paths=["$..FirewallDomainRedirectionAction"]) | ||
def test_list_firewall_rules(self, cleanups, snapshot, aws_client): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test executes a combination of two variables: Priority
and Action
. Since there are a small number of reasonable combinations, I don't think it's too hard to get coverage over all of the combinations
Priority | Action |
---|---|
0 | None |
1 | None |
0 | ALLOW |
1 | ALLOW |
0 | BLOCK |
1 | BLOCK |
0 | ALERT |
1 | ALERT |
Is it worth adding a loop to cover this table? It might make the test a bit shorter as well, with less repetition. You should also then test not including one or both of the priority and action.
This is the beauty of snapshot tests: asserting that the result is correct is easy, compared to asserting this behaviour by hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to keep the tests explicit (the Tests should not be clever
principle).
FirewallDomainListId=domain_list_id | ||
) | ||
) | ||
_ = aws_client.route53resolver.create_firewall_rule( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since we do not collect any response here, we can directly remove the left hand side and write aws_client.route53resolver.create_firewall_rule(...)
@pytest.fixture(scope="function") | ||
def route53resolver_create_firewall_rules(self, aws_client, cleanups): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just have a fixture for create_firewall_rule
and cleanups since it was duplicated a lot of times and call it from the test itself for different params. This will be helpful in re-using this fixture even outside of this test 🙂
Also we can remove adding a scope as it falls to this default value 😉 and just a nit: we can remove the route53resolver
prefix in the name of the fixture.
def create_firewall_rule(self, aws_client, ..):
# create logic
# yield the response
# cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the feedback 🙌 🚀 I have just added a nit and a comment for the suggested fixture. Please let me know if you need some more inputs 🙂
Motivation
Implement priority and action filtering for the
ListFirewallRules
operation in Route 53 ResolverChanges
Added functionality to filter by
action
andpriority
for the Route53Resolver operation ListFirewallRules.Additionally fixed a bug during which: listing a firewall-rule-group for which no rules were created resulted in a ResourceNotFoundException. This was because the store object for firewall-rules was only created when the 1st rule was added.
This change lifts creating a store space for firewall-rules at the firewall-rule-group creation.
Parity tests were included for: