Fix NetworkACL default rules sync (#247)

Filter out AWS default rules (rule #32767) from both desired and actual states during synchronization
Implement clear and documented behavior for default rules:
1. If default rules are explicitly defined in spec -> they remain in spec
2. If default rules are not defined in spec -> they are ignored during sync but preserved in AWS

Author: rushmash91

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,5 +1,5 @@
 ack_generate_info:
-  build_date: "2025-02-20T17:55:49Z"
+  build_date: "2025-02-26T00:29:35Z"
   build_hash: a326346bd3a6973254d247c9ab2dc76790c36241
   go_version: go1.24.0
   version: v0.43.2

pkg/resource/network_acl/hooks.go

@@ -303,6 +303,14 @@ func (rm *resourceManager) upsertNewAssociations(
 	return nil
 }
 
+// The function filters out AWS-managed default rules (rule #32767) from both desired
+// and latest states to prevent interference with AWS's automatic management of these
+// rules and to maintain GitOps compatibility.
+//
+// Default rules behavior:
+//   - If default rules (rule #32767) are explicitly defined in the spec, they will remain in the spec
+//   - If default rules are not defined in the spec, they will be ignored during sync
+//     operations and not be added to the spec
 func (rm *resourceManager) syncEntries(
 	ctx context.Context,
 	desired *resource,
@@ -316,6 +324,19 @@ func (rm *resourceManager) syncEntries(
 	toDelete := []*svcapitypes.NetworkACLEntry{}
 	toUpdate := []*svcapitypes.NetworkACLEntry{}
 
+	// Filter out AWS default rules (rule #32767) from desired entries to ensure
+	// they don't interfere with GitOps workflows. These rules are automatically
+	// managed by AWS and should not be included in the desired state.
+	filteredDesiredEntries := []*svcapitypes.NetworkACLEntry{}
+	for _, entry := range desired.ko.Spec.Entries {
+		if entry.RuleNumber != nil && *entry.RuleNumber == int64(DefaultRuleNumber) {
+			continue
+		}
+		filteredDesiredEntries = append(filteredDesiredEntries, entry)
+	}
+	desired.ko.Spec.Entries = filteredDesiredEntries
+
+	// Check for duplicate rule numbers within the same direction (egress/ingress)
 	uniqEntries := lo.UniqBy(desired.ko.Spec.Entries, func(entry *svcapitypes.NetworkACLEntry) string {
 		return strconv.FormatBool(*entry.Egress) + strconv.Itoa(int(*entry.RuleNumber))
 	})
@@ -324,25 +345,26 @@ func (rm *resourceManager) syncEntries(
 		return errors.New("multple rules with the same rule number and Egress in the desired spec")
 	}
 
+	// Identify new entries that need to be created
 	for _, desiredEntry := range desired.ko.Spec.Entries {
-
-		if *((*desiredEntry).RuleNumber) == int64(DefaultRuleNumber) {
-			// no-op for default route
-			continue
-		}
-
 		if latest != nil && !containsEntry(latest.ko.Spec.Entries, desiredEntry) {
-			// a desired rule is not in the latest resource; therefore, create
 			toAdd = append(toAdd, desiredEntry)
 		}
 	}
 
 	if latest != nil {
-		for _, latestEntry := range latest.ko.Spec.Entries {
-			if *((*latestEntry).RuleNumber) == int64(DefaultRuleNumber) {
-				// no-op for default route
+		// Filter out AWS default rules from latest entries before comparison
+		// to ensure consistent state management between desired and actual
+		filteredLatestEntries := []*svcapitypes.NetworkACLEntry{}
+		for _, entry := range latest.ko.Spec.Entries {
+			if entry.RuleNumber != nil && *entry.RuleNumber == int64(DefaultRuleNumber) {
 				continue
 			}
+			filteredLatestEntries = append(filteredLatestEntries, entry)
+		}
+
+		// Identify entries that need to be deleted (exist in latest but not in desired)
+		for _, latestEntry := range filteredLatestEntries {
 			if !containsEntry(desired.ko.Spec.Entries, latestEntry) {
 				// entry is in latest resource, but not in desired resource; therefore, delete
 				toDelete = append(toDelete, latestEntry)

pkg/resource/network_acl/sdk.go

@@ -12,9 +12,9 @@
 
 	if len(desired.ko.Spec.Entries) > 0 {
 		//desired rules are overwritten by NetworkACL's default rules
-		ko.Spec.Entries = append(ko.Spec.Entries, desired.ko.Spec.Entries...)
+		ko.Spec.Entries = desired.ko.Spec.Entries
 		copy := ko.DeepCopy()
 		if err := rm.createEntries(ctx, &resource{copy}); err != nil {
-			rlog.Debug("Error while syncing routes", err)
+			rlog.Debug("Error while syncing entries", err)
 		}
 	}

templates/hooks/network_acl/sdk_create_post_set_output.go.tpl

@@ -0,0 +1,28 @@
+apiVersion: ec2.services.k8s.aws/v1alpha1
+kind: NetworkACL
+metadata:
+  name: $NETWORK_ACL_NAME
+spec:
+  entries:
+    # Default egress rule
+    - cidrBlock: 0.0.0.0/0
+      egress: true
+      protocol: "-1"
+      ruleAction: deny
+      ruleNumber: 32767
+    # Default ingress rule
+    - cidrBlock: 0.0.0.0/0
+      egress: false
+      protocol: "-1"
+      ruleAction: deny
+      ruleNumber: 32767
+    # Custom rule
+    - cidrBlock: $CIDR_BLOCK
+      egress: true
+      portRange:
+        from: 443
+        to: 443
+      protocol: "6"
+      ruleAction: allow
+      ruleNumber: 100
+  vpcID: $VPC_ID

test/e2e/resources/network_acl_with_default_rules.yaml

@@ -93,6 +93,55 @@ def simple_network_acl(request):
     except:
         pass
 
+@pytest.fixture
+def network_acl_with_default_rules(request):
+    resource_name = random_suffix_name("network-acl-default-rules", 32)
+    resource_file = "network_acl_with_default_rules"
+    resources = get_bootstrap_resources()
+
+    replacements = REPLACEMENT_VALUES.copy()
+    replacements["NETWORK_ACL_NAME"] = resource_name
+    replacements["VPC_ID"] = resources.SharedTestVPC.vpc_id
+    replacements["CIDR_BLOCK"] = "10.0.0.0/24"
+
+    marker = request.node.get_closest_marker("resource_data")
+    if marker is not None:
+        data = marker.args[0]
+        if 'vpc_id' in data:
+            replacements["VPC_ID"] = data['vpc_id']
+        if 'cidr_block' in data:
+            replacements["CIDR_BLOCK"] = data['cidr_block']
+        if 'resource_file' in data:
+            resource_file = data['resource_file']
+
+    # Load NetworkACL CR with default rules
+    resource_data = load_ec2_resource(
+        resource_file,
+        additional_replacements=replacements,
+    )
+    logging.debug(resource_data)
+
+    # Create k8s resource
+    ref = k8s.CustomResourceReference(
+        CRD_GROUP, CRD_VERSION, RESOURCE_PLURAL,
+        resource_name, namespace="default",
+    )
+    k8s.create_custom_resource(ref, resource_data)
+    time.sleep(CREATE_WAIT_AFTER_SECONDS)
+
+    cr = k8s.wait_resource_consumed_by_controller(ref)
+    assert cr is not None
+    assert k8s.get_resource_exists(ref)
+
+    yield (ref, cr)
+
+    # Try to delete, if doesn't already exist
+    try:
+        _, deleted = k8s.delete_custom_resource(ref, 3, 10)
+        assert deleted
+    except:
+        pass
+
 @service_marker
 @pytest.mark.canary
 class TestNetworkACLs:
@@ -324,3 +373,77 @@ def test_crud_tags(self, ec2_client, simple_network_acl):
 
         # Check networkAcl no longer exists in AWS
         ec2_validator.assert_network_acl(resource_id, exists=False)
+
+    def test_default_rules_not_duplicated_on_create(self, ec2_client, network_acl_with_default_rules):
+        (ref, cr) = network_acl_with_default_rules
+        network_acl_id = cr["status"]["id"]
+
+        # Check NetworkACL exists in AWS
+        ec2_validator = EC2Validator(ec2_client)
+        ec2_validator.assert_network_acl(network_acl_id)
+
+        resource = k8s.get_resource(ref)
+
+        # Count how many rules with number 32767 exist in the spec
+        default_rule_count = 0
+        for entry in resource["spec"]["entries"]:
+            if entry.get("ruleNumber") == 32767:
+                default_rule_count += 1
+
+        # default rules are no op
+        assert default_rule_count == 2, "Default rules should not be added to spec when not explicitly defined"
+
+        # Verify custom rule
+        custom_rule_exists = False
+        for entry in resource["spec"]["entries"]:
+            if entry.get("ruleNumber") == 100:
+                custom_rule_exists = True
+                break
+
+        assert custom_rule_exists, "Custom rule with ruleNumber 100 not found in spec"
+
+        # Clean up
+        _, deleted = k8s.delete_custom_resource(ref)
+        assert deleted is True
+
+        time.sleep(DELETE_WAIT_AFTER_SECONDS)
+
+        # Verify the NetworkACL was deleted
+        ec2_validator.assert_network_acl(network_acl_id, exists=False)
+
+    def test_default_rules_not_added_to_spec(self, ec2_client, simple_network_acl):
+        (ref, cr) = simple_network_acl
+        network_acl_id = cr["status"]["id"]
+
+        # Check NetworkACL exists in AWS
+        ec2_validator = EC2Validator(ec2_client)
+        ec2_validator.assert_network_acl(network_acl_id)
+
+        resource = k8s.get_resource(ref)
+
+        # Count how many rules with number 32767 exist in the spec
+        default_rule_count = 0
+        for entry in resource["spec"]["entries"]:
+            if entry.get("ruleNumber") == 32767:
+                default_rule_count += 1
+
+        # Verify no default rules were added to spec
+        assert default_rule_count == 0, "Default rules should not be added to spec when not explicitly defined"
+
+        # Verify custom rule exists
+        custom_rule_exists = False
+        for entry in resource["spec"]["entries"]:
+            if entry.get("ruleNumber") == 100:
+                custom_rule_exists = True
+                break
+
+        assert custom_rule_exists, "Custom rule with ruleNumber 100 not found in spec"
+
+        # Clean up
+        _, deleted = k8s.delete_custom_resource(ref)
+        assert deleted is True
+
+        time.sleep(DELETE_WAIT_AFTER_SECONDS)
+
+        # Verify the NetworkACL was deleted
+        ec2_validator.assert_network_acl(network_acl_id, exists=False)