Skip to content

loadBalancerClass support? #2466

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
TBBle opened this issue Jan 20, 2022 · 2 comments · Fixed by #2489
Closed

loadBalancerClass support? #2466

TBBle opened this issue Jan 20, 2022 · 2 comments · Fixed by #2489
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@TBBle
Copy link
Contributor

TBBle commented Jan 20, 2022

Is your feature request related to a problem?

Not a problem per-se. I just came across Service.spec.loadBalancerClass and thought it might be a good alternative or replacement for the service.beta.kubernetes.io/aws-load-balancer-nlb-target-type annotation.

By it's very presence, it implies service.beta.kubernetes.io/aws-load-balancer-type: "external" behaviour, so the legacy AWS Cloud Controller will ignore it.

I'm not sure if the AWS Cloud Controller v1 will also inherit that behaviour. I expect that it already works, since the relevant feature has moved to beta and the KEP didn't mention any work needed by non-legacy Cloud Controller implementations. Otherwise it will need to separately implement the same check (similar to the current isLBExternal annotation check, I guess).

The AWS Cloud Controller v2 doesn't appear to support load balancers at all yet, I recall a discussion that the plan for v2 is to never implement it in favour of this controller (which presents a different challenge), but I may be misremembering anyway. (Edit: I appear to be wrong about this, it looks like there was intention for LB support in the V2 Cloud Controller, but work stalled and has staled away...)

Per KEP-1959, one of the goals of this feature is:

prevent every cloud provider from implementing a custom "opt-out" annotation for their load balancer.

which we already have done in this controller and the various AWS Cloud Controllers.

Describe the solution you'd like

The simple-obvious solution is that we recognise the following loadBalancerClass values as equivalent to the given aws-load-balancer-nlb-target-type annotation. (I'm making up the domain name. Looking around, all the AWS annotations for services seem to be in kubernetes.io domains, I'm not sure if that's legacy or not.)

  • aws.lb.kubernetes.io/ip => ip
  • aws.lb.kubernetes.io/instance => instance

However, we could also go further, and throw another axis into the mix, e.g., aws-load-balancer-scheme, since it's the other one described on the NLB docs (as opposed to the Service annotation docs).

  • aws.lb.kubernetes.io/ip-internal => nlb-target-type: ip, scheme: internal
  • aws.lb.kubernetes.io/instance-internal => nlb-target-type: instance, scheme: internal
  • aws.lb.kubernetes.io/ip-internet => nlb-target-type: ip, scheme: internet-facing
  • aws.lb.kubernetes.io/instance-internet => nlb-target-type: instance, scheme: internet-facing

If new values are added to either of these annotations, then the list of recognised classes would similarly grow.

In the case that the feature gate is disabled, i.e. k8s 1.21, then the loadBalancerClass value is stripped, so to avoid losing functionality, the controller would need to ignore the absence of this value if the feature gate is disabled or not present (pre-1.21) and honour the annotations.

When the feature gate is active, back-compat suggests it should still ignore the absence of this value and honour the annotations. I don't immediately see any issues with this, since we have the service.beta.kubernetes.io/aws-load-balancer-type: "external" annotation in the ecosystem already, and I don't think it's likely that any other conflicting use-cases for that annotation value will appear in future.

Describe alternatives you've considered

If one considers this controller the "default load balancer implementation", then per the docs,

Any default load balancer implementation (for example, the one provided by the cloud provider) will ignore Services that have this field set.

then the opposite change would need to be made, and this controller must ignore Services with loadBalancerClass set. This is the "different challenge" I mentioned earlier, but is also fully backwards compatible.

This might end up in the situation like IngressClass and StorageClass, where it's up to the cluster administrator to bless one particular implementation as default, used when no loadBalancerClass is provided. I'm not aware of a standard mechanism for that though, and the current cloud-controller implementation in kubernetes itself doesn't seem to allow for such behaviour.


An alternative implementation would be to simply support a single loadBalancerClass, e.g. aws.lb.kubernetes.io/aws-lb-controller, to replace the service.beta.kubernetes.io/aws-load-balancer-type: "external" annotation, and keep using the existing nlb-target-type and scheme annotations to configure any deeper than that. This feels like a missed opportunity though.

@kishorj kishorj self-assigned this Jan 20, 2022
@kishorj
Copy link
Collaborator

kishorj commented Jan 20, 2022

@TBBle, thanks for the feature request. It is already in our roadmap for the upcoming v2.4.0 release.

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 20, 2022
@TBBle
Copy link
Contributor Author

TBBle commented Feb 3, 2022

I observe that the current PR has gone with the simple "single loadBalancerClass value to select this controller", unlike the IngressClass support (or my suggestion above).

Perhaps in future, we might see something like LoadBalancerClassParams (along the line of IngressClassParams) to allow setting defaults (or just more-structured-than-annotations configuration) for a given loadBalancerClass values. That would require running multiple instances of the controller to be able to offer multiple config options selected by loadBalancerClass though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants