From e3357d0c5f387e122712158285dd4cd036a91f60 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Tue, 26 Sep 2023 10:01:33 -0400 Subject: [PATCH] Clean up Endpoint / BaseEndpointInfo a bit Remove NodeName, which was unused because we only care about IsLocal which was tracked separately. Remove Zone, which was unused because it's from the old topology system? Fix up some comments which still referred to Endpoints vs EndpointSlice differences. Also remove an unhelpful helper function in endpoints_test.go --- pkg/proxy/endpoints.go | 34 ++++------------------------ pkg/proxy/endpoints_test.go | 32 ++++++++++++++++---------- pkg/proxy/endpointslicecache.go | 14 ++---------- pkg/proxy/endpointslicecache_test.go | 7 ------ pkg/proxy/types.go | 13 ++--------- pkg/proxy/winkernel/proxier.go | 15 ------------ 6 files changed, 29 insertions(+), 86 deletions(-) diff --git a/pkg/proxy/endpoints.go b/pkg/proxy/endpoints.go index 0a45c948a98..8e04bcb54e0 100644 --- a/pkg/proxy/endpoints.go +++ b/pkg/proxy/endpoints.go @@ -50,27 +50,16 @@ type BaseEndpointInfo struct { // ZoneHints represent the zone hints for the endpoint. This is based on // endpoint.hints.forZones[*].name in the EndpointSlice API. ZoneHints sets.Set[string] - // Ready indicates whether this endpoint is ready and NOT terminating. - // For pods, this is true if a pod has a ready status and a nil deletion timestamp. - // This is only set when watching EndpointSlices. If using Endpoints, this is always - // true since only ready endpoints are read from Endpoints. - // TODO: Ready can be inferred from Serving and Terminating below when enabled by default. + // Ready indicates whether this endpoint is ready and NOT terminating, unless + // PublishNotReadyAddresses is set on the service, in which case it will just + // always be true. Ready bool - // Serving indiciates whether this endpoint is ready regardless of its terminating state. + // Serving indicates whether this endpoint is ready regardless of its terminating state. // For pods this is true if it has a ready status regardless of its deletion timestamp. - // This is only set when watching EndpointSlices. If using Endpoints, this is always - // true since only ready endpoints are read from Endpoints. Serving bool // Terminating indicates whether this endpoint is terminating. // For pods this is true if it has a non-nil deletion timestamp. - // This is only set when watching EndpointSlices. If using Endpoints, this is always - // false since terminating endpoints are always excluded from Endpoints. Terminating bool - - // NodeName is the name of the node this endpoint belongs to - NodeName string - // Zone is the name of the zone this endpoint belongs to - Zone string } var _ Endpoint = &BaseEndpointInfo{} @@ -117,18 +106,7 @@ func (info *BaseEndpointInfo) Port() (int, error) { return proxyutil.PortPart(info.Endpoint) } -// GetNodeName returns the NodeName for this endpoint. -func (info *BaseEndpointInfo) GetNodeName() string { - return info.NodeName -} - -// GetZone returns the Zone for this endpoint. -func (info *BaseEndpointInfo) GetZone() string { - return info.Zone -} - -func newBaseEndpointInfo(IP, nodeName, zone string, port int, isLocal bool, - ready, serving, terminating bool, zoneHints sets.Set[string]) *BaseEndpointInfo { +func newBaseEndpointInfo(IP string, port int, isLocal, ready, serving, terminating bool, zoneHints sets.Set[string]) *BaseEndpointInfo { return &BaseEndpointInfo{ Endpoint: net.JoinHostPort(IP, strconv.Itoa(port)), IsLocal: isLocal, @@ -136,8 +114,6 @@ func newBaseEndpointInfo(IP, nodeName, zone string, port int, isLocal bool, Serving: serving, Terminating: terminating, ZoneHints: zoneHints, - NodeName: nodeName, - Zone: zone, } } diff --git a/pkg/proxy/endpoints_test.go b/pkg/proxy/endpoints_test.go index 35bfa16ca89..b21044e7c17 100644 --- a/pkg/proxy/endpoints_test.go +++ b/pkg/proxy/endpoints_test.go @@ -1654,7 +1654,11 @@ func TestCheckoutChanges(t *testing.T) { expectedChanges: []*endpointsChange{{ previous: EndpointsMap{}, current: EndpointsMap{ - svcPortName0: []Endpoint{newTestEp("10.0.1.1:80", "host1", true, true, false), newTestEp("10.0.1.2:80", "host1", false, true, true), newTestEp("10.0.1.3:80", "host1", false, false, false)}, + svcPortName0: []Endpoint{ + &BaseEndpointInfo{Endpoint: "10.0.1.1:80", Ready: true, Serving: true, Terminating: false}, + &BaseEndpointInfo{Endpoint: "10.0.1.2:80", Ready: false, Serving: true, Terminating: true}, + &BaseEndpointInfo{Endpoint: "10.0.1.3:80", Ready: false, Serving: false, Terminating: false}, + }, }, }}, appliedSlices: []*discovery.EndpointSlice{}, @@ -1666,11 +1670,23 @@ func TestCheckoutChanges(t *testing.T) { endpointsChangeTracker: NewEndpointsChangeTracker("", nil, v1.IPv4Protocol, nil, nil), expectedChanges: []*endpointsChange{{ previous: EndpointsMap{ - svcPortName0: []Endpoint{newTestEp("10.0.1.1:80", "host1", true, true, false), newTestEp("10.0.1.2:80", "host1", true, true, false), newTestEp("10.0.1.3:80", "host1", false, false, false)}, - svcPortName1: []Endpoint{newTestEp("10.0.1.1:443", "host1", true, true, false), newTestEp("10.0.1.2:443", "host1", true, true, false), newTestEp("10.0.1.3:443", "host1", false, false, false)}, + svcPortName0: []Endpoint{ + &BaseEndpointInfo{Endpoint: "10.0.1.1:80", Ready: true, Serving: true, Terminating: false}, + &BaseEndpointInfo{Endpoint: "10.0.1.2:80", Ready: true, Serving: true, Terminating: false}, + &BaseEndpointInfo{Endpoint: "10.0.1.3:80", Ready: false, Serving: false, Terminating: false}, + }, + svcPortName1: []Endpoint{ + &BaseEndpointInfo{Endpoint: "10.0.1.1:443", Ready: true, Serving: true, Terminating: false}, + &BaseEndpointInfo{Endpoint: "10.0.1.2:443", Ready: true, Serving: true, Terminating: false}, + &BaseEndpointInfo{Endpoint: "10.0.1.3:443", Ready: false, Serving: false, Terminating: false}, + }, }, current: EndpointsMap{ - svcPortName0: []Endpoint{newTestEp("10.0.1.1:80", "host1", true, true, false), newTestEp("10.0.1.2:80", "host1", true, true, false), newTestEp("10.0.1.3:80", "host1", false, false, false)}, + svcPortName0: []Endpoint{ + &BaseEndpointInfo{Endpoint: "10.0.1.1:80", Ready: true, Serving: true, Terminating: false}, + &BaseEndpointInfo{Endpoint: "10.0.1.2:80", Ready: true, Serving: true, Terminating: false}, + &BaseEndpointInfo{Endpoint: "10.0.1.3:80", Ready: false, Serving: false, Terminating: false}, + }, }, }}, appliedSlices: []*discovery.EndpointSlice{ @@ -1743,14 +1759,6 @@ func compareEndpointsMapsStr(t *testing.T, newMap EndpointsMap, expected map[Ser } } -func newTestEp(ep, host string, ready, serving, terminating bool) *BaseEndpointInfo { - endpointInfo := &BaseEndpointInfo{Endpoint: ep, Ready: ready, Serving: serving, Terminating: terminating} - if host != "" { - endpointInfo.NodeName = host - } - return endpointInfo -} - func initializeCache(endpointSliceCache *EndpointSliceCache, endpointSlices []*discovery.EndpointSlice) { for _, endpointSlice := range endpointSlices { endpointSliceCache.updatePending(endpointSlice, false) diff --git a/pkg/proxy/endpointslicecache.go b/pkg/proxy/endpointslicecache.go index ffbab9913c4..87e4305e60c 100644 --- a/pkg/proxy/endpointslicecache.go +++ b/pkg/proxy/endpointslicecache.go @@ -294,19 +294,9 @@ func (cache *EndpointSliceCache) addEndpoints(svcPortName *ServicePortName, port continue } - isLocal := false - nodeName := "" - if endpoint.NodeName != nil { - isLocal = cache.isLocal(*endpoint.NodeName) - nodeName = *endpoint.NodeName - } + isLocal := endpoint.NodeName != nil && cache.isLocal(*endpoint.NodeName) - zone := "" - if endpoint.Zone != nil { - zone = *endpoint.Zone - } - - endpointInfo := newBaseEndpointInfo(endpoint.Addresses[0], nodeName, zone, portNum, isLocal, + endpointInfo := newBaseEndpointInfo(endpoint.Addresses[0], portNum, isLocal, endpoint.Ready, endpoint.Serving, endpoint.Terminating, endpoint.ZoneHints) // This logic ensures we're deduplicating potential overlapping endpoints diff --git a/pkg/proxy/endpointslicecache_test.go b/pkg/proxy/endpointslicecache_test.go index 414d2ef60b8..3683a4f405f 100644 --- a/pkg/proxy/endpointslicecache_test.go +++ b/pkg/proxy/endpointslicecache_test.go @@ -235,7 +235,6 @@ func TestEndpointInfoByServicePort(t *testing.T) { "10.0.1.1:80": &BaseEndpointInfo{ Endpoint: "10.0.1.1:80", IsLocal: false, - NodeName: "host2", Ready: true, Serving: true, Terminating: false, @@ -243,7 +242,6 @@ func TestEndpointInfoByServicePort(t *testing.T) { "10.0.1.2:80": &BaseEndpointInfo{ Endpoint: "10.0.1.2:80", IsLocal: true, - NodeName: "host1", Ready: true, Serving: true, Terminating: false, @@ -251,7 +249,6 @@ func TestEndpointInfoByServicePort(t *testing.T) { "10.0.1.3:80": &BaseEndpointInfo{ Endpoint: "10.0.1.3:80", IsLocal: false, - NodeName: "host2", Ready: true, Serving: true, Terminating: false, @@ -271,7 +268,6 @@ func TestEndpointInfoByServicePort(t *testing.T) { "10.0.1.1:80": &BaseEndpointInfo{ Endpoint: "10.0.1.1:80", IsLocal: false, - NodeName: "host2", Ready: true, Serving: true, Terminating: false, @@ -279,7 +275,6 @@ func TestEndpointInfoByServicePort(t *testing.T) { "10.0.1.2:80": &BaseEndpointInfo{ Endpoint: "10.0.1.2:80", IsLocal: true, - NodeName: "host1", Ready: true, Serving: true, Terminating: false, @@ -287,7 +282,6 @@ func TestEndpointInfoByServicePort(t *testing.T) { "10.0.1.1:8080": &BaseEndpointInfo{ Endpoint: "10.0.1.1:8080", IsLocal: false, - NodeName: "host2", Ready: true, Serving: true, Terminating: false, @@ -295,7 +289,6 @@ func TestEndpointInfoByServicePort(t *testing.T) { "10.0.1.2:8080": &BaseEndpointInfo{ Endpoint: "10.0.1.2:8080", IsLocal: true, - NodeName: "host1", Ready: true, Serving: true, Terminating: false, diff --git a/pkg/proxy/types.go b/pkg/proxy/types.go index 4e2d23ed330..02497ca03ad 100644 --- a/pkg/proxy/types.go +++ b/pkg/proxy/types.go @@ -110,19 +110,14 @@ type Endpoint interface { String() string // GetIsLocal returns true if the endpoint is running in same host as kube-proxy, otherwise returns false. GetIsLocal() bool - // IsReady returns true if an endpoint is ready and not terminating. - // This is only set when watching EndpointSlices. If using Endpoints, this is always - // true since only ready endpoints are read from Endpoints. + // IsReady returns true if an endpoint is ready and not terminating, or + // if PublishNotReadyAddresses is set on the service. IsReady() bool // IsServing returns true if an endpoint is ready. It does not account // for terminating state. - // This is only set when watching EndpointSlices. If using Endpoints, this is always - // true since only ready endpoints are read from Endpoints. IsServing() bool // IsTerminating returns true if an endpoint is terminating. For pods, // that is any pod with a deletion timestamp. - // This is only set when watching EndpointSlices. If using Endpoints, this is always - // false since terminating endpoints are always excluded from Endpoints. IsTerminating() bool // GetZoneHints returns the zone hint for the endpoint. This is based on // endpoint.hints.forZones[0].name in the EndpointSlice API. @@ -131,10 +126,6 @@ type Endpoint interface { IP() string // Port returns the Port part of the endpoint. Port() (int, error) - // GetNodeName returns the node name for the endpoint - GetNodeName() string - // GetZone returns the zone for the endpoint - GetZone() string } // ServiceEndpoint is used to identify a service and one of its endpoint pair. diff --git a/pkg/proxy/winkernel/proxier.go b/pkg/proxy/winkernel/proxier.go index ef285e99fb8..fb080a4db2d 100644 --- a/pkg/proxy/winkernel/proxier.go +++ b/pkg/proxy/winkernel/proxier.go @@ -329,21 +329,6 @@ func (info *endpointInfo) Port() (int, error) { return int(info.port), nil } -// Equal is part of proxy.Endpoint interface. -func (info *endpointInfo) Equal(other proxy.Endpoint) bool { - return info.String() == other.String() && info.GetIsLocal() == other.GetIsLocal() -} - -// GetNodeName returns the NodeName for this endpoint. -func (info *endpointInfo) GetNodeName() string { - return "" -} - -// GetZone returns the Zone for this endpoint. -func (info *endpointInfo) GetZone() string { - return "" -} - // Uses mac prefix and IPv4 address to return a mac address // This ensures mac addresses are unique for proper load balancing // There is a possibility of MAC collisions but this Mac address is used for remote endpoints only