diff --git a/pkg/proxy/winkernel/hns.go b/pkg/proxy/winkernel/hns.go index 13f1ea868fb..d6e425f8b77 100644 --- a/pkg/proxy/winkernel/hns.go +++ b/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 - // 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] + for index, ipConfig := range ep.IpConfigurations { - if len(ep.IpConfigurations) == 1 { - continue - } + if index > 1 { + // 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) - // in the endpoint list - endpointDualstack := &endpointInfo{ - ip: ep.IpConfigurations[1].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, + isLocal := 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 + } + + // Add to map with key endpoint ID or IP address + // 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) diff --git a/pkg/proxy/winkernel/hns_test.go b/pkg/proxy/winkernel/hns_test.go index 74a51b68f31..ca436394b01 100644 --- a/pkg/proxy/winkernel/hns_test.go +++ b/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.") diff --git a/pkg/proxy/winkernel/proxier.go b/pkg/proxy/winkernel/proxier.go index d3504964200..91fb6fad8d4 100644 --- a/pkg/proxy/winkernel/proxier.go +++ b/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) - proxier.hns.deleteEndpoint(epToDelete.hnsID) + klog.V(3).InfoS("Deleting unreferenced remote endpoint", "hnsID", epToDelete.hnsID, "IP", epToDelete.ip) + err := proxier.hns.deleteEndpoint(epToDelete.hnsID) + if err != nil { + klog.ErrorS(err, "Deleting unreferenced remote endpoint failed", "hnsID", epToDelete.hnsID) + } } } }