From 27a70d4dcd2c8cc6b6fa0a604fbeb8a589981b57 Mon Sep 17 00:00:00 2001 From: mmamczur Date: Tue, 28 Feb 2023 15:01:13 +0100 Subject: [PATCH] Fix NEG endpoint attachment for L4 NEGs. The L4 EndpointCalculators do not compute pod endpoints data. It always failed checks in CheckEndpointInfo but the failure was ignored. Now the code returns upon failure after changes from ff4751f08645101e5c0fc60857726d59d4a9d25a. This change skips the checks for calculators that don't return the pods data as these checks compare it with endpoints from slices. --- pkg/neg/syncers/endpoints_calculator.go | 19 ++++++++++++++++ pkg/neg/syncers/endpoints_calculator_test.go | 23 ++++++++++++++++++++ pkg/neg/syncers/transaction.go | 9 +++++--- pkg/neg/types/interfaces.go | 3 +++ 4 files changed, 51 insertions(+), 3 deletions(-) diff --git a/pkg/neg/syncers/endpoints_calculator.go b/pkg/neg/syncers/endpoints_calculator.go index 4025f892f7..7678d1572c 100644 --- a/pkg/neg/syncers/endpoints_calculator.go +++ b/pkg/neg/syncers/endpoints_calculator.go @@ -109,6 +109,12 @@ func (l *LocalL4ILBEndpointsCalculator) CalculateEndpoints(eds []types.Endpoints return subsetMap, nil, 0, err } +// UsesPodEndpoints Indicates if the calculator is calculating pod endpoints in addition to network endpoints. +// It returns false for LocalL4ILBEndpointsCalculator since it does not calculate the pod endpoint map. +func (l *LocalL4ILBEndpointsCalculator) UsesPodEndpoints() bool { + return false +} + // ClusterL4ILBEndpointGetter implements the NetworkEndpointsCalculator interface. // It exposes methods to calculate Network endpoints for GCE_VM_IP NEGs when the service // uses "ExternalTrafficPolicy: Cluster" mode This is the default mode. @@ -161,6 +167,13 @@ func (l *ClusterL4ILBEndpointsCalculator) CalculateEndpoints(_ []types.Endpoints return subsetMap, nil, 0, err } +// UsesPodEndpoints Indicates if the calculator is calculating pod endpoints in addition to network endpoints. +// It returns false for ClusterL4ILBEndpointsCalculator since it does not calculate the pod endpoint map. +// ClusterL4ILBEndpointsCalculator does not even look at pod data from EndpointSlices in fact. +func (l *ClusterL4ILBEndpointsCalculator) UsesPodEndpoints() bool { + return false +} + // L7EndpointsCalculator implements methods to calculate Network endpoints for VM_IP_PORT NEGs type L7EndpointsCalculator struct { zoneGetter types.ZoneGetter @@ -190,6 +203,12 @@ func (l *L7EndpointsCalculator) CalculateEndpoints(eds []types.EndpointsData, _ return toZoneNetworkEndpointMap(eds, l.zoneGetter, l.servicePortName, l.networkEndpointType) } +// UsesPodEndpoints Indicates if the calculator is calculating pod endpoints in addition to network endpoints. +// It returns true for L7 since it calculates the pod endpoint map. +func (l *L7EndpointsCalculator) UsesPodEndpoints() bool { + return true +} + func nodeMapToString(nodeMap map[string][]*v1.Node) string { var str []string for zone, nodeList := range nodeMap { diff --git a/pkg/neg/syncers/endpoints_calculator_test.go b/pkg/neg/syncers/endpoints_calculator_test.go index 6a8c6a5e37..b6b0310828 100644 --- a/pkg/neg/syncers/endpoints_calculator_test.go +++ b/pkg/neg/syncers/endpoints_calculator_test.go @@ -207,6 +207,29 @@ func TestClusterGetEndpointSet(t *testing.T) { } } +func TestClusterL4EndpointsCalculatorUsesPodEndpoints(t *testing.T) { + ec := &ClusterL4ILBEndpointsCalculator{} + + if ec.UsesPodEndpoints() { + t.Errorf("ClusterL4EndpointsCalculator should not use PodEndpoints but UsesPodEndpoints() returned true") + } +} + +func TestLocalL4EndpointsCalculatorUsesPodEndpoints(t *testing.T) { + ec := &LocalL4ILBEndpointsCalculator{} + + if ec.UsesPodEndpoints() { + t.Errorf("LocalL4ILBEndpointsCalculator should not use PodEndpoints but UsesPodEndpoints() returned true") + } +} + +func TestL7EndpointsCalculatorUsesPodEndpoints(t *testing.T) { + ec := &L7EndpointsCalculator{} + + if !ec.UsesPodEndpoints() { + t.Errorf("L7EndpointsCalculator should use PodEndpoints but UsesPodEndpoints() returned false") + } +} func createNodes(t *testing.T, nodeNames []string, nodeLabels map[string]map[string]string, nodeReadyStatus map[string]v1.ConditionStatus, nodeIndexer cache.Indexer) { t.Helper() for i, nodeName := range nodeNames { diff --git a/pkg/neg/syncers/transaction.go b/pkg/neg/syncers/transaction.go index 6b96e53e86..52ac30ec72 100644 --- a/pkg/neg/syncers/transaction.go +++ b/pkg/neg/syncers/transaction.go @@ -266,9 +266,12 @@ func (s *transactionSyncer) syncInternalImpl() *negtypes.NegSyncResult { s.setErrorState(result.Result) return result } - if valid, result := s.CheckEndpointInfo(endpointsData, endpointPodMap, dupCount); !valid { - s.setErrorState(result.Result) - return result + // skip CheckEndpointInfo for calculators that don't return endpointPodMap data. + if s.endpointsCalculator.UsesPodEndpoints() { + if valid, result := s.CheckEndpointInfo(endpointsData, endpointPodMap, dupCount); !valid { + s.setErrorState(result.Result) + return result + } } if err != nil { return negtypes.NewNegSyncResult(fmt.Errorf("endpoints calculation error in mode %q, err: %w", s.endpointsCalculator.Mode(), err), negtypes.ResultOtherError) diff --git a/pkg/neg/types/interfaces.go b/pkg/neg/types/interfaces.go index 054b3dd9d1..34a40a6064 100644 --- a/pkg/neg/types/interfaces.go +++ b/pkg/neg/types/interfaces.go @@ -86,4 +86,7 @@ type NetworkEndpointsCalculator interface { CalculateEndpoints(eds []EndpointsData, currentMap map[string]NetworkEndpointSet) (map[string]NetworkEndpointSet, EndpointPodMap, int, error) // Mode indicates the mode that the EndpointsCalculator is operating in. Mode() EndpointsCalculatorMode + // UsesPodEndpoints Indicates if the calculator is calculating pod endpoints in addition to network endpoints. + // If this function returns false then EndpointPodMap returned from CalculateEndpoints will be empty. + UsesPodEndpoints() bool }