Skip to content

[RayService] Validate ray.io/num-worker-groups annotation to prevent operator panic - #5104

Open
ryanaoleary wants to merge 1 commit into
ray-project:masterfrom
ryanaoleary:fix-rayservice-num-worker-groups-dos
Open

[RayService] Validate ray.io/num-worker-groups annotation to prevent operator panic#5104
ryanaoleary wants to merge 1 commit into
ray-project:masterfrom
ryanaoleary:fix-rayservice-num-worker-groups-dos

Conversation

@ryanaoleary

Copy link
Copy Markdown
Collaborator

Why are these changes needed?

In RayService controller's isClusterSpecHashEqual, the ray.io/num-worker-groups annotation on a RayCluster is parsed via strconv.Atoi without validating that the value is non-negative.

If the annotation contains a negative integer (e.g. "-1"):

  1. goalNumWorkerGroups >= clusterNumWorkerGroups evaluates to true (since len() is non-negative).
  2. Slicing goalClusterSpec.WorkerGroupSpecs[:clusterNumWorkerGroups] with a negative index triggers a runtime panic (slice bounds out of range), crashing the operator during reconciliation.

Additionally:

  • When annotation parsing (strconv.Atoi) or hash generation failed, the function previously returned true, incorrectly treating the specs as matching.
  • Missing cluster hash annotations could lead to false-positive matches ("" == "").

Changes in this PR:

  1. Ensure clusterNumWorkerGroups is parsed successfully and is $\ge 0$ before slicing WorkerGroupSpecs.
  2. Return false if clusterHash is empty, if annotation parsing fails, or if hash generation fails.
  3. Added test cases in TestIsClusterSpecHashEqual covering negative values, invalid non-integer strings, empty annotations, and missing cluster hashes.

Related issue number

Labels

  • If this PR has user-facing changes that require documentation updates at release time, I have added the doc-updates-required label.
  • If this PR contains breaking changes, I have added the breaking-change label.

Checks

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

Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
@machichima

machichima commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

The ray.io/num-worker-groups is set in the rayservice controller, which should always be non-negative. This can only happen if someone manually edits the RayCluster annotations. Did you hit this panic in practice?

rayClusterAnnotations[utils.NumWorkerGroupsKey] = strconv.Itoa(len(rayService.Spec.RayClusterSpec.WorkerGroupSpecs))

@ryanaoleary

Copy link
Copy Markdown
Collaborator Author

The ray.io/num-worker-groups is set in the rayservice controller, which should always be non-negative. This can only happen if someone manually edits the RayCluster annotations. Did you hit this panic in practice?

The ray.io/num-worker-groups is set in the rayservice controller, which should always be non-negative. This can only happen if someone manually edits the RayCluster annotations. Did you hit this panic in practice?

rayClusterAnnotations[utils.NumWorkerGroupsKey] = strconv.Itoa(len(rayService.Spec.RayClusterSpec.WorkerGroupSpecs))

No I didn't hit it in practice, this issue got identified as a possible vulnerability in the Ray operator through vulnerability scanning of our managed component (Ray on GKE which installs the operator). KubeRay does always sets this to a non-negative value under normal operation, but since CR annotations are exposed via the Kubernetes API unexpected modifications of this field could trigger the panic and crash the operator pod. I'd agree the issue isn't a super high priority since it just causes a panic and would be due erroneous user modification of the CR, but I think it's still worth guarding against.

This PR also cleans up the error branches where parse/hash failures previously returned trueas well as missing hash annotations matching ("" == "").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants