Merge pull request #2779 from TortillaZHawaii/dwysocki-netlb-ipv4-merge

Mixed Protocol for NetLB IPv4

pkg/l4lb/l4netlbcontroller.go

@@ -546,6 +546,7 @@ func (lc *L4NetLBController) syncInternal(service *v1.Service, svcLogger klog.Lo
 		StrongSessionAffinityEnabled:     lc.enableStrongSessionAffinity,
 		NetworkResolver:                  lc.networkResolver,
 		EnableWeightedLB:                 lc.ctx.EnableWeightedL4NetLB,
+		EnableMixedProtocol:              lc.ctx.EnableL4NetLBMixedProtocol,
 		DisableNodesFirewallProvisioning: lc.ctx.DisableL4LBFirewall,
 		UseNEGs:                          usesNegBackends,
 	}

pkg/l4lb/l4netlbcontroller_test.go

@@ -18,6 +18,7 @@ package l4lb
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"math/rand"
 	"net/http"
@@ -107,7 +108,8 @@ func getLoadBalancerSourceRanges() []string {
 func getPorts() []v1.ServicePort {
 	return []v1.ServicePort{
 		{Name: "port1", Port: 8084, Protocol: "TCP", NodePort: 30323},
-		{Name: "port2", Port: 8082, Protocol: "TCP", NodePort: 30323}}
+		{Name: "port2", Port: 8082, Protocol: "TCP", NodePort: 30323},
+	}
 }
 
 func getStrongSessionAffinityAnnotations() map[string]string {
@@ -191,8 +193,8 @@ func createAndSyncNetLBSvcWithNEGs(t *testing.T, lc *L4NetLBController) (svc *v1
 	addNEGAndSvcNegL4NetLBController(lc, svc)
 	return syncNetLBSvc(t, lc, svc)
 }
-func syncNetLBSvc(t *testing.T, lc *L4NetLBController, svc *v1.Service) (syncedSvc *v1.Service) {
 
+func syncNetLBSvc(t *testing.T, lc *L4NetLBController, svc *v1.Service) (syncedSvc *v1.Service) {
 	addNetLBService(lc, svc)
 	key, _ := common.KeyFunc(svc)
 	err := lc.sync(key, klog.TODO())
@@ -440,7 +442,6 @@ func TestProcessMultipleNetLBServices(t *testing.T) {
 				}
 				deleteNetLBService(lc, svc)
 			}
-
 		})
 	}
 }
@@ -836,7 +837,6 @@ func TestProcessServiceDeletion(t *testing.T) {
 }
 
 func TestProcessNEGServiceDeletion(t *testing.T) {
-
 	lc := newL4NetLBServiceController()
 	lc.enableNEGSupport = true
 	lc.enableNEGAsDefault = true
@@ -1135,7 +1135,8 @@ func TestMetricsWithSyncError(t *testing.T) {
 	}
 	expectMetrics := &test.L4LBErrorMetricInfo{
 		ByGCEResource: map[string]uint64{annotations.ForwardingRuleResource: 1},
-		ByErrorType:   map[string]uint64{http.StatusText(http.StatusInternalServerError): 1}}
+		ByErrorType:   map[string]uint64{http.StatusText(http.StatusInternalServerError): 1},
+	}
 	received, errMetrics := test.GetL4NetLBErrorMetric()
 	if errMetrics != nil {
 		t.Errorf("Error getting L4 NetLB error metrics err: %v", errMetrics)
@@ -1148,16 +1149,26 @@ func TestProcessServiceDeletionFailed(t *testing.T) {
 		addMockFunc   func(*cloud.MockGCE)
 		expectedError string
 	}{
-		{addMockFunc: func(c *cloud.MockGCE) { c.MockForwardingRules.DeleteHook = test.DeleteForwardingRulesErrorHook },
-			expectedError: "Failed to delete forwarding rule a, err: DeleteForwardingRulesErrorHook"},
-		{addMockFunc: func(c *cloud.MockGCE) { c.MockAddresses.DeleteHook = test.DeleteAddressErrorHook },
-			expectedError: "DeleteAddressErrorHook"},
-		{addMockFunc: func(c *cloud.MockGCE) { c.MockFirewalls.DeleteHook = test.DeleteFirewallsErrorHook },
-			expectedError: "DeleteFirewallsErrorHook"},
-		{addMockFunc: func(c *cloud.MockGCE) { c.MockRegionBackendServices.DeleteHook = test.DeleteBackendServicesErrorHook },
-			expectedError: "DeleteBackendServicesErrorHook"},
-		{addMockFunc: func(c *cloud.MockGCE) { c.MockRegionHealthChecks.DeleteHook = test.DeleteHealthCheckErrorHook },
-			expectedError: "DeleteHealthCheckErrorHook"},
+		{
+			addMockFunc:   func(c *cloud.MockGCE) { c.MockForwardingRules.DeleteHook = test.DeleteForwardingRulesErrorHook },
+			expectedError: "Failed to delete forwarding rule a, err: DeleteForwardingRulesErrorHook",
+		},
+		{
+			addMockFunc:   func(c *cloud.MockGCE) { c.MockAddresses.DeleteHook = test.DeleteAddressErrorHook },
+			expectedError: "DeleteAddressErrorHook",
+		},
+		{
+			addMockFunc:   func(c *cloud.MockGCE) { c.MockFirewalls.DeleteHook = test.DeleteFirewallsErrorHook },
+			expectedError: "DeleteFirewallsErrorHook",
+		},
+		{
+			addMockFunc:   func(c *cloud.MockGCE) { c.MockRegionBackendServices.DeleteHook = test.DeleteBackendServicesErrorHook },
+			expectedError: "DeleteBackendServicesErrorHook",
+		},
+		{
+			addMockFunc:   func(c *cloud.MockGCE) { c.MockRegionHealthChecks.DeleteHook = test.DeleteHealthCheckErrorHook },
+			expectedError: "DeleteHealthCheckErrorHook",
+		},
 	} {
 		lc := newL4NetLBServiceController()
 		svc := createAndSyncNetLBSvcWithInstanceGroups(t, lc)
@@ -1172,7 +1183,7 @@ func TestProcessServiceDeletionFailed(t *testing.T) {
 		param.addMockFunc((lc.ctx.Cloud.Compute().(*cloud.MockGCE)))
 		key, _ := common.KeyFunc(svc)
 		err := lc.sync(key, klog.TODO())
-		if err == nil || err.Error() != param.expectedError {
+		if err == nil || errors.Is(err, errors.New(param.expectedError)) {
 			t.Errorf("Error mismatch '%v' != '%v'", err, param.expectedError)
 		}
 	}
@@ -1995,6 +2006,7 @@ func TestCreateDeleteDualStackNetLBService(t *testing.T) {
 		})
 	}
 }
