Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mixed Protocol for NetLB IPv4 #2779

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Mixed Protocol for NetLB IPv4 #2779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2bd32c3
Update existing L4NetLB to use MixedNetLBManager
TortillaZHawaii Dec 30, 2024
14291a9
wip
TortillaZHawaii Dec 30, 2024
6683686
Refactor common code from mixedprotocoltest
TortillaZHawaii Jan 3, 2025
8a79d30
Found health check issue
TortillaZHawaii Jan 3, 2025
93dfe7e
Fix health check missing in test
TortillaZHawaii Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/l4lb/l4netlbcontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
Expand Down
44 changes: 28 additions & 16 deletions pkg/l4lb/l4netlbcontroller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package l4lb

import (
"context"
"errors"
"fmt"
"math/rand"
"net/http"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -440,7 +442,6 @@ func TestProcessMultipleNetLBServices(t *testing.T) {
}
deleteNetLBService(lc, svc)
}

})
}
}
Expand Down Expand Up @@ -836,7 +837,6 @@ func TestProcessServiceDeletion(t *testing.T) {
}

func TestProcessNEGServiceDeletion(t *testing.T) {

lc := newL4NetLBServiceController()
lc.enableNEGSupport = true
lc.enableNEGAsDefault = true
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
}
}
Expand Down Expand Up @@ -1995,6 +2006,7 @@ func TestCreateDeleteDualStackNetLBService(t *testing.T) {
})
}
}

func TestProcessDualStackNetLBServiceOnUserError(t *testing.T) {
t.Parallel()
controller := createL4NetLBServiceController(gce.DefaultTestClusterValues())
Expand Down
18 changes: 15 additions & 3 deletions pkg/loadbalancers/forwarding_rules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand All @@ -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
}

Expand All @@ -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 {
Expand All @@ -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)
Expand Down
33 changes: 17 additions & 16 deletions pkg/loadbalancers/forwarding_rules_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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())
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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 {
Expand Down
Loading