Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ericpaulsen merged 8 commits intocoder:mainfromMRColorR:feat/loadbalancer-nodeport
Jan 20, 2025

Conversation

@MRColorR
Copy link
Contributor

Short description:

This pull request introduces support for optionally specifynodePort values when usingLoadBalancer service type in the Coder Helm chart. This enhancement addresses a limitation wherehttpNodePort andhttpsNodePort values were previously ignored forLoadBalancer 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 PRfeat: 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!

@cdr-botcdr-botbot added the communityPull Requests and issues created by the community. labelJan 3, 2025
@github-actions
Copy link

github-actionsbot commentedJan 3, 2025•
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by theCLA Assistant Lite bot.

@MRColorRMRColorR changed the titleFeat: Add support for nodePort specification in LoadBalancer services helm chartfeat(helm): add support for nodePort specification in LoadBalancer services helm chartJan 3, 2025
@MRColorR
Copy link
ContributorAuthor

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

cdrci2 added a commit to coder/cla that referenced this pull requestJan 10, 2025
@MRColorR
Copy link
ContributorAuthor

recheck

@matifali
Copy link
Member

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

MRColorR reacted with thumbs up emoji

Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

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

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

@MRColorR
Copy link
ContributorAuthor

MRColorR commentedJan 16, 2025•
edited
Loading

Hi,@MRColorR, you also need to runmake 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
Copy link
ContributorAuthor

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

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

@johnstcn
Copy link
Member

johnstcn commentedJan 16, 2025•
edited
Loading

could you please elaborate more ? i've see there's an already existingsvc_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:

  • Ifcoder.service.type isLoadBalancer, then templatecoder.service.httpNodePort andcoder.service.httpsNodePort (new behaviour, currently not covered)
  • Ifcoder.service.type isNodePort, then templatecoder.service.httpNodePort andcoder.service.httpsNodePort (existing behaviour, currently not covered)
  • Otherwise, do not templatecoder.service.httpNodePort andcoder.service.httpsNodePort (existing behaviour, covered)

Based on that, I think we need to:

  • Addhelm/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 inhelm/coder/tests/testdata/chart_test.go.
  • Update existinghelm/coder/tests/testdata/*.golden with your current set of changes and review the output ofhelm template.

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

Hope that helps!

MRColorR reacted with thumbs up emoji

@MRColorR
Copy link
ContributorAuthor

could you please elaborate more ? i've see there's an already existingsvc_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:

  • Ifcoder.service.type isLoadBalancer, then templatecoder.service.httpNodePort andcoder.service.httpsNodePort (new behaviour, currently not covered)
  • Ifcoder.service.type isNodePort, then templatecoder.service.httpNodePort andcoder.service.httpsNodePort (existing behaviour, currently not covered)
  • Otherwise, do not templatecoder.service.httpNodePort andcoder.service.httpsNodePort (existing behaviour, covered)

Based on that, I think we need to:

  • Addhelm/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 inhelm/coder/tests/testdata/chart_test.go.
  • Update existinghelm/coder/tests/testdata/*.golden with your current set of changes and review the output ofhelm template.

Tip: you can rungo 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.

johnstcn and matifali reacted with thumbs up emoji

@MRColorR
Copy link
ContributorAuthor

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

Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

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

Manual testing:

Rankubectl apply on following files in a temporary namespace:

  • default_values.golden applied successfully:
NAME                         READY   STATUS    RESTARTS   AGEpod/coder-7cc8f95b66-d2gcs   0/1     Running   0          3sNAME            TYPE           CLUSTER-IP      EXTERNAL-IP    PORT(S)        AGEservice/coder   LoadBalancer   10.101.182.44   192.168.1.34   80:31198/TCP   3sNAME                    READY   UP-TO-DATE   AVAILABLE   AGEdeployment.apps/coder   0/1     1            0           3sNAME                               DESIRED   CURRENT   READY   AGEreplicaset.apps/coder-7cc8f95b66   1         1         0       3s
  • svc_loadbalancer.golden applied successfully:
NAME                         READY   STATUS    RESTARTS   AGEpod/coder-7cc8f95b66-wtw9j   0/1     Running   0          5sNAME            TYPE           CLUSTER-IP       EXTERNAL-IP    PORT(S)        AGEservice/coder   LoadBalancer   10.104.167.126   192.168.1.34   80:30080/TCP   5sNAME                    READY   UP-TO-DATE   AVAILABLE   AGEdeployment.apps/coder   0/1     1            0           5sNAME                               DESIRED   CURRENT   READY   AGEreplicaset.apps/coder-7cc8f95b66   1         1         0       5s
  • svc_loadbalancer_class.golden applied successfully:
NAME                         READY   STATUS    RESTARTS   AGEpod/coder-7cc8f95b66-tq5ht   0/1     Running   0          3sNAME            TYPE           CLUSTER-IP      EXTERNAL-IP   PORT(S)        AGEservice/coder   LoadBalancer   10.99.184.182   <pending>     80:32053/TCP   3sNAME                    READY   UP-TO-DATE   AVAILABLE   AGEdeployment.apps/coder   0/1     1            0           3sNAME                               DESIRED   CURRENT   READY   AGEreplicaset.apps/coder-7cc8f95b66   1         1         0       3s
  • svc_nodeport.golden applied successfully:
NAME                         READY   STATUS    RESTARTS   AGEpod/coder-7cc8f95b66-fnlg6   0/1     Running   0          6sNAME            TYPE       CLUSTER-IP       EXTERNAL-IP   PORT(S)        AGEservice/coder   NodePort   10.111.126.236   <none>        80:30080/TCP   6sNAME                    READY   UP-TO-DATE   AVAILABLE   AGEdeployment.apps/coder   0/1     1            0           6sNAME                               DESIRED   CURRENT   READY   AGEreplicaset.apps/coder-7cc8f95b66   1         1         0       6s

@johnstcn
Copy link
Member

@ericpaulsen OK to merge?

@ericpaulsen
Copy link
Member

ericpaulsen commentedJan 20, 2025•
edited
Loading

@johnstcn LGTM - pressing the big green button.

@ericpaulsenericpaulsen merged commitd8fbbcb intocoder:mainJan 20, 2025
30 checks passed
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJan 20, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

@ericpaulsenericpaulsenericpaulsen approved these changes

Assignees

@MRColorRMRColorR

Labels

communityPull Requests and issues created by the community.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MRColorR@matifali@johnstcn@ericpaulsen
[8]ページ先頭
©2009-2025 Movatter.jp