Add support for NLB security group #3329
Conversation
sg rules restriction on NLB
remove discovery from default code path, not needed for NLB with SG
k8s-ci-robot
added
cncf-cla: yes
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3329 +/- ##
==========================================
+ Coverage 54.71% 55.61% +0.90%
==========================================
Files 148 149 +1
Lines 8626 8819 +193
==========================================
+ Hits 4720 4905 +185
- Misses 3574 3577 +3
- Partials 332 337 +5
☔ View full report in Codecov by Sentry. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: M00nF1sh, oliviassss 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 |
k8s-ci-robot
added
the
approved
| } | ||
| if !t.featureGates.Enabled(config.NLBSecurityGroup) { | ||
| if existingLB != nil && len(existingLB.LoadBalancer.SecurityGroups) != 0 { | ||
| return nil, errors.New("conflicting security groups configuration") |
There was a problem hiding this comment.
This is not a terribly helpful error message, as it gives no clue as to what is conflicting. Better to say something like "existing NLB has security groups attached, but NLBSecurityGroup feature gate is disabled"
| func (t *defaultModelBuildTask) buildLoadBalancerSecurityGroups(ctx context.Context, existingLB *elbv2deploy.LoadBalancerWithTags, | ||
| ipAddressType elbv2model.IPAddressType) ([]core.StringToken, error) { | ||
| if existingLB != nil && len(existingLB.LoadBalancer.SecurityGroups) == 0 { | ||
| return nil, nil |
There was a problem hiding this comment.
Should it not be possible to add a SG to an existing NLB by explicitly setting the relevant annotation?
| ctx := context.Background() | ||
| stack, _, err := builder.Build(ctx, tt.svc) | ||
| stack, _, _, err := builder.Build(ctx, tt.svc) | ||
| if tt.wantError { |
There was a problem hiding this comment.
wantError should be more specific: it should be a string and should assert.EqualError()
| ctx := context.Background() | ||
| stack, _, err := builder.Build(ctx, tt.svc) | ||
| stack, _, _, err := builder.Build(ctx, tt.svc) |
There was a problem hiding this comment.
Should probably be doing asserts on the returned value of backendSGRequired
Issue
Description
Added support of Security Groups for NLB. With the security group support, it is feasible to forward the NLB traffic to the EC2 instances without having to open up the instances for global access. For backwards compatibility, NLBs created without the security groups or the existing NLBs will continue to provide the legacy behavior. Similar to ALB, there are two sets of SGs for NLB - frontend and backend SGs:
inbound-cidrsandlisten-ports. If the users want to attach existing frontend SG to the NLB, they can explicitly specify via annotationservice.beta.kubernetes.io/aws-load-balancer-security-groupsservice.beta.kubernetes.io/aws-load-balancer-manage-backend-security-group-rules: true/falseChecklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