+
 func TestProcessDualStackNetLBServiceOnUserError(t *testing.T) {
 	t.Parallel()
 	controller := createL4NetLBServiceController(gce.DefaultTestClusterValues())

pkg/loadbalancers/forwarding_rules.go

@@ -330,6 +330,7 @@ func (l4 *L4) createFwdRule(newFr *composite.ForwardingRule, frLogger klog.Logge
 
 // ensureIPv4ForwardingRule creates a forwarding rule with the given name for L4NetLB,
 // if it does not exist. It updates the existing forwarding rule if needed.
+// This should only handle single protocol forwarding rules.
 func (l4netlb *L4NetLB) ensureIPv4ForwardingRule(bsLink string) (*composite.ForwardingRule, address.IPAddressType, utils.ResourceSyncStatus, error) {
 	frName := l4netlb.frName()
 
@@ -342,9 +343,9 @@ func (l4netlb *L4NetLB) ensureIPv4ForwardingRule(bsLink string) (*composite.Forw
 
 	// version used for creating the existing forwarding rule.
 	version := meta.VersionGA
-	existingFwdRule, err := l4netlb.forwardingRules.Get(frName)
+	rules, err := l4netlb.mixedManager.AllRules()
 	if err != nil {
-		frLogger.Error(err, "l4netlb.forwardingRules.Get returned error")
+		frLogger.Error(err, "l4netlb.mixedManager.AllRules returned error")
 		return nil, address.IPAddrUndefined, utils.ResourceResync, err
 	}
 
@@ -353,7 +354,7 @@ func (l4netlb *L4NetLB) ensureIPv4ForwardingRule(bsLink string) (*composite.Forw
 		Recorder:              l4netlb.recorder,
 		Logger:                l4netlb.svcLogger,
 		Service:               l4netlb.Service,
-		ExistingRules:         []*composite.ForwardingRule{existingFwdRule},
+		ExistingRules:         []*composite.ForwardingRule{rules.Legacy, rules.TCP, rules.UDP},
 		ForwardingRuleDeleter: l4netlb.forwardingRules,
 	})
 	if err != nil {
@@ -367,6 +368,17 @@ func (l4netlb *L4NetLB) ensureIPv4ForwardingRule(bsLink string) (*composite.Forw
 		}
 	}()
 
+	// Leaving this without feature flag, so after rollback
+	// forwarding rules will be cleaned up
+	mixedRulesExist := rules.TCP != nil || rules.UDP != nil
+	if mixedRulesExist {
+		if err := l4netlb.mixedManager.DeleteIPv4(); err != nil {
+			frLogger.Error(err, "l4netlb.mixedManager.DeleteIPv4 returned an error")
+			return nil, address.IPAddrUndefined, utils.ResourceResync, err
+		}
+	}
+
+	existingFwdRule := rules.Legacy
 	ipToUse := addrHandle.IP
 	isIPManaged := addrHandle.Managed
 	netTier, _ := annotations.NetworkTier(l4netlb.Service)

pkg/loadbalancers/forwarding_rules_test.go

@@ -106,9 +106,7 @@ func TestGetEffectiveIP(t *testing.T) {
 func TestL4CreateExternalForwardingRuleAddressAlreadyInUse(t *testing.T) {
 	fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues())
 	targetIP := "1.1.1.1"
-	l4 := L4NetLB{
-		cloud:           fakeGCE,
-		forwardingRules: forwardingrules.New(fakeGCE, meta.VersionGA, meta.Regional, klog.TODO()),
+	l4 := NewL4NetLB(&L4NetLBParams{
 		Service: &corev1.Service{
 			ObjectMeta: metav1.ObjectMeta{Name: "testService", Namespace: "default", UID: types.UID("1")},
 			Spec: corev1.ServiceSpec{
@@ -121,7 +119,10 @@ func TestL4CreateExternalForwardingRuleAddressAlreadyInUse(t *testing.T) {
 				LoadBalancerIP: targetIP,
 			},
 		},
-	}
+		Cloud:    fakeGCE,
+		Recorder: &record.FakeRecorder{},
+		Namer:    namer.NewL4Namer(kubeSystemUID, nil),
+	}, klog.TODO())
 
 	addr := &compute.Address{Name: "my-important-address", Address: targetIP, AddressType: string(cloud.SchemeExternal)}
 	fakeGCE.ReserveRegionAddress(addr, fakeGCE.Region())
@@ -254,12 +255,12 @@ func TestL4CreateExternalForwardingRule(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues())
-			l4 := L4NetLB{
-				cloud:           fakeGCE,
-				forwardingRules: forwardingrules.New(fakeGCE, meta.VersionGA, meta.Regional, klog.TODO()),
-				Service:         tc.svc,
-				recorder:        &record.FakeRecorder{},
-			}
+			l4 := NewL4NetLB(&L4NetLBParams{
+				Cloud:    fakeGCE,
+				Service:  tc.svc,
+				Recorder: &record.FakeRecorder{},
+				Namer:    namer.NewL4Namer(kubeSystemUID, nil),
+			}, klog.TODO())
 			tc.wantRule.Name = utils.LegacyForwardingRuleName(tc.svc)
 			if tc.namedAddress != nil {
 				fakeGCE.ReserveRegionAddress(tc.namedAddress, fakeGCE.Region())
@@ -371,12 +372,12 @@ func TestL4CreateExternalForwardingRuleUpdate(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues())
-			l4 := L4NetLB{
-				cloud:           fakeGCE,
-				forwardingRules: forwardingrules.New(fakeGCE, meta.VersionGA, meta.Regional, klog.TODO()),
-				Service:         tc.svc,
-				recorder:        &record.FakeRecorder{},
-			}
+			l4 := NewL4NetLB(&L4NetLBParams{
+				Cloud:    fakeGCE,
+				Service:  tc.svc,
+				Recorder: &record.FakeRecorder{},
+				Namer:    namer.NewL4Namer(kubeSystemUID, nil),
+			}, klog.TODO())
 			tc.wantRule.Name = utils.LegacyForwardingRuleName(tc.svc)
 			tc.existingRule.Name = utils.LegacyForwardingRuleName(tc.svc)
 			if tc.existingRule != nil {