[BPF] skip FIB when dest is unknown at HEP

* fix when CALI_ST_SKIP_FIB is set on the way to the host, set CALI_CT_FLAG_SKIP_FIB on conntrack - not just when from WEP * add test for ^^^ and issue #6450 * In addition to skipping FIB when there is no route to post-dnat destination, also skip FIB when there is a route, but it is not local while there was no service involved. In that case, we are not forwarding a service (NodePort) to another node and we should only forward locally. Let the host decide what to do with such a packet. Fixes #8918
projectcalico · Jun 17, 2024 · 327c4fd · 327c4fd
1 parent d7cb0d4
commit 327c4fd
Show file tree

Hide file tree

Showing 3 changed files with 138 additions and 3 deletions.
diff --git a/felix/bpf-gpl/tc.c b/felix/bpf-gpl/tc.c
@@ -598,6 +598,16 @@ static CALI_BPF_INLINE void calico_tc_process_ct_lookup(struct cali_tc_ctx *ctx)
 			goto skip_policy;
 		}
 		ctx->state->flags |= CALI_ST_DEST_IS_HOST;
+	} else if (CALI_F_FROM_HEP && !ctx->nat_dest && !cali_rt_is_local(dest_rt)) {
+		/* Disable FIB, let the packet go through the host after it is
+		 * policed. It is ingress into the system and we got a packet, which is
+		 * not for this host, and it wasn't resolved as a service and it is not
+		 * for a local workload either. But we hit a route so it may be some L2
+		 * broadcast, we do not quite know. Let the host route it or dump it.
+		 *
+		 * https://github.com/projectcalico/calico/issues/8918
+		 */
+		ctx->state->flags |= CALI_ST_SKIP_FIB;
 	}
 
 	if (CALI_F_TO_HEP && ctx->nat_dest && !skb_seen(ctx->skb) && !(ctx->state->flags & CALI_ST_HOST_PSNAT)) {
@@ -1306,7 +1316,7 @@ int calico_tc_skb_new_flow_entrypoint(struct __sk_buff *skb)
 	if (state->flags & CALI_ST_NAT_OUTGOING) {
 		ct_ctx_nat->flags |= CALI_CT_FLAG_NAT_OUT;
 	}
-	if (CALI_F_FROM_WEP && state->flags & CALI_ST_SKIP_FIB) {
+	if (CALI_F_TO_HOST && state->flags & CALI_ST_SKIP_FIB) {
 		ct_ctx_nat->flags |= CALI_CT_FLAG_SKIP_FIB;
 	}
 	/* Packets received at WEP with CALI_CT_FLAG_SKIP_FIB mark signal

diff --git a/felix/bpf/ut/unknown_dest_test.go b/felix/bpf/ut/unknown_dest_test.go
@@ -0,0 +1,125 @@
+// Copyright (c) 2024 Tigera, Inc. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package ut_test
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+
+	tcdefs "github.com/projectcalico/calico/felix/bpf/tc/defs"
+
+	"github.com/projectcalico/calico/felix/bpf/routes"
+)
+
+func TestUnknownEnterHostNoRoute(t *testing.T) {
+	RegisterTestingT(t)
+
+	bpfIfaceName = "noRt"
+	defer func() { bpfIfaceName = "" }()
+
+	_, _, _, _, pktBytes, err := testPacketUDPDefault()
+	Expect(err).NotTo(HaveOccurred())
+
+	resetCTMap(ctMap) // ensure it is clean
+
+	hostIP = node1ip
+
+	// Insert a reverse route for the source workload.
+	rtKey := routes.NewKey(srcV4CIDR).AsBytes()
+	rtVal := routes.NewValue(routes.FlagsRemoteWorkload | routes.FlagInIPAMPool).AsBytes()
+	err = rtMap.Update(rtKey, rtVal)
+	Expect(err).NotTo(HaveOccurred())
+	defer resetRTMap(rtMap)
+
+	skbMark = 0
+	runBpfTest(t, "calico_from_host_ep", nil, func(bpfrun bpfProgRunFn) {
+		res, err := bpfrun(pktBytes)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(res.Retval).To(Equal(resTC_ACT_UNSPEC))
+	})
+
+	expectMark(tcdefs.MarkSeenSkipFIB) // It must have skip FIB set as we did not know what to do with the packet.
+}
+
+func TestUnknownEnterHostRouteNotLocal(t *testing.T) {
+	RegisterTestingT(t)
+
+	bpfIfaceName = "noLo"
+	defer func() { bpfIfaceName = "" }()
+
+	_, _, _, _, pktBytes, err := testPacketUDPDefault()
+	Expect(err).NotTo(HaveOccurred())
+
+	resetCTMap(ctMap) // ensure it is clean
+
+	hostIP = node1ip
+
+	// Insert a reverse route for the source workload.
+	rtKey := routes.NewKey(srcV4CIDR).AsBytes()
+	rtVal := routes.NewValue(routes.FlagsRemoteWorkload | routes.FlagInIPAMPool).AsBytes()
+	err = rtMap.Update(rtKey, rtVal)
+	Expect(err).NotTo(HaveOccurred())
+	defer resetRTMap(rtMap)
+
+	rtKey = routes.NewKey(dstV4CIDR).AsBytes()
+	rtVal = routes.NewValueWithIfIndex(routes.FlagsRemoteWorkload|routes.FlagInIPAMPool, 1).AsBytes()
+	err = rtMap.Update(rtKey, rtVal)
+	Expect(err).NotTo(HaveOccurred())
+
+	skbMark = 0
+	runBpfTest(t, "calico_from_host_ep", nil, func(bpfrun bpfProgRunFn) {
+		res, err := bpfrun(pktBytes)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(res.Retval).To(Equal(resTC_ACT_UNSPEC))
+	})
+
+	expectMark(tcdefs.MarkSeenSkipFIB) // It must have skip FIB set as we did not know what to do with the packet.
+
+	// Next packet will follow conntrack, but must skip FIB as well.
+	skbMark = 0
+	runBpfTest(t, "calico_from_host_ep", nil, func(bpfrun bpfProgRunFn) {
+		res, err := bpfrun(pktBytes)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(res.Retval).To(Equal(resTC_ACT_UNSPEC))
+	})
+
+	expectMark(tcdefs.MarkSeenSkipFIB) // It must have skip FIB set as we did not know what to do with the packet.
+
+	// Next packet will follow conntrack, but must skip FIB as well.
+	skbMark = 0
+	runBpfTest(t, "calico_from_host_ep", nil, func(bpfrun bpfProgRunFn) {
+		res, err := bpfrun(pktBytes)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(res.Retval).To(Equal(resTC_ACT_UNSPEC))
+	})
+
+	expectMark(tcdefs.MarkSeenSkipFIB) // It must have skip FIB set as we did not know what to do with the packet.
+
+	resetCTMap(ctMap)
+
+	rtVal = routes.NewValueWithIfIndex(routes.FlagsRemoteHost|routes.FlagInIPAMPool, 1).AsBytes()
+	err = rtMap.Update(rtKey, rtVal)
+	Expect(err).NotTo(HaveOccurred())
+
+	skbMark = 0
+	runBpfTest(t, "calico_from_host_ep", nil, func(bpfrun bpfProgRunFn) {
+		res, err := bpfrun(pktBytes)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(res.Retval).To(Equal(resTC_ACT_UNSPEC))
+	})
+
+	expectMark(tcdefs.MarkSeenSkipFIB) // It must have skip FIB set as we did not know what to do with the packet.
+}
diff --git a/felix/bpf/ut/whitelist_test.go b/felix/bpf/ut/whitelist_test.go
@@ -116,7 +116,7 @@ func TestAllowEnterHostToWorkload(t *testing.T) {
 	err = rtMap.Update(rtKey, rtVal)
 	Expect(err).NotTo(HaveOccurred())
 	rtKey = routes.NewKey(dstV4CIDR).AsBytes()
-	rtVal = routes.NewValueWithIfIndex(routes.FlagsRemoteWorkload|routes.FlagInIPAMPool, 1).AsBytes()
+	rtVal = routes.NewValueWithIfIndex(routes.FlagsLocalWorkload|routes.FlagInIPAMPool, 1).AsBytes()
 	err = rtMap.Update(rtKey, rtVal)
 	Expect(err).NotTo(HaveOccurred())
 	defer resetRTMap(rtMap)
@@ -338,7 +338,7 @@ func TestAllowEnterHostToWorkloadV6(t *testing.T) {
 	err = rtMapV6.Update(rtKey, rtVal)
 	Expect(err).NotTo(HaveOccurred())
 	rtKey = routes.NewKeyV6(dstV6CIDR).AsBytes()
-	rtVal = routes.NewValueV6WithIfIndex(routes.FlagsRemoteWorkload|routes.FlagInIPAMPool, 1).AsBytes()
+	rtVal = routes.NewValueV6WithIfIndex(routes.FlagsLocalWorkload|routes.FlagInIPAMPool, 1).AsBytes()
 	err = rtMapV6.Update(rtKey, rtVal)
 	Expect(err).NotTo(HaveOccurred())
 	defer resetRTMap(rtMapV6)