[RHOAIENG-13566] Configure SSL for the communication between head and worker #314

Jooho · 2024-11-26T15:17:45Z

Description

Between head node and worker node, we should configure certificate for SSL communication.
https://issues.redhat.com/browse/RHOAIENG-13566

How Has This Been Tested?

Setup DataScienceCluster with below onfiguration.

apiVersion: datasciencecluster.opendatahub.io/v1
kind: DataScienceCluster
metadata:
  labels:
    app.kubernetes.io/created-by: rhods-operator
    app.kubernetes.io/instance: default-dsc
    app.kubernetes.io/managed-by: kustomize
    app.kubernetes.io/name: datasciencecluster
    app.kubernetes.io/part-of: rhods-operator
  name: default-dsc
spec:
  components:
    codeflare:
      managementState: Managed
    dashboard:
      managementState: Managed
    datasciencepipelines:
      managementState: Managed
    kserve:
      devFlags:
        manifests:
          - contextDir: config
            sourcePath: overlays/odh
            uri: 'https://github.com/opendatahub-io/kserve/tarball/master'
          - contextDir: config
            sourcePath: ''
            uri: 'https://github.com/jooho/odh-model-controller/tarball/ray_tls_test'
      serving:
        ingressGateway:
          certificate:
            type: OpenshiftDefaultIngress
        managementState: Managed
        name: knative-serving
      managementState: Managed
    kueue:
      managementState: Managed
    modelmeshserving:
      managementState: Removed
    ray:
      managementState: Managed
    trainingoperator:
      managementState: Removed
    trustyai:
      managementState: Removed
    workbenches:
      managementState: Managed

Verify ray-tls-scripts configmap and ray-ca-cert secret are created in the redhat-ods-applications
Create a MultiNode ServingRuntime in a test ns

oc new-project kserve-demo
oc process vllm-multinode-runtime-template -n redhat-ods-applications|oc create -n kserve-demo -f -

Verify ray-tls-scripts configmap and ray-ca-cert secret are created in the kserve-demo
Deploy a model with multi-node feature.
- It should deploy without any errors.

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

Signed-off-by: jooho lee <[email protected]>

israel-hdez

Please, change the target branch to incubating.
The main branch is now a stable/downstream branch.

spolti · 2024-11-27T14:38:26Z

config/overlays/odh/ray_tls_resources.yaml

