datapath: Per endpoint routes broken for IPv6 traffic between (remote) node to pod when netpol is applied #23910

brb · 2023-02-21T14:04:28Z

The ICMPv6 neighbor resolution is striking us again (#23852). When sending a request from a remote node / a local node to the echo pod, the ICMPv6 NS is using the echo's pod veth iface in the host netns IPv6 addr:

7: lxc75bc3f9009e7    inet6 fe80::ec8b:fdff:fe18:6632/64 scope link

And of course, it's not recognized by the to-container @ lxc75bc3f9009e7:

CPU 07: MARK 0x0 FROM 1192 DEBUG: Inheriting identity=1 from stack
<- stack flow 0x0 , identity host->unknown state unknown ifindex 0 orig-ip 0.0.0.0: fe80::ec8b:fdff:fe18:6632 -> ff02::1:ff00:3816 NeighborSolicitation
CPU 07: MARK 0x0 FROM 1192 DEBUG: Conntrack lookup 1/2: src=[::fe18:6632]:0 dst=[::ff00:3816]:0
CPU 07: MARK 0x0 FROM 1192 DEBUG: Conntrack lookup 2/2: nexthdr=58 flags=0
CPU 07: MARK 0x0 FROM 1192 DEBUG: CT verdict: New, revnat=0
CPU 07: MARK 0x0 FROM 1192 DEBUG: Successfully mapped addr.p4=[::fe18:6632] to identity=2
CPU 07: MARK 0x0 FROM 1192 DEBUG: Attempting local delivery for container id 1192 from seclabel 44107
CPU 07: MARK 0x0 FROM 1192 DEBUG: Policy evaluation would deny packet from 2 to 44107
Policy verdict log: flow 0x0 local EP ID 1192, remote ID world, proto 58, ingress, action deny, match none, fe80::ec8b:fdff:fe18:6632 -> ff02::1:ff00:3816 NeighborSolicitation
xx drop (Policy denied) flow 0x0 to endpoint 1192, ifindex 7, file 2:1604, , identity world->44107: fe80::ec8b:fdff:fe18:6632 -> ff02::1:ff00:3816 NeighborSolicitation

A few possible fixes:

Add the IPv6 local scope addr of a pod into the IPCache, so that the non-world sec ID is used.
Respect the HOST_ID skb mark in the to-container section.

The text was updated successfully, but these errors were encountered:

The per-endpoint routes feature is broken with IPv6 when there are any netpols installed (tracked in cilium/cilium#23852 and cilium/cilium#23910). Once both issues are resolved, we can start testing IPv6 with netpols. Signed-off-by: Martynas Pumputis <m@lambda.lt>

jschwinger233 · 2023-04-06T09:43:06Z

@brb , do you think we can do the same for ICMP6 NS/ND as ARP?

In from-container, we either tail_call CILIUM_CALL_ARP, or pass the skb to stack:

cilium/bpf/bpf_lxc.c

Lines 1354 to 1360 in 62f72cd

    
           #ifdef ENABLE_ARP_PASSTHROUGH 
        
           	case bpf_htons(ETH_P_ARP): 
        
           		ret = CTX_ACT_OK; 
        
           		break; 
        
           #elif defined(ENABLE_ARP_RESPONDER) 
        
           	case bpf_htons(ETH_P_ARP): 
        
           		ep_tail_call(ctx, CILIUM_CALL_ARP);

In to-container, we pass the ARP skb to stack anyhow:

cilium/bpf/bpf_lxc.c

Lines 2163 to 2165 in 62f72cd

    
           #if defined(ENABLE_ARP_PASSTHROUGH) || defined(ENABLE_ARP_RESPONDER) 
        
           	case bpf_htons(ETH_P_ARP): 
        
           		ret = CTX_ACT_OK;

For both directions, our bpf program on lxc doesn't perform policy check for ARP, and I was wondering if it's reasonable to follow this rule and skip policy check for IPv6 NDP at all? If it's acceptable, we won't be bothered by these issues at all.

aspsk · 2023-04-06T10:15:23Z

I had a similar problem in #22006 Namely, it appeared that when endpoint routes are enabled, we send NS requests using link-local addresses of lxc*, and thus pods reply to link-local addresses and they are recognized as world and dropped by fib_redirect as non-routable. I was able to fix this by installing neighbour entries manually. See the 45f8c19 commit (in the https://github.com/aspsk/cilium/tree/aspsk/pr/endpoint-routes-with-bpf-redirection-ipv6 branch).

As we are already installing per-endpoint routes, this is simple to install IPv6 neighbour entries in sync. This fixes the neighbour discovery in case endpoint routes and host routing are enabled together (in the opposite case we drop ICMPv6 NA messages from pods as they are destined to link-local addresses, see also cilium#23910 for a similar problem).

As cilium/cilium#2385 and cilium/cilium#23910 has been resolved, we enable IPv6 test for per-endpoint routing. Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>

This is a fix for a regression in the local addresses logic, introduced in 080857b as part of the implementation for AddressScopeMax. Addresses with the form of link-local unicast addresses began to be filtered out of the local address aggregation, causing them to labeled with the "world" identity for the sake of policy enforcement. Examples of such addresses include: 169.254.10.10 fe80::1234 This caused issues for a variety of users, whose policies allowing "host" traffic would no longer allow traffic to these addresses, forcing the use of workarounds involving CIDR policies, which is not the intended behavior for this type of address. This was a regression as of Cilium 1.12.0-rc2. One reason for this regression was that logic prior to the change looked at the address scope, whereas logic after the change looked at the address bytes, and it was found that many users had assigned addresses of the forms above but with scope global, causing them to again be filtered unconditionally. This patch factors out the local address filtering logic into a function, removes the skip over IsLinkLocalUnicast(), and adds a variety of unit tests for that function. fixes: #25242 fixes: #23910 fixes: #16308 fixes: #20055 Signed-off-by: Andrew Sauber <andrew.sauber@isovalent.com>

brb · 2023-05-08T12:04:18Z

Once fixed, let's remove https://github.com/cilium/cilium-cli/blob/v0.14.1/connectivity/check/test.go#L563.

This is a fix for a regression in the local addresses logic, introduced in 080857b as part of the implementation for AddressScopeMax. Addresses with the form of link-local unicast addresses began to be filtered out of the local address aggregation, causing them to labeled with the "world" identity for the sake of policy enforcement. Examples of such addresses include: 169.254.10.10 fe80::1234 This caused issues for a variety of users, whose policies allowing "host" traffic would no longer allow traffic to these addresses, forcing the use of workarounds involving CIDR policies, which is not the intended behavior for this type of address. This was a regression as of Cilium 1.12.0-rc2. One reason for this regression was that logic prior to the change looked at the address scope, whereas logic after the change looked at the address bytes, and it was found that many users had assigned addresses of the forms above but with scope global, causing them to again be filtered unconditionally. This patch factors out the local address filtering logic into a function, removes the skip over IsLinkLocalUnicast(), and adds a variety of unit tests for that function. fixes: #25242 fixes: #23910 fixes: #16308 fixes: #20055 Signed-off-by: Andrew Sauber <andrew.sauber@isovalent.com>

[ upstream commit 05b6d82 ] This is a fix for a regression in the local addresses logic, introduced in 080857b as part of the implementation for AddressScopeMax. Addresses with the form of link-local unicast addresses began to be filtered out of the local address aggregation, causing them to labeled with the "world" identity for the sake of policy enforcement. Examples of such addresses include: 169.254.10.10 fe80::1234 This caused issues for a variety of users, whose policies allowing "host" traffic would no longer allow traffic to these addresses, forcing the use of workarounds involving CIDR policies, which is not the intended behavior for this type of address. This was a regression as of Cilium 1.12.0-rc2. One reason for this regression was that logic prior to the change looked at the address scope, whereas logic after the change looked at the address bytes, and it was found that many users had assigned addresses of the forms above but with scope global, causing them to again be filtered unconditionally. This patch factors out the local address filtering logic into a function, removes the skip over IsLinkLocalUnicast(), and adds a variety of unit tests for that function. fixes: cilium#25242 fixes: cilium#23910 fixes: cilium#16308 fixes: cilium#20055 Signed-off-by: Andrew Sauber <andrew.sauber@isovalent.com> Signed-off-by: Tim Horner <timothy.horner@isovalent.com>

[ upstream commit 05b6d82 ] This is a fix for a regression in the local addresses logic, introduced in 080857b as part of the implementation for AddressScopeMax. Addresses with the form of link-local unicast addresses began to be filtered out of the local address aggregation, causing them to labeled with the "world" identity for the sake of policy enforcement. Examples of such addresses include: 169.254.10.10 fe80::1234 This caused issues for a variety of users, whose policies allowing "host" traffic would no longer allow traffic to these addresses, forcing the use of workarounds involving CIDR policies, which is not the intended behavior for this type of address. This was a regression as of Cilium 1.12.0-rc2. One reason for this regression was that logic prior to the change looked at the address scope, whereas logic after the change looked at the address bytes, and it was found that many users had assigned addresses of the forms above but with scope global, causing them to again be filtered unconditionally. This patch factors out the local address filtering logic into a function, removes the skip over IsLinkLocalUnicast(), and adds a variety of unit tests for that function. fixes: #25242 fixes: #23910 fixes: #16308 fixes: #20055 Signed-off-by: Andrew Sauber <andrew.sauber@isovalent.com> Signed-off-by: Tim Horner <timothy.horner@isovalent.com>

The per-endpoint routes feature is broken with IPv6 when there are any netpols installed (tracked in cilium#23852 and cilium#23910). Once both issues are resolved, we can start testing IPv6 with netpols. Signed-off-by: Martynas Pumputis <m@lambda.lt>

Previously, our policy check for IPv6 NDP traffic caused issues such as cilium#23852 and cilium#23910 because this traffic was identified as WORLD_ID, which would be given a verdict of drop when CiliumNetworkPolicy is applied for per-endpoint routing. To resolve this issue, we pass all IPv6 NDP traffic to the stack without policy check. This change aligns with how we handle IPv4 ARP: the cilium bpf never performs policy check for ARP, regardless of whether we enable `ENABLE_ARP_PASSTHROUGH` or `ENABLE_ARP_RESPONDER`. Fixes: cilium#23852 Fixes: cilium#23910 Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>

Previously, our policy check for IPv6 NDP traffic caused issues such as #23852 and #23910 because this traffic was identified as WORLD_ID, which would be given a verdict of drop when CiliumNetworkPolicy is applied for per-endpoint routing. To resolve this issue, we pass all IPv6 NDP traffic to the stack without policy check. This change aligns with how we handle IPv4 ARP: the cilium bpf never performs policy check for ARP, regardless of whether we enable `ENABLE_ARP_PASSTHROUGH` or `ENABLE_ARP_RESPONDER`. Fixes: #23852 Fixes: #23910 Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>

As cilium/cilium#2385 and cilium/cilium#23910 has been resolved, we enable IPv6 test for per-endpoint routing. Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>

Previously, our policy check for IPv6 NDP traffic caused issues such as cilium#23852 and cilium#23910 because this traffic was identified as WORLD_ID, which would be given a verdict of drop when CiliumNetworkPolicy is applied for per-endpoint routing. To resolve this issue, we pass all IPv6 NDP traffic to the stack without policy check. This change aligns with how we handle IPv4 ARP: the cilium bpf never performs policy check for ARP, regardless of whether we enable `ENABLE_ARP_PASSTHROUGH` or `ENABLE_ARP_RESPONDER`. Fixes: cilium#23852 Fixes: cilium#23910 Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>

As cilium/cilium#2385 and cilium/cilium#23910 has been resolved, we enable IPv6 test for per-endpoint routing. Signed-off-by: Zhichuan Liang <gray.liang@isovalent.com>

brb added kind/bug This is a bug in the Cilium logic. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. feature/ipv6 Relates to IPv6 protocol support labels Feb 21, 2023

jschwinger233 self-assigned this Apr 5, 2023

aspsk mentioned this issue Apr 6, 2023

bpf: allow to enable host_routing and endpoint routes simultaneously #22006

Merged

jschwinger233 mentioned this issue Apr 28, 2023

datapath: pass IPv6 NDP traffic to stack without policy check #24919

Merged

jschwinger233 mentioned this issue May 2, 2023

connectivity: enable IPv6 test for per-endpoint routing cilium/cilium-cli#1562

Merged

asauber mentioned this issue May 5, 2023

datapath: link-local unicast addresses can be "host" #25298

Merged

pchaigno mentioned this issue May 8, 2023

KNPs tests are failing with endpoint routes enabled cilium/cilium-cli#1585

Closed

michi-covalent closed this as completed in #25298 May 10, 2023

asauber self-assigned this May 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

datapath: Per endpoint routes broken for IPv6 traffic between (remote) node to pod when netpol is applied #23910

datapath: Per endpoint routes broken for IPv6 traffic between (remote) node to pod when netpol is applied #23910

brb commented Feb 21, 2023

jschwinger233 commented Apr 6, 2023 •

edited

aspsk commented Apr 6, 2023 •

edited

brb commented May 8, 2023

datapath: Per endpoint routes broken for IPv6 traffic between (remote) node to pod when netpol is applied #23910

datapath: Per endpoint routes broken for IPv6 traffic between (remote) node to pod when netpol is applied #23910

Comments

brb commented Feb 21, 2023

jschwinger233 commented Apr 6, 2023 • edited

aspsk commented Apr 6, 2023 • edited

brb commented May 8, 2023

jschwinger233 commented Apr 6, 2023 •

edited

aspsk commented Apr 6, 2023 •

edited