From f7eed43a1a0acf1b2d725c7f10cc4916fc0bc16d Mon Sep 17 00:00:00 2001 From: David Cheung Date: Wed, 1 Mar 2023 23:31:34 +0000 Subject: [PATCH 1/3] remove duplicate error in utils.go --- pkg/neg/syncers/transaction.go | 4 ++-- pkg/neg/syncers/utils.go | 13 +++---------- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/pkg/neg/syncers/transaction.go b/pkg/neg/syncers/transaction.go index 0140732ee2..814341ae3e 100644 --- a/pkg/neg/syncers/transaction.go +++ b/pkg/neg/syncers/transaction.go @@ -388,11 +388,11 @@ func (s *transactionSyncer) isValidEndpointInfo(eds []negtypes.EndpointsData, en // isValidEPField returns false and the corresponding reason if there is endpoint with missing zone or nodeName func (s *transactionSyncer) isValidEPField(err error) (bool, string) { - if errors.Is(err, ErrEPMissingNodeName) { + if errors.Is(err, negtypes.ErrEPMissingNodeName) { s.logger.Info("Detected unexpected error when checking missing nodeName", "error", err) return false, negtypes.ResultEPMissingNodeName } - if errors.Is(err, ErrEPMissingZone) { + if errors.Is(err, negtypes.ErrEPMissingZone) { s.logger.Info("Detected unexpected error when checking missing zone", "error", err) return false, negtypes.ResultEPMissingZone } diff --git a/pkg/neg/syncers/utils.go b/pkg/neg/syncers/utils.go index 8b9025fea7..e973feae39 100644 --- a/pkg/neg/syncers/utils.go +++ b/pkg/neg/syncers/utils.go @@ -17,7 +17,6 @@ limitations under the License. package syncers import ( - "errors" "fmt" "strconv" "strings" @@ -46,12 +45,6 @@ const ( separator = "||" ) -var ( - ErrEPMissingNodeName = errors.New("endpoint has empty nodeName field") - ErrNodeNotFound = errors.New("failed to retrieve associated zone of node") - ErrEPMissingZone = errors.New("endpoint has empty zone field") -) - // encodeEndpoint encodes ip and instance into a single string func encodeEndpoint(ip, instance, port string) string { return strings.Join([]string{ip, instance, port}, separator) @@ -253,7 +246,7 @@ func toZoneNetworkEndpointMap(eds []negtypes.EndpointsData, zoneGetter negtypes. for _, endpointAddress := range ed.Addresses { if endpointAddress.NodeName == nil || len(*endpointAddress.NodeName) == 0 { klog.V(2).Infof("Endpoint %q in Endpoints %s/%s does not have an associated node. Skipping", endpointAddress.Addresses, ed.Meta.Namespace, ed.Meta.Name) - return nil, nil, dupCount, ErrEPMissingNodeName + return nil, nil, dupCount, negtypes.ErrEPMissingNodeName } if endpointAddress.TargetRef == nil { klog.V(2).Infof("Endpoint %q in Endpoints %s/%s does not have an associated pod. Skipping", endpointAddress.Addresses, ed.Meta.Namespace, ed.Meta.Name) @@ -261,10 +254,10 @@ func toZoneNetworkEndpointMap(eds []negtypes.EndpointsData, zoneGetter negtypes. } zone, err := zoneGetter.GetZoneForNode(*endpointAddress.NodeName) if err != nil { - return nil, nil, dupCount, ErrNodeNotFound + return nil, nil, dupCount, negtypes.ErrNodeNotFound } if zone == "" { - return nil, nil, dupCount, ErrEPMissingZone + return nil, nil, dupCount, negtypes.ErrEPMissingZone } if zoneNetworkEndpointMap[zone] == nil { zoneNetworkEndpointMap[zone] = negtypes.NewNetworkEndpointSet() From 7cc18587270fee8e1422500261cce2f83058ca69 Mon Sep 17 00:00:00 2001 From: David Cheung Date: Thu, 2 Mar 2023 18:48:42 +0000 Subject: [PATCH 2/3] create ValidateEndpoints for L4 and L7 --- pkg/neg/syncers/endpoints_calculator.go | 41 ++ pkg/neg/syncers/endpoints_calculator_test.go | 609 +++++++++++++++++++ pkg/neg/types/interfaces.go | 2 + 3 files changed, 652 insertions(+) diff --git a/pkg/neg/syncers/endpoints_calculator.go b/pkg/neg/syncers/endpoints_calculator.go index 4025f892f7..afdaf495b7 100644 --- a/pkg/neg/syncers/endpoints_calculator.go +++ b/pkg/neg/syncers/endpoints_calculator.go @@ -109,6 +109,11 @@ func (l *LocalL4ILBEndpointsCalculator) CalculateEndpoints(eds []types.Endpoints return subsetMap, nil, 0, err } +func (l *LocalL4ILBEndpointsCalculator) ValidateEndpoints(endpointData []types.EndpointsData, endpointPodMap types.EndpointPodMap, dupCount int) error { + // this should be a no-op for now + return nil +} + // 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 +166,11 @@ func (l *ClusterL4ILBEndpointsCalculator) CalculateEndpoints(_ []types.Endpoints return subsetMap, nil, 0, err } +func (l *ClusterL4ILBEndpointsCalculator) ValidateEndpoints(endpointData []types.EndpointsData, endpointPodMap types.EndpointPodMap, dupCount int) error { + // this should be a no-op for now + return nil +} + // L7EndpointsCalculator implements methods to calculate Network endpoints for VM_IP_PORT NEGs type L7EndpointsCalculator struct { zoneGetter types.ZoneGetter @@ -197,3 +207,34 @@ func nodeMapToString(nodeMap map[string][]*v1.Node) string { } return strings.Join(str, ",") } + +// ValidateEndpoints checks if endpoint information is correct. +// +// For L7 Endpoint Calculator, it returns error if one of the two checks fails: +// 1. The endpoint count from endpointData doesn't equal to the one from endpointPodMap: +// endpiontPodMap removes the duplicated endpoints, and dupCount stores the number of duplicated it removed +// and we compare the endpoint counts with duplicates +// 2. The endpoint count from endpointData or the one from endpointPodMap is 0 +func (l *L7EndpointsCalculator) ValidateEndpoints(endpointData []types.EndpointsData, endpointPodMap types.EndpointPodMap, dupCount int) error { + // Endpoint count from EndpointPodMap + countFromPodMap := len(endpointPodMap) + dupCount + if countFromPodMap == 0 { + l.logger.Info("Detected endpoint count from endpointPodMap going to zero", "endpointPodMap", endpointPodMap) + return types.ErrEPCalculationCountZero + } + // Endpoint count from EndpointData + countFromEndpointData := 0 + for _, ed := range endpointData { + countFromEndpointData += len(ed.Addresses) + } + if countFromEndpointData == 0 { + l.logger.Info("Detected endpoint count from endpointData going to zero", "endpointData", endpointData) + return types.ErrEPSEndpointCountZero + } + + if countFromEndpointData != countFromPodMap { + l.logger.Info("Detected error when comparing endpoint counts", "endpointData", endpointData, "endpointPodMap", endpointPodMap, "dupCount", dupCount) + return types.ErrEPCountsDiffer + } + return nil +} diff --git a/pkg/neg/syncers/endpoints_calculator_test.go b/pkg/neg/syncers/endpoints_calculator_test.go index 6a8c6a5e37..d1cf853950 100644 --- a/pkg/neg/syncers/endpoints_calculator_test.go +++ b/pkg/neg/syncers/endpoints_calculator_test.go @@ -17,12 +17,14 @@ limitations under the License. package syncers import ( + "errors" "fmt" "reflect" "testing" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" negtypes "k8s.io/ingress-gce/pkg/neg/types" @@ -207,6 +209,613 @@ func TestClusterGetEndpointSet(t *testing.T) { } } +func TestValidateEndpoints(t *testing.T) { + t.Parallel() + + instance1 := testInstance1 + instance2 := testInstance2 + instance4 := testInstance4 + testPortName := "" + testServiceNamespace := "namespace" + port80 := int32(80) + port81 := int32(81) + testIP1 := "10.100.1.1" + testIP2 := "10.100.1.2" + testIP3 := "10.100.2.2" + testIP4 := "10.100.4.1" + testPodName1 := "pod1" + testPodName2 := "pod2" + testPodName3 := "pod3" + testPodName4 := "pod4" + svcKey := fmt.Sprintf("%s/%s", testServiceName, testServiceNamespace) + + zoneGetter := negtypes.NewFakeZoneGetter() + testContext := negtypes.NewTestContext() + podLister := testContext.PodInformer.GetIndexer() + nodeLister := listers.NewNodeLister(testContext.NodeInformer.GetIndexer()) + L7EndpointsCalculator := NewL7EndpointsCalculator(zoneGetter, podLister, testPortName, negtypes.VmIpPortEndpointType, klog.TODO()) + L4LocalEndpointCalculator := NewLocalL4ILBEndpointsCalculator(nodeLister, zoneGetter, svcKey, klog.TODO()) + L4ClusterEndpointCalculator := NewClusterL4ILBEndpointsCalculator(nodeLister, zoneGetter, svcKey, klog.TODO()) + + testEndpointPodMap := map[negtypes.NetworkEndpoint]types.NamespacedName{ + { + IP: testIP1, + Port: "80", + Node: instance1, + }: { + Namespace: testServiceNamespace, + Name: testPodName1, + }, + { + IP: testIP2, + Port: "80", + Node: instance1, + }: { + Namespace: testServiceNamespace, + Name: testPodName2, + }, + { + IP: testIP3, + Port: "81", + Node: instance2, + }: { + Namespace: testServiceNamespace, + Name: testPodName3, + }, + { + IP: testIP4, + Port: "81", + Node: instance4, + }: { + Namespace: testServiceNamespace, + Name: testPodName4, + }, + } + + testCases := []struct { + desc string + ec negtypes.NetworkEndpointsCalculator + endpointsData []negtypes.EndpointsData + endpointPodMap map[negtypes.NetworkEndpoint]types.NamespacedName + dupCount int + expect error + }{ + { + desc: "ValidateEndpoints for L4 local endpoint calculator", // we are adding this test to make sure the test is updated when the functionality is added + ec: L4LocalEndpointCalculator, + endpointsData: nil, // for now it is a no-op + endpointPodMap: nil, // L4 EndpointCalculators do not compute endpoints pod data + dupCount: 0, + expect: nil, + }, + { + + desc: "ValidateEndpoints for L4 cluster endpoint calculator", // we are adding this test to make sure the test is updated when the functionality is added + ec: L4ClusterEndpointCalculator, + endpointsData: nil, // for now it is a no-op + endpointPodMap: nil, // L4 EndpointCalculators do not compute endpoints pod data + dupCount: 0, + expect: nil, + }, + { + desc: "ValidateEndpoints for L7 Endpoint Calculator. Endpoint counts equal, endpointData has no duplicated endpoints", + ec: L7EndpointsCalculator, + endpointsData: []negtypes.EndpointsData{ + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-1", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port80, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName1, + }, + NodeName: &instance1, + Addresses: []string{testIP1}, + Ready: true, + }, + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName2, + }, + NodeName: &instance1, + Addresses: []string{testIP2}, + Ready: true, + }, + }, + }, + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-2", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port81, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName3, + }, + NodeName: &instance2, + Addresses: []string{testIP3}, + Ready: true, + }, + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName4, + }, + NodeName: &instance4, + Addresses: []string{testIP4}, + Ready: true, + }, + }, + }, + }, + endpointPodMap: testEndpointPodMap, + dupCount: 0, + expect: nil, + }, + { + desc: "ValidateEndpoints for L7 Endpoint Calculator. Endpoint counts equal, endpointData has duplicated endpoints", + ec: L7EndpointsCalculator, + endpointsData: []negtypes.EndpointsData{ + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-1", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port80, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName1, + }, + NodeName: &instance1, + Addresses: []string{testIP1}, + Ready: true, + }, + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName2, + }, + NodeName: &instance1, + Addresses: []string{testIP2}, + Ready: true, + }, + }, + }, + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-2", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port81, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName2, + }, + NodeName: &instance1, + Addresses: []string{testIP2}, + Ready: true, + }, // this is a duplicated endpoint + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName3, + }, + NodeName: &instance2, + Addresses: []string{testIP3}, + Ready: true, + }, + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName4, + }, + NodeName: &instance4, + Addresses: []string{testIP4}, + Ready: true, + }, + }, + }, + }, + endpointPodMap: testEndpointPodMap, + dupCount: 1, + expect: nil, + }, + { + desc: "ValidateEndpoints for L7 Endpoint Calculator. Endpoint counts not equal, endpointData has no duplicated endpoints", + ec: L7EndpointsCalculator, + endpointsData: []negtypes.EndpointsData{ + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-1", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port80, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName2, + }, + NodeName: &instance1, + Addresses: []string{testIP2}, + Ready: true, + }, + }, + }, + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-2", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port81, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName3, + }, + NodeName: &instance2, + Addresses: []string{testIP3}, + Ready: true, + }, + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName4, + }, + NodeName: &instance4, + Addresses: []string{testIP4}, + Ready: true, + }, + }, + }, + }, + endpointPodMap: testEndpointPodMap, + dupCount: 0, + expect: negtypes.ErrEPCountsDiffer, + }, + { + desc: "ValidateEndpoints for L7 Endpoint Calculator. Endpoint counts not equal, endpointData has duplicated endpoints", + ec: L7EndpointsCalculator, + endpointsData: []negtypes.EndpointsData{ + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-1", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port80, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName2, + }, + NodeName: &instance1, + Addresses: []string{testIP2}, + Ready: true, + }, + }, + }, + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-2", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port81, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName2, + }, + NodeName: &instance1, + Addresses: []string{testIP2}, + Ready: true, + }, // this is a duplicated endpoint + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName3, + }, + NodeName: &instance2, + Addresses: []string{testIP3}, + Ready: true, + }, + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName4, + }, + NodeName: &instance4, + Addresses: []string{testIP4}, + Ready: true, + }, + }, + }, + }, + endpointPodMap: testEndpointPodMap, + dupCount: 1, + expect: negtypes.ErrEPCountsDiffer, + }, + { + desc: "ValidateEndpoints for L7 Endpoint Calculator. EndpointData has zero endpoint", + ec: L7EndpointsCalculator, + endpointsData: []negtypes.EndpointsData{ + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-1", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port80, + }, + }, + Addresses: []negtypes.AddressData{}, + }, + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-2", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port81, + }, + }, + Addresses: []negtypes.AddressData{}, + }, + }, + endpointPodMap: testEndpointPodMap, + dupCount: 0, + expect: negtypes.ErrEPSEndpointCountZero, + }, + { + desc: "ValidateEndpoints for L7 Endpoint Calculator. EndpointPodMap has zero endpoint", + ec: L7EndpointsCalculator, + endpointsData: []negtypes.EndpointsData{ + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-1", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port80, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName1, + }, + NodeName: &instance1, + Addresses: []string{testIP1}, + Ready: true, + }, + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName2, + }, + NodeName: &instance1, + Addresses: []string{testIP2}, + Ready: true, + }, + }, + }, + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-2", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port81, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName3, + }, + NodeName: &instance2, + Addresses: []string{testIP3}, + Ready: true, + }, + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName4, + }, + NodeName: &instance4, + Addresses: []string{testIP4}, + Ready: true, + }, + }, + }, + }, + endpointPodMap: map[negtypes.NetworkEndpoint]types.NamespacedName{}, + dupCount: 0, + expect: negtypes.ErrEPCalculationCountZero, + }, + { + desc: "ValidateEndpoints for L7 Endpoint Calculator. EndpointData and endpointPodMap both have zero endpoint", + ec: L7EndpointsCalculator, + endpointsData: []negtypes.EndpointsData{ + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-1", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port80, + }, + }, + Addresses: []negtypes.AddressData{}, + }, + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-2", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port81, + }, + }, + Addresses: []negtypes.AddressData{}, + }, + }, + endpointPodMap: map[negtypes.NetworkEndpoint]types.NamespacedName{}, + dupCount: 0, + expect: negtypes.ErrEPCalculationCountZero, // PodMap count is check and returned first, + }, + { + desc: "ValidateEndpoints for L7 Endpoint Calculator. EndpointData and endpointPodMap both have non-zero endpoints", + ec: L7EndpointsCalculator, + endpointsData: []negtypes.EndpointsData{ + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-1", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port80, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName1, + }, + NodeName: &instance1, + Addresses: []string{testIP1}, + Ready: true, + }, + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName2, + }, + NodeName: &instance1, + Addresses: []string{testIP2}, + Ready: true, + }, + }, + }, + { + Meta: &metav1.ObjectMeta{ + Name: testServiceName + "-2", + Namespace: testServiceNamespace, + }, + Ports: []negtypes.PortData{ + { + Name: testPortName, + Port: port81, + }, + }, + Addresses: []negtypes.AddressData{ + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName3, + }, + NodeName: &instance2, + Addresses: []string{testIP3}, + Ready: true, + }, + { + TargetRef: &v1.ObjectReference{ + Namespace: testServiceNamespace, + Name: testPodName4, + }, + NodeName: &instance4, + Addresses: []string{testIP4}, + Ready: true, + }, + }, + }, + }, + endpointPodMap: testEndpointPodMap, + dupCount: 0, + expect: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + if got := tc.ec.ValidateEndpoints(tc.endpointsData, tc.endpointPodMap, tc.dupCount); !errors.Is(got, tc.expect) { + t.Errorf("ValidateEndpoints() = %t, expected %t", got, tc.expect) + } + }) + } +} + 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/types/interfaces.go b/pkg/neg/types/interfaces.go index 054b3dd9d1..cf5709e0e3 100644 --- a/pkg/neg/types/interfaces.go +++ b/pkg/neg/types/interfaces.go @@ -86,4 +86,6 @@ 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 + // ValidateEndpoints validates the NEG endpoint information is correct + ValidateEndpoints(endpointData []EndpointsData, endpointPodMap EndpointPodMap, dupCount int) error } From 915788c6e28b2f838b7c7a5c6af28badd9339b96 Mon Sep 17 00:00:00 2001 From: David Cheung Date: Wed, 8 Mar 2023 23:18:12 +0000 Subject: [PATCH 3/3] Refactor error state checking Create function ValidateEndpoints, so it would correctly validate endpoints for L4 and L7 endpoint calculators. Validation for L4 endpoints is currently a noop. --- pkg/neg/syncers/transaction.go | 93 +-- pkg/neg/syncers/transaction_test.go | 965 ---------------------------- pkg/neg/syncers/utils.go | 3 +- pkg/neg/types/sync_results.go | 59 +- 4 files changed, 59 insertions(+), 1061 deletions(-) diff --git a/pkg/neg/syncers/transaction.go b/pkg/neg/syncers/transaction.go index 814341ae3e..bf1a2dba4d 100644 --- a/pkg/neg/syncers/transaction.go +++ b/pkg/neg/syncers/transaction.go @@ -187,19 +187,15 @@ func (s *transactionSyncer) syncInternal() error { start := time.Now() err := s.syncInternalImpl() - + if err != nil { + s.setErrorState(s.getErrorStateReason(err)) + } s.updateStatus(err) metrics.PublishNegSyncMetrics(string(s.NegSyncerKey.NegType), string(s.endpointsCalculator.Mode()), err, start) return err } func (s *transactionSyncer) syncInternalImpl() error { - // TODO(cheungdavid): for now we reset the boolean so it is a no-op, but - // in the future, it will be used to trigger degraded mode if the syncer is in error state. - if s.inErrorState() { - s.resetErrorState() - } - if s.needInit || s.isZoneChange() { if err := s.ensureNetworkEndpointGroups(); err != nil { return err @@ -259,16 +255,16 @@ func (s *transactionSyncer) syncInternalImpl() error { } endpointsData := negtypes.EndpointsDataFromEndpointSlices(endpointSlices) targetMap, endpointPodMap, dupCount, err = s.endpointsCalculator.CalculateEndpoints(endpointsData, currentMap) - if valid, reason := s.isValidEPField(err); !valid { - s.setErrorState(reason) - } - if valid, reason := s.isValidEndpointInfo(endpointsData, endpointPodMap, dupCount); !valid { - s.setErrorState(reason) - } if err != nil { + s.setErrorState(s.getErrorStateReason(err)) return fmt.Errorf("endpoints calculation error in mode %q, err: %w", s.endpointsCalculator.Mode(), err) } - + err = s.endpointsCalculator.ValidateEndpoints(endpointsData, endpointPodMap, dupCount) + if err != nil { + // TODO(cheungdavid): return error from ValidateEndpoint after degraded mode is implemented + // for now we don't return error so it won't break the sync + s.setErrorState(s.getErrorStateReason(err)) + } s.logStats(targetMap, "desired NEG endpoints") // Calculate the endpoints to add and delete to transform the current state to desire state @@ -308,11 +304,6 @@ func (s *transactionSyncer) setErrorState(errorState string) { s.errorState = errorState } -// syncLock must already be acquired before execution -func (s *transactionSyncer) resetErrorState() { - s.errorState = "" -} - // ensureNetworkEndpointGroups ensures NEGs are created and configured correctly in the corresponding zones. func (s *transactionSyncer) ensureNetworkEndpointGroups() error { var err error @@ -354,69 +345,32 @@ func (s *transactionSyncer) ensureNetworkEndpointGroups() error { return utilerrors.NewAggregate(errList) } -// isValidEndpointInfo checks if endpoint information is correct. -// It returns false and the corresponding reason if one of the two checks fails: -// -// 1. The endpoint count from endpointData doesn't equal to the one from endpointPodMap: -// endpiontPodMap removes the duplicated endpoints, and dupCount stores the number of duplicated it removed -// and we compare the endpoint counts with duplicates -// 2. The endpoint count from endpointData or the one from endpointPodMap is 0 -func (s *transactionSyncer) isValidEndpointInfo(eds []negtypes.EndpointsData, endpointPodMap negtypes.EndpointPodMap, dupCount int) (bool, string) { - // Endpoint count from EndpointPodMap - countFromPodMap := len(endpointPodMap) + dupCount - if countFromPodMap == 0 { - s.logger.Info("Detected endpoint count from endpointPodMap going to zero", "endpointPodMap", endpointPodMap) - return false, negtypes.ResultEPCalculationCountZero - } - - // Endpoint count from EndpointData - countFromEndpointData := 0 - for _, ed := range eds { - countFromEndpointData += len(ed.Addresses) - } - if countFromEndpointData == 0 { - s.logger.Info("Detected endpoint count from endpointData going to zero", "endpointData", eds) - return false, negtypes.ResultEPSEndpointCountZero - } - - if countFromEndpointData != countFromPodMap { - s.logger.Info("Detected error when comparing endpoint counts", "endpointData", eds, "endpointPodMap", endpointPodMap, "dupCount", dupCount) - return false, negtypes.ResultEPCountsDiffer +// getErrorStateReason returns the reason of the error state based returned error +func (s *transactionSyncer) getErrorStateReason(err error) string { + if result, contains := negtypes.ErrorStateResult[err]; contains { + return result } - return true, negtypes.ResultSuccess + return "" } -// isValidEPField returns false and the corresponding reason if there is endpoint with missing zone or nodeName -func (s *transactionSyncer) isValidEPField(err error) (bool, string) { - if errors.Is(err, negtypes.ErrEPMissingNodeName) { - s.logger.Info("Detected unexpected error when checking missing nodeName", "error", err) - return false, negtypes.ResultEPMissingNodeName - } - if errors.Is(err, negtypes.ErrEPMissingZone) { - s.logger.Info("Detected unexpected error when checking missing zone", "error", err) - return false, negtypes.ResultEPMissingZone - } - return true, negtypes.ResultSuccess -} - -// isValidEPBatch returns false and the corresponding reason if the error from endpoint batch response is due to bad request -func (s *transactionSyncer) isValidEPBatch(err error, operation transactionOp, networkEndpoints []*composite.NetworkEndpoint) (bool, string) { +// ValidEndpointBatch parse the error from endpoint batch response to NEG sync errors +func (s *transactionSyncer) ValidateEndpointBatch(err error, operation transactionOp) error { apiErr, ok := err.(*googleapi.Error) if !ok { s.logger.Info("Detected error when parsing batch response error", "operation", operation, "error", err) - return false, negtypes.ResultInvalidAPIResponse + return negtypes.ErrInvalidAPIResponse } errCode := apiErr.Code if errCode == http.StatusBadRequest { s.logger.Info("Detected error when sending endpoint batch information", "operation", operation, "errorCode", errCode) if operation == attachOp { - return false, negtypes.ResultInvalidEPAttach + return negtypes.ErrInvalidEPAttach } if operation == detachOp { - return false, negtypes.ResultInvalidEPDetach + return negtypes.ErrInvalidEPDetach } } - return true, negtypes.ResultSuccess + return nil } // syncNetworkEndpoints spins off go routines to execute NEG operations @@ -526,10 +480,11 @@ func (s *transactionSyncer) operationInternal(operation transactionOp, zone stri s.recordEvent(apiv1.EventTypeNormal, operation.String(), fmt.Sprintf("%s %d network endpoint(s) (NEG %q in zone %q)", operation.String(), len(networkEndpointMap), s.NegSyncerKey.NegName, zone)) } else { s.recordEvent(apiv1.EventTypeWarning, operation.String()+"Failed", fmt.Sprintf("Failed to %s %d network endpoint(s) (NEG %q in zone %q): %v", operation.String(), len(networkEndpointMap), s.NegSyncerKey.NegName, zone, err)) - if valid, reason := s.isValidEPBatch(err, operation, networkEndpoints); !valid { + endpointBatchErr := s.ValidateEndpointBatch(err, operation) + if endpointBatchErr != nil { s.syncLock.Lock() defer s.syncLock.Unlock() - s.setErrorState(reason) + s.setErrorState(s.getErrorStateReason(endpointBatchErr)) } } diff --git a/pkg/neg/syncers/transaction_test.go b/pkg/neg/syncers/transaction_test.go index b146e8cacd..c35b79783a 100644 --- a/pkg/neg/syncers/transaction_test.go +++ b/pkg/neg/syncers/transaction_test.go @@ -18,10 +18,8 @@ package syncers import ( context2 "context" - "errors" "fmt" "net" - "net/http" "reflect" "strconv" "testing" @@ -29,8 +27,6 @@ import ( "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" - "google.golang.org/api/compute/v1" - "google.golang.org/api/googleapi" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -1392,967 +1388,6 @@ func TestUnknownNodes(t *testing.T) { } } -func TestIsValidEndpointInfo(t *testing.T) { - t.Parallel() - _, transactionSyncer := newTestTransactionSyncer(negtypes.NewAdapter(gce.NewFakeGCECloud(gce.DefaultTestClusterValues())), negtypes.VmIpPortEndpointType, false) - - instance1 := testInstance1 - instance2 := testInstance2 - instance4 := testInstance4 - testPortName := "" - testServiceNamespace := "namespace" - port80 := int32(80) - port81 := int32(81) - testIP1 := "10.100.1.1" - testIP2 := "10.100.1.2" - testIP3 := "10.100.2.2" - testIP4 := "10.100.4.1" - testPodName1 := "pod1" - testPodName2 := "pod2" - testPodName3 := "pod3" - testPodName4 := "pod4" - - testEndpointPodMap := map[negtypes.NetworkEndpoint]types.NamespacedName{ - { - IP: testIP1, - Port: "80", - Node: instance1, - }: { - Namespace: testServiceNamespace, - Name: testPodName1, - }, - { - IP: testIP2, - Port: "80", - Node: instance1, - }: { - Namespace: testServiceNamespace, - Name: testPodName2, - }, - { - IP: testIP3, - Port: "81", - Node: instance2, - }: { - Namespace: testServiceNamespace, - Name: testPodName3, - }, - { - IP: testIP4, - Port: "81", - Node: instance4, - }: { - Namespace: testServiceNamespace, - Name: testPodName4, - }, - } - - testCases := []struct { - desc string - endpointsData []negtypes.EndpointsData - endpointPodMap map[negtypes.NetworkEndpoint]types.NamespacedName - dupCount int - expect bool - expectedReason string - }{ - { - desc: "counts equal, endpointData has no duplicated endpoints", - endpointsData: []negtypes.EndpointsData{ - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-1", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port80, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName1, - }, - NodeName: &instance1, - Addresses: []string{testIP1}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName2, - }, - NodeName: &instance1, - Addresses: []string{testIP2}, - Ready: true, - }, - }, - }, - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-2", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port81, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName3, - }, - NodeName: &instance2, - Addresses: []string{testIP3}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName4, - }, - NodeName: &instance4, - Addresses: []string{testIP4}, - Ready: true, - }, - }, - }, - }, - endpointPodMap: testEndpointPodMap, - dupCount: 0, - expect: true, - expectedReason: "", - }, - { - desc: "counts equal, endpointData has duplicated endpoints", - endpointsData: []negtypes.EndpointsData{ - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-1", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port80, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName1, - }, - NodeName: &instance1, - Addresses: []string{testIP1}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName2, - }, - NodeName: &instance1, - Addresses: []string{testIP2}, - Ready: true, - }, - }, - }, - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-2", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port81, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName2, - }, - NodeName: &instance1, - Addresses: []string{testIP2}, - Ready: true, - }, // this is a duplicated endpoint - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName3, - }, - NodeName: &instance2, - Addresses: []string{testIP3}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName4, - }, - NodeName: &instance4, - Addresses: []string{testIP4}, - Ready: true, - }, - }, - }, - }, - endpointPodMap: testEndpointPodMap, - dupCount: 1, - expect: true, - expectedReason: "", - }, - { - desc: "counts not equal, endpointData has no duplicated endpoints", - endpointsData: []negtypes.EndpointsData{ - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-1", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port80, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName2, - }, - NodeName: &instance1, - Addresses: []string{testIP2}, - Ready: true, - }, - }, - }, - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-2", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port81, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName3, - }, - NodeName: &instance2, - Addresses: []string{testIP3}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName4, - }, - NodeName: &instance4, - Addresses: []string{testIP4}, - Ready: true, - }, - }, - }, - }, - endpointPodMap: testEndpointPodMap, - dupCount: 0, - expect: false, - expectedReason: negtypes.ResultEPCountsDiffer, - }, - { - desc: "counts not equal, endpointData has duplicated endpoints", - endpointsData: []negtypes.EndpointsData{ - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-1", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port80, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName2, - }, - NodeName: &instance1, - Addresses: []string{testIP2}, - Ready: true, - }, - }, - }, - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-2", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port81, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName2, - }, - NodeName: &instance1, - Addresses: []string{testIP2}, - Ready: true, - }, // this is a duplicated endpoint - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName3, - }, - NodeName: &instance2, - Addresses: []string{testIP3}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName4, - }, - NodeName: &instance4, - Addresses: []string{testIP4}, - Ready: true, - }, - }, - }, - }, - endpointPodMap: testEndpointPodMap, - dupCount: 1, - expect: false, - expectedReason: negtypes.ResultEPCountsDiffer, - }, - { - desc: "endpointData has zero endpoint", - endpointsData: []negtypes.EndpointsData{ - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-1", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port80, - }, - }, - Addresses: []negtypes.AddressData{}, - }, - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-2", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port81, - }, - }, - Addresses: []negtypes.AddressData{}, - }, - }, - endpointPodMap: testEndpointPodMap, - dupCount: 0, - expect: false, - expectedReason: negtypes.ResultEPSEndpointCountZero, - }, - { - desc: "endpointPodMap has zero endpoint", - endpointsData: []negtypes.EndpointsData{ - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-1", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port80, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName1, - }, - NodeName: &instance1, - Addresses: []string{testIP1}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName2, - }, - NodeName: &instance1, - Addresses: []string{testIP2}, - Ready: true, - }, - }, - }, - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-2", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port81, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName3, - }, - NodeName: &instance2, - Addresses: []string{testIP3}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName4, - }, - NodeName: &instance4, - Addresses: []string{testIP4}, - Ready: true, - }, - }, - }, - }, - endpointPodMap: map[negtypes.NetworkEndpoint]types.NamespacedName{}, - dupCount: 0, - expect: false, - expectedReason: negtypes.ResultEPCalculationCountZero, - }, - { - desc: "endpointData and endpointPodMap both have zero endpoint", - endpointsData: []negtypes.EndpointsData{ - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-1", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port80, - }, - }, - Addresses: []negtypes.AddressData{}, - }, - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-2", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port81, - }, - }, - Addresses: []negtypes.AddressData{}, - }, - }, - endpointPodMap: map[negtypes.NetworkEndpoint]types.NamespacedName{}, - dupCount: 0, - expect: false, - expectedReason: negtypes.ResultEPCalculationCountZero, // PodMap count is check and returned first, - }, - { - desc: "endpointData and endpointPodMap both have non-zero endpoints", - endpointsData: []negtypes.EndpointsData{ - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-1", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port80, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName1, - }, - NodeName: &instance1, - Addresses: []string{testIP1}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName2, - }, - NodeName: &instance1, - Addresses: []string{testIP2}, - Ready: true, - }, - }, - }, - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-2", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port81, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName3, - }, - NodeName: &instance2, - Addresses: []string{testIP3}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName4, - }, - NodeName: &instance4, - Addresses: []string{testIP4}, - Ready: true, - }, - }, - }, - }, - endpointPodMap: testEndpointPodMap, - dupCount: 0, - expect: true, - expectedReason: negtypes.ResultSuccess, - }, - } - - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - if got, reason := transactionSyncer.isValidEndpointInfo(tc.endpointsData, tc.endpointPodMap, tc.dupCount); got != tc.expect && reason != tc.expectedReason { - t.Errorf("invalidEndpointInfo() = %t, expected %t", got, tc.expect) - } - }) - } -} - -func TestIsValidEPField(t *testing.T) { - t.Parallel() - _, transactionSyncer := newTestTransactionSyncer(negtypes.NewAdapter(gce.NewFakeGCECloud(gce.DefaultTestClusterValues())), negtypes.VmIpPortEndpointType, false) - - instance1 := testInstance1 - instance2 := testInstance2 - instance4 := testInstance4 - testPortName := "" - testServiceNamespace := "namespace" - port80 := int32(80) - port81 := int32(81) - testIP1 := "10.100.1.1" - testIP2 := "10.100.1.2" - testIP3 := "10.100.2.2" - testIP4 := "10.100.4.1" - testPodName1 := "pod1" - testPodName2 := "pod2" - testPodName3 := "pod3" - testPodName4 := "pod4" - emptyZoneNodeName := "empty-zone-node" - emptyNodeName := "" - fakeZoneGetter := transactionSyncer.zoneGetter.(*negtypes.FakeZoneGetter) - if err := fakeZoneGetter.AddZone("", emptyZoneNodeName); err != nil { - t.Errorf("error when adding zone, expected nil, err: %v", err) - } - - testCases := []struct { - desc string - endpointsData []negtypes.EndpointsData - expect bool - expectedReason string - }{ - { - desc: "no missing zone or nodeName", - endpointsData: []negtypes.EndpointsData{ - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-1", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port80, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName1, - }, - NodeName: &instance1, - Addresses: []string{testIP1}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName2, - }, - NodeName: &instance1, - Addresses: []string{testIP2}, - Ready: true, - }, - }, - }, - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-2", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port81, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName3, - }, - NodeName: &instance2, - Addresses: []string{testIP3}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName4, - }, - NodeName: &instance4, - Addresses: []string{testIP4}, - Ready: true, - }, - }, - }, - }, - expect: true, - expectedReason: negtypes.ResultSuccess, - }, - { - desc: "contains one missing nodeName", - endpointsData: []negtypes.EndpointsData{ - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-1", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port80, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName1, - }, - NodeName: nil, - Addresses: []string{testIP1}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName2, - }, - NodeName: &instance1, - Addresses: []string{testIP2}, - Ready: true, - }, - }, - }, - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-2", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port81, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName3, - }, - NodeName: &instance2, - Addresses: []string{testIP3}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName4, - }, - NodeName: &instance4, - Addresses: []string{testIP4}, - Ready: true, - }, - }, - }, - }, - expect: false, - expectedReason: negtypes.ResultEPMissingNodeName, - }, - { - desc: "contains one empty nodeName", - endpointsData: []negtypes.EndpointsData{ - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-1", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port80, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName1, - }, - NodeName: &emptyNodeName, - Addresses: []string{testIP1}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName2, - }, - NodeName: &instance1, - Addresses: []string{testIP2}, - Ready: true, - }, - }, - }, - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-2", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port81, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName3, - }, - NodeName: &instance2, - Addresses: []string{testIP3}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName4, - }, - NodeName: &instance4, - Addresses: []string{testIP4}, - Ready: true, - }, - }, - }, - }, - expect: false, - expectedReason: negtypes.ResultEPMissingNodeName, - }, - { - desc: "contains one missing zone", - endpointsData: []negtypes.EndpointsData{ - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-1", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port80, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName1, - }, - NodeName: &emptyZoneNodeName, - Addresses: []string{testIP1}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName2, - }, - NodeName: &instance1, - Addresses: []string{testIP2}, - Ready: true, - }, - }, - }, - { - Meta: &metav1.ObjectMeta{ - Name: testServiceName + "-2", - Namespace: testServiceNamespace, - }, - Ports: []negtypes.PortData{ - { - Name: testPortName, - Port: port81, - }, - }, - Addresses: []negtypes.AddressData{ - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName3, - }, - NodeName: &instance2, - Addresses: []string{testIP3}, - Ready: true, - }, - { - TargetRef: &corev1.ObjectReference{ - Namespace: testServiceNamespace, - Name: testPodName4, - }, - NodeName: &instance4, - Addresses: []string{testIP4}, - Ready: true, - }, - }, - }, - }, - expect: false, - expectedReason: negtypes.ResultEPMissingZone, - }, - } - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - _, _, _, err := transactionSyncer.endpointsCalculator.CalculateEndpoints(tc.endpointsData, nil) - if got, reason := transactionSyncer.isValidEPField(err); got != tc.expect && reason != tc.expectedReason { - t.Errorf("isValidEPField() = %t, expected %t, err: %v, ", got, tc.expect, err) - } - }) - } -} - -func TestCheckValidEPBatch(t *testing.T) { - fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues()) - fakeCloud := negtypes.NewAdapter(fakeGCE) - zone := "us-central1-a" - networkEndpoints := []*composite.NetworkEndpoint{} - - testCases := []struct { - desc string - APIResponse error - expect bool - expectedReason string - }{ - { - desc: "NEG API call server error, status code 500", - APIResponse: &googleapi.Error{ - Code: http.StatusOK, - }, - expect: true, - expectedReason: negtypes.ResultSuccess, - }, - { - desc: "NEG API call request error, status code 400", - APIResponse: &googleapi.Error{ - Code: http.StatusBadRequest, - }, - expect: false, - expectedReason: negtypes.ResultInvalidEPAttach, - }, - { - desc: "NEG API call invalid response", - APIResponse: errors.New("non googleapi error"), - expect: false, - expectedReason: negtypes.ResultInvalidAPIResponse, - }, - } - - for _, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - mockGCE := fakeGCE.Compute().(*cloud.MockGCE) - mockGCE.MockNetworkEndpointGroups.AttachNetworkEndpointsHook = func(ctx context2.Context, key *meta.Key, arg0 *compute.NetworkEndpointGroupsAttachEndpointsRequest, neg *cloud.MockNetworkEndpointGroups) error { - return tc.APIResponse - } - _, transactionSyncer := newTestTransactionSyncer(fakeCloud, negtypes.VmIpPortEndpointType, false) - - err := transactionSyncer.cloud.AttachNetworkEndpoints(transactionSyncer.NegSyncerKey.NegName, zone, networkEndpoints, transactionSyncer.NegSyncerKey.GetAPIVersion()) - if got, reason := transactionSyncer.isValidEPBatch(err, attachOp, networkEndpoints); got != tc.expect || reason != tc.expectedReason { - t.Errorf("isInvalidEPBatch() = %t, expected %t", got, tc.expect) - } - }) - } -} - func newL4ILBTestTransactionSyncer(fakeGCE negtypes.NetworkEndpointGroupCloud, mode negtypes.EndpointsCalculatorMode) (negtypes.NegSyncer, *transactionSyncer) { negsyncer, ts := newTestTransactionSyncer(fakeGCE, negtypes.VmIpEndpointType, false) ts.endpointsCalculator = GetEndpointsCalculator(ts.nodeLister, ts.podLister, ts.zoneGetter, ts.NegSyncerKey, mode, klog.TODO()) diff --git a/pkg/neg/syncers/utils.go b/pkg/neg/syncers/utils.go index e973feae39..35e2bd4cbd 100644 --- a/pkg/neg/syncers/utils.go +++ b/pkg/neg/syncers/utils.go @@ -245,7 +245,7 @@ func toZoneNetworkEndpointMap(eds []negtypes.EndpointsData, zoneGetter negtypes. for _, endpointAddress := range ed.Addresses { if endpointAddress.NodeName == nil || len(*endpointAddress.NodeName) == 0 { - klog.V(2).Infof("Endpoint %q in Endpoints %s/%s does not have an associated node. Skipping", endpointAddress.Addresses, ed.Meta.Namespace, ed.Meta.Name) + klog.V(2).Infof("Detected unexpected error when checking missing nodeName. Endpoint %q in Endpoints %s/%s does not have an associated node. Skipping", endpointAddress.Addresses, ed.Meta.Namespace, ed.Meta.Name) return nil, nil, dupCount, negtypes.ErrEPMissingNodeName } if endpointAddress.TargetRef == nil { @@ -257,6 +257,7 @@ func toZoneNetworkEndpointMap(eds []negtypes.EndpointsData, zoneGetter negtypes. return nil, nil, dupCount, negtypes.ErrNodeNotFound } if zone == "" { + klog.V(2).Info("Detected unexpected error when checking missing zone") return nil, nil, dupCount, negtypes.ErrEPMissingZone } if zoneNetworkEndpointMap[zone] == nil { diff --git a/pkg/neg/types/sync_results.go b/pkg/neg/types/sync_results.go index 6610869879..b32fd57a50 100644 --- a/pkg/neg/types/sync_results.go +++ b/pkg/neg/types/sync_results.go @@ -15,33 +15,16 @@ package types import "errors" -var ( - ResultEPCountsDiffer = "EPCountsDiffer" - ErrEPCountsDiffer = errors.New("endpoint counts from endpointData and endpointPodMap differ") - - ResultEPMissingNodeName = "EPMissingNodeName" - ErrEPMissingNodeName = errors.New("endpoint has empty nodeName field") - - ResultNodeNotFound = "NodeNotFound" - ErrNodeNotFound = errors.New("failed to retrieve associated zone of node") - - ResultEPMissingZone = "EPMissingZone" - ErrEPMissingZone = errors.New("endpoint has empty zone field") - - ResultEPSEndpointCountZero = "EPSEndpointCountZero" - ErrEPSEndpointCountZero = errors.New("endpoint count from endpointData cannot be zero") - +const ( + ResultEPCountsDiffer = "EPCountsDiffer" + ResultEPMissingNodeName = "EPMissingNodeName" + ResultNodeNotFound = "NodeNotFound" + ResultEPMissingZone = "EPMissingZone" + ResultEPSEndpointCountZero = "EPSEndpointCountZero" ResultEPCalculationCountZero = "EPCalculationCountZero" - ErrEPCalculationCountZero = errors.New("endpoint count from endpointPodMap cannot be zero") - - ResultInvalidAPIResponse = "InvalidAPIResponse" - ErrInvalidAPIResponse = errors.New("received response error doesn't match googleapi.Error type") - - ResultInvalidEPAttach = "InvalidEPAttach" - ErrInvalidEPAttach = errors.New("endpoint information for attach operation is incorrect") - - ResultInvalidEPDetach = "InvalidEPDetach" - ErrInvalidEPDetach = errors.New("endpoint information for detach operation is incorrect") + ResultInvalidAPIResponse = "InvalidAPIResponse" + ResultInvalidEPAttach = "InvalidEPAttach" + ResultInvalidEPDetach = "InvalidEPDetach" // these results have their own errors ResultNegNotFound = "NegNotFound" @@ -52,6 +35,30 @@ var ( ResultSuccess = "Success" ) +var ( + ErrEPCountsDiffer = errors.New("endpoint counts from endpointData and endpointPodMap differ") + ErrEPMissingNodeName = errors.New("endpoint has empty nodeName field") + ErrNodeNotFound = errors.New("failed to retrieve associated zone of node") + ErrEPMissingZone = errors.New("endpoint has empty zone field") + ErrEPSEndpointCountZero = errors.New("endpoint count from endpointData cannot be zero") + ErrEPCalculationCountZero = errors.New("endpoint count from endpointPodMap cannot be zero") + ErrInvalidAPIResponse = errors.New("received response error doesn't match googleapi.Error type") + ErrInvalidEPAttach = errors.New("endpoint information for attach operation is incorrect") + ErrInvalidEPDetach = errors.New("endpoint information for detach operation is incorrect") + + // use this map for conversion between errors and sync results + ErrorStateResult = map[error]string{ + ErrEPMissingNodeName: ResultEPMissingNodeName, + ErrEPMissingZone: ResultEPMissingZone, + ErrEPCalculationCountZero: ResultEPCalculationCountZero, + ErrEPSEndpointCountZero: ResultEPSEndpointCountZero, + ErrEPCountsDiffer: ResultEPCountsDiffer, + ErrInvalidAPIResponse: ResultInvalidAPIResponse, + ErrInvalidEPAttach: ResultInvalidEPAttach, + ErrInvalidEPDetach: ResultInvalidEPDetach, + } +) + type NegSyncResult struct { Error error Result string