Skip to content

Conversation

pippolo84
Copy link
Member

@pippolo84 pippolo84 commented Oct 29, 2024

BPF Masquerading relies on IP{V4,V6}_SNAT_EXCLUSION_DST_CIDR datapath config value to know which destination CIDRs should not be SNATed for pod to pod traffic.

In case of IPAM modes "eni", "azure" or "alibabacloud", if "ipv4-native-routing-cidr" is not explicitly set, the exclusion destination CIDR is autodetected at runtime, specifically from the relevant cloud provider Status section in the CiliumNode CRD.

In commit 50e42f1 the way that autodetection logic propagates that value was changed and it now relies on the LocalNodeStore to emit an update for the new IPv4 native routing CIDR value. But the datapath still reads the value stored in the global config option variable, thus it ignores the update from the autodetection logic.

To let the datapath configure the correct SNAT exclusion CIDR, let the loader rely on the local node store read by the orchestrator, instead of reading the global configuration options directly.

This guarantees that the datapath will be reinitialized in case of a runtime update to the IPv4NativeRoutingCIDR following the autodetection.

Furthermore, update the cilium status output to reflect the actual masquerading status reading the native routing CIDRs from the local node store.

Fixes BPF Masquerading exclusion CIDR for IPAM modes "eni", "azure" and "alibabacloud".

@pippolo84 pippolo84 added kind/bug This is a bug in the Cilium logic. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/ipam labels Oct 29, 2024
@pippolo84

This comment was marked as outdated.

@pippolo84 pippolo84 force-pushed the pr/pippolo84/fix-ipv4-native-routing-cidr-autodetect branch 2 times, most recently from f8a57e8 to f2e11e7 Compare October 29, 2024 18:37
@pippolo84
Copy link
Member Author

/test

@pippolo84 pippolo84 force-pushed the pr/pippolo84/fix-ipv4-native-routing-cidr-autodetect branch 3 times, most recently from 6983ed2 to 29bac1c Compare October 30, 2024 10:12
BPF Masquerading relies on IP{V4,V6}_SNAT_EXCLUSION_DST_CIDR datapath
config value to know which destination CIDRs should not be SNATed for
pod to pod traffic.

In case of IPAM modes "eni", "azure" or "alibabacloud", if
"ipv4-native-routing-cidr" is not explicitly set, the exclusion
destination CIDR is autodetected at runtime, specifically from the
relevant cloud provider Status section in the CiliumNode CRD.

In commit 50e42f1 the way that autodetection logic propagates that
value was changed and it now relies on the LocalNodeStore to emit an
update for the new IPv4 native routing CIDR value. But the datapath
still reads the value stored in the global config option variable, thus
it ignores the update from the autodetection logic.

To let the datapath configure the correct SNAT exclusion CIDR, let the
loader rely on the local node store read by the orchestrator, instead of
reading the global configuration options directly.  This guarantees that
the datapath will be reinitialized in case of a runtime update to the
IPv4NativeRoutingCIDR following the autodetection.

Finally, to correctly reflect the current masquerading status in
`cilium-dbg status`, the exclusion CIDRs should be read from the local
node store instead of the possibly stale global configuration options.

Related: 50e42f1 ("node: Add ip{v4,v6} native routing CIDR")

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84 pippolo84 force-pushed the pr/pippolo84/fix-ipv4-native-routing-cidr-autodetect branch from 29bac1c to 022c4c8 Compare October 30, 2024 10:14
@pippolo84
Copy link
Member Author

/test

@pippolo84 pippolo84 marked this pull request as ready for review October 30, 2024 10:27
@pippolo84 pippolo84 requested review from a team as code owners October 30, 2024 10:27
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super familiar with the orchestrator, but from a masquerading logic perspective this seems safe.

@joestringer
Copy link
Member

Is there a test that exercises this logic?

@pippolo84
Copy link
Member Author

Is there a test that exercises this logic?

Not yet, but I intend to add one (issue here) based on the manual test I used to verify this PR (basically it's the repro proposed here)

@maintainer-s-little-helper 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 Nov 4, 2024
@julianwiedmann julianwiedmann added this pull request to the merge queue Nov 4, 2024
Merged via the queue into cilium:main with commit 69e172e Nov 4, 2024
74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants