Merge pull request #8259 from tomastigera/tomas-bpf-internal-traffic-…

…policy-fix [BPF] ClusterIP should reflect InternalTrafficPolicy=Local
projectcalico · Nov 23, 2023 · 408246a · 408246a
2 parents 6f713d2 + ad65c95
commit 408246a
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 9 deletions.
diff --git a/felix/bpf/nat/maps.go b/felix/bpf/nat/maps.go
@@ -255,12 +255,16 @@ func (v FrontendValue) FlagsAsString() string {
 		flg := uint32(1 << i)
 		if flgs&flg != 0 {
 			fstr += flgTostr[int(flg)]
+			fstr += ", "
 		}
 		flgs &= ^flg
 		if flgs == 0 {
 			break
 		}
-		fstr += ", "
+	}
+
+	if fstr != "" {
+		return fstr[:len(fstr)-2]
 	}
 
 	return fstr

diff --git a/felix/bpf/proxy/syncer.go b/felix/bpf/proxy/syncer.go
@@ -780,7 +780,12 @@ func (s *Syncer) updateService(skey svcKey, sinfo k8sp.ServicePort, id uint32, e
 		cnt++
 	}
 
-	if err := s.writeSvc(sinfo, id, cnt, local, 0); err != nil {
+	flags := uint32(0)
+	if sinfo.InternalPolicyLocal() {
+		flags |= nat.NATFlgInternalLocal
+	}
+
+	if err := s.writeSvc(sinfo, id, cnt, local, flags); err != nil {
 		return 0, 0, err
 	}
 

diff --git a/felix/bpf/proxy/syncer_test.go b/felix/bpf/proxy/syncer_test.go
@@ -816,6 +816,8 @@ var _ = Describe("BPF Syncer", func() {
 			Expect(ok).To(BeTrue())
 			Expect(val1.Count()).To(Equal(uint32(4)))
 			Expect(val1.LocalCount()).To(Equal(uint32(2)))
+			// ClusterIP only reflects internal traffic policy, not the external one
+			Expect(val1.Flags()).To(Equal(uint32(nat.NATFlgInternalLocal)))
 
 			val2, ok := svcs.m[nat.NewNATKey(net.IPv4(192, 168, 0, 1), 4444, proxy.ProtoV1ToIntPanic(v1.ProtocolTCP))]
 			Expect(ok).To(BeTrue())

diff --git a/felix/fv/bpf_test.go b/felix/fv/bpf_test.go
@@ -2837,9 +2837,14 @@ func describeBPFTests(opts ...bpfTestOpt) bool {
 						clusterIP := testSvc.Spec.ClusterIP
 						port := uint16(testSvc.Spec.Ports[0].Port)
 
-						cc.ExpectSome(w[0][1], TargetIP(clusterIP), port)
-						cc.ExpectSome(w[1][0], TargetIP(clusterIP), port)
-						cc.ExpectSome(w[1][1], TargetIP(clusterIP), port)
+						exp := Some
+						if intLocal {
+							exp = None
+						}
+
+						cc.Expect(Some, w[0][1], TargetIP(clusterIP), ExpectWithPorts(port))
+						cc.Expect(exp, w[1][0], TargetIP(clusterIP), ExpectWithPorts(port))
+						cc.Expect(exp, w[1][1], TargetIP(clusterIP), ExpectWithPorts(port))
 						cc.CheckConnectivity()
 					})
 
@@ -2850,12 +2855,12 @@ func describeBPFTests(opts ...bpfTestOpt) bool {
 							node0IP := felixIP(0)
 							node1IP := felixIP(1)
 
-							// Should work through the nodeport where the pods is
+							// Should work through the nodeport from a pod on the node where the backend is
 							cc.ExpectSome(w[0][1], TargetIP(node0IP), npPort)
-							cc.ExpectSome(w[1][0], TargetIP(node0IP), npPort)
-							cc.ExpectSome(w[1][1], TargetIP(node0IP), npPort)
 
-							// Should not work through the nodeport where the pod isn't
+							// Should not work through the nodeport from a node where the backend is not.
+							cc.ExpectNone(w[1][0], TargetIP(node0IP), npPort)
+							cc.ExpectNone(w[1][1], TargetIP(node0IP), npPort)
 							cc.ExpectNone(w[0][1], TargetIP(node1IP), npPort)
 							cc.ExpectNone(w[1][0], TargetIP(node1IP), npPort)
 							cc.ExpectNone(w[1][1], TargetIP(node1IP), npPort)