Skip to content
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.

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

feat(backend): Rootless and secure leightweight python component pipelines with k8sapi #4645

Closed
wants to merge 10 commits into from

Conversation

juliusvonkohout
Copy link
Member

@juliusvonkohout juliusvonkohout commented Oct 20, 2020

Change necessary for secure k8sapi executor and leightweight python components. The k8sapi cannot extract from /tmp but mount an empytdir to /outputs and extract from there.

I have tested it with k8sapi and PNS executor on kubernetes 1.18. It supports also metrics, mlpipeline-ui-metadata etc.

  • Do you want this pull request (PR) cherry-picked into the current release branch?

I can achieve the same with

import kfp, sys
from kfp import components
# monkey patching 
# https://medium.com/@chipiga86/python-monkey-patching-like-a-boss-87d7ddb8098e
components._components._outputs_dir = '/outputs'
del sys.modules['kfp.components']
from kfp.components import func_to_container_op, InputPath, OutputPath
from typing import NamedTuple

def add_outputs_dir(op):
    from kubernetes import client as k8s_client
    op.add_volume(k8s_client.V1Volume(name='outputs', empty_dir=k8s_client.V1EmptyDirVolumeSource()))
    return op.container.add_volume_mount(k8s_client.V1VolumeMount(name='outputs', mount_path='/outputs'))

Example pipeline snippet:

def sklearn_pipeline(test_size: float = 0.5):
    # Warning all images are run as USER 1000 (non-root) for security reasons
    # Base image with support for packages_to_install=['tensorflow', 'tensorflow_datasets']
    BASE_IMAGE = 'docker.io/jupyter/tensorflow-notebook'
    sklearn_preprocess_component = func_to_container_op(func=sklearn_preprocess, base_image=BASE_IMAGE, 
                                                        packages_to_install=['scikit-learn>=0.23'])(test_size)
    sklearn_preprocess_component = add_outputs_dir(sklearn_preprocess_component) # For k8sapi

It would be amazing to add additional parameters to func_to_container_op() that automatically adds an emptydir and specific user if desired

func_to_container_op(... , add_outputs_dir=True, user=None):
    ...
    op = ...
    ...
    if add_outputs_dir == True:
        op = add_outputs_dir(op)
    if user not None:
        op.set_security_context(V1SecurityContext(run_as_user=user))

Please tell me if i should add this too. For me it seems necessary because e.g. packages_to_install only works with the correct user

Change necessary for secure k8sapi executor and leightweight python components. The k8sapi cannot extract from /tmp but mount an empytdir to /outputs and extract from there.
@google-cla google-cla bot added the cla: yes label Oct 20, 2020
@kubeflow-bot
Copy link

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @juliusvonkohout. 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.

@Ark-kun
Copy link
Contributor

Ark-kun commented Oct 20, 2020

One of the reasons /tmp/ has been chosen is because in some containers the user is not root and cannot create directories in /, but can create them in /tmp/

The k8sapi cannot extract from /tmp but mount an empytdir to /outputs and extract from there.

Can an emptyDir be mounted to /tmp/output_name/ ?

It would be amazing to add additional parameters to func_to_container_op() that automatically adds an emptydir and specific user if desired

The proper place for such customizations would be DSL (ContainerOp) and DSL-Compiler. The compiler could add emptyDir volumes under all output artifacts. And the most ideal place would be the Argo orchestrator: argoproj/argo-workflows#2679 It would be great to fix the issue there.

What do you think?

@juliusvonkohout
Copy link
Member Author

One of the reasons /tmp/ has been chosen is because in some containers the user is not root and cannot create directories in /, but can create them in /tmp/

The directory mount is done by kubernetes i think. That also explains why it is working in my rootless containers and is writeable by any container user. So there is no reason to use /tmp. We could also move /tmp/inputs to /inputs, i guess.

The k8sapi cannot extract from /tmp but mount an empytdir to /outputs and extract from there.

Can an emptyDir be mounted to /tmp/output_name/ ?

Yes, but k8sapi and PNS cannot extract artifacts if it is mounted over the base docker image like /tmp/... . it must be a new directory like /inputs or /outputs

It would be amazing to add additional parameters to func_to_container_op() that automatically adds an emptydir and specific user if desired

The proper place for such customizations would be DSL (ContainerOp) and DSL-Compiler. The compiler could add emptyDir volumes under all output artifacts.

Well i can also add it there, just tell me where exactly.

And the most ideal place would be the Argo orchestrator: argoproj/argo#2679 It would be great to fix the issue there.

What do you think?

Well this might take ages. If argo implements it after some months, we also have to switch to the latest argo. I also think that one emptydir per artifact might be overkill. just using one emptydir /outputs and maybe even another /inputs is a fast, simple and clean solution for the current executors (PNS + K8sapi).

If argo really changes it and we have the latest officially released version in a year, then we can always reconsider. But i think this small change in exchange for secure and rootless containers is definitely sensible.

@Ark-kun
Copy link
Contributor

Ark-kun commented Oct 22, 2020

The directory mount is done by kubernetes i think. That also explains why it is working in my rootless containers and is writeable by any container user.

But that only works when something is mounted there, right?

@juliusvonkohout
Copy link
Member Author

juliusvonkohout commented Oct 22, 2020

The directory mount is done by kubernetes i think. That also explains why it is working in my rootless containers and is writeable by any container user.

But that only works when something is mounted there, right?

  1. if you are root you can access it anyway
  2. if the User is not root (and no extra capabilities like needed for PNS) then only the k8sapi executor works and you need the mount anyway
  3. if the rootless user has capabilities and uses PNS, well then there is a problem without a mount

So yes also PNS without root but capabilities needs that mount too.

i also found out that a newer argoexec is needed for pipelines that output string/int/float instead of files or namedtuples e.g.

def execute_shell_command(command: str) -> str:
    import subprocess
    process = subprocess.run(command, shell=True, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, check=False)
    for line in process.stdout.decode('utf-8').split('\n'): print(line);
    return process.stdout.decode('utf-8')
kubectl edit deployment/workflow-controller -n kubeflow
spec:
      containers:
      - args:
        - --configmap
        - workflow-controller-configmap
        - --executor-image
        - docker.io/argoproj/argoexec:v2.11.6
        # bug https://github.com/argoproj/argo/issues/1445
        command:
        - workflow-controller

@Ark-kun
Copy link
Contributor

Ark-kun commented Oct 22, 2020

  1. if you are root you can access it anyway
  2. if the User is not root

Just to make it clear: I was talking about the internal container user. Some container images have USER <non-root>. When running on these images, you cannot even do pip install - you either need pip install --user or sudo pip install (which does not usually work on images where USER==root).
All of that is with the docker executor.
I'm just trying to asses and prevent the issues that docker executor users might experience after such change.
I'm not sure this change is safe without adding volumes in the compiler.

i also found out that a newer argoexec is needed for pipelines that output string/int/float instead of files or namedtuples e.g.

That's really strange. There should not be any difference between -> str and -> NamedTuple('Outputs', [('Output', str)]). Can you please create a bug for that issue?

@juliusvonkohout
Copy link
Member Author

juliusvonkohout commented Oct 23, 2020

  1. if you are root you can access it anyway
  2. if the User is not root

Just to make it clear: I was talking about the internal container user. Some container images have USER <non-root>. When running on these images, you cannot even do pip install - you either need pip install --user or sudo pip install (which does not usually work on images where USER==root).
All of that is with the docker executor.
I'm just trying to asses and prevent the issues that docker executor users might experience after such change.
I'm not sure this change is safe without adding volumes in the compiler.

The docker executor accesses the docker sock and is therefore insecure by design. So you should just run your images a root. You can configure the user of the image inside workflow template. But if you really want this strange scenario, well then we should

  1. add the /outputs emptydir. I am currently doing this manually. Please tell me where it should be added, i am not that familiar with the compiler.
  2. Use argoexec 2.11.6 image
  3. change from /tmp/outputs to /outputs

With those three changes all of my python pipelines run secure and rootless with k8sapi. This should also work for PNS and Docker where the main container is rootless.

i also found out that a newer argoexec is needed for pipelines that output string/int/float instead of files or namedtuples e.g.

That's really strange. There should not be any difference between -> str and -> NamedTuple('Outputs', [('Output', str)]). Can you please create a bug for that issue?

The bug is here bug argoproj/argo-workflows#1445 and fixed in newer argoexec images. I guess i just did not test namedtuple before, but with the newer argoexec everything is working

@Ark-kun
Copy link
Contributor

Ark-kun commented Oct 24, 2020

PNS cannot extract artifacts if it is mounted over the base docker image like /tmp/... . it must be a new directory like /inputs or /outputs

I thought the PNS has a way to do that: https://github.com/argoproj/argo/blob/220ac736c1297c566667d3fb621a9dadea955c76/workflow/executor/pns/pns.go#L175

The docker executor accesses the docker sock and is therefore insecure by design.

The main container is not privileged though.

You can configure the user of the image inside workflow template. But if you really want this strange scenario

It's not my choice really. Some authors of official container images (e.g. Airflow or Jupyter) do that. They release images where the user is not a root. The KFP SDK can do nothing about that.

The bug is here bug argoproj/argo-workflows#1445

It's pretty strange that you got issues depending on how you produce the outputs. They're supposed to be produced the same way (as files) on the low level.

Please tell me where it should be added, i am not that familiar with the compiler.

It might be possible to add emptyDir volumes around here:

You can access template['outputs']['artifacts'] to learn the artifact paths.

@juliusvonkohout
Copy link
Member Author

PNS cannot extract artifacts if it is mounted over the base docker image like /tmp/... . it must be a new directory like /inputs or /outputs

I thought the PNS has a way to do that: https://github.com/argoproj/argo/blob/220ac736c1297c566667d3fb621a9dadea955c76/workflow/executor/pns/pns.go#L175

https://github.com/argoproj/argo/blob/master/docs/workflow-executors.md says: PNS Cannot capture artifacts from a base layer which has a volume mounted under it

The docker executor accesses the docker sock and is therefore insecure by design.

The main container is not privileged though.

Yes, but the host is compromised by accessing docker.sock. If the host is compromised, what is the point on rootless containers?

You can configure the user of the image inside workflow template. But if you really want this strange scenario

It's not my choice really. Some authors of official container images (e.g. Airflow or Jupyter) do that. They release images where the user is not a root. The KFP SDK can do nothing about that.

An important note:
Een if you set another user by default it runs as what the pod security policy allows. If i use minikube with docker it is just root. It does not matter whether "USER XXX" is set in the dockerfile. It is ignored as far as i remember.
If i set the PSP to only allow e.g. user 1000-2000 then it runs as user 1000.

you have to set the user inside the securitycontext via op.set_security_context(V1SecurityContext(run_as_user=user))

currently i am using

def add_output_dir(op, user = 1000):
    from kubernetes import client as k8s_client
    from kubernetes.client.models import V1SecurityContext
    op.add_volume(k8s_client.V1Volume(name='outputs', empty_dir=k8s_client.V1EmptyDirVolumeSource()))
    op.container.add_volume_mount(k8s_client.V1VolumeMount(name='outputs', mount_path='/outputs'))
    op.set_security_context(V1SecurityContext(run_as_user=user))
    return op

But you are right, that if you run the strange scenario of compromised host via docker.sock but rootless containers, then this will be a problem. So we just always add the emptydir

The bug is here bug argoproj/argo#1445

It's pretty strange that you got issues depending on how you produce the outputs. They're supposed to be produced the same way (as files) on the low level.

Yes, i think it was a mistake on my side. NamedTuples and Outputpath should be files. Reagarding 'function() -> int:' its not a file i think. But it does not matter anymore since we need the newer argoexec image anyway.

Please tell me where it should be added, i am not that familiar with the compiler.

It might be possible to add emptyDir volumes around here:


You can access template['outputs']['artifacts'] to learn the artifact paths.

Thank you, i will look into it. I will just add it unconditionally here

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: juliusvonkohout
To complete the pull request process, please assign ark-kun after the PR has been reviewed.
You can assign the PR to them by writing /assign @ark-kun in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@juliusvonkohout
Copy link
Member Author

juliusvonkohout commented Oct 24, 2020

I have finally found the right place to insert the volume and volume mount. Way earlier than in _op_to_template.py. I used /argo because later on we might be able to also move inputs to /argo/inputs.
Leightweight component python pipelines run properly rootless if you set the right container user.

I still encounter a general rootless k8sapi issue:

  1. RessourceOPs
    vop = kfp.dsl.VolumeOp(
        name="volume_creation",
        resource_name="mypvc",
        size="1Gi"
    )
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  name: sum-pipeline
  generateName: sum-pipeline-
  annotations: {pipelines.kubeflow.org/kfp_sdk_version: 1.0.4, pipelines.kubeflow.org/pipeline_compilation_time: '2020-10-24T17:15:11.808285',
    pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "100000", "name":
      "count", "optional": true, "type": "Integer"}], "name": "Sum pipeline"}'}
  labels: {pipelines.kubeflow.org/kfp_sdk_version: 1.0.4}
spec:
  entrypoint: sum-pipeline
  templates:
  - name: sum-pipeline
    dag:
      tasks:
      - {name: volume-creation, template: volume-creation}
  - name: volume-creation
    resource:
      action: create
      manifest: |
        apiVersion: v1
        kind: PersistentVolumeClaim
        metadata:
          name: '{{workflow.name}}-mypvc'
        spec:
          accessModes:
          - ReadWriteMany
          resources:
            requests:
              storage: 1Gi
    outputs:
      parameters:
      - name: volume-creation-manifest
        valueFrom: {jsonPath: '{}'}
      - name: volume-creation-name
        valueFrom: {jsonPath: '{.metadata.name}'}
      - name: volume-creation-size
        valueFrom: {jsonPath: '{.status.capacity.storage}'}
  arguments:
    parameters:
    - {name: count, value: '100000'}
  serviceAccountName: pipeline-runner

which is a RessourceOP only works if i click on retry, which is very strange. on first try i get the following error
Bildschirmfoto von 2020-10-24 15-57-41

I have created a bug report argoproj/argo-workflows#4367

@juliusvonkohout
Copy link
Member Author

I upgraded the workflow-controller and argoexec images to 2.11.6 and added the missing CRDs and serviceaccount permission for the new version from https://github.com/argoproj/argo/blob/master/manifests/install.yaml .

I tested successfully with rootless k8sapi and normal pns. Would you mind running the CI/CD tests for verification?

@Ark-kun
Copy link
Contributor

Ark-kun commented Oct 27, 2020

An important note:
Een if you set another user by default it runs as what the pod security policy allows. If i use minikube with docker it is just root. It does not matter whether "USER XXX" is set in the dockerfile. It is ignored as far as i remember.

JFYI: An example user issue: #1694 also #1531

@juliusvonkohout
Copy link
Member Author

An important note:
Een if you set another user by default it runs as what the pod security policy allows. If i use minikube with docker it is just root. It does not matter whether "USER XXX" is set in the dockerfile. It is ignored as far as i remember.

JFYI: An example user issue: #1694 also #1531

@Ark-kun Both usecases are covered by my approach. They are now able to write there, because now there is an emptydir underneath. @numerology do you have any objections about running the automated tests?

@Bobgy
Copy link
Contributor

Bobgy commented Oct 29, 2020

/ok-to-test
This is a very substantial change, so I don't think it's suitable for cherry picking to a patch release.

@k8s-ci-robot
Copy link
Contributor

@juliusvonkohout: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
kubeflow-pipelines-sdk-python37 4fdad7a link /test kubeflow-pipelines-sdk-python37
kubeflow-pipelines-sdk-python36 4fdad7a link /test kubeflow-pipelines-sdk-python36
kubeflow-pipelines-sdk-python38 4fdad7a link /test kubeflow-pipelines-sdk-python38
kubeflow-pipelines-tfx-python36 4fdad7a link /test kubeflow-pipelines-tfx-python36

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@juliusvonkohout
Copy link
Member Author

juliusvonkohout commented Oct 29, 2020

/ok-to-test
This is a very substantial change, so I don't think it's suitable for cherry picking to a patch release.

Alright, i looked at the test results and saw that the pipelines ran successfully, but of course the compiled output changed for container ops. So i do not see problems with my changes, but the expected reference output for some tests has to be updated because, there is an additional volume and mount now. Should i do it in this pull request? Does someone else want do do it?

@juliusvonkohout
Copy link
Member Author

/ok-to-test
This is a very substantial change, so I don't think it's suitable for cherry picking to a patch release.

Alright, i looked at the test results and saw that the pipelines ran successfully, but of course the compiled output changed for container ops. So i do not see problems with my changes, but the expected reference output for some tests has to be updated because, there is an additional volume and mount now. Should i do it in this pull request? Does someone else want do do it?

@Ark-kun should i work on that?

@Bobgy
Copy link
Contributor

Bobgy commented Nov 3, 2020

/cc @chensun @numerology

@k8s-ci-robot k8s-ci-robot requested a review from chensun November 3, 2020 12:36
@juliusvonkohout
Copy link
Member Author

This might make this pull request obsolete argoproj/argo-workflows#4766

@chensun chensun force-pushed the master branch 2 times, most recently from 7542f0e to 44d22a6 Compare February 12, 2021 09:23
@juliusvonkohout
Copy link
Member Author

juliusvonkohout commented May 20, 2021

is this still interesting or do you want to update argo to 3.11 and use the emissary executor instead?

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from ark-kun after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

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
Copy link

google-oss-robot commented May 20, 2021

@juliusvonkohout: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
kubeflow-pipelines-sdk-python37 a7fd65e link /test kubeflow-pipelines-sdk-python37
kubeflow-pipelines-sdk-python39 a7fd65e link /test kubeflow-pipelines-sdk-python39
kubeflow-pipelines-sdk-python38 a7fd65e link /test kubeflow-pipelines-sdk-python38
kubeflow-pipelines-sdk-python36 a7fd65e link /test kubeflow-pipelines-sdk-python36
kubeflow-pipelines-tfx-python36 a7fd65e link /test kubeflow-pipelines-tfx-python36

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.

@juliusvonkohout
Copy link
Member Author

juliusvonkohout commented May 20, 2021

@Ark-kun @Bobgy the tests are written in such a way that i cannot satisfy them. They just fail if there is an additional mount because they check container_dict["volume_mounts"][0] instead of properly checking for the volume_mount with the right name.

class TestAddSecrets(unittest.TestCase):
def test_use_default_use_secret(self):
op1 = ContainerOp(name="op1", image="image")
secret_name = "my-secret"
secret_path = "/here/are/my/secret"
op1 = op1.apply(use_secret(secret_name=secret_name,
secret_volume_mount_path=secret_path))
self.assertEqual(type(op1.container.env), type(None))
container_dict = op1.container.to_dict()
volume_mounts = container_dict["volume_mounts"][0]
self.assertEqual(volume_mounts["name"], secret_name)
self.assertEqual(type(volume_mounts), dict)
self.assertEqual(volume_mounts["mount_path"], secret_path)

Should i change them, or is the focus on the emissary executor (argo 3.1) for rootless pipelines?

@Bobgy
Copy link
Contributor

Bobgy commented May 21, 2021

Hi @juliusvonkohout, there were other reasons we want to upgrade to argo v3 emissionary executor now. I think that will resolve your requirements too. Can you confirm?

@juliusvonkohout
Copy link
Member Author

juliusvonkohout commented May 21, 2021

Hi @juliusvonkohout, there were other reasons we want to upgrade to argo v3 emissionary executor now. I think that will resolve your requirements too. Can you confirm?

Yes, i confirm that this solves the requirements. I have created a new tracking issue for argo 3.1 here #5718

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants