[kube-proxy/winkernel] Guard against load balancers without port mappings

Project: kubernetes/kubernetes | Language: Go | Concepts: error-handling, testing-strategy, refactoring | Difficulty: mid
Link: https://github.com/kubernetes/kubernetes/pull/141489

The problem

The Windows implementation of kube-proxy (winkernel) could crash if it encountered a load balancer object from HNS (Host Network Service) that had an empty PortMappings array. This could happen if a load balancer was only partially created due to a failure. The code assumed at least one port mapping existed and accessed the first element without checking, causing a panic and terminating kube-proxy.

Why it's non-trivial

A naive fix might simply check for an empty array, but the code must also ensure that these malformed load balancers are skipped entirely, since they cannot be reliably used or cached. Furthermore, the fix should not silently ignore the issue; it should log the occurrence for troubleshooting. Tests must also cover these edge cases to prevent regressions.

The approach

Guard against empty PortMappings and log the skip:

+		// PortMappings is populated by HNS and can be empty, for example when a load
+		// balancer is partially created. Such an entry cannot be keyed by port.
+		if len(lb.PortMappings) == 0 {
+			klog.V(2).InfoS("Skipping load balancer with no port mappings", "hnsLbID", lb.Id)
+			continue
+		}

This snippet adds a check before accessing lb.PortMappings[0]. If the array is empty, it logs a message and skips processing that load balancer. This prevents a panic and provides visibility into the occurrence.

Existing (now safe) access to the first port mapping:

-		isIPv6 := (lb.Flags & LoadBalancerFlagsIPv6) == LoadBalancerFlagsIPv6
-		portMap := lb.PortMappings[0]
+		portMap := lb.PortMappings[0]
+		isIPv6 := (lb.Flags & LoadBalancerFlagsIPv6) == LoadBalancerFlagsIPv6

After the guard, accessing lb.PortMappings[0] is now safe.

Unit tests for the new behavior:

+func TestGetAllLoadBalancers_SkipsLoadBalancersWithoutPortMappings(t *testing.T) {
+	testCases := []struct {
+		name         string
+		portMappings []hcn.LoadBalancerPortMapping
+	}{
+		{
+			name:         "nil port mappings",
+			portMappings: nil,
+		},
+		{
+			name:         "empty port mappings",
+			portMappings: []hcn.LoadBalancerPortMapping{},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			hcnMock := getHcnMock("L2Bridge")
+			hns := hns{hcn: hcnMock}
+
+			hcnMock.PopulateRawQueriedLoadbalancer(&hcn.HostComputeLoadBalancer{
+				Id:           "LBID-malformed",
+				FrontendVIPs: []string{serviceVip},
+				PortMappings: tc.portMappings,
+			})
+			hcnMock.PopulateQueriedLoadbalancers("LBID-valid", serviceVip, protocol, internalPort, externalPort, false, "EPID-1")
+
+			loadBalancers, err := hns.getAllLoadBalancers()
+			assert.NoError(t, err)
+
+			hnsIDs := make([]string, 0, len(loadBalancers))
+			for _, lb := range loadBalancers {
+				hnsIDs = append(hnsIDs, lb.hnsID)
+			}
+			assert.ElementsMatch(t, []string{"LBID-valid"}, hnsIDs)
+		})
+	}
+}

This test verifies that load balancers with nil or empty port mappings are skipped, and only valid ones are returned.

Helper for injecting malformed load balancers into the mock:

+// PopulateRawQueriedLoadbalancer injects a load balancer into the mocked HNS state verbatim,
+// bypassing the validation CreateLoadBalancer performs.
+func (hcnObj HcnMock) PopulateRawQueriedLoadbalancer(lb *hcn.HostComputeLoadBalancer) {
+	loadbalancerMap[lb.Id] = lb
+}

This utility allows tests to directly add malformed objects for precise testing.

From the review thread

No notable review discussion on this PR.

Takeaways

• Always validate external or system-provided data before indexing or dereferencing, even if you expect a certain structure.
• When skipping malformed or unusable data, log the event at an appropriate verbosity for future troubleshooting.
• Add targeted tests for edge cases and regression scenarios, especially for error paths that could previously cause crashes.