[INFRA-413] fix(plane-enterprise): decouple ingress controller type from class name#255
[INFRA-413] fix(plane-enterprise): decouple ingress controller type from class name#255pratapalakshmi wants to merge 3 commits into
Conversation
The ingress templates used `ingress.ingressClass` to decide both the resource kind (standard Ingress vs Traefik IngressRoute) and the class name string. As a result `ingress.yaml` rendered only for the exact value `nginx` and `ingress-traefik.yaml` only for values prefixed `traefik` — any other class name (a Traefik alias, or `nginx-new`) matched neither template and the ingress was silently dropped. Introduce `ingress.controller` to select the resource kind, decoupled from the free-form class name. Only `traefik` selects the IngressRoute; any other value selects a standard Ingress. Empty falls back to the previous auto-detection from `ingressClass`, so existing installs are unaffected. Also fix a pre-existing latent bug in ingress.yaml where `len` of a nil `ingress_annotations` threw a hard render error; switched to `with`. Bumps chart 2.6.2 -> 2.6.3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughThe chart introduces a new Changesingress.controller Selector for Traefik vs Standard Ingress
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
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 |
The ingress.controller feature stays (it implements the F5/nginx-new request). Remove only the README/values examples about serving a Traefik-named class alias via a standard Ingress, which is no longer a needed use case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/plane-enterprise/templates/ingress-traefik.yaml`:
- Line 1: The gate condition mismatch between ingress-traefik.yaml and
traefik-middleware.yaml causes the Middleware resource to fail rendering when
the IngressRoute succeeds. In traefik-middleware.yaml, replace the current gate
condition that uses hasPrefix with .Values.ingress.ingressClass with the same
condition used in ingress-traefik.yaml, which checks eq (include
"plane.ingressController" .) "traefik". This ensures both resources use the same
logic to determine when to render, preventing route admission failures due to
missing Middleware definitions.
In `@charts/plane-enterprise/values.yaml`:
- Around line 41-57: The ingress.ingressClass field has conflicting default
values between values.yaml and questions.yml. In values.yaml, the controller
field is set to an empty string while ingressClass is set to 'traefik', but
questions.yml specifies a different default. Update the default value in either
values.yaml or questions.yml to ensure consistency, so that the
ingress.ingressClass field has a single authoritative default value across both
configuration sources. Choose the default value that aligns with your intended
ingress controller behavior and ensure both files reflect the same default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dbec525f-2c8f-4840-bd79-e0e204816478
📒 Files selected for processing (6)
charts/plane-enterprise/Chart.yamlcharts/plane-enterprise/README.mdcharts/plane-enterprise/templates/_helpers.tplcharts/plane-enterprise/templates/ingress-traefik.yamlcharts/plane-enterprise/templates/ingress.yamlcharts/plane-enterprise/values.yaml
- traefik-middleware.yaml: gate on plane.ingressController (matching ingress-traefik.yaml) instead of hasPrefix on ingressClass. Prevents a missing Middleware (and route admission failure) when controller=traefik with a non-traefik ingressClass, and avoids an orphan Middleware in the reverse case. - questions.yml: default ingress.ingressClass to "traefik" to match values.yaml (the authoritative chart default). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
What
Decouples the ingress controller type (which resource kind to render) from the ingress class name in
plane-enterprise, via a newingress.controllervalue.templates/_helpers.tpl— newplane.ingressControllerhelper. Onlycontroller: traefikselects the TraefikIngressRoute; any other value selects the standardnetworking.k8s.io/v1 Ingress. Empty falls back to the previous auto-detect fromingressClass.templates/ingress.yaml— gate changed fromeq .Values.ingress.ingressClass "nginx"toeq (include "plane.ingressController" .) "ingress". Also fixed a pre-existing nil-pointer render error.templates/ingress-traefik.yaml— gate changed fromhasPrefix "traefik" .Values.ingress.ingressClasstoeq (include "plane.ingressController" .) "traefik".values.yaml— added documentedingress.controller: ''; clarifiedingressClassis now justspec.ingressClassName.README.md— rewrote the controller-selection section and values tables.Chart.yaml—version2.6.2 → 2.6.3 (appVersionunchanged).Key gate change:
Why
ingress.ingressClasswas overloaded to pick both the resource kind and the class-name string. The standardIngressrendered only for the exact valuenginx, so a standard controller using a non-standard class name likenginx-new(e.g. F5 NGINX) got no ingress at all, with no error and no resource — undocumented, silent behavior. Operators had to hand-roll a custom ingress and keep its routes in sync manually.ingress.controllerlets operators state the controller type explicitly while using any class name their controller exposes. This also aligns the templates with what the README already claimed ("any value other than traefik → standard Ingress"), which the code never actually did.Separately,
templates/ingress.yamlhad a latent bug:gt (len .Values.ingress.ingress_annotations) 0throwslen of nil pointerwheningress_annotationsis unset. It was rarely hit while the path was gated tonginx; making the standard path broadly reachable would expose it. Switched to{{- with .Values.ingress.ingress_annotations }}.Scope / behavior
ingress.controllerdefaults to''. When empty, the controller is auto-detected fromingressClassusing the prior rule (starts withtraefik→ Traefik, else standard Ingress). Existing values files render the same resource as before.controller: traefik→ TraefikIngressRoute. Any other non-empty value (nginx,f5,haproxy, ...) → standardIngress. The value is only a selector; the actual class name still comes fromingress.ingressClass.controllerto override auto-detect — e.g. a class name likenginx-new.ingress.ingress_annotationsrender only on the standardIngress; ignored when the controller resolves to Traefik (unchanged).license.licenseDomain), TLS (ssl.*), or any non-ingress template.Testing
helm lintclean;helm templateverified for each combination:ingressClass: traefikIngressRouteingressClass: nginx-new(nocontroller)Ingress,ingressClassName: nginx-newingress_annotationsunsetIngressrenders (no nil-pointer error)controller: traefik,ingressClass: whateverIngressRoutelicense.licenseDomainsetUpgrade notes
No action required for existing installs (auto-detect preserves prior behavior). Operators with atypical class names can now set
ingress.controllerinstead of maintaining a hand-rolled ingress.🤖 Generated with Claude Code