+data:
+  # output from cat ca.crt | base64
+  ca.crt: |
+    LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUZIekNDQXdlZ0F3SUJBZ0lVSlAwL1FCY0xTMFFFV1ZiRDE4NndyUnZjLzdFd0RRWUpLb1pJaHZjTkFRRUwKQlFBd0hqRWNNQm9HQTFVRUF3d1RjMlZzWmkxemFXZHVaV1F0WTJFdFkyVnlkREFnRncweU5ERXhNak13TXpReApOVEphR0E4eU1USTBNVEF6TURBek5ERTFNbG93SGpFY01Cb0dBMVVFQXd3VGMyVnNaaTF6YVdkdVpXUXRZMkV0ClkyVnlkRENDQWlJd0RRWUpLb1pJaHZjTkFRRUJCUUFEZ2dJUEFEQ0NBZ29DZ2dJQkFML0lpWVdabUc0Q05qOS8KMzV6RUJheU1tL2djeGhJSFArZ1M4S04wdm9YV2krdk1Kd1hEUWR3T0JOdEZaa1l0elpJc29ESHVHenRnN0RWNApyZXZONE5JOHRKTmc2b2Jma0tVcGQ3eHdvNHNIdXMwd0h6NWVwZGt1MDhNVzVzZFZOUFNRMkhpVnhialpXSnBRClpJYWYyQWRKa1psUFdtVDBaS1pPdFFEN2oySTRtM3VCeG1jYzhTNWhiNkpaYW9NbGNVVXhkNDFscG43T21iMGEKVjRBUGZiWS9vYytwZmVDczN1cG5xamxZamVGQjR2RTV4WU1ZV0FNeitJRGh4RTRxRGVSaXNMQnhhN1kvcFRScQo2OWVhVXN6Qjl5eEQ3R0FySTJsSDhyUCtVeGpGYUl1K2tBVjVtbjc4OXdlejh0TDVGNEErWlE5cGM5TVI2UXBuCmRkanlaRXcvcFpkdTcwVmo3WUE1MU91S2owcTF2dGw3d1BPcDBUc3lwUDhadW04ZkZSNG5KbmNPaFhMTWV3bGEKTWxBeFZaUWRiMEF5dEE2TUl0dFdXSjA5L3BEOWZ6SkdjRnZOL245ZzZWZ0o5NjNhbmRoTEwyYlJMc0VKRmxUTQpEdTJIeW1CNkErb0ZlcDdjZXNxOUpJRFhkVmFqR3NxMmgrZVpPNGdxWW5nWGNmQVl5ZUloYzlYNnFoT2QvVmlZCmY2eUZoOTNuUnRYTFFNdUJRN2E1WTFzRVN3RHp3WWJKdEtuK3NrcGg5SEtCMTdVRUVOU3BNNHJSNHdxekRQd3AKSmZZeWt2a2Iyd2w0TkNCb2pjaU9icDYwV2ZDQytRcTFsNEo4VXpSOEpvWmFiQ0IzOWxVcHBKa09qNVFxYnEwMApKaUFzWENQQlp3OCtnQnV0b2JBVUs0RklqMGVQQWdNQkFBR2pVekJSTUIwR0ExVWREZ1FXQkJTcmNlRWNhMjNxCjBUQm14VmtZTWtMQTNSeHpKekFmQmdOVkhTTUVHREFXZ0JTcmNlRWNhMjNxMFRCbXhWa1lNa0xBM1J4ekp6QVAKQmdOVkhSTUJBZjhFQlRBREFRSC9NQTBHQ1NxR1NJYjNEUUVCQ3dVQUE0SUNBUUJseW5IM2doVG9ObDQrTlQ1MgpNTTA1V1A3UCszVXJkQ0tGNEJCa0VzN0VueldSUjZ4bkVoVUY5VWhGZ0ZhTFBiQ1pacnlCS2krT1hrUHUva3JCCk12aE1LVGl0WnNWbVBzRktEWDYyVG9zMEJZV0VzanZ0VDM4WFhSZXA3T3BWR0lPQi85V09YVGl3VkpaT2tSZ2MKbHd3U2U1dnBQQXRpMzhUZ3BhM0FVSk5haG00bDhHNWF5WktRQWFnUDg1NHBFTjhPOW54Nk9odytWN1hzSGlNdQpwUmpvc0VTN0JJY1lXVGJxR05yNFR3eXo1cVUwOE9LOEUySFNFSnE5THA4YzZ6UTZnZzBhV1dLYWJyTUpNeSt2CkpIbjM5TEI0U3dONzJjVXJkRWYvQVVrWktNYTRKVFRjMTJnaGpKN1JvUENYUFdPOUJGZ09aZEdoUlpBYkZRMXgKcnB6b3BLZllkT0hWZ0tncG9MOVJIRm40TzZRaTBjbnBML0NZZEFGd0pXNmZYcGEyekhobEJqWXlWdHk5T1Y4TQppV01IVUNXZnl4anVaSno5NFFWZGxLRGVrY2YzUFJzU0RBRGZ4TXlBYVdJQ1NnYXNHTVNPSnRoRTlGM0JhaXNvCnNYM1NLYzRFSEc4Sk1VM1hoeWYwbkhDY2hQdWVRblU5akFBVVBDMHRrVlZtOWhmMXVkdjlOTUk4bktncWRFMkMKK2ExNnR3RVBpZzhkS1pkaFRMOFdXMGwxS3FRcCs4SnBGdTdNWUM1SGNaa0F0NEE3QXlXOHRsYmlQQ1B4RVYwZwpsYkc0eXFyV2lIOG5rWE9tNFBKb0FEMDhzNjA5Y3lFY2Iyblgvck92KzBSdFdOOWFqZGNxYlM1Z0JJaEhRSVUwCko1N0cyTVFYZ0hHbU9QL1ZZTi8vMTlsaXd3PT0KLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo=


is this only used for tests?

No, it will be used for internal ssl communication for ray. It is just self-signed certificate

config/runtimes/vllm-multinode-template.yaml

terrytangyuan · 2024-11-27T14:35:41Z

controllers/testdata/deploy/vllm-multinode-servingruntime.yaml

+              - -c
+              - |
+                # Check if the registered nodes count matches PIPELINE_PARALLEL_SIZE
+                registered_node_count=$(ray status --address ${HEAD_SVC}.${POD_NAMESPACE}.svc.cluster.local:6379 | grep -c node_)


