Skip to content

Conversation

@bapung
Copy link

@bapung bapung commented Dec 22, 2025

issue #97

Copilot AI review requested due to automatic review settings December 22, 2025 14:37
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for configuring additional host ports in DaemonSet deployments by introducing the controller.daemonset.extraHostPorts configuration option. This addresses issue #97 by allowing users to expose extra ports beyond the standard HTTP, HTTPS, and TCP ports when using host port mode.

Key Changes

  • Adds extraHostPorts configuration loop in the pod template to dynamically configure additional host port mappings
  • Documents the new controller.daemonset.extraHostPorts parameter in the README with field specifications

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
haproxy-ingress/templates/_podtemplate.yaml Adds template logic to iterate over extraHostPorts and create port configurations with name, containerPort, and hostPort
haproxy-ingress/README.md Documents the new extraHostPorts configuration parameter and its expected structure

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{{- if $.Values.controller.daemonset.useHostPort }}
{{- range $row := .Values.controller.daemonset.extraHostPorts }}
- name: "extra-port-{{ $row.port }}"
containerPort: {{ $row.containerPort }}
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extraHostPorts feature lacks support for the hostIP configuration that is available for other daemonset host ports. All other daemonset host port configurations in this template (see lines 315-316, 325-326, 353-354) include conditional hostIP support when controller.daemonset.hostIP is set. This should be added to allow users to bind extra ports to a specific host IP address.

Suggested change
containerPort: {{ $row.containerPort }}
containerPort: {{ $row.containerPort }}
{{- if $.Values.controller.daemonset.hostIP }}
hostIP: {{ $.Values.controller.daemonset.hostIP }}
{{- end }}

Copilot uses AI. Check for mistakes.
Comment on lines 365 to 371
{{- if $.Values.controller.daemonset.useHostPort }}
{{- range $row := .Values.controller.daemonset.extraHostPorts }}
- name: "extra-port-{{ $row.port }}"
containerPort: {{ $row.containerPort }}
hostPort: {{ $row.hostPort }}
{{- end }}
{{- end }}
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extraHostPorts block is not wrapped in a check for controller.kind being "DaemonSet". All other daemonset-specific host port configurations in this template (lines 314-321, 324-331, 352-363) only apply when controller.kind equals "DaemonSet". Without this check, the extraHostPorts configuration could incorrectly be applied to Deployment configurations where host ports are not supported, potentially causing deployment failures.

Copilot uses AI. Check for mistakes.
`controller.daemonset.hostPorts.http` | If `controller.daemonset.useHostPort` is `true` and this is non-empty sets the hostPort for http | `"80"`
`controller.daemonset.hostPorts.https` | If `controller.daemonset.useHostPort` is `true` and this is non-empty sets the hostPort for https | `"443"`
`controller.daemonset.hostPorts.tcp` | If `controller.daemonset.useHostPort` is `true` use hostport for these ports from `tcp` | `[]`
`controller.daemonset.extraHostPorts` | If `controller.daemonset.useHostPort` is `true` configure additional host ports mapping. Each entry should specify `port`, `containerPort`, and `hostPort` | `[]`
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation mentions that each entry should specify port, containerPort, and hostPort, but the template only uses the port field for generating the port name. This creates confusion about the purpose of the port field. Consider either removing port from the documentation if it's only used for naming, or clarifying its purpose (e.g., "Each entry should specify port (used for naming), containerPort, and hostPort").

Suggested change
`controller.daemonset.extraHostPorts` | If `controller.daemonset.useHostPort` is `true` configure additional host ports mapping. Each entry should specify `port`, `containerPort`, and `hostPort` | `[]`
`controller.daemonset.extraHostPorts` | If `controller.daemonset.useHostPort` is `true` configure additional host ports mapping. Each entry should specify `port` (used for naming), `containerPort`, and `hostPort` | `[]`

Copilot uses AI. Check for mistakes.
{{- if $.Values.controller.daemonset.useHostPort }}
{{- range $row := .Values.controller.daemonset.extraHostPorts }}
- name: "extra-port-{{ $row.port }}"
containerPort: {{ $row.containerPort }}
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extraHostPorts implementation is missing a protocol specification. All other port definitions in this template include protocol: TCP (see lines 336, 344, 351). This field should be added to maintain consistency and ensure proper protocol configuration.

Suggested change
containerPort: {{ $row.containerPort }}
containerPort: {{ $row.containerPort }}
protocol: TCP

Copilot uses AI. Check for mistakes.
@jcmoraisjr
Copy link
Member

Hi thanks for the contribution! A few suggestions below, including the ones from copilot.

  • Consider using a hashmap instead, see e.g. how .Values.controller.tcp is configured. When adding the name of the port, configure it as "{{ $key }}-extra" following the current pattern. .extraHostPorts under .daemonset looks good.
  • Missing protocol field - not mandatory but good to be declared
  • Missing a check for the Daemonset kind, see .tcp iteration
  • Missing an entry under values.yaml. You can simplify the documentation under README, adding the details about the syntax under values.yaml instead, e.g. adding an usage example.

@bapung
Copy link
Author

bapung commented Jan 5, 2026

Thanks for taking a look at this @jcmoraisjr

I wonder if map should be just defined like so:

extraHostPorts:
  <hostPort>:<containerPort>

As this is configured for daemonset, we only interested in the host to container port mapping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants