Skip to content

Conversation

@Subzidion
Copy link
Member

@Subzidion Subzidion commented Sep 18, 2025

Only combine Rules Endpoints with the same CIDR if they share identical Exception lists. Currently, Rules Endpoints are combined if the share the same CIDR, however, some of these Endpoints are incompatible when combined with each other.

The example from #146 shows a NetworkPolicy with 2 Egress Rules, one allowing egress out to 0.0.0.0/0 except to CIDRs 10.0.0.0/8, 172.16.0.0/12, and 192.168.0.0/16. The second rule allowing all egress traffic out for TCP and UDP on Port 53. Both rules egress out 0.0.0.0/0 and so the controller merges their internal representation. However, this is contradictory, as the second rule should allow Port 53 (DNS) traffic to any CIDR, including the ones mentioned in the except list of Rule 1.

Note: This undoes a lot of #80, so we need to consider the performance impact of making this operation more expensive by doing this.

Rule:

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: curl
  namespace: default
spec:
  egress:
  - to:
    - ipBlock:
        cidr: 0.0.0.0/0
        except:
          - 10.0.0.0/8
          - 172.16.0.0/12
          - 192.168.0.0/16
  - ports:
      - protocol: UDP
        port: 53
      - port: 53
        protocol: TCP
  policyTypes:
    - Egress
  podSelector:
    matchLabels:
      app: curl

This is again visible from #146 in the PolicyEndpoint where the two separate rules appear merged under the single 0.0.0.0/0 Egress Rule.

Name:         curl-4mhvr
Namespace:    test1
Labels:       <none>
Annotations:  <none>
API Version:  networking.k8s.aws/v1alpha1
Kind:         PolicyEndpoint
Metadata:
  Creation Timestamp:  2024-11-12T23:06:46Z
  Generate Name:       curl-
  Generation:          1
  Owner References:
    API Version:           networking.k8s.io/v1
    Block Owner Deletion:  true
    Controller:            true
    Kind:                  NetworkPolicy
    Name:                  curl
    UID:                   acc0d91d-e6ab-46fe-a8bb-aad64e6eaaec
  Resource Version:        1073029
  UID:                     7415e7fe-6711-4df3-8664-ba5ae73d71a1
Spec:
  Egress:
    Cidr:  ::/0
    Ports:
      Port:      53
      Protocol:  UDP
      Port:      53
      Protocol:  TCP
    Cidr:        0.0.0.0/0
    Except:
      10.0.0.0/8
      172.16.0.0/12
      192.168.0.0/16
    Ports:
      Port:      53
      Protocol:  UDP
      Port:      53
      Protocol:  TCP
  Pod Isolation:
    Egress
  Pod Selector:
    Match Labels:
      App:  tester2
  Pod Selector Endpoints:
    Host IP:    192.168.21.1
    Name:       tester2-55c7c875f-xx96w
    Namespace:  test1
    Pod IP:     192.168.15.193
  Policy Ref:
    Name:       curl
    Namespace:  test1
Events:         <none>

The code is changed to only combine Rules Endpoints with the same CIDR if they share identical Exception lists.

What type of PR is this?
Bug Fix.

Which issue does this PR fix: #146

What does this PR do / Why do we need it: Only combine Rules Endpoints with the same CIDR if they share identical Exception lists. Currently, incompatible rules are being merged.

Testing done on this change:
BEFORE:

=== RUN   Test_policyEndpointsManager_computePolicyEndpoints
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/no_existing_endpoints,_create_one
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/no_existing_endpoints,_create_two
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/existing_endpoints,_update
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/delete_unneeded_endpoints
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/create_new_to_fit_endpoints
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/reuse_to_be_deleted_endpoints
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/cleanup_endpoints_with_same_entries
--- PASS: Test_policyEndpointsManager_computePolicyEndpoints (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/no_existing_endpoints,_create_one (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/no_existing_endpoints,_create_two (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/existing_endpoints,_update (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/delete_unneeded_endpoints (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/create_new_to_fit_endpoints (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/reuse_to_be_deleted_endpoints (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/cleanup_endpoints_with_same_entries (0.00s)
=== RUN   Test_processPolicyEndpoints
--- PASS: Test_processPolicyEndpoints (0.00s)
=== RUN   TestCombineRulesEndpoints_MultipleEgressRules
    manager_test.go:587:
        	Error Trace:	/Users/carlhilt/work/eks/networking/network-policy/amazon-network-policy-controller-k8s/pkg/policyendpoints/manager_test.go:587
        	Error:      	"[{0.0.0.0/0 [10.0.0.0/8 172.16.0.0/12 192.168.0.0/16] [{0x140001968c0 0x14000184390 <nil>}]}]" should have 2 item(s), but has 1
        	Test:       	TestCombineRulesEndpoints_MultipleEgressRules
        	Messages:   	Should keep contradictory rules separate, not combine them
--- FAIL: TestCombineRulesEndpoints_MultipleEgressRules (0.00s)
FAIL
FAIL	github.com/aws/amazon-network-policy-controller-k8s/pkg/policyendpoints	0.564s
FAIL

AFTER:

=== RUN   Test_policyEndpointsManager_computePolicyEndpoints
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/no_existing_endpoints,_create_one
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/no_existing_endpoints,_create_two
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/existing_endpoints,_update
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/delete_unneeded_endpoints
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/create_new_to_fit_endpoints
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/reuse_to_be_deleted_endpoints
=== RUN   Test_policyEndpointsManager_computePolicyEndpoints/cleanup_endpoints_with_same_entries
--- PASS: Test_policyEndpointsManager_computePolicyEndpoints (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/no_existing_endpoints,_create_one (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/no_existing_endpoints,_create_two (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/existing_endpoints,_update (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/delete_unneeded_endpoints (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/create_new_to_fit_endpoints (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/reuse_to_be_deleted_endpoints (0.00s)
    --- PASS: Test_policyEndpointsManager_computePolicyEndpoints/cleanup_endpoints_with_same_entries (0.00s)
=== RUN   Test_processPolicyEndpoints
--- PASS: Test_processPolicyEndpoints (0.00s)
=== RUN   TestCombineRulesEndpoints_MultipleEgressRules
--- PASS: TestCombineRulesEndpoints_MultipleEgressRules (0.00s)
PASS
ok  	github.com/aws/amazon-network-policy-controller-k8s/pkg/policyendpoints	(cached)

Test Cluster

BEFORE:

apiVersion: v1
items:
- apiVersion: networking.k8s.aws/v1alpha1
  kind: PolicyEndpoint
  metadata:
    creationTimestamp: "2025-09-18T08:26:33Z"
    generateName: curl-
    generation: 2
    name: curl-h6fxv
    namespace: default
    ownerReferences:
    - apiVersion: networking.k8s.io/v1
      blockOwnerDeletion: true
      controller: true
      kind: NetworkPolicy
      name: curl
      uid: fca05ff9-89b1-416e-9b1d-fa8fbfbdd801
    resourceVersion: "37967"
    uid: 1f0c4cc7-c353-4574-a854-05f8192014c9
  spec:
    egress:
    - cidr: ::/0
      ports:
      - port: 53
        protocol: UDP
      - port: 53
        protocol: TCP
    - cidr: 0.0.0.0/0
      except:
      - 10.0.0.0/8
      - 172.16.0.0/12
      - 192.168.0.0/16
      ports:
      - port: 53
        protocol: UDP
      - port: 53
        protocol: TCP
    podIsolation:
    - Egress
    podSelector:
      matchLabels:
        app: curl
    podSelectorEndpoints:
    - hostIP: 10.4.1.2
      name: curl-test
      namespace: default
      podIP: 10.244.0.27
    policyRef:
      name: curl
      namespace: default
kind: List
metadata:
  resourceVersion: ""

After:

apiVersion: v1
items:
- apiVersion: networking.k8s.aws/v1alpha1
  kind: PolicyEndpoint
  metadata:
    creationTimestamp: "2025-09-18T08:26:33Z"
    generateName: curl-
    generation: 3
    name: curl-h6fxv
    namespace: default
    ownerReferences:
    - apiVersion: networking.k8s.io/v1
      blockOwnerDeletion: true
      controller: true
      kind: NetworkPolicy
      name: curl
      uid: fca05ff9-89b1-416e-9b1d-fa8fbfbdd801
    resourceVersion: "38388"
    uid: 1f0c4cc7-c353-4574-a854-05f8192014c9
  spec:
    egress:
    - cidr: 0.0.0.0/0
      ports:
      - port: 53
        protocol: UDP
      - port: 53
        protocol: TCP
    - cidr: 0.0.0.0/0
      except:
      - 10.0.0.0/8
      - 172.16.0.0/12
      - 192.168.0.0/16
    - cidr: ::/0
      ports:
      - port: 53
        protocol: UDP
      - port: 53
        protocol: TCP
    podIsolation:
    - Egress
    podSelector:
      matchLabels:
        app: curl
    podSelectorEndpoints:
    - hostIP: 10.4.1.2
      name: curl-test
      namespace: default
      podIP: 10.244.0.27
    policyRef:
      name: curl
      namespace: default
kind: List
metadata:
  resourceVersion: ""

Automation added to e2e: N/A

Will this PR introduce any new dependencies?: No.

Will this break upgrades or downgrades. Has updating a running cluster been tested?: No. N/A.

Does this PR introduce any user-facing change?: No, just a bug fix.

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

@Pavani-Panakanti
Copy link

Instead of not combining, is it possible to combine/merge in a way that allow rules are given precedence over deny rules when there are conflicting ones. This is how we handle merging in NP agent today
There are other cases to be handled too as part of this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants