Nothing Special   »   [go: up one dir, main page]

Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ecukalla
Copy link

Motivation

Implement priority and action filtering for the ListFirewallRules operation in Route 53 Resolver

Changes

Added functionality to filter by action and priority 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:

  • test listing firewall rules for a non-existing rule-group.
  • test listing firewall rules for empty rule-group
  • test listing all rules in the rule-group
  • test listing rules filtered by priority
  • test listing rules filtered by action
  • test listing rules filtered by priority and action

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.
@localstack-bot
Copy link
Collaborator
localstack-bot commented Oct 24, 2024

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

Copy link
Collaborator
@localstack-bot localstack-bot left a 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.

@ecukalla
Copy link
Author

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

localstack-bot added a commit that referenced this pull request Oct 24, 2024
@alexrashed alexrashed added the semver: patch Non-breaking changes which can be included in patch releases label Oct 24, 2024
Copy link
Contributor
@sannya-singal sannya-singal left a 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! 🚀

tests/aws/services/route53resolver/test_route53resolver.py Outdated Show resolved Hide resolved
tests/aws/services/route53resolver/test_route53resolver.py Outdated Show resolved Hide resolved
Comment on lines 848 to 861
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"],
)
)
Copy link
Contributor

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 🙂)

Copy link
Author

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!

tests/aws/services/route53resolver/test_route53resolver.py Outdated Show resolved Hide resolved
tests/aws/services/route53resolver/test_route53resolver.py Outdated Show resolved Hide resolved
Copy link
Contributor
@simonrw simonrw left a 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 👏 🎉

Comment on lines 744 to 747
match = re.search("Trace Id: [\"'](.+)[\"']", error_msg)
if match:
trace_id = match.groups()[0]
snapshot.add_transformer(snapshot.transform.regex(trace_id, "<trace-id>"))
Copy link
Contributor

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:

Suggested change
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>"))

Copy link
Author

Choose a reason for hiding this comment

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

Done.

tests/aws/services/route53resolver/test_route53resolver.py Outdated Show resolved Hide resolved
Comment on lines 774 to 775
@markers.snapshot.skip_snapshot_verify(paths=["$..ManagedOwnerName"])
@markers.snapshot.skip_snapshot_verify(paths=["$..FirewallDomainRedirectionAction"])
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
@markers.snapshot.skip_snapshot_verify(paths=["$..ManagedOwnerName"])
@markers.snapshot.skip_snapshot_verify(paths=["$..FirewallDomainRedirectionAction"])
@markers.snapshot.skip_snapshot_verify(
paths=[
"$..ManagedOwnerName",
"$..FirewallDomainRedirectionAction",
],
)

Comment on lines 812 to 817
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"]
)
)
Copy link
Contributor

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 cleanups 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... 😂

Suggested change
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)

Copy link
Author

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):
Copy link
Contributor

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.

Copy link
Author

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(
Copy link
Contributor

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(...)

Comment on lines +89 to +90
@pytest.fixture(scope="function")
def route53resolver_create_firewall_rules(self, aws_client, cleanups):
Copy link
Contributor

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

Copy link
Contributor
@sannya-singal sannya-singal left a 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 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants