Handle overlaps in except and allow CIDRs #344

Pavani-Panakanti · 2024-12-06T01:25:08Z

Issue #, if available:
If there is an overlap of IPs in an allow rule and in an except block of other rule, we do not handle the merging

Description of changes:

Example:

egress:
  - to:
      - ipBlock:
          cidr: 192.168.0.0/16
    ports:
      - port: 3306
  - to:
      - ipBlock:
          cidr: 0.0.0.0/0
          except:
            - 192.168.0.0/16

Before this change we evaluate this as

0.0.0.0/0 - startPort:0 endPort: 0
except block - 192.168.0.0/16 - startPort: 0 endPort:0
192.168.0.0/16 - allow on port 3306, as this is part of 0.0.0.0/0 allow on all ports. Append previous ports to this new L4 info. Final L4 ports info looks as below
- startPort: 3306, endPort: 0 - allow
- startPort:0, endPort: 0 - allow
- startPort:0, endPort:0 - deny

The correct behavior for 192.168.0.0/16 should be allow on 3306 as explicit allow takes precedence and deny on rest all ports due to except
New evaluation logic - all except blocks are handled at end

0.0.0.0/0 - startPort:0 endPort: 0
192.168.0.0/16 - allow on port 3306. This is part of 0.0.0.0/0, so check if it is part of any except under this cidr. If yes do not add ports of 0.0.0.0/0 to 192.168.0.0/16. If no, add ports. In this case it is part of except so do not add it
Handle all except at end. For every except key, check if entry is already present in cidrsMap. If yes, there is some explicit allow and everything else will be default deny so no change required. If no, we need to add deny all entry to make sure we deny all traffic and do not allow it if it is part of some other superset CIDR

Other Changes in the PR:

Code refactoring to make it simpler and easy to follow
- catch all need not be handled separately. We evaluate rules in the order of prefix length, so catch all will be handled first followed by others
If no protocol specified, it should be allow any protocol. Changed default to any_ip_protocol from tcp
For any_ip_protocol, check start and end ports for traffic evaluation. Before fix if it is any_protocol we allow on all ports
- example:
  - Port:53 → This should be evaluated as allow any_protocol on port 53

Testing done with multiple cases of except overlaps

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

jayanthvn · 2024-12-07T19:25:47Z

Can you also add a test case for this in our integration suite?

pkg/ebpf/bpf_client.go

pkg/ebpf/c/tc.v4egress.bpf.c

pkg/ebpf/bpf_client_test.go

jayanthvn · 2025-05-31T18:21:22Z

@Pavani-Panakanti - Can you fix conflicts?

Copilot

Pull Request Overview

This PR enhances CIDR rule processing by postponing except handling to the end of rule evaluation, inserting explicit deny‐all entries for excluded ranges, and simplifying catch‐all logic. It also updates default protocol behavior to allow any protocol, refactors key utilities, and enriches eBPF C code with detailed port‐matching comments.

Refactor computeMapEntriesFromEndpointRules to aggregate rules in a cidrsMap, inherit broader‐CIDR ports, and append deny‐all entries for except blocks at the end.
Remove legacy IsCatchAllIPEntry logic and change default protocol to ANY_IP_PROTOCOL instead of TCP; derive port ranges correctly for any_ip_protocol.
Add explicit comments and port‐matching conditions in all four eBPF .bpf.c files to clarify ANY_IP_PROTOCOL and specific‐protocol logic.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

| File | Description |

| -------------------------------------- | -------------------------------------------------------------- |

| pkg/utils/utils_test.go | Removed TestIsCatchAllIPEntry, updated expected IsNonHostCIDR behavior |

| pkg/utils/utils.go | Added DENY_ALL_PROTOCOL, removed IsCatchAllIPEntry, updated deriveProtocolValue and IsNonHostCIDR |
| pkg/ebpf/c/tc.v6ingress.bpf.c | Expanded port‐matching logic comments for ANY_IP_PROTOCOL and specific protocols |
| pkg/ebpf/c/tc.v6egress.bpf.c | Same as above for egress v6 |
| pkg/ebpf/c/tc.v4ingress.bpf.c | Same detailed comments for IPv4 ingress |
| pkg/ebpf/c/tc.v4egress.bpf.c | Same detailed comments for IPv4 egress |
| pkg/ebpf/bpf_client_test.go | Removed old catch‐all test, updated helper from nonHostCIDRs to cidrsMap |
| pkg/ebpf/bpf_client.go | Refactored rule merging into cidrsMap, added addDenyAllL4Entry, removed old catch‐all path |

viveksb007 · 2025-06-15T17:40:12Z

pkg/ebpf/bpf_client.go

+//
+// How it works:
+//   1. A default allow-all entry is added for the node IP to ensure local node traffic is always permitted.
+//   2. The list of firewall rules is sorted by prefix length in descending order. This is crucial for


I think the sorting happens in ascending order of prefix length. I was trying to understand this while refactoring and also wrote a test for this sorting fn https://github.com/aws/aws-network-policy-agent/pull/419/files#diff-36a28a4029b0030c4806fc3502df4be1c44c2f35e54196df5cdf7c93f3448d71

haouc · 2025-06-17T06:20:48Z

pkg/ebpf/bpf_client.go

 		}
+		cidrsMap[string(firewallRule.IPCidr)] = firewallRule


Should I assume .IPCidr has no collision? I am not sure if it is safe to use rule.ipcidr as key.

haouc · 2025-06-17T06:26:23Z

pkg/ebpf/bpf_client.go

