-
Notifications
You must be signed in to change notification settings - Fork 364
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Bug][breaking change] Unauthorized 401 error on fetching Ray Custom Resources from K8s API server #1128
Conversation
cc @anshulomar @msumitjain would you mind taking a look at this PR? You can check the section "3. Root cause of this issue" in the PR description for the root cause. Thanks! |
cc @davidxia would you mind providing some feedback on this PR? Thank you! |
LGTM! Thank you for the detailed explanation, I learned a lot! leave notes for reference:
|
I can try to make some time. Do you have a deadline? |
I am not sure why the 401 error occurs on the worker nodes. I guess this is a typo based on the following reasons:
This explanation is not entirely accurate. The root cause of the issue is that the new RayCluster's head Pod is using the ServiceAccount from the old RayCluster's head Pod. When the old RayCluster is deleted, its ServiceAccount is also deleted because they have an owner/dependent relationship. As a result, the new RayCluster ends up using the token of the deleted ServiceAccount to communicate with the Kubernetes API Server, leading to the 401 error. |
Thanks! No rush. I can also ping some folks to take a look too. This issue is blocking a user, so it would be ideal to have it merged by next Monday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevin85421 Thanks for taking this up! Your explanation is very thorough and meticulous.
if instance.Spec.HeadGroupSpec.Template.Spec.ServiceAccountName == namespacedName.Name { | ||
r.Log.Error(err, fmt.Sprintf( | ||
"If users specify ServiceAccountName for the head Pod, they need to create a ServiceAccount themselves."+ | ||
"However, ServiceAccount %s is not found. Please create one.", namespacedName.Name), "ServiceAccount", namespacedName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevin85421 just curious, where are we checking whether the serviceaccount exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, we should make the call-to-action ("Please create one") in the error message as foolproof as possible so that users with varying levels of expertise can unblock themselves. Is there a link to a doc we can include, or a very brief example of how to create a service account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated 5a9e3e2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
@kevin85421 as @Yicheng-Lu-llll explains, what is happening is that when a new cluster is being created, it will check whether the service account exists, which it does. So service account will not be created and the existing one is going to be used. Now when the old cluster is being deleted, it will delete this service account which will lead to a 401 error. Note that exactly the same error will occur if Role and RoleBinding are deleted by the initial cluster. |
@kevin85421 there is also additional inconsistency in the current implementation. If ServiceAccount is specified in the head node, depending on whether autoscaling is defined, the ServiceAccount will be created (autoscaler is True) or not (autoscaler is False). So my suggestion is to make sure that ServiceAccount, defined in either the head node or some of the worker groups, exists. |
Hi @blublinsky, thank you for sharing your thoughts!
It is true, but the names of the [Example] Assume: # T=0: (Only old RayCluster):
ServiceAccount1 (`my-sa`) <-- RoleBinding1 (`rayservice-sample-raycluster-4p798`) --> Role1 (`rayservice-sample-raycluster-4p798`)
# T=1: (Old RayCluster + new RayCluster)
`my-sa` <-- RoleBinding1 --> Role1
^
|
RoleBinding2 (`rayservice-sample-raycluster-ab123`)
|
v
Role2 (`rayservice-sample-raycluster-ab123`)
# T=2: The old RayCluster will be deleted after Serve deployments on the new RayCluster are ready and healthy.
# That is, RoleBinding1 and Role1 will be deleted.
`my-sa`
^
|
RoleBinding2 (`rayservice-sample-raycluster-ab123`)
|
v
Role2 (`rayservice-sample-raycluster-ab123`) The ServiceAccount will still have enough permissions after
[1] It would be beneficial if it could be reused by other custom resources managed by KubeRay. However, I am concerned about the possibility of other applications also attaching to this ServiceAccount. Typically, it is highly likely to happen because users often use simple names, such as
Agree. The bugs caused by [1] will be very difficult for maintainers to debug. Unlike an in-house product, KubeRay maintainers do not have access to users' Kubernetes clusters. In the case of an in-house product, I completely agree with your point because I have access to all Kubernetes logs, allowing me to focus more on user experience (e.g. self-contained [2]). However, for open-source projects, it is better to surface the issue as early as possible. It is not that painful for users to create the ServiceAccount by themselves. |
Hi @blublinsky, I will merge this PR tonight because some users are blocked by it. If necessary, we can open another issue to discuss it further. Here is a possible solution for achieving a balance between user experience and "unknown unknowns": (1) By default, we won't create a ServiceAccount for users if they specify a serviceAccountName. Is it OK for you? |
nit, We can have this breaking change in the document: kuberay/docs/guidance/autoscaler.md Line 78 in 4b6f1df
|
Added in #1071. |
Hi @blublinsky, I will merge this PR. If necessary, we can discuss the points mentioned in #1128 (comment) in a separate issue. |
…Resources from K8s API server (ray-project#1128) Unauthorized 401 error on fetching Ray Custom Resources from K8s API server
Why are these changes needed?
In #1123, the autoscaler container fails to GET the RayCluster from the Kubernetes API Server and receives error code 401 after a zero-downtime upgrade. If you are interested in the root cause, you can read the "debug process" section.
We have two solutions for this issue:
Solution 1: Still create a ServiceAccount for users if the specified
ServiceAccountName
does not exist. The only difference is that we do not set the ControllerReference for the ServiceAccount to avoid it being deleted by the Kubernetes garbage collector. (Step 5 in the section "3. Root cause of this issue")Solution 2 (this PR): If users specify ServiceAccountName, they need to create the ServiceAccount by themselves.
ServiceAccountName
without creating a ServiceAccount manually.Debug process (optional)
1. Understand ServiceAccount
In the head Pod, we can get three files,
ca.crt
,namespace
,token
, related to the ServiceAccount. You can check this article for the descriptions about these 3 files.ls /var/run/secrets/kubernetes.io/serviceaccount/ # example output: ca.crt namespace token
[Experiment for
ca.crt
andtoken
]:You can execute the following commands in the head Pod of an autoscaling RayCluster. The ServiceAccount should have sufficient permissions to perform a
GET
operation on the RayCluster.ca.crt
: Use the anonymous ServiceAccount "system:anonymous" which does not have the permission to GET RayCluster => 403 Forbiddentoken
: Use the ServiceAccount that associated with this Pod which has the permission to GET RayCluster => Succeedca.crt
andtoken
at the same time => Succeed (same as Case 2)The result is same as the result in this article. I am not sure the purpose of ca.crt. It may only be used when some Kubernetes API Server configurations are set.
2. 401 Unauthorized vs 403 Forbidden
Based on this Kubernetes link,
When we did not configure
token
and Kubernetes API Server enables anonymous access (it is enabled by default after Kubernetes 1.6+, and you can check whether it is disabled or not by this link.) => It will use the anonymous ServiceAccount"system:anonymous"
. If the ServiceAccount does not have enough permissions to perform the operation. The error code will be403 Forbidden
.If we specify an invalid token in the request, we will get a 401 Unauthorized error. For example, we can get 401 by changing
to
TL;DR
401 Unauthorized
:token
is invalid.403 Forbidden
: Kubernetes API Server can associate the ServiceAccount, but the account does not enough permission to perform the operation.3. Root cause of this issue
Because we received a 401 error instead of a 403 error, our focus should be on why the
token
is invalid rather than whether the ServiceAccount has sufficient permissions or not.ServiceAccountName
(e.g.,my-sa
) for the head Pod without manually creating the ServiceAccount, KubeRay will create a ServiceAccount for them.my-sa
ServiceAccount as the old RayCluster.my-sa
ServiceAccount have an owner/dependent relationship, themy-sa
ServiceAccount will be deleted when the old RayCluster is deleted.my-sa
(referred to as newmy-sa
below), the new RayCluster has already mounted the oldmy-sa
ServiceAccount in Step 3.401 Unauthorized
error.Related issue number
Closes #1123
Checks