[kuberay] Fix inconsistent RBAC truncation for autoscaling clusters. (r…

…ay-project#689) Due to inconsistent truncation of RBAC names, it's not possible to deploy an autoscaling RayService with a long name. This PR fixes that issue. Closes ray-project#643. Signed-off-by: Dmitri Gekhtman <[email protected]>
lowang-bh · Nov 5, 2022 · 353df33 · 353df33
1 parent ff7b8bb
commit 353df33
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 12 deletions.
diff --git a/.github/workflows/test-job.yaml b/.github/workflows/test-job.yaml
@@ -41,7 +41,7 @@ jobs:
       uses: golangci/golangci-lint-action@v2
       with:
           # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version
-          version: v1.29
+          version: v1.50.1
 
           # Optional: working directory, useful for monorepos
           working-directory: ./ray-operator

diff --git a/ray-operator/controllers/ray/common/pod.go b/ray-operator/controllers/ray/common/pod.go
@@ -109,7 +109,8 @@ func DefaultHeadPodTemplate(instance rayiov1alpha1.RayCluster, headSpec rayiov1a
 	if instance.Spec.EnableInTreeAutoscaling != nil && *instance.Spec.EnableInTreeAutoscaling {
 		headSpec.RayStartParams["no-monitor"] = "true"
 		// set custom service account with proper roles bound.
-		podTemplate.Spec.ServiceAccountName = utils.GetHeadGroupServiceAccountName(&instance)
+		// utils.CheckName clips the name to match the behavior of reconcileAutoscalerServiceAccount
+		podTemplate.Spec.ServiceAccountName = utils.CheckName(utils.GetHeadGroupServiceAccountName(&instance))
 
 		rayContainerIndex := getRayContainerIndex(podTemplate.Spec)
 		rayHeadImage := podTemplate.Spec.Containers[rayContainerIndex].Image

diff --git a/ray-operator/controllers/ray/common/pod_test.go b/ray-operator/controllers/ray/common/pod_test.go
@@ -523,6 +523,15 @@ func TestHeadPodTemplate_WithAutoscalingEnabled(t *testing.T) {
 	if !reflect.DeepEqual(expectedResult, actualResult) {
 		t.Fatalf("Expected `%v` but got `%v`", expectedResult, actualResult)
 	}
+
+	// Repeat ServiceAccountName check with long cluster name.
+	cluster.Name = longString(t) // 200 chars long
+	podTemplateSpec = DefaultHeadPodTemplate(*cluster, cluster.Spec.HeadGroupSpec, podName, svcName, "6379")
+	actualResult = podTemplateSpec.Spec.ServiceAccountName
+	expectedResult = shortString(t) // 50 chars long, truncated by utils.CheckName
+	if !reflect.DeepEqual(expectedResult, actualResult) {
+		t.Fatalf("Expected `%v` but got `%v`", expectedResult, actualResult)
+	}
 }
 
 // If no service account is specified in the RayCluster,

diff --git a/ray-operator/controllers/ray/common/rbac.go b/ray-operator/controllers/ray/common/rbac.go
@@ -56,6 +56,7 @@ func BuildRole(cluster *v1alpha1.RayCluster) (*rbacv1.Role, error) {
 
 // BuildRole
 func BuildRoleBinding(cluster *v1alpha1.RayCluster) (*rbacv1.RoleBinding, error) {
+	serviceAccountName := utils.GetHeadGroupServiceAccountName(cluster)
 	rb := &rbacv1.RoleBinding{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      cluster.Name,
@@ -68,15 +69,17 @@ func BuildRoleBinding(cluster *v1alpha1.RayCluster) (*rbacv1.RoleBinding, error)
 		},
 		Subjects: []rbacv1.Subject{
 			{
-				Kind:      rbacv1.ServiceAccountKind,
-				Name:      utils.GetHeadGroupServiceAccountName(cluster),
+				Kind: rbacv1.ServiceAccountKind,
+				// Clip name for consistency with the function reconcileAutoscalerServiceAccount.
+				Name:      utils.CheckName(serviceAccountName),
 				Namespace: cluster.Namespace,
 			},
 		},
 		RoleRef: rbacv1.RoleRef{
 			APIGroup: rbacv1.GroupName,
 			Kind:     "Role",
-			Name:     cluster.Name,
+			// Clip name for consistency with the function reconcileAutoscalerRole.
+			Name: utils.CheckName(cluster.Name),
 		},
 	}
 

diff --git a/ray-operator/controllers/ray/common/rbac_test.go b/ray-operator/controllers/ray/common/rbac_test.go
@@ -1,6 +1,7 @@
 package common
 
 import (
+	"reflect"
 	"testing"
 
 	rayiov1alpha1 "github.com/ray-project/kuberay/ray-operator/apis/ray/v1alpha1"
@@ -11,10 +12,11 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func TestBuildRoleBindingSubjectName(t *testing.T) {
+// Test subject and role ref names in the function BuildRoleBinding.
+func TestBuildRoleBindingSubjectAndRoleRefName(t *testing.T) {
 	tests := map[string]struct {
 		input *rayiov1alpha1.RayCluster
-		want  string
+		want  []string
 	}{
 		"Ray cluster with head group service account": {
 			input: &rayiov1alpha1.RayCluster{
@@ -32,7 +34,7 @@ func TestBuildRoleBindingSubjectName(t *testing.T) {
 					},
 				},
 			},
-			want: "my-service-account",
+			want: []string{"my-service-account", "raycluster-sample"},
 		},
 		"Ray cluster without head group service account": {
 			input: &rayiov1alpha1.RayCluster{
@@ -48,17 +50,36 @@ func TestBuildRoleBindingSubjectName(t *testing.T) {
 					},
 				},
 			},
-			want: "raycluster-sample",
+			want: []string{"raycluster-sample", "raycluster-sample"},
+		},
+		"Ray cluster with a long name and without head group service account": {
+			input: &rayiov1alpha1.RayCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      longString(t), // 200 chars long
+					Namespace: "default",
+				},
+				Spec: rayiov1alpha1.RayClusterSpec{
+					HeadGroupSpec: rayiov1alpha1.HeadGroupSpec{
+						Template: corev1.PodTemplateSpec{
+							Spec: corev1.PodSpec{},
+						},
+					},
+				},
+			},
+			want: []string{
+				shortString(t), // 50 chars long, truncated by utils.CheckName
+				shortString(t),
+			},
 		},
 	}
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
 			rb, err := BuildRoleBinding(tc.input)
 			assert.Nil(t, err)
-			got := rb.Subjects[0].Name
-			if got != tc.want {
-				t.Fatalf("got %s, want %s", got, tc.want)
+			got := []string{rb.Subjects[0].Name, rb.RoleRef.Name}
+			if !reflect.DeepEqual(got, tc.want) {
+				t.Fatalf("got %v, want %v", got, tc.want)
 			}
 		})
 	}

diff --git a/ray-operator/controllers/ray/common/test_utils.go b/ray-operator/controllers/ray/common/test_utils.go
@@ -0,0 +1,30 @@
+package common
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/ray-project/kuberay/ray-operator/controllers/ray/utils"
+	"github.com/stretchr/testify/assert"
+)
+
+// Generate a string of length 200.
+func longString(t *testing.T) string {
+	var b bytes.Buffer
+	for i := 0; i < 200; i++ {
+		b.WriteString("a")
+	}
+	result := b.String()
+	// Confirm length.
+	assert.Equal(t, len(result), 200)
+	return result
+}
+
+// Clip the above string using utils.CheckName
+// to a string of length 50.
+func shortString(t *testing.T) string {
+	result := utils.CheckName(longString(t))
+	// Confirm length.
+	assert.Equal(t, len(result), 50)
+	return result
+}