policy: fix CIDRGroup ref handling in cilium NetworkPolicy rule spec #40139
Conversation
There was a problem hiding this comment.
Is this a backport of a fix in main? IIRC, @pippolo84 once fixed something similar?
d7891ea to
28ecc1a
Compare
My bad, I didn't add the full context. I have updated the commit message with more details. |
I also remember fixing something similar, but looking at the closed issues/PRs I can't find anything.
Anyway, I see there are tests for each of the above cases, so I guess we should be able to intercept any regression here. 👍 |
This commit fixes an issue introduced as part of another bugfix related to CIDRGroupRef in cilium policy rule spec (mentioned below). In the current CIDR group reference handling, if *any* rule in the network policy spec contains a CIDR group reference, we explicitly set `FromCIDRSet` as an empty slice even if that specific rule doesn't contain the reference. https://github.com/cilium/cilium/blob/v1.16.11/pkg/policy/k8s/cilium_cidr_group.go#L228C3-L230C4 This causes a conflict in policy resolution if there is another mutually exclusive selector present in that rule(`fromEntities`, `fromCIDR`), resulting in the rule not processing the intended selectors. https://github.com/cilium/cilium/blob/v1.16/pkg/policy/api/ingress.go#L223 For Example the first rule in the below spec skips processing the `fromEntities` selector thinking there is another empty selector `fromCIDRSet`. ``` spec: ingress: - fromEntities: - cluster - fromCIDRSet: - cidrGroupRef: cidr-group ``` This fix processes CIDR Group reference in the spec rule only if that rule specifically has a reference. This issue only effects v1.16. For v1.17 and above the handling of CiliumCIDRGroup has been refactored and decoupled from CNP processing. Now we don't transform the CNP object by resolving references against CIDRGroup object. Instead an identity is created for this object and CNP directly creates the selector from CIDR Group reference that selects it. https://github.com/cilium/cilium/blob/v1.17/pkg/policy/api/cidr.go#L201-L206 Fixes cilium#36393 Fixes: 0a5d2f7 ("policy: wrong cidrGroupRefs select nothing not all") Signed-off-by: fristonio <deepeshpathak09@gmail.com>
28ecc1a to
65d8b9d
Compare
|
Thanks for sharing the context Fabio. I see that the PR you shared attest to the behavior we expect from empty vs nil selectors. I have added the unit test to verify the behavior where translating the policy to resolve CIDR references shouldn't effect selectors in other rule part of the spec. This test fails before the fix and what this change fixes. |
|
/test |
There was a problem hiding this comment.
The fix LGTM and makes sense. Having the unit test validate that the fix is correct is good, thanks for adding. Did you verify that this implementation is aligned with v1.17 and main behavior? What we don't want is subtly different behavior in different versions for the same policy.
|
Hey Chris! Yeah, i have verified the same policy with both v1.17 and main. The way we realize the policy is different between v1.16 and later so inherently there is a behavioral difference. For example with v1.17 the CIDR group policy are directly selected for CIDR group identity: With v1.16 we create a separate identity for each CIDR in the Group object(see PR summary). |
See commit message for more details.
This CCNP now generates the expected policy.
Fixes: #36393