WIP: Application load balancer controller by fischerman · Pull Request #3 · stackitcloud/application-load-balancer-controller

fischerman · 2026-06-24T20:24:10Z

How to categorize this PR?

/kind enhancement

What this PR does / why we need it:

Implements the first alpha version of the STACKIT ALB controller.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Breaking changes:

nschad · 2026-06-25T09:58:47Z

@fischerman either mark the PR as "Draft" or use "WIP" as the title. The PR will then automatically marked as WIP/Draft

ske-prow · 2026-06-25T14:33:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign dergeberl for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

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

dergeberl

This is my first batch of review comments. Only had a look into the /cmd/*, /pkg/controller/* (except the tests) and the *.md files, yet. I will continue with my review for the rest.

Maybe some comments are already resolved as you also pushed during my review (I did not crosscheck again, before submitting the comments now).

dergeberl · 2026-06-25T14:43:18Z

+| `alb.stackit.cloud/traget-pool-tls-skip-certificate-validation`| Boolean | IngressClass, Ingress, Service | Optional | Enables TLS bridging but skips certificate validation. |
+| `alb.stackit.cloud/allowed-source-ranges`| String | IngressClass | Accepts a comma-separated list of IP ranges. E.g. 10.0.0.0/24,1.2.3.4/32. If unset, all IPs are allowed. |
+
+### Known Limitations


Maybe we add the NodePort requirement here? Also there are limits in the amount of resources (like listeners) which we should add here (or somewhere else).

The limits like listeners, targetpools and path should also be documented. I remeber you having a list what the limits. We should also document how the this translate to ingress objects, like for the listeners it is the ports.

dergeberl · 2026-06-26T12:04:57Z

+### Limits
+
+The following limitations are imposed directly by the STACKIT ALB API (not the controller itself):
+- Maximum targets per pool: An individual target pool can contain a maximum of 250 targets.


The limit should be 1000 or?

dergeberl · 2026-06-26T12:07:20Z

+
+#### When to watch out for the listener limit
+
+Because each IngressClass provisions a dedicated ALB instance, hitting the 20-listener threshold is rarely an issue for a basic setup but becomes a real risk when you start stacking custom ports across multiple applications sharing that same ALB. If your Ingress resources use the `alb.stackit.cloud/http-port` or `alb.stackit.cloud/https-port` annotations to expose different apps on unique custom port numbers, each distinctive port allocates its own listener on the shared ALB instance. This risk compounds quickly when those applications also require TLS encryption; since the controller must keep an extra HTTP listener active alongside the HTTPS listener to smoothly process automated ACME certificate challenges, a single secure app immediately consumes two slots instead of one, accelerating how fast you approach the API limit if multiple unique custom ports are configured.


This needs a rephrase. The ALB per ingress does not make it rarely to hit the limit (It is the default of 2 ports).

dergeberl · 2026-06-26T12:07:31Z

+
+A "target" in a pool corresponds directly to a worker node in your cluster. If you run a large cluster with a high number of worker nodes, or expect your cluster to dynamically scale to a large size, keep this limit in mind since a single backend Service port mapping cannot route traffic to more than 250 worker nodes simultaneously.
+
+#### When to watch out for the listener limit


Also something for the Known Limitations.

dergeberl · 2026-06-26T12:08:42Z

+
+Because each IngressClass provisions a dedicated ALB instance, hitting the 20-listener threshold is rarely an issue for a basic setup but becomes a real risk when you start stacking custom ports across multiple applications sharing that same ALB. If your Ingress resources use the `alb.stackit.cloud/http-port` or `alb.stackit.cloud/https-port` annotations to expose different apps on unique custom port numbers, each distinctive port allocates its own listener on the shared ALB instance. This risk compounds quickly when those applications also require TLS encryption; since the controller must keep an extra HTTP listener active alongside the HTTPS listener to smoothly process automated ACME certificate challenges, a single secure app immediately consumes two slots instead of one, accelerating how fast you approach the API limit if multiple unique custom ports are configured.
+
+### Configuration


Please check that this are all, WAF seems to be missing

dergeberl

Comments for the stackit package and testutil

dergeberl · 2026-06-29T06:48:25Z

@@ -0,0 +1,22 @@
+package stackit


Can we also add the ResponseID to the error, to be able to trace errors
We do this also in cloud-provider-stackit for the LB api see:
https://github.com/stackitcloud/cloud-provider-stackit/blob/7a0ef7fa43f1d30f6ed4f88bfa2de1312c79b62f/pkg/stackit/client/loadbalancer.go#L43
https://github.com/stackitcloud/cloud-provider-stackit/blob/7a0ef7fa43f1d30f6ed4f88bfa2de1312c79b62f/pkg/stackit/client/helper.go#L12

I looked at a request to our API and the response does not contain the header X-Request-Id. So it is not an easy copy. I feel like this expected API behavior is a bit underspec'ed. So I wouldn't add this logic until this is properly spec'ed.

hown3d · 2026-06-29T06:01:29Z

+
+	if *alb.Status != stackit.LBStatusReady {
+		// ALB is not yet ready, requeue
+		return ctrl.Result{RequeueAfter: 10 * time.Second}, nil


Should we write an event for this? In NLB controller we inform the user about this state via event

hown3d · 2026-06-29T06:25:10Z

+		})
+	}
+
+	if len(listeners) == 0 {


This feels weird to me. If the load balancer does not yet contain any port, we shouldn't create it at all.

hown3d · 2026-06-29T06:28:32Z

+		DisableTargetSecurityGroupAssignment: new(true), // TODO: Make this configurable via flag.
+		Name:                                 new(fmt.Sprintf("k8s-ingress-%s", t.ingressClass.UID)),
+		Labels: &map[string]string{
+			"ingress-class-uid": string(t.ingressClass.UID),


use SanitizeLabelValue

Also from a user perspective it might be useful to add a cluster name label or something likewise here, so one can easily see which alb is for which cluster.

dergeberl · 2026-06-29T09:13:21Z

+	}
+	for i := range ingressClassCertificates {
+		cert := &ingressClassCertificates[i]
+		if err := r.CertificateClient.DeleteCertificate(ctx, r.ALBConfig.Global.ProjectID, r.ALBConfig.Global.Region, *cert.Id); err != nil {


Is this an async call? If yes we should wait until certs are gone before removing the finalizer.

dergeberl · 2026-06-29T11:23:01Z

+					CertificateIds: []string{},
+				},
+			}
+			for fingerprint, cert := range t.certificates {


Map interactions are not deterministic. If I remember correctly the order in the ALB matters, as always the first match will be used. This could lead in a flapping which cert is used if multiple are fitting.

Fixed in 5c25864

The order is stable but cannot be influenced by the customer.

dergeberl · 2026-06-29T13:43:59Z

+			expected: true,
+		},
+		{
+			name: "https certificates changed",


we should add the same but with also 2 Ids.

dergeberl · 2026-06-29T13:44:56Z

+			expected: true,
+		},
+		{
+			name: "target pool tls validation changed",


We should also add targets itself and test.

dergeberl · 2026-06-29T13:48:03Z

@@ -0,0 +1,302 @@
+package ingress


In the update.go are multiple functions which also should be tested, like getServicesForIngresses, getTLSSecretsFromIngresses, getCertificatesForIngressClass. But also the logic for duplicateCerts should get tested.

dergeberl · 2026-06-29T14:33:07Z

+		)
+
+		Expect(errs).To(HaveLen(1))
+		Expect(errs[0].Description).To(Equal("invalid certificate: tls: failed to find any PEM data in certificate input"))


Same as in test should return an error when the TLS secret isn't of type TLS

dergeberl · 2026-06-29T14:54:20Z

+
+	tree := &WorkTreeALB{
+		ingressClass: ingressClass,
+		planID:       GetAnnotation(AnnotationPlanID, "", ingressClass),


Should we validate that we only get working PlanIDs. For NLBs we check that in cloud-provider-stackit

Ties together with https://github.com/stackitcloud/application-load-balancer-controller/pull/3/changes#r3511950744

However, this does not only break an ingress but the entire ALB. Because we introduce the webhook I would just return an error here and not process any updates.

dergeberl · 2026-06-29T14:58:46Z

@@ -0,0 +1,658 @@
+package spec


We should also add tests for:

~~the AnnotationHTTPSOnly and that it not serves HTTP.~~

~~the sorting based on AnnotationPriority and creation timestamp.~~

AnnotationPlanID

the handling when setting not allowed values in annotation (like no valid int/port, not valid cidr in source range)

~~GetUnusedCertificates~~

Added tests for 1, 2 and 5 in c9d5229

Updated comment

dergeberl · 2026-06-30T05:48:57Z

@@ -0,0 +1,85 @@
+apiVersion: apps/v1


We should align this with the deploy of CSI and CCM in cloud-provider-stackit (which are also not perfect, see stackitcloud/cloud-provider-stackit#1162).
For example:

we can use also the kube-system namespace as this is also a system component like CSI and CCM

the cloud config don't need to be a secret

the mount path of the service account and cloud config

the apiEndpoints don't need to be in the example (this is only internally needed)

lets also create a /deploy folder instead if the /samples/deploy/

we need to find a image tag which is working and don't need to be updated like latest (not sure if this will be tagged on release)

With #6 we get a latest tag for tagged versions. Lets use that in the deployment.

Done in 5248047

I vote for latest tag here. We can leave it like it is for now and solve this directly before merging. After merge I will create a release which then also creates the latest tag

dergeberl · 2026-06-30T05:59:50Z

+	ConditionNodeTermination corev1.NodeConditionType = "Terminating"
+)
+
+func isNodeTerminating(node *corev1.Node) bool {


That changes also need to be added to the nodePredicate().

ske-prow · 2026-07-08T15:39:37Z

@fischerman: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-application-load-balancer-controller-verify	`9008b15`	link	true	`/test pull-application-load-balancer-controller-verify`

Full PR test history. Your PR dashboard. Command help for this repository.
Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see the gardener testing guideline for how to avoid and hunt flakes.

Details

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-sigs/prow repository. I understand the commands that are listed here.

ske-prow Bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 24, 2026

nschad changed the title ~~Draft: Application load balancer controller~~ WIP: Application load balancer controller Jun 25, 2026

ske-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2026

nschad force-pushed the main branch from e5c3fee to fba4e1b Compare June 25, 2026 14:11

dergeberl requested review from breuerfelix, dergeberl and hown3d June 25, 2026 14:29

ske-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2026

fischerman force-pushed the albc branch from d73714b to 209f335 Compare June 25, 2026 14:37

ske-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2026

fischerman commented Jun 26, 2026

View reviewed changes

Comment thread .golangci.yaml

Adi146 reviewed Jun 26, 2026

View reviewed changes

Comment thread pkg/controller/ingress/setup.go Outdated

dergeberl reviewed Jun 26, 2026

View reviewed changes

dergeberl reviewed Jun 29, 2026

View reviewed changes

hown3d reviewed Jun 29, 2026

View reviewed changes

fischerman mentioned this pull request Jun 29, 2026

WIP: Add Application Load Balancer Controller Manager stackitcloud/cloud-provider-stackit#879

Closed

dergeberl reviewed Jun 29, 2026

View reviewed changes

Comment thread pkg/controller/ingress/suite_test.go

dergeberl reviewed Jun 29, 2026

View reviewed changes

dergeberl reviewed Jun 30, 2026

View reviewed changes

Comment thread pkg/controller/ingress/ingressclass_controller.go Outdated

dergeberl reviewed Jun 30, 2026

View reviewed changes

Comment thread pkg/controller/ingress/spec/worktree.go

ftl reviewed Jun 30, 2026

View reviewed changes

Comment thread cmd/application-load-balancer-controller/main.go

ftl reviewed Jun 30, 2026

View reviewed changes

Comment thread cmd/application-load-balancer-controller/main.go

dergeberl reviewed Jun 30, 2026

View reviewed changes

Comment thread hack/tools.mk Outdated

fischerman added 6 commits June 30, 2026 15:21

Set user agent for STACKIT APIs

8208205

Wrap original error for not found

88bb483

Don't start variables with underscore

ed2f054

Switch to the new event API

207691b

Make controller tests run in CI

d57675e

Remove kubebuilder markers as RBAC rules are hand-written

77f7939

fischerman force-pushed the albc branch from aeca48c to 77f7939 Compare June 30, 2026 13:21

fischerman added 2 commits June 30, 2026 15:32

Use envtest version from controller-runtime in go.mod

18f90f7

Add more tests to for work tree

c9d5229

dergeberl mentioned this pull request Jul 1, 2026

feat: add validating admission webhooks for Ingress and IngressClass annotations stackitcloud/cloud-provider-stackit#1119

Closed

fischerman added 19 commits July 1, 2026 10:13

If HTTPS doesn't work (yet) then HTTP still should

c43e8a8

Also test HTTP on the ingress with missing TLS cert

4668c66

Add gosec linter

fc1c542

Actually add gosec as a linter

f84439a

Make updateNeeded test Ginkgo

aa02b49

Refactor deployment assets

5248047

Create fakes for STACKIT APIs to simplify controllers tests

3c644c1

Sort all lists in create and update payload

5c25864

Delete correct duplicate certificate

670af13

Fix linter issues

4e64691

Remove unused methods from ALB API

b610728

Support paging for list endpoint

80483a4

Move values into constants

5db59af

Added tests for the status of ingresses

5468f37

Extend change detection and tests

7bea972

Fix linter issues

630ddf4

Refactor update.go

81d358c

Updated ALB SDK to latest

a90ab33

Add tests for out of range HTTP/S ports

9008b15


		#### When to watch out for the listener limit

		Because each IngressClass provisions a dedicated ALB instance, hitting the 20-listener threshold is rarely an issue for a basic setup but becomes a real risk when you start stacking custom ports across multiple applications sharing that same ALB. If your Ingress resources use the `alb.stackit.cloud/http-port` or `alb.stackit.cloud/https-port` annotations to expose different apps on unique custom port numbers, each distinctive port allocates its own listener on the shared ALB instance. This risk compounds quickly when those applications also require TLS encryption; since the controller must keep an extra HTTP listener active alongside the HTTPS listener to smoothly process automated ACME certificate challenges, a single secure app immediately consumes two slots instead of one, accelerating how fast you approach the API limit if multiple unique custom ports are configured.


		A "target" in a pool corresponds directly to a worker node in your cluster. If you run a large cluster with a high number of worker nodes, or expect your cluster to dynamically scale to a large size, keep this limit in mind since a single backend Service port mapping cannot route traffic to more than 250 worker nodes simultaneously.

		#### When to watch out for the listener limit


		Because each IngressClass provisions a dedicated ALB instance, hitting the 20-listener threshold is rarely an issue for a basic setup but becomes a real risk when you start stacking custom ports across multiple applications sharing that same ALB. If your Ingress resources use the `alb.stackit.cloud/http-port` or `alb.stackit.cloud/https-port` annotations to expose different apps on unique custom port numbers, each distinctive port allocates its own listener on the shared ALB instance. This risk compounds quickly when those applications also require TLS encryption; since the controller must keep an extra HTTP listener active alongside the HTTPS listener to smoothly process automated ACME certificate challenges, a single secure app immediately consumes two slots instead of one, accelerating how fast you approach the API limit if multiple unique custom ports are configured.

		### Configuration

Uh oh!

Conversation

fischerman commented Jun 24, 2026 • edited by nschad Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nschad commented Jun 25, 2026

Uh oh!

ske-prow Bot commented Jun 25, 2026

Uh oh!

Uh oh!

Uh oh!

dergeberl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Uh oh!

dergeberl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fischerman commented Jun 24, 2026 •

edited by nschad

Loading

dergeberl Jun 29, 2026 •

edited

Loading