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

Jump to bottom

NEG controller part - L4 RBS External LB NEG support #2566

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

NEG controller part - L4 RBS External LB NEG support #2566

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d117176
Add flags for L4 RBS NetLB NEGs.
mmamczur May 23, 2024
03463b1
NetLBFinalizerV3 for NEG variant of L4 RBS NetLB.
mmamczur May 23, 2024
09281f9
NEG Controller support for L4 NetLB.
mmamczur May 23, 2024
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 cmd/glbc/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ func createNEGController(ctx *ingctx.ControllerContext, stopCh <-chan struct{},
lpConfig,
flags.F.EnableMultiNetworking,
ctx.EnableIngressRegionalExternal,
flags.F.EnableL4NetLBNEG,
stopCh,
logger,
)
Expand Down
4 changes: 4 additions & 0 deletions pkg/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ var (
EnableL4NetLBDualStack bool
EnableNEGController bool
EnableL4NEG bool
EnableL4NetLBNEG bool
EnableL4NetLBNEGDefault bool
GateNEGByLock bool
EnableMultipleIGs bool
EnableL4StrongSessionAffinity bool
Expand Down Expand Up @@ -281,6 +283,8 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5
flag.BoolVar(&F.RunL4NetLBController, "run-l4-netlb-controller", false, `Optional, if enabled then the L4NetLbController will be run.`)
flag.BoolVar(&F.EnableNEGController, "enable-neg-controller", true, `Optional, if enabled then the NEG controller will be run.`)
flag.BoolVar(&F.EnableL4NEG, "enable-l4-neg", false, `Optional, if enabled then the NEG controller will process L4 NEGs.`)
flag.BoolVar(&F.EnableL4NetLBNEG, "enable-l4-netlb-neg", false, `Optional, if enabled then the NetLB controller can create L4 NetLB services with NEG backends.`)
flag.BoolVar(&F.EnableL4NetLBNEGDefault, "enable-l4-netlb-neg-default", false, `Optional, if enabled then newly created L4 NetLB services will use NEG backends. Has effect only if '--enable-l4-netlb-neg' is set to true.`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do these interact with EnableL4NEG? I think there should be a requirement that EnableL4NEG is enabled too for these new flags

flag.BoolVar(&F.GateNEGByLock, "gate-neg-by-lock", false, "If enabled then the NEG controller will be run via leader election with NEG resource lock")
flag.BoolVar(&F.EnableIGController, "enable-ig-controller", true, `Optional, if enabled then the IG controller will be run.`)
flag.BoolVar(&F.EnablePSC, "enable-psc", false, "Enable PSC controller")
Expand Down
48 changes: 39 additions & 9 deletions pkg/neg/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,16 @@ type Controller struct {
// syncerMetrics collects NEG controller metrics
syncerMetrics *syncMetrics.SyncerMetrics

// runL4 indicates whether to run NEG controller that processes L4 services
runL4 bool
// runL4ForILB indicates whether to run NEG controller that processes L4 ILB services
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that this is to preserve the existing behavior and be able to gate the new logic.

nit, the existing behavior also turns on NEGs for multi-networking which isn't captured by this boolean.

runL4ForILB bool

// enableIngressRegionalExternal indicates where NEG controller should process
// gce-regional-external ingresses
enableIngressRegionalExternal bool

// runL4ForNetLB indicates if the controller can create NEGs for L4 NetLB services.
runL4ForNetLB bool

stopCh <-chan struct{}
logger klog.Logger
}
Expand Down Expand Up @@ -140,6 +143,7 @@ func NewController(
lpConfig labels.PodLabelPropagationConfig,
enableMultiNetworking bool,
enableIngressRegionalExternal bool,
runL4ForNetLB bool,
stopCh <-chan struct{},
logger klog.Logger,
) *Controller {
Expand Down Expand Up @@ -231,8 +235,9 @@ func NewController(
syncTracker: utils.NewTimeTracker(),
reflector: reflector,
syncerMetrics: syncerMetrics,
runL4: runL4Controller,
runL4ForILB: runL4Controller,
enableIngressRegionalExternal: enableIngressRegionalExternal,
runL4ForNetLB: runL4ForNetLB,
stopCh: stopCh,
logger: logger,
}
Expand Down Expand Up @@ -503,7 +508,8 @@ func (c *Controller) processService(key string) error {
}
}

if c.runL4 {
// Create L4 PortInfo if subsetting is enable `runL4ForILB==true` or RBS NEG support is enabled `runL4ForNetLB==true`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or if multi networking is enabled?

if c.runL4ForILB || c.runL4ForNetLB {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we check for the variables again in the utils function, do we need the check here still?

if err := c.mergeVmIpNEGsPortInfo(service, types.NamespacedName{Namespace: namespace, Name: name}, svcPortInfoMap, &negUsage, networkInfo); err != nil {
return err
}
Expand Down Expand Up @@ -597,13 +603,13 @@ func (c *Controller) mergeStandaloneNEGsPortInfo(service *apiv1.Service, name ty
// mergeVmIpNEGsPortInfo merges the PortInfo for ILB and multinet NetLB services using GCE_VM_IP NEGs into portInfoMap
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: update the comments to include NEG default

func (c *Controller) mergeVmIpNEGsPortInfo(service *apiv1.Service, name types.NamespacedName, portInfoMap negtypes.PortInfoMap, negUsage *metricscollector.NegServiceState, networkInfo *network.NetworkInfo) error {
wantsILB, _ := annotations.WantsL4ILB(service)
wantsNetLB, _ := annotations.WantsL4NetLB(service)
needsNEGForNetLB := wantsNetLB && !networkInfo.IsDefault && annotations.HasRBSAnnotation(service)
if !wantsILB && !needsNEGForNetLB {
needsNEGForILB := c.runL4ForILB && wantsILB
needsNEGForNetLB := c.netLBServiceNeedsNEG(service, networkInfo)
if !needsNEGForILB && !needsNEGForNetLB {
return nil
}
// Only process ILB services after L4 controller has marked it with v2 finalizer.
if wantsILB && !utils.IsSubsettingL4ILBService(service) {
if needsNEGForILB && !utils.IsSubsettingL4ILBService(service) {
msg := fmt.Sprintf("Ignoring ILB Service %s, namespace %s as it does not have the v2 finalizer", service.Name, service.Namespace)
c.logger.Info(msg)
c.recorder.Eventf(service, apiv1.EventTypeWarning, "ProcessServiceSkipped", msg)
Expand All @@ -619,8 +625,32 @@ func (c *Controller) mergeVmIpNEGsPortInfo(service *apiv1.Service, name types.Na
onlyLocal := helpers.RequestsOnlyLocalTraffic(service)
// Update usage metrics.
negUsage.VmIpNeg = metricscollector.NewVmIpNegType(onlyLocal)
var l4LBType negtypes.L4LBType
if needsNEGForILB {
l4LBType = negtypes.L4InternalLB
} else {
l4LBType = negtypes.L4ExternalLB
}

return portInfoMap.Merge(negtypes.NewPortInfoMapForVMIPNEG(name.Namespace, name.Name, c.l4Namer, onlyLocal, networkInfo))
return portInfoMap.Merge(negtypes.NewPortInfoMapForVMIPNEG(name.Namespace, name.Name, c.l4Namer, onlyLocal, networkInfo, l4LBType))
}

// netLBServiceNeedsNEG determines if NEGs need to be created for L4 NetLB.
// - service must be an L4 External Load Balancer service
// - service must have the RBS annotation
// - service is a multinetwork service on a non default network OR NEGs are enabled and V3 finalizer is present.
func (c *Controller) netLBServiceNeedsNEG(service *apiv1.Service, networkInfo *network.NetworkInfo) bool {
wantsNetLB, _ := annotations.WantsL4NetLB(service)
if !wantsNetLB {
return false
}
if !annotations.HasRBSAnnotation(service) {
return false
}
if !networkInfo.IsDefault {
return true
}
return c.runL4ForNetLB && utils.HasL4NetLBFinalizerV3(service)
}

// mergeDefaultBackendServicePortInfoMap merge the PortInfoMap for the default backend service into portInfoMap
Expand Down
55 changes: 49 additions & 6 deletions pkg/neg/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ func newTestControllerWithParamsAndContext(kubeClient kubernetes.Interface, test
labels.PodLabelPropagationConfig{},
true,
false,
false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add tests where this is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a test case to verify combinations of this being turned off etc

make(<-chan struct{}),
klog.TODO(),
)
Expand Down Expand Up @@ -388,7 +389,7 @@ func TestEnableNEGServiceWithL4ILB(t *testing.T) {
if err != nil {
t.Fatalf("Service was not created.(*apiv1.Service) successfully, err: %v", err)
}
expectedPortInfoMap := negtypes.NewPortInfoMapForVMIPNEG(testServiceNamespace, testServiceName, controller.l4Namer, false, defaultNetwork)
expectedPortInfoMap := negtypes.NewPortInfoMapForVMIPNEG(testServiceNamespace, testServiceName, controller.l4Namer, false, defaultNetwork, negtypes.L4InternalLB)
// There will be only one entry in the map
for key, val := range expectedPortInfoMap {
prevSyncerKey = manager.getSyncerKey(testServiceNamespace, testServiceName, key, val)
Expand All @@ -409,7 +410,7 @@ func TestEnableNEGServiceWithL4ILB(t *testing.T) {
if err = controller.processService(svcKey); err != nil {
t.Fatalf("Failed to process updated L4 ILB service: %v", err)
}
expectedPortInfoMap = negtypes.NewPortInfoMapForVMIPNEG(testServiceNamespace, testServiceName, controller.l4Namer, true, defaultNetwork)
expectedPortInfoMap = negtypes.NewPortInfoMapForVMIPNEG(testServiceNamespace, testServiceName, controller.l4Namer, true, defaultNetwork, negtypes.L4InternalLB)
// There will be only one entry in the map
for key, val := range expectedPortInfoMap {
updatedSyncerKey = manager.getSyncerKey(testServiceNamespace, testServiceName, key, val)
Expand Down Expand Up @@ -1258,13 +1259,16 @@ func TestMergeVmIpNEGsPortInfo(t *testing.T) {
desc string
svc *apiv1.Service
networkInfo *network.NetworkInfo
runL4ILB bool
runL4NetLB bool
wantSvcPortMap negtypes.PortInfoMap
}{
{
desc: "ILB subsetting service",
svc: serviceILBWithFinalizer,
networkInfo: defaultNetwork,
wantSvcPortMap: negtypes.NewPortInfoMapForVMIPNEG(testServiceNamespace, testServiceName, controller.l4Namer, false, defaultNetwork),
runL4ILB: true,
wantSvcPortMap: negtypes.NewPortInfoMapForVMIPNEG(testServiceNamespace, testServiceName, controller.l4Namer, false, defaultNetwork, negtypes.L4InternalLB),
},
{
desc: "ILB legacy service",
Expand All @@ -1276,14 +1280,28 @@ func TestMergeVmIpNEGsPortInfo(t *testing.T) {
desc: "RBS Multinet Service",
svc: newTestRBSMultinetService(controller, true, 80),
networkInfo: secondaryNetwork,
wantSvcPortMap: negtypes.NewPortInfoMapForVMIPNEG(testServiceNamespace, testServiceName, controller.l4Namer, true, secondaryNetwork),
wantSvcPortMap: negtypes.NewPortInfoMapForVMIPNEG(testServiceNamespace, testServiceName, controller.l4Namer, true, secondaryNetwork, negtypes.L4ExternalLB),
},
{
desc: "RBS non-multinet Service",
svc: newTestRBSMultinetService(controller, true, 80),
svc: newTestRBSService(controller, true, 80, common.NetLBFinalizerV2),
networkInfo: defaultNetwork,
wantSvcPortMap: nil,
},
{
desc: "RBS non-multinet Service with NEG",
svc: newTestRBSService(controller, true, 80, common.NetLBFinalizerV3),
networkInfo: defaultNetwork,
runL4NetLB: true,
wantSvcPortMap: negtypes.NewPortInfoMapForVMIPNEG(testServiceNamespace, testServiceName, controller.l4Namer, true, defaultNetwork, negtypes.L4ExternalLB),
},
{
desc: "RBS non-multinet Service with NEG but NEGs not enabled for NetLB",
svc: newTestRBSService(controller, true, 80, common.NetLBFinalizerV3),
networkInfo: defaultNetwork,
runL4NetLB: false,
wantSvcPortMap: nil,
},
{
desc: "Service with load balancer class",
svc: serviceWithLoadBalancerClass,
Expand All @@ -1294,6 +1312,8 @@ func TestMergeVmIpNEGsPortInfo(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
controller.runL4ForNetLB = tc.runL4NetLB
controller.runL4ForILB = tc.runL4ILB
portInfoMap := make(negtypes.PortInfoMap)
negUsage := metricscollector.NegServiceState{}
controller.mergeVmIpNEGsPortInfo(tc.svc, types.NamespacedName{Namespace: tc.svc.Namespace, Name: tc.svc.Name}, portInfoMap, &negUsage, tc.networkInfo)
Expand Down Expand Up @@ -1609,7 +1629,7 @@ func TestEnableNEGServiceWithL4NetLB(t *testing.T) {
if err != nil {
t.Fatalf("Service was not created.(*apiv1.Service) successfully, err: %v", err)
}
expectedPortInfoMap := negtypes.NewPortInfoMapForVMIPNEG(testServiceNamespace, testServiceName, controller.l4Namer, true, networkInfo)
expectedPortInfoMap := negtypes.NewPortInfoMapForVMIPNEG(testServiceNamespace, testServiceName, controller.l4Namer, true, networkInfo, negtypes.L4ExternalLB)
// There will be only one entry in the map
for key, val := range expectedPortInfoMap {
prevSyncerKey = manager.getSyncerKey(testServiceNamespace, testServiceName, key, val)
Expand Down Expand Up @@ -2020,6 +2040,29 @@ func newTestRBSMultinetService(c *Controller, onlyLocal bool, port int) *apiv1.S
return svc
}

func newTestRBSService(c *Controller, onlyLocal bool, port int, finalizer string) *apiv1.Service {
svc := &apiv1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: testServiceName,
Namespace: testServiceNamespace,
Annotations: map[string]string{annotations.RBSAnnotationKey: annotations.RBSEnabled},
Finalizers: []string{finalizer},
},
Spec: apiv1.ServiceSpec{
Type: apiv1.ServiceTypeLoadBalancer,
Ports: []apiv1.ServicePort{
{Name: "testport", Port: int32(port)},
},
},
}
if onlyLocal {
svc.Spec.ExternalTrafficPolicy = apiv1.ServiceExternalTrafficPolicyTypeLocal
}

c.client.CoreV1().Services(testServiceNamespace).Create(context.TODO(), svc, metav1.CreateOptions{})
return svc
}

func newTestService(c *Controller, negIngress bool, negSvcPorts []int32) *apiv1.Service {
svcAnnotations := map[string]string{}
if negIngress || len(negSvcPorts) > 0 {
Expand Down
1 change: 1 addition & 0 deletions pkg/neg/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ func (manager *syncerManager) EnsureSyncers(namespace, name string, newPorts neg
manager.enableDualStackNEG,
manager.syncerMetrics,
&portInfo.NetworkInfo,
portInfo.L4LBType,
)
syncer = negsyncer.NewTransactionSyncer(
syncerKey,
Expand Down
Loading