fix(v2,sdk.v2): Remove need for base image root access to write artifacts

[judahrand]

Description of your changes:
The changes in this PR are based on @Bobgy's design here.

Custom base images may be built specifically to avoid granting root privileges. Without these privileges the local mount points cannot be created if they live at the root of the filesystem. /gcs must remain where it is for now as it is currently a contract for KFP v2 python component wrappers. Therefore, this directory is also mounted as an emptyDir so that it already exists and is accessible to all users.

Checklist:

• The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

[google-oss-robot]

Hi @judahrand. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[chensun]

This needs to be /gcs/ for GCS Fuse support on Vertex Pipelines.
The content you write to the local path /gcs/my_project/bucket/blob would be automatically synced to gs://my_project/bucket/blob.

[judahrand]

Do you have an alternate solution for being able to execute in non-root images? Or is this something for which additional compatibility is needed elsewhere.

Looking at the gcsfuse docs it looks like the mount path is arbitrary.

[chensun]

I think it is an implementation decision by Vertex Pipelines backend. Following that, we probably have multiple places in code where we rely on this conversation between local path and gs uri.
Another example:

pipelines/v2/component/launcher.go

Line 655 in 3a621fb

return "/gcs/" + strings.TrimPrefix(uri, "gs://"), nil

[judahrand]

This PR does also change those.

But you're probably right that it's actually probably a problem closer to how Vertex uses gcsfuse and the file permissions which are set there in my case.

However, do you not think that trying to create these directories on at the root of the filesystem is an unnecessary hurdle in the way of non-root containers?

[judahrand]

I've opened a similar issue with Vertex Pipelines:
https://issuetracker.google.com/issues/199297769

[chensun]

I've opened a similar issue with Vertex Pipelines:
https://issuetracker.google.com/issues/199297769

Thank you for opening the issue. I'll pass it along to the Vertex Pipelines team.

Meanwhile, I am curious is there any specific reason you want to avoid running container as root user? Or it is an image out of your control?

[judahrand]

I've opened a similar issue with Vertex Pipelines:
https://issuetracker.google.com/issues/199297769

Thank you for opening the issue. I'll pass it along to the Vertex Pipelines team.

Meanwhile, I am curious is there any specific reason you want to avoid running container as root user? Or it is an image out of your control?

It is an image which we also use in our GKE cluster and our policy there is to use non root containers wherever possible. It would be possible to create a different image for Vertex... But that seems unnecessary.

[chensun]

I've opened a similar issue with Vertex Pipelines:
https://issuetracker.google.com/issues/199297769

Thank you for opening the issue. I'll pass it along to the Vertex Pipelines team.

Meanwhile, I am curious is there any specific reason you want to avoid running container as root user? Or it is an image out of your control?

I've opened a similar issue with Vertex Pipelines:
https://issuetracker.google.com/issues/199297769

Thank you for opening the issue. I'll pass it along to the Vertex Pipelines team.
Meanwhile, I am curious is there any specific reason you want to avoid running container as root user? Or it is an image out of your control?

It is an image which we also use in our GKE cluster and our policy there is to use non root containers wherever possible. It would be possible to create a different image for Vertex... But that seems unnecessary.

Got it.
Maybe something related

• Kubernetes is deprecating Docker as a container runtime after v1.20. Part of the reason is for security: https://www.kubeflow.org/docs/components/pipelines/installation/choose-executor/#docker-executor
• And KFP is offering emissary executor, which will become the default in the future: [feature] default to the emissary executor #5718

I wonder if these changes may address your concerns behind the policy of avoiding root containers.

That said, I'll still pass your concern to the team, and see if we could make a change.

[judahrand]

Unfortunately, I don't set the rules 😉

I also suspect that we'll continue to see many K8s installations using the Docker shim for quite a while to come.

I appreciate that there is progress being made on the security side of containerization which may make non-root less important but it still just strikes me as an odd failure mode (especially as it isn't documented).

Thank you very much for passing the issue along - I can't imagine a small file permission tweak is that big a change to enable non-root 😛

[Bobgy]

FYI, there were a similar request raised for v2 compatible mode: #5673

[judahrand]

FYI, there were a similar request raised for v2 compatible mode: #5673

Ah! I missed this - might as well close this in that case.

Or alternatively I can use this PR to have a go at making your suggested changes if you think they are likely to be mergable? Though, I might need some pointers of where to look to implement the changes (eg. where is the container spec defined)

[judahrand]

Here is where some some these changes are needed I think?

pipelines/v2/compiler/container.go

Line 134 in a0b18eb

func containerExecutorTemplate(container *pipelinespec.PipelineDeploymentConfig_PipelineContainerSpec, launcherImage string) *wfapi.Template {

[judahrand]

@Bobgy I've altered this PR to try to tackle #5673. I'd appreciate any feedback - I may well be going in very much the wrong direction?

[Bobgy]

/test kubeflow-pipelines-samples-v2

[judahrand]

Is there anything else here that needs to be done to make this mergeable?

[capri-xiyue]

/lgtm
/approve

[judahrand]

/assign @chensun

[Bobgy]

Thank you @judahrand!
/lgtm

[chensun]

/approve

Thanks!

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: capri-xiyue, chensun

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:

• sdk/OWNERS [chensun]
• v2/OWNERS [capri-xiyue,chensun]

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

[judahrand]

/retest

[jie8357IOII]

Does there have any updates?

[Bobgy]

/assign @chensun @zijianjoy

[dandawg]

This looks to be close to the finish line. What work is still needed to get it across, and do we have a timeline?

[zijianjoy]

Hello @judahrand , would you like to resolve the merge conflict and run test again? This PR has been approved so we don't expect much changes required beyond that.

[google-oss-prow]

@judahrand: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
kubeflow-pipelines-v2-go-test	729c7ce	link	true	/test kubeflow-pipelines-v2-go-test
kubeflow-pipelines-sdk-isort	729c7ce	link	unknown	/test kubeflow-pipelines-sdk-isort
kubeflow-pipelines-sdk-yapf	729c7ce	link	unknown	/test kubeflow-pipelines-sdk-yapf
kubeflow-pipelines-sdk-python310	729c7ce	link	unknown	/test kubeflow-pipelines-sdk-python310
kubeflow-pipelines-sdk-python38	729c7ce	link	unknown	/test kubeflow-pipelines-sdk-python38
kubeflow-pipelines-sdk-python311	729c7ce	link	unknown	/test kubeflow-pipelines-sdk-python311
test-upgrade-kfp-sdk	729c7ce	link	unknown	/test test-upgrade-kfp-sdk
kubeflow-pipelines-component-yaml	729c7ce	link	unknown	/test kubeflow-pipelines-component-yaml
test-run-all-gcpc-modules	729c7ce	link	unknown	/test test-run-all-gcpc-modules
kubeflow-pipelines-sdk-python39	729c7ce	link	unknown	/test kubeflow-pipelines-sdk-python39
kubeflow-pipelines-sdk-docformatter	729c7ce	link	unknown	/test kubeflow-pipelines-sdk-docformatter
kubeflow-pipelines-sdk-python37	729c7ce	link	unknown	/test kubeflow-pipelines-sdk-python37

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[rimolive]

/close

[google-oss-prow]

@rimolive: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.