Make FirewallPolicyARN updatable #25

kunhwiko · 2024-04-05T22:46:52Z

Description of changes
The FirewallPolicyARN on the Firewall resource can be updatable through the AssociateFirewallPolicy API. This is something we're doing with our own local copy of this repo.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-prow · 2024-04-05T22:47:03Z

Hi @kunhwiko. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kunhwiko · 2024-04-05T22:47:22Z

apis/v1alpha1/ack-generate-metadata.yaml

  build_hash: e8df4d5a4b86dea0e227786c2c3d213e5aeda97a
-  go_version: go1.22.0
-  version: v0.33.0
+  go_version: go1.21.0


umm...? something I missed :D?

Nothing critical (for code-gen), we just started using go 1.22.x :)

kunhwiko · 2024-04-05T22:50:01Z

pkg/resource/firewall/hooks.go

+	b *resource,
+) {
+	// Sort subnet mappings such that they can be compared in a deterministic way.
+	customPreCompareSubnetMappings(a, b)


This change doesn't actually have to do anything with making the FirewallPolicyARN updatable. I've noticed the same set of subnet mappings might be detected as "different" during updates because they're arranged in a different order.

Makes sense! It appears that the AWS Firewall API sorts the resources behind the scenes. Ideally, we should avoid mutating the desired state here to maintain consistency. The common runtime patches back the Spec returned by an sdkUpdate, potentially sorting the SubnetMapping in the desired Spec.

Maybe we could create a new function called equalSubnetMapping and use that to detect if something change, regardless of the order?

related: aws-controllers-k8s/community#2053

@a-hilaly Ahh makes sense! If the spec gets patched by sdkUpdate, that might end up in an OutOfSync issue for folks using GitOps.

Hmm problem though is if we don't sort here, the controller ends up picking up a diff later on in the delta function:

https://github.com/aws-controllers-k8s/networkfirewall-controller/blob/main/pkg/resource/firewall/delta.go#L109

The above is code-gened, so I think there's little control over what we can do there...

What we need to do is to figure out whether the SubnetMapping changed or not without sorting and usinf reflect.DeepEqual. In general we can do:

Instruct the code-gen delta generator to ignore SubnetMapping using something like: https://github.com/aws-controllers-k8s/dynamodb-controller/blob/main/generator.yaml#L53-L54

Implement a equalSubnetMappings using something similar to: https://github.com/aws-controllers-k8s/dynamodb-controller/blob/main/pkg/resource/table/hooks_tags.go#L108-L135

Call equalSubnetMappings in the customPreCompare to decide whether we want to register a delta for Spec.SubnetMappings

oh sweet, was not aware of compare.is_ignored. that'll work, thanks!

kunhwiko · 2024-04-05T22:53:10Z

test/e2e/tests/test_firewall.py

-        pass
+    assert cr["status"]["firewall"]["firewallPolicyARN"] == policy_arn

-@pytest.fixture


I needed to use this function twice in simple_firewall...so had to stop using this as a fixture

a-hilaly · 2024-04-08T13:50:37Z

/ok-to-test

a-hilaly

Thanks a lot @kunhwiko ! Left a comment in-line

a-hilaly · 2024-04-08T13:51:43Z

apis/v1alpha1/ack-generate-metadata.yaml

  build_hash: e8df4d5a4b86dea0e227786c2c3d213e5aeda97a
-  go_version: go1.22.0
-  version: v0.33.0
+  go_version: go1.21.0


Nothing critical (for code-gen), we just started using go 1.22.x :)

a-hilaly · 2024-04-08T14:33:17Z

pkg/resource/firewall/hooks.go

+	b *resource,
+) {
+	// Sort subnet mappings such that they can be compared in a deterministic way.
+	customPreCompareSubnetMappings(a, b)


Makes sense! It appears that the AWS Firewall API sorts the resources behind the scenes. Ideally, we should avoid mutating the desired state here to maintain consistency. The common runtime patches back the Spec returned by an sdkUpdate, potentially sorting the SubnetMapping in the desired Spec.

Maybe we could create a new function called equalSubnetMapping and use that to detect if something change, regardless of the order?

a-hilaly

Nice, thanks a lot @kunhwiko !
/lgtm

ack-prow · 2024-04-22T13:34:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, kunhwiko

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [a-hilaly]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

a-hilaly · 2024-04-22T13:34:19Z

/test all

Make FirewallPolicyARN updatable

bebe82a

ack-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 5, 2024

ack-prow bot requested a review from a-hilaly April 5, 2024 22:46

ack-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 5, 2024

kunhwiko commented Apr 5, 2024

View reviewed changes

Cleanup firewall policies after E2E execution

107afa4

kunhwiko marked this pull request as ready for review April 6, 2024 00:34

ack-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2024

ack-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 8, 2024

a-hilaly reviewed Apr 8, 2024

View reviewed changes

Modify delta logic for subnet mappings

ad3edb6

a-hilaly approved these changes Apr 22, 2024

View reviewed changes

ack-prow bot assigned a-hilaly Apr 22, 2024

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2024

ack-prow bot added the approved label Apr 22, 2024

ack-prow bot merged commit 527f748 into aws-controllers-k8s:main Apr 22, 2024

Make FirewallPolicyARN updatable #25

Make FirewallPolicyARN updatable #25

Uh oh!

Conversation

kunhwiko commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-prow bot commented Apr 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunhwiko Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunhwiko Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Apr 8, 2024

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

ack-prow bot commented Apr 22, 2024

Uh oh!

a-hilaly commented Apr 22, 2024

Uh oh!

Uh oh!

kunhwiko commented Apr 5, 2024 •

edited

Loading

kunhwiko Apr 5, 2024 •

edited

Loading

kunhwiko Apr 17, 2024 •

edited

Loading