Allow running MPI applications as non-root #383

alculquicondor · 2021-07-21T19:09:40Z

Part of #373

Adds the spec field sshAuthMountPath for MPIJob.
The init script sets the permissions and ownership based on the securityContext of the launcherPod
Adds a pure-MPI example to show how to create and use an image as non-root

Tested: unit and integration suites as well as manual runs.

alculquicondor · 2021-07-21T19:09:49Z

/hold
Does this work?

terrytangyuan · 2021-07-21T19:25:40Z

Yes it should work

kawych

Looks good so far, but it's a fairly large change and it's easy to miss things. Could you confirm that it was tested end-to-end in addition to unit tests?

kawych · 2021-07-22T10:26:45Z

examples/pi/Dockerfile

+RUN useradd -m mpiuser
+WORKDIR /home/mpiuser
+COPY --chown=mpiuser sshd_config .sshd_config
+RUN cat /etc/ssh/ssh_config | grep -Ev 'StrictHostKeyChecking' > /etc/ssh/ssh_config.new && \


nit: Maybe something like this would be more explicit and simpler?

sed -i 's/( )StrictHostKeyChecking ./\1StrictHostKeyChecking no/g' /etc/ssh/ssh_config.new

Simplified it a bit differently, but thanks for the suggestion :)

kawych · 2021-07-22T12:11:06Z

v2/pkg/controller/mpi_job_controller.go

@@ -74,21 +74,19 @@ const (
 	sshAuthVolume           = "ssh-auth"
 	sshAuthMountPath        = "/mnt/ssh"
 	sshHomeVolume           = "ssh-home"
-	// TODO(alculquicondor): Make home directory configurable through the API.


Just a thing to consider: would it make sense to make the whole home directory configurable as opposed to just .ssh directory?

I thought about that. But someone creating a container image might want not to use $HOME/.ssh but a completely different location. And, at the moment, I can't think of other uses of the home folder from the controller perspective.

kawych · 2021-07-22T12:29:30Z

v2/pkg/controller/mpi_job_controller_test.go

+					RestartPolicy: corev1.RestartPolicyNever,
+					Containers: []corev1.Container{
+						{
+							Env: func() []corev1.EnvVar {


Can all of this be replaced by "append(ompiEnvVars, nvidiaDisableEnvVars...)?

I considered it, but not sure if it will have consequences on the allocation of ompiEnvVars

another alternative would be:

append(append([]corev1.EnvVar(nil), ompiEnvVars...), nvidiaDisableEnvVars...)

WDYT?

Yeah, I've just played with append a little and it's a little funny. [1]: "If it has sufficient capacity, the destination is resliced to accommodate the new elements. If it does not, a new underlying array will be allocated."

[1] https://pkg.go.dev/builtin#append

So in this case "append(ompiEnvVars, nvidiaDisableEnvVars...)" will work the same as "append(append(...)", but if you allocate additional memory for the ompiEnvVars slice, it will have a different effect. Examples:

a := []int{1} b := append(a[:1], []int{3}...) a[0] = 2 b[0] = 3 fmt.Printf("%d %d\n", a[0], b[0]) <- this prints "2 3"

a := []int{1, 2} b := append(a[:1], []int{3}...) a[0] = 2 b[0] = 3 fmt.Printf("%d %d\n", a[0], b[0]) <- this prints "3 3"

So append(append([]corev1.EnvVar(nil), ompiEnvVars...), nvidiaDisableEnvVars...) would be better here.

Yeah, that will be more predictable. I decided to add a helper method.

alculquicondor

I added a note in the description on what I did to test the change.

alculquicondor · 2021-07-22T14:54:00Z

v2/pkg/controller/mpi_job_controller.go

@@ -74,21 +74,19 @@ const (
 	sshAuthVolume           = "ssh-auth"
 	sshAuthMountPath        = "/mnt/ssh"
 	sshHomeVolume           = "ssh-home"
-	// TODO(alculquicondor): Make home directory configurable through the API.


I thought about that. But someone creating a container image might want not to use $HOME/.ssh but a completely different location. And, at the moment, I can't think of other uses of the home folder from the controller perspective.

alculquicondor · 2021-07-22T14:56:46Z

v2/pkg/controller/mpi_job_controller_test.go

+					RestartPolicy: corev1.RestartPolicyNever,
+					Containers: []corev1.Container{
+						{
+							Env: func() []corev1.EnvVar {


I considered it, but not sure if it will have consequences on the allocation of ompiEnvVars

another alternative would be:

append(append([]corev1.EnvVar(nil), ompiEnvVars...), nvidiaDisableEnvVars...)

WDYT?

alculquicondor · 2021-07-23T18:13:21Z

/hold cancel

gaocegege · 2021-07-26T03:37:09Z

/assign @terrytangyuan @zw0610 @carmark

zw0610

I believe this pr does more than allowing non-root mode. If so, I would prefer this pr comes with multiple commits. What do you think? @terrytangyuan

zw0610 · 2021-07-26T04:05:02Z

v2/pkg/apis/kubeflow/v2beta1/default.go

@@ -56,6 +56,9 @@ func SetDefaults_MPIJob(mpiJob *MPIJob) {
 	if mpiJob.Spec.SlotsPerWorker == nil {
 		mpiJob.Spec.SlotsPerWorker = newInt32(1)
 	}
+	if mpiJob.Spec.SSHAuthMountPath == "" {
+		mpiJob.Spec.SSHAuthMountPath = "/root/.ssh"


could you make the default path ("/root/.ssh") a constant?

I think that makes sense for controller code, where a value might be used more than once. However, this file is all about defaults. Creating a constant would just make the reader have to jump from one line to another to see what the default for a field is.

zw0610 · 2021-07-26T04:08:39Z

v2/pkg/apis/kubeflow/validation/validation.go

@@ -46,6 +46,9 @@ func validateMPIJobSpec(spec *kubeflow.MPIJobSpec, path *field.Path) field.Error
 	} else if !validCleanPolicies.Has(string(*spec.CleanPodPolicy)) {
 		errs = append(errs, field.NotSupported(path.Child("cleanPodPolicy"), *spec.CleanPodPolicy, validCleanPolicies.List()))
 	}
+	if spec.SSHAuthMountPath == "" {


does this validation contradict with the defaulting here?

I wanted to keep validation independent from defaulting. But yes, if you put them together, this line of code would not be reachable.

zw0610 · 2021-07-26T04:18:41Z

v2/pkg/controller/mpi_job_controller.go

+	}
+	podSpec.InitContainers = append(podSpec.InitContainers, corev1.Container{
+		Name:  "init-ssh",
+		Image: "alpine:3.14",


could we make this image configurable? some cluster environment is isolated from docker registry.

zw0610 · 2021-07-26T04:19:12Z

v2/pkg/controller/mpi_job_controller.go

 				Name:      sshHomeVolume,
-				MountPath: sshHomeMountPath,
+				MountPath: "/mnt/home-ssh",


why do we replace constant with hardcoded string here?

It's a different value now (it was /root/.ssh). But sure, putting it as a constant.

Adds the spec field sshAuthMountPath for MPIJob. The init script sets the permissions and ownership based on the securityContext of the launcherPod

alculquicondor

I put the sample in a separate commit

alculquicondor · 2021-07-26T14:20:48Z

v2/pkg/apis/kubeflow/v2beta1/default.go

@@ -56,6 +56,9 @@ func SetDefaults_MPIJob(mpiJob *MPIJob) {
 	if mpiJob.Spec.SlotsPerWorker == nil {
 		mpiJob.Spec.SlotsPerWorker = newInt32(1)
 	}
+	if mpiJob.Spec.SSHAuthMountPath == "" {
+		mpiJob.Spec.SSHAuthMountPath = "/root/.ssh"


I think that makes sense for controller code, where a value might be used more than once. However, this file is all about defaults. Creating a constant would just make the reader have to jump from one line to another to see what the default for a field is.

alculquicondor · 2021-07-26T14:21:54Z

v2/pkg/apis/kubeflow/validation/validation.go

@@ -46,6 +46,9 @@ func validateMPIJobSpec(spec *kubeflow.MPIJobSpec, path *field.Path) field.Error
 	} else if !validCleanPolicies.Has(string(*spec.CleanPodPolicy)) {
 		errs = append(errs, field.NotSupported(path.Child("cleanPodPolicy"), *spec.CleanPodPolicy, validCleanPolicies.List()))
 	}
+	if spec.SSHAuthMountPath == "" {


I wanted to keep validation independent from defaulting. But yes, if you put them together, this line of code would not be reachable.

alculquicondor · 2021-07-26T14:24:35Z

v2/pkg/controller/mpi_job_controller.go

 				Name:      sshHomeVolume,
-				MountPath: sshHomeMountPath,
+				MountPath: "/mnt/home-ssh",


It's a different value now (it was /root/.ssh). But sure, putting it as a constant.

alculquicondor · 2021-07-26T18:16:50Z

v2/pkg/controller/mpi_job_controller.go

+	}
+	podSpec.InitContainers = append(podSpec.InitContainers, corev1.Container{
+		Name:  "init-ssh",
+		Image: "alpine:3.14",


zw0610 · 2021-07-27T05:21:49Z

/lgtm

terrytangyuan

Thanks!

/lgtm
/approve

google-oss-robot · 2021-07-27T05:34:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kawych, terrytangyuan

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~~ [terrytangyuan]

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

google-oss-robot requested review from carmark and gaocegege July 21, 2021 19:09

google-oss-robot added do-not-merge/hold size/XXL labels Jul 21, 2021

alculquicondor force-pushed the run-as-user branch from 974007f to fb30471 Compare July 21, 2021 19:20

alculquicondor mentioned this pull request Jul 21, 2021

Implement v2 controller that sets up SSH for communication #373

Open

16 tasks

kawych reviewed Jul 22, 2021

View reviewed changes

alculquicondor force-pushed the run-as-user branch from fb30471 to 89ae0e1 Compare July 22, 2021 14:57

alculquicondor commented Jul 22, 2021

View reviewed changes

alculquicondor force-pushed the run-as-user branch from 89ae0e1 to 1d6ec25 Compare July 22, 2021 21:13

kawych approved these changes Jul 23, 2021

View reviewed changes

alculquicondor force-pushed the run-as-user branch 2 times, most recently from 66a5d91 to 1f59983 Compare July 23, 2021 18:12

google-oss-robot added size/XL and removed size/XXL labels Jul 23, 2021

google-oss-robot removed the do-not-merge/hold label Jul 23, 2021

google-oss-robot assigned carmark, terrytangyuan and zw0610 Jul 26, 2021

zw0610 reviewed Jul 26, 2021

View reviewed changes

alculquicondor added 2 commits July 26, 2021 14:16

Allow running MPI applications as non-root

f692d41

Adds the spec field sshAuthMountPath for MPIJob. The init script sets the permissions and ownership based on the securityContext of the launcherPod

Add pure MPI sample that run as non-root

15a6e78

alculquicondor force-pushed the run-as-user branch from 1f59983 to 15a6e78 Compare July 26, 2021 18:17

alculquicondor commented Jul 26, 2021

View reviewed changes

google-oss-robot added the lgtm label Jul 27, 2021

terrytangyuan approved these changes Jul 27, 2021

View reviewed changes

google-oss-robot added the approved label Jul 27, 2021

google-oss-robot merged commit 9ce6467 into kubeflow:master Jul 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow running MPI applications as non-root #383

Allow running MPI applications as non-root #383

alculquicondor commented Jul 21, 2021 •

edited

Loading

alculquicondor commented Jul 21, 2021

terrytangyuan commented Jul 21, 2021

kawych left a comment

kawych Jul 22, 2021

alculquicondor Jul 22, 2021

kawych Jul 22, 2021

alculquicondor Jul 22, 2021 •

edited

Loading

kawych Jul 22, 2021

alculquicondor Jul 22, 2021

kawych Jul 23, 2021

alculquicondor Jul 23, 2021

alculquicondor left a comment

alculquicondor Jul 22, 2021 •

edited

Loading

alculquicondor Jul 22, 2021

alculquicondor commented Jul 23, 2021

gaocegege commented Jul 26, 2021

zw0610 left a comment

zw0610 Jul 26, 2021

alculquicondor Jul 26, 2021

zw0610 Jul 26, 2021

alculquicondor Jul 26, 2021

zw0610 Jul 26, 2021

alculquicondor Jul 26, 2021

zw0610 Jul 26, 2021

alculquicondor Jul 26, 2021

alculquicondor left a comment

alculquicondor Jul 26, 2021

alculquicondor Jul 26, 2021

alculquicondor Jul 26, 2021

alculquicondor Jul 26, 2021

zw0610 commented Jul 27, 2021

terrytangyuan left a comment

google-oss-robot commented Jul 27, 2021

Allow running MPI applications as non-root #383

Allow running MPI applications as non-root #383

Conversation

alculquicondor commented Jul 21, 2021 • edited Loading

alculquicondor commented Jul 21, 2021

terrytangyuan commented Jul 21, 2021

kawych left a 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

alculquicondor Jul 22, 2021 • 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

alculquicondor left a comment

Choose a reason for hiding this comment

alculquicondor Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Jul 23, 2021

gaocegege commented Jul 26, 2021

zw0610 left a 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

alculquicondor left a 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

zw0610 commented Jul 27, 2021

terrytangyuan left a comment

Choose a reason for hiding this comment

google-oss-robot commented Jul 27, 2021

alculquicondor commented Jul 21, 2021 •

edited

Loading

alculquicondor Jul 22, 2021 •

edited

Loading

alculquicondor Jul 22, 2021 •

edited

Loading