feat: add nodeport support #205
Conversation
WalkthroughThe Kubernetes service manifest template was updated to conditionally configure the service's port definitions based on the service type. When the type is "NodePort," specific port fields are set, including Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm Chart
participant Kubernetes
User->>Helm Chart: Deploys chart with values (service.type, nodePort)
Helm Chart->>Kubernetes: Renders service.yaml with conditional port configuration
Kubernetes-->>User: Creates Service with correct port and nodePort (if NodePort type)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
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}
charts/n8n/templates/service.yaml
Outdated
| {{ if .Values.main.service.type | eq "NodePort" -}} | ||
| - port: {{ default 80 .Values.main.service.port }} | ||
| targetPort: {{ default 80 .Values.main.service.port }} | ||
| nodePort: {{ default 30080 .Values.main.service.nodePort }} | ||
| {{- else }} | ||
| - port: {{ default 80 .Values.main.service.port }} | ||
| targetPort: http | ||
| protocol: TCP | ||
| name: http | ||
| {{- end }} |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Simplify the conditional and align the NodePort stanza with Helm/K8s conventions
-
The current conditional uses the pipeline form
{{ if .Values.main.service.type | eq "NodePort" -}}.
While technically valid, the canonical Helm style is{{- if eq .Values.main.service.type "NodePort" }}– it’s easier to read and avoids the cognitive “which side is piped?” question. -
The NodePort branch omits
protocolandname, and hard-codestargetPortto the same numeric value asport.
In the existingClusterIPbranch,targetPortis the named porthttp; changing semantics for the NodePort path is surprising and may break probes or NetworkPolicy rules that rely on the named target port. -
Adding
name: httpkeeps the port list deterministic and avoids the Helm linter warning “(name) must be specified when more than one port is defined” if a second port is added later.
Proposed patch:
- {{ if .Values.main.service.type | eq "NodePort" -}}
- - port: {{ default 80 .Values.main.service.port }}
- targetPort: {{ default 80 .Values.main.service.port }}
- nodePort: {{ default 30080 .Values.main.service.nodePort }}
+ {{- if eq .Values.main.service.type "NodePort" }}
+ - name: http
+ port: {{ default 80 .Values.main.service.port }}
+ targetPort: http
+ protocol: TCP
+ nodePort: {{ default 30080 .Values.main.service.nodePort }}This keeps the generated Service manifest consistent across service types and follows Helm best practices.
Simplify Helm conditional and unify NodePort Service stanza
- Replace the piped conditional with the canonical Helm form for readability:
{{- if eq .Values.main.service.type "NodePort" }} - In the NodePort branch, mirror the ClusterIP settings by:
- Adding
name: http - Using
targetPort: httpinstead of a hard-coded number - Specifying
protocol: TCP
- Adding
- Preserve the
nodePortassignment as-is.
Proposed diff (charts/n8n/templates/service.yaml, around lines 15–24):
@@ -15,8 +15,11 @@ spec:
- {{ if .Values.main.service.type | eq "NodePort" -}}
- - port: {{ default 80 .Values.main.service.port }}
- targetPort: {{ default 80 .Values.main.service.port }}
- nodePort: {{ default 30080 .Values.main.service.nodePort }}
+ {{- if eq .Values.main.service.type "NodePort" }}
+ - name: http
+ port: {{ default 80 .Values.main.service.port }}
+ targetPort: http
+ protocol: TCP
+ nodePort: {{ default 30080 .Values.main.service.nodePort }}
{{- else }}
- port: {{ default 80 .Values.main.service.port }}
targetPort: http🤖 Prompt for AI Agents
In charts/n8n/templates/service.yaml around lines 15 to 24, simplify the Helm
conditional by replacing the piped form with the canonical form {{- if eq
.Values.main.service.type "NodePort" }} for better readability. In the NodePort
branch, unify the service stanza by adding name: http, changing targetPort to
http instead of a hard-coded port number, and specifying protocol: TCP, while
keeping the nodePort assignment unchanged.
|
Thanks, I suggest to also include this option into the readme |
|
Thanks for the suggestion, @Vad1mo |
charts/n8n/templates/service.yaml
Outdated
| spec: | ||
| type: {{ default "ClusterIP_" .Values.main.service.type }} | ||
| ports: | ||
| {{ if .Values.main.service.type | eq "NodePort" -}} |
There was a problem hiding this comment.
| {{ if .Values.main.service.type | eq "NodePort" -}} | |
| {{- if eq .Values.main.service.type "NodePort" }} |
Let's not use the pipe notation here
|
Hi guys, |
|
Hi guys, I'm a bit confused with the linting error. It seems something wrong with github action configs. Can someone please help? Thanks. |
4 participants
Just a simple change to add support of
nodePort, so that we can specify our ownnodePortfor a service of a servicetype: NodePortSummary by CodeRabbit
New Features
Bug Fixes