feat(network): mechanism-agnostic LoadBalancer exposure (ExposureClass + ServiceExposure) - #3081

Actionable comments posted: 2

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/controller/serviceexposure/backend/backend.go`:
- Around line 51-56: The Cilium LB-IPAM version pins in backend.go require a
minimum Cilium 1.16, so update the project documentation to state that this
controller only supports clusters running Cilium 1.16 or newer. Add the
requirement in the main docs/installation notes where compatibility is
described, referencing the CiliumLoadBalancerIPPool and ciliumPoolAPIVersion
behavior so users know cilium.io/v2 is not valid on earlier releases.

In `@packages/core/platform/templates/exposureclass.yaml`:
- Around line 24-27: The exposure class template only emits the default-class
annotation when .isDefault is explicitly set, so omitted values no longer follow
the documented true default. Update the exposureclass.yaml template to treat a
missing isDefault the same as true when rendering the annotations block, using
the existing .isDefault check in this template so values.yaml’s default behavior
is preserved.

---

Nitpick comments:
In `@hack/e2e-apps/serviceexposure.bats`:
- Around line 129-131: Remove the redundant inline fixture cleanup from the E2E
test body in serviceexposure.bats: the test should only assert the
ServiceExposure removal and rely on the existing teardown() helper to delete the
Service and exposureclass fixtures after each test. Keep the cleanup logic
centralized in teardown() and delete the explicit kubectl delete calls from the
deletion test block, preserving the ServiceExposure assertion and other test
flow unchanged.

In `@internal/controller/serviceexposure/status.go`:
- Around line 47-52: Replace the direct type assertion in reasonOf with
errors.As so wrapped reconcile errors still expose their reason instead of
falling back to "ReconcileError". Update the logic in reasonOf(err error) to
unwrap into a reconcileError using errors.As, and add the errors import in the
same file if it is not already present.

Code Review

This pull request introduces a mechanism-agnostic LoadBalancer exposure system by adding ExposureClass and ServiceExposure CRDs under the network.cozystack.io API group, along with their respective controllers and backend implementations (MetalLB, Cilium, externalIPs, and RobotLB). The review feedback highlights several actionable improvements: exposing and documenting the 'interfaces' field in the Helm templates and values.yaml, returning early after finalizer updates in the reconciler to avoid redundant work, using the 'backendCRDMissing' helper consistently, adopting 'errors.As' for robust error type assertions, and validating trimmed IP range boundaries to prevent malformed Cilium pool configurations.

Verdict

LGTM with non-blocking notes

The PR is mechanically clean, semantically sound, and behaves identically to today's clusters until an operator opts in by setting publishing.exposureClass.name. CRDs ship via the cozystack-controller chart's definitions/* glob, no migration is required (purely additive), upgrade- and fresh-install paths are both safe. Notes below are MINOR.

Findings

[MINOR] hack/e2e-apps/serviceexposure.bats:21,31 — setup() and teardown() shell functions are silently dead code under cozystack's bats runner.

hack/cozytest.sh:88-89 is explicit: "there are no bats setup/teardown directives here, this runner only knows test + bash." The awk converter (hack/cozytest.sh:118-135) recognises only ^@test "..."$ and emits test_* functions; the setup() and teardown() definitions are sourced into TMP_SH but never called. docs/agents/e2e-testing.md:31-36 reinforces this: do pre-cleanup inline at test start and inline cleanup at test end, no trap-based or hook-based cleanup. Concrete effect here: the second @test reuses cluster state created by the first (CLASS / SVC / EXP all carry over), so it passes against stale state without exercising the deletion path against a fresh resource. Fix by inlining kubectl ... delete --ignore-not-found ... at the top of each @test and at the end (the second @test already inlines the trailing cleanup; the first does not), or define a cozy_cleanup function which the EXIT trap honours.

[MINOR] api/network/v1alpha1/serviceexposure_types.go:34 — ServiceRef accepts an empty name.

corev1.LocalObjectReference emits name with default: "" and the surrounding properties block in the generated CRD has no required: [name]. A ServiceExposure created with spec.serviceRef: {} is admitted; the controller then reports Ready=False/ServiceNotFound against a lookup for "". Adding +kubebuilder:validation:Required plus +kubebuilder:validation:MinLength=1 on a wrapping field (or a custom ServiceReference type) catches the misconfiguration at admission instead of as a reconcile failure.

[MINOR] PR title scope feat(network) is not in the auto-labeler map.

.github/workflows/pr-labeler.yaml maps ingress, gateway, metallb, cilium, kube-ovn, etc. to area/networking, but network is not in the table, so this PR landed with both area/networking (manual) and area/uncategorized (auto-labeler fallback, which AGENTS.md describes as the "needs manual area selection" signal). Either rename the scope to networking (consistent with neighbours), or extend the labeler map with 'network': 'area/networking'.

Operational risks

• A ServiceExposure whose class resolves to backend metallb/cilium when the corresponding CRD is not installed surfaces as Ready=False/BackendUnavailable. The deletion-time reconcile in reconciler.go:93-108 falls through to a destructive class-wide reclaim only on BackendRenderError / BackendUnsupported — a BackendUnavailable leaves the finalizer pinning the SE until the operator either reinstalls the backend, switches the class to a different backend, or manually clears the finalizer. The conservative behaviour is correct; an operator-facing note (NOTES.txt or a doc snippet linked from the SE CRD description) on the unwedge recipe would save a future pager incident.

Caveats

• ciliumBlocks (backend/backend.go:161-180) classifies an address as a range iff it contains - and no /, else CIDR. A bare IPv6 like ::1 falls into the CIDR branch and is rendered as cidr: "::1", which CiliumLoadBalancerIPPool rejects (needs an explicit prefix). Edge case, not regression-blocking.
• Two @test blocks in hack/e2e-apps/serviceexposure.bats exercise only the externalIPs backend; the metallb and cilium rendering paths rely entirely on the Go unit tests. The file's own header comment justifies this (avoids collisions with real LB pools / L2 reachability), and unit coverage in reconciler_test.go / backend_test.go is thorough — flagging as caveat, not finding.
• publishing.exposureClass.isDefault defaults to true in packages/core/platform/values.yaml. With name: "" the whole block is gated off, but an operator who later sets name without explicitly setting isDefault: false will silently install a cluster-default class — any future ServiceExposure with an empty exposureClassName will resolve to it. Documented in the comment, but worth a NOTES.txt callout for operators copying values.

Recommended follow-ups

• Define an ApplicationDefinition for ServiceExposure (cozyrds pattern) so tenants surface it in the dashboard catalog — explicitly deferred to a later phase in the PR body, but tracking here so it isn't forgotten when the per-app exposure conversion happens.
• Consider a kubebuilder:validation:Pattern (or webhook) on ExposureClass.spec.addresses rejecting bare IPv6 entries for the cilium backend so the bug above can't reach the cluster.

LGTM — clean, opt-in foundation for mechanism-agnostic LoadBalancer exposure; the default externalIPs path is verifiably unchanged and the test coverage defines the full contract. The items below are non-blocking polish.

Business context: lays the API + controller foundation to migrate tenant LoadBalancer exposure off the deprecated Service.spec.externalIPs (KEP-5707) and eventually off MetalLB onto Cilium-native announcement — the mechanism-agnostic shape prescribed after the chart-side approach (#2468) was reverted in #2603. Part of #2586; converting per-app exposure, flipping defaults, and removing MetalLB are deferred to later phases.

Verified green: go build + go vet clean; 29/29 Go unit tests pass; re-running controller-gen leaves zero codegen drift (CRDs + deepcopy in sync); platform 75/75 and ingress 18/18 helm-unittests pass; the hardcoded Cilium apiVersions (cilium.io/v2 pool, cilium.io/v2alpha1 L2 policy) match the shipped Cilium v1.19.5; and with publishing.exposureClass.name: "" both new templates render nothing, so the default host-ingress path is byte-for-byte unchanged.

Non-blocking follow-ups

1. Template isDefault not defaulted in-template like l2 is — exposureclass.yaml gates the default-class annotation on {{- if .isDefault }} while l2 uses dig "l2" true .. Through normal values overrides this is harmless (Helm map-merge keeps the chart's isDefault: true default — verified), so the "default class silently dropped" concern doesn't materialize via values; it only bites a caller that hands the chart a fully-reconstructed map without the key. Mirroring the l2 pattern would make the two optional booleans behave identically and self-document. (inline)

2. No validation of malformed VIP ranges in ciliumBlocks — an address like "192.168.1.1-" yields {start, stop:""}, which Cilium rejects at apply time, leaving the exposure requeuing on a generic Create error rather than a clear Ready=False reason. Admin-supplied input and it fails loud at the cluster, so not a blocker, but a non-empty start/stop check (or a CRD pattern) returning a BackendRenderError would improve operability. MetalLB passes addresses through verbatim (MetalLB validates its own). (inline)

3. Finalizer-add doesn't return early before runReconcileSteps (reconciler.go) — functionally safe here (the Update refreshes resourceVersion so the later Status().Update won't conflict, and MaxConcurrentReconciles defaults to 1), so this is optional best-practice polish, not a fix.

4. Docs — no user-facing docs ship with this phase, which is fine for an opt-in foundation, but publishing.exposureClass.* is a new operator-facing knob; the migration story is worth landing against #2586 when defaults flip.

Strengths worth preserving: the class-level one-pool-per-ExposureClass model sidesteps the overlapping-pool problem (covered by TestReconcile_SecondExposureSharesOnePool); the subset-merge in applyObject (reconcile only ManagedSpecKeys, leave apiserver-defaulted keys intact) is proven idempotent and non-destructive; the finalizer/cross-scope cleanup and take-over guard are unusually thorough; and the fail guard turns the pool-backend + publishing.externalIPs dead-end into a loud error.

Note: dual-model leg was Claude-only (Codex unavailable in env); bot findings independently re-verified, build/test/codegen checks run directly.