search

kubeflow / pipelines

Machine Learning Pipelines for Kubeflow
https://www.kubeflow.org/docs/components/pipelines/
Apache License 2.0
3.6k stars 1.62k forks source link

[Multi User] Support separate artifact repository for each namespace #4649

Open Bobgy opened 4 years ago

Bobgy commented 4 years ago

Part of https://github.com/kubeflow/pipelines/issues/1223

Support configuring separate artifact repository for each namespace, so that artifact repository can have namespace separation too.

This is currently not supported in KFP multi user support as explained in https://github.com/kubeflow/pipelines/issues/1223#issuecomment-656507073.

Use your thumbs up to show how important this issue is for you.

We'll most likely need upstream enhancement from argo in https://github.com/argoproj/argo/issues/3307.

Ark-kun commented 4 years ago

Last year I've added the ArtifactRepositoryRef feature to Argo to support this goal. The idea is to make it possible to specify the artifact repository information by referencing a ConfigMap. See https://github.com/argoproj/argo/pull/1350 and https://github.com/argoproj/argo/pull/1821 With both features in place it might even be unnecessary to make any SDK-side changes. Each namespace can have a different artifact repository ConfigMap with the same name. Then Argo will choose one based on the namespace. The backend can add the artifactRepositoryRef field when submitting the run.

I'm not sure argoproj/argo#3307 is relevant since it's about explicit input artifacts, a feature not used by KFP.

Bobgy commented 3 years ago

If anyone would like to help this issue, confirming artifactRepositoryRef usage and whether it supports all the features we want can be a step forward

solarist commented 3 years ago

In case of S3, would it make sense to perform the auth for minio using dex? Because otherwise someone has to generate access/secret keys for each namespace -> https://github.com/minio/minio/blob/master/docs/sts/dex.md

marcjimz commented 3 years ago

@Bobgy we can pick this up as this will be a requirement for us to go multi-tenant. I think this makes sense as the administrator of the platform would be provisioning the namespaces and with that can provision the scoped configMap.

I suspect we want to edit the workflow spec more upstream to here: https://github.com/kubeflow/pipelines/blob/89e42105bd941e1d918d0bff9ba0c4f80b106ed4/backend/src/crd/controller/scheduledworkflow/client/workflow_client.go#L138

Any suggestions on the best place to support the change to the workflow spec?

edit: Suppose writing methods to add this new object to the workflow spec could be added here: https://github.com/kubeflow/pipelines/blob/89e42105bd941e1d918d0bff9ba0c4f80b106ed4/backend/src/crd/controller/scheduledworkflow/util/scheduled_workflow.go#L165

Bobgy commented 3 years ago

Hi @marcjimz, thank you for offering help.

You can look at https://github.com/kubeflow/pipelines/blob/135bfbb9f2945a7e841d5891ee0f5862c75aa3b7/backend/src/apiserver/resource/resource_manager.go#L317 and https://github.com/kubeflow/pipelines/blob/135bfbb9f2945a7e841d5891ee0f5862c75aa3b7/backend/src/apiserver/resource/resource_manager.go#L645

The two methods are responsible for preparing argo workflow yaml before submitting to K8s api, we might be able to inject the artifact repository ref there.

Read documentation for argo in https://argoproj.github.io/argo/configure-artifact-repository/,

arllanos commented 3 years ago

Hi @Bobgy, I have been testing the artifactRepositoryRef feature and also tested locally initial pipeline code change as to inject the artifactRepositoryRef as suggested above.

Before creating a PR I would like to bring up the following points to get your feedback:

  1. This feature seems to be working since version 2.9. So, we need to upgrade argo from 2.3.0 to 2.9.3 or newer. We have been discussing with @marcjimz to use 2.10.0. Do you have any comments or issues regarding 2.10.0?
  2. Regarding the configMap mentioned by @Ark-kun, based on the documentation if we name the config map artifact-repositories it will be the default config map for artifact repositories in the namespace. 
    apiVersion: v1
kind: ConfigMap
metadata:
# if you want to use this config map by default - name it "artifact-repositories" 
name: artifact-repositories
annotations:
# if you want to use a specific key, put that's key into this annotation 
workflows.argoproj.io/default-artifact-repository: mlpipeline-repository
data:
mlpipeline-repository: |
s3:
  bucket: my-bucket
  keyPrefix: artifacts
  endpoint: minio-service.kubeflow:9000
  insecure: true
  accessKeySecret:
    name: mlpipeline-minio-artifact
    key: accesskey
  secretKeySecret:
    name: mlpipeline-minio-artifact
    key: secretkey

    We can either rely on the default config map name (artifact-repositories) or we can have separate config map like mlpipeline-minio-artifact. I would be using the default artifact-repositories config map but you may have reasons to use a separate config map instead. What is you feedback here?

Bobgy commented 3 years ago

Awesome progress,

  1. We can go to argo 2.12 directly if we need to upgrade anyway. Argo maintainer said it will be LTS.
  2. Whatever the simpler is better, if there is a config map picked by default, that sounds the best. More options can be added as follow ups.
arllanos commented 3 years ago

@Bobgy, feel free to take a look into #5017. Note this new functionality requires the following:

We could have them created as part of profile creation and tackle this in a separate PR.

amitripshtos commented 3 years ago

I'm really glad that this feature is pushed! I wonder if the standalone version will be able to use this feature. Any thoughts about that?

arllanos commented 3 years ago

I'm really glad that this feature is pushed! I wonder if the standalone version will be able to use this feature. Any thoughts about that?

Good point @amitripshtos regarding standalone version. I'll to test this particular scenario. Depending on the result, extra changes might be needed.

arllanos commented 3 years ago

@Bobgy / @Ark-kun, as mentioned above, when submitting workflows with the ArtifactRepositoryRef field, two things have to exist for each namespace: (1) the referenced config map artifact-repositories and (2) the bucket that corresponds to the namespace.

  1. To create the config map for each namespace we can take advantage of the pipeline profile controller. The config map can be created as part of the multiuser namespace initialization.
  2. To create the bucket for each namespace, since this is not a k8s object I'm not sure using the controller will be the right approach. Right now I see the default bucket is created in the backend apiserver. So we have different alternatives. Do you have any recommendation?

Looking forward to getting your advise about these two points.

Bobgy commented 3 years ago

Content of the config map should be user defined, so it's not suitable for the profile controller.

Bobgy commented 3 years ago

My gut feeling is that, we should make this ref configurable with API request. It's similar to a service account.

Maybe we can define a user configurable namespace default in some way.

Anyway, I'd suggest thinking about the MVP of this feature, so we do not expand responsibility of a single pr too much. There can always be follow ups to add the additional features

arllanos commented 3 years ago

@Bobgy, sorry I was disconnected for a while. I would like to resume with issue and to share some points.

The use of ArtifactRepositoryRef is making it possible for runs to put their output artifacts to the bucket configured in the user namespace ConfigMap.

However, for CRUD operations related to pipelines, KFP will not use the ArtifactRepositoryRef feature. All CRUD functionality implemented in the object store does not account for a bucket as parameter. These use the bucketName that was set during minio client initialization.

https://github.com/kubeflow/pipelines/blob/1cd189567d4ba7e71aa1f31ddaf9a46c17ae8ae3/backend/src/apiserver/client_manager.go#L391

To fully achieve namespace separation we can adjust the object store to account for a bucketName parameter. This will require to adjust resource manager, persistence agent, etc, so we pass the bucketName as parameter accordingly. Do you think there is another better way?

We can also set by convention that user namespace and bucket name be used interchangeably, that is, if user namespace = xyz; bucketName = xyz.

Regarding your last comment on Feb 7, do you mind to elaborate more on that idea?

My gut feeling is that, we should make this ref configurable with API request. It's similar to a service account.

amitripshtos commented 3 years ago

Argo workflows has released a new major version which introduce default artifact repository for every namespace ( https://www.cncf.io/blog/2021/04/02/argo-workflows-3-0-released/)

Maybe we can use this feature here?

On Fri, Apr 2, 2021, 08:47 arllanos @.***> wrote:

@Bobgy https://github.com/Bobgy, sorry I was disconnected for a while. I would like to resume with issue and to share some points.

The use of ArtifactRepositoryRef is making it possible for runs to put their output artifacts to the bucket configured in the user namespace ConfigMap.

However, for CRUD operations related to pipelines, KFP will not use the ArtifactRepositoryRef feature. All CRUD functionality implemented in the object store http://../blob/master/backend/src/apiserver/storage/object_store.go does not account for a bucket as parameter. All CRUD operations use the bucketName that was set during minio client initialization.

https://github.com/kubeflow/pipelines/blob/1cd189567d4ba7e71aa1f31ddaf9a46c17ae8ae3/backend/src/apiserver/client_manager.go#L391

Do you think there another way to fully achieve namespace separation other than also adjust the object store to account for a bucketName parameter? This will require adjust resource manager, persistence agent, etc, so we pass the bucketName as parameter accordingly.

It appears to be convenient to set by convention that user namespace and bucket name can be used interchangeably, that is, if user namespace = xyz; bucketName = xyz.

Regarding your last comment on Feb 7, do you mind to elaborate more on that idea?

My gut feeling is that, we should make this ref configurable with API request. It's similar to a service account.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kubeflow/pipelines/issues/4649#issuecomment-812333170, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACSFNTE7NZCAN6BO2FBM4JDTGVK7BANCNFSM4SZHMH3A .

Bobgy commented 3 years ago

Agreed, for namespace default artifact repository, we can ask users to config in the argo way.

maganaluis commented 3 years ago

What about public vs namespaced pipelines, we introduced this feature when adding the functionality to the backend, the front-end changes for that are still to be added.

So we would need to take this into account when dealing with a multi-user kubeflow installation. When we retrieve objects we'll have to somehow know if this is a public or namespaced pipeline, so it's not so trivial. In my opinion all artifacts should be stored in their respective namespaced bucket, and the metadata can be used just to determine if they can be accessed or not.

Bobgy commented 3 years ago

@maganaluis not sure if I fully understood the concern, the current model is that pipeline (templates) may be public or namespaced, but when you run a pipeline, it's always namespaced. Therefore, I don't think there are complexity regarding global/namespaced objects. Did I miss anything?

maganaluis commented 3 years ago

@Bobgy Yeah my concern was about the template files, it's typically a YAML file, or tar which contains the pipeline version. Right now this sits on a single bucket, but given there is public and namespaced pipelines, I had concerns what to do with the public pipelines if keep them on the current bucket or also moved them to the user's namespaced bucket. I think it's best to have all artifacts namespaced when implementing this feature.

amitripshtos commented 3 years ago

@Bobgy, sorry I was disconnected for a while. I would like to resume with issue and to share some points.

The use of ArtifactRepositoryRef is making it possible for runs to put their output artifacts to the bucket configured in the user namespace ConfigMap.

However, for CRUD operations related to pipelines, KFP will not use the ArtifactRepositoryRef feature. All CRUD functionality implemented in the object store does not account for a bucket as parameter. These use the bucketName that was set during minio client initialization.

https://github.com/kubeflow/pipelines/blob/1cd189567d4ba7e71aa1f31ddaf9a46c17ae8ae3/backend/src/apiserver/client_manager.go#L391

To fully achieve namespace separation we can adjust the object store to account for a bucketName parameter. This will require to adjust resource manager, persistence agent, etc, so we pass the bucketName as parameter accordingly. Do you think there is another better way?

We can also set by convention that user namespace and bucket name be used interchangeably, that is, if user namespace = xyz; bucketName = xyz.

Regarding your last comment on Feb 7, do you mind to elaborate more on that idea?

My gut feeling is that, we should make this ref configurable with API request. It's similar to a service account.

After thinking about it for some time, I think we should be able to specify the bucket information for each pipeline (like service account) since it will be able to support separate repositories for standalone deployment of KFP.

If we use Argo 3 feature - that will work only for multi-user version of KFP. I would like to help in this feature, can we set up a design meeting to figure out all of the cases? @Bobgy @arllanos

amitripshtos commented 3 years ago

I'm adding a comment from a chat between me and @arllanos

We want to run different pipelines at different namespaces and different buckets. Kubeflow pipelines have two modes:

I think our solution should match both versions of KFP. Because of that, I don’t think Argo 3 may be good for us 100%.

We need the good for both worlds: For standalone - we need a way to configure which storage bucket we want to save artifacts on For multi-user - if no bucket is configured in a run, we should take the default in a namespace

Therefore - I think this feature should be split into multiple parts:

josete89 commented 3 years ago

Hey there!

Just a small thing, maybe doesn't make any sense, but there is an issue with the same purpose #4790 do you think both can be combined?

Regards

maganaluis commented 3 years ago

@amitripshtos I'm working with Ariel on this, I like this breakdown. We have completed points 2, and 4 internally in PwC and want to make the changes available upstream.

I think point 1 (having it REST configurable like service accounts) will require changes to the Proto files, and UI changes but ultimately this should be our goal.

In regards to point 3, we first have to upgrade Argo. Have you tested the current release? I think we should do this first.

Overall good plan, we just have to get approval from the KFP team. @Bobgy

I'm adding a comment from a chat between me and @arllanos

We want to run different pipelines at different namespaces and different buckets. Kubeflow pipelines have two modes:

  • Standalone - runs on a specific namespace, with 1 bucket
  • Multi-user - namespace per user, currently 1 bucket

I think our solution should match both versions of KFP. Because of that, I don’t think Argo 3 may be good for us 100%.

We need the good for both worlds: For standalone - we need a way to configure which storage bucket we want to save artifacts on For multi-user - if no bucket is configured in a run, we should take the default in a namespace

Therefore - I think this feature should be split into multiple parts:

  • Like service account, add a name of a ConfigMap which has different from a pipeline run and job
  • Change KFP backend’s Minio client to be generated by the run’s new field (so KFP UI will be able to show artifacts)
  • Upgrade to Argo 3 to support per-namespace default configmap
  • Change KFP backend’s MinIO client to fetch default configmap from the run’s namespace (if it’s in multi-user mode / the run started in a namespace that is different from KFP’s deployment)
Bobgy commented 3 years ago

Good points @amitripshtos @maganaluis ! Agree with the plan for KFP v1.

Note, there are early efforts to build KFP v2 experimental, where we support configuring a different artifact output_directory at

Credentials config is decoupled, allowing config at

Note, for platforms that support binding kubernetes service account with other credentials, like workload identity. Changing service account at runtime is possible.

How do you feel these sound like? Maybe we only need namespace default as an extra thing.

/cc @capri-xiyue Is working on this.

arllanos commented 3 years ago

Hello @Bobgy et al. I would like to share with you the following document where we have put together what we observed while internally implementing some of the points discussed so far. https://docs.google.com/document/d/1CCtr6FJmpdlxCQBfjUcMR3oJpY2yQDT9NkdjhhhbZ6E/edit?usp=sharing Soon @maganaluis will put the code changes we did in PwC in a PR. We'll appreciate your feedback.

Bobgy commented 3 years ago

FYI, KFP v2 compatible mode has been released, see documentation: https://www.kubeflow.org/docs/components/pipelines/sdk/v2/.

It doesn't support artifact repository configuration, this is one of the things we want to support too. So I'm posting early thoughts on this related issue.

Let me first try to summarize requirements for configuring artifact repositories for both KFP v2 compatible and v2.

Object store specific credentials requirements

For GCS, AWS S3, we suggest setting up credentials, so that they represent identity of the pipeline step, so that not only artifact upload/download, calls to other Cloud services should also use the same credentials. For this reason, we don't recommend setting credentials in artifact repository config. The suggestion is to configure the identity transparently if possible using GCP workload identity or AWS IRSA. If credentials are really necessary, they can be configured using pipeline DSL via kfp.gcp.use_gcp_secret or kfp.aws.use_aws_secret etc. These principles should apply to other Cloud Providers that has credentials that can be used with all its services.

For on-prem object store like MinIO, the credentials do not represent an identity, they are only used to access a specified object store instance. Therefore, it's reasonable to include them in artifact repository config.

In summary

We cannot implement a spec for every possible object stores, so likely we should use the same spec as what Go CDK supports or depend on cloud provider contributions.

Go CDK supports provider specific query params to configure some things other than object key, so we might consider adopting these query params, so that pipeline root can be more expressive, so we might not need other configurations. e.g. for S3, it's possible to configure region via a query param: https://gocloud.dev/howto/blob/#s3

s3://my-bucket?region=us-west-1

How we configure pipeline root, other configs and credentials?

Ideally, pipeline root can include all other configs, so that we can uniquely identify an artifact. Ideally, credentials should be configurable transparently.

When both ideal requirements are met, we only need to support namespace level default pipeline root. All other configurations can be done by specifying different pipeline roots.

However, now MinIO violates the requirement that credentials can be configured transparently. Therefore, we need a mechanism to either

We probably need more thoughts on the exact config format, this seems like a complex problem.

capri-xiyue commented 3 years ago

I think we can make both pipeline root and the credential of pipeline root configurable similar to what argo does https://github.com/argoproj/argo-workflows/blob/master/docs/configure-artifact-repository.md. We can differentiate which cloud provider the user is using by the uri and the initialize specific storage services with the credentials configured with GO CDK https://gocloud.dev/howto/blob/#services

DianaDai commented 3 years ago

Hello @Bobgy et al. I would like to share with you the following document where we have put together what we observed while internally implementing some of the points discussed so far. https://docs.google.com/document/d/1CCtr6FJmpdlxCQBfjUcMR3oJpY2yQDT9NkdjhhhbZ6E/edit?usp=sharing Soon @maganaluis will put the code changes we did in PwC in a PR. We'll appreciate your feedback.

@arllanos Can you tell me when this feature will be released，thx.

Bobgy commented 3 years ago

TODO: document how this can be done with v2 compatible mode

Bobgy commented 3 years ago

Documentation is tracked in https://github.com/kubeflow/pipelines/issues/6310

juliusvonkohout commented 3 years ago

Documentation is tracked in #6310

I think the best solution would be if those per namespaces settings are created by default. E.g. minio should create a new bucket with different credentials for each namespace and use /namespace/mlpipeline/artifacts and namespace/mlpipeline/pipelines. The bucket location and credentials could be stored inside the kubeflow profile object from which then the other configs for argo are created per namespace.

amitripshtos commented 3 years ago

@Bobgy , I wonder if the KFP cache has the pipeline_root as part of the cache key :)

capri-xiyue commented 3 years ago

@Bobgy , I wonder if the KFP cache has the pipeline_root as part of the cache key :)

pipeline_root is not part of the cache key in KFP V2. Please refer to caching v2 doc for v2 caching. The task’s interface is defined as the combination of the pipeline task specification (base image, command, args), the pipeline task’s inputs (the name and id of artifacts, the name and value of parameters), and the pipeline task’s outputs specification(artifacts and parameters). regarding the how cache key is generated.

For KFP V1, please refer to caching v1 doc. For KFP V1, the cache key is the value stored in pod annotations whose key is "workflows.argoproj.io/template", refer to code

capri-xiyue commented 3 years ago

This feature is added in v2 compatible mode and here is the relevant doc

juliusvonkohout commented 3 years ago

This feature is added in v2 compatible mode and here is the relevant doc

It is not yet supported if you install the pipelines as part of kubeflow. There should be a seperate minio folder with seperate credentials for each namespace

Bobgy commented 3 years ago

This feature is added in v2 compatible mode and here is the relevant doc

It is not yet supported if you install the pipelines as part of kubeflow. There should be a seperate minio folder with seperate credentials for each namespace

@juliusvonkohout I would argue that configuring MinIO per namespace is a user configuration problem, therefore out of scope for KFP. I think you are already able to do that with pipeline root -- by default v2 compatible mode looks for minio-service service in the same namespace and reads mlpipeline-minio-artifact secret in the same namespace for credentials. Please let me know if that doesn't work.

But you did remind me that artifact visualization doesn't support multi repository yet. /reopen

/cc @zijianjoy Welcome contribution like before to fill in the feature gap!

google-oss-robot commented 3 years ago

@Bobgy: Reopened this issue.

In response to [this](https://github.com/kubeflow/pipelines/issues/4649#issuecomment-926722534): >> > This feature is added in v2 compatible mode and here is the [relevant doc](https://www.kubeflow.org/docs/components/pipelines/pipeline-root/) >> >> It is not yet supported if you install the pipelines as part of kubeflow. There should be a seperate minio folder with seperate credentials for each namespace > >@juliusvonkohout I would argue that configuring MinIO per namespace is a user configuration problem, therefore out of scope for KFP. I think you are already able to do that with pipeline root -- by default v2 compatible mode looks for minio-service service in the same namespace and reads mlpipeline-minio-artifact secret in the same namespace for credentials. Please let me know if that doesn't work. > >But you did remind me that artifact visualization doesn't support multi repository yet. >/reopen > >/cc @zijianjoy >Welcome contribution like before to fill in the feature gap! Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
juliusvonkohout commented 3 years ago

@Bobgy

There is mlpipeline-minio-artifact which contains 

accesskey:    minio
secretkey: minio123

so yes we can change the credentials, but not the bucket. The goal is to use one bucket per namespace (e.g. mlpipeline-$NAMESPACE) with different credentials per bucket. So there is no point in using minio-service since we want to use the same minio for all namespaces.

The configuration happens instead in the kubeflow namespace There is the workflow-controller-configmap

artifactRepository
    archiveLogs: true
    s3:
      endpoint: "minio-service.kubeflow:9000"
      bucket: "mlpipeline"
      # keyFormat is a format pattern to define how artifacts will be organized in a bucket.
      # It can reference workflow metadata variables such as workflow.namespace, workflow.name,
      # pod.name. Can also use strftime formating of workflow.creationTimestamp so that workflow
      # artifacts can be organized by date. If omitted, will use `{{workflow.name}}/{{pod.name}}`,
      # which has potential for have collisions, because names do not guarantee they are unique
      # over the lifetime of the cluster.
      # Refer to https://kubernetes.io/docs/concepts/overview/working-with-objects/names/.
      #
      # The following format looks like:
      # artifacts/my-workflow-abc123/2018/08/23/my-workflow-abc123-1234567890
      # Adding date into the path greatly reduces the chance of {{pod.name}} collision.
      keyFormat: "artifacts/{{workflow.name}}/{{workflow.creationTimestamp.Y}}/{{workflow.creationTimestamp.m}}/{{workflow.creationTimestamp.d}}/{{pod.name}}"
      # insecure will disable TLS. Primarily used for minio installs not configured with TLS
      insecure: true
      accessKeySecret:
        name: mlpipeline-minio-artifact
        key: accesskey
      secretKeySecret:
        name: mlpipeline-minio-artifact
        key: secretkey

bucket: "mlpipeline" is the important part. We need a way to change that per namespace to mlpipeline-$NAMESPACE

based on https://argoproj.github.io/argo-workflows/configure-artifact-repository/ 

The actual repository used by a workflow is choosen by the following rules:
    1. Anything explicitly configured using Artifact Repository Ref. This is the most flexible, safe, and secure option.
    2. From a config map named artifact-repositories if it has the workflows.argoproj.io/default-artifact-repository annotation in the workflow's namespace.
    3. From a workflow controller configmap.

So i could delete the key s3 or even artifactRepository from workflow-controller-configmap in the kubeflow namespace and add according to https://argoproj.github.io/argo-workflows/artifact-repository-ref/ something like

apiVersion: v1
kind: ConfigMap
metadata:
    # if you want to use this config map by default - name it "artifact-repositories" 
  name: artifact-repositories
  annotations:
    # v3.0 and after - if you want to use a specific key, put that's key into this annotation 
    workflows.argoproj.io/default-artifact-repository: default-v1
data:
  default-v1: |
    s3:
      endpoint: "minio-service.kubeflow:9000"
      bucket: "mlpipeline-$NAMESPACE"
      insecure: true
      accessKeySecret:
        name: mlpipeline-minio-artifact
        key: accesskey
      secretKeySecret:
        name: mlpipeline-minio-artifact
        key: secretkey

to each users $NAMESPACE. Of course there is some effort to modify the pipeline-profiles-controller etc. to generate such namespace-specific configmaps, but it is definitely possible.

I will test that in the next weeks. This user isolation would be a big milestone for enterprise Kubeflow deployments.

The next step would be that pipeline-profiles-controller generates random passwords for mlpipeline-minio-artifact per namespace. For that pipeline-profile-controller must login to minio and create the new minio user, password and bucket. I just need to find a way to generate passwords in a deterministic way (such that metacontoller always creates the same resources) but depending on some namespace secret that is not known in other namespaces. What do you think about using a cryptographic hash of the token from profiles-controller-service-account-token mixed with $NAMESPACE @bobgy ?

Bobgy commented 3 years ago

@juliusvonkohout our future plan is like the following:

  1. Refer to https://www.kubeflow.org/docs/components/pipelines/pipeline-root/#via-configmaps, you can configure different pipeline root per namespace, e.g. minio://mlpipeline-ns1.
  2. For v2 compatible, artifact metadata is still passed via argo artifacts, but it's only metadata info, the actual artifact content are on pipeline root. Is there a strong need to require artifact metadata to be separated per namespace?
  3. For v2, KFP will stop using argo artifacts altogether only using ml metadata for artifact passing.

Because in the 3rd stage we don't rely on argo artifacts, I am a bit hesitant to keep investing in that direction. Anyway, feel free to do so if you have a strong business requirement right now.

juliusvonkohout commented 3 years ago

@Bobgy ok, If I simply set minio://mlpipeline-namespace in kfp-launcher in namespace, does that also modify the user interface to only show pipelines from that pipeline root?

So for v1 and pure Argo artifacts I can try approach mentioned in the post above. For V2 I would rely on kfp-launcher.

Is it possible to set usernames and passwords for ml-metadata? I really need a way to isolate the namespaces completely in the long term. Each namespace might belong to a different customer or department and they must not be able to access other customers/departments pipelines, metadata etc.

Bobgy commented 3 years ago

@Bobgy ok, If I simply set minio://mlpipeline-namespace in kfp-launcher in namespace, does that also modify the user interface to only show pipelines from that pipeline root?

No, pipelines are namespace separated on its own.

So for v1 and pure Argo artifacts I can try approach mentioned in the post above. For V2 I would rely on kfp-launcher.

Yes, you can do that.

Is it possible to set usernames and passwords for ml-metadata? I really need a way to isolate the namespaces completely in the long term. Each namespace might belong to a different customer or department and they must not be able to access other customers/departments pipelines, metadata etc.

That is a question to google/ml-metadata. Can you raise a FR there? As far as I know, there is no current plan. Would it be fine bringing up a separate mlmd instance per namespace? Any downsides?

juliusvonkohout commented 3 years ago

@Bobgy ok, If I simply set minio://mlpipeline-namespace in kfp-launcher in namespace, does that also modify the user interface to only show pipelines from that pipeline root?

No, pipelines are namespace separated on its own.

Ok this is a big issue. We really need a strict namespace isolation for pipeline definitions, runs, experiments, artifacts etc. minio://mlpipeline-namespace in kfp-launcher should also be respected by the visualization server etc.

So for v1 and pure Argo artifacts I can try approach mentioned in the post above. For V2 I would rely on kfp-launcher.

Yes, you can do that.

Is it possible to set usernames and passwords for ml-metadata? I really need a way to isolate the namespaces completely in the long term. Each namespace might belong to a different customer or department and they must not be able to access other customers/departments pipelines, metadata etc.

That is a question to google/ml-metadata. Can you raise a FR there? As far as I know, there is no current plan. Would it be fine bringing up a separate mlmd instance per namespace? Any downsides?

Yes i will ask them, but your suggestion is more interesting. The tiny downside would be one more PVC and pod per user namespace. Would it be possible and sensible to move the instances inside the users namespace? The same would be interesting for minio. Then each profile/user really has all its own ressources in its own namespace. This sounds very interesting. Then we could create proper quotas and billing per profile/user/department/customer. And from a security perspective this would make it also a bit cleaner i guess.

Bobgy commented 3 years ago

A bit more context: namespaced pipeline template API has been implemented. What is missing -- https://github.com/kubeflow/pipelines/issues/5084

Also for the level of separation you want to achieve, I think the simplest solution is installing KFP standalone for each namespace.

juliusvonkohout commented 3 years ago

A bit more context: namespaced pipeline template API has been implemented. What is missing -- #5084

Also for the level of separation you want to achieve, I think the simplest solution is installing KFP standalone for each namespace.

Well we also want to use notebooks katib kfserving etc. so standalone pipelines are not an option. Is that level of separation not desirable for everyone?

So the UI is lagging behind, but hopefully kfp.client supports namespaced pipelines already.

raghulkrishna commented 3 years ago

Any update on this feature i would be really helpful for our usecases

juliusvonkohout commented 3 years ago

Any update on this feature i would be really helpful for our usecases

The only really clean solution I can think of is updating the profile controller to create minio and ml-metadata instances per profile / namespace similar to the ml-pipeline-visualization-server. The overhead is negligible and all artifacts etc are strictly seperated with different instances and passwords. As a second measure I would add a Networkpolicy that blocks all incoming traffic to user namespaces that does not come from istio-system, kubeflow, knative-eventing, knative-serving. Maybe even more strict. So we just need a way to set the minio and ml-metadata endpoints per namespace. I think there are already configmaps for that (workflow-controller-configmap, kfp-launcher etc.). In the long term with V2 pipelines we can drop minio. If @bobgy is fine with that and willing to merge such a pullrequest I could implement it in November.

The shared pipeline yamls could reside in the instances in the kubeflow namespace if that is essential. The webinterface must be adjusted to work per namespace, @bobgy is actually working on this

jacobmalmberg commented 3 years ago

This will enable multi user minio bucket supporting v1/v2 compatible pipeline. For each namespace you will need:

You can now have different buckets in different namespaces. Note, as mentioned by @Bobgy in https://github.com/kubeflow/pipelines/issues/4649#issuecomment-926722534 visualization of artifacts is not supported yet. Also note, defaultpipelineroot does not seem to matter. AFAICT defaultpipelineroot is not really supported when using KF with minio (see https://www.kubeflow.org/docs/components/pipelines/pipeline-root/). The configmap kfp-launcher is not needed. Note that normally minio envs are not mounted on ml-pipeline-ui-artifact upon namespace creation.

With that said, working example below

---
apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: minio
    application-crd-id: kubeflow-pipelines
  name: minio
  namespace: {{ $namespace | quote }}
spec:
  selector:
    matchLabels:
      app: minio
      application-crd-id: kubeflow-pipelines
  strategy:
    type: Recreate
  template:
    metadata:
      labels:
        app: minio
        application-crd-id: kubeflow-pipelines
    spec:
      containers:
      - args:
        - gateway
        - s3
        - https://your.minio.here:9000
        env:
        - name: MINIO_ACCESS_KEY
          valueFrom:
            secretKeyRef:
              key: accesskey
              name: mlpipeline-minio-artifact
        - name: MINIO_SECRET_KEY
          valueFrom:
            secretKeyRef:
              key: secretkey
              name: mlpipeline-minio-artifact
        image: gcr.io/ml-pipeline/minio:RELEASE.2019-08-14T20-37-41Z-license-compliance
        name: minio
        ports:
        - containerPort: 9000
        resources:
          requests:
            cpu: 20m
            memory: 100Mi

---
apiVersion: v1
kind: Service
metadata:
  name: minio-service
  namespace: {{ $namespace | quote }}
spec:
  ports:
  - name: http
    port: 9000
    protocol: TCP
    targetPort: 9000
  selector:
    app: minio

---
apiVersion: security.istio.io/v1beta1
kind: AuthorizationPolicy
metadata:
  labels:
    application-crd-id: kubeflow-pipelines
  name: minio-service
  namespace: {{ $namespace | quote }}
spec:
  action: ALLOW
  rules:
  - from:
    - source:
        principals:
        - cluster.local/ns/kubeflow/sa/ml-pipeline
  - from:
    - source:
        principals:
        - cluster.local/ns/kubeflow/sa/ml-pipeline-ui
  - {}
  selector:
    matchLabels:
      app: minio
---
apiVersion: v1
kind: ConfigMap
metadata:
    # if you want to use this config map by default - name it "artifact-repositories"
  name: artifact-repositories
  namespace: {{ $namespace | quote }}
  annotations:
    # v3.0 and after - if you want to use a specific key, put that's key into this annotation
    workflows.argoproj.io/default-artifact-repository: default-v1
data:
  default-v1: |
    archiveLogs: true
    s3:
      endpoint: "minio-service.{{ $namespace }}:9000"
      bucket: {{ $minio_bucket | quote }}
      keyFormat: "artifacts/{{workflow.name}}/{{workflow.creationTimestamp.Y}}/{{workflow.creationTimestamp.m}}/{{workflow.creationTimestamp.d}}/{{pod.name}}"
      insecure: true
      accessKeySecret:
        name: mlpipeline-minio-artifact
        key: accesskey
      secretKeySecret:
        name: mlpipeline-minio-artifact
        key: secretkey

---

apiVersion: apps/v1
kind: Deployment
metadata:
  labels:
    app: ml-pipeline-ui-artifact
  name: ml-pipeline-ui-artifact
  namespace: {{ $namespace | quote }}
spec:
  selector:
    matchLabels:
      app: ml-pipeline-ui-artifact
  template:
    metadata:
      labels:
        app: ml-pipeline-ui-artifact
    spec:
      containers:
      - env:
        - name: MINIO_ACCESS_KEY
          valueFrom:
            secretKeyRef:
              key: accesskey
              name: mlpipeline-minio-artifact
        - name: MINIO_SECRET_KEY
          valueFrom:
            secretKeyRef:
              key: secretkey
              name: mlpipeline-minio-artifact
        image: gcr.io/ml-pipeline/frontend:1.7.0
        imagePullPolicy: IfNotPresent
        name: ml-pipeline-ui-artifact
        ports:
        - containerPort: 3000
          protocol: TCP
        resources:
          limits:
            cpu: 100m
            memory: 500Mi
          requests:
            cpu: 10m
            memory: 70Mi
      serviceAccountName: default-editor

---
juliusvonkohout commented 3 years ago

@jacobmalmberg I have also found a way to use the default minio with one bucket per namespace and different passwords. But I think in the long term one minio per user namespace is better. I will automate the per namespace minio in November.

So according to https://github.com/kubeflow/pipelines/issues/4649#issuecomment-926722534 and https://github.com/kubeflow/pipelines/issues/4649#issuecomment-927003986 we can use minio for v1 andv2-compatible pipelines. This will be enough for the next year I guess, especially since V2 is still in development. I will just disable ml-metadata in the kubeflow namespace and delete any related configurations to prevent data lleaks (at least for my company installation)

@jacobmalmberg is the visualization not even working when applying https://github.com/kubeflow/pipelines/pull/5864 ?

Regarding https://github.com/kubeflow/pipelines/issues/4649#issuecomment-926722534 @bobgy and @zijianjoy is there an issue, pull request or something I can do to help you fix the visualization?

If all of this goes as planned and the visualization is fixed, I can create pull request for those changes and we have proper isolated v1 and v2-compatible pipelines while @bobgy continues the development of V2 and hopefully finds a solution for proper security / user installation from the beginning on.

jacobmalmberg commented 3 years ago

@juliusvonkohout it does not work even when applying #5864. I think the problem is that in the user namespaces I use different minio credentials / buckets than I do in the kubeflow namespace. If I have minio credentials in a user namespace that are identical to the credentials that are in the kubeflow namespace visuaization works. Looking at the istio-ingress log below I see that ml-pipeline-ui.kubeflow.svc.cluster.local is trying to get the artifact. This probably doesn't work because the credentials mounted to the ml-pipeline-ui pod in the kubeflow namespace are different from the minio credentials in the user namespace.

When clicking on the artifact in the browser the output is

Failed to get object in bucket $BUCKETNAME at path $PATH S3Error: 
The access key ID you provided does not exist in our records.

Log from istio-ingress gateway

[2021-10-22T07:24:27.529Z] "GET $ARTIFACT_PATH HTTP/1.1" 500 - via_upstream - "-" 0 247 16 14
"10.7.188.90,10.42.3.0" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:93.0) Gecko/20100101 Firefox/93.0"
"94e5112c-dfab-90b9-a4f9-deeb178783f7" "$ADDRESS "10.42.9.127:3000" 
outbound|80||ml-pipeline-ui.kubeflow.svc.cluster.local 10.42.8.42:42230 10.42.8.42:8080 10.42.3.0:42108
vinayan3 commented 3 years ago

I've hit a need for having a different storage bucket per namespace. Each namespace represents a different part of the org which have their data retention policies and budgets.

Therefore, being able to set this per namespace would be really useful.

I've been hitting the issue where I need to add a secret with the AWS s3 creds per namespace to get the visualization server working. I have a small script that runs after creating a new namespace(profile).