+		if len(firewallRule.L4Info) == 0 {
+			log().Debugf("No L4 specified. Add Catch all entry CIDR: %s", string(firewallRule.IPCidr))
+			l.addCatchAllL4Entry(&firewallRule)
+			log().Debugf("Total L4 entries count: %d", len(firewallRule.L4Info))


nit: I think L4Info's len is 0 if getting here.

haouc · 2025-06-17T06:28:29Z

pkg/ebpf/bpf_client.go


+		if existingFirewallRuleInfo, ok := cidrsMap[string(firewallRule.IPCidr)]; ok {
+			firewallRule.L4Info = append(firewallRule.L4Info, existingFirewallRuleInfo.L4Info...)


Just for my curiosity, these L4Info and Except are guaranteed not empty?

haouc · 2025-06-17T06:31:00Z

pkg/ebpf/bpf_client.go

+		// Key format: Prefix length (4 bytes) followed by 4/16byte IP address
+		firewallKey := utils.ComputeTrieKey(*firewallMapKey, l.enableIPv6)
+
+		if len(value.L4Info) != 0 {


nit: maybe > 1 then check to dedup?

haouc · 2025-06-17T06:32:52Z

pkg/ebpf/bpf_client.go

-	for nonHostCIDR, l4Info := range nonHostCIDRs {
-		_, cidrEntry, _ := net.ParseCIDR(nonHostCIDR)
+	for cidr, cidrFirewallInfo := range cidrsMap {
+		if !utils.IsNonHostCIDR(cidr) {


nit: sounds like

if utils.IsNonHostCIDR(cidr) { ... }

will do the work?

haouc · 2025-06-17T06:54:48Z

pkg/ebpf/c/tc.v4egress.bpf.c

+		//    - If trie_val->protocol matches the packet's IP protocol (e.g., TCP or UDP),
+		//    - Then apply the same port match logic as above.
+
+		if ((trie_val->protocol == ANY_IP_PROTOCOL && 


They are really complicate if check... Any chance we can take them out into a func? I think there is only one small different on ip-> for v4 and v6.

haouc · 2025-06-17T06:56:16Z

pkg/ebpf/bpf_client.go

+				if _, ok := cidrsMap[string(exceptCidr)]; !ok {
+					exceptFirewall := EbpfFirewallRules{
+						IPCidr: exceptCidr,
+						Except: []v1alpha1.NetworkAddress{},


nit: they can be assigned with nil.

Pavani-Panakanti added 3 commits December 5, 2024 02:33

changes for except cidrs

f74d533

Update logs

f6cc88b

update denyall logic

0e035bd

Pavani-Panakanti requested a review from a team as a code owner December 6, 2024 01:25

Pavani-Panakanti requested review from orsenthil and jayanthvn December 6, 2024 01:27

update unit tests

645fc4f

Pavani-Panakanti mentioned this pull request Dec 6, 2024

Unexpected ports allowed with egress rules using ipBlock and except #180

Open

Pavani-Panakanti mentioned this pull request Dec 18, 2024

trie value for except should not inherite if [overlaps between except and allow CIDR] or [except is larger than allow CIDR] ; trie value for except should inherite if except is smaller than allow cidr #347

Open

orsenthil reviewed Jan 22, 2025

View reviewed changes

pkg/ebpf/bpf_client.go Outdated Show resolved Hide resolved

orsenthil reviewed Jan 23, 2025

View reviewed changes

pkg/ebpf/c/tc.v4egress.bpf.c Outdated Show resolved Hide resolved

orsenthil reviewed Jan 23, 2025

View reviewed changes

pkg/ebpf/bpf_client_test.go Outdated Show resolved Hide resolved

Pavani-Panakanti added 2 commits May 8, 2025 22:32

Merge branch 'main' into fix-handling-except

e079bed

add description to functions

9a3650b

Pavani-Panakanti mentioned this pull request May 11, 2025

Add integration tests #406

Open

jayanthvn requested a review from Copilot May 31, 2025 18:21

Copilot AI reviewed May 31, 2025

View reviewed changes

Pavani-Panakanti force-pushed the fix-handling-except branch from 8613586 to 9ed76e5 Compare June 2, 2025 02:46

Merge branch 'main' into fix-handling-except

70f1c6f

Pavani-Panakanti force-pushed the fix-handling-except branch from 9ed76e5 to 70f1c6f Compare June 4, 2025 00:38

Merge branch 'main' into fix-handling-except

2fb6ca5

viveksb007 reviewed Jun 15, 2025

View reviewed changes

haouc reviewed Jun 17, 2025

View reviewed changes

merge main

3290f1c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Handle overlaps in except and allow CIDRs #344

Handle overlaps in except and allow CIDRs #344

Uh oh!

Pavani-Panakanti commented Dec 6, 2024 •

edited

Loading

Uh oh!

jayanthvn commented Dec 7, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jayanthvn commented May 31, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

viveksb007 Jun 15, 2025

Uh oh!

haouc Jun 17, 2025

Uh oh!

haouc Jun 17, 2025

Uh oh!

haouc Jun 17, 2025

Uh oh!

haouc Jun 17, 2025

Uh oh!

haouc Jun 17, 2025

Uh oh!

haouc Jun 17, 2025

Uh oh!

haouc Jun 17, 2025

Uh oh!

Uh oh!


		if existingFirewallRuleInfo, ok := cidrsMap[string(firewallRule.IPCidr)]; ok {
		firewallRule.L4Info = append(firewallRule.L4Info, existingFirewallRuleInfo.L4Info...)

Handle overlaps in except and allow CIDRs #344

Are you sure you want to change the base?

Handle overlaps in except and allow CIDRs #344

Uh oh!

Conversation

Pavani-Panakanti commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayanthvn commented Dec 7, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jayanthvn commented May 31, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

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!

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!

Uh oh!

Pavani-Panakanti commented Dec 6, 2024 •

edited

Loading