diff --git a/pkg/proxy/winkernel/hns.go b/pkg/proxy/winkernel/hns.go index 6c9aaa85790..392cd79cf99 100644 --- a/pkg/proxy/winkernel/hns.go +++ b/pkg/proxy/winkernel/hns.go @@ -32,7 +32,10 @@ import ( type HostNetworkService interface { getNetworkByName(name string) (*hnsNetworkInfo, error) - getAllEndpointsByNetwork(networkName string) (map[string]*endpointInfo, error) + // Returns a map of endpoints keyed by both endpoint ID and IP address for all endpoints on the specified network, and a map of remote endpoints with duplicate IPs to be deleted. + getAllEndpointsByNetwork(networkName string) (map[string]*endpointInfo, map[string]bool, error) + // deleteAllRemoteEndpointsWithDupIP deletes all remote endpoints with duplicate IPs that were found in getAllEndpointsByNetwork. This is needed to clean up stale remote endpoints that can be left behind due to a Windows bug. + deleteAllRemoteEndpointsWithDupIP(remoteEPsWithDupIP map[string]bool) getEndpointByID(id string) (*endpointInfo, error) getEndpointByIpAddress(ip string, networkName string) (*endpointInfo, error) getEndpointByName(id string) (*endpointInfo, error) @@ -115,17 +118,20 @@ func (hns hns) getNetworkByName(name string) (*hnsNetworkInfo, error) { }, nil } -func (hns hns) getAllEndpointsByNetwork(networkName string) (map[string]*(endpointInfo), error) { +func (hns hns) getAllEndpointsByNetwork(networkName string) (map[string]*(endpointInfo), map[string]bool, error) { hcnnetwork, err := hns.hcn.GetNetworkByName(networkName) if err != nil { klog.ErrorS(err, "failed to get HNS network by name", "name", networkName) - return nil, err + return nil, nil, err } endpoints, err := hns.hcn.ListEndpointsOfNetwork(hcnnetwork.Id) if err != nil { - return nil, fmt.Errorf("failed to list endpoints: %w", err) + return nil, nil, fmt.Errorf("failed to list endpoints: %w", err) } + endpointInfos := make(map[string]*(endpointInfo)) + remoteEPsWithDupIP := make(map[string]bool) + for _, ep := range endpoints { if len(ep.IpConfigurations) == 0 { @@ -143,14 +149,22 @@ func (hns hns) getAllEndpointsByNetwork(networkName string) (map[string]*(endpoi break } - isLocal := uint32(ep.Flags&hcn.EndpointFlagsRemoteEndpoint) == 0 + curEpIsLocal := uint32(ep.Flags&hcn.EndpointFlagsRemoteEndpoint) == 0 - if existingEp, ok := endpointInfos[ipConfig.IpAddress]; ok && isLocal { - // If the endpoint is already part of the queried endpoints map and is local, - // then we should not add it again to the map - // This is to avoid overwriting the remote endpoint info with a local endpoint. - klog.V(3).InfoS("Endpoint already exists in queried endpoints map; skipping.", "newLocalEndpoint", ep, "ipConfig", ipConfig, "existingEndpoint", existingEp) - continue + if existingEp, ok := endpointInfos[ipConfig.IpAddress]; ok { + if curEpIsLocal && !existingEp.isLocal { + // Local found, stale remote in map → delete remote from HNS, overwrite + remoteEPsWithDupIP[existingEp.hnsID] = true + delete(endpointInfos, existingEp.hnsID) + delete(endpointInfos, existingEp.ip) + // fall through to add local + } else if !curEpIsLocal && existingEp.isLocal { + // Local already in map, remote arriving → delete remote from HNS, skip + remoteEPsWithDupIP[ep.Id] = true + continue + } else { + continue // same type, keep existing + } } // Add to map with key endpoint ID or IP address @@ -158,7 +172,7 @@ func (hns hns) getAllEndpointsByNetwork(networkName string) (map[string]*(endpoi // TODO: Store by IP only and remove any lookups by endpoint ID. epInfo := &endpointInfo{ ip: ipConfig.IpAddress, - isLocal: isLocal, + isLocal: curEpIsLocal, macAddress: ep.MacAddress, hnsID: ep.Id, hns: hns, @@ -173,7 +187,17 @@ func (hns hns) getAllEndpointsByNetwork(networkName string) (map[string]*(endpoi } klog.V(3).InfoS("Queried endpoints from network", "network", networkName, "count", len(endpointInfos)) klog.V(5).InfoS("Queried endpoints details", "network", networkName, "endpointInfos", endpointInfos) - return endpointInfos, nil + return endpointInfos, remoteEPsWithDupIP, nil +} + +func (hns hns) deleteAllRemoteEndpointsWithDupIP(remoteEPsWithDupIP map[string]bool) { + for hnsID := range remoteEPsWithDupIP { + klog.V(3).InfoS("Deleting stale remote endpoint with duplicate IP", "hnsID", hnsID) + err := hns.deleteEndpoint(hnsID) + if err != nil { + klog.ErrorS(err, "Failed to delete stale remote endpoint with duplicate IP", "hnsID", hnsID) + } + } } func (hns hns) getEndpointByID(id string) (*endpointInfo, error) { diff --git a/pkg/proxy/winkernel/hns_test.go b/pkg/proxy/winkernel/hns_test.go index 604eec74443..ec504ab906b 100644 --- a/pkg/proxy/winkernel/hns_test.go +++ b/pkg/proxy/winkernel/hns_test.go @@ -95,7 +95,7 @@ func TestGetAllEndpointsByNetwork(t *testing.T) { t.Error(err) } - mapEndpointsInfo, err := hns.getAllEndpointsByNetwork(Network.Name) + mapEndpointsInfo, _, err := hns.getAllEndpointsByNetwork(Network.Name) if err != nil { t.Error(err) } @@ -157,24 +157,25 @@ func TestGetAllEndpointsByNetworkWithDupEP(t *testing.T) { t.Error(err) } - mapEndpointsInfo, err := hns.getAllEndpointsByNetwork(Network.Name) + mapEndpointsInfo, remoteEPsWithDupIP, err := hns.getAllEndpointsByNetwork(Network.Name) if err != nil { t.Error(err) } + hns.deleteAllRemoteEndpointsWithDupIP(remoteEPsWithDupIP) 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") + assert.Equal(t, endpointIpv4.hnsID, dupLocalEndpoint.Id, "HNS ID is not matching with local 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") + assert.Equal(t, endpointIpv6.hnsID, dupLocalEndpoint.Id, "HNS ID is not matching with local endpoint") - err = hns.hcn.DeleteEndpoint(remoteEndpoint) - if err != nil { - t.Error(err) - } + remoteEpExists, _ := hns.hcn.GetEndpointByID(remoteEndpoint.Id) + assert.Nil(t, remoteEpExists, "Remote endpoint with duplicate IP should have been deleted") + + // Clean up the duplicate local endpoint err = hns.hcn.DeleteEndpoint(dupLocalEndpoint) if err != nil { t.Error(err) diff --git a/pkg/proxy/winkernel/proxier.go b/pkg/proxy/winkernel/proxier.go index 46ed064b62f..4cb12eda389 100644 --- a/pkg/proxy/winkernel/proxier.go +++ b/pkg/proxy/winkernel/proxier.go @@ -184,7 +184,8 @@ type remoteSubnetInfo struct { } const ( - NETWORK_TYPE_OVERLAY = "overlay" + NETWORK_TYPE_OVERLAY = "overlay" + NETWORK_TYPE_L2BRIDGE = "L2Bridge" // MAX_COUNT_STALE_LOADBALANCERS is the maximum number of stale loadbalancers which cleanedup in single syncproxyrules. // If there are more stale loadbalancers to clean, it will go to next iteration of syncproxyrules. MAX_COUNT_STALE_LOADBALANCERS = 20 @@ -1243,7 +1244,9 @@ func (proxier *Proxier) syncProxyRules() (retryError error) { _ = proxier.endpointsMap.Update(proxier.endpointsChanges) // Query HNS for endpoints and load balancers - queriedEndpoints, err := hns.getAllEndpointsByNetwork(hnsNetworkName) + queriedEndpoints, remoteEPsWithDupIP, err := hns.getAllEndpointsByNetwork(hnsNetworkName) + defer hns.deleteAllRemoteEndpointsWithDupIP(remoteEPsWithDupIP) + if err != nil { klog.ErrorS(err, "Querying HNS for endpoints failed") return @@ -1788,25 +1791,32 @@ func (proxier *Proxier) syncProxyRules() (retryError error) { } // remove stale endpoint refcount entries - for epIP := range proxier.terminatedEndpoints { - 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 { - err := proxier.hns.deleteEndpoint(epToDelete.hnsID) - if err != nil { - klog.ErrorS(err, "Deleting unreferenced remote endpoint failed", "hnsID", epToDelete.hnsID) - } else { - klog.V(3).InfoS("Deleting unreferenced remote endpoint succeeded", "hnsID", epToDelete.hnsID, "IP", epToDelete.ip) - } - } - } - } + proxier.deleteTerminatedEndpoints(queriedEndpoints) + // This will cleanup stale load balancers which are pending delete // in last iteration proxier.cleanupStaleLoadbalancers() return } +func (proxier *Proxier) deleteTerminatedEndpoints(queriedEndpoints map[string]*(endpointInfo)) { + for epIP := range proxier.terminatedEndpoints { + klog.V(5).InfoS("Terminated endpoints ready for deletion", "epIP", epIP) + if epToDelete := queriedEndpoints[epIP]; epToDelete != nil && epToDelete.hnsID != "" && !epToDelete.IsLocal() { + refCount := proxier.endPointsRefCount.getRefCount(epToDelete.hnsID) + if refCount == nil || *refCount == 0 { + if err := proxier.hns.deleteEndpoint(epToDelete.hnsID); err != nil { + klog.ErrorS(err, "Deleting unreferenced remote endpoint failed", "hnsID", epToDelete.hnsID) + } else { + klog.V(3).InfoS("Deleting unreferenced remote endpoint succeeded", "hnsID", epToDelete.hnsID, "IP", epToDelete.ip) + } + } else { + klog.V(3).InfoS("Not deleting remote endpoint as it is still referenced", "hnsID", epToDelete.hnsID, "IP", epToDelete.ip, "refCount", refCount) + } + } + } +} + // deleteExistingLoadBalancer checks whether loadbalancer delete is needed or not. // If it is needed, the function will delete the existing loadbalancer and return true, else false. func (proxier *Proxier) deleteExistingLoadBalancer(hns HostNetworkService, winProxyOptimization bool, lbHnsID *string, vip string, protocol, intPort, extPort uint16, endpoints []endpointInfo, queriedLoadBalancers map[loadBalancerIdentifier]*loadBalancerInfo) bool { diff --git a/pkg/proxy/winkernel/testing/hcnutils_mock.go b/pkg/proxy/winkernel/testing/hcnutils_mock.go index bea3eb17539..85a73884abf 100644 --- a/pkg/proxy/winkernel/testing/hcnutils_mock.go +++ b/pkg/proxy/winkernel/testing/hcnutils_mock.go @@ -34,8 +34,9 @@ var ( ) type HcnMock struct { - supportedFeatures hcn.SupportedFeatures - network *hcn.HostComputeNetwork + supportedFeatures hcn.SupportedFeatures + network *hcn.HostComputeNetwork + ShouldFailDeleteLoadBalancer bool } func (hcnObj HcnMock) generateEndpointGuid() (endpointId string, endpointName string) { @@ -217,6 +218,9 @@ func (hcnObj HcnMock) DeleteLoadBalancer(loadBalancer *hcn.HostComputeLoadBalanc if _, ok := loadbalancerMap[loadBalancer.Id]; !ok { return hcn.LoadBalancerNotFoundError{LoadBalancerId: loadBalancer.Id} } + if hcnObj.ShouldFailDeleteLoadBalancer { + return fmt.Errorf("injected DeleteLoadBalancer failure for %s", loadBalancer.Id) + } delete(loadbalancerMap, loadBalancer.Id) return nil }