Skip to content

[apiserver] Use ClusterIP instead of NodePort for KubeRay API server service #3708

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 13 commits into from
Jun 1, 2025
Merged

[apiserver] Use ClusterIP instead of NodePort for KubeRay API server service #3708

merged 13 commits into from
Jun 1, 2025

Conversation

machichima
Copy link
Contributor

  • Use ClusterIP in APIServer helm chart
  • Update installation guide to do port-forwarding when using helm chart

Why are these changes needed?

We should not use NodePort for the KubeRay API server by default, as it is not suitable for production.

Related issue number

Closes #3705

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

@machichima
Copy link
Contributor Author

Hi @kevin85421 , PTAL

I use ClusterIP instead in helm chart and update APIServer V1 docs accordingly.

Once this is merge I will then update the V2 docs in PR #3594 (comment)

@kevin85421
Copy link
Member

@rueian would you mind reviewing this PR?

@MortalHappiness MortalHappiness self-assigned this May 29, 2025
MortalHappiness
MortalHappiness reviewed May 29, 2025
View reviewed changes
helm-chart/kuberay-apiserver/values.yaml Outdated
@@ -62,27 +62,27 @@ tolerations: []
# Only one service type needs to be picked
service:
# Nodeport service
Copy link
Member

@MortalHappiness MortalHappiness May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nit:

Suggested change
# Nodeport service
# Nodeport service

apiserver/README.md Outdated
make docker-image cluster load-image deploy
```
```sh
kubectl port-forward service/kuberay-apiserver-service 31888:8888 > /dev/null &
Copy link
Member

@MortalHappiness MortalHappiness May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nit:

Suggested change
kubectl port-forward service/kuberay-apiserver-service 31888:8888 > /dev/null &
kubectl port-forward service/kuberay-apiserver-service 31888:8888

@machichima
nit: fix nit
03e0ca6 
Signed-off-by: machichima <[email protected]>
kevin85421
kevin85421 reviewed May 29, 2025
View reviewed changes
helm-chart/kuberay-apiserver/values.yaml Outdated
@@ -61,28 +61,28 @@ tolerations: []

# Only one service type needs to be picked
service:
# Nodeport service
type: NodePort
# Nodeport service
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason not to remove this? If there’s no specific reason, we can go ahead and remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section keeps the two service type settings with the one unused commented out originally. I simply follow the same format.

I think we can safely remove this. Will remove this for now

apiserver/Autoscaling.md Outdated
@@ -6,7 +6,8 @@ run an example.

## Setup

Refer to the [README](README.md) for setting up the KubeRay operator and APIServer.
Refer to the [Install with Helm](README.md#install-with-helm) section in the README for
setting up the KubeRay operator and APIServer.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
setting up the KubeRay operator and APIServer.
setting up the KubeRay operator and APIServer, and port-forward the HTTP endpoint to local port 31888.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix! Thanks!

@machichima
Copy link
Contributor Author

I've also update the V1 docs here to make them easier to follow

@machichima machichima mentioned this pull request May 30, 2025
Merged
4 tasks
rueian
rueian reviewed May 30, 2025
View reviewed changes
apiserver/README.md Outdated
> [!IMPORTANT]
> If you receive an "Unauthorized" error when making a request, please add an
> authorization header to the request: `-H 'Authorization: 12345'` or install the
> APIServer without a security proxy.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we simply attach -H 'Authorization: 12345' in every curl example? Or could we make the guide in the helm-chart/kuberay-apiserver/README.md to be no security proxy by default?

Copy link
Contributor Author

@machichima machichima May 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer making no security proxy by default, just wondering if it is ok to do so? (e.g. with security proxy is actually recommended?)

I will add this to installation section in README:

Refer to this document to install the latest
stable KubeRay apiserver from the Helm repository "without security proxy".

Important

If you install APIServer with security proxy, you may receive an "Unauthorized" error
when making a request. Please add an authorization header to the request: -H 'Authorization: 12345'
or install the APIServer without a security proxy.

@machichima machichima requested review from rueian and kevin85421 May 31, 2025 01:54
@kevin85421
Copy link
Member

@rueian would you mind reviewing another iteration? Thanks!

rueian
rueian approved these changes May 31, 2025
View reviewed changes
Copy link
Contributor

@rueian rueian left a comment

Choose a reason for hiding this comment

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

LGTM

kevin85421
kevin85421 reviewed May 31, 2025
View reviewed changes
Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

should we also update services in https://github.com/ray-project/kuberay/tree/master/apiserver/deploy/base?

@machichima
Copy link
Contributor Author

machichima commented May 31, 2025 •
edited
Loading

should we also update services in https://github.com/ray-project/kuberay/tree/master/apiserver/deploy/base?

I think services in apiserver/deploy/base are only used for local development (like using make start-local-apiserver)? In this case I think we can keep it as usual

@kevin85421
Copy link
Member

I don’t think it’s only for development. Also, I still don’t think using NodePort for development is a good idea. I will open a follow up PR directly to unblock the branch cut.

kevin85421
kevin85421 approved these changes Jun 1, 2025
View reviewed changes
Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

I didn't review the doc changes. I will go through all related docs and open follow up PRs to update if needed.

@kevin85421 kevin85421 merged commit 23d0195 into ray-project:master Jun 1, 2025
24 of 25 checks passed
pawelpaszki pushed a commit to opendatahub-io/kuberay that referenced this pull request Jun 10, 2025
@machichima @pawelpaszki
[apiserver] Use ClusterIP instead of NodePort for KubeRay API server …
6646db8 
…service (ray-project#3708)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MortalHappiness MortalHappiness MortalHappiness left review comments

@rueian rueian rueian approved these changes

@kevin85421 kevin85421 kevin85421 approved these changes

Assignees

@rueian rueian

@dentiny dentiny

@MortalHappiness MortalHappiness

Labels
1.4.0 branch-cut-blocker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[apiserver] Use ClusterIP instead of NodePort for KubeRay API server service
5 participants
@machichima @kevin85421 @rueian @MortalHappiness @dentiny