Skip to content

feat(ingress): add loadBalancer exposure mode via CiliumLoadBalancerIPPool - #2468

Merged
Aleksei Sviridkin (lexfrei) merged 5 commits into
mainfrom
feat/ingress-lb-ipam
Apr 28, 2026
Merged

feat(ingress): add loadBalancer exposure mode via CiliumLoadBalancerIPPool#2468
Aleksei Sviridkin (lexfrei) merged 5 commits into
mainfrom
feat/ingress-lb-ipam

Conversation

@lexfrei

@lexfrei Aleksei Sviridkin (lexfrei) commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Adds an opt-in publishing.exposure=loadBalancer mode for the ingress-nginx Service as a migration path away from Service.spec.externalIPs, which is deprecated upstream in Kubernetes v1.36 (KEP-5707, kubernetes#137293). The AllowServiceExternalIPs feature gate is expected to default to off around v1.40 and the implementation to be removed around v1.43.

Stacked on top of #2464 (cilium v1.19.3 bump) — depends on it for the CiliumLoadBalancerIPPool at cilium.io/v2.

Behavior

  • New platform value publishing.exposure — enum externalIPs | loadBalancer, default externalIPs (current behavior unchanged on upgrade).
  • Plumbed through cozystack-values into each tenant's ingress HelmRelease via the new expose-mode key.
  • When exposure=loadBalancer and the current namespace matches publishing.ingressName, the Service becomes type: LoadBalancer with externalTrafficPolicy: Local and a CiliumLoadBalancerIPPool announces the addresses from publishing.externalIPs via Cilium LB IPAM.
  • The pool uses a namespace-only serviceSelector (io.kubernetes.service.namespace: <ns>) — any LoadBalancer Service in the tenant namespace draws from it. See "Pool ownership" below.
  • IPv4 addresses get /32 CIDRs, IPv6 addresses get /128. Mixed families supported. Pre-CIDR entries (192.0.2.10/32) are accepted without double-suffixing.
  • Unknown values, loadBalancer with empty externalIPs, and stray empty entries from publishing.externalIPs are rejected at render time with explicit error messages.

Pool ownership

The CiliumLoadBalancerIPPool is rendered from packages/apps/tenant/templates/cilium-lb-pool.yaml, not from the ingress chart. The tenant chart is the per-tenant owner of cross-cutting resources (Namespace, cozystack-values Secret, HelmReleases for ingress and gateway), so one pool per tenant lives there.

Cilium LB IPAM rejects overlapping CIDRs across pools regardless of serviceSelector — the last-added pool gets cilium.io/PoolConflict and stops allocating. Keeping the pool in the ingress chart would collide with the Gateway-API PR (#2470), which materialises its own LoadBalancer Service from the same publishing.externalIPs range when tenant.spec.gateway=true. Moving the pool to the tenant chart with a namespace-only selector lets a single pool back both services (ingress-nginx today, a Cilium Gateway Service once #2470 lands).

Only the ingress-loadBalancer signal is wired here (_cluster.expose-mode=loadBalancer + .Values.ingress=true + publishing tenant). #2470 rebases on top of this PR, drops its own packages/extra/gateway/templates/cilium-lb-pool.yaml, and adds an OR branch for .Values.gateway in the tenant template.

Scope

Only the ingress-nginx Service is migrated by this setting. Other cozystack components that still write Service.spec.externalIPs directly (notably the vpn app at packages/apps/vpn/templates/service.yaml) need separate follow-up before Kubernetes v1.40.

Tests

  • packages/extra/ingress/tests/exposure_test.yaml — 10 helm-unittest cases on the ingress-nginx Service: type / externalTrafficPolicy / externalIPs assertion for both modes, unknown-mode rejection (case-sensitive enum), empty-IPs failure, empty-entry filtering, non-publishing-tenant fallback.
  • packages/apps/tenant/tests/exposure_test.yaml — 9 cases on the pool itself: IPv4, IPv6, mixed, pre-CIDR input, empty-entry filtering, ingress=false in publishing tenant (no pool), non-publishing tenant (no pool), empty externalIPs (no pool).
  • Both suites are auto-discovered by hack/helm-unit-tests.sh via the test: target in each package's Makefile.

Caveats (copied from the inline values.yaml comment)

  • loadBalancer mode uses externalTrafficPolicy: Local. The external IP must already be routed to a node that hosts an ingress pod (floating IP / upstream router / podAntiAffinity).
  • Cilium does not announce the IP on its own unless L2 announcements or BGP are enabled in the Cilium values (disabled by default in cozystack).
  • Switching the value on a running cluster causes the ingress-nginx Service to be recreated (upgrade.force: true on the HelmRelease + Service kind change); expect a brief interruption of ingress traffic.

Release note

feat(ingress): add opt-in publishing.exposure=loadBalancer mode that uses type: LoadBalancer + CiliumLoadBalancerIPPool instead of the deprecated Service.spec.externalIPs. Default (externalIPs) preserves existing behavior.

@coderabbitai

coderabbitai Bot commented Apr 23, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This PR introduces configurable ingress exposure modes via a new publishing.exposure configuration field that allows switching between deprecated Service.spec.externalIPs and LoadBalancer modes with Cilium LB IPAM. Changes span platform configuration, ingress-nginx service selection logic, tenant load balancer pool templating, and corresponding test suites with documentation updates.

Changes

Cohort / File(s) Summary
Configuration & Platform
packages/core/platform/values.yaml, packages/core/platform/templates/apps.yaml
Adds new publishing.exposure field (default: externalIPs) to control ingress Service exposure mode, passed to rendered cozystack-values Secret. Includes documentation on mode behaviors, IP CIDR conversion, traffic policy implications, and deprecation timeline.
Ingress Service Logic
packages/extra/ingress/templates/nginx-ingress.yaml
Updates ingress-nginx template to conditionally select Service type based on expose-mode: uses LoadBalancer with externalTrafficPolicy: Local for loadBalancer mode, or ClusterIP with spec.externalIPs for externalIPs mode. Includes validation and empty IP list checks.
Tenant Load Balancer Pool
packages/apps/tenant/templates/cilium-lb-pool.yaml
New template conditionally generates CiliumLoadBalancerIPPool resource when expose-mode=loadBalancer, ingress matches current namespace, and .Values.ingress enabled. Parses comma-separated external IPs into CIDR blocks with /32 (IPv4) or /128 (IPv6) suffixes and restricts pool to namespace-scoped services.
Test Suites
packages/extra/ingress/tests/exposure_test.yaml, packages/apps/tenant/tests/exposure_test.yaml
Comprehensive Helm unit tests validating Service configuration selection, CiliumLoadBalancerIPPool rendering conditions, CIDR conversion logic, IPv4/IPv6 handling, empty IP filtering, and error cases (invalid modes, missing IPs).
Build & Documentation
packages/apps/tenant/Makefile, packages/extra/ingress/Makefile, packages/extra/ingress/README.md
Adds test Makefile targets for Helm unittest execution and documents exposure modes in ingress README with behavioral caveats, traffic policy expectations, and migration scope clarification.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Poem

🐰 A config option hops into view,
Exposure modes now—old ways bid adieu,
Cilium pools catch IPs with care,
While LoadBalancer services glide through the air! 🚀

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main feature: adding a loadBalancer exposure mode for ingress-nginx that uses CiliumLoadBalancerIPPool. It accurately represents the primary change in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ingress-lb-ipam

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@lexfrei
Aleksei Sviridkin (lexfrei) marked this pull request as ready for review April 23, 2026 12:47
@dosubot dosubot Bot added the size/S This PR changes 10-29 lines, ignoring generated files label Apr 23, 2026
@dosubot dosubot Bot added the kind/feature Categorizes issue or PR as related to a new feature label Apr 23, 2026
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, 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 adds an opt-in 'loadBalancer' exposure mode for the ingress-nginx service to prepare for the upcoming deprecation of 'Service.spec.externalIPs' in Kubernetes. It provides a migration path by leveraging CiliumLoadBalancerIPPool for IP management, while maintaining the existing 'externalIPs' behavior as the default to ensure backward compatibility.

Highlights

  • New Exposure Mode: Introduced a new 'loadBalancer' exposure mode for ingress-nginx, allowing migration away from the deprecated 'Service.spec.externalIPs' field.
  • Cilium Integration: Implemented 'CiliumLoadBalancerIPPool' support to manage external IP advertisements via Cilium LB IPAM when the new mode is enabled.
  • Validation and Testing: Added robust validation to prevent invalid configurations and included comprehensive helm-unittest cases to ensure correct behavior across various scenarios.

🧠 New Feature in Public Preview: You can now enable Memory 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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment Gemini (@gemini-code-assist) Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize 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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. 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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an exposure mode configuration for the platform and ingress charts, facilitating a transition from the deprecated Service.spec.externalIPs field to a LoadBalancer service type utilizing CiliumLoadBalancerIPPool. The changes include updated templates, comprehensive documentation, and a new suite of unit tests. Feedback suggests adding the new exposure field to the platform's values.schema.json for UI visibility and input validation, as well as refining the CIDR suffix logic in the Cilium pool template to handle cases where input IPs might already contain a mask.

# (e.g. the vpn app at packages/apps/vpn/templates/service.yaml) are NOT
# migrated by flipping this value and must be addressed separately before
# the AllowServiceExternalIPs feature gate flips to off in ~v1.40.
exposure: externalIPs # "externalIPs" or "loadBalancer"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The exposure field is introduced here with extensive documentation, but it should also be added to the values.schema.json file for the platform chart to ensure validation and visibility in the dashboard UI. This aligns with the project's Helm chart conventions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The platform chart at packages/core/platform/ does not ship a values.schema.json — it reads runtime cluster data (ConfigMaps, Secrets, the cozystack system namespace) during render, and schema validation would fight that flow. values.schema.json is used in this repo only for leaf app charts (harbor, clickhouse, mariadb, redis) where the values are purely user-supplied. Adding a schema to the platform chart would be a much larger structural change than introducing one field, so it's out of scope for this PR.

spec:
blocks:
{{- range $exposeIPsList }}
- cidr: {{ . }}/{{ if contains ":" . }}128{{ else }}32{{ end }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low

The logic for determining the CIDR suffix assumes that the input is a single IP address. While this is consistent with how externalIPs are typically defined, if a user provides an IP that already includes a CIDR mask (e.g., 1.2.3.4/24), this template will produce an invalid string like 1.2.3.4/24/32. Consider adding a check to see if the string already contains a / before appending the default suffix.

- cidr: {{ . }}{{ if not (contains "/" .) }}/{{ if contains ":" . }}128{{ else }}32{{ end }}{{ end }}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — good catch. Fixed in 6af74ce: the /32 and /128 suffix append is now guarded on the absence of / in the input, so 192.0.2.10/32 or 2001:db8::1/128 in publishing.externalIPs will no longer produce a malformed 192.0.2.10/32/32 CIDR block. Added a helm-unittest case covering pre-CIDR input to packages/extra/ingress/tests/exposure_test.yaml.

…PPool

Service.spec.externalIPs is deprecated upstream in Kubernetes v1.36
(KEP-5707, kubernetes#137293). The AllowServiceExternalIPs feature gate
is expected to default to off around v1.40 and the implementation to
be removed around v1.43. For bare-metal installs that rely on
externalIPs today, cozystack needs a migration path.

This change adds an opt-in 'loadBalancer' exposure mode for the
ingress-nginx Service:

- New platform value 'publishing.exposure' (enum: externalIPs |
  loadBalancer, default externalIPs). Plumbed through cozystack-values
  into each tenant's ingress HelmRelease via the new 'expose-mode' key.
- Unknown values and loadBalancer with an empty externalIPs list fail
  the chart render with explicit error messages, rather than silently
  producing a broken Service.
- When exposure=loadBalancer and the current namespace matches
  publishing.ingressName, the Service becomes type: LoadBalancer with
  externalTrafficPolicy: Local.
- A new template renders a CiliumLoadBalancerIPPool whose blocks come
  from publishing.externalIPs (IPv4 addresses get /32, IPv6 addresses
  get /128) and whose serviceSelector uses Cilium's synthetic
  io.kubernetes.service.namespace key combined with the standard
  app.kubernetes.io/name: ingress-nginx label. No custom label is
  written to the Service itself, avoiding cross-tenant collisions from
  user-defined labels.

Default behaviour is unchanged: without opting in, the Service is
still ClusterIP + spec.externalIPs as today.

Scope: only ingress-nginx is migrated by this setting. Other cozystack
components that still write Service.spec.externalIPs directly (notably
the vpn app) must be migrated separately before the v1.40 feature gate
flip.

Tests: packages/extra/ingress/tests/exposure_test.yaml adds 13
helm-unittest cases covering both modes, IPv4/IPv6, empty-token
filtering, unknown-mode rejection, and the non-matching-namespace
fallback.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Address review feedback from gemini-code-assist on
packages/extra/ingress/templates/cilium-lb-pool.yaml:19: if the operator
passes an externalIP already in CIDR form (192.0.2.10/32 or 2001:db8::1/128),
the template appended a second /32 or /128 suffix producing an invalid
CiliumLoadBalancerIPPool block. Guard the suffix append on the absence of
"/" in the input.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Apr 24, 2026

@IvanHunters IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Quality: Excellent

The implementation is well-architected with comprehensive testing (13 helm-unittest cases), proper validation (unknown mode rejection, empty IPs check), and thorough documentation. The opt-in approach with backward compatibility is correct.

Critical Issues

1. Commit Attribution (Blocker)

Both commits contain Assisted-By: Claude <noreply@anthropic.com> trailer which should be removed before merge.

Commits affected:

  • adc7abe5 - feat(ingress): add loadBalancer exposure mode
  • 6af74cec - fix(ingress): accept pre-CIDR externalIPs

Fix: Rebase/amend to remove the Assisted-By: line.


2. Pool Conflict with Gateway API (Important - Design Issue)

This PR creates a CiliumLoadBalancerIPPool named root-ingress when publishing.exposure=loadBalancer. PR #2470 (Gateway API) creates a separate pool named root-gateway. Both pools use identical CIDR blocks from publishing.externalIPs.

Problem: Cilium LB IPAM does not allow overlapping CIDRs across pools. From Cilium docs:

IP Pools are not allowed to have overlapping CIDRs. When an administrator does create pools which overlap, a soft error is caused. The last added pool will be marked as Conflicting and no further allocation will happen from that pool.

Impact: If an operator enables both mechanisms simultaneously:

publishing:
  exposure: loadBalancer       # creates root-ingress pool
  externalIPs: [192.0.2.10]
gateway:
  enabled: true
tenant:
  spec:
    gateway: true               # creates root-gateway pool with SAME IP

The second-created pool will be marked CONFLICTING and its Service will hang in <pending>.

Current state: No validation prevents this scenario.

Proposed solutions:

  1. Add mutual exclusion check in packages/extra/ingress/templates/cilium-lb-pool.yaml:

    {{- $gatewayEnabled := (index .Values._cluster "gateway-enabled") | default false }}
    {{- if and (eq $exposeMode "loadBalancer") (eq $exposeIngress .Release.Namespace) $gatewayEnabled }}
    {{- fail "publishing.exposure=loadBalancer conflicts with gateway.enabled=true: both create overlapping CiliumLoadBalancerIPPools. Use gateway.enabled OR exposure=loadBalancer, not both. Migration path: disable exposure=loadBalancer before enabling gateway." }}
    {{- end }}
  2. Document the limitation in packages/core/platform/values.yaml under publishing.exposure:

    # IMPORTANT: If you plan to use Gateway API (gateway.enabled=true with
    # tenant.spec.gateway=true), do NOT use exposure=loadBalancer. Both create
    # CiliumLoadBalancerIPPools from publishing.externalIPs and Cilium marks
    # overlapping pools as conflicting. Migration: disable exposure=loadBalancer
    # before enabling Gateway API.
  3. Track in follow-up issue if deferring the fix.

Question for maintainer: Is simultaneous use intended to be supported (requires shared pool design), or is this a mutually-exclusive choice (requires validation)?


Minor Issues (Non-blockers)

3. Naming Inconsistency

File: packages/core/platform/templates/apps.yaml:33

expose-mode: {{ .Values.publishing.exposure | default "externalIPs" | quote }}

All other keys follow the pattern expose-<source-value-name>:

  • expose-ingresspublishing.ingressName
  • expose-external-ipspublishing.externalIPs
  • expose-modepublishing.exposure

Suggestion: Rename to expose-exposure for consistency (or rename source to publishing.exposeMode).

Impact: Low (cosmetic, but affects API consistency).


Summary

Verdict: LGTM after fixing critical issues #1 and #2.

Must fix before merge:

  • Remove Assisted-By: Claude from commits
  • Address pool conflict scenario (validation or documentation)

Nice to have:

  • Fix naming inconsistency

Excellent work on:

  • Comprehensive test coverage (13 cases, all edge cases)
  • CIDR suffix logic with pre-CIDR input support
  • Clever use of Cilium synthetic io.kubernetes.service.namespace label
  • Clear error messages and documentation

The pool is now rendered from packages/apps/tenant/templates/cilium-lb-pool.yaml instead of packages/extra/ingress/templates/cilium-lb-pool.yaml. Cilium LB IPAM forbids overlapping CIDRs across pools regardless of serviceSelector, so both the ingress-loadBalancer path and the upcoming per-tenant Gateway in #2470 cannot each own their own pool on the same publishing.externalIPs range. The tenant chart is the natural per-tenant owner — it already creates the Namespace, the cozystack-values Secret, and the HelmReleases for both ingress and gateway.

The new pool uses a namespace-only serviceSelector (io.kubernetes.service.namespace: <ns>), which matches any LoadBalancer Service in the tenant namespace. The metadata.name changed from <trim>-ingress to <trim>-exposure to reflect that the pool is not ingress-specific.

Only the ingress-loadBalancer signal is wired in this commit (_cluster.expose-mode=loadBalancer plus .Values.ingress=true on the publishing tenant). The gateway branch is added in #2470 on top of this commit — it rebases, drops its own packages/extra/gateway/templates/cilium-lb-pool.yaml, and adds an OR branch for .Values.gateway in the tenant template.

Pool-rendering unit tests moved from packages/extra/ingress/tests/ to packages/apps/tenant/tests/. The ingress chart tests keep the Service-level asserts. packages/apps/tenant/Makefile gains a test target so hack/helm-unit-tests.sh picks up the new suite.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
@lexfrei

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review.

(1) Assisted-By: Claude trailer

Keeping it. The project's own agent-contribution docs at docs/agents/contributing.md:36-46 explicitly describe this trailer as the required format for AI-assisted commits (alongside Signed-off-by:). Rewriting history to drop a trailer the docs tell me to add would contradict the project's own convention.

(2) Pool conflict with the Gateway API PR (#2470)

Good catch — this is a real issue. Cilium LB IPAM rejects overlapping CIDRs regardless of serviceSelector (verified against Cilium 1.19.3 docs and source; disjoint selectors do not help, the last-added pool gets cilium.io/PoolConflict and stops allocating).

Rather than a mutual-exclusion fail, I moved the pool to a single owner: packages/apps/tenant/templates/cilium-lb-pool.yaml. The tenant chart is the natural per-tenant owner — it already materialises the namespace, the cozystack-values Secret, and the HelmReleases for both ingress and gateway. One CiliumLoadBalancerIPPool per tenant with a namespace-only serviceSelector (io.kubernetes.service.namespace: <ns>) matches any LoadBalancer Service in that namespace: ingress-nginx today, a Cilium Gateway Service once #2470 lands. Pool name changed from <trim>-ingress to <trim>-exposure to reflect that it is not ingress-specific.

This PR wires only the ingress-loadBalancer signal (_cluster.expose-mode=loadBalancer + .Values.ingress=true + publishing tenant). #2470 rebases on top of this commit, drops its own packages/extra/gateway/templates/cilium-lb-pool.yaml, and adds an OR branch in the tenant template for .Values.gateway. That keeps #2468 self-contained and reduces #2470 to a pure delete on the pool side.

See 500816b for the refactor and the tenant-chart unit tests.

(3) expose-mode naming

Not changing this. expose-mode is part of the _cluster cross-chart contract already consumed downstream (cert-manager-issuers, cozystack-api, dashboard, kubevirt, linstor-gui, keycloak, kubevirt-cdi). Renaming would break every reader for a purely cosmetic gain, and mode reads more naturally than exposure in context. Happy to revisit as a broader pass across all expose-* keys if there is appetite.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (7)
packages/extra/ingress/Makefile (1)

14-15: Declare test as .PHONY.

Static analysis (checkmake) flags this target as non-phony. Since helm unittest . is not producing a file named test, declaring it PHONY avoids accidental shadowing and aligns with the other package Makefiles.

♻️ Proposed change
+.PHONY: test
 test:
 	helm unittest .
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/extra/ingress/Makefile` around lines 14 - 15, The Makefile's test
target is not declared phony which checkmake flags; update the Makefile by
adding a .PHONY declaration for the test target (e.g., add ".PHONY: test" near
the top or alongside other .PHONY entries) so that the "test" target (which runs
"helm unittest .") cannot be shadowed by a file named "test" and matches the
other package Makefiles' conventions.
packages/apps/tenant/Makefile (1)

7-8: Declare test as .PHONY (same as ingress Makefile).

♻️ Proposed change
+.PHONY: test
 test:
 	helm unittest .
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/apps/tenant/Makefile` around lines 7 - 8, The Makefile declares a
test target named "test" but doesn't mark it as phony; update the Makefile to
declare .PHONY: test so the test target (helm unittest .) is always run rather
than being treated as a file, mirroring the ingress Makefile pattern and
preventing caching/incorrect behavior for the test target.
packages/apps/tenant/templates/cilium-lb-pool.yaml (1)

1-16: Minor: validation asymmetry with the ingress chart for empty expose-external-ips.

When expose-mode=loadBalancer in the publishing namespace with empty IPs, packages/extra/ingress/templates/nginx-ingress.yaml calls fail (Lines 14–16 there), but this template silently emits zero documents because $ipsList is empty. In practice the ingress chart errors first so the user sees a clear failure, but if the tenant chart is ever rendered in isolation (e.g. for debugging) the missing pool is silent. Consider mirroring the same fail here for symmetry, or leave a comment explaining the delegation.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/apps/tenant/templates/cilium-lb-pool.yaml` around lines 1 - 16, The
template silently emits nothing when expose-mode is "loadBalancer" in the
publishing namespace and expose-external-ips is empty (the logic around
$exposeMode, $exposeIngress, $exposeIPs, $ipsList and $isPublishingIngressLB),
which is asymmetric with the ingress chart that calls fail for empty IPs; update
this template to mirror that behavior by adding a Helm fail() when
$isPublishingIngressLB is true but $ipsList is empty (or else add a clear
comment explaining that validation is delegated to the ingress chart), ensuring
the check references the same symbols ($exposeMode, $exposeIngress,
$expose-external-ips/$ipsList and $isPublishingIngressLB) so rendering this
chart alone fails with a clear error instead of producing zero documents.
packages/apps/tenant/tests/exposure_test.yaml (1)

77-89: Optional: add a mixed case combining a pre-CIDR and a bare IP in the same list.

The current tests verify "all pre-CIDR" (lines 77–89) and "all bare IPs with mixed families" (lines 63–75) separately. A combined input like "192.0.2.10/32,192.0.2.11" would extra-guard the contains "/" check from regressions (e.g., if someone refactors it to a global contains "/" $exposeIPs).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/apps/tenant/tests/exposure_test.yaml` around lines 77 - 89, Add a
new test case similar to "loadBalancer mode accepts pre-CIDR input without
double-suffixing" that provides a mixed expose-external-ips input combining a
pre-CIDR and a bare IP (e.g., "192.0.2.10/32,192.0.2.11") to ensure the parsing
logic in the expose-mode/loadBalancer path correctly leaves pre-CIDR values
unchanged and adds CIDR for bare IPs; update the test's
set._cluster.expose-external-ips and asserts.spec.blocks to expect both
192.0.2.10/32 and 192.0.2.11/32 (or the appropriate CIDR for the bare IP) so the
contains "/" check cannot be circumvented by refactors.
packages/extra/ingress/templates/nginx-ingress.yaml (1)

2-10: Minor: | nospace + per-entry trim is redundant.

nospace already strips all whitespace from $exposeExternalIPs, so $ip := . | trim inside the loop never has anything to trim. Either drop the global nospace (and keep per-item trim, which is slightly more permissive with weird inputs like embedded newlines) or drop the trim — currently both coexist. Not a correctness issue.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/extra/ingress/templates/nginx-ingress.yaml` around lines 2 - 10, The
template currently applies | nospace to $exposeExternalIPs and also calls trim
on each split entry ($ip := . | trim), which is redundant; remove the global |
nospace from the $exposeExternalIPs assignment so the per-item trim inside the
range remains and handles whitespace/newlines more permissively—update the
declaration that sets $exposeExternalIPs (the line referencing .Values._cluster
"expose-external-ips") to drop | nospace while leaving the splitList and the
per-item trim intact.
packages/extra/ingress/tests/exposure_test.yaml (2)

34-43: Strengthen the legacy-fallback assertion.

This test claims to verify that omitting expose-mode "falls back to externalIPs behavior", but only asserts service.type: ClusterIP. The externalIPs list and externalTrafficPolicy: Cluster are the defining signals of that behavior — without asserting them, a regression that e.g. forgets to populate externalIPs when the mode defaults would still pass this case (the first test happens to already cover it, but the intent of this one is narrower).

🧪 Suggested tightening
     asserts:
       - template: templates/nginx-ingress.yaml
         equal:
           path: spec.values.ingress-nginx.controller.service.type
           value: ClusterIP
+      - template: templates/nginx-ingress.yaml
+        equal:
+          path: spec.values.ingress-nginx.controller.service.externalTrafficPolicy
+          value: Cluster
+      - template: templates/nginx-ingress.yaml
+        equal:
+          path: spec.values.ingress-nginx.controller.service.externalIPs
+          value:
+            - 192.0.2.10
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/extra/ingress/tests/exposure_test.yaml` around lines 34 - 43, Update
the test case under templates/nginx-ingress.yaml that currently asserts only
path spec.values.ingress-nginx.controller.service.type to also assert that
spec.values.ingress-nginx.controller.service.externalIPs contains the configured
IP ("192.0.2.10") and that
spec.values.ingress-nginx.controller.service.externalTrafficPolicy equals
"Cluster"; locate the test block named "legacy config without expose-mode falls
back to externalIPs behavior" and add assertions for externalIPs and
externalTrafficPolicy so the legacy-fallback behavior is fully validated.

98-118: Consider consolidating to a single case-sensitivity test case.

The two unknown-mode tests (LoadBalancer vs loadbalancer) exercise the same validation branch and assert the same template contract—they differ only in the interpolated value. One case is sufficient to guarantee case-sensitivity.

The PR's claim about IP families coverage is backed: the tenant chart test suite at packages/apps/tenant/tests/exposure_test.yaml includes explicit coverage for IPv6 (emitting /128 CIDR) and mixed IPv4/IPv6 inputs (validating /32 and /128 per family).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/extra/ingress/tests/exposure_test.yaml` around lines 98 - 118, Two
identical validation tests for case-sensitive exposure modes are redundant: keep
a single case-sensitivity test and remove the duplicate. Specifically, retain
the test titled "unknown exposure mode is rejected with a clear error
(case-sensitive enum)" (with its expose-mode value and expected errorMessage)
and remove the test titled "another typo in exposure mode also fails"; if you
prefer to cover multiple failing inputs in one spec, convert the kept spec into
a single parameterized/looped case that asserts the same template failure for
both "LoadBalancer" and "loadbalancer", referencing the failing template
assertion on templates/nginx-ingress.yaml and the errorMessage comparing
publishing.exposure mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/apps/tenant/Makefile`:
- Around line 7-8: The Makefile declares a test target named "test" but doesn't
mark it as phony; update the Makefile to declare .PHONY: test so the test target
(helm unittest .) is always run rather than being treated as a file, mirroring
the ingress Makefile pattern and preventing caching/incorrect behavior for the
test target.

In `@packages/apps/tenant/templates/cilium-lb-pool.yaml`:
- Around line 1-16: The template silently emits nothing when expose-mode is
"loadBalancer" in the publishing namespace and expose-external-ips is empty (the
logic around $exposeMode, $exposeIngress, $exposeIPs, $ipsList and
$isPublishingIngressLB), which is asymmetric with the ingress chart that calls
fail for empty IPs; update this template to mirror that behavior by adding a
Helm fail() when $isPublishingIngressLB is true but $ipsList is empty (or else
add a clear comment explaining that validation is delegated to the ingress
chart), ensuring the check references the same symbols ($exposeMode,
$exposeIngress, $expose-external-ips/$ipsList and $isPublishingIngressLB) so
rendering this chart alone fails with a clear error instead of producing zero
documents.

In `@packages/apps/tenant/tests/exposure_test.yaml`:
- Around line 77-89: Add a new test case similar to "loadBalancer mode accepts
pre-CIDR input without double-suffixing" that provides a mixed
expose-external-ips input combining a pre-CIDR and a bare IP (e.g.,
"192.0.2.10/32,192.0.2.11") to ensure the parsing logic in the
expose-mode/loadBalancer path correctly leaves pre-CIDR values unchanged and
adds CIDR for bare IPs; update the test's set._cluster.expose-external-ips and
asserts.spec.blocks to expect both 192.0.2.10/32 and 192.0.2.11/32 (or the
appropriate CIDR for the bare IP) so the contains "/" check cannot be
circumvented by refactors.

In `@packages/extra/ingress/Makefile`:
- Around line 14-15: The Makefile's test target is not declared phony which
checkmake flags; update the Makefile by adding a .PHONY declaration for the test
target (e.g., add ".PHONY: test" near the top or alongside other .PHONY entries)
so that the "test" target (which runs "helm unittest .") cannot be shadowed by a
file named "test" and matches the other package Makefiles' conventions.

In `@packages/extra/ingress/templates/nginx-ingress.yaml`:
- Around line 2-10: The template currently applies | nospace to
$exposeExternalIPs and also calls trim on each split entry ($ip := . | trim),
which is redundant; remove the global | nospace from the $exposeExternalIPs
assignment so the per-item trim inside the range remains and handles
whitespace/newlines more permissively—update the declaration that sets
$exposeExternalIPs (the line referencing .Values._cluster "expose-external-ips")
to drop | nospace while leaving the splitList and the per-item trim intact.

In `@packages/extra/ingress/tests/exposure_test.yaml`:
- Around line 34-43: Update the test case under templates/nginx-ingress.yaml
that currently asserts only path
spec.values.ingress-nginx.controller.service.type to also assert that
spec.values.ingress-nginx.controller.service.externalIPs contains the configured
IP ("192.0.2.10") and that
spec.values.ingress-nginx.controller.service.externalTrafficPolicy equals
"Cluster"; locate the test block named "legacy config without expose-mode falls
back to externalIPs behavior" and add assertions for externalIPs and
externalTrafficPolicy so the legacy-fallback behavior is fully validated.
- Around line 98-118: Two identical validation tests for case-sensitive exposure
modes are redundant: keep a single case-sensitivity test and remove the
duplicate. Specifically, retain the test titled "unknown exposure mode is
rejected with a clear error (case-sensitive enum)" (with its expose-mode value
and expected errorMessage) and remove the test titled "another typo in exposure
mode also fails"; if you prefer to cover multiple failing inputs in one spec,
convert the kept spec into a single parameterized/looped case that asserts the
same template failure for both "LoadBalancer" and "loadbalancer", referencing
the failing template assertion on templates/nginx-ingress.yaml and the
errorMessage comparing publishing.exposure mode.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: acacf36d-4e45-450d-8113-24474f84a6bf

📥 Commits

Reviewing files that changed from the base of the PR and between ad7d25f and 500816b.

📒 Files selected for processing (9)
  • packages/apps/tenant/Makefile
  • packages/apps/tenant/templates/cilium-lb-pool.yaml
  • packages/apps/tenant/tests/exposure_test.yaml
  • packages/core/platform/templates/apps.yaml
  • packages/core/platform/values.yaml
  • packages/extra/ingress/Makefile
  • packages/extra/ingress/README.md
  • packages/extra/ingress/templates/nginx-ingress.yaml
  • packages/extra/ingress/tests/exposure_test.yaml

Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Apr 24, 2026
#2468 introduced packages/apps/tenant/templates/cilium-lb-pool.yaml as the per-tenant owner of the LoadBalancer IP pool. Extend that template with the .Values.gateway branch: the pool now renders when either _cluster.expose-mode=loadBalancer (with .Values.ingress=true on the publishing tenant) OR .Values.gateway=true. Cilium LB IPAM forbids overlapping CIDRs across pools regardless of serviceSelector, so a single pool per tenant is required — the same CIDRs cannot back both ingress and gateway from two separate pools without one of them being marked Conflicting.

Delete packages/extra/gateway/templates/cilium-lb-pool.yaml — the gateway chart no longer renders its own pool. The empty-externalIPs render-time fail moves to the tenant template, guarded on the gateway branch so the legacy ingress path remains unchanged.

Gateway tests lose the pool-rendering asserts that previously lived in packages/extra/gateway/tests/gateway_test.yaml. The tenant chart tests in packages/apps/tenant/tests/exposure_test.yaml gain five gateway-path cases covering pool rendering, non-publishing-tenant rendering, the simultaneous ingress+gateway single-pool case, gateway-only with ingress=false, and the empty-externalIPs fail.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Empty commit to retrigger Build job that failed on Docker Hub anonymous
pull rate limit (429 Too Many Requests) for golang:1.26.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Empty commit to retrigger Build that lost the OCI registry tag race
on the previous attempt.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
@lexfrei
Aleksei Sviridkin (lexfrei) merged commit 3364290 into main Apr 28, 2026
7 checks passed
@lexfrei
Aleksei Sviridkin (lexfrei) deleted the feat/ingress-lb-ipam branch April 28, 2026 12:21
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request May 1, 2026
#2468 introduced packages/apps/tenant/templates/cilium-lb-pool.yaml as the per-tenant owner of the LoadBalancer IP pool. Extend that template with the .Values.gateway branch: the pool now renders when either _cluster.expose-mode=loadBalancer (with .Values.ingress=true on the publishing tenant) OR .Values.gateway=true. Cilium LB IPAM forbids overlapping CIDRs across pools regardless of serviceSelector, so a single pool per tenant is required — the same CIDRs cannot back both ingress and gateway from two separate pools without one of them being marked Conflicting.

Delete packages/extra/gateway/templates/cilium-lb-pool.yaml — the gateway chart no longer renders its own pool. The empty-externalIPs render-time fail moves to the tenant template, guarded on the gateway branch so the legacy ingress path remains unchanged.

Gateway tests lose the pool-rendering asserts that previously lived in packages/extra/gateway/tests/gateway_test.yaml. The tenant chart tests in packages/apps/tenant/tests/exposure_test.yaml gain five gateway-path cases covering pool rendering, non-publishing-tenant rendering, the simultaneous ingress+gateway single-pool case, gateway-only with ingress=false, and the empty-externalIPs fail.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request May 2, 2026
#2468 introduced packages/apps/tenant/templates/cilium-lb-pool.yaml as the per-tenant owner of the LoadBalancer IP pool. Extend that template with the .Values.gateway branch: the pool now renders when either _cluster.expose-mode=loadBalancer (with .Values.ingress=true on the publishing tenant) OR .Values.gateway=true. Cilium LB IPAM forbids overlapping CIDRs across pools regardless of serviceSelector, so a single pool per tenant is required — the same CIDRs cannot back both ingress and gateway from two separate pools without one of them being marked Conflicting.

Delete packages/extra/gateway/templates/cilium-lb-pool.yaml — the gateway chart no longer renders its own pool. The empty-externalIPs render-time fail moves to the tenant template, guarded on the gateway branch so the legacy ingress path remains unchanged.

Gateway tests lose the pool-rendering asserts that previously lived in packages/extra/gateway/tests/gateway_test.yaml. The tenant chart tests in packages/apps/tenant/tests/exposure_test.yaml gain five gateway-path cases covering pool rendering, non-publishing-tenant rendering, the simultaneous ingress+gateway single-pool case, gateway-only with ingress=false, and the empty-externalIPs fail.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request May 6, 2026
… LB-IPAM

Switch the per-tenant Gateway IP allocator from Cilium LB-IPAM to MetalLB.
The chart renders a per-tenant metallb.io/v1beta1 IPAddressPool in
cozy-metallb (label cozystack.io/per-tenant-gateway=true, /32 or /128
by family, serviceAllocation.namespaces scoped to the tenant). The
TenantGateway controller writes
metallb.universe.tf/address-pool=<tenant-namespace>-gateway on
Gateway.spec.infrastructure.annotations; Cilium's Gateway-API
controller forwards the annotation to the auto-created Service via
GEP-1762, and MetalLB allocates the address held by the pool.

L2/BGP advertisement stays admin-side: the chart renders no
Advertisement resource. A cluster-wide L2/BGPAdvertisement (no
ipAddressPools selector) covers per-tenant pools automatically;
admins targeting specifically can match on
cozystack.io/per-tenant-gateway=true.

Drops the Cilium-pool branch added by the earlier per-tenant
gatewayIP commit. The tenant-side cilium-lb-pool.yaml is restored
to main shape (ingress-only flow). Refactoring the ingress side onto
MetalLB is a separate follow-up against #2468.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request May 7, 2026
… LB-IPAM

Switch the per-tenant Gateway IP allocator from Cilium LB-IPAM to MetalLB.
The chart renders a per-tenant metallb.io/v1beta1 IPAddressPool in
cozy-metallb (label cozystack.io/per-tenant-gateway=true, /32 or /128
by family, serviceAllocation.namespaces scoped to the tenant). The
TenantGateway controller writes
metallb.universe.tf/address-pool=<tenant-namespace>-gateway on
Gateway.spec.infrastructure.annotations; Cilium's Gateway-API
controller forwards the annotation to the auto-created Service via
GEP-1762, and MetalLB allocates the address held by the pool.

L2/BGP advertisement stays admin-side: the chart renders no
Advertisement resource. A cluster-wide L2/BGPAdvertisement (no
ipAddressPools selector) covers per-tenant pools automatically;
admins targeting specifically can match on
cozystack.io/per-tenant-gateway=true.

Drops the Cilium-pool branch added by the earlier per-tenant
gatewayIP commit. The tenant-side cilium-lb-pool.yaml is restored
to main shape (ingress-only flow). Refactoring the ingress side onto
MetalLB is a separate follow-up against #2468.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Andrei Kvapil (kvaps) added a commit that referenced this pull request May 11, 2026
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>
Andrei Kvapil (kvaps) added a commit that referenced this pull request May 19, 2026
…#2603)

## What this PR does

Reverts the `publishing.exposure=loadBalancer` mode for ingress-nginx
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, see [docs
section "4b. Node Public IP
Setup"](https://cozystack.io/docs/v1.3/install/cozystack/platform/#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.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`. The Service now renders 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.

### Compatibility

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.

### Release note

```release-note
revert(ingress): remove the publishing.exposure=loadBalancer mode added in #2468. The default externalIPs path is unchanged; operators who explicitly opted into loadBalancer will have their ingress Service revert to ClusterIP + spec.externalIPs on upgrade. Migration off Service.spec.externalIPs (deprecated in KEP-5707) will be reintroduced via a mechanism-agnostic API knob in a follow-up.
```

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Release Notes

* **Refactor**
* Removed conditional generation of the Cilium load‑balancer IP pool and
simplified service exposure to rely on explicit external IP values.
  * Streamlined configuration for service exposure keys.

* **Documentation**
  * Removed the exposure-mode guidance from ingress documentation.

* **Tests**
* Deleted the exposure-mode-related test suites covering various
external IP and loadBalancer scenarios.

[![Review Change
Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2603)
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request May 26, 2026
…2470)

## What this PR does

Replaces ingress-nginx as the default ingress layer for cozystack-native
services with **Cilium Gateway API**, materialised per-tenant via a new
`TenantGateway` CRD reconciled by `cozystack-controller`. Supersedes
#2213 / #2208. Stacked on top of #2468.

## Architecture

The chart renders one `gateway.cozystack.io/v1alpha1 TenantGateway` CR
per tenant that owns its own Gateway. The cozystack-controller
reconciles the actual `Gateway`, per-tenant `Issuer`, `Certificate`
(wildcard or per-listener), and HTTP→HTTPS redirect `HTTPRoute` from
there. Helm does not render `Gateway` or `Certificate` directly — that
prevents the Helm-vs-controller race on `Gateway.spec.listeners` that
route-driven listener additions would otherwise cause.

```
┌── TenantGateway CR (rendered by extra/gateway chart for owning tenants)
│
└──▶ cozystack-controller reconciles:
        ├─ Gateway (with dynamic listeners, label-based allowedRoutes)
        ├─ Issuer (per-tenant ACME account)
        ├─ Certificate(s) — wildcard + per-child SANs in DNS-01, per-listener in HTTP-01
        ├─ HTTPRoute (controller-owned http→https redirect)
        ├─ Namespace label patching (cozy-* system namespaces from spec.AttachedNamespaces)
        └─ Watches HTTPRoute / TLSRoute attachments to materialise listeners
```

## Inheritance: tenants attach by default, opt into their own Gateway

A tenant gets a dedicated Gateway (own LB Service, own LB IP, own
Certificate) only when it explicitly asks via
`tenant.spec.gateway=true`. Every other tenant in the tree publishes
through the Gateway of the nearest ancestor that owns one — same shape
as the existing `_namespace.ingress` inheritance.

The `apps/tenant` chart writes a `namespace.cozystack.io/gateway` label
on each tenant namespace, carrying either the tenant's own name (when
owning a Gateway) or the inherited ancestor name (when inheriting). The
Gateway's listener `allowedRoutes.namespaces.selector` is keyed on that
label, so the same selector admits routes from every namespace pointing
at the owner — descendants and cozy-* system namespaces alike.
cozystack-controller patches the same label onto every namespace in
`tgw.Spec.AttachedNamespaces` and garbage-collects labels it wrote when
an entry is removed from the attach list (annotation-gated GC so labels
written by the apps/tenant chart are never stripped).

In DNS-01 mode, the controller extends the parent's wildcard Certificate
with `<child-apex>` + `*.<child-apex>` SANs per inheriting child, and
adds a `*.<child-apex>` HTTPS listener per child apex referencing the
same cert. Without this expansion the parent's single-level wildcard
cannot match a child route's hostname (`harbor.alice.example.org` is two
labels past the parent's `*.example.org`).

Opt-out into own Gateway is the right choice when: a tenant needs its
own LB IP (DNS pinned, firewall rule), the apex is not derived from the
parent (custom `host` value the ancestor's cert can't cover), or the
tenant wants its own ACME account / cert authority. Otherwise leave
`gateway` unset and inherit.

## End-to-end flow

1. Platform: `gateway.enabled=true` toggles cert-manager solver,
switches per-system-app templates from Ingress to HTTPRoute / TLSRoute.
2. Per tenant: explicit `tenant.spec.gateway=true` renders a
`TenantGateway` CR and triggers the controller to materialise the owner
Gateway. Tenants with the field unset inherit through
`_namespace.gateway` propagation in the tenant chart.
3. Adding a published app under any tenant (owner or inheriting) is just
deploying its HTTPRoute. The controller picks up the hostname, adds a
per-listener Certificate (HTTP-01) or extends the wildcard cert's SANs
(DNS-01), and updates the owner Gateway's listeners.

Defaults stay legacy: `gateway.enabled=false`,
`tenant.spec.gateway=false`. Existing clusters upgrade unchanged.

## External IP allocation — mechanism-agnostic

The per-tenant Gateway's auto-created `LoadBalancer` Service draws its
IP from whatever LB allocator the cluster admin has configured at the
platform layer — same shape as ingress-nginx today. The tenant API stays
mechanism-agnostic — no `gatewayIP` field, no allocator-specific
manifest in the tenant chart. Cozystack itself ships MetalLB installed
but does not render any `IPAddressPool` / `L2Advertisement` /
`BGPAdvertisement` from this chart; admins set up the allocator that
suits their environment (MetalLB pool with L2 / BGP, Cilium LB-IPAM with
announcer, robotlb against a cloud provider, or
`Service.spec.externalIPs` pinning).

If a tenant needs a specific address (DNS already pinned, firewall rule,
etc.), the operator pre-allocates it on the admin side: either
pre-create the Service with `loadBalancerIP` set, or hand the tenant a
reference to a named admin-managed pool. Per-Service IP uniqueness is
the allocator's responsibility — same as for any other LoadBalancer
Service.

## Cert mode: HTTP-01 (default) vs DNS-01 (opt-in)

`publishing.certificates.solver` selects how the controller sources TLS
certs.

**HTTP-01 (default)** — per-app HTTPS listener + per-app Certificate.
New apps require zero platform-side config: deploy the HTTPRoute,
controller does the rest. Inheriting children's hostnames pick up
per-listener certs the same way the owner's own apps do.

**DNS-01 (opt-in)** — single wildcard Certificate (`<apex>` +
`*.<apex>`) covering every published app under the apex, plus per-child
SANs (`<child-apex>` + `*.<child-apex>`) for every inheriting tenant.
Pick this for clusters where many apps share the apex and Let's Encrypt
rate limits matter. The DNS provider account must be able to write TXT
records under every apex level the parent serves — for deeply-nested
inherited children that requires zone delegation or a wide-scope
provider credential.

| `publishing.certificates.dns01.provider` | Required
`publishing.certificates.dns01.<provider>` keys |
| --- | --- |
| `cloudflare` (default) | `cloudflare.secretName`,
`cloudflare.secretKey` |
| `route53` | `route53.region`, `route53.secretName` (and `accessKeyID`
if not on IRSA) |
| `digitalocean` | `digitalocean.secretName` |
| `rfc2136` | `rfc2136.nameserver`, `rfc2136.tsigKeyName`,
`rfc2136.secretName` |

The platform chart wires every provider's keys into `_cluster.dns01-*`;
the per-tenant gateway chart and the cluster-wide `cluster-issuers.yaml`
both read them.

## Strengthens admission coverage on apps.cozystack.io/*

The `cozystack-tenant-host-policy` probe in `gateway.bats` initially
looked like a flake — the apply that was supposed to be VAP-rejected
just succeeded. Investigation traced it to the custom REST handler at
`pkg/registry/apps/application/rest.go`. genericapiserver hands every
storage method a callback into the validating admission chain:
`createValidation` for Create, `updateValidation` for Update,
`deleteValidation` for Delete. `Update` was correctly invoking its
callback since 23e399b, but `Create` and `Delete` accepted the
parameter and never called it — admission was silently bypassed for both
verbs on every `apps.cozystack.io` resource (Tenant, MariaDB, Postgres,
Kubernetes, …).

This PR wires the missing `createValidation` and `deleteValidation`
calls into Create and Delete. Every ValidatingAdmissionPolicy and
ValidatingWebhook against `apps.cozystack.io/*` now fires on all three
verbs as the API contract requires. Per KEP-3488 aggregated APIServers
are responsible for enforcing admission on their own kinds;
cozystack-api is configured to do so via
`genericoptions.NewRecommendedOptions`, the only thing missing was the
explicit `*Validation` callbacks in the custom REST.

## Security — five guards, grouped by what they defend

Tenants in Cozystack interact with the platform exclusively through
`apps.cozystack.io/*` resources (Tenant, Bucket, Kubernetes, …) served
by `cozystack-api`. Tenant RBAC (`cozy:tenant:*` aggregated to a
RoleBinding in the tenant's own namespace) does not grant write access
to `gateway.networking.k8s.io/*`, core `Namespaces`, or
`cozystack.io/Package`. The protections below split into three groups by
who they defend against:

**Tenant-user-input gates.** Layer 4 (`cozystack-tenant-host-policy`).
`Tenant.spec.host` is the user-supplied field that surfaces as a
security boundary at the hostname layer; gated on every Create / Update
via `cozystack-api`'s admission chain.

**Defense-in-depth.** Layers 1, 2, 5, 7. These do not protect against
tenant-user input (tenants don't hold the relevant RBAC). They guard
against bugs in cozystack-controller / Flux, supply-chain compromise of
an app chart that emits Gateway API resources, and confused-deputy
mistakes by a cluster admin. Fail-closed via `failurePolicy: Fail` +
`validationActions: [Deny]`.

The five layers themselves:

1. **Listener `allowedRoutes` label selector** —
`namespace.cozystack.io/gateway = <owner-tenant-name>`
(controller-written for cozy-* via `AttachedNamespaces`, Helm-written
for tenant namespaces via the tenant chart, both feeding into the same
Gateway selector).
2. **`cozystack-gateway-hostname-policy`** — VAP on `Gateway`
CREATE/UPDATE. Restricts listener hostnames to the namespace's
`namespace.cozystack.io/host` apex.
4. **`cozystack-tenant-host-policy`** — VAP on `Tenant` CREATE/UPDATE.
5. **`cozystack-namespace-host-label-policy`** — VAP on core `Namespace`
CREATE/UPDATE. Immutability gate on the host label.
7. **`cozystack-route-hostname-policy`** — VAP on `HTTPRoute` and
`TLSRoute` CREATE/UPDATE. Closes the hostname-hijack vector across
namespaces under inheritance — a child cannot claim a hostname outside
its own apex.

The previous Layer 3 (`cozystack-gateway-attached-namespaces-policy`)
and Layer 6 (render-time `fail` in cozystack-basics) banned `tenant-*`
entries in `publishing.gateway.attachedNamespaces` on the assumption
that the attach list was the hijack vector. Under inheritance the attach
surface is the label selector, not the static list; the hijack vector is
closed by Layers 4/5/7 independently. Both Layer 3 and Layer 6 are
dropped — kept in numbering above for continuity with the earlier review
threads.

## Foreign-takeover guards

Five reconcile paths refuse to silently rewrite a pre-existing object
that shares the controller-derived name but carries no `OwnerReference`
back to the TenantGateway: `Gateway`, redirect `HTTPRoute`, per-tenant
`Issuer`, wildcard `Certificate`, per-listener `Certificate`. An
operator who hand-pinned a Certificate or Issuer at the controller's
derived name (private CA, manual cert pinning) gets an explicit
`Ready=False/ReconcileError` condition instead of having their config
silently destroyed and the resource re-issued from a different ACME
account.

The namespace-label patching path applies the same discipline:
cozystack-controller only writes / strips
`namespace.cozystack.io/gateway` on namespaces it annotates with
`cozystack.io/gateway-attached-by`. Labels written by the apps/tenant
chart (no annotation) are never touched, so inheritance for tenant
namespaces survives every reconcile.

## Tests

- **Go controller tests** — 50+ cases in
`internal/controller/tenantgateway/`: idempotency, OwnerReference
cascade, multi-parentRef status, HTTP-01↔DNS-01 mode-transition cert
lifecycle, foreign-takeover refusals (Gateway / HTTPRoute / Issuer /
Certificate / per-listener Certificate), DNS-01 provider matrix
(cloudflare / route53 / digitalocean / rfc2136), HTTPS Kinds=[HTTPRoute]
restriction, fail-closed CEL on missing host label, status condition
wiring. Plus, for inheritance: label-based allowedRoutes selector shape,
namespace label patch + garbage-collect via owner annotation, Helm-owned
label safety, DNS-01 wildcard SAN expansion per inheriting child,
per-child wildcard listener materialisation.
- **Admission-chain tests** —
`pkg/registry/apps/application/rest_admission_test.go` pins
`createValidation`, `updateValidation`, `deleteValidation` paths via
sentinel callbacks.
- **helm-unittest** across `extra/gateway` (TenantGateway CR rendering,
both modes), `core/platform` (DNS-01 wiring), `apps/tenant`
(gateway-effective decision matrix including inheritance),
`system/cozystack-basics` (three VAPs + Layer 7 VAP + tenant-root
namespace label including the conditional
`namespace.cozystack.io/gateway: tenant-root` + cozystack-values-secret
`_namespace.gateway` propagation), `system/cert-manager-issuers` (full
DNS-01 provider matrix), `system/cozystack-api` and `system/dashboard`
(Ingress↔TLSRoute/HTTPRoute toggle), `system/monitoring` (Grafana /
Alerta Ingress↔HTTPRoute toggle).
- **e2e bats** — `hack/e2e-apps/gateway.bats` (17 cases) covering
GatewayClass Accepted, Gateway Programmed + LB Service, HTTPRoute
Accepted with status, VAP rejects cross-tenant hostname hijack, VAP
rejects `tenant.spec.host` writes from non-trusted SA
(regression-detector for the admission-chain fix), VAP rejects
`namespace.cozystack.io/host` label changes from non-trusted SA,
route-hostname VAP rejects HTTPRoute hostnames outside the apex, and
three new cases for inheritance — Package admission accepts `tenant-*`
in `attachedNamespaces` (regression guard for the dropped ban), child
tenant without explicit gateway inherits `_namespace.gateway` from a
Gateway-owning parent in lockstep across the namespace label and the
cozystack-values Secret, and the cross-namespace attach end-to-end where
a child tenant's HTTPRoute reaches Accepted on the parent's Gateway via
the inheritance label.
- **CI wiring** — `make test-controllers` target invoked from
`pull-requests.yaml` runs `go test ./internal/...`.

## Backward compatibility

The only always-on change is the flip of `cilium.envoy.enabled` +
`cilium.gatewayAPI.enabled` to `true` (extra `cilium-envoy` DaemonSet,
~100 MB RAM/node idle). Everything else is gated behind
`gateway.enabled=false` / `tenant.spec.gateway=false`. Existing clusters
see no behavioural change until an operator explicitly opts in.

The cozystack-api admission-chain fix is unconditional. Existing
operators who do not define any VAP / admission webhook on
`apps.cozystack.io` resources see no observable change. Operators who DO
have their own webhooks/VAPs against these kinds will start seeing them
fire on Create and Delete (as intended) — review your policies before
merging if you maintain custom admission for `apps.cozystack.io/*`.

Operators who set `publishing.gateway.attachedNamespaces` to include
`tenant-*` entries used to hit a render-time fail or an admission
rejection — now the apply succeeds and the listed tenant namespaces
simply pick up the gateway-attach label alongside the cozy-* system
namespaces. This matches the inheritance model and was kvaps' explicit
ask in the May 25 review.

## Release note

```release-note
- Adds Gateway API support via Cilium, materialised per-tenant via the new `gateway.cozystack.io/v1alpha1 TenantGateway` CRD reconciled by `cozystack-controller`. Opt-in via `publishing.gateway.enabled=true` at the platform level; per-tenant opt-in via `tenant.spec.gateway=true` for dedicated Gateway / LB IP, otherwise the tenant inherits the nearest ancestor's Gateway via a label-based `allowedRoutes` selector (same shape as the existing `_namespace.ingress` inheritance). Includes HTTP-01 (default) and DNS-01 (opt-in, with cloudflare / route53 / digitalocean / rfc2136 providers) cert solvers; DNS-01 extends the parent's wildcard Certificate with per-child apex SANs and adds a `*.<child-apex>` listener per inheriting tenant. Tenant API is mechanism-agnostic for IP allocation — the LoadBalancer Service draws its IP from the cluster's admin-configured allocator (MetalLB / Cilium LB-IPAM / robotlb / externalIPs).
- Strengthens admission coverage on `apps.cozystack.io/*` resources: `cozystack-api` now invokes `createValidation` and `deleteValidation` callbacks on Create and Delete (Update was already wired). Operators with custom ValidatingAdmissionPolicies or webhooks targeting `apps.cozystack.io/*` will see them fire on all three verbs as the API contract requires; review your policies before merging if you maintain such admission.
```
Aleksei Sviridkin (lexfrei) added a commit that referenced this pull request Jun 30, 2026
…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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature Categorizes issue or PR as related to a new feature lgtm This PR has been approved by a maintainer size/S This PR changes 10-29 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants