Fix CIDR to World Entity Conversion Bug #22625

nathanjsweet · 2022-12-08T18:27:07Z

Previously if either the "0.0.0.0/0" CIDR (hereafter "world-ipv4")
or a world-ipv6 (i.e. "::/0") CIDR was present the selector creation logic
for CIDRs would also add the "reserved:world" entity to the
selector labels generated by the CIDR for the sake of the policy engine.

This does not work when Cilium is dual-stack mode as the "reserved:world"
entity covers both IPv4 and IPv6, so both CIDRs should be present before such
an addition to the policy engine logic is made. Moreover, dual-stack mode lacks
any mechanism to distinguish world-ipv4 and world-ipv6 from one another.
This bug has prevented the possibility of managing world-ipv4 and world-ipv6
separately from one another in network policies.

Separate identities (and labels, etc) for world-ipv4 and world-ipv6 now exist
so they can be distinguished.

Signed-off-by: Nate Sweet nathanjsweet@pm.me

Fixes: #22610

bug: In dual-stack mode (both IPv4 and IPv6 are enabled), Cilium incorrectly converted CIDRs that covered all possible addresses for an IP Family (e.g. 0.0.0.0/0) to the "reserved:world" entity. Both IP families must be completely covered for "reserved:world" to apply. This resulted in dual-stack mode network policies that could not distinguish between world IPv4 and IPv6 traffic, treating them as one entity instead.

aanm

LGTM @nathanjsweet just a heads-up that since we have branched v1.13, this PR will not be part of the 1.13 unless it is set with the needs-backport/1.13 label.

nathanjsweet · 2022-12-09T18:22:37Z

/test

joestringer

Thanks for the fix! LGTM.

joestringer · 2022-12-14T02:17:44Z

Smoke tests are failing with agents reporting Error: unknown command "build-config" for "cilium", probably this just means the PR is not based against a recent master version?

Other failures are non-required builds ConformanceGKE and Cilium Datapath. @nathanjsweet can you rebase this PR so we can see that the smoke tests can pass?

joestringer · 2022-12-14T02:18:32Z

Ooh, I found a magic "Update with rebase" button on GitHub, so I clicked it. Let's see.

joestringer · 2022-12-14T06:27:25Z

/test

joestringer · 2022-12-14T07:02:11Z

@nathanjsweet it looks like the Cilium Datapath tests are failing consistently on a CIDR deny policy issue. Do you think that this may be related to the PR content? I don't see any similar failures in other recent failures for the workflow.

pchaigno · 2022-12-14T12:11:26Z

@nathanjsweet it looks like the Cilium Datapath tests are failing consistently on a CIDR deny policy issue. Do you think that this may be related to the PR content? I don't see any similar failures in other recent failures for the workflow.

I checked one of the failures and it does include a policy with the 0.0.0.0/0 CIDR. Since all tests are failing with the exact same test cases, I assume they all have the same policy. Seems fairly likely to be related to the changes here.

pkg/policy/api/cidr.go

joestringer · 2022-12-14T22:40:24Z

@nathanjsweet were you able to reproduce / track down the datapath failure?

nathanjsweet · 2022-12-14T23:00:52Z

@joestringer no, not yet. I was waiting to see if this fixed it.

nathanjsweet · 2022-12-14T23:01:09Z

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathCustomCalls Basic test with byte-counter Loads byte-counter and gets consistent values, with per-endpoint routes

Failure Output

FAIL: Byte count (980) differs from expected value (490)

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

pchaigno

Blocking on above conversation with Joe and CI failure. I suspect the two are related 🙂

nathanjsweet · 2023-01-12T18:53:26Z

/test

github-actions · 2023-02-12T02:02:40Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

nathanjsweet · 2023-02-12T22:53:27Z

I meant to update this PR, but never did. This PR is also blocked on #22966 as it significantly alters the policy engine code. Making this change should happen after that PR is merged.

nathanjsweet · 2023-08-15T17:26:13Z

/test

christarazi

LGTM from proxy side

joestringer

The latest changes for encap look good, and there's one decap case handled, then I think there's a couple more remaining decap cases in bpf/bpf_overlay.c:handle_ipv4() and bpf/bpf_overlay.c:handle_ipv6() , both for the high scale and regular tunnel decap cases without encryption.

These suggest to me that we don't have policy coverage for an IPv4-only 0.0.0.0/0 allow for pods where the traffic is received from outside the cluster through a nodeport on a different node. That sort of test should be able to highlight a policy deny issue with this latest implementation.

nathanjsweet · 2023-08-17T16:38:19Z

/test

bpf/bpf_overlay.c

bpf/lib/overloadable_skb.h

