Enable frontend NLB #4126
Enable frontend NLB #4126
Conversation
k8s-ci-robot
added
cncf-cla: yes
| @@ -152,7 +152,8 @@ func (r *serviceReconciler) buildModel(ctx context.Context, svc *corev1.Service) | |||
| } | |||
|
|
|||
| func (r *serviceReconciler) deployModel(ctx context.Context, svc *corev1.Service, stack core.Stack) error { | |||
| if err := r.stackDeployer.Deploy(ctx, stack, r.metricsCollector, "service"); err != nil { | |||
| albTargetGroupDesiredState := core.NewFrontendNlbTargetGroupDesiredState() | |||
There was a problem hiding this comment.
curious what this is for?
There was a problem hiding this comment.
It is not needed for Service. Updated it to pass nil instead
| logger: logger, | ||
| stack: stack, | ||
| frontendNlbTargetGroupDesiredState: frontendNlbTargetGroupDesiredState, | ||
| unmatchedResTGs: nil, |
There was a problem hiding this comment.
nit: no need to specify nil
There was a problem hiding this comment.
What's the purpose of unmatchedResTGs?
There was a problem hiding this comment.
It is not used. Removed it
| } | ||
|
|
||
| for _, sdkTG := range unmatchedSDKTGs { | ||
| if sdkTG.TargetGroup.TargetType != "alb" { |
There was a problem hiding this comment.
Please use elbv2types.TargetTypeEnumAlb :)
| func filterALBTargetGroups(targetGroups []*elbv2model.TargetGroup) []*elbv2model.TargetGroup { | ||
| var filteredTargetGroups []*elbv2model.TargetGroup | ||
| for _, tg := range targetGroups { | ||
| if tg.Spec.TargetType == "alb" { |
There was a problem hiding this comment.
same - elbv2types.TargetTypeEnumAlb
| stackTagsLegacy := s.trackingProvider.StackTagsLegacy(s.stack) | ||
| return s.taggingManager.ListTargetGroups(ctx, | ||
| tracking.TagsAsTagFilter(stackTags), | ||
| tracking.TagsAsTagFilter(stackTagsLegacy)) |
There was a problem hiding this comment.
Is stack tagging legacy needed as this is a new feature?
There was a problem hiding this comment.
Good catch. Removed it
| } | ||
|
|
||
| func isALbInactiveError(err error) bool { | ||
| var awsErr *elbv2types.InvalidTargetException |
There was a problem hiding this comment.
I think we might want to tread carefully here..
https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-aws-sdk-client-elastic-load-balancing-v2/Class/InvalidTargetException/
The InvalidTargetException can be used for a lot of things.
I think it might be better to ignore the error and continue on with the rest of the reconcilers. When all other reconcilers complete, we could return a "please reconcile again in 15 seconds" error to ensure we keep retrying the alb registration.
There was a problem hiding this comment.
Agree, updated it to ignore the error, and requeue after reconcilation
pkg/deploy/stack_deployer.go
Outdated
| synthesizers := []ResourceSynthesizer{ | ||
| ec2.NewSecurityGroupSynthesizer(d.cloud.EC2(), d.trackingProvider, d.ec2TaggingManager, d.ec2SGManager, d.vpcID, d.logger, stack), | ||
| } | ||
|
|
||
| if controllerName == "ingress" { |
There was a problem hiding this comment.
haha! Nice. Maybe use a constant here to ensure that we don't have any weird casing issues.
There was a problem hiding this comment.
Sure, updated it to use const
| } | ||
|
|
||
| func (t *defaultModelBuildTask) buildEnableFrontendNlbViaAnnotation(ctx context.Context) (bool, bool, error) { | ||
| explicitEnableFrontendNlb := make(map[bool]struct{}) |
There was a problem hiding this comment.
This is a very confusing way to see if there's annotations with differing values.
There was a problem hiding this comment.
Yeah, it was confusing. I updated it to use a pointer
| return t.defaultScheme, nil | ||
| } | ||
|
|
||
| func (t *defaultModelBuildTask) buildFrontendNlbSubnetMappings(ctx context.Context, scheme elbv2model.LoadBalancerScheme) ([]elbv2model.SubnetMapping, error) { |
There was a problem hiding this comment.
Did you consider re-using the svc / ingress subnet mapping builder? It would be good to reduce code duplication.
There was a problem hiding this comment.
Yes, But I think they are using different annotation. Also existing svc/ingress handle more complex logic.
| return elbv2model.LoadBalancerSpec{}, err | ||
| } | ||
|
|
||
| // use alb security group if it is not explicitly specified |
There was a problem hiding this comment.
Do you want to some kind of validation that ensures the ALB alb sg / subnets are compatible with the NLB? It's ok to say we will differ the validation to ELB API :)
There was a problem hiding this comment.
Yes, I though we can differ to API. But we can decide if we want to add it after bugbash.
| } | ||
|
|
||
| func (t *defaultModelBuildTask) buildFrontendNlbListeners(ctx context.Context, albListenerPorts []int32) error { | ||
| FrontendNlbListenerConfigsByPort := make(map[int32][]FrontendNlbListenConfigWithIngress) |
There was a problem hiding this comment.
conventionally, it would be better to start this var with a lower case letter.
There was a problem hiding this comment.
Updated all lower case.
| // build frontend nlb config by port for ingress | ||
| for _, member := range t.ingGroup.Members { | ||
| ingKey := k8s.NamespacedName(member.Ing) | ||
| FrontendNlbListenerConfigByPortForIngress, err := t.buildFrontendNlbListenerConfigByPortForIngress(ctx, &member, albListenerPorts) |
There was a problem hiding this comment.
same, local vars should be lower case.
| finalConfig.HealthCheckConfig.TimeoutSeconds = healthCheckConfig.TimeoutSeconds | ||
| } | ||
|
|
||
| if explicit["HealthyThresholdCount"] { |
There was a problem hiding this comment.
It looks like this block of code is repeated a couple times with differing hash map keys. Can you refactor this into a helper function that takes in the necessary parameters?
There was a problem hiding this comment.
Yeah, added helper to cover all expect protocol.
| return nil, err | ||
| } | ||
|
|
||
| FrontendNlbListenerResID := fmt.Sprintf("FrontendNlb-ls-%v-%v", config.Protocol, port) |
There was a problem hiding this comment.
nit: helper function for this
| } | ||
|
|
||
| func (t *defaultModelBuildTask) buildFrontendNlbTargetGroup(ctx context.Context, port int32, config FrontendNlbListenerConfig) (*elbv2model.TargetGroup, error) { | ||
| FrontendNlbtgResID := fmt.Sprintf("FrontendNlb-tg-%v-%v", config.Protocol, port) |
|
Update:
Fix
|
| if len(a) != len(b) { | ||
| return false | ||
| } | ||
| for i := range a { |
There was a problem hiding this comment.
do we care about ordering here? Might better to use two sets to compare ingress status equality but it's probably not a big deal.
There was a problem hiding this comment.
Agree, updated it to use two sets to compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wweiwei-li, zac-nixon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
wweiwei-li
force-pushed
the
frontendNlb
branch
2 times, most recently
from
9809175 to
919118f
Compare
Issue
#2297
Description
With this feature , the controller will automatically provision a Network Load Balancer and register the Application Load Balancer as its target. Additional annotations are available to customize the NLB configurations, including options for scheme, security groups, subnets, and health check. The ingress will expose both the ALB DNS name and NLB DNS name. This allows users to combine the benefits of NLB and ALB into a single solution, leveraging NLB features like static IP address and PrivateLink, while retaining the rich routing capabilities of ALB.
Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