Support storing loadbalancer reverse nat state in conntrack #131

tgraf · 2016-09-19T21:58:35Z

No description provided.

aanm · 2016-09-19T22:10:10Z

bpf/Makefile

@@ -18,8 +18,15 @@ bpf_netdev.o:
 bpf_overlay.o:
 	clang ${CLANG_FLAGS} -c bpf_overlay.c -o $@

+LB_OPTIONS = \
+	 \


empty line?

Needed to force a compilation run without additional options

aanm · 2016-09-19T22:14:47Z

daemon/daemon/endpoint.go

@@ -273,6 +273,10 @@ func (d *Daemon) writeBPFHeader(lxcDir string, ep *types.Endpoint, geneveOpts []
 	fmt.Fprintf(fw, "#define CT_MAP6 %s\n", path.Base(common.BPFMapCT6+strconv.Itoa(int(ep.ID))))
 	fmt.Fprintf(fw, "#define CT_MAP4 %s\n", path.Base(common.BPFMapCT4+strconv.Itoa(int(ep.ID))))

+	// Always enable L4 and L3 load balancer for now


we have a d.conf.LBMode flag, at least we could either define them all or none.

Feel free to link it together in a follow up commit as we add higher level integration.

mchalla

In general looks very good. Have a few comments / questions inline. Thanks.

mchalla · 2016-09-19T22:25:41Z

bpf/Makefile

+	 \
+	-DLB_L3 \
+	-DLB_L4 \
+	-DLB_L3 -DLB_L4


Looks like its repeated.

It's a Make list and the foreach iterates over each line below.

aah ok makes sense now.

mchalla · 2016-09-19T22:27:18Z

bpf/Makefile

 bpf_lb.o:
-	clang ${CLANG_FLAGS} -c bpf_lb.c -o $@
+	$(foreach OPTS,$(LB_OPTIONS), \
+		clang ${OPTS} ${CLANG_FLAGS} -c bpf_lb.c -o $@;)


Is this for LB_L3 / LB_L4 or something else ? what would the default usage be ?

The idea here is to compile with each possible combination of configuration options. I want to replace the dummy header files with this to make sure we can compile each program with each possible combination of options that can be provided.

mchalla · 2016-09-19T22:38:42Z

bpf/bpf_lxc.c

 	uint16_t sport;
 	int i, ret;
 	struct portmap local_map[] = {
 		LXC_PORT_MAPPINGS
 	};

 	/* Ignore unknown L4 protocols */
-	if (unlikely(!csum_off))
+	if (nexthdr != IPPROTO_TCP && nexthdr != IPPROTO_UDP)


So we don't support icmp for east west, but support it for north south. I guess it makes sense but its probably more of a test tool in both cases. I actually did not realize l4_checksum_offset did not support that either until I took a look at the function. No need to change anything and just an observation.

This is in the port mapping code. We used the offset function to determine TCP/UDP or something else. As we now need checksum offsets for ICMPv6 I've made the check for TCP/UDP in port mapping explicit to allow using the same offset function for all of them. ICMP for east west is support.

Sounds good.

mchalla · 2016-09-19T23:27:11Z

bpf/bpf_lxc.c


 	/* derive reverse NAT index and zero it. */
 	rev_nat_index = ip6->daddr.s6_addr32[3] & 0xFFFF;
-	if (rev_nat_index) {
-		int csum_off, csum_flags = 0;
+	if ((skb->cb[CB_FLAGS] & CB_F_CLUSTER_DST) && rev_nat_index) {


Is this check for east-west traffic within same cluster ?

I will drop this patch as it was only needed to test LB to endpoints in containers with an address outside of our cluster.

mchalla · 2016-09-19T23:30:04Z

bpf/lib/common.h

@@ -138,15 +138,14 @@ struct drop_notify {
 #define DROP_FRAG_NOSUPPORT	-157
 #define DROP_NO_SERVICE		-158

+#define CB_F_CLUSTER_DST	1
+


May be add a comment above this /* values for CB_FLAGS */

mchalla · 2016-09-19T23:51:18Z

bpf/lib/lb.h

+	if (flags & REV_NAT_F_TUPLE_SADDR) {
+		ipv6_addr_copy(&old_saddr, &tuple->addr);
+		ipv6_addr_copy(&tuple->addr, &nat->address);
+		new_saddr = tuple->addr.addr;


How does this logic work for csum_replace when we are getting the address from the tuple and not the skb ? Are they guaranteed to be same. also I do not see this being invoked for v6, so is this only for v4 ?

new_saddr points to the address in both tuple and packet case. Both ipv6_store_saddr and csum_diff() use new_saddr as pointer for the new address.

This is also handled in both __lb4_rev_nat() and __lb6_rev_nat(). Can you check again what you mean?

What I meant was the logic would work if old_saddr is the same as what would be obtained via ipv6_load_saddr(skb, ETH_HLEN, &old_saddr) so that a later csum_l4_replace will work fine. Because the skb csum was originally computed using whatever address was in the skb ? And the replace is simply offsetting the diff and not recomputing the entire csum.

OK. I understand what you mean. The tuple contains the same address as in the packet so the logic in the revnat is identical. It's simply a matter of avoiding a slow skb_load_bytes()

Provide generic helper to extract L4 checksum field offset and necessary flags to pass into l4_csum_replace(). Also fixes a bug in L4 port mapping where the checksum offset was calculated incorrectly. Fixes: f1c24eb ("L4 port mapping") Acked-by: Andre Martins <andre@cilium.io> Acked-by: Madhu Challa <madhu@cilium.io> Signed-off-by: Thomas Graf <thomas@cilium.io>

Acked-by: Andre Martins <andre@cilium.io> Acked-by: Madhu Challa <madhu@cilium.io> Signed-off-by: Thomas Graf <thomas@cilium.io>

Avoids invoking unnnecssary skb_load_bytes(). Acked-by: Andre Martins <andre@cilium.io> Acked-by: Madhu Challa <madhu@cilium.io> Signed-off-by: Thomas Graf <thomas@cilium.io>

After an outgoing connection has been loadbalanced its reverse NAT index is stored in the connection tracking table to be looked up for reply and related packets. Also makes the loadbalancer configurable via LB_L3 and LB_L4 defines to limit key extraction and logic to either L3, L4 or both. Acked-by: Andre Martins <andre@cilium.io> Acked-by: Madhu Challa <madhu@cilium.io> Signed-off-by: Thomas Graf <thomas@cilium.io>

tgraf added kind/enhancement This would improve or streamline existing functionality. pending-review labels Sep 19, 2016

tgraf assigned mchalla Sep 19, 2016

aanm requested changes Sep 19, 2016

View reviewed changes

aanm approved these changes Sep 19, 2016

View reviewed changes

aanm mentioned this pull request Sep 19, 2016

cilium-kube-proxy ipv6-only implementation #126

Merged

mchalla reviewed Sep 19, 2016

View reviewed changes

tgraf force-pushed the lb-l34 branch from 9b90a75 to 34ab7d5 Compare September 20, 2016 07:41

tgraf added 3 commits September 20, 2016 10:21

bpf: debug: Fix drop reason string lookup

65b9fd2

Acked-by: Andre Martins <andre@cilium.io> Acked-by: Madhu Challa <madhu@cilium.io> Signed-off-by: Thomas Graf <thomas@cilium.io>

bpf: lb: Only attempt L4 reverse NAT if port is stored in revnat

2e63499

Avoids invoking unnnecssary skb_load_bytes(). Acked-by: Andre Martins <andre@cilium.io> Acked-by: Madhu Challa <madhu@cilium.io> Signed-off-by: Thomas Graf <thomas@cilium.io>

tgraf force-pushed the lb-l34 branch from 34ab7d5 to 6ae344a Compare September 20, 2016 08:22

tgraf added acked and removed pending-review labels Sep 20, 2016

tgraf force-pushed the lb-l34 branch from 6ae344a to d4fc3e2 Compare September 20, 2016 09:49

mchalla approved these changes Sep 20, 2016

View reviewed changes

tgraf merged commit 0ac0e63 into master Sep 20, 2016

tgraf deleted the lb-l34 branch September 20, 2016 15:51

agrevtsev mentioned this pull request Oct 3, 2024

"ICMP rules can only be applied when the "enable-icmp-rules" flag is set" in CiliumClusterwideNetworkPolicy status, but --enable-icmp-rules='true' is applied #35214

Closed

3 tasks

Support storing loadbalancer reverse nat state in conntrack #131

Support storing loadbalancer reverse nat state in conntrack #131

Uh oh!

Conversation

tgraf commented Sep 19, 2016

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 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!

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 Sep 19, 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!

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!

Uh oh!

mchalla Sep 19, 2016 •

edited

Loading