Previously if either the "0.0.0.0/0" CIDR (hereafter "world-ipv4") or a world-ipv6 CIDR was present the selector creation logic for CIDRs would also add the "reserved:world" entity to the selector labels generated by the CIDR. This does not work when Cilium is dual-stack mode as "reserved:world" covers both IPv4 and IPv6, so both CIDRs should be present before such an addition is made. Moreover, dual-stack mode lacked any mechanism to distinguish world-ipv4 and world-ipv6 from another. This bug has prevented the possibility of managing world-ipv4 and world-ipv6 separately from one another in the policy engine. This inccorect assumption goes all the way to the datapath, where SECLABEL and WORLD_ID were part of drop notification, policy logic, and tunnel logic. SECLABLE and WORLD_ID were both set by userspace compilation logic. Separate identities (and labels, etc) for world-ipv4 and world-ipv6 now exist so they can be distinguished in userspace and in the datapath. Some utility methods have been added in userspace and datapath to check for the necessity of using world-ipv4 and world-ipv6 identities. Explicit tests for dual-stack mode have been added. The generic WORLD_ID is still used as the VNI tag for all tunnel logic in the datapath. When tunnel code is encapsulating any WORLD_IPX_ID identity it will convert the VNI to WORLD_ID. When tunnel code is decapsulating any WORLD_ID VNI it will convert the VNI to either WORLD_IPV4_ID or WORLD_IPV6_ID based on the underlying packet protocol. Signed-off-by: Nate Sweet <nathanjsweet@pm.me>

nathanjsweet · 2023-08-17T17:30:43Z

/test

bpf/bpf_overlay.c

Previously, we had a single label, `reserved:world`, applied to all CIDR (non-cluster) identities. With cilium#22625, this is no longer the case; CIDR identities get either `reserved:world-ipv4` or `reserved:world-ipv6` labels **in dual stack clusters**. This PR updates the `toEntities: world` selector to select *all* world entities, as it worked previously. It does so by expanding the set of selectors underlying the `world` entity. No other magic in the SelectorCache or policy engine is required. Fixes: cilium#29666 Fixes: a94fa56 Signed-off-by: Casey Callendrello <cdc@isovalent.com>

Previously, we had a single label, `reserved:world`, applied to all CIDR (non-cluster) identities. With #22625, this is no longer the case; CIDR identities get either `reserved:world-ipv4` or `reserved:world-ipv6` labels **in dual stack clusters**. This PR updates the `toEntities: world` selector to select *all* world entities, as it worked previously. It does so by expanding the set of selectors underlying the `world` entity. No other magic in the SelectorCache or policy engine is required. Fixes: #29666 Fixes: a94fa56 Signed-off-by: Casey Callendrello <cdc@isovalent.com>

[ upstream commit 1288473 ] Previously, we had a single label, `reserved:world`, applied to all CIDR (non-cluster) identities. With cilium#22625, this is no longer the case; CIDR identities get either `reserved:world-ipv4` or `reserved:world-ipv6` labels **in dual stack clusters**. This PR updates the `toEntities: world` selector to select *all* world entities, as it worked previously. It does so by expanding the set of selectors underlying the `world` entity. No other magic in the SelectorCache or policy engine is required. Fixes: cilium#29666 Fixes: a94fa56 Signed-off-by: Casey Callendrello <cdc@isovalent.com>

[ upstream commit 1288473 ] Previously, we had a single label, `reserved:world`, applied to all CIDR (non-cluster) identities. With #22625, this is no longer the case; CIDR identities get either `reserved:world-ipv4` or `reserved:world-ipv6` labels **in dual stack clusters**. This PR updates the `toEntities: world` selector to select *all* world entities, as it worked previously. It does so by expanding the set of selectors underlying the `world` entity. No other magic in the SelectorCache or policy engine is required. Fixes: #29666 Fixes: a94fa56 Signed-off-by: Casey Callendrello <cdc@isovalent.com>

joestringer · 2024-02-28T18:10:05Z

bpf/bpf_overlay.c

-				if (*identity != WORLD_ID)
+				if (identity_is_world_ipv4(*identity))


This looks like it reversed the logic, breaking VTEP support, see #31023.

[ upstream commit 1288473 ] Previously, we had a single label, `reserved:world`, applied to all CIDR (non-cluster) identities. With #22625, this is no longer the case; CIDR identities get either `reserved:world-ipv4` or `reserved:world-ipv6` labels **in dual stack clusters**. This PR updates the `toEntities: world` selector to select *all* world entities, as it worked previously. It does so by expanding the set of selectors underlying the `world` entity. No other magic in the SelectorCache or policy engine is required. Fixes: #29666 Fixes: a94fa56 Signed-off-by: Casey Callendrello <cdc@isovalent.com> Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>

nathanjsweet added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Dec 8, 2022

nathanjsweet requested a review from a team as a code owner December 8, 2022 18:27

nathanjsweet requested a review from aanm December 8, 2022 18:27

aanm approved these changes Dec 9, 2022

View reviewed changes

nathanjsweet added needs-backport/1.11 labels Dec 9, 2022

joestringer approved these changes Dec 14, 2022

View reviewed changes

