Rework container IPv6 to/from container IPv4 via NAT46 by mchalla · Pull Request #190 · cilium/cilium

mchalla · 2016-11-09T22:16:24Z

When sending a IPv6 packet to a IPv4 mapped destination (::FFFF:IPv4) we do NAT64 and convert the packet to an IPv4 packet with a source address corresponding to the IPv4 address of the container and the destination address as specified in the mapped IPv6 address.

The connection tracking entry will remeber that the packet was translated to allow reverse translation for reply packets.

ENABLE_NAT46 depends on LXC_IPV4 and CONNTRACK features to do the IPv4 packet processing
and connection tracking.

Signed-off-by: Madhu Challa madhu@cilium.io

aanm · 2016-11-11T03:04:29Z

 	      tx_closing:1,
-	      reserve:14;
+	      nat46:1,
+	      reserve:13;


Where are these magic numbers coming from?

__u16 is 16 bits.

tgraf · 2016-11-28T07:56:02Z

@mchalla What is the status on this? Are you looking for a review or are you still working on this?

mchalla · 2016-11-28T08:06:37Z

Yes these are the changes after our discussion when you were here. This voids the nat46 branch changes. Pl review these ones. Thanks.

tgraf

Looks almost good to be merged. See some inline comments and please clean up the commit message of this commit a bit.

tgraf · 2016-11-29T14:19:58Z

-			goto to_host;
-		}
+#if defined ENABLE_NAT46 && defined ENABLE_IPV4 && defined CONNTRACK
+		if (unlikely(isMappedIPv6Addr(&tuple->addr))) {


Please don't use camelCase in the c code.

Fixed and moved this function to ipv6.h

tgraf · 2016-11-29T14:25:14Z

-		}
+#if defined ENABLE_NAT46 && defined ENABLE_IPV4 && defined CONNTRACK
+		if (unlikely(isMappedIPv6Addr(&tuple->addr))) {
+			ret = ipv6_to_ipv4(skb, 14, htonl(LXC_IPV4));


Are you sure the htonl() here is correct? LXC_IPV4 should be in network byte order already.

yeah looks like its not in NB order.

IPv4 address: 10.1.15.138

inet_addr("10.1.15.138") = 0x8a0f010a
#define LXC_IPV4 0xa010f8a

I need to check why the conversion is not happening in go code.

tgraf · 2016-11-29T14:26:29Z

-			do_nat46 = 1;
-			goto to_host;
-		}
+#if defined ENABLE_NAT46 && defined ENABLE_IPV4 && defined CONNTRACK


Please define a macro to hide this logic as you are using this guard in multiple places.

tgraf · 2016-11-29T14:31:28Z

+
+		cilium_trace_capture(skb, DBG_CAPTURE_AFTER_V46, skb->ingress_ifindex);
+
+		ret = ipv6_policy(skb, skb->cb[CB_IFINDEX], skb->cb[CB_SRC_LABEL]);


skb->protocol is changed here, right? It might be better to do another tail call into __section_tail(CILIUM_MAP_POLICY, LXC_ID) so we are not inlining the IPv6 policy code segment twice in the program. Did you try that, does the verifier allow for that?

That works.

mchalla · 2016-12-02T00:11:52Z

Also changed the commit message a bit.

tgraf

This looks great now. Some minor stuff left. Also needs a rebase.

tgraf · 2016-12-04T09:49:29Z

Can we use LXC_ID directly here? lxc_id should always be the ID of the container we are forwarding into, right? I think you can save yourself the hassle of extracting the lxc id again.

tgraf · 2016-12-04T09:51:04Z

return addr->p1 == 0 && addr->p2 == 0 && addr->p3 == 0xFFFF0000;

tgraf · 2016-12-04T09:54:16Z

Let's add a warning here to make sure we catch this:

#if defined ENABLE_NAT46 #if defined ENABLE_IPV4 && defined CONNTRACK #define LXC_NAT46 #else #warning "ENABLE_NAT46 requires ENABLE_IPv4 and CONNTRACK" #undef LXC_NAT46 #endif #else #undef LXC_NAT46 #endif

tgraf

LGTM

tgraf · 2016-12-06T07:29:46Z

I think you should move this function into bpf_lxc.c because CILIUM_CALL_IPV4 which is the tail call you make next is only available in that context. We want to avoid somebody making a tail call to CILIUM_CALL_NAT64 from outside lxc context.

tgraf · 2016-12-06T07:31:52Z

Same here. You rely on LXC_ID which is only valid in bpf_lxc.c context. Let's move this tail call wrapper into bpf_lxc.c

Good point, moved the two tail calls to bpf_lxc.c.

@tgraf

If destination is a v4 mapped v6 address ::FFFF:<a.b.c.d> use nat46 to talk to the v4 container. ENABLE_NAT46 depends on LXC_IPV4 and CONNTRACK features. packet flow: v6 container(lxc1) ---------- nat64 -----------> v4 container (lxc2) v6 container(lxc1) <--------- nat46 ------------ v4 container (lxc2) lxc1 v4 conntrack will have needs_nat46=1 in lxc2 -> lxc1 direction. Address @tgraf review comments. 1. LXC_NAT46 to represent combination of ENABLE_NAT46, ENABLE_IPV4 & CONNTRACK 2. rename isMappedIPv6Addr to ipv6_addr_is_mapped and move it to ipv6.h 3. Move ipv6_policy after v46 nat to a tail call. 4. Resolve merge conflict by renaming 07-nat46.sh to 08-nat46.sh 5. Cleanups suggested by @tgraf 6. Move NAT46 processing into its own tail calls to keep the insturction complexity under 4K. 7. Move tail_ipv6_to_ipv4 and tail_ipv4_to_ipv6 to bpf_lxc.c Acked-by: Thomas Graf <thomas@cilium.io> Signed-off-by: Madhu Challa madhu@cilium.io

- cilium#190 ,add new param for storing func return value - use k8s API to get current cilium version - use new param if CM enable with deafult version Signed-off-by: Gergo Szakony <janilution@gmail.com>

add troubleshooting blog

mchalla force-pushed the nat46-1 branch 3 times, most recently from 23db481 to 690dd15 Compare November 10, 2016 11:55

aanm reviewed Nov 11, 2016

View reviewed changes

tgraf added kind/enhancement This would improve or streamline existing functionality. wip labels Nov 28, 2016

tgraf changed the title ~~Enable container with ipv6 address to talk to container with ipv4 add…~~ Enable container IPv6 to/from container IPv4 via NAT46 Nov 28, 2016

mchalla removed the wip label Nov 28, 2016

tgraf added the pending-review label Nov 28, 2016

tgraf suggested changes Nov 29, 2016

View reviewed changes

tgraf added wip and removed pending-review labels Nov 29, 2016

mchalla force-pushed the nat46-1 branch 6 times, most recently from cebc6b7 to 4cc896a Compare December 1, 2016 23:39

mchalla force-pushed the nat46-1 branch from 7d5fb76 to fd41dff Compare December 3, 2016 22:47

tgraf suggested changes Dec 4, 2016

View reviewed changes

tgraf changed the title ~~Enable container IPv6 to/from container IPv4 via NAT46~~ Rework container IPv6 to/from container IPv4 via NAT46 Dec 4, 2016

mchalla force-pushed the nat46-1 branch 2 times, most recently from ffe3b30 to c951456 Compare December 4, 2016 18:21

mchalla removed the wip label Dec 4, 2016

tgraf approved these changes Dec 5, 2016

View reviewed changes

tgraf added the acked label Dec 5, 2016

mchalla force-pushed the nat46-1 branch from 3e3747d to 4e7a6b3 Compare December 6, 2016 00:15

mchalla force-pushed the nat46-1 branch from 4e7a6b3 to 41fb7d8 Compare December 6, 2016 00:16

aanm added pending-review and removed acked labels Dec 6, 2016

tgraf suggested changes Dec 6, 2016

View reviewed changes

mchalla force-pushed the nat46-1 branch from da872e5 to 3577162 Compare December 6, 2016 10:49

mchalla force-pushed the nat46-1 branch from 3577162 to db0d250 Compare December 6, 2016 10:49

tgraf approved these changes Dec 6, 2016

View reviewed changes

mchalla merged commit 9e108c1 into master Dec 6, 2016

mchalla deleted the nat46-1 branch December 6, 2016 11:16

pchaigno mentioned this pull request Jun 3, 2020

cocci: Detect unlogged missed tail calls #11808

Merged

yoursanonymous pushed a commit to yoursanonymous/cilium that referenced this pull request Jan 31, 2026

Merge pull request cilium#190 from cilium/xmulligan-patch-2

bd09e96

add troubleshooting blog


		cilium_trace_capture(skb, DBG_CAPTURE_AFTER_V46, skb->ingress_ifindex);

		ret = ipv6_policy(skb, skb->cb[CB_IFINDEX], skb->cb[CB_SRC_LABEL]);

Conversation

mchalla commented Nov 9, 2016 • edited by tgraf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgraf commented Nov 28, 2016

Uh oh!

mchalla commented Nov 28, 2016

Uh oh!

tgraf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mchalla Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mchalla commented Dec 2, 2016

Uh oh!

tgraf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgraf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mchalla commented Nov 9, 2016 •

edited by tgraf

Loading

mchalla Dec 1, 2016 •

edited

Loading