Uppercase registered_node_count is better

Actually, this is not about this Ray certificate. I used uppercase for environment variables and lowercase for variables in the script. I think this is just to make a little difference, so it shouldn't be a big problem.

controllers/testdata/deploy/vllm-multinode-servingruntime.yaml

terrytangyuan · 2024-11-27T15:34:21Z

config/overlays/odh/ray_tls_resources.yaml

+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: ray-tls-scripts


scripts -> script since there's only one shell script

There are 2 scripts in the configmap so I use plural.

Jooho · 2024-11-28T18:08:09Z

@spolti @terrytangyuan @israel-hdez any more comments?

controllers/kserve_ray_tls_controller.go

spolti · 2024-11-28T19:19:29Z

controllers/kserve_ray_tls_controller.go

+				"app.kubernetes.io/name":       "odh-model-controller",
+				"app.kubernetes.io/component":  "kserve",
+				"app.kubernetes.io/part-of":    "odh-model-serving",
+				"app.kubernetes.io/managed-by": "odh-model-controller",


isn't owner reference needed on the configMaps?

it gives more information so I does not hurt .

what do you mean?

:) It is recommended labels by rhoai team. https://github.com/opendatahub-io/odh-model-controller/pull/221/files#r1655337054
based on this, I just added it but it might not need this for manifests though.
But it is just more information about the object so it should not be an issue.

openshift-ci · 2024-11-28T19:39:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jooho, spolti

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Jooho,spolti]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spolti · 2024-11-28T19:39:21Z

nice work, minor comments.

Signed-off-by: jooho lee <[email protected]>

it is done.

config/runtimes/vllm-multinode-template.yaml

spolti · 2024-12-05T13:37:31Z

controllers/utils/cert.go