joestringer force-pushed the pr/nathanjsweet/fix-world-identity-bug branch from 18fa7b3 to eca53f7 Compare December 14, 2022 02:18

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 Dec 14, 2022

pchaigno removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 14, 2022

pchaigno added the dont-merge/bad-bot To prevent MLH from marking ready-to-merge. label Dec 14, 2022

pchaigno reviewed Dec 14, 2022

View reviewed changes

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

nathanjsweet force-pushed the pr/nathanjsweet/fix-world-identity-bug branch from eca53f7 to c600cc9 Compare December 14, 2022 21:15

nathanjsweet requested a review from pchaigno December 14, 2022 21:15

joestringer reviewed Dec 14, 2022

View reviewed changes

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

pchaigno previously requested changes Jan 11, 2023

View reviewed changes

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Feb 12, 2023

christarazi approved these changes Aug 15, 2023

View reviewed changes

joestringer requested changes Aug 15, 2023

View reviewed changes

nathanjsweet force-pushed the pr/nathanjsweet/fix-world-identity-bug branch from 12bb6d6 to c5928d5 Compare August 17, 2023 16:29

nathanjsweet requested a review from joestringer August 17, 2023 16:30

nathanjsweet force-pushed the pr/nathanjsweet/fix-world-identity-bug branch from c5928d5 to 9aaca8b Compare August 17, 2023 16:32

joestringer requested changes Aug 17, 2023

View reviewed changes

bpf/bpf_overlay.c Show resolved Hide resolved

bpf/bpf_overlay.c Show resolved Hide resolved

bpf/bpf_overlay.c Show resolved Hide resolved

bpf/lib/overloadable_skb.h Outdated Show resolved Hide resolved

nathanjsweet force-pushed the pr/nathanjsweet/fix-world-identity-bug branch from 9aaca8b to 21274a7 Compare August 17, 2023 17:30

nathanjsweet requested a review from joestringer August 17, 2023 17:30

joestringer approved these changes Aug 17, 2023

View reviewed changes

bpf/bpf_overlay.c Show resolved Hide resolved

bpf/bpf_overlay.c Show resolved Hide resolved

bpf/bpf_overlay.c Show resolved Hide resolved

nathanjsweet added dont-merge/waiting-for-user-feedback Waiting for feedback from user before the PR should be merged. and removed dont-merge/waiting-for-user-feedback Waiting for feedback from user before the PR should be merged. labels Aug 17, 2023

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 Aug 17, 2023

nathanjsweet merged commit a94fa56 into main Aug 18, 2023

nathanjsweet deleted the pr/nathanjsweet/fix-world-identity-bug branch August 18, 2023 01:48

gandro mentioned this pull request Sep 12, 2023

policy: Move getNets to selector cache #27670

Merged

squeed mentioned this pull request Dec 18, 2023

labels: re-add reserved:world label to all CIDR identities #29956

Closed

squeed mentioned this pull request Dec 18, 2023

policy: expand "world" entity selector to select all address families #29958

Merged

joestringer mentioned this pull request Feb 28, 2024

VTEP traffic is dropped when cluster is configured in VXLAN tunnel mode. #31023

Closed

3 tasks

joestringer reviewed Feb 28, 2024

View reviewed changes

joestringer mentioned this pull request Feb 28, 2024

bpf: Fix VTEP drop check #31039

Merged

nathanjsweet mentioned this pull request May 9, 2025

policy: Remove Redundant Dualstack Identity Insertion #39471

Merged

		if (*identity != WORLD_ID)
		if (identity_is_world_ipv4(*identity))

Fix CIDR to World Entity Conversion Bug #22625

Fix CIDR to World Entity Conversion Bug #22625

Uh oh!

Conversation

nathanjsweet commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

nathanjsweet commented Dec 9, 2022

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

joestringer commented Dec 14, 2022

Uh oh!

joestringer commented Dec 14, 2022

Uh oh!

joestringer commented Dec 14, 2022

Uh oh!

joestringer commented Dec 14, 2022

Uh oh!

pchaigno commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joestringer commented Dec 14, 2022

Uh oh!

nathanjsweet commented Dec 14, 2022

Uh oh!

nathanjsweet commented Dec 14, 2022 • edited by maintainer-s-little-helper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Name

Failure Output

Uh oh!

pchaigno left a comment

Choose a reason for hiding this comment

Uh oh!

nathanjsweet commented Jan 12, 2023

Uh oh!

github-actions bot commented Feb 12, 2023

Uh oh!

nathanjsweet commented Feb 12, 2023

Uh oh!

nathanjsweet commented Aug 15, 2023

Uh oh!

christarazi left a comment

Choose a reason for hiding this comment

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

nathanjsweet commented Aug 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nathanjsweet commented Aug 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joestringer Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nathanjsweet commented Dec 8, 2022 •

edited

Loading

pchaigno commented Dec 14, 2022 •

edited

Loading

nathanjsweet commented Dec 14, 2022 •

edited by maintainer-s-little-helper bot

Loading