feat: support auto assign node port for stream subsystem#258
Conversation
Signed-off-by: Nic <qianyong@api7.ai>
There was a problem hiding this comment.
Pull request overview
This pull request adds support for automatic nodePort assignment in the stream subsystem (TCP/UDP) and adds a nodePort configuration option for the serviceMonitor. The changes include updating the Helm chart configuration, templates, documentation, and bumping the chart version from 0.2.53 to 0.2.54.
Changes:
- Added
gateway.stream.autoAssignNodePortconfiguration option to enable automatic nodePort assignment for TCP/UDP stream ports - Added
serviceMonitor.nodePortconfiguration option to specify a nodePort for the Prometheus metrics endpoint - Updated the service-gateway.yaml template to implement the nodePort assignment logic
- Updated documentation in README.md and values.yaml to reflect the new configuration options
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| charts/gateway/values.yaml | Added autoAssignNodePort flag for stream subsystem and nodePort option for serviceMonitor |
| charts/gateway/templates/service-gateway.yaml | Implemented nodePort assignment logic for serviceMonitor and stream TCP/UDP ports based on new configuration |
| charts/gateway/README.md | Updated documentation to reflect new configuration parameters |
| charts/gateway/Chart.yaml | Bumped chart version from 0.2.53 to 0.2.54 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- if eq $global.Values.gateway.type "NodePort" }} | ||
| {{- if $global.Values.gateway.stream.autoAssignNodePort }} | ||
| nodePort: {{ splitList ":" ($port.addr | toString) | last }} | ||
| {{- else }} | ||
| {{- if not (empty $port.nodePort) }} | ||
| nodePort: {{ $port.nodePort }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
The auto-assign nodePort logic may not work as expected. When autoAssignNodePort is true, the code sets nodePort to the port value extracted from the address. However, if a user specifies addr: 5353 (a port outside the typical Kubernetes nodePort range of 30000-32767), this will fail when the service is created. The comment in values.yaml warns users to ensure the port is in the valid range, but the template doesn't validate or handle this case. Consider adding validation or a more explicit error message in the template comments to make this limitation clearer.
| {{- end}} | ||
| {{- end }} |
There was a problem hiding this comment.
Missing space before closing braces. The line should be {{- end }} instead of {{- end}} to maintain consistency with the rest of the template file.
| {{- end}} | |
| {{- end }} | |
| {{- end }} | |
| {{- end }} |
| {{- if eq $global.Values.gateway.type "NodePort" }} | ||
| {{- if $global.Values.gateway.stream.autoAssignNodePort }} | ||
| nodePort: {{ splitList ":" ($port.addr | toString) | last }} | ||
| {{- else }} | ||
| {{- if not (empty $port.nodePort) }} | ||
| nodePort: {{ $port.nodePort }} | ||
| {{- end }} | ||
| {{- end}} | ||
| {{- end }} |
There was a problem hiding this comment.
The auto-assign nodePort logic may not work as expected. When autoAssignNodePort is true, the code sets nodePort to the port value extracted from the address. However, if a user specifies addr: 3302 (a port outside the typical Kubernetes nodePort range of 30000-32767), this will fail when the service is created. The comment in values.yaml warns users to ensure the port is in the valid range, but the template doesn't validate or handle this case. Consider adding validation or a more explicit error message in the template comments to make this limitation clearer.
2 participants
No description provided.