support security groups for NLB

[nwsparks]

What would you like to be added:

It was recently announced that NLB's now support security groups. https://aws.amazon.com/about-aws/whats-new/2023/08/network-load-balancer-supports-security-groups/

It would be great if support for this could be added to the cloud controller.

Why is this needed:

Enhances security posture and allows for parity with existing NLB configuration options.

/kind feature

[k8s-ci-robot]

This issue is currently awaiting triage.

If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[mikestef9]

Hi @nwsparks the in-tree service controller is deprecated and not taking new features. Is there any reason you can't use the AWS Load Balancer controller? NLB is fully supported in that controller including support for the recently launched NLB security groups feature.

[nwsparks]

@mikestef9 thanks for clarifying. I'll close this.

[JoelSpeed]

the in-tree service controller is deprecated and not taking new features.

@mikestef9 Is this documented or announced anywhere? I've looked through this repo, and online documentation and cannot see it mentioned anywhere.

This feature is important and, given it already has a parallel both in the ELB implementation, and in the AWS LB controller, I don't expect it's likely to cause too many issues if it were implemented, it is at the end of the day, taking an annotation and passing the values to AWS right?

I'd also like to push back on the suggestion the the LB controller should be the default. This is the AWS CCM, a core part of any Kubernetes cluster running on AWS. The idea of deprecating a key part of the cluster functionality like this and requiring an another add-on (another thing for admins to manage) seems like a massive downgrade in UX no?

Since the AWS LB controller doesn't support ELB, it is not a complete replacement either, which leaves users in an even more awkward position.

I understand the want to have 1 way to do a thing and not duplicate effort, but, is there a better way to achieve this, where there's one LB controller for all LB types? Perhaps integrating the LB controller code as a library into the CCM somehow? Assuming of course that it is behaviourally compatible

[elmiko]

i generally concur with @JoelSpeed , i would like to understand more about the deprecation of the service controller as well.

if it is the case that to continue normal features and functioning that the user needs to install the LB controller as well, then as Joel suggests, perhaps the controller should be integrated into the ccm using a library, or it should be deployed as part of the ccm process.

[dims]

@elmiko for context, what we have here in this repo was to support the extraction of things from k/k and support existing users of that piece of code / scenario.

[elmiko]

@dims that makes sense as a starting point for this, but is it the case that this repo only accepts bug and security fixes? or can we propose features here?

[dims]

@elmiko we have only landed the former for a long while now ... https://github.com/kubernetes/cloud-provider-aws/pulls?q=is%3Apr+loadbalancer+sort%3Aupdated-desc

https://github.com/kubernetes-sigs/aws-load-balancer-controller is funded, has had 65-ish releases, with the first tag back in 2017 ( https://github.com/kubernetes-sigs/aws-load-balancer-controller/releases/tag/1.0-alpha.3 ). So i think the ship has sailed on this any feature requests here in this repo.

[elmiko]

thanks for the links, that is helpful. i can understand that the maintainers of this project and the load balancer controller have moved the logic into the lb controller, and that is the preferred solution now.

one thing i have confusion about are the statements about features on this repo, is it the case that this repo is not accepting new features (load balancer related or not)?

if so, i think we should update the readme or something as it is confusing from a community development perspective.

edit:

i guess this is the statement, perhaps it could be clarified

The in-tree cloud provider code has mostly stopped accepting new features, so future development for the AWS cloud provider should continue here. The in-tree plugins will be removed in a future release of Kubernetes.

[dims]

i believe the context was about the loadbalancers. Yes, this is the only repo for AWS out-of-tree cloud provider and is used by both kops and EKS teams[1].

[1] https://github.com/search?q=repo%3Akubernetes%2Fkops%20cloud-provider-aws&type=code [2] https://gallery.ecr.aws/eks-distro/kubernetes/cloud-provider-aws/cloud-controller-manager

Note, kops has been using 2022 (k8s 1.19-ish) [3] [3] https://github.com/kubernetes/kops/commits/master/upup/models/cloudup/resources/addons/aws-load-balancer-controller.addons.k8s.io/k8s-1.19.yaml.template

[JoelSpeed]

@dims Do you have any context on how EKS and KOPS support ELBs? Are they still using the CCM for that?

In an ideal world, we would have clusters that have 1 controller that handles ELB, NLB, and ALB, but, I don't think that's the current plan.

The next best thing would be to have 1 controller for each of the types, but, if I run both CCM and the AWS LB controller, I now have two NLB controllers running concurrently.

If the decision is that we should be running the LB controller, then I think the migration path currently is not clear, and I'd appreciate if we can work on that.

It would be good to be able to stop the CCM processing NLBs, and have the LB controller pick up all the existing CCM NLBs.

The former part of that doesn't sound too hard, I'm not sure about the latter.

[dims]

cc @M00nF1sh @oliviassss from EKS side cc @rifelpet @hakman from kops side

@JoelSpeed these are the folks you should talk to ^^

[oliviassss]

@JoelSpeed, Hi, overal CCM provisions CLB and NLB, and aws load balancer controller (AWS LBC) provisions ALB and NLB. if you have both controllers installed in your cluster, the CCM will ignore the service type of load balancer with the spec spec.loadBalancerClass: service.k8s.aws/nlb, and this service will be picked up by AWS LBC to provision NLB for it. The good news is that from AWS LBC v2.5.0, we have a service mutator webhook, which automatically injects the spec for service type of load balancer, if the spec does not exist already, to make the AWS LBC the default controller for service, over CCM. With that said, if you have both controllers installed (CCM installed by default with EKS CP), and your AWS LBC is on version v2.5.0+, your service type of load balancer should be provisioned by LBC by default. For existing NLBs provisioned by CCM, you can add this spec manually and have it provisioned by LBC (it may cause some downtime so better to test in dev first) some reference you can check:

1. https://kubernetes-sigs.github.io/aws-load-balancer-controller/latest/guide/service/nlb/
2. https://github.com/kubernetes-sigs/aws-load-balancer-controller/releases/tag/v2.5.0

[JoelSpeed]

For existing NLBs provisioned by CCM, you can add this spec manually and have it provisioned by LBC (it may cause some downtime so better to test in dev first)

Is taking over NLBs from CCM something that AWS tests as part of the AWS LB controller E2E?

Do you have awareness of differences in behaviour between the two implementations so that we could predict when to expect downtime during this transition?

What can we do to expose this information in a more visible way and ensure that the entire CCM consuming community are aware that they should no longer be using the CCM service controller? I stumbled across this thread, but I wouldn't expect every CCM user to stumble across it

And, will ELBs ever be migrated out of the CCM? Or are PRs still welcome for the ELB implementation?