Merge pull request #139629 from Bafff/fix/conntrack-cleanup-no-endpoints

kube-proxy: clear stale conntrack entries for UDP services with no endpoints

pkg/proxy/conntrack/cleanup.go

@@ -86,10 +86,15 @@ func CleanStaleEntries(ct Interface, ipFamily v1.IPFamily,
 			}
 		}
 
-		// a Service without endpoints does not require to clean the conntrack entries associated.
-		if endpoints.Len() == 0 {
-			continue
-		}
+		// Note: a Service without any serving endpoints is processed too, with an
+		// empty endpoints set, so that all the existing entries directed to its
+		// frontends are removed. The REJECT (iptables) / reject (nftables) rule
+		// installed for a Service with no endpoints does not cover the previously
+		// established flows: those are DNATed to the (deleted) endpoint IP before
+		// the reject rule, which matches on the Service IP, can be evaluated.
+		// One-way UDP flows (e.g. statsd) refresh the conntrack entry timeout with
+		// every packet, so without this cleanup they keep sending traffic to the
+		// deleted endpoint IP indefinitely.
 
 		// we need to filter entries that are directed to a Service IP:Port frontend
 		// that does not have an Endpoint IP:Port backend as part of the serving endpoints
@@ -105,8 +110,13 @@ func CleanStaleEntries(ct Interface, ipFamily v1.IPFamily,
 			serviceIPEndpoints[net.JoinHostPort(externalIP.String(), portStr)] = endpoints
 		}
 		// we need to filter entries that are directed to a *:NodePort that does not have
-		// an Endpoint IP:Port backend as part of the serving endpoints
-		if svc.NodePort() != 0 {
+		// an Endpoint IP:Port backend as part of the serving endpoints.
+		// NodePort entries are matched on the destination port only, so with an
+		// empty endpoints set every UDP flow towards that port number would be
+		// removed, including flows not owned by kube-proxy (e.g. traffic to an
+		// unrelated host on the same port). Skip NodePort cleanup for services
+		// without serving endpoints until the match can be restricted to node IPs.
+		if svc.NodePort() != 0 && endpoints.Len() > 0 {
 			// *:NodePort
 			serviceNodePortEndpoints[svc.NodePort()] = endpoints
 		}

pkg/proxy/conntrack/cleanup_test.go

@@ -458,6 +458,7 @@ func TestServiceWithoutEndpoints(t *testing.T) {
 				{
 					Name:     "test-udp",
 					Port:     testServicePort,
+					NodePort: testServiceNodePort,
 					Protocol: v1.ProtocolUDP,
 				},
 			},
@@ -508,10 +509,38 @@ func TestServiceWithoutEndpoints(t *testing.T) {
 	_ = endpointsMap.Update(ect)
 
 	flows := []*netlink.ConntrackFlow{}
-	// 1 valid entry
-	flows = append(flows, generateConntrackEntry(testExternalIP, testServicePort, testServingEndpointIP, testEndpointPort, unix.IPPROTO_UDP))
-	// 1 stale entry
-	flows = append(flows, generateConntrackEntry(testExternalIP, testServicePort, testDeletedEndpointIP, testEndpointPort, unix.IPPROTO_UDP))
+	var entriesAfterCleanup []*netlink.ConntrackFlow
+
+	// All UDP entries directed to the frontends of a service without serving
+	// endpoints are stale and must be cleared, regardless of which (deleted)
+	// endpoint they are DNATed to: established flows bypass the REJECT rule
+	// installed for services with no endpoints and would otherwise keep
+	// blackholing traffic to the deleted endpoint IP indefinitely.
+	for _, origDest := range []string{testClusterIP, testLoadBalancerIP, testExternalIP} {
+		for _, dnatDest := range []string{testServingEndpointIP, testDeletedEndpointIP} {
+			flows = append(flows, generateConntrackEntry(origDest, testServicePort, dnatDest, testEndpointPort, unix.IPPROTO_UDP))
+		}
+	}
+
+	// UDP entries not directed to a service frontend are not owned by
+	// kube-proxy and must be preserved.
+	entry := generateConntrackEntry(testExternalIP, testNonServicePort, testDeletedEndpointIP, testEndpointPort, unix.IPPROTO_UDP)
+	flows = append(flows, entry)
+	entriesAfterCleanup = append(entriesAfterCleanup, entry)
+
+	// non-UDP entries must be preserved.
+	entry = generateConntrackEntry(testExternalIP, testServicePort, testDeletedEndpointIP, testEndpointPort, unix.IPPROTO_TCP)
+	flows = append(flows, entry)
+	entriesAfterCleanup = append(entriesAfterCleanup, entry)
+
+	// *:NodePort entries are matched on the destination port only, which with an
+	// empty endpoints set would also remove flows not owned by kube-proxy, so
+	// NodePort cleanup is skipped for services without serving endpoints and
+	// these entries must be preserved.
+	entry = generateConntrackEntry("", testServiceNodePort, testDeletedEndpointIP, testEndpointPort, unix.IPPROTO_UDP)
+	flows = append(flows, entry)
+	entriesAfterCleanup = append(entriesAfterCleanup, entry)
+
 	ct := newConntracker(
 		&fakeHandler{
 			entries: flows,
@@ -520,7 +549,19 @@ func TestServiceWithoutEndpoints(t *testing.T) {
 
 	CleanStaleEntries(ct, testIPFamily, svcPortMap, endpointsMap)
 	actualEntries, _ := ct.ListEntries(ipFamilyMap[testIPFamily])
-	if len(actualEntries) != 2 {
-		t.Errorf("unexpected number of entries, got %d expected %d", len(actualEntries), 2)
+
+	require.Len(t, actualEntries, len(entriesAfterCleanup))
+
+	// sort the actual flows before comparison
+	sort.Slice(actualEntries, func(i, j int) bool {
+		return actualEntries[i].String() < actualEntries[j].String()
+	})
+	// sort the expected flows before comparison
+	sort.Slice(entriesAfterCleanup, func(i, j int) bool {
+		return entriesAfterCleanup[i].String() < entriesAfterCleanup[j].String()
+	})
+
+	if diff := cmp.Diff(entriesAfterCleanup, actualEntries); len(diff) > 0 {
+		t.Errorf("unexpected entries after cleanup: %s", diff)
 	}
 }