+			Labels: map[string]string{
+				"opendatahub.io/managed":       "true",
+				"app.kubernetes.io/name":       "odh-model-controller",
+				"app.kubernetes.io/component":  "kserve",


component: odh-model-controller
part-of: kserve
name: shouldn't it be something related to the secret or something else?

This is util function and it is not only for the ray. So I think we can set the name like self-signed-cert. I will update the others as well.

controllers/utils/cert.go

Signed-off-by: jooho lee <[email protected]>

israel-hdez · 2024-11-29T18:18:41Z

config/runtimes/vllm-multinode-template.yaml

+          - name: gen-tls-script
+            configMap:
+              name: ray-tls-scripts
+              defaultMode: 0777
+              # An array of keys from the ConfigMap to create as files
+              items:
+                - key: gencert_ray.sh
+                  path: gencert_ray.sh


Can the scripts be made part of the runtime image?

Otherwise, the template it is no longer self-contained.

israel-hdez · 2024-12-06T17:18:28Z

config/runtimes/vllm-multinode-template.yaml

+            emptyDir: {}
+          - name: ray-ca-cert
+            secret:
+              secretName: ray-ca-cert


Improvement for later: we should find a way to not mount the root CA certificate. This is for security reasons.

israel-hdez · 2024-12-06T17:36:16Z

controllers/kserve_ray_tls_controller.go

+		}
+	}
+	var servingRuntimeList kservev1alpha1.ServingRuntimeList
+	if err := r.client.List(ctx, &servingRuntimeList); err != nil {


you get from all namespaces?

israel-hdez · 2024-12-06T17:45:33Z

controllers/kserve_ray_tls_controller.go

+			log.Info("Original Ray CA Cert Secret is updated", "name", constants.RayCASecretName, "namespace", req.Namespace)
+			for _, sr := range servingRuntimeList.Items {
+				if isMultiNodeServingRuntime(sr) {
+					if err := r.cleanupRayResourcesByKind(ctx, log, sr.Namespace, "Secret"); err != nil {


do you expect only one multi-node SR per namespace?
The dashboard creates a copy per InferenceService, so it is proably a good perf improvement to do the cleanup once per namespace, in case servingRuntimeList contains runtimes from all namespaces.

This comment also applies for the ConfigMap cleanup.

israel-hdez · 2024-12-06T17:52:13Z

controllers/kserve_ray_tls_controller.go

+				}
+			}
+		}
+		if err := r.reconcileRayCACertSecret(ctx, log, srcSecret, controllerNs, req.Namespace, noMultiNodeSrExistInNs); err != nil {


So, if servingRuntimeList holds runtimes from all namespaces.... Looks like the clean-up is ran over all namespaces, but the creation of the Secret (and ConfigMap) is ran only on the target namespace?

Just trying to understand...

israel-hdez · 2024-12-06T17:53:45Z

controllers/kserve_ray_tls_controller.go

+		if err != nil {
+			return ctrl.Result{}, err
+		}
+		err = r.reconcileRayCACertSecret(ctx, log, srcSecret, controllerNs, req.Namespace, !existMultiNodeServingRuntimeInNs(req.Namespace, servingRuntimeList))


Nit:

Suggested change

err = r.reconcileRayCACertSecret(ctx, log, srcSecret, controllerNs, req.Namespace, !existMultiNodeServingRuntimeInNs(req.Namespace, servingRuntimeList))

err = r.reconcileRayCACertSecret(ctx, log, srcSecret, controllerNs, req.Namespace, noMultiNodeSrExistInNs)

israel-hdez · 2024-12-06T18:02:57Z

controllers/kserve_ray_tls_controller.go

+// TO-DO upstream Kserve 0.15 will have a new API WorkerSpec
+// So for now, it will check servingRuntime name, but after we move to 0.15, it needs to check workerSpec is specified or not.(RHOAIENG-16147)
+func isMultiNodeServingRuntime(servingRuntime kservev1alpha1.ServingRuntime) bool {
+	return servingRuntime.Name == "vllm-multinode-runtime"


Perhaps, slightly better to check for the presence of the multi-node fields (was it the workerSpec field?), like in KServe to keep it generic.

israel-hdez · 2024-12-06T18:23:26Z

controllers/kserve_ray_tls_controller_test.go

+			Expect(cli.Update(ctx, rayTlsScriptsUpdatedConfigMap)).Should(Succeed())
+
+			// Check if 'ray-tls-script' ConfigMap is rollback
+			originalRayTlsScriptsConfigMap, err := waitForConfigMap(cli, testNs, constants.RayTlsScriptConfigMapName, 30, 1*time.Second)


Suggested change

originalRayTlsScriptsConfigMap, err := waitForConfigMap(cli, testNs, constants.RayTlsScriptConfigMapName, 30, 1*time.Second)

originalRayTlsScriptsConfigMap, err := waitForConfigMap(cli, WorkingNamespace, constants.RayTlsScriptConfigMapName, 30, 1*time.Second)

israel-hdez · 2024-12-06T18:25:02Z

controllers/kserve_ray_tls_controller_test.go

+				return compareConfigMap(originalRayTlsScriptsConfigMap, updatedConfigMapFromTestNs)
+			}, timeout, interval).Should(BeTrue())
+		})
+		It("should 'ray-tls-script' ConfigMap in the testNs when original one in the ctrlNs updated", func() {


Suggested change

It("should 'ray-tls-script' ConfigMap in the testNs when original one in the ctrlNs updated", func() {

It("should update 'ray-tls-script' ConfigMap in the testNs when original one in the ctrlNs updated", func() {

israel-hdez · 2024-12-06T18:26:25Z

controllers/kserve_ray_tls_controller_test.go

+			// Check if all ray tls script are NOT removed
+			_, err = waitForConfigMap(cli, testNs, constants.RayTlsScriptConfigMapName, 30, 1*time.Second)
+			Expect(err).NotTo(HaveOccurred())
+			_, err = waitForSecret(cli, testNs, constants.RayCASecretName, 30, 1*time.Second)
+			Expect(err).NotTo(HaveOccurred())


It is better to surround these inside Consistently to make sure the conditions remain true over a timespan.

Jooho added 6 commits November 14, 2024 21:05

add ray tls feature

70ad46b

Signed-off-by: jooho lee <[email protected]>

add copy logic for ray resource to targeNs

bfeb9fb

Signed-off-by: jooho lee <[email protected]>

add unit test for ray tls

8c974e3

Signed-off-by: jooho lee <[email protected]>

enhance logic

d3e769d

Signed-off-by: jooho lee <[email protected]>

add more unit tests

63c9e30

Signed-off-by: jooho lee <[email protected]>

add more unit tests and cleanup

d2085fb

Signed-off-by: jooho lee <[email protected]>

openshift-ci bot requested review from hdefazio and israel-hdez November 26, 2024 15:17

openshift-ci bot added the approved label Nov 26, 2024

Jooho requested review from mwaykole and rnetser November 26, 2024 15:56

israel-hdez previously requested changes Nov 26, 2024

View reviewed changes

Jooho changed the base branch from main to incubating November 26, 2024 17:32

spolti requested a review from israel-hdez November 27, 2024 14:36

spolti reviewed Nov 27, 2024

View reviewed changes

config/runtimes/vllm-multinode-template.yaml Show resolved Hide resolved

terrytangyuan reviewed Nov 27, 2024

View reviewed changes

Merge branch 'incubating' into ray_tls

5024ef2

spolti reviewed Nov 28, 2024

View reviewed changes

controllers/kserve_ray_tls_controller.go Show resolved Hide resolved

spolti reviewed Nov 28, 2024

View reviewed changes

spolti approved these changes Nov 28, 2024

View reviewed changes

remove public ca and generate ca by operator

797907d

Signed-off-by: jooho lee <[email protected]>

Jooho force-pushed the ray_tls branch from f74bcf0 to 797907d Compare December 2, 2024 20:38

spolti reviewed Dec 5, 2024

View reviewed changes

config/runtimes/vllm-multinode-template.yaml Show resolved Hide resolved

spolti reviewed Dec 5, 2024

View reviewed changes

controllers/utils/cert.go Show resolved Hide resolved

spolti reviewed Dec 5, 2024

View reviewed changes

controllers/utils/cert.go Show resolved Hide resolved

spolti reviewed Dec 5, 2024

View reviewed changes

controllers/utils/cert.go Show resolved Hide resolved

update label for cert

64fe75b

Signed-off-by: jooho lee <[email protected]>

israel-hdez reviewed Dec 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHOAIENG-13566] Configure SSL for the communication between head and worker #314

[RHOAIENG-13566] Configure SSL for the communication between head and worker #314

Jooho commented Nov 26, 2024

israel-hdez left a comment

spolti Nov 27, 2024

Jooho Nov 27, 2024 •

edited

Loading

terrytangyuan Nov 27, 2024

Jooho Nov 27, 2024

terrytangyuan Nov 27, 2024

Jooho Nov 27, 2024

Jooho commented Nov 28, 2024

spolti Nov 28, 2024

Jooho Nov 28, 2024

spolti Nov 28, 2024

Jooho Nov 29, 2024

openshift-ci bot commented Nov 28, 2024

spolti commented Nov 28, 2024

spolti Dec 5, 2024

Jooho Dec 5, 2024

israel-hdez Nov 29, 2024

israel-hdez Dec 6, 2024

israel-hdez Dec 6, 2024

israel-hdez Dec 6, 2024

israel-hdez Dec 6, 2024

israel-hdez Dec 6, 2024

israel-hdez Dec 6, 2024

israel-hdez Dec 6, 2024

israel-hdez Dec 6, 2024

israel-hdez Dec 6, 2024

	err = r.reconcileRayCACertSecret(ctx, log, srcSecret, controllerNs, req.Namespace, !existMultiNodeServingRuntimeInNs(req.Namespace, servingRuntimeList))
	err = r.reconcileRayCACertSecret(ctx, log, srcSecret, controllerNs, req.Namespace, noMultiNodeSrExistInNs)

	originalRayTlsScriptsConfigMap, err := waitForConfigMap(cli, testNs, constants.RayTlsScriptConfigMapName, 30, 1*time.Second)
	originalRayTlsScriptsConfigMap, err := waitForConfigMap(cli, WorkingNamespace, constants.RayTlsScriptConfigMapName, 30, 1*time.Second)

	It("should 'ray-tls-script' ConfigMap in the testNs when original one in the ctrlNs updated", func() {
	It("should update 'ray-tls-script' ConfigMap in the testNs when original one in the ctrlNs updated", func() {

[RHOAIENG-13566] Configure SSL for the communication between head and worker #314

Are you sure you want to change the base?

[RHOAIENG-13566] Configure SSL for the communication between head and worker #314

Conversation

Jooho commented Nov 26, 2024

Description

How Has This Been Tested?

Merge criteria:

israel-hdez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jooho Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jooho commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 28, 2024

spolti commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jooho Nov 27, 2024 •

edited

Loading