Repository navigation
refactor(network): drop ExposureClass/ServiceExposure for native loadBalancerClass - #3218
Conversation
github-actions
Bot
added
area/uncategorized
📝 WalkthroughWalkthroughThis PR removes the network.cozystack.io/v1alpha1 API (ExposureClass, ServiceExposure), their CRDs, controllers, backends, and tests, along with manager wiring and RBAC rules. It replaces the exposure-class mechanism with native Kubernetes loadBalancerClass configuration, wired through platform values, cluster secrets, and ingress templates with an immutability guard. ChangesExposureClass/ServiceExposure API and controller removal
Native loadBalancerClass adoption
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant HelmRender as nginx-ingress.yaml
participant K8sAPI as Kubernetes API
participant Service as ingress-nginx Service
Operator->>HelmRender: render chart with publishing.loadBalancerClass
HelmRender->>K8sAPI: lookup existing host Service
K8sAPI-->>HelmRender: existing Service (type, loadBalancerClass)
alt Service is LoadBalancer and class differs
HelmRender-->>Operator: fail render (immutability error)
else class matches or no Service exists
HelmRender->>Service: set spec.loadBalancerClass
HelmRender-->>Operator: render succeeds
end
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
size/XXL
…s/ServiceExposure Aligns the expose field with cozystack/cozystack#3218, which removes the network.cozystack.io ExposureClass/ServiceExposure API group. expose[].class now maps to a native Service.spec.loadBalancerClass; each entry renders an additive type: LoadBalancer Service and the address is read from status.loadBalancer.ingress. The dedicated exposure API is recorded as a rejected alternative. Signed-off-by: Andrei Kvapil <[email protected]>
…BalancerClass Remove the network.cozystack.io API group (ExposureClass + ServiceExposure) and its reconciler. Managed-application charts own their Service, so native type: LoadBalancer + loadBalancerClass + an admin-provisioned pool covers external exposure without a dedicated API group and controller. The host ingress -- the only consumer -- keeps rendering type: LoadBalancer and gains an optional publishing.loadBalancerClass to pick the LB controller; the externalIPs (node-IP) default path is unchanged. Rationale in #3164. Assisted-By: Claude Signed-off-by: Andrei Kvapil <[email protected]>
…ress publishing.loadBalancerClass is a host-publishing setting, but its _cluster.load-balancer-class block was emitted on any ingress taking the LoadBalancer branch — including tenant ingresses — unlike expose-external-ips and proxy-protocol, which gate on the host namespace. A tenant ingress would then be claimed by an LB controller the tenant never selected. Gate it the same way, and add helm-unittest coverage for the host/tenant split and the platform to _cluster wiring. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…ting Services spec.loadBalancerClass is immutable once a Service is type LoadBalancer, so setting, changing, or clearing publishing.loadBalancerClass on a cluster whose host ingress Service already exists would make Flux wedge on an API-rejected patch instead of migrating. Fail the render early with the exact recreate command whenever an existing type: LoadBalancer Service's class differs from the requested value; a fresh install (lookup nil) or a ClusterIP Service transitioning to LoadBalancer is left untouched. Note the caveat in the values comment and cover the guard with helm-unittest. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
9634de9 to
43104c9
Compare
|
Pushed two follow-up commits and rebased the branch onto current Host-scoping fix — Immutability guard — Both are covered by helm-unittest (host/tenant split, plus the guard's set/change/clear/silent paths via a mocked Heads-up: the rebase replayed the |
dosubot
Bot
added
area/networking
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM at the code level.
I authored #3081 (the API group this removes), and I agree the managed-app path is fully covered by native Service type: LoadBalancer + loadBalancerClass. The only ServiceExposure capability lost is dynamic per-namespace VIP-pool scoping, which the sole consumer — the host ingress — doesn't need. The removal is clean: no dangling references, go build green, RBAC and codegen consistent, and it's main-only (absent from release-1.4/release-1.5), so no migration.
For transparency, I pushed two fix commits on top: host-scoping loadBalancerClass (it was leaking from the host ingress onto tenant ingresses) and a lookup-based guard that fails early instead of wedging Flux on the immutable-field migration. Both are covered by helm-unittest.
Whether to remove the group at all is the design call in #3164 — this is a code-level LGTM, not a substitute for that decision.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
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 `@hack/update-codegen.sh`:
- Line 94: The controller-gen invocation in update-codegen.sh passes the CRD
artifacts config path unquoted, which can break when TMPDIR contains spaces or
glob characters. Update the command so the output:crd:artifacts:config argument
in the controller-gen call is quoted or otherwise safely passed as a single
shell argument, and keep the change localized to the codegen command that uses
TMPDIR.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a206bc26-3b1e-48d7-a216-9bb23a7ae27d
📒 Files selected for processing (35)
api/network/v1alpha1/exposureclass_types.goapi/network/v1alpha1/groupversion_info.goapi/network/v1alpha1/serviceexposure_types.goapi/network/v1alpha1/zz_generated.deepcopy.gocmd/cozystack-controller/main.gohack/e2e-apps/serviceexposure.batshack/update-codegen.shinternal/controller/serviceexposure/backend/backend.gointernal/controller/serviceexposure/backend/backend_test.gointernal/controller/serviceexposure/backend/cilium.gointernal/controller/serviceexposure/backend/externalips.gointernal/controller/serviceexposure/backend/metallb.gointernal/controller/serviceexposure/backend/registry.gointernal/controller/serviceexposure/backend/robotlb.gointernal/controller/serviceexposure/classreconciler.gointernal/controller/serviceexposure/conflict.gointernal/controller/serviceexposure/mapper.gointernal/controller/serviceexposure/mapper_test.gointernal/controller/serviceexposure/reconciler.gointernal/controller/serviceexposure/reconciler_test.gointernal/controller/serviceexposure/status.gopackages/core/platform/templates/apps.yamlpackages/core/platform/templates/exposureclass.yamlpackages/core/platform/tests/apps_exposure_class_test.yamlpackages/core/platform/tests/apps_load_balancer_class_wiring_test.yamlpackages/core/platform/tests/exposureclass_wiring_test.yamlpackages/core/platform/values.yamlpackages/extra/ingress/templates/nginx-ingress.yamlpackages/extra/ingress/templates/serviceexposure.yamlpackages/extra/ingress/tests/load_balancer_class_immutable_test.yamlpackages/extra/ingress/tests/load_balancer_class_test.yamlpackages/extra/ingress/tests/serviceexposure_test.yamlpackages/system/cozystack-controller/definitions/network.cozystack.io_exposureclasses.yamlpackages/system/cozystack-controller/definitions/network.cozystack.io_serviceexposures.yamlpackages/system/cozystack-controller/templates/rbac.yaml
💤 Files with no reviewable changes (27)
- packages/extra/ingress/tests/serviceexposure_test.yaml
- packages/extra/ingress/templates/serviceexposure.yaml
- internal/controller/serviceexposure/reconciler.go
- internal/controller/serviceexposure/conflict.go
- api/network/v1alpha1/serviceexposure_types.go
- hack/e2e-apps/serviceexposure.bats
- internal/controller/serviceexposure/backend/registry.go
- packages/system/cozystack-controller/definitions/network.cozystack.io_serviceexposures.yaml
- internal/controller/serviceexposure/status.go
- packages/core/platform/tests/apps_exposure_class_test.yaml
- packages/core/platform/templates/exposureclass.yaml
- internal/controller/serviceexposure/classreconciler.go
- packages/system/cozystack-controller/definitions/network.cozystack.io_exposureclasses.yaml
- internal/controller/serviceexposure/backend/metallb.go
- internal/controller/serviceexposure/backend/backend.go
- internal/controller/serviceexposure/mapper.go
- internal/controller/serviceexposure/backend/externalips.go
- api/network/v1alpha1/zz_generated.deepcopy.go
- internal/controller/serviceexposure/backend/cilium.go
- internal/controller/serviceexposure/backend/robotlb.go
- internal/controller/serviceexposure/backend/backend_test.go
- packages/core/platform/tests/exposureclass_wiring_test.yaml
- internal/controller/serviceexposure/mapper_test.go
- api/network/v1alpha1/exposureclass_types.go
- api/network/v1alpha1/groupversion_info.go
- cmd/cozystack-controller/main.go
- internal/controller/serviceexposure/reconciler_test.go
|
|
||
| $CONTROLLER_GEN object:headerFile="hack/boilerplate.go.txt" paths="./api/..." | ||
| $CONTROLLER_GEN rbac:roleName=manager-role crd paths="./api/v1alpha1/..." paths="./api/backups/..." paths="./api/gateway/..." paths="./api/network/..." output:crd:artifacts:config=${TMPDIR} | ||
| $CONTROLLER_GEN rbac:roleName=manager-role crd paths="./api/v1alpha1/..." paths="./api/backups/..." paths="./api/gateway/..." output:crd:artifacts:config=${TMPDIR} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Quote the CRD output path.
${TMPDIR} is interpolated into a controller-gen argument unquoted, so a temp directory path containing spaces or glob characters can split the argument.
Proposed fix
-$CONTROLLER_GEN rbac:roleName=manager-role crd paths="./api/v1alpha1/..." paths="./api/backups/..." paths="./api/gateway/..." output:crd:artifacts:config=${TMPDIR}
+$CONTROLLER_GEN rbac:roleName=manager-role crd paths="./api/v1alpha1/..." paths="./api/backups/..." paths="./api/gateway/..." output:crd:artifacts:config="${TMPDIR}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $CONTROLLER_GEN rbac:roleName=manager-role crd paths="./api/v1alpha1/..." paths="./api/backups/..." paths="./api/gateway/..." output:crd:artifacts:config=${TMPDIR} | |
| $CONTROLLER_GEN rbac:roleName=manager-role crd paths="./api/v1alpha1/..." paths="./api/backups/..." paths="./api/gateway/..." output:crd:artifacts:config="${TMPDIR}" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 94-94: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/update-codegen.sh` at line 94, The controller-gen invocation in
update-codegen.sh passes the CRD artifacts config path unquoted, which can break
when TMPDIR contains spaces or glob characters. Update the command so the
output:crd:artifacts:config argument in the controller-gen call is quoted or
otherwise safely passed as a single shell argument, and keep the change
localized to the codegen command that uses TMPDIR.
Source: Linters/SAST tools
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request simplifies the network exposure architecture by deprecating the custom network.cozystack.io API group in favor of native Kubernetes LoadBalancer services. By leveraging the standard loadBalancerClass field, the platform reduces operational complexity and removes the need for a dedicated controller to manage VIP pools and announcers, aligning the project more closely with standard Kubernetes practices. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
…review - #29 (ServiceExposure/ExposureClass) was removed by cozystack/cozystack#3218 in favor of native Service type: LoadBalancer + loadBalancerClass; retarget the tunnel entry point and drop the Phase 1 blocker - record initiator-vs-responder as an explicit decision: Phase 1 responder-only (native LB), dial-out/either-end deferred to Phase 4 gated on stable per-tenant egress IPs (#35) - reframe Phase 2 (site-gateway/NAT) as a deliberate design re-open - add portal-side image/cloud-init synchronization requirement - record NETMAP-style 1:1 subnet mapping as NAT future work Assisted-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
…review - #29 (ServiceExposure/ExposureClass) was removed by cozystack/cozystack#3218 in favor of native Service type: LoadBalancer + loadBalancerClass; retarget the tunnel entry point and drop the Phase 1 blocker - record initiator-vs-responder as an explicit decision: Phase 1 responder-only (native LB), dial-out/either-end deferred to Phase 4 gated on stable per-tenant egress IPs (#35) - reframe Phase 2 (site-gateway/NAT) as a deliberate design re-open - add portal-side image/cloud-init synchronization requirement - record NETMAP-style 1:1 subnet mapping as NAT future work Assisted-By: Claude Opus 4.8 (1M context) <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
… values The publishing.* reference on the next trunk documented five publishing.exposureClass.* keys the platform no longer has: the ExposureClass/ServiceExposure machinery was dropped for the native Service loadBalancerClass (cozystack/cozystack#3218), and packages/core/platform/values.yaml carries no exposureClass key. Remove those rows and rewrite the publishing.exposure note that still named exposureClass as the successor. Add the publishing.loadBalancerClass row that replaced them and never had one: it names the LoadBalancer controller for the host ingress Service, the address pool or announcer is provisioned out of band, and because spec.loadBalancerClass is immutable an existing Service has to be recreated once when the value changes. Add the publishing.serviceDomain row for the new display-only DNS suffix the dashboard shows under the external IP of LoadBalancer Services (cozystack/cozystack#4035). Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
3 participants
What
Removes the
network.cozystack.ioAPI group —ExposureClass+ServiceExposure(#3081) — and its reconciler, in favor of nativeServicetype: LoadBalancer+loadBalancerClass.Why
Full rationale and discussion in #3164. For managed applications the chart already owns the Service, so:
type: LoadBalancer+loadBalancerClass+ an admin-provisioned pool covers vendor-neutral external exposure without a dedicated API group and controller;externalIPs(node-IP) default never went throughServiceExposure(its backend is a no-op) and is left unchanged;type: LoadBalancerand just drops theServiceExposure; the admin provisions the address pool / announcer out of band, as the install docs already assume.ServiceExposure's one unique feature — dynamic per-namespace scoping of a shared VIP pool — is a convenience the managed-app path does not need, and it does not justify a second tenant-networking API group next tosdn.cozystack.io.Changes
api/network/v1alpha1andinternal/controller/serviceexposure(+ CRD definitions, e2e bats, controller registration, RBAC).ServiceExposuretemplate; add an optionalpublishing.loadBalancerClassthat setsspec.loadBalancerClasson the host-ingress LoadBalancer Service.externalIPspath and default behavior are unchanged.Verification
go build ./...— green.helm unittest—core/platform72/72,extra/ingress25/25 (incl. host-vs-tenantloadBalancerClassscoping and the immutable-field migration guard).Migration
None required.
network.cozystack.io(#3081) has never shipped in a release — it is onmainonly (not backported torelease-1.5; absent from v1.4.x / v1.5.x), so no upgrading cluster has these CRDs and this removes it before it is ever released.Clusters that track
mainand explicitly setpublishing.exposureClass(opt-in, only since 2026-06-30) should provision their MetalLB / Cilium address pool themselves and setpublishing.loadBalancerClass; any orphanedcozystack-<class>pool can be deleted by hand. Default clusters (externalIPs path) are unaffected.Status
Draft / RFC for the #3164 discussion — this removes an API group that landed only days ago, so nothing here should merge before the design call. cc Aleksei Sviridkin (@lexfrei) (author of #3081) and Timofei Larkin (@lllamnyp) (#3164).
Deferred: when Kubernetes actually removes
Service.spec.externalIPs, the node-IP default migrates via a MetalLBIPAddressPoolwith noL2Advertisement(allocate-but-don't-announce) — a separate change.Summary by CodeRabbit
New Features
loadBalancerClassvalue.Bug Fixes