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

Limit is still 250. SKE will look into limiting the auto scaler to 250 for clusters the have the ALB extension enabled.

dergeberl

Comments for the stackit package and testutil

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.

TODO: Document behavior when two certificates collide.

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

Will keep the thread unresolved as a reminder.

dergeberl · 2026-07-10T11:59:50Z

+	for {
+		req := cl.client.DefaultAPI.ListCertificates(ctx, projectID, region)
+		if nextPage != "" {
+			req = req.PageId(nextPage)
+		}
+		page, err := req.Execute()
+		if err != nil {
+			return nil, err
+		}
+		certs = append(certs, page.Items...)
+		if ptr.Deref(page.NextPageId, "") != "" {
+			nextPage = *page.NextPageId
+		} else {
+			break
+		}
+	}


I think we need a context with timeout here and also check if the context is closed in the for loop. The SDK itself should have a timeout in the HTTP client which would error the call if something is stuck. If the SKD returns no errors but always a NextPageID we would stuck in that loop forever.

Nit:
I would prefer turn around the check and avoid the else. Like this:

nextPage = ptr.Deref(page.NextPageId, "") if nextPage == "" { break } }

I think we need a context with timeout here...

Why? As you said yourself: "The SDK itself should have a timeout in the HTTP client which would error the call if something is stuck"

and also check if the context is closed in the for loop.

I pass the context into every request. So that is covered.

If the SKD returns no errors but always a NextPageID we would stuck in that loop forever.

That would require a severe bug in the API or SDK. To protect against the worst I've added a hard limit on the number of pages in cfe62b2. But that should never happen.

I would prefer turn around the check and avoid the else. Like this:

Also done in cfe62b2

Append ingress class name to ALB name

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/update.go Outdated

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

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

Comment thread pkg/controller/ingress/ingressclass_controller.go

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

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 18 commits July 1, 2026 17:52

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

Paraphrase limits

d4ea623

Remove sanitize label function

60459aa

Added controller tests for node changes

a578e28

fischerman commented Jul 10, 2026

View reviewed changes

Comment thread docs/user.md

dergeberl reviewed Jul 10, 2026

View reviewed changes

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

dergeberl reviewed Jul 10, 2026

View reviewed changes

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

fischerman added 8 commits July 10, 2026 15:49

Log event while waiting for load balancer to become ready

0e9f09a

Append ingress class name to ALB name

Refactor tests for certificate changes

b5a40a3

Fix linter issues

74aa536

Add page limit for listing certificates

cfe62b2

Refactor HTTPS option diff

076a909

Refactor isNodeTerminating with helper functions

be55146

Add troubleshooting section

a508db2

Run go mod tidy

eb25ad3

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dergeberl Jun 29, 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

fischerman commented Jun 24, 2026 •

edited by nschad

Loading

dergeberl Jun 29, 2026 •

edited

Loading

fischerman Jul 10, 2026 •

edited

Loading