feat(ingress): add loadBalancer exposure mode via CiliumLoadBalancerIPPool - #2468
Conversation
📝 WalkthroughWalkthroughThis PR introduces configurable ingress exposure modes via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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 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 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 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 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 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
|
8b5f270 to
829349e
Compare
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
There was a problem hiding this comment.
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 }} |
There was a problem hiding this comment.
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 }}There was a problem hiding this comment.
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.
c0a5f2f to
a78505e
Compare
829349e to
2601dee
Compare
f9503e9 to
3f36a1b
Compare
2601dee to
608b80a
Compare
…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>
608b80a to
adc7abe
Compare
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>
IvanHunters
left a comment
There was a problem hiding this comment.
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 mode6af74cec- 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 IPThe second-created pool will be marked CONFLICTING and its Service will hang in <pending>.
Current state: No validation prevents this scenario.
Proposed solutions:
-
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 }}
-
Document the limitation in
packages/core/platform/values.yamlunderpublishing.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.
-
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-ingress←publishing.ingressNameexpose-external-ips←publishing.externalIPsexpose-mode←publishing.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: Claudefrom 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.namespacelabel - 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>
|
Thanks for the thorough review. (1)
|
There was a problem hiding this comment.
🧹 Nitpick comments (7)
packages/extra/ingress/Makefile (1)
14-15: Declaretestas.PHONY.Static analysis (checkmake) flags this target as non-phony. Since
helm unittest .is not producing a file namedtest, 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: Declaretestas.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 emptyexpose-external-ips.When
expose-mode=loadBalancerin the publishing namespace with empty IPs,packages/extra/ingress/templates/nginx-ingress.yamlcallsfail(Lines 14–16 there), but this template silently emits zero documents because$ipsListis 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 samefailhere 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 thecontains "/"check from regressions (e.g., if someone refactors it to a globalcontains "/" $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-entrytrimis redundant.
nospacealready strips all whitespace from$exposeExternalIPs, so$ip := . | triminside the loop never has anything to trim. Either drop the globalnospace(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 assertsservice.type: ClusterIP. TheexternalIPslist andexternalTrafficPolicy: Clusterare the defining signals of that behavior — without asserting them, a regression that e.g. forgets to populateexternalIPswhen 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 (
LoadBalancervsloadbalancer) 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.yamlincludes explicit coverage for IPv6 (emitting/128CIDR) and mixed IPv4/IPv6 inputs (validating/32and/128per 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
📒 Files selected for processing (9)
packages/apps/tenant/Makefilepackages/apps/tenant/templates/cilium-lb-pool.yamlpackages/apps/tenant/tests/exposure_test.yamlpackages/core/platform/templates/apps.yamlpackages/core/platform/values.yamlpackages/extra/ingress/Makefilepackages/extra/ingress/README.mdpackages/extra/ingress/templates/nginx-ingress.yamlpackages/extra/ingress/tests/exposure_test.yaml
#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>
#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>
#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>
… 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>
… 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>
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>
…#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. [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2603) <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…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. ```
…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
Adds an opt-in
publishing.exposure=loadBalancermode for the ingress-nginx Service as a migration path away fromService.spec.externalIPs, which is deprecated upstream in Kubernetes v1.36 (KEP-5707, kubernetes#137293). TheAllowServiceExternalIPsfeature 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
CiliumLoadBalancerIPPoolatcilium.io/v2.Behavior
publishing.exposure— enumexternalIPs | loadBalancer, defaultexternalIPs(current behavior unchanged on upgrade).cozystack-valuesinto each tenant's ingress HelmRelease via the newexpose-modekey.exposure=loadBalancerand the current namespace matchespublishing.ingressName, the Service becomestype: LoadBalancerwithexternalTrafficPolicy: Localand aCiliumLoadBalancerIPPoolannounces the addresses frompublishing.externalIPsvia Cilium LB IPAM.serviceSelector(io.kubernetes.service.namespace: <ns>) — any LoadBalancer Service in the tenant namespace draws from it. See "Pool ownership" below./32CIDRs, IPv6 addresses get/128. Mixed families supported. Pre-CIDR entries (192.0.2.10/32) are accepted without double-suffixing.loadBalancerwith empty externalIPs, and stray empty entries frompublishing.externalIPsare rejected at render time with explicit error messages.Pool ownership
The
CiliumLoadBalancerIPPoolis rendered frompackages/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-valuesSecret, 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 getscilium.io/PoolConflictand 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 samepublishing.externalIPsrange whentenant.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 ownpackages/extra/gateway/templates/cilium-lb-pool.yaml, and adds an OR branch for.Values.gatewayin the tenant template.Scope
Only the ingress-nginx Service is migrated by this setting. Other cozystack components that still write
Service.spec.externalIPsdirectly (notably thevpnapp atpackages/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=falsein publishing tenant (no pool), non-publishing tenant (no pool), empty externalIPs (no pool).hack/helm-unit-tests.shvia thetest:target in each package's Makefile.Caveats (copied from the inline
values.yamlcomment)loadBalancermode usesexternalTrafficPolicy: Local. The external IP must already be routed to a node that hosts an ingress pod (floating IP / upstream router / podAntiAffinity).upgrade.force: trueon the HelmRelease + Service kind change); expect a brief interruption of ingress traffic.Release note