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:07:18Z

+		return ctrl.Result{}, fmt.Errorf("failed to get load balancer: %w", err)
+	}
+
+	if *alb.Status != stackit.LBStatusReady {


In the latest SDK (v0.15.0) there are const for that LOADBALANCERSTATUS_STATUS_READY.

There are some other changes that come with the update of the SDK. Can we postpone this to after the merge?

Based in the changelog the new release only added the enums. The change should be minimal and only be a search and replace of the existing enums (as the prefix renamed). So not sure if it worth to postpone this.

https://github.com/stackitcloud/stackit-sdk-go/blob/main/services/alb/CHANGELOG.md

dergeberl · 2026-06-29T06:08:41Z

+type ProjectStatus string
+
+const (
+	LBStatusReady       = "STATUS_READY"


Lets use the LOADBALANCERSTATUS_STATUS_READY from the SDK

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

dergeberl · 2026-06-29T07:16:12Z

+	internalLB    bool
+	externalIP    string
+
+	listeners map[int16]*workTreeListener


int16 is not big enough for ports. It is later converted again to int32. Let's enable the gosec lint (this is how I found this error)

I changed the type to uin16 and added out of range checks.

lets also add gosec to the golangci-lint

Added in fc1c542

fc1c542 does not add the gosec to the golangci-lint config

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:07:16Z

+		hosts := []albsdk.HostConfig{}
+		for hostname, host := range listener.hosts {
+			paths := slices.Collect(maps.Values(host.paths))
+			typeRank := map[networkingv1.PathType]int{


this is static, can this live as a global variable?

hown3d · 2026-06-29T06:09:23Z

+				networkingv1.PathTypeImplementationSpecific: 2,
+				networkingv1.PathTypePrefix:                 3,
+			}
+			slices.SortFunc(paths, func(a, b *workTreePath) int {


I'd like to have this sort function as a standalone instead of inlined in this for loop.

hown3d · 2026-06-29T06:21:47Z

+				Path:       new("/healthz"),
+				OkStatuses: []string{"200"},
+			},
+			HealthyThreshold:   new(int32(1)),


Should be constants for these defaults to easily find them in the code.

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.

hown3d · 2026-06-29T07:11:14Z

file should be called reconcile.go or smth like that, has nothing to do with updating things.
Not sure in general why there are two files with the reconciler code.

dergeberl · 2026-06-29T08:56:27Z

+	duplicateCerts := []string{}
+	for _, cert := range ingressClassCertificates {
+		if id, exists := certIDMap[spec.CertificateFingerprint(*cert.Data.FingerprintSha256)]; exists {
+			duplicateCerts = append(duplicateCerts, id)


Currently, we add the id from the certIDMap map which would result in deleting the certs that want to keep.

Suggested change

duplicateCerts = append(duplicateCerts, id)

duplicateCerts = append(duplicateCerts, cert.Id)

dergeberl · 2026-06-29T09:09:03Z

+
+		if ptr.Deref(c.Name, "") != ptr.Deref(d.Name, "") ||
+			ptr.Deref(c.TargetPort, 0) != ptr.Deref(d.TargetPort, 0) ||
+			len(c.Targets) != len(d.Targets) {


A len check is not enough here.

dergeberl · 2026-06-29T09:09:29Z

+	return len(c.CertificateConfig.CertificateIds) != len(d.CertificateConfig.CertificateIds)
+}
+
+func targetPoolsChanged(current, desired []albsdk.TargetPool) bool {


Misses activeHealthCheck

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:18:39Z

+	region string,
+) *albsdk.CreateLoadBalancerPayload {
+	listeners := []albsdk.Listener{}
+	for port, listener := range t.listeners {


Map interactions are not deterministic. This will lead to issues while compare if a change is needed in

application-load-balancer-controller/pkg/controller/ingress/update.go

Line 238 in f698c9d

func listenersChanged(current, desired []albsdk.Listener) bool {

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.

dergeberl · 2026-06-29T13:41:35Z

+				WithRule("my-host.local", WithPath("/", new(networkingv1.PathTypePrefix), service.Name, networkingv1.ServiceBackendPort{Number: 80})),
+			)
+			testutil.CreateKubernetesResourceAndDeferDeletion(ctx, k8sClient, &ingress)
+


We also shpuld check that the IP is set in the status of the ingress.

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().

dergeberl · 2026-06-30T06:20:30Z

+		if !httpsOnly && (httpPort <= 0 || httpPort > math.MaxUint16) {
+			errors = append(errors, ErrorEvent{
+				Ingress:     ingress,
+				Description: "HTTP port is out of range.",
+			})
+			continue
+		}
+		if len(ingress.Spec.TLS) > 0 && (httpsPort <= 0 || httpsPort > math.MaxUint16) {
+			errors = append(errors, ErrorEvent{
+				Ingress:     ingress,
+				Description: "HTTPS port is out of range.",
+			})
+			continue
+		}
+


We should add a test for that.

Avoid the use of underscores at the beginning of variable names

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

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 20 commits June 30, 2026 15:21

Add a topic sentence to README

8d0bc92

Implement feedback for STACKIT package

b312eb3

Validate that HTTP and HTTPS port are in range

cdd9e18

Change port to uint16

b24f972

Remove custom scheme

a695688

Consistently use LoadBalancerName

5b65b47

Use newer DEV image

bcfcc56

Make tree receiver of addPath

8912388

Fix test assertion

1df502f

Initialize target pool as late as possible

d247bde

Add comment to clarify API behavior for health checks

245b06f

Check that health check port is set

e9658d2

Don't use the same name for variables and types

3992a4f

Avoid the use of underscores at the beginning of variable names

Remove early return after adding finalizer

632a531

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 6 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


		#### 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

	duplicateCerts = append(duplicateCerts, id)
	duplicateCerts = append(duplicateCerts, cert.Id)

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!

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!

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!

breuerfelix Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fischerman commented Jun 24, 2026 •

edited by nschad

Loading

breuerfelix Jun 30, 2026 •

edited

Loading

hown3d Jun 29, 2026 •

edited

Loading