feat(helm): Add loadBalancerClass

[LucasBoisserie]

What this PR does / why we need it:

Add loadBalancerClass params on svc: https://kubernetes.io/docs/concepts/services-networking/service/#load-balancer-class

The option appears in Kubernetes 1.21 (stable in 1.24).
We need this field to use another load balancer implementation than the cloud provider default.

In my case, i use EKS and EKS default load balancer implementation is Kubernetes in-tree cloud controller which is stop accepting new features and will be removed in a future release of Kubernetes (https://github.com/kubernetes/cloud-provider-aws#migration-from-in-tree).
AWS create a new controller to managed service type LoadBalancer (https://kubernetes-sigs.github.io/aws-load-balancer-controller/) and recommend to use it to have latest feature releases (https://docs.aws.amazon.com/eks/latest/userguide/aws-load-balancer-controller.html).

To avoid having 2 controllers reconcile the service type LoadBalancer and create an infinite loop, we can use loadbalancerClass to specify which controller is targeted. It's like the ingressClass but for Service.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• CVE Report (Scanner found CVE and adding report)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

Which issue/s this PR fixes

fixes #8716

How Has This Been Tested?

EKS cluster in 1.24.7 with aws-load-balancer-controller in 2.4

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added unit and/or e2e tests to cover my changes.
• All new and existing tests passed.
• Added Release Notes.

Does my pull request need a release note?

Helm: Add `controller.service.loadBalancerClass`  values

[k8s-ci-robot]

This issue is currently awaiting triage.

If Ingress contributors determines 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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

Hi @LucasBoisserie. Thanks for your PR.

I'm waiting for a kubernetes 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.

[longwuyuan]

@LucasBoisserie, it will help if you create an issue first and post the details for everyone to read and triage.

• Why is the loadbalancerClass becoming important now ? Was there a recent change in specs ?
• How many users need to use loadbalancerClass ? Is there a major breaking change by any provider ?
• What problem does it solve spec a different loadbalancerClass ?
• Other such details

[LucasBoisserie]

@longwuyuan , the issue already exist and is available here: #8716

Why is the loadbalancerClass becoming important now ? Was there a recent change in specs ?

The option appears in Kubernetes 1.21 (stable in 1.24).
We need this field to use another load balancer implementation than the cloud provider default.

In my case, i use EKS and EKS default load balancer implementation is Kubernetes in-tree cloud controller which is stop accepting new features and will be removed in a future release of Kubernetes (https://github.com/kubernetes/cloud-provider-aws#migration-from-in-tree).
AWS create a new controller to managed service type LoadBalancer (https://kubernetes-sigs.github.io/aws-load-balancer-controller/) and recommend to use it to have latest feature releases (https://docs.aws.amazon.com/eks/latest/userguide/aws-load-balancer-controller.html).

To avoid having 2 controllers reconcile the service type LoadBalancer and create an infinite loop, we can use loadbalancerClass to specify which controller is targeted. It's like the ingressClass but for Service.

How many users need to use loadbalancerClass ? Is there a major breaking change by any provider ?

It's not breaking change because loadbalancerClass is set by default to nil by Kubernetes and in-tree cloud controller continue to reconcile them.
Multiple users add reactions and comments in the issue.

What problem does it solve spec a different loadbalancerClass ?

Targeting of which Loadbalancer controller must reconcile service type LoadBalancer. It's the same behavior than ingressClass

[longwuyuan]

Humble request is please edit the description of the PR and add all the helpful information, with any addition elaborate story on it there @LucasBoisserie

[LucasBoisserie]

@longwuyuan, I edit the PR's description
Is it good for you ?

[longwuyuan]

@LucasBoisserie thanks for editing description.

My next thoughts are this ;

• only you are asking for this change. Usually changes are done when several users ask or need changes

• the changes are not in the controller binaries/executables. the changes are only in the user-configurable yaml files. Do you know if just using a custom yaml file for components like the service resource spec and the helm vales spec, is enough. If so, then you can maintain this yourself, in a fork or a clone in your git. Changing the project default for only one user and a corner-case use does not seem a improvement

• by description, this seems like a major change for AWS users so I think ther eis less description here as to why all EKS users are not asking for this change. It is not even described as to how your description about in-tree controller for AWS LB, impacts other EKS users

I am not an expert so I will read more. So waiting for others to comment and see how this goes. Project is in stabilization phase but almost done on that.

[zentavr]

@longwuyuan I have kube-vip which works as a loadbalancer and metalLB. I would like to choose which LB to use by my service and nginx ingress helm chart misses that.

[Lirt]

Hi, +1 for this feature.

This is needed for MetalLB use-case:

• MetalLB controller has possibility to watch only changes to specific services of type LoadBalancer.
• Maintainers of MetalLB decided to use loadBalancerClass as filtering attribute.
• So when I specify filter loadBalancerClass in MetalLB and deploy ingress-nginx with service of type LoadBalancer + same loadBalancerClass - it will only reconcile service of ingress-nginx.

[bbetter173]

My use-case for this is to expose ingress-nginx over tailscale using their new operator. A lot of people would like this functionality for all sorts of reasons, and it's 100% backwards compatible with existing behavior, I don't see any reason why it wouldn't be accepted.

[cskinfill]

I can't get the AWS LoadBalancer Controller to properly work without using the spec.loadBalancerClass in the Service definition. I'm definitely another person that needs this feature.

[zentavr]

@cskinfill i'm using helm hooks and kustomize right not to achieve that.

[JuniorJPDJ]

Bump. It would be very helpful to have support of that.

My usecase is cilium (bgp) + metallb (l2).

[simonsCatalyst]

@longwuyuan

I would also like this, my reason being the default load balancer provisioned is an AWS "Classic" load balancer. With an AWS "Network" load balancer (NLB) I can have a static IP address from an existing pool of addresses, the "Classic" type cannot have a static IP address. In theory the performance could also be better with an AWS NLB.

[longwuyuan]

@simonsCatalyst the manifest https://kubernetes.github.io/ingress-nginx/deploy/#aws creates NLB

[simonsCatalyst]

@simonsCatalyst the manifest https://kubernetes.github.io/ingress-nginx/deploy/#aws creates NLB

Hi @longwuyuan

I am aware I could have done this outside the Helm chart, but this PR deals with adding functionality to the Helm chart - which I (And I think many others will) very much appreciate.

Cheers

Simon

[longwuyuan]

• If there were tests and data related to tests posted here, it would help readers know more. Something like a new can read and know the impact of using or not-using LoadBalancerClass field
• Adding a feature is not the problem. Maintaining and supporting features requires resources and that is when details of the tests conducted before making changes help a lot.
• Would you know if this makes the spec LoadBalancerClass optional or is it forced on all users, including those who are not using LoadBalancerClass spec currently

[JuniorJPDJ]

Look at this diff @longwuyuan. It's like 5 lines of code + some docs.
This needs no additional maintenance - it's just making one manifest field available to be set from helm values.

[longwuyuan]

@JuniorJPDJ @simonsCatalyst @LucasBoisserie the idea is to help push this PR forward. If you are ignoring that you need to rebase , it does not help.

• If you can avoid editing the Chart.yaml fields like version, it will help as that is better taken care of during release process

• Its better if you rebase and push

• Someone can clone the branch of the fork of @LucasBoisserie and do a install locally, and then copy/paste the install details and running state here (to show that the change works normally for people not using the spec LoadBalancerClass , and thus inheriting the default value from the values.yaml). Or you can help and do this copy/paste here. Not absolutely needed but it helps the reader who comes here

• I hope you used helm-docs to generate the README inside the charts folder. It is required that the README inside the charts folder be generated using helm-docs

[longwuyuan]

@JuniorJPDJ @simonsCatalyst @LucasBoisserie also to help triage this, PTAL at #9209 . My curiosity is the question asked by @strongjz there. Do you have any comments related to the question. Is that question still relevant to this PR as well ?

I would think we need to close that older PR #9209 as it could be a likely duplicate and also for the lack of tracktion on it

[longwuyuan]

I tested from your fork+branch on a kind cluster and found no problems. But its not a real test as there is no real loadBalancer provider or multiple ingress-controllers

[LucasBoisserie]

@longwuyuan

I've just rebase and push. I also removed field version in Chart.yaml.
Readme has been updated with helm-docs.

For the question asked by @strongjz here: #9209 (comment)

I think the question is not correct because service.beta.kubernetes.io/aws-load-balancer-nlb-target-type annotation is for configuring LB targetting (pod with vpc ip or node directly).

Maybe the real question is for service.beta.kubernetes.io/aws-load-balancer-type which can be set to external and make AWS in-tree controller ignore this service for reconciliation.
But this solution is not perfect because if you have multiple service load-balancer provider (AWS, Cilium kube-vip, metalLB, ...), you can't target specific one.
This the reason why kubernetes introduced this new field.

I make the same test as you on EKS cluster:

On first install without any params, we see in-tree service loadbalancer controller reconcile service. On the second install with loadBalancerClass set, service loadbalancer is reconcile by aws-loadbalancer-controller reconcile.

[strongjz]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LucasBoisserie, strongjz

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:

• charts/ingress-nginx/OWNERS [strongjz]

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

[Gacko]

Bit late to the party, but would have been nice to also implement this for the internal service.

[xDmitriev]

The value controller.service.loadBalancerClass is a string. Why toYaml is used here?