Add support for optionally enforcing NLB security groups on PrivateLi… #3594

wweiwei-li · 2024-02-29T23:30:16Z

…nk traffic

Issue

Description

This is a feature to support optionally enforce NLB security groups on PrivateLink traffic. For the change, added a new annotation aws-load-balancer-inbound-sg-rules-on-private-link-traffic to configure whether to apply security group rules to traffic sent to the load balancer through AWS PrivateLink. The value of it can be set to "off" and "on". After creating new NLB, will check if the annotation is specified. If yes, will call updateSDKLoadBalancerWithSecurityGroups, where added the logic to check if we need to call setSecurityGroup to update EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic.

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the docs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

linux-foundation-easycla · 2024-02-29T23:30:19Z

The committers listed above are authorized under a signed CLA.

✅ login: wweiwei-li (bfff95f)

k8s-ci-robot · 2024-02-29T23:30:24Z

Welcome @wweiwei-li!

It looks like this is your first PR to kubernetes-sigs/aws-load-balancer-controller 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-load-balancer-controller has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-02-29T23:30:25Z

Hi @wweiwei-li. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

shraddhabang · 2024-03-01T00:03:01Z

pkg/service/model_build_load_balancer.go

+	case string(elbv2model.SecurityGroupsInboundRulesOnPrivateLinkOff):
+		return elbv2model.SecurityGroupsInboundRulesOnPrivateLinkOff, nil
+	default:
+		return "", errors.Errorf("unknown securityGroupsInboundRulesOnPrivateLink status: %v", securityGroupsInboundRulesOnPrivateLink)


We should modify this error message to give them suggestion to use the allowed values as well. Something like errors.Errorf("Invalid value for securityGroupsInboundRulesOnPrivateLink status: %v, value must be one of [%v, %v]", securityGroupsInboundRulesOnPrivateLink), string(elbv2model.SecurityGroupsInboundRulesOnPrivateLinkOn), string(elbv2model.SecurityGroupsInboundRulesOnPrivateLinkOff)

dims · 2024-03-01T01:55:29Z

/check-cla
/easycla

shraddhabang · 2024-03-12T22:47:39Z

/ok-to-test

oliviassss · 2024-03-13T01:00:21Z

pkg/model/elbv2/load_balancer.go

+	SecurityGroupsInboundRulesOnPrivateLinkOn  SecurityGroupsInboundRulesOnPrivateLinkStatus = "on"
+	SecurityGroupsInboundRulesOnPrivateLinkOff SecurityGroupsInboundRulesOnPrivateLinkStatus = "off"
+)
+


should we move these to pkg/model/ec2/security_group.go instead?

Yes, it is okay to move these to pkg/model/ec2/security_group.go. One concern is this field is actually a load balancer field, even though it is related to security groups

oliviassss · 2024-03-13T17:23:12Z

pkg/deploy/elbv2/load_balancer_manager.go

@@ -75,6 +76,12 @@ func (m *defaultLoadBalancerManager) Create(ctx context.Context, resLB *elbv2mod
 		return elbv2model.LoadBalancerStatus{}, err
 	}

+	if resLB.Spec.Type == elbv2model.LoadBalancerTypeNetwork && string(*resLB.Spec.SecurityGroupsInboundRulesOnPrivateLink) != "" {


why need to convert it to string? can we just check if it's nil or not? Same for all the string() != "" below

As you suggested if user does not specify the annotation, we will skip adding it as a spec. In this case, we will no longer have resLB.Spec.SecurityGroupsInboundRulesOnPrivateLink = "", so we can just check if it is nil.

oliviassss · 2024-03-13T17:31:40Z

pkg/deploy/elbv2/load_balancer_manager.go

+
+	if resLB.Spec.Type == elbv2model.LoadBalancerTypeNetwork {
+		desiredEnforceSecurityGroupInboundRulesOnPrivateLinkTraffic := string(*resLB.Spec.SecurityGroupsInboundRulesOnPrivateLink)
+		currentEnforceSecurityGroupInboundRulesOnPrivateLinkTraffic := "on"


why need a default value for currentEnforceSecurityGroupInboundRulesOnPrivateLinkTraffic? let's say, if the resLB.Spec.SecurityGroupsInboundRulesOnPrivateLink is on, and there's no such setting on sdkLB, so the currentEnforceSecurityGroupInboundRulesOnPrivateLinkTraffic defaults to on, we ignore this situation?

Good catch. I was thinking it was "on" by default, but it is not. I will the set currentEnforceSecurityGroupInboundRulesOnPrivateLinkTraffic based on sdkLB as below.

var currentEnforceSecurityGroupInboundRulesOnPrivateLinkTraffic string if sdkLB.LoadBalancer.EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic != nil { currentEnforceSecurityGroupInboundRulesOnPrivateLinkTraffic = awssdk.StringValue(sdkLB.LoadBalancer.EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic) }

oliviassss · 2024-03-13T17:34:45Z

pkg/service/model_build_load_balancer.go

@@ -173,6 +179,22 @@ func (t *defaultModelBuildTask) buildLoadBalancerIPAddressType(_ context.Context
 	}
 }

+func (t *defaultModelBuildTask) buildSecurityGroupsInboundRulesOnPrivateLink(_ context.Context) (elbv2model.SecurityGroupsInboundRulesOnPrivateLinkStatus, error) {
+	securityGroupsInboundRulesOnPrivateLink := ""


maybe just a var securityGroupsInboundRulesOnPrivateLink string

Sure, we can just use var securityGroupsInboundRulesOnPrivateLink string

oliviassss · 2024-03-13T17:40:23Z

pkg/service/model_build_load_balancer.go

+func (t *defaultModelBuildTask) buildSecurityGroupsInboundRulesOnPrivateLink(_ context.Context) (elbv2model.SecurityGroupsInboundRulesOnPrivateLinkStatus, error) {
+	securityGroupsInboundRulesOnPrivateLink := ""
+	if exists := t.annotationParser.ParseStringAnnotation(annotations.SvcLBSuffixEnforceSGInboundRulesOnPrivateLinkTraffic, &securityGroupsInboundRulesOnPrivateLink, t.service.Annotations); !exists {
+		return "", nil


if user does not specify the annotation, can we just skip adding it as a spec in the LB? it's a bit strange to have "securityGroupsInboundRulesOnPrivateLink":"".
And I think we should have the default value to "off", for backward compatibility.

Good point, I agree if user does not specify the annotation, we should just skip adding it as a spec in the LB.

Just note : Default value for this is "On".

we need unit tests added for the cases: 1)annotation setting to "on",
2)annotation setting to "off"
3)no annotation at all.

I added some code in pkg/service/model_builder_test.go. When the annotation is set to "on", the build model will have "securityGroupsInboundRulesOnPrivateLink":"on". If the annotation is set to "off", the build model will have "securityGroupsInboundRulesOnPrivateLink":"off". If there is no annotation, "securityGroupsInboundRulesOnPrivateLink" will not be included. Is this sufficient?

oliviassss · 2024-03-13T17:59:44Z

pkg/service/model_build_load_balancer.go

+		return elbv2model.SecurityGroupsInboundRulesOnPrivateLinkOff, nil
+	default:
+		return "", errors.Errorf("Invalid value for securityGroupsInboundRulesOnPrivateLink status: %v, value must be one of [%v, %v]", securityGroupsInboundRulesOnPrivateLink, string(elbv2model.SecurityGroupsInboundRulesOnPrivateLinkOn), string(elbv2model.SecurityGroupsInboundRulesOnPrivateLinkOff))
+	}


it will be confusing that it says only accept "on" and "off", but it actually allows an empty string "".

It won't allow empty string "". An empty string will also fall into the condition of an invalid value.

oliviassss · 2024-03-13T18:03:58Z

@wweiwei-li, thanks for the contribution. I left some comments.

The value of it can be set to "off" and "on". It is optional and if it is default to "on"

For backward compatibility, I think we shouldn't have "on" as default.
If the user doesn't specify the annotation, we skip adding any spec to their NLBs, nothing changed; if the annotation exists, we validate the value and add the spec accordingly.

shraddhabang · 2024-03-14T20:57:20Z

/lgtm
/approve

oliviassss · 2024-03-15T16:49:18Z

/lgtm
/assign @M00nF1sh

M00nF1sh · 2024-03-19T22:10:21Z

pkg/annotations/constants.go

+	SvcLBSuffixLoadBalancerAttributes                    = "aws-load-balancer-attributes"
+	SvcLBSuffixLoadBalancerSecurityGroups                = "aws-load-balancer-security-groups"
+	SvcLBSuffixManageSGRules                             = "aws-load-balancer-manage-backend-security-group-rules"
+	SvcLBSuffixEnforceSGInboundRulesOnPrivateLinkTraffic = "aws-load-balancer-inbound-sg-rules-on-private-link-traffic"


should this be something like

aws-load-balancer-enforce-sg-inbound-rules-on-pl-traffic

or

aws-load-balancer-enforce-sg-inbound-rules-on-private-link-traffic

Good point, I like aws-load-balancer-enforce-sg-inbound-rules-on-private-link-traffic but annotation must be 63 characters or less, so we decided to use aws-load-balancer-inbound-sg-rules-on-private-link-traffic

M00nF1sh · 2024-03-19T22:12:43Z

pkg/deploy/elbv2/load_balancer_manager.go

@@ -75,6 +76,12 @@ func (m *defaultLoadBalancerManager) Create(ctx context.Context, resLB *elbv2mod
 		return elbv2model.LoadBalancerStatus{}, err
 	}

+	if resLB.Spec.Type == elbv2model.LoadBalancerTypeNetwork && resLB.Spec.SecurityGroupsInboundRulesOnPrivateLink != nil {


is this EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic can only be set with this SetSecurityGroups call after LB creation?

Yes, we can only set it after LB creation

M00nF1sh · 2024-03-19T22:15:10Z

pkg/deploy/elbv2/load_balancer_manager.go

@@ -186,20 +193,52 @@ func (m *defaultLoadBalancerManager) updateSDKLoadBalancerWithSecurityGroups(ctx
 	}
 	desiredSecurityGroups := sets.NewString(awssdk.StringValueSlice(securityGroups)...)
 	currentSecurityGroups := sets.NewString(awssdk.StringValueSlice(sdkLB.LoadBalancer.SecurityGroups)...)
-	if desiredSecurityGroups.Equal(currentSecurityGroups) {
+
+	if desiredSecurityGroups.Equal(currentSecurityGroups) && (resLB.Spec.Type != elbv2model.LoadBalancerTypeNetwork || resLB.Spec.SecurityGroupsInboundRulesOnPrivateLink == nil) {


let's add a method that checks whether SGInboundRulesOnPrivateLink shall be enforced
something like

isEnforceSGInboundRulesOnPrivateLinkUpdated(sdkLB, resLB) { ...checks sdkLB.EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic & resLB.EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic }

then if desiredSecurityGroups.Equal(currentSecurityGroups) && !isEnforceSGInboundRulesOnPrivateLinkUpdated(...) { return nil}

and later, simple set req. EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic based on resLB.EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic

Wrap up all checks into isEnforceSGInboundRulesOnPrivateLinkUpdated(sdkLB, resLB) method is a great idea. I made change based on your suggestion. To be able to use current and desired value in logs, I also return currentEnforceSecurityGroupInboundRulesOnPrivateLinkTraffic, desiredEnforceSecurityGroupInboundRulesOnPrivateLinkTraffic value. Please let me know if it looks good to you.

M00nF1sh · 2024-04-03T21:03:22Z

pkg/service/model_build_load_balancer.go

@@ -87,6 +92,22 @@ func (t *defaultModelBuildTask) buildLoadBalancerSpec(ctx context.Context, schem
 		LoadBalancerAttributes: lbAttributes,
 		Tags:                   tags,
 	}
+
+	if securityGroupsInboundRulesOnPrivateLink != "" {


i'd prefer to use below:

if securityGroupsInboundRulesOnPrivateLink != "" { spec.SecurityGroupsInboundRulesOnPrivateLink = &securityGroupsInboundRulesOnPrivateLink }

Yes, good point, will update

M00nF1sh · 2024-04-03T21:17:02Z

pkg/service/model_build_load_balancer.go

@@ -177,6 +198,22 @@ func (t *defaultModelBuildTask) buildLoadBalancerIPAddressType(_ context.Context
 	}
 }

+func (t *defaultModelBuildTask) buildSecurityGroupsInboundRulesOnPrivateLink(_ context.Context) (elbv2model.SecurityGroupsInboundRulesOnPrivateLinkStatus, error) {


can we change the return value to be *elbv2model.SecurityGroupsInboundRulesOnPrivateLinkStatus, error
thus we can return nil instead of "" to represent "not set".
The reason why this is better is technically "" is not a valid value for SecurityGroupsInboundRulesOnPrivateLinkStatus enum.

M00nF1sh · 2024-04-03T21:30:30Z

pkg/deploy/elbv2/load_balancer_manager.go

 		return nil
 	}

+	if !desiredSecurityGroups.Equal(currentSecurityGroups) {


Nit, since there is only one change call technically, i'd prefer to use a single log call
var changeDescriptions []string
if !desiredSecurityGroups.Equal(currentSecurityGroups) {
changeDescriptions = append(changeDescriptions, "changeSecurityGroups", changeSecurityGroupsDesc)
}
req := &elbv2sdk.SetSecurityGroupsInput{
LoadBalancerArn: sdkLB.LoadBalancer.LoadBalancerArn,
SecurityGroups: securityGroups,
}

if ! isEnforceSGInboundRulesOnPrivateLinkUpdated {
changeDescriptions = append(changeDescriptions, "changeEnforceSecurityGroupInboundRulesOnPrivateLinkTraffic", changeEnforceSecurityGroupInboundRulesOnPrivateLinkTrafficDesc)
req.EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic: awssdk.String(desiredEnforceSecurityGroupInboundRulesOnPrivateLinkTraffic),
}
m.logger.Info("modifying loadBalancer security group settings",
"stackID", resLB.Stack().StackID(),
"resourceID", resLB.ID(),
"arn", awssdk.StringValue(sdkLB.LoadBalancer.LoadBalancerArn),
...changeDescriptions)
2. per the current log, req.EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic is only set when there is a change. So if the security group changed but isEnforceSGInboundRulesOnPrivateLinkUpdated is false, then req.EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic will be nil, what's the API behavior under such case, will it untouch the existing EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic settings on LB or reset it to default value(something we don't want).

Yes, we can switch to use a single log call.

Good question, it will untouch the existing EnforceSecurityGroupInboundRulesOnPrivateLinkTraffic settings

M00nF1sh

/lgtm
/approve

k8s-ci-robot · 2024-04-04T21:59:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: M00nF1sh, shraddhabang, wweiwei-li

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:

~~OWNERS~~ [M00nF1sh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vumdao · 2024-04-11T08:04:56Z

Hi, can we know when this ticket is released?

elaineclsouza · 2024-05-14T16:17:20Z

Any predictions on when this will be published in the release?

VanessaDiniz · 2024-05-14T16:21:15Z

I'm also waiting

william-araujo-dock · 2024-05-14T16:23:08Z

When will you implement this feature?

wweiwei-li · 2024-05-14T16:44:56Z

Hey, This feature will be released in V2.8.0 targeting the end of this week.

k8s-ci-robot requested review from kishorj and oliviassss February 29, 2024 23:30

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 29, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 29, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 29, 2024

shraddhabang reviewed Mar 1, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 12, 2024

wweiwei-li force-pushed the nlbSecurityGroup branch 2 times, most recently from fedae1a to 2189784 Compare March 13, 2024 05:03

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 13, 2024

wweiwei-li force-pushed the nlbSecurityGroup branch from 2189784 to 2382541 Compare March 13, 2024 17:10

oliviassss reviewed Mar 13, 2024

View reviewed changes

wweiwei-li force-pushed the nlbSecurityGroup branch from 2382541 to 591a64a Compare March 13, 2024 23:43

k8s-ci-robot assigned shraddhabang Mar 14, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2024

k8s-ci-robot assigned M00nF1sh and oliviassss Mar 15, 2024

oliviassss added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 15, 2024

M00nF1sh reviewed Mar 19, 2024

View reviewed changes

wweiwei-li force-pushed the nlbSecurityGroup branch from 591a64a to a7c57a3 Compare March 21, 2024 06:02

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2024

M00nF1sh reviewed Apr 3, 2024

View reviewed changes

Add support for optionally enforcing NLB security groups on PrivateLink

bfff95f

wweiwei-li force-pushed the nlbSecurityGroup branch from a7c57a3 to bfff95f Compare April 4, 2024 04:05

M00nF1sh approved these changes Apr 4, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 4, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2024

k8s-ci-robot merged commit 30a15d5 into kubernetes-sigs:main Apr 4, 2024

This was referenced May 17, 2024

aws-load-balancer-controller: v2.8.0 aws/eks-charts#1105

Merged

REQUEST: New membership for <your-GH-handle> kubernetes/org#4962

Closed

REQUEST: New membership for wweiwei-li kubernetes/org#4968

Closed

Add support for optionally enforcing NLB security groups on PrivateLi… #3594

Add support for optionally enforcing NLB security groups on PrivateLi… #3594

Uh oh!

Conversation

wweiwei-li commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Uh oh!

linux-foundation-easycla bot commented Feb 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Feb 29, 2024

Uh oh!

k8s-ci-robot commented Feb 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dims commented Mar 1, 2024

Uh oh!

shraddhabang commented Mar 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviassss commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shraddhabang commented Mar 14, 2024

Uh oh!

oliviassss commented Mar 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

M00nF1sh Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

wweiwei-li commented Feb 29, 2024 •

edited

Loading

linux-foundation-easycla bot commented Feb 29, 2024 •

edited

Loading

oliviassss commented Mar 13, 2024 •

edited

Loading

M00nF1sh Mar 19, 2024 •

edited

Loading