[kubernetes] Add option for exposing ingress-nginx via LoadBalancer - #1114
Conversation
Signed-off-by: Andrei Kvapil <[email protected]>
WalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HelmChart
participant values.yaml
participant ingress-nginx Controller
User->>HelmChart: Deploy with values.yaml
HelmChart->>values.yaml: Read addons.ingressNginx.exposeMethod
alt exposeMethod == "Proxied"
HelmChart->>ingress-nginx Controller: Set hostNetwork: true, service.enabled: false
HelmChart->>HelmChart: Render ingress resources if hosts present
else exposeMethod == "LoadBalancer"
HelmChart->>ingress-nginx Controller: Do not set hostNetwork/service.enabled
HelmChart->>HelmChart: Do not render ingress resources
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml (1)
6-10: Consider explicit Service defaults forLoadBalancermode
service.enabled: falseis now gated behindexposeMethod == "Proxied".
When the user switches to"LoadBalancer"we rely on the upstream chart’s defaults to create a Service of the desired type. That is fine, but it makes the behaviour implicit and can surprise operators.A tiny explicit override keeps intent clear:
{{- if eq .Values.addons.ingressNginx.exposeMethod "Proxied" }} hostNetwork: true service: enabled: false +{{- else if eq .Values.addons.ingressNginx.exposeMethod "LoadBalancer" }} + service: + type: LoadBalancer {{- end }}No functional break today, but worth considering for clarity.
packages/apps/kubernetes/templates/ingress.yaml (1)
3-4: Guard clause is fine – watch for nil.hostsUsing
.Values.addons.ingressNginx.hostsdirectly works when the key exists, but if the user removes it entirely the template may error.
A safer check:{{- if and (eq .Values.addons.ingressNginx.exposeMethod "Proxied") (hasKey .Values.addons.ingressNginx "hosts") }}Not blocking, but prevents an edge-case crash.
packages/apps/kubernetes/values.yaml (1)
64-72: Confirm template logic & tighten wording
exposeMethod/hostslook correct here, but please double-check that every Helm template compares the value case-sensitively against the same"Proxied"/"LoadBalancer"strings; otherwise the default could silently break.
Minor nit: “Taken into account” reads wordy—“considered only when” is shorter.packages/apps/kubernetes/README.md (1)
101-103: Rephrase hosts description for claritySame wordiness as in
values.yaml. Consider:-| `addons.ingressNginx.hosts` | List of domain names that the parent cluster should route to this tenant cluster. Taken into account only when `exposeMethod` is set to `Proxied`. | `[]` | +| `addons.ingressNginx.hosts` | List of domain names the parent cluster should route to this tenant cluster; considered only when `exposeMethod` is `Proxied`. | `[]` |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/apps/kubernetes/Chart.yaml(1 hunks)packages/apps/kubernetes/Makefile(1 hunks)packages/apps/kubernetes/README.md(1 hunks)packages/apps/kubernetes/templates/helmreleases/ingress-nginx.yaml(1 hunks)packages/apps/kubernetes/templates/ingress.yaml(1 hunks)packages/apps/kubernetes/values.schema.json(1 hunks)packages/apps/kubernetes/values.yaml(1 hunks)packages/apps/versions_map(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/apps/kubernetes/README.md
[style] ~102-~102: ‘Taken into account’ might be wordy. Consider a shorter alternative.
Context: ...er should route to this tenant cluster. Taken into account only when exposeMethod is set to `Pro...
(EN_WORDINESS_PREMIUM_TAKEN_INTO_ACCOUNT)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (3)
packages/apps/kubernetes/Chart.yaml (1)
18-20: Version bump looks good – double-check release notes & dependenciesGoing from
0.24.2→0.25.0is a minor-level bump, so remember to add a short entry to CHANGELOG/RELEASE_NOTES and verify that any chart dependencies also satisfy their version constraints.packages/apps/versions_map (1)
52-55: Mapping update LGTMThe pin of
0.24.2to commit4369b031and addition of0.25.0 HEADare consistent with the chart bump.packages/apps/kubernetes/values.schema.json (1)
181-189: Enum looks good—ensure matching case in templatesJSON-Schema enumeration is case-sensitive. Verify that template conditionals use exactly
Proxied/LoadBalancer; any mismatch will bypass the intended logic.
| generate: | ||
| readme-generator -v values.yaml -s values.schema.json -r README.md | ||
| yq -o json -i '.properties.addons.properties.ingressNginx.properties.exposeMethod.enum = ["Proxied","LoadBalancer"]' values.schema.json | ||
| yq -o json -i '.properties.controlPlane.properties.apiServer.properties.resourcesPreset.enum = ["none","nano","micro","small","medium","large","xlarge","2xlarge"]' values.schema.json |
There was a problem hiding this comment.
exposeMethod enum written after README generation – order likely wrong
readme-generator consumes values.schema.json, but the enum for exposeMethod is inserted only after the README is generated.
Result: the rendered docs won’t list the new allowed values.
-generate:
- readme-generator -v values.yaml -s values.schema.json -r README.md
- yq -o json -i '.properties.addons.properties.ingressNginx.properties.exposeMethod.enum = ["Proxied","LoadBalancer"]' values.schema.json
+generate:
+ yq -o json -i '.properties.addons.properties.ingressNginx.properties.exposeMethod.enum = ["Proxied","LoadBalancer"]' values.schema.json
+ readme-generator -v values.yaml -s values.schema.json -r README.mdSwitching the two lines ensures documentation and schema stay in sync.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| generate: | |
| readme-generator -v values.yaml -s values.schema.json -r README.md | |
| yq -o json -i '.properties.addons.properties.ingressNginx.properties.exposeMethod.enum = ["Proxied","LoadBalancer"]' values.schema.json | |
| yq -o json -i '.properties.controlPlane.properties.apiServer.properties.resourcesPreset.enum = ["none","nano","micro","small","medium","large","xlarge","2xlarge"]' values.schema.json | |
| generate: | |
| yq -o json -i '.properties.addons.properties.ingressNginx.properties.exposeMethod.enum = ["Proxied","LoadBalancer"]' values.schema.json | |
| readme-generator -v values.yaml -s values.schema.json -r README.md | |
| yq -o json -i '.properties.controlPlane.properties.apiServer.properties.resourcesPreset.enum = ["none","nano","micro","small","medium","large","xlarge","2xlarge"]' values.schema.json |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 7-7: Target body for "generate" exceeds allowed length of 5 (6).
(maxbodylength)
🤖 Prompt for AI Agents
In packages/apps/kubernetes/Makefile around lines 7 to 10, the enum values for
exposeMethod are updated in values.schema.json only after the README is
generated, causing the documentation to miss these new values. To fix this,
reorder the commands so that the yq commands updating the schema run before the
readme-generator command, ensuring the README is generated from the updated
schema.
There was a problem hiding this comment.
it's okay baby
There was a problem hiding this comment.
Andrei Kvapil (@kvaps) Got it! I'll respect your decision on keeping the current order. Thanks for the feedback.
1 participant
Signed-off-by: Andrei Kvapil [email protected]
Summary by CodeRabbit
exposeMethodoption and clarified the conditions under which domain names are used.