From 5101a4fe81bafeea5c18b3862f13607d87f9815c Mon Sep 17 00:00:00 2001 From: Aaron U'Ren Date: Thu, 2 Dec 2021 09:47:58 -0600 Subject: [PATCH] fix(nsc): remove error for lookupFWMarkByService lookupFWMarkByService() was previous returning an error when no fwmark was found in the tracking map for a given service. However, this isn't really an error condition and shouldn't be treated as such. When it was treated as an error condition users got a lot of confusing errors in the logs. --- pkg/controllers/proxy/service_endpoints_sync.go | 4 +--- pkg/controllers/proxy/utils.go | 6 +++--- pkg/controllers/proxy/utils_test.go | 8 ++------ 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/pkg/controllers/proxy/service_endpoints_sync.go b/pkg/controllers/proxy/service_endpoints_sync.go index aca46aae..f9cf8e46 100644 --- a/pkg/controllers/proxy/service_endpoints_sync.go +++ b/pkg/controllers/proxy/service_endpoints_sync.go @@ -367,10 +367,8 @@ func (nsc *NetworkServicesController) setupExternalIPServices(serviceInfoMap ser externalIPServiceID = generateIPPortID(externalIP, svc.protocol, strconv.Itoa(svc.port)) // ensure there is NO iptables mangle table rule to FW mark the packet - fwMark, err := nsc.lookupFWMarkByService(externalIP, svc.protocol, strconv.Itoa(svc.port)) + fwMark := nsc.lookupFWMarkByService(externalIP, svc.protocol, strconv.Itoa(svc.port)) switch { - case err != nil: - klog.Errorf("failed to find FW mark for the service: %v", err) case fwMark == 0: klog.V(2).Infof("no FW mark found for service, nothing to cleanup") case fwMark != 0: diff --git a/pkg/controllers/proxy/utils.go b/pkg/controllers/proxy/utils.go index 510f6d07..164e048c 100644 --- a/pkg/controllers/proxy/utils.go +++ b/pkg/controllers/proxy/utils.go @@ -211,14 +211,14 @@ func (nsc *NetworkServicesController) generateUniqueFWMark(ip, protocol, port st // lookupFWMarkByService finds the related FW mark from the internal fwMarkMap kept by the NetworkServiceController // given the related ip, protocol, and port. If it isn't able to find a matching FW mark, then it returns an error. -func (nsc *NetworkServicesController) lookupFWMarkByService(ip, protocol, port string) (uint32, error) { +func (nsc *NetworkServicesController) lookupFWMarkByService(ip, protocol, port string) uint32 { needle := fmt.Sprintf("%s-%s-%s", ip, protocol, port) for fwMark, serviceKey := range nsc.fwMarkMap { if needle == serviceKey { - return fwMark, nil + return fwMark } } - return 0, fmt.Errorf("no key matching %s:%s:%s was found in fwMarkMap", protocol, ip, port) + return 0 } // lookupServiceByFWMark Lookup service ip, protocol, port by given FW Mark value (reverse of lookupFWMarkByService) diff --git a/pkg/controllers/proxy/utils_test.go b/pkg/controllers/proxy/utils_test.go index 1c2a3508..89c13006 100644 --- a/pkg/controllers/proxy/utils_test.go +++ b/pkg/controllers/proxy/utils_test.go @@ -87,21 +87,17 @@ func TestNetworkServicesController_lookupFWMarkByService(t *testing.T) { t.Run("ensure existing FW mark is found in lookup", func(t *testing.T) { nsc := getMoqNSC() fwMark1, err1 := nsc.generateUniqueFWMark("10.255.0.1", "TCP", "80") - fwMark2, err2 := nsc.lookupFWMarkByService("10.255.0.1", "TCP", "80") + fwMark2 := nsc.lookupFWMarkByService("10.255.0.1", "TCP", "80") assert.NoError(t, err1, "there shouldn't have been an error calling generateUniqueFWMark") - assert.NoError(t, err2, "there shouldn't have been an error calling lookupFWMarkByService") assert.Equal(t, fwMark1, fwMark2, "given the same inputs, lookupFWMarkByService should be able to find the previously generated FW mark") }) t.Run("ensure error is returned when a service doesn't exist in FW mark map", func(t *testing.T) { nsc := getMoqNSC() - fwMark, err := nsc.lookupFWMarkByService("10.255.0.1", "TCP", "80") + fwMark := nsc.lookupFWMarkByService("10.255.0.1", "TCP", "80") - assert.EqualErrorf(t, err, - fmt.Sprintf("no key matching %s:%s:%s was found in fwMarkMap", "TCP", "10.255.0.1", "80"), - "expected to get an error when service had not yet been added to fwMarkMap") assert.Equal(t, uint32(0), fwMark, "expected FW mark to be 0 on error condition") }) }