Add webhook for claiming load balancers without LoadBalancerClass #2925

olemarkus · 2022-12-13T08:21:00Z

Description

Kubernetes on AWS have two service controllers, this one (LBC) and the one that sits with Cloud Controller Manager (CCM). The CCM repo has a significant amount of bugs/issues related to it, and the most common resolution is to refer the users to LBC.

As a secondary issue, CCM will default, and only fully support classic ELBs.

A third issue is that in IPv6-only clusters, you have to use NLBs, so the CCM controller doesn't work at all.

The requirements for a solution is something like this:

There should be a migration path for existing clusters.
Existing services should continue to work (i.e something still needs to support Classic ELBs) and preserve CCMs behavior.

I don't think implementing CCM's controller in LBC is all that feasible. And it may be a moving target, as behaviour in CCM may change as well.

The easiest solution I could think of is to add a webhook that mutates services, adding some annotations and LoadBalancerClass if none are set. This effectively makes LBC the default while CCM will continue to operate on existing services. Migrating an existing service would have to be done through deletion and recreation of the Service resource, but that would be the case today as well since LoadBalancerClass is immutable, and LBC has no way of just adopting an ELB from CCM.

This PR creates a proof of concept that makes the few cloud LB related e2e tests pass and does some minimal configuration of the appropriate annotations (e.g without service class set, the service should be internet-facing by default). More of the CCM annotations needs to be implemented here. There should also be an option to skip the mutation if a specific annotation is set if one really wants CCM to manage a specific service. A feature gate that just noops the webhook should also be added to allow the webhook configuration to exist in the cluster, but disable its functionality.

If this has merit, I can add this functionality, but right now I am just looking for input.

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 🌟

kishorj · 2022-12-13T17:24:06Z

@olemarkus, thanks for the PR, we are also tracking this feature internally for the upcoming v2.5.0 release and working on the webhook changes :). We intend to make the lb controller default for all new services of type LoadBalancer either by adding the annotation or specifying the load balancer class. We will include some changes from your PR.

kishorj · 2022-12-13T17:26:19Z

webhooks/core/service_mutator.go

+	if svc.Annotations == nil {
+		svc.Annotations = make(map[string]string)
+	}
+	svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-scheme"] = "internet-facing"


due to security reasons, we do not want to create an internet-facing load balancer by default. The end user needs to be explicit they want to create an internet-facing load balancer.

I'm aware. But kubetest2 needs to confirm connectivity with the LB, so it would be a breaking change if we do not do this when the LB class is empty. One could very well say that kubernetes requires the CCM controller (and those that emulate it) to create a public cloud LB by default.

What we probably have to do here is to introduce a compatibility mode flag. I can imagine people want both behaviors.

I'm looking forward to merging the changes for v2.5.0. Lets limit the webhook to setting the spec.loadBalancerClass. We will leave the default service as internal for now. I'm open to providing a flag to change the default behavior in future releases.

kishorj · 2022-12-13T17:27:08Z

webhooks/core/service_mutator.go

+		svc.Annotations = make(map[string]string)
+	}
+	svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-scheme"] = "internet-facing"
+	svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-target-type"] = "instance"


We can make it default to the controller's configuration.

We definitely want this one to be configurable. Especially since ipv6 NLBs doesn't support instance targeting.

default target type is already configurable at the controller level, lets remove it as well.

johngmyers · 2022-12-25T01:28:07Z

My approach to this problem is to add a flag that makes LBC the default service controller.

johngmyers@5f2da30 was my first attempt, but that commit incorrectly conflates being the default service controller with specifying the default target-type. #2840 addresses setting the default target-type; once that is resolved I was planning on sending a follow-up adding a flag to make LBC the default service controller.

codecov-commenter · 2023-03-10T23:00:16Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (1392585) 54.40% compared to head (0449794) 54.40%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2925   +/-   ##
=======================================
  Coverage   54.40%   54.40%           
=======================================
  Files         145      145           
  Lines        8429     8429           
=======================================
  Hits         4586     4586           
  Misses       3512     3512           
  Partials      331      331

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

kishorj

I plan to include the webhook configuration in the helm chart, enabled by default the helm chart, with an option to disable if needed.

kishorj · 2023-03-10T22:53:00Z

webhooks/core/service_mutator.go

+	if svc.Annotations == nil {
+		svc.Annotations = make(map[string]string)
+	}
+	svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-scheme"] = "internet-facing"


I'm looking forward to merging the changes for v2.5.0. Lets limit the webhook to setting the spec.loadBalancerClass. We will leave the default service as internal for now. I'm open to providing a flag to change the default behavior in future releases.

kishorj · 2023-03-10T22:58:24Z

webhooks/core/service_mutator.go

+		svc.Annotations = make(map[string]string)
+	}
+	svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-scheme"] = "internet-facing"
+	svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-target-type"] = "instance"


default target type is already configurable at the controller level, lets remove it as well.

kishorj · 2023-03-10T23:02:25Z

For v2.5.0, we intend to change the perquisite to k8s 1.22 or later. Setting the spec.loadBalancerClass would be sufficient to make the LBC the default controller for service of type load balancer.

kishorj · 2023-03-10T23:04:08Z

webhooks/core/service_mutator.go

+	if svc.Annotations == nil {
+		svc.Annotations = make(map[string]string)
+	}
+	svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-scheme"] = "internet-facing"
+	svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-target-type"] = "instance"


Suggested change

if svc.Annotations == nil {

svc.Annotations = make(map[string]string)

}

svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-scheme"] = "internet-facing"

svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-target-type"] = "instance"

olemarkus · 2023-03-21T07:25:43Z

Thanks for the comments. How do you want to proceed with this this? Should I amend this PR and then you pull this into your internal changes?

kishorj · 2023-03-23T17:48:39Z

Thanks for the comments. How do you want to proceed with this this? Should I amend this PR and then you pull this into your internal changes?

Lets amend the PR, I can merge in the suggested changes as well. Once we merge this change, we can add the webhook manifest.

olemarkus · 2023-03-23T19:10:51Z

Cool. Amended.

k8s-ci-robot · 2023-03-23T21:25:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kishorj, olemarkus

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~~ [kishorj]

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

kishorj · 2023-03-23T21:25:43Z

/lgtm

…bernetes-sigs#2925)

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 13, 2022

k8s-ci-robot requested review from kishorj and M00nF1sh December 13, 2022 08:21

kishorj reviewed Dec 13, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2023

kishorj added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 10, 2023

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2023

kishorj reviewed Mar 10, 2023

View reviewed changes

olemarkus force-pushed the legacy-webhook branch from 0449794 to e80b41a Compare March 23, 2023 19:07

Add webhook for claiming load balancers without LoadBalancerClass

9aa07b8

olemarkus force-pushed the legacy-webhook branch from e80b41a to 9aa07b8 Compare March 23, 2023 19:10

kishorj approved these changes Mar 23, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2023

k8s-ci-robot assigned kishorj Mar 23, 2023

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

k8s-ci-robot merged commit 161168e into kubernetes-sigs:main Mar 23, 2023

This was referenced Apr 14, 2023

🥳 aws-load-balancer-controller v2.5.0 Automated Release! 🥑 aws/eks-charts#932

Closed

🥳 aws-load-balancer-controller v2.5.0 Automated Release! 🥑 aws/eks-charts#933

Merged

Timothy-Dougherty pushed a commit to adammw/aws-load-balancer-controller that referenced this pull request Nov 9, 2023

Add webhook for claiming load balancers without LoadBalancerClass (ku…

952238c

…bernetes-sigs#2925)

Add webhook for claiming load balancers without LoadBalancerClass #2925

Add webhook for claiming load balancers without LoadBalancerClass #2925

Uh oh!

Conversation

olemarkus commented Dec 13, 2022

Description

Checklist

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

Uh oh!

kishorj commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

johngmyers commented Dec 25, 2022

Uh oh!

codecov-commenter commented Mar 10, 2023

Codecov Report

Uh oh!

kishorj left a comment

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!

kishorj commented Mar 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olemarkus commented Mar 21, 2023

Uh oh!

kishorj commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olemarkus commented Mar 23, 2023

Uh oh!

k8s-ci-robot commented Mar 23, 2023

Uh oh!

kishorj commented Mar 23, 2023

Uh oh!

Uh oh!

kishorj commented Dec 13, 2022 •

edited

Loading

kishorj commented Mar 23, 2023 •

edited

Loading