Merge pull request #131427 from princepereira/automated-cherry-pick-of-#131138-upstream-release-1.33

Automated cherry pick of #131138: Fix for HNS local endpoint was being deleted instead of the remote endpoint.

pkg/proxy/winkernel/hns.go

@@ -133,40 +133,43 @@ func (hns hns) getAllEndpointsByNetwork(networkName string) (map[string]*(endpoi
 			continue
 		}
 
-		// Add to map with key endpoint ID or IP address
+		for index, ipConfig := range ep.IpConfigurations {
-		// Storing this is expensive in terms of memory, however there is a bug in Windows Server 2019 that can cause two endpoints to be created with the same IP address.
-		// TODO: Store by IP only and remove any lookups by endpoint ID.
-		endpointInfos[ep.Id] = &endpointInfo{
-			ip:         ep.IpConfigurations[0].IpAddress,
-			isLocal:    uint32(ep.Flags&hcn.EndpointFlagsRemoteEndpoint) == 0,
-			macAddress: ep.MacAddress,
-			hnsID:      ep.Id,
-			hns:        hns,
-			// only ready and not terminating endpoints were added to HNS
-			ready:       true,
-			serving:     true,
-			terminating: false,
-		}
-		endpointInfos[ep.IpConfigurations[0].IpAddress] = endpointInfos[ep.Id]
 
-		if len(ep.IpConfigurations) == 1 {
+			if index > 1 {
-			continue
+				// Expecting only ipv4 and ipv6 ipaddresses
-		}
+				// This is highly unlikely to happen, but if it does, we should log a warning
+				// and break out of the loop
+				klog.Warning("Endpoint ipconfiguration holds more than 2 IP addresses.", "hnsID", ep.Id, "IP", ipConfig.IpAddress, "ipConfigCount", len(ep.IpConfigurations))
+				break
+			}
 
-		// If ipFamilyPolicy is RequireDualStack or PreferDualStack, then there will be 2 IPS (iPV4 and IPV6)
+			isLocal := uint32(ep.Flags&hcn.EndpointFlagsRemoteEndpoint) == 0
-		// in the endpoint list
+
-		endpointDualstack := &endpointInfo{
+			if existingEp, ok := endpointInfos[ipConfig.IpAddress]; ok && isLocal {
-			ip:         ep.IpConfigurations[1].IpAddress,
+				// If the endpoint is already part of the queried endpoints map and is local,
-			isLocal:    uint32(ep.Flags&hcn.EndpointFlagsRemoteEndpoint) == 0,
+				// then we should not add it again to the map
-			macAddress: ep.MacAddress,
+				// This is to avoid overwriting the remote endpoint info with a local endpoint.
-			hnsID:      ep.Id,
+				klog.V(3).InfoS("Endpoint already exists in queried endpoints map; skipping.", "newLocalEndpoint", ep, "ipConfig", ipConfig, "existingEndpoint", existingEp)
-			hns:        hns,
+				continue
-			// only ready and not terminating endpoints were added to HNS
+			}
-			ready:       true,
+
-			serving:     true,
+			// Add to map with key endpoint ID or IP address
-			terminating: false,
+			// Storing this is expensive in terms of memory, however there is a bug in Windows Server 2019 and 2022 that can cause two endpoints (local and remote) to be created with the same IP address.
+			// TODO: Store by IP only and remove any lookups by endpoint ID.
+			epInfo := &endpointInfo{
+				ip:         ipConfig.IpAddress,
+				isLocal:    isLocal,
+				macAddress: ep.MacAddress,
+				hnsID:      ep.Id,
+				hns:        hns,
+				// only ready and not terminating endpoints were added to HNS
+				ready:       true,
+				serving:     true,
+				terminating: false,
+			}
+			endpointInfos[ep.Id] = epInfo
+			endpointInfos[ipConfig.IpAddress] = epInfo
 		}
-		endpointInfos[ep.IpConfigurations[1].IpAddress] = endpointDualstack
 	}
 	klog.V(3).InfoS("Queried endpoints from network", "network", networkName, "count", len(endpointInfos))
 	klog.V(5).InfoS("Queried endpoints details", "network", networkName, "endpointInfos", endpointInfos)

pkg/proxy/winkernel/hns_test.go

@@ -112,6 +112,70 @@ func TestGetAllEndpointsByNetwork(t *testing.T) {
 	}
 }
 
+func TestGetAllEndpointsByNetworkWithDupEP(t *testing.T) {
+	hcnMock := getHcnMock("L2Bridge")
+	hns := hns{hcn: hcnMock}
+
+	ipv4Config := &hcn.IpConfig{
+		IpAddress: epIpAddress,
+	}
+	ipv6Config := &hcn.IpConfig{
+		IpAddress: epIpv6Address,
+	}
+	remoteEndpoint := &hcn.HostComputeEndpoint{
+		IpConfigurations: []hcn.IpConfig{*ipv4Config, *ipv6Config},
+		MacAddress:       epMacAddress,
+		SchemaVersion: hcn.SchemaVersion{
+			Major: 2,
+			Minor: 0,
+		},
+		Flags: hcn.EndpointFlagsRemoteEndpoint,
+	}
+	Network, _ := hcnMock.GetNetworkByName(testNetwork)
+	remoteEndpoint, err := hns.hcn.CreateEndpoint(Network, remoteEndpoint)
+	if err != nil {
+		t.Error(err)
+	}
+
+	// Create a duplicate local endpoint with the same IP address
+	dupLocalEndpoint := &hcn.HostComputeEndpoint{
+		IpConfigurations: []hcn.IpConfig{*ipv4Config, *ipv6Config},
+		MacAddress:       epMacAddress,
+		SchemaVersion: hcn.SchemaVersion{
+			Major: 2,
+			Minor: 0,
+		},
+	}
+
+	dupLocalEndpoint, err = hns.hcn.CreateEndpoint(Network, dupLocalEndpoint)
+	if err != nil {
+		t.Error(err)
+	}
+
+	mapEndpointsInfo, err := hns.getAllEndpointsByNetwork(Network.Name)
+	if err != nil {
+		t.Error(err)
+	}
+	endpointIpv4, ipv4EpPresent := mapEndpointsInfo[ipv4Config.IpAddress]
+	assert.True(t, ipv4EpPresent, "IPV4 endpoint is missing in Dualstack mode")
+	assert.Equal(t, endpointIpv4.ip, epIpAddress, "IPV4 IP is missing in Dualstack mode")
+	assert.Equal(t, endpointIpv4.hnsID, remoteEndpoint.Id, "HNS ID is not matching with remote endpoint")
+
+	endpointIpv6, ipv6EpPresent := mapEndpointsInfo[ipv6Config.IpAddress]
+	assert.True(t, ipv6EpPresent, "IPV6 endpoint is missing in Dualstack mode")
+	assert.Equal(t, endpointIpv6.ip, epIpv6Address, "IPV6 IP is missing in Dualstack mode")
+	assert.Equal(t, endpointIpv6.hnsID, remoteEndpoint.Id, "HNS ID is not matching with remote endpoint")
+
+	err = hns.hcn.DeleteEndpoint(remoteEndpoint)
+	if err != nil {
+		t.Error(err)
+	}
+	err = hns.hcn.DeleteEndpoint(dupLocalEndpoint)
+	if err != nil {
+		t.Error(err)
+	}
+}
+
 func TestGetEndpointByID(t *testing.T) {
 	// TODO: remove skip once the test has been fixed.
 	t.Skip("Skipping failing test on Windows.")

pkg/proxy/winkernel/proxier.go

@@ -535,6 +535,11 @@ func (ep *endpointInfo) DecrementRefCount() {
 	if !ep.IsLocal() && ep.refCount != nil && *ep.refCount > 0 {
 		*ep.refCount--
 	}
+	refCount := 0
+	if ep.refCount != nil {
+		refCount = int(*ep.refCount)
+	}
+	klog.V(5).InfoS("Endpoint RefCount after decrement.", "endpointInfo", ep, "refCount", refCount)
 }
 
 func (ep *endpointInfo) Cleanup() {
@@ -1770,10 +1775,14 @@ func (proxier *Proxier) syncProxyRules() {
 
 	// remove stale endpoint refcount entries
 	for epIP := range proxier.terminatedEndpoints {
-		if epToDelete := queriedEndpoints[epIP]; epToDelete != nil && epToDelete.hnsID != "" {
+		klog.V(5).InfoS("Terminated endpoints ready for deletion", "epIP", epIP)
+		if epToDelete := queriedEndpoints[epIP]; epToDelete != nil && epToDelete.hnsID != "" && !epToDelete.IsLocal() {
 			if refCount := proxier.endPointsRefCount.getRefCount(epToDelete.hnsID); refCount == nil || *refCount == 0 {
-				klog.V(3).InfoS("Deleting unreferenced remote endpoint", "hnsID", epToDelete.hnsID)
+				klog.V(3).InfoS("Deleting unreferenced remote endpoint", "hnsID", epToDelete.hnsID, "IP", epToDelete.ip)
-				proxier.hns.deleteEndpoint(epToDelete.hnsID)
+				err := proxier.hns.deleteEndpoint(epToDelete.hnsID)
+				if err != nil {
+					klog.ErrorS(err, "Deleting unreferenced remote endpoint failed", "hnsID", epToDelete.hnsID)
+				}
 			}
 		}
 	}