revert(ingress)!: remove broken publishing.exposure=loadBalancer mode - #2603
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (6)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR removes the ChangesExposure Configuration Simplification
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
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 reverts the recently introduced 'publishing.exposure=loadBalancer' mode for ingress-nginx. The implementation was found to cause non-deterministic traffic routing and service unavailability due to conflicts with existing DNAT configurations and a lack of IP advertisement in the default Cozystack environment. By removing this mode, the system returns to its previous, stable state while allowing for a more robust, mechanism-agnostic migration strategy to be developed in the future. 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. 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
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extra/ingress/templates/nginx-ingress.yaml (1)
44-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the remaining
LoadBalancerfallback from ingress service renderingLine 50 still sets
type: LoadBalancerin theelsebranch, which effectively keeps the behavior this PR is reverting. This can still produce unreachable services in environments without L2/BGP announcement support. Prefer renderingClusterIPby default and only addingexternalIPswhen configured.Suggested patch
service: - {{- if and (eq $exposeIngress .Release.Namespace) $exposeExternalIPs }} + type: ClusterIP + externalTrafficPolicy: Cluster + {{- if and (eq $exposeIngress .Release.Namespace) $exposeExternalIPs }} externalIPs: {{- toYaml (splitList "," $exposeExternalIPs) | nindent 12 }} - type: ClusterIP - externalTrafficPolicy: Cluster - {{- else }} - type: LoadBalancer - externalTrafficPolicy: Local {{- end }}🤖 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 `@packages/extra/ingress/templates/nginx-ingress.yaml` around lines 44 - 52, The template currently falls back to rendering type: LoadBalancer and externalTrafficPolicy: Local in the else branch of the conditional that checks and (eq $exposeIngress .Release.Namespace) $exposeExternalIPs; change this so the default/else branch renders a ClusterIP service (no LoadBalancer) and do not set externalTrafficPolicy there — only inject externalIPs and (if needed) externalTrafficPolicy when $exposeExternalIPs is provided in the if branch; remove the type: LoadBalancer and externalTrafficPolicy: Local lines from the else branch and ensure type: ClusterIP is the default output unless externalIPs are explicitly configured.
🤖 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.
Outside diff comments:
In `@packages/extra/ingress/templates/nginx-ingress.yaml`:
- Around line 44-52: The template currently falls back to rendering type:
LoadBalancer and externalTrafficPolicy: Local in the else branch of the
conditional that checks and (eq $exposeIngress .Release.Namespace)
$exposeExternalIPs; change this so the default/else branch renders a ClusterIP
service (no LoadBalancer) and do not set externalTrafficPolicy there — only
inject externalIPs and (if needed) externalTrafficPolicy when $exposeExternalIPs
is provided in the if branch; remove the type: LoadBalancer and
externalTrafficPolicy: Local lines from the else branch and ensure type:
ClusterIP is the default output unless externalIPs are explicitly configured.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9a057e64-bd6d-4c51-9f7e-712f9dd88db0
📒 Files selected for processing (7)
packages/apps/tenant/templates/cilium-lb-pool.yamlpackages/apps/tenant/tests/exposure_test.yamlpackages/core/platform/templates/apps.yamlpackages/core/platform/values.yamlpackages/extra/ingress/README.mdpackages/extra/ingress/templates/nginx-ingress.yamlpackages/extra/ingress/tests/exposure_test.yaml
💤 Files with no reviewable changes (6)
- packages/extra/ingress/README.md
- packages/apps/tenant/tests/exposure_test.yaml
- packages/apps/tenant/templates/cilium-lb-pool.yaml
- packages/core/platform/templates/apps.yaml
- packages/core/platform/values.yaml
- packages/extra/ingress/tests/exposure_test.yaml
There was a problem hiding this comment.
Code Review
This pull request reverts the "loadBalancer" exposure mode for ingress services, removing the CiliumLoadBalancerIPPool template, configuration options, and associated tests. Feedback highlights that the new implementation for externalIPs fails to filter empty strings from the IP list, potentially resulting in invalid Service manifests. Furthermore, the PR title uses an unauthorized commit type, violating the project's conventional commit guidelines.
| {{- if and (eq $exposeIngress .Release.Namespace) $exposeExternalIPs }} | ||
| externalIPs: | ||
| {{- toYaml $exposeIPsList | nindent 12 }} | ||
| {{- toYaml (splitList "," $exposeExternalIPs) | nindent 12 }} |
There was a problem hiding this comment.
The previous implementation (which was removed in this PR) safely filtered out empty strings from the IP list. The new implementation using splitList directly on the raw string can result in empty strings being passed to the externalIPs field (e.g., if there is a trailing comma), which is invalid in a Kubernetes Service. Using the compact function will restore the previous robustness by removing any empty elements from the list.
{{- $ips := splitList "," $exposeExternalIPs | compact }}
{{- if and (eq $exposeIngress .Release.Namespace) $ips }}
externalIPs:
{{- toYaml $ips | nindent 12 }}| @@ -1,19 +1,5 @@ | |||
| {{- $exposeIngress := (index .Values._cluster "expose-ingress") | default "tenant-root" }} | |||
There was a problem hiding this comment.
The pull request title uses the commit type revert, which is not included in the list of valid types specified in the repository style guide. Please use one of the allowed types: feat, fix, docs, style, refactor, perf, test, build, ci, chore.
References
- Valid commit types are feat, fix, docs, style, refactor, perf, test, build, ci, chore. (link)
Reverts the ingress loadBalancer exposure mode added in #2468. The mode rendered the ingress-nginx Service as type: LoadBalancer and a CiliumLoadBalancerIPPool over publishing.externalIPs, as a migration path off Service.spec.externalIPs (deprecated upstream in Kubernetes v1.36 per KEP-5707). In default Cozystack the implementation produces two failure modes: - publishing.externalIPs is the operator-supplied list of node public IPs (addresses the cluster nodes already hold and listen on), not a virtual-VIP pool. Treating it as one and DNATing those IPs via Cilium LB-IPAM collides with the existing ingress-nginx DNAT on the same IPs (Service.spec.externalIPs path), so traffic is forwarded non-deterministically. - Cilium in default Cozystack runs with ipam.mode: kubernetes and no L2/BGP announcer, so a pool over addresses outside the node range is allocated but never advertised — the Service stays unreachable. The KEP-5707 deprecation is real, but the right migration is a mechanism-agnostic tenant API knob translated by a Cozystack-side controller into whichever backend the cluster admin has configured (MetalLB pool, Cilium LB-IPAM, robotlb, externalIPs pin) — not a chart-side IPPool baked into the tenant chart. That follow-up belongs in its own PR. Removed: - publishing.exposure value (and the inline comment) from core/platform/values.yaml - expose-mode plumbing line in core/platform/templates/apps.yaml - CiliumLoadBalancerIPPool template at apps/tenant/templates/cilium-lb-pool.yaml - type: LoadBalancer + externalTrafficPolicy: Local branch in extra/ingress/templates/nginx-ingress.yaml (Service renders back as ClusterIP + spec.externalIPs when externalIPs are set, falling through to type: LoadBalancer when they are not — pre-#2468 shape) - Exposure mode section from extra/ingress/README.md - exposure_test.yaml fixtures in both charts Operators who explicitly set publishing.exposure=loadBalancer will see their ingress Service revert to ClusterIP + spec.externalIPs on upgrade. Anyone on the default value (externalIPs) is unaffected. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
0642ee0 to
f919afe
Compare
…s + ServiceExposure) (#3081) ## What this PR does Lays the foundation for migrating tenant LoadBalancer exposure off the deprecated `Service.spec.externalIPs` (KEP-5707) and, eventually, off MetalLB onto Cilium-native announcement — the mechanism-agnostic shape prescribed when the earlier chart-side approach (#2468) was reverted in #2603. It adds a new `network.cozystack.io/v1alpha1` API group and a controller that translates intent into whichever LoadBalancer backend the cluster admin configured: - `ExposureClass` (cluster-scoped, admin): binds a logical class name to a backend (`externalIPs` | `metallb` | `cilium` | `robotlb`) and its VIP address scope — the StorageClass/IngressClass pattern. - `ServiceExposure` (namespaced, tenant intent): "expose this Service via class X"; status reports the resolved backend and assigned IPs. - A `serviceexposure` controller renders the backend's pool + announcer once per class (MetalLB `IPAddressPool` + `L2Advertisement`; Cilium `CiliumLoadBalancerIPPool` + `CiliumL2AnnouncementPolicy`), scoped to the namespaces that opted in. Backends are pluggable behind a small Go interface, so adding BGP or another cloud LB is one implementation, not an API change. The controller never mutates the target Service — the chart still owns its shape. Platform wiring: `publishing.exposureClass.*` renders the admin `ExposureClass` and the host ingress publishes through it. The default (no class set, `externalIPs`) leaves every existing cluster's behaviour byte-for-byte unchanged; the MetalLB→Cilium announcer migration becomes a single `ExposureClass.spec.backend` change. This is the foundation only — Part of #2586. Converting per-app exposure, flipping Cilium `l2announcements` on, changing the default, and removing MetalLB are deferred to later phases. ### Release note ```release-note feat(network): add ExposureClass and ServiceExposure for mechanism-agnostic LoadBalancer exposure. Opt-in foundation; the default externalIPs publishing path is unchanged. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added new `ExposureClass` and `ServiceExposure` custom resources with support for multiple backends (externalIPs, MetalLB, Cilium, RobotLB), including optional L2 and address configuration. * Added Helm wiring and templates to enable service/ingress publishing via `ServiceExposure`, selecting a configured (or default) `ExposureClass`. * **Bug Fixes** * Improved reconciliation, finalizer-based cleanup, and status updates for readiness and assigned IP reporting, including safer handling when backends or managed resources change. * **Tests** * Added end-to-end and unit test coverage for backend rendering, readiness, and cleanup flows. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Reverts the
publishing.exposure=loadBalancermode for ingress-nginx added in #2468.The mode rendered the ingress-nginx Service as
type: LoadBalancerand aCiliumLoadBalancerIPPooloverpublishing.externalIPs, as a migration path offService.spec.externalIPs(deprecated upstream in Kubernetes v1.36 per KEP-5707). In default Cozystack the implementation produces two failure modes:publishing.externalIPsis the operator-supplied list of node public IPs (addresses the cluster nodes already hold and listen on, see docs section "4b. Node Public IP Setup"), not a virtual-VIP pool. Treating it as one and DNATing those IPs via Cilium LB-IPAM collides with the existing ingress-nginx DNAT on the same IPs (Service.spec.externalIPspath), so traffic is forwarded non-deterministically.ipam.mode: kubernetesand no L2/BGP announcer, so a pool over addresses outside the node range is allocated but never advertised — the Service stays unreachable.The KEP-5707 deprecation is real, but the right migration is a mechanism-agnostic tenant API knob translated by a Cozystack-side controller into whichever backend the cluster admin has configured (MetalLB pool, Cilium LB-IPAM, robotlb, externalIPs pin) — not a chart-side
IPPoolbaked into the tenant chart. That follow-up belongs in its own PR.Removed
publishing.exposurevalue (and the inline comment) fromcore/platform/values.yaml.expose-modeplumbing line incore/platform/templates/apps.yaml.CiliumLoadBalancerIPPooltemplate atapps/tenant/templates/cilium-lb-pool.yaml.type: LoadBalancer + externalTrafficPolicy: Localbranch inextra/ingress/templates/nginx-ingress.yaml. The Service now renders asClusterIP + spec.externalIPswhen externalIPs are set, falling through totype: LoadBalancerwhen they are not — pre-feat(ingress): add loadBalancer exposure mode via CiliumLoadBalancerIPPool #2468 shape.extra/ingress/README.md.exposure_test.yamlfixtures in both charts.Compatibility
Operators who explicitly set
publishing.exposure=loadBalancerwill see their ingress Service revert toClusterIP + spec.externalIPson upgrade. Anyone on the default value (externalIPs) is unaffected.Release note
Summary by CodeRabbit
Release Notes
Refactor
Documentation
Tests