From ae7255bb181a95b67b88aa0f15d0121cd50263cf Mon Sep 17 00:00:00 2001
From: Carl Hiltbrunner <carlhilt@amazon.com>
Date: Tue, 16 Sep 2025 00:41:08 -0700
Subject: [PATCH] Only combine Rules Endpoints with the same CIDR if they share
 identical Exception lists.

---
 pkg/policyendpoints/manager.go      | 45 ++++++++++++++++++++++-------
 pkg/policyendpoints/manager_test.go | 33 +++++++++++++++++++++
 2 files changed, 68 insertions(+), 10 deletions(-)

diff --git a/pkg/policyendpoints/manager.go b/pkg/policyendpoints/manager.go
index 9611f2e..e269b3d 100644
--- a/pkg/policyendpoints/manager.go
+++ b/pkg/policyendpoints/manager.go
@@ -180,23 +180,48 @@ func (m *policyEndpointsManager) processPolicyEndpoints(pes []policyinfo.PolicyE
 	return newPEs
 }
 
-// the controller should consolidate the ingress and egress endpoints and put entries to one CIDR if they belong to a same CIDR
+// the controller should consolidate the ingress and egress endpoints
 func combineRulesEndpoints(ingressEndpoints []policyinfo.EndpointInfo) []policyinfo.EndpointInfo {
+	var result []policyinfo.EndpointInfo
 	combinedMap := make(map[string]policyinfo.EndpointInfo)
+
 	for _, iep := range ingressEndpoints {
-		if _, ok := combinedMap[string(iep.CIDR)]; ok {
-			tempIEP := combinedMap[string(iep.CIDR)]
-			tempIEP.Ports = append(combinedMap[string(iep.CIDR)].Ports, iep.Ports...)
-			tempIEP.Except = append(combinedMap[string(iep.CIDR)].Except, iep.Except...)
-			combinedMap[string(iep.CIDR)] = tempIEP
+		cidrKey := string(iep.CIDR)
+		if existing, ok := combinedMap[cidrKey]; ok {
+			// Only combine if exception lists are identical to preserve rule intent
+			if exceptionsEqual(existing.Except, iep.Except) {
+				existing.Ports = append(existing.Ports, iep.Ports...)
+				combinedMap[cidrKey] = existing
+			} else {
+				// Different exceptions - keep both separate to preserve intent
+				result = append(result, existing)
+				delete(combinedMap, cidrKey)
+				result = append(result, iep)
+			}
 		} else {
-			combinedMap[string(iep.CIDR)] = iep
+			combinedMap[cidrKey] = iep
 		}
 	}
-	if len(combinedMap) > 0 {
-		return maps.Values(combinedMap)
+
+	// Add remaining combined endpoints
+	for _, ep := range combinedMap {
+		result = append(result, ep)
 	}
-	return nil
+
+	return result
+}
+
+// exceptionsEqual checks if two exception lists are identical
+func exceptionsEqual(a, b []policyinfo.NetworkAddress) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i, addr := range a {
+		if addr != b[i] {
+			return false
+		}
+	}
+	return true
 }
 
 func (m *policyEndpointsManager) newPolicyEndpoint(policy *networking.NetworkPolicy,
diff --git a/pkg/policyendpoints/manager_test.go b/pkg/policyendpoints/manager_test.go
index 16049e7..5d63f26 100644
--- a/pkg/policyendpoints/manager_test.go
+++ b/pkg/policyendpoints/manager_test.go
@@ -553,3 +553,36 @@ func Test_processPolicyEndpoints(t *testing.T) {
 	assert.Equal(t, 3, len(pes[0].Spec.Ingress[0].Ports))
 	assert.Equal(t, 2, len(pes[0].Spec.Egress[0].Ports))
 }
+
+func TestCombineRulesEndpoints_MultipleEgressRules(t *testing.T) {
+	protocolUDP := corev1.ProtocolUDP
+	port53 := int32(53)
+
+	// This test demonstrates the bug where combineRulesEndpoints incorrectly
+	// merged contradictory egress rules with the same CIDR,
+	// see https://github.com/aws/amazon-network-policy-controller-k8s/issues/146
+	endpoints := []policyinfo.EndpointInfo{
+		{
+			// First rule: allow traffic to internet, except private ranges
+			CIDR: "0.0.0.0/0",
+			Except: []policyinfo.NetworkAddress{
+				"10.0.0.0/8",
+				"172.16.0.0/12",
+				"192.168.0.0/16",
+			},
+			Ports: []policyinfo.Port{},
+		},
+		{
+			// Second rule: allow DNS traffic to any destination (including private ranges)
+			CIDR:   "0.0.0.0/0",
+			Except: []policyinfo.NetworkAddress{}, // No exceptions = allow to anywhere
+			Ports: []policyinfo.Port{
+				{Protocol: &protocolUDP, Port: &port53},
+			},
+		},
+	}
+
+	combined := combineRulesEndpoints(endpoints)
+
+	assert.Len(t, combined, 2, "Should keep contradictory rules separate, not combine them")
+}