[lb] enable nodeports for services - #1254
Conversation
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
WalkthroughThe changes remove the explicit Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
💤 Files with no reviewable changes (7)
✨ 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 (
|
There was a problem hiding this comment.
Summary of Changes
Hello IvanHunters, 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 standardizes the configuration of several application services by enabling NodePort allocation for LoadBalancer type services. This change aims to enhance the flexibility and external accessibility of these applications within a Kubernetes environment by ensuring that NodePorts are always provisioned alongside LoadBalancers.
Highlights
- Kubernetes Service Configuration: I've updated the Helm templates for several application services to explicitly enable NodePort allocation for
LoadBalancertype services. This is achieved by settingallocateLoadBalancerNodePorts: true. - Enhanced External Accessibility: This change ensures that when a service is exposed via a LoadBalancer, corresponding NodePorts are also allocated, which can be crucial for certain network configurations or direct node-based access patterns.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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 or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
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. ↩
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
packages/apps/vm-instance/templates/service.yaml (1)
6-16: Template appears to copy references fromvirtual-machinechart.
include "virtual-machine.fullname"and"virtual-machine.labels"will break rendering unless those helper templates are deliberately shared across charts.
Ifvm-instancehas its own helpers, update the include names; otherwise, consider renaming the chart directory for consistency.- name: {{ include "virtual-machine.fullname" . }} + name: {{ include "vm-instance.fullname" . }} ... - {{- include "virtual-machine.labels" . | nindent 4 }} + {{- include "vm-instance.labels" . | nindent 4 }}packages/apps/tcp-balancer/templates/service.yaml (1)
5-16: Service/label names referencehaproxy– likely copy-paste error.A TCP balancer service named
*-haproxyis confusing and risks collision with the real HAProxy chart. Rename to match the chart’s purpose.- name: {{ .Release.Name }}-haproxy ... - app: {{ .Release.Name }}-haproxy + name: {{ .Release.Name }}-tcp-balancer ... + app: {{ .Release.Name }}-tcp-balancer
🧹 Nitpick comments (2)
packages/apps/postgres/templates/external-svc.yaml (1)
8-11:allocateLoadBalancerNodePorts: trueis functionally redundant—verify necessity and cluster compatibility.
allocateLoadBalancerNodePortsdefaults totrueforLoadBalancerservices in K8s ≥ 1.20.
Keeping it explicit is harmless but adds noise and must be supported by every target cluster version; older (< 1.20) installs or providers that still expose the alpha/feature-gated field will reject the manifest.Action items
- Confirm all supported clusters are ≥ 1.20 (or the provider’s API accepts the field).
- If the minimum version guarantee is not firm, consider omitting the line or template-guarding it behind a
.Values.k8sVersion/feature flag to avoid failed applies.packages/apps/redis/templates/service.yaml (1)
12-15: Repeat of the explicitallocateLoadBalancerNodePorts: true—consider de-duplication via a helper or variable.The same reasoning as above applies; additionally, replicating the identical stanza across many charts invites drift.
You could move the conditional block into a shared_helpers.tplsnippet or a global.Values.service.allocateLBNodePortsto ensure consistency across all services.Example helper extraction:
{{/* _helpers.tpl */}} {{- define "cozystack.lbNodePortSettings" -}} {{- if .Values.external }} externalTrafficPolicy: Local allocateLoadBalancerNodePorts: true {{- end }} {{- end }}Then:
spec: type: {{ ternary "LoadBalancer" "ClusterIP" .Values.external }} {{- include "cozystack.lbNodePortSettings" . | nindent 2 }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/apps/ferretdb/templates/external-svc.yaml(1 hunks)packages/apps/http-cache/templates/haproxy/service.yaml(1 hunks)packages/apps/postgres/templates/external-svc.yaml(1 hunks)packages/apps/redis/templates/service.yaml(1 hunks)packages/apps/tcp-balancer/templates/service.yaml(1 hunks)packages/apps/virtual-machine/templates/service.yaml(1 hunks)packages/apps/vm-instance/templates/service.yaml(1 hunks)
🔇 Additional comments (3)
packages/apps/http-cache/templates/haproxy/service.yaml (1)
11-13: Be certain the target cluster supportsallocateLoadBalancerNodePorts.The field is only honored on clusters ≥ v1.20 and ignored (or causes validation errors) on older control-planes.
Double-check the minimum supported version in your deployment matrix before merging.packages/apps/virtual-machine/templates/service.yaml (1)
14-16: OK – change is consistent with the other charts.
No functional concerns here.packages/apps/ferretdb/templates/external-svc.yaml (1)
9-11: Cluster-version caveat (same as other charts).Same remark as in HAProxy chart: verify the control-plane is ≥ v1.20 so the new field is accepted.
There was a problem hiding this comment.
Code Review
The pull request enables nodeports for services by setting allocateLoadBalancerNodePorts to true in the service templates for several applications. This change could expose services on node ports, so it's important to ensure that appropriate security measures are in place and that the implications of this change are documented.
| {{- if .Values.external }} | ||
| externalTrafficPolicy: Local | ||
| allocateLoadBalancerNodePorts: false | ||
| allocateLoadBalancerNodePorts: true |
There was a problem hiding this comment.
Enabling allocateLoadBalancerNodePorts can expose services on node ports, which might have security implications. Ensure that this is the desired behavior and that appropriate security measures are in place, such as network policies, to restrict access to these ports. Consider documenting the implications of enabling this feature in the chart's README or values.yaml file.
allocateLoadBalancerNodePorts: true # Ensure appropriate security measures are in place
| {{- if .Values.external }} | ||
| externalTrafficPolicy: Local | ||
| allocateLoadBalancerNodePorts: false | ||
| allocateLoadBalancerNodePorts: true |
There was a problem hiding this comment.
Enabling allocateLoadBalancerNodePorts can expose services on node ports, which might have security implications. Ensure that this is the desired behavior and that appropriate security measures are in place, such as network policies, to restrict access to these ports. Consider documenting the implications of enabling this feature in the chart's README or values.yaml file.
allocateLoadBalancerNodePorts: true # Ensure appropriate security measures are in place
| {{- if .Values.external }} | ||
| externalTrafficPolicy: Local | ||
| allocateLoadBalancerNodePorts: false | ||
| allocateLoadBalancerNodePorts: true |
There was a problem hiding this comment.
Enabling allocateLoadBalancerNodePorts can expose services on node ports, which might have security implications. Ensure that this is the desired behavior and that appropriate security measures are in place, such as network policies, to restrict access to these ports. Consider documenting the implications of enabling this feature in the chart's README or values.yaml file.
allocateLoadBalancerNodePorts: true # Ensure appropriate security measures are in place
| {{- if .Values.external }} | ||
| externalTrafficPolicy: Local | ||
| allocateLoadBalancerNodePorts: false | ||
| allocateLoadBalancerNodePorts: true |
There was a problem hiding this comment.
Enabling allocateLoadBalancerNodePorts can expose services on node ports, which might have security implications. Ensure that this is the desired behavior and that appropriate security measures are in place, such as network policies, to restrict access to these ports. Consider documenting the implications of enabling this feature in the chart's README or values.yaml file.
allocateLoadBalancerNodePorts: true # Ensure appropriate security measures are in place
| {{- if .Values.external }} | ||
| externalTrafficPolicy: Local | ||
| allocateLoadBalancerNodePorts: false | ||
| allocateLoadBalancerNodePorts: true |
There was a problem hiding this comment.
Enabling allocateLoadBalancerNodePorts can expose services on node ports, which might have security implications. Ensure that this is the desired behavior and that appropriate security measures are in place, such as network policies, to restrict access to these ports. Consider documenting the implications of enabling this feature in the chart's README or values.yaml file.
allocateLoadBalancerNodePorts: true # Ensure appropriate security measures are in place
| type: {{ ternary "LoadBalancer" "ClusterIP" .Values.external }} | ||
| externalTrafficPolicy: Local | ||
| allocateLoadBalancerNodePorts: false | ||
| allocateLoadBalancerNodePorts: true |
There was a problem hiding this comment.
Enabling allocateLoadBalancerNodePorts can expose services on node ports, which might have security implications. Ensure that this is the desired behavior and that appropriate security measures are in place, such as network policies, to restrict access to these ports. Consider documenting the implications of enabling this feature in the chart's README or values.yaml file.
allocateLoadBalancerNodePorts: true # Ensure appropriate security measures are in place
| type: {{ ternary "LoadBalancer" "ClusterIP" .Values.external }} | ||
| externalTrafficPolicy: Local | ||
| allocateLoadBalancerNodePorts: false | ||
| allocateLoadBalancerNodePorts: true |
There was a problem hiding this comment.
Enabling allocateLoadBalancerNodePorts can expose services on node ports, which might have security implications. Ensure that this is the desired behavior and that appropriate security measures are in place, such as network policies, to restrict access to these ports. Consider documenting the implications of enabling this feature in the chart's README or values.yaml file.
allocateLoadBalancerNodePorts: true # Ensure appropriate security measures are in place
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
Suppressed by #1271 |
What this PR does
enable nodeports for services
Release note
Summary by CodeRabbit