Skip to content

[feat] enforce DNS1035 validations on RayCluster, RayService, and RayJob names #3239

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

Merged
merged 1 commit into from
Apr 3, 2025
Merged

[feat] enforce DNS1035 validations on RayCluster, RayService, and RayJob names #3239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ray-operator/controllers/ray/utils/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/validation"

rayv1 "github.com/ray-project/kuberay/ray-operator/apis/ray/v1"
"github.com/ray-project/kuberay/ray-operator/pkg/features"
Expand All @@ -24,6 +25,9 @@ func ValidateRayClusterMetadata(metadata metav1.ObjectMeta) error {
if len(metadata.Name) > MaxRayClusterNameLength {
return fmt.Errorf("RayCluster name should be no more than %d characters", MaxRayClusterNameLength)
}
if errs := validation.IsDNS1035Label(metadata.Name); len(errs) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Are we certain this won't break any existing systems using KubeRay that fail this validation? What are possible names previously used that would now fail this validation?

Copy link
Contributor Author

@rueian rueian Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This PR makes any names that start with a number or contain dots or upper letters fail at the beginning.

Also note that #3083 makes RayCluster names longer than 53 characters fail, and RayJob/RayServices names longer than 47 characters fail.

Copy link
Member

Choose a reason for hiding this comment

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

Names starting with dots or upper-case letters is fine because the previous validation didn't allow it:

$ kubectl ray create cluster Sample-cluster
Error: Failed to create Ray cluster with: RayCluster.ray.io "Sample-cluster" is invalid: metadata.name: Invalid value: "Sample-cluster": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

But disallowing RayClusters that start with numbers seems like a breaking change:

$ kubectl ray create cluster 1-sample-cluster
Created Ray Cluster: 1-sample-cluster

Is there a really good reason why we should break support this naming scheme? Could we adapt validation to still allow it?

return fmt.Errorf("RayCluster name should be a valid DNS1035 label: %v", errs)
}
return nil
}

Expand Down Expand Up @@ -109,6 +113,9 @@ func ValidateRayJobMetadata(metadata metav1.ObjectMeta) error {
if len(metadata.Name) > MaxRayJobNameLength {
return fmt.Errorf("RayJob name should be no more than %d characters", MaxRayJobNameLength)
}
if errs := validation.IsDNS1035Label(metadata.Name); len(errs) > 0 {
return fmt.Errorf("RayJob name should be a valid DNS1035 label: %v", errs)
}
return nil
}

Expand Down Expand Up @@ -176,6 +183,9 @@ func ValidateRayServiceMetadata(metadata metav1.ObjectMeta) error {
if len(metadata.Name) > MaxRayServiceNameLength {
return fmt.Errorf("RayService name should be no more than %d characters", MaxRayServiceNameLength)
}
if errs := validation.IsDNS1035Label(metadata.Name); len(errs) > 0 {
return fmt.Errorf("RayService name should be a valid DNS1035 label: %v", errs)
}
return nil
}

Expand Down
22 changes: 20 additions & 2 deletions ray-operator/controllers/ray/utils/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,18 +380,26 @@ func TestValidateRayClusterSpecNames(t *testing.T) {
{
name: "RayCluster name is too long (> MaxRayClusterNameLength characters)",
metadata: metav1.ObjectMeta{
Name: strings.Repeat("A", MaxRayClusterNameLength+1),
Name: strings.Repeat("a", MaxRayClusterNameLength+1),
},
expectError: true,
errorMessage: fmt.Sprintf("RayCluster name should be no more than %d characters", MaxRayClusterNameLength),
},
{
name: "RayCluster name is ok (== MaxRayClusterNameLength)",
metadata: metav1.ObjectMeta{
Name: strings.Repeat("A", MaxRayClusterNameLength),
Name: strings.Repeat("a", MaxRayClusterNameLength),
},
expectError: false,
},
{
name: "RayCluster name is not a DNS1035 label",
metadata: metav1.ObjectMeta{
Name: strings.Repeat("1", MaxRayClusterNameLength),
},
expectError: true,
errorMessage: "RayCluster name should be a valid DNS1035 label: [a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name', or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')]",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down Expand Up @@ -745,6 +753,11 @@ func TestValidateRayJobMetadata(t *testing.T) {
})
require.ErrorContains(t, err, fmt.Sprintf("RayJob name should be no more than %d characters", MaxRayJobNameLength))

err = ValidateRayJobMetadata(metav1.ObjectMeta{
Name: strings.Repeat("1", MaxRayJobNameLength),
})
require.ErrorContains(t, err, "RayJob name should be a valid DNS1035 label: [a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name', or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')]")

err = ValidateRayJobMetadata(metav1.ObjectMeta{
Name: strings.Repeat("j", MaxRayJobNameLength),
})
Expand Down Expand Up @@ -819,6 +832,11 @@ func TestValidateRayServiceMetadata(t *testing.T) {
})
require.ErrorContains(t, err, fmt.Sprintf("RayService name should be no more than %d characters", MaxRayServiceNameLength))

err = ValidateRayServiceMetadata(metav1.ObjectMeta{
Name: strings.Repeat("1", MaxRayServiceNameLength),
})
require.ErrorContains(t, err, "RayService name should be a valid DNS1035 label: [a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name', or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')]")

err = ValidateRayServiceMetadata(metav1.ObjectMeta{
Name: strings.Repeat("j", MaxRayServiceNameLength),
})
Expand Down
Loading