From cfe4379473e7817bb745811c8d9304e9268cdad7 Mon Sep 17 00:00:00 2001 From: Tomas Hruby Date: Mon, 17 Jun 2024 14:16:55 -0700 Subject: [PATCH 1/2] set RPF to strict as test provision for that. Make tests execute the path again after we introduced the option to disable RPF. --- felix/bpf/ut/bpf_prog_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/felix/bpf/ut/bpf_prog_test.go b/felix/bpf/ut/bpf_prog_test.go index 286aefb3ec4..b9eac67dbaf 100644 --- a/felix/bpf/ut/bpf_prog_test.go +++ b/felix/bpf/ut/bpf_prog_test.go @@ -701,11 +701,12 @@ func objLoad(fname, bpfFsDir, ipFamily string, topts testOpts, polProg, hasHostC } else if topts.ipv6 { ifaceLog := topts.progLog + "-" + bpfIfaceName globals := libbpf.TcGlobalData6{ - Tmtu: natTunnelMTU, - VxlanPort: testVxlanPort, - PSNatStart: uint16(topts.psnaStart), - PSNatLen: uint16(topts.psnatEnd-topts.psnaStart) + 1, - Flags: libbpf.GlobalsIPv6Enabled | libbpf.GlobalsNoDSRCidrs, + Tmtu: natTunnelMTU, + VxlanPort: testVxlanPort, + PSNatStart: uint16(topts.psnaStart), + PSNatLen: uint16(topts.psnatEnd-topts.psnaStart) + 1, + Flags: libbpf.GlobalsIPv6Enabled | libbpf.GlobalsNoDSRCidrs | + libbpf.GlobalsRPFOptionStrict, LogFilterJmp: 0xffffffff, } From 70daa434ea9d92e7a12d9989c98ec0b242fabd80 Mon Sep 17 00:00:00 2001 From: Tomas Hruby Date: Mon, 17 Jun 2024 15:00:00 -0700 Subject: [PATCH 2/2] 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 https://github.com/projectcalico/calico/issues/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 https://github.com/projectcalico/calico/issues/8918 --- felix/bpf-gpl/tc.c | 12 ++- felix/bpf/ut/unknown_dest_test.go | 125 ++++++++++++++++++++++++++++++ felix/bpf/ut/whitelist_test.go | 4 +- 3 files changed, 138 insertions(+), 3 deletions(-) create mode 100644 felix/bpf/ut/unknown_dest_test.go diff --git a/felix/bpf-gpl/tc.c b/felix/bpf-gpl/tc.c index 3854b304a18..f346cf1eb97 100644 --- a/felix/bpf-gpl/tc.c +++ b/felix/bpf-gpl/tc.c @@ -597,6 +597,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)) { @@ -1280,7 +1290,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 new file mode 100644 index 00000000000..3a41db5a354 --- /dev/null +++ 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 index e003980b4e6..b81d4f9c6e3 100644 --- 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)