Nginx custom ports #581
Nginx custom ports #581
Conversation
pleshakov
added
the
enhancement
|
@ameijer-corsha thanks for the PR! we will review it shortly |
There was a problem hiding this comment.
Sorry for the delay in reviewing!
Currently, the controller.service.httpPort.* and controller.service.httpsPort.* parameters configure the ports of the service but don't configure the ports of the Ingress Controller deployment/deamonset. However, the proposed controller.service.customPorts parameter configures both service and deployment/deamonset ports. This is inconsistent.
What do you think about the following proposal, illustrated by an excerpt from a values file?
controller:
. . .
customPorts:
- name: dns
containerPort: 53
. . .
service:
. . .
customPorts:
- port: 53
targetPort: 53
protocol: UDP
name: dns
Here we have independent customPorts - for deployment/daemonset and for the service.
I understand that this will lead to some duplication in the values file when configuring the ports. However, this will be consistent with the existing parameters and also allow some flexibility. For example, if a user wants to expose container ports but doesn't want to expose them through the service.
Please also see some comments regarding the current implementation.
deployments/helm-chart/values.yaml
Outdated
| @@ -157,6 +157,9 @@ controller: | |||
| ## The HTTPS port on the POD where the Ingress controller service is running. | |||
| targetPort: 443 | |||
|
|
|||
| ## yaml definition of custom ports to expose in the service | |||
There was a problem hiding this comment.
the doc should match the doc string in the table in the README.
deployments/helm-chart/values.yaml
Outdated
| @@ -157,6 +157,9 @@ controller: | |||
| ## The HTTPS port on the POD where the Ingress controller service is running. | |||
| targetPort: 443 | |||
|
|
|||
| ## yaml definition of custom ports to expose in the service | |||
| customPorts: {} | |||
There was a problem hiding this comment.
customPorts is an array - []
|
@pleshakov Thank you for the review! I believe I have addressed all your concerns. I rendered this a few times, all looks good. Could you review again when you have the chance? much appreciated... |
|
@ameijer-corsha thx! looks good! |
ameijer-corsha
force-pushed
the
nginx-custom-ports
branch
from
08f62ea
to
c23fe89
Compare
|
@ameijer-corsha could you possibly change the commit message to "Support custom ports in helm chart"? In the message body, you can add "Closes #580", which will close the issue once the commit is merged. Please see our git style guide https://github.com/nginxinc/kubernetes-ingress/blob/master/CONTRIBUTING.md#git-style-guide We're trying to keep the commit history consistent https://github.com/nginxinc/kubernetes-ingress/commits/master Thanks for understanding! |
closes nginxinc#580 dox Update deployments/helm-chart/README.md Co-Authored-By: Michael Pleshakov <pleshakov@users.noreply.github.com> address code review feedback, allow different ports for each tier fix indenting of container ports
ameijer-corsha
force-pushed
the
nginx-custom-ports
branch
from
c23fe89
to
a463d7c
Compare
|
@ameijer-corsha thanks! |
|
@pleshakov is there a roadmap for when this feature will make it into the stable chart? we are trying to plan our prod deployment |
|
@ameijer-corsha |
Proposed changes
Addresses #580
Checklist
Before creating a PR, run through this checklist and mark each as complete.