feat(helm): add support for nodePort specification in LoadBalancer services helm chart #16032

MRColorR · 2025-01-03T17:07:26Z

Short description:

This pull request introduces support for optionally specify nodePort values when using LoadBalancer service type in the Coder Helm chart. This enhancement addresses a limitation where httpNodePort and httpsNodePort values were previously ignored for LoadBalancer services. This PR should expand the service customization options without disrupting existing configurations.

Why this is Useful

In some enterprise environments, applications may be required to use specific ports for compliance with organizational policies or cloud infrastructure requirements. For instance:

Reserved port blocks are allocated for specific applications for security and clarity.
Ensuring predictable port assignments helps in debugging and management scenarios.

Since LoadBalancer in Kubernetes operates on top of nodePort, this feature is useful for enabling enterprises to adhere to such policies if they whish.

What Was Changed

Updated helm/coder/templates/service.yaml:
- Allowed nodePort specification for both NodePort and LoadBalancer service types.
Updated helm/coder/templates/values.yaml:
- Updated inline comments to reflect the changes for nodeport values use cases.

Regarding backward compatibility:

If nodePort is not specified, Kubernetes dynamically assigns a port, maintaining the current behavior.

Testing Performed

Validated through Helm dry-run: nodePort values are rendered correctly in the resulting Kubernetes YAML.
Deployed the updated chart in an enterprise Kubernetes cluster.
Tested coder environment with LoadBalancer service and specified nodePort values for both HTTP and HTTPS.

Additional Notes

This PR expands the nodeport functionality introduced in PR feat: add support for NodePort service type in Helm chart #8993 to the Loadbalancer service.
If merged, an update to the documentation to include examples of LoadBalancer with nodePort values may be useful.
I've read the contributing guidelines and code of conduct. This is my first PR for the Coder project, and I hope it meets the community standards. Any advice, feedback, or help is greatly appreciated!

…on for LoadBalancer services, expanding coder#8993 work

… changes for nodeport values use cases

github-actions · 2025-01-03T17:07:46Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

MRColorR · 2025-01-10T14:18:44Z

I have read the CLA Document and I hereby sign the CLA

MRColorR · 2025-01-10T15:36:08Z

recheck

matifali · 2025-01-15T13:37:03Z

Hi, @MRColorR, you also need to run make gen to update generated files.

johnstcn

Please also add a corresponding helm test for the new loadbalancer type.

MRColorR · 2025-01-16T17:36:51Z

Hi, @MRColorR, you also need to run make gen to update generated files.

hello, thank you for the help. i've done it in my code devcontainer but i see no new files in git status.
make gen exited without errors after some time.

MRColorR · 2025-01-16T17:38:21Z

Please also add a corresponding helm test for the new loadbalancer type.

could you please elaborate more ? i've see there's an already existing svc_loadbalancer_class test , would you like me to extend this one or create a new one ?

…better readability

…golden files

johnstcn · 2025-01-16T17:58:49Z

could you please elaborate more ? i've see there's an already existing svc_loadbalancer_class test , would you like me to extend this one or create a new one ?

@MRColorR Sure thing! As I see it, there are three paths we need to consider:

If coder.service.type is LoadBalancer, then template coder.service.httpNodePort and coder.service.httpsNodePort (new behaviour, currently not covered)
If coder.service.type is NodePort, then template coder.service.httpNodePort and coder.service.httpsNodePort (existing behaviour, currently not covered)
Otherwise, do not template coder.service.httpNodePort and coder.service.httpsNodePort (existing behaviour, covered)

Based on that, I think we need to:

Add helm/coder/tests/testdata/svc_nodeport.yaml with the corresponding values to set service_type to NodePort. It looks like test coverage was missed when this was previously added. You'll also want to add a corresponding test case in helm/coder/tests/testdata/chart_test.go.
Update existing helm/coder/tests/testdata/*.golden with your current set of changes and review the output of helm template.

Tip: you can run go test ./helm/coder/tests -update to update the existing golden files without failing on a diff.

Hope that helps!

MRColorR · 2025-01-16T18:06:28Z

could you please elaborate more ? i've see there's an already existing svc_loadbalancer_class test , would you like me to extend this one or create a new one ?

@MRColorR Sure thing! As I see it, there are three paths we need to consider:

If coder.service.type is LoadBalancer, then template coder.service.httpNodePort and coder.service.httpsNodePort (new behaviour, currently not covered)

If coder.service.type is NodePort, then template coder.service.httpNodePort and coder.service.httpsNodePort (existing behaviour, currently not covered)

Otherwise, do not template coder.service.httpNodePort and coder.service.httpsNodePort (existing behaviour, covered)

Based on that, I think we need to:

Add helm/coder/tests/testdata/svc_nodeport.yaml with the corresponding values to set service_type to NodePort. It looks like test coverage was missed when this was previously added. You'll also want to add a corresponding test case in helm/coder/tests/testdata/chart_test.go.

Update existing helm/coder/tests/testdata/*.golden with your current set of changes and review the output of helm template.

Tip: you can run go test ./helm/coder/tests -update to update the existing golden files without failing on a diff.

Hope that helps!

ok , i was already working on a test file sorry for the overlap. now i'll add the case you've mentioned.

…test

MRColorR · 2025-01-16T20:50:33Z

I've added the test cases as requested. Please let me know if there's anything else I should do.

johnstcn

Manual testing:

Ran kubectl apply on following files in a temporary namespace:

default_values.golden applied successfully:

NAME                         READY   STATUS    RESTARTS   AGE
pod/coder-7cc8f95b66-d2gcs   0/1     Running   0          3s

NAME            TYPE           CLUSTER-IP      EXTERNAL-IP    PORT(S)        AGE
service/coder   LoadBalancer   10.101.182.44   192.168.1.34   80:31198/TCP   3s

NAME                    READY   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/coder   0/1     1            0           3s

NAME                               DESIRED   CURRENT   READY   AGE
replicaset.apps/coder-7cc8f95b66   1         1         0       3s

svc_loadbalancer.golden applied successfully:

NAME                         READY   STATUS    RESTARTS   AGE
pod/coder-7cc8f95b66-wtw9j   0/1     Running   0          5s

NAME            TYPE           CLUSTER-IP       EXTERNAL-IP    PORT(S)        AGE
service/coder   LoadBalancer   10.104.167.126   192.168.1.34   80:30080/TCP   5s

NAME                    READY   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/coder   0/1     1            0           5s

NAME                               DESIRED   CURRENT   READY   AGE
replicaset.apps/coder-7cc8f95b66   1         1         0       5s

svc_loadbalancer_class.golden applied successfully:

NAME                         READY   STATUS    RESTARTS   AGE
pod/coder-7cc8f95b66-tq5ht   0/1     Running   0          3s

NAME            TYPE           CLUSTER-IP      EXTERNAL-IP   PORT(S)        AGE
service/coder   LoadBalancer   10.99.184.182   <pending>     80:32053/TCP   3s

NAME                    READY   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/coder   0/1     1            0           3s

NAME                               DESIRED   CURRENT   READY   AGE
replicaset.apps/coder-7cc8f95b66   1         1         0       3s

svc_nodeport.golden applied successfully:

NAME                         READY   STATUS    RESTARTS   AGE
pod/coder-7cc8f95b66-fnlg6   0/1     Running   0          6s

NAME            TYPE       CLUSTER-IP       EXTERNAL-IP   PORT(S)        AGE
service/coder   NodePort   10.111.126.236   <none>        80:30080/TCP   6s

NAME                    READY   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/coder   0/1     1            0           6s

NAME                               DESIRED   CURRENT   READY   AGE
replicaset.apps/coder-7cc8f95b66   1         1         0       6s

johnstcn · 2025-01-17T10:11:02Z

@ericpaulsen OK to merge?

ericpaulsen · 2025-01-20T13:13:33Z

@johnstcn LGTM - pressing the big green button.

MRColorR added 2 commits January 3, 2025 17:38

feat(helm\coder\templates\service.yaml): support nodePort specificati…

efb0468

…on for LoadBalancer services, expanding coder#8993 work

chore(helm\coder\values.yaml): updated inline comments to reflect the…

cd88420

… changes for nodeport values use cases

cdr-bot bot added the community Pull Requests and issues created by the community. label Jan 3, 2025

github-actions bot assigned MRColorR Jan 3, 2025

MRColorR changed the title ~~Feat: Add support for nodePort specification in LoadBalancer services helm chart~~ feat(helm): add support for nodePort specification in LoadBalancer services helm chart Jan 3, 2025

cdrci2 added a commit to coder/cla that referenced this pull request Jan 10, 2025

@MRColorR has signed the CLA in coder/coder#16032

9ca2f65

matifali requested review from johnstcn and ericpaulsen January 15, 2025 05:24

johnstcn reviewed Jan 15, 2025

View reviewed changes

Merge branch 'coder:main' into feat/loadbalancer-nodeport

2393104

MRColorR added 2 commits January 16, 2025 17:53

chore(helm): adjust nodeport whitespace in service.yaml template for …

4c2b7a4

…better readability

feat(tests): added LoadBalancer nodePort type test cases and updated …

25f85d0

…golden files

Merge branch 'coder:main' into feat/loadbalancer-nodeport

46d6707

MRColorR and others added 2 commits January 16, 2025 20:54

Merge branch 'coder:main' into feat/loadbalancer-nodeport

6ab963a

chore(tests): added nodePort type test and updated LoadBalancer type …

e7baf75

…test

matifali requested a review from johnstcn January 17, 2025 07:31

johnstcn approved these changes Jan 17, 2025

View reviewed changes

ericpaulsen approved these changes Jan 20, 2025

View reviewed changes

ericpaulsen merged commit d8fbbcb into coder:main Jan 20, 2025
30 checks passed

github-actions bot locked and limited conversation to collaborators Jan 20, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(helm): add support for nodePort specification in LoadBalancer services helm chart #16032

feat(helm): add support for nodePort specification in LoadBalancer services helm chart #16032

MRColorR commented Jan 3, 2025

github-actions bot commented Jan 3, 2025 •

edited

Loading

MRColorR commented Jan 10, 2025

MRColorR commented Jan 10, 2025

matifali commented Jan 15, 2025

johnstcn left a comment

MRColorR commented Jan 16, 2025 •

edited

Loading

MRColorR commented Jan 16, 2025

johnstcn commented Jan 16, 2025 •

edited

Loading

MRColorR commented Jan 16, 2025

MRColorR commented Jan 16, 2025

johnstcn left a comment

johnstcn commented Jan 17, 2025

ericpaulsen commented Jan 20, 2025 •

edited

Loading

feat(helm): add support for nodePort specification in LoadBalancer services helm chart #16032

feat(helm): add support for nodePort specification in LoadBalancer services helm chart #16032

Conversation

MRColorR commented Jan 3, 2025

Short description:

Why this is Useful

What Was Changed

Regarding backward compatibility:

Testing Performed

Additional Notes

github-actions bot commented Jan 3, 2025 • edited Loading

MRColorR commented Jan 10, 2025

MRColorR commented Jan 10, 2025

matifali commented Jan 15, 2025

johnstcn left a comment

Choose a reason for hiding this comment

MRColorR commented Jan 16, 2025 • edited Loading

MRColorR commented Jan 16, 2025

johnstcn commented Jan 16, 2025 • edited Loading

MRColorR commented Jan 16, 2025

MRColorR commented Jan 16, 2025

johnstcn left a comment

Choose a reason for hiding this comment

johnstcn commented Jan 17, 2025

ericpaulsen commented Jan 20, 2025 • edited Loading

github-actions bot commented Jan 3, 2025 •

edited

Loading

MRColorR commented Jan 16, 2025 •

edited

Loading

johnstcn commented Jan 16, 2025 •

edited

Loading

ericpaulsen commented Jan 20, 2025 •

edited

Loading