WIP: Add Application Load Balancer Controller Manager by kamilprzybyl · Pull Request #879 · stackitcloud/cloud-provider-stackit

kamilprzybyl · 2026-03-17T09:27:48Z

How to categorize this PR?

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Breaking changes:

ske-prow · 2026-03-17T09:27:55Z

[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 timebertt 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

Adi146 · 2026-03-18T08:30:26Z

+	// generate self-signed certificates for the metrics server. While convenient for development and testing,
+	// this setup is not recommended for production.
+	//
+	// TODO(user): If you enable certManager, uncomment the following lines:


might be good to create separate examples for that

Adi146 · 2026-03-18T08:48:39Z

+	sort.SliceStable(ruleMetadataList, func(i, j int) bool {
+		a, b := ruleMetadataList[i], ruleMetadataList[j]
+		// 1. Host name (lexicographically)
+		if a.host != b.host {


does this work with * host matcher?

Adi146 · 2026-03-18T08:56:24Z

+}
+
+// cleanupCerts deletes the certificates from the Certificates API that are no longer associated with any Ingress in the IngressClass
+func (r *IngressClassReconciler) cleanupCerts(ctx context.Context, ingressClass *networkingv1.IngressClass, ingresses []*networkingv1.Ingress) error {


do we really want to delete certificates? That could be very dangerous as users might just not associate the certificate anymore and want to keep it.

Each Ingress TLS secret reference is mapped to a unique ALB Certificate resource. Even if multiple Ingresses reference the same k8s Secret, they will each trigger the creation of a separate, independent certificate on the ALB. That means, deleting one Ingress (and its associated ALB Certificate) does not impact the certificates still being used by other Ingresses.

Adi146 · 2026-03-18T08:57:19Z

+
+// isCertValid checks if the certificate chain is complete. It is used for checking if
+// the cert-manager's ACME challenge is completed, or if it's sill ongoing.
+func isCertValid(secret *corev1.Secret) (bool, error) {


isn't that already handled by the certificate api?

The Cert API only checks if the public and private keys are matching + if the certificate is a valid X.509 format but it does not reject incomplete chains.

Renamed the function from isCertValid to isCertReady and clarified the function in the comment above.

kamilprzybyl · 2026-03-22T10:13:48Z

We currently only delete certificates when the IngressClass is deleted or no Ingress references the IngressClass.
We need to add logic to clean up certificates that are no longer referenced by active Ingresses.

kamilprzybyl · 2026-03-25T11:53:05Z

We currently only delete certificates when the IngressClass is deleted or no Ingress references the IngressClass. We need to add logic to clean up certificates that are no longer referenced by active Ingresses.

hown3d · 2026-03-27T12:30:16Z

+	// It uses the trusted CAs from the operating system for validation.
+	tlsBridgingTrustedCaAnnotation = "alb.stackit.cloud/tls-bridging-trusted-ca"
+	// If set, the application load balancer enables TLS bridging with a custom CA provided as value.
+	tlsBridgingCustomCaAnnotation = "alb.stackit.cloud/tls-bridging-custom-ca"


This should probably use as value a reference to a TLS secret.

hown3d · 2026-03-27T12:31:00Z

+// It merges and sorts all routing rules across the ingresses based on host, priority, path specificity, path type, and ingress origin.
+// The resulting ALB payload includes targets derived from cluster nodes, target pools per backend service, HTTP(S) listeners,
+// and optional TLS certificate bindings. This spec is later used to create or update the actual ALB instance.
+func (r *IngressClassReconciler) albSpecFromIngress( //nolint:funlen,gocyclo // We go through a lot of fields. Not much complexity.


this is indeed complex, should be split up into multiple functions.

hown3d · 2026-03-27T12:34:27Z

+		return 0, fmt.Errorf("service not found: %s", path.Backend.Service.Name)
+	}
+
+	if path.Backend.Service.Port.Name != "" {


should be refactored to have less branching, 4 indents is too much imo.

hown3d · 2026-03-27T12:38:32Z

 ## Overview

-The STACKIT Cloud Provider includes both the Cloud Controller Manager (CCM) for managing cloud resources and the CSI driver for persistent storage. This deployment provides a unified solution for cloud integration and storage provisioning.
+The STACKIT Cloud Provider includes the Cloud Controller Manager (CCM) for managing cloud resources, the CSI driver for persistent storage and the Application Load Balancer Controller Manager (ALBCM) for managing STACKIT Application Load Balancer (ALB) via Ingress Resources.


we should have different deployment docs for each binary. CCM and ALBCM should be different md files. It's easier to comprehend.

# Conflicts: # go.mod # go.sum

fischerman · 2026-06-03T13:59:41Z

+	}
+
+	targets := []albsdk.Target{}
+	for _, node := range nodes.Items {


We should also filter out nodes that are to be removed, similar to the CCM. This also requires extending the watcher predicate.

…tests

hown3d

some more general code concerns.

hown3d · 2026-06-08T07:59:25Z

+        var evtErr *errorEvent
+
+        if errors.As(errItem, &evtErr) {
+            log.Info(evtErr.description, "typ", evtErr.typ, "ingressRef", evtErr.ingressRef)


Instead of filtering here for the error type, include the "typ" and "ingressRef" in the Error method of the errorEvent type. This way you can just always use errItem.Error().

hown3d · 2026-06-08T08:02:57Z

+	ctx context.Context,
+	class *networkingv1.IngressClass,
+	ingresses []networkingv1.Ingress,
+) (payload *albsdk.CreateLoadBalancerPayload, activeCertIDs map[string]string, validationErrors []error, err error) {


It is a bad practice to have separate error variables return and also having slices of errors. Please use one error type and use errors.Join to join your errors.
In the calling function you can use unwrap to get a list of all the errors.

hown3d · 2026-06-08T08:05:21Z

+	return e.description
+}
+
+func (r *IngressClassReconciler) SendEvents(class *networkingv1.IngressClass, validationErrors []error) {


This could be a method of the errorEvent type.

func (e *errorEvent) RecordEvent(class *networkingv1.IngressClass, recorder record.EventRecorder) { if evtErr.ingressRef.Name == "" { return } recorder.Eventf(class, corev1.EventTypeWarning, "ALB", "Error in %s %s in Namespace %s: %s", evtErr.ingressRef.Kind, evtErr.ingressRef.Name, evtErr.ingressRef.Namespace, evtErr.description) recorder.Event(&evtErr.ingressRef, corev1.EventTypeWarning, "ALB", evtErr.description) }

You currently parse the errors into the errorEvent to later call this SendEvents function to parse the errors as types again. This is not neccessary.

replaced SendEvents with RecordEvent and made it an errorEvent method. The new method records a single event instead of a list of events.

hown3d · 2026-06-08T08:09:57Z

+type errorEvent struct {
+	ingressRef  corev1.ObjectReference
+	description string
+	typ         string


This is always set to "error". What purpose does it bring?
Maybe a field.Path could be useful here for the user to directly see from what field in his ingress manifest the error stems.

replaced with field.NewPath

hown3d · 2026-06-08T08:11:35Z

+			continue
+		}
+		if dstTargetPool.skipCertificateValidation != srcTargetPool.skipCertificateValidation {
+			mergeErrors = append(mergeErrors, &errorEvent{


Please use errors.Join and don't return an error slice

hown3d · 2026-06-08T08:16:16Z

+| `alb.stackit.cloud/cookie-persistence-name` | IngressClass, Ingress | Optional | Sets the name for session cookie persistence. |
+| `alb.stackit.cloud/cookie-persistence-ttl-seconds`| IngressClass, Ingress | Optional | Sets the TTL (in seconds) for cookie persistence. |
+| `alb.stackit.cloud/priority` | Ingress | Optional | Defines the evaluation priority of the Ingress. |
+| `alb.stackit.cloud/traget-pool-tls-enabled` | IngressClass, Ingress, Service | Optional | Enables TLS bridging using OS trusted CAs. |


typos: target

hown3d · 2026-06-08T08:17:57Z

+Currently, the Ingress API is supported.
+Support for Gateway API is planned.
+
+##### Environment Variables


Maybe a section for "Get Started" that also links to the example deployment.yaml that tells a user how to actually run this alb controller in their cluster (if not managed by SKE).
Having a headline and then 5th headline with environment variable is confusing.

hown3d · 2026-06-08T08:19:00Z

+		For(&networkingv1.IngressClass{}, builder.WithPredicates(ingressClassPredicate())).
+		Watches(&corev1.Node{}, nodeEventHandler(r.Client), builder.WithPredicates(nodePredicate())).
+		Watches(&networkingv1.Ingress{}, ingressEventHandler(r.Client)).
+		Watches(&corev1.Secret{}, secretEventHandler(r.Client)).


Please make sure to disable the cache for secrets, as this would otherwise get quite huge pretty fast if we watch all secrets.

In addition, the secret contains a server side field selector for "type". Therefore we could just watch for secrets of type TLS to reduce API server churn

hown3d · 2026-06-08T08:19:39Z

+)
+
+func init() {
+	utilruntime.Must(clientgoscheme.AddToScheme(scheme))


no need to use a custom scheme if we just use kubernetes core resources

hown3d · 2026-06-08T08:22:49Z

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


please implement these changed functions with slices.EqualFunc

… tls annotations

Utilized getSortedIngressesForIngressClass for getAlbSpecForIngressClass

… alb specs via getAlbSpecForResources at last step

fixed unit tests and added more for ingress controller

unit tests for alb spec are added ensured that ephemeral address option defaults to false when internal-alb annotation is defined.

…errorEvent method; change logic to record a single event instead of a list of events

ske-prow · 2026-06-08T20:21:22Z

@kamilprzybyl: 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-cloud-provider-stackit-verify	`f36047f`	link	true	`/test pull-cloud-provider-stackit-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 Mar 17, 2026

kamilprzybyl changed the title ~~Add Application Load Balancer Controller Manager~~ WIP: Add Application Load Balancer Controller Manager Mar 17, 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 Mar 17, 2026

kamilprzybyl force-pushed the feat/kp/add-alb-ingress-controller branch from 7ecc416 to 86bdf1d Compare March 17, 2026 09:37

Adi146 reviewed Mar 18, 2026

View reviewed changes

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

fischerman reviewed Mar 19, 2026

View reviewed changes

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

kamilprzybyl force-pushed the feat/kp/add-alb-ingress-controller branch 2 times, most recently from d8de28a to f903b25 Compare March 20, 2026 19:05

kamilprzybyl closed this Mar 25, 2026

kamilprzybyl reopened this Mar 25, 2026

dergeberl mentioned this pull request Mar 25, 2026

Refactor config #915

Merged

dergeberl force-pushed the feat/kp/add-alb-ingress-controller branch from 3ad18ef to 9d6f767 Compare March 26, 2026 10:04

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

hown3d reviewed Mar 27, 2026

View reviewed changes

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

Kamil Przybyl and others added 9 commits May 11, 2026 14:20

add application load balancer controller manager

2fde114

# Conflicts: # go.mod # go.sum

chore: add alb ingress controller docs run-it-locally how-to

efaf6a0

Fix errors in stackit package

079953a

chore: add new Makefile build for alb ingress controller manager

04e5bf3

Fix errors in ingress package (only non-test files)

38b50a0

Add mocks for ALB and certificates API

686f0a7

Fix syntax errors in test in ingress package

68811dc

wip: Add alb-controller-manager deploy files

8076443

Fix main.go

b3fb5bd

chore: add a list of supported annotation

cde054d

kamilprzybyl force-pushed the feat/kp/add-alb-ingress-controller branch from ccab903 to cde054d Compare June 2, 2026 17:50

Kamil Przybyl added 8 commits June 2, 2026 19:52

refactor(docs): add links to sample setup

9d399f2

chore: clarify ingress to ALB mapping

8e41fa9

chore: remove redundant comment

4971747

refactor: simplify boolean return

ca9a9c9

refactor: return early when unmanaged ingressclass

078c2ed

docs: add ingress rule order evaluation

2a04142

docs(annotations): add requirement column

828761c

refactor: rename errorEvents struct to singular

5f117f6

fischerman reviewed Jun 3, 2026

View reviewed changes

Comment thread pkg/alb/ingress/add.go

fischerman reviewed Jun 3, 2026

View reviewed changes

Kamil Przybyl added 6 commits June 3, 2026 20:37

fix: flip the condition to fix events

320046b

refactor(events): implement error interface for errorEvent struct

b760bea

refactor: split updateNeeded function into smaller helpers; add unit …

7f13375

…tests

refactor: move controller options/flags into a struct

4059353

feat: add tls secret watcher

ed68ab9

refactor: cleanup orphaned certificates

8061441

hown3d reviewed Jun 8, 2026

View reviewed changes

fischerman and others added 10 commits June 8, 2026 16:01

Add cloud-config.yaml to gitignore

d031f3e

fix tls bridging missing tlspool config and add tests and fix typo in…

6913a03

… tls annotations

updated annotation lookup when sorting ingresses for ingressClass

ad97ccd

Utilized getSortedIngressesForIngressClass for getAlbSpecForIngressClass

updated tests

cc3f51c

introduced sequenceIndex to not lose the order of rules when creating…

91e23c7

… alb specs via getAlbSpecForResources at last step

deleted unused comment

2cb2228

fixed unit tests and added more for ingress controller

internal alb annotation is fixed

8d893b9

unit tests for alb spec are added ensured that ephemeral address option defaults to false when internal-alb annotation is defined.

Remove license headers

b0bc504

refactor: replace SendEvents function with RecordEvent and make it a …

4f2d8e7

…errorEvent method; change logic to record a single event instead of a list of events

refactor: replaced hardcoded typ field with field.NewPath

f36047f

Conversation

kamilprzybyl commented Mar 17, 2026

Uh oh!

ske-prow Bot commented Mar 17, 2026

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!

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!

kamilprzybyl Mar 20, 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!

Uh oh!

Uh oh!

kamilprzybyl commented Mar 22, 2026

Uh oh!

kamilprzybyl commented Mar 25, 2026

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hown3d 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!

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!

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!

ske-prow Bot commented Jun 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

kamilprzybyl Mar 20, 2026 •

edited

Loading