Fix CNP/CCNP update when selectors change from nil to empty non-nil slices #33506

pippolo84 · 2024-07-01T14:55:19Z

As suggested in #33432, the autogenerated DeepEqual methods to compare IngressCommonRule structs and EgressCommonRule structs do not take into account that a nil slice in one of the fields should not be compared equal to a empty non-nil slice in the same field.
Since #29608, the semantic of a empty non-nil slice in both Ingress and Egress section of CNPs and CCNPs has been changed to not select any identity.
Therefore, a CNP with a nil ToEndpoints should not compare equal to a CNP with an empty non-nil ToEndpoints fields, otherwise the following update:

apiVersion: "cilium.io/v2"
kind: CiliumNetworkPolicy
metadata:
  name: test
spec:
  endpointSelector:
    matchLabels:
      run: tmp-shell
  egress:
  - toCIDR:
    - 1.1.1.1/32
    toPorts:
      - ports:
          - port: "80"

-->

apiVersion: "cilium.io/v2"
kind: CiliumNetworkPolicy
metadata:
  name: test
spec:
  endpointSelector:
    matchLabels:
      run: tmp-shell
  egress:
  - toCIDR:
    - 1.1.1.1/32
    toEndpoints: [] # <-- this
    toPorts:
      - ports:
          - port: "80"

is incorrectly discarded, instead of being propagated to the policy repository.

Related: #29608
Fixes: #33432

pippolo84 · 2024-07-01T15:25:03Z

/test

pippolo84 · 2024-07-01T15:28:59Z

Also, I think we should extend the proposed behavior to:

ToServices, ToGroups and ToNodes on the egress side
FromGroups and FromNodes on the ingress side

WDYT @christarazi ?

christarazi

Nice find, LGTM. Could you add a unit test similar to #33439 to actually test the expected behavior end to end?

pippolo84 · 2024-07-09T17:43:21Z

Could you add a unit test similar to #33439 to actually test the expected behavior end to end?

Actually we already have a similar test to validate the generated policy map starting from an empty non-nil ToEndpoints slice:

cilium/pkg/policy/l4_filter_test.go

Lines 2274 to 2306 in 3f8ec1d

    
           // Case 13: deny all at L3 in case of an empty non-nil toEndpoints slice. 
        
           func TestEgressEmptyToEndpoints(t *testing.T) { 
        
           	td := newTestData() 
        
           	rule := &rule{ 
        
           		Rule: api.Rule{ 
        
           			EndpointSelector: endpointSelectorA, 
        
           			Egress: []api.EgressRule{ 
        
           				{ 
        
           					EgressCommonRule: api.EgressCommonRule{ 
        
           						ToEndpoints: []api.EndpointSelector{}, 
        
           					}, 
        
           					ToPorts: []api.PortRule{{ 
        
           						Ports: []api.PortProtocol{ 
        
           							{Port: "80", Protocol: api.ProtoTCP}, 
        
           						}, 
        
           					}}, 
        
           				}, 
        
           			}, 
        
           		}} 
        
           	buffer := new(bytes.Buffer) 
        
           	ctxFromA := SearchContext{From: labelsA, Trace: TRACE_VERBOSE} 
        
           	ctxFromA.Logging = stdlog.New(buffer, "", 0) 
        
           	t.Log(buffer) 
        
           	state := traceState{} 
        
           	res, err := rule.resolveEgressPolicy(td.testPolicyContext, &ctxFromA, &state, NewL4PolicyMap(), nil, nil) 
        
           	require.NoError(t, err) 
        
           	require.Nil(t, res) 
        
           	require.Equal(t, 1, state.selectedRules) 
        
           	require.Equal(t, 0, state.matchedRules) 
        
           }

But I have added once more to test the propagation of rules update in the policy repository using the same CNP as in #33432 (see commit 8c2eee0)

I did that because before the fix the update would have been discarded here:

cilium/pkg/policy/k8s/cilium_network_policy.go

Lines 39 to 41 in 3f8ec1d

    
           if oldCNP.DeepEqual(cnp) { 
        
           	return nil 
        
           }

Is this enough? I'm afraid a more comprehensive e2e test (from the policy watcher down to the policy map) might not be trivial.

derailed

@pippolo84 Great catch Fabio!

pkg/policy/api/ingress.go

XorNil is a helper that returns true only if one of the two input slices is nil and the other is not. It is useful when comparing two slices and the semantic of a nil slice is different from the semantic of an empty non-nil one. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

The semantic of a nil slice in one of the IngressCommonRule fields is different from the semantic of an empty non-nil slice. In order to compare two IngressCommonRule instances correctly, we add a custom DeepEqual method the explicitly checks for this case before calling the autogenerated method. This allows to correctly propagate CNP/CCNP updates when, for example, the FromEndpoints selector is changed from nil to an empty non-nil slice. In the latter case, the CNP should not select any identity, falling back to the default behavior (e.g: default deny for an allow policy). Fixes: e97df7b ("policy: Do not select any identity with ingress empty slices") Fixes: cilium#33432 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

The semantic of a nil slice in one of the EgressCommonRule fields is different from the semantic of an empty non-nil slice. In order to compare two EgressCommonRule instances correctly, we add a custom DeepEqual method the explicitly checks for this case before calling the autogenerated method. This allows to correctly propagate CNP/CCNP updates when, for example, the ToEndpoints selector is changed from nil to an empty non-nil slice. In the latter case, the CNP should not select any identity, falling back to the default behavior (e.g: default deny for an allow policy). Fixes: 6ccd044 ("policy: Do not select any identity with egress empty slices") Fixes: cilium#33432 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Add a non regression test for GitHub issue cilium#33432: updating a CNP with a nil ToEndpoints slice to an empty non-nil ToEndpoints slice should result in the update not being discarded and the rules in the policy repository reporting the empty non-nil ToEndpoints slice. Related: cilium#33432 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

pippolo84 · 2024-07-12T07:51:35Z

/test

pippolo84 · 2024-07-12T13:17:07Z

CI triage:

ci-e2e failure tracked here
ci-ginkgo trackere here

Rerunning

pippolo84 mentioned this pull request Jul 1, 2024

Fix bug that skips other selectors if ToEndpoints is empty #33439

Closed

pippolo84 marked this pull request as ready for review July 1, 2024 15:25

pippolo84 requested a review from a team as a code owner July 1, 2024 15:25

pippolo84 requested a review from aanm July 1, 2024 15:25

pippolo84 changed the title ~~Fix CNP/CCNP update when selectors change from nil to non-empty slices~~ Fix CNP/CCNP update when selectors change from nil to empty non-nil slices Jul 1, 2024

pippolo84 force-pushed the pr/pippolo84/fix-33432 branch from 6040abc to 28796ee Compare July 1, 2024 15:33

aanm requested review from christarazi and removed request for aanm July 1, 2024 15:40

christarazi reviewed Jul 8, 2024

View reviewed changes

pippolo84 requested a review from a team as a code owner July 9, 2024 17:31

pippolo84 requested a review from derailed July 9, 2024 17:31

pippolo84 requested a review from christarazi July 9, 2024 17:47

christarazi approved these changes Jul 9, 2024

View reviewed changes

derailed approved these changes Jul 9, 2024

View reviewed changes

pkg/policy/api/ingress.go Outdated Show resolved Hide resolved

pippolo84 added 4 commits July 10, 2024 11:18

pippolo84 force-pushed the pr/pippolo84/fix-33432 branch from 8c2eee0 to fc74339 Compare July 10, 2024 09:18

pippolo84 requested a review from a team as a code owner July 10, 2024 09:18

pippolo84 requested a review from dylandreimerink July 10, 2024 09:18

nathanjsweet approved these changes Jul 10, 2024

View reviewed changes

dylandreimerink approved these changes Jul 11, 2024

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 12, 2024

julianwiedmann added this pull request to the merge queue Jul 12, 2024

Merged via the queue into cilium:main with commit 7294716 Jul 12, 2024

giorio94 mentioned this pull request Jul 15, 2024

v1.16 Backports 2024-07-15 #33804

Merged

27 tasks

giorio94 added backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. and removed needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Jul 15, 2024

github-actions bot added backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. and removed backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. labels Jul 15, 2024

cilium-release-bot bot mentioned this pull request Jul 15, 2024

Prepare for release v1.16.0-rc.2 #33831

Merged

pippolo84 mentioned this pull request Jun 20, 2025

policy: fix CIDRGroup ref handling in cilium NetworkPolicy rule spec #40139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix CNP/CCNP update when selectors change from nil to empty non-nil slices #33506

Fix CNP/CCNP update when selectors change from nil to empty non-nil slices #33506

Uh oh!

pippolo84 commented Jul 1, 2024 •

edited

Loading

Uh oh!

pippolo84 commented Jul 1, 2024

Uh oh!

pippolo84 commented Jul 1, 2024

Uh oh!

christarazi left a comment

Uh oh!

pippolo84 commented Jul 9, 2024 •

edited

Loading

Uh oh!

derailed left a comment

Uh oh!

Uh oh!

pippolo84 commented Jul 12, 2024

Uh oh!

pippolo84 commented Jul 12, 2024

Uh oh!

Uh oh!

Fix CNP/CCNP update when selectors change from nil to empty non-nil slices #33506

Fix CNP/CCNP update when selectors change from nil to empty non-nil slices #33506

Uh oh!

Conversation

pippolo84 commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pippolo84 commented Jul 1, 2024

Uh oh!

pippolo84 commented Jul 1, 2024

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

pippolo84 commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derailed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pippolo84 commented Jul 12, 2024

Uh oh!

pippolo84 commented Jul 12, 2024

Uh oh!

Uh oh!

pippolo84 commented Jul 1, 2024 •

edited

Loading

pippolo84 commented Jul 9, 2024 •

edited

Loading