feat(backend): Support separate artifact repository for each namespace

[juliusvonkohout]

Description of your changes:

This fixes #4649

The only limitation is that metrics are not rendered by the UI for whatever reason as shown in #4649 (comment). But MinIO provides them successfully according to the logs and you can download them from the web interface.

@Bobgy @zijianjoy I mainly used the black formatter. If you want a different python style lease tell me.

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-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign zijianjoy after the PR has been reviewed.
You can assign the PR to them by writing /assign @zijianjoy 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:

• manifests/kustomize/OWNERS

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

[juliusvonkohout]

i also need to use the local instead of the kubeflow namespace to generate the minio secret. So it must happen in

    class Controller(BaseHTTPRequestHandler):
        def sync(self, parent, children):
            # parent is a namespace
            namespace = parent.get("metadata", {}).get("name")

Please tell me if i am allowed to move the minio secret settings into that function?

[juliusvonkohout]

@jacobmalmberg maybe it fails here

pipelines/frontend/src/apis/run/api.ts

Line 935 in 2768705

const localVarPath = `/apis/v1beta1/runs/{run_id}:reportMetrics`.replace(

These files are also interesing https://github.com/kubeflow/pipelines/blob/master/frontend/src/components/tabs/MetricsTab.tsx https://github.com/kubeflow/pipelines/blob/master/frontend/src/components/viewers/MetricsVisualizations.tsx

Maybe the persistenceagent

pipelines/backend/src/agent/persistence/worker/metrics_reporter.go

Line 129 in 6b7adfa

func (r MetricsReporter) readNodeMetricsJSONOrEmpty(runID string, nodeStatus workflowapi.NodeStatus) (string, error) {

does not use the ml-pipeline-ui-artifact proxy in the users namespace.

[jacobmalmberg]

@juliusvonkohout Entirely possible! I probably won't have time to dig in deeper for the foreseeable future, though.

[juliusvonkohout]

@Bobgy gy @jacobmalmberg @ca-scribner
There is a really strange design decision in kubeflow which will prevent this pull request from working properly. ml-pieline-ui uses (#3554) the artifact proxy in the users namespace ml-pipeline-ui-artifact. ml-pipeline-ui-artifact uses the proper local minio instance. ml-pipeline(apiserver) does not use that proxy (

pipelines/backend/src/apiserver/storage/minio_client.go

Line 38 in 6b7adfa

func (c *MinioClient) GetObject(bucketName, objectName string, opts minio.GetObjectOptions) (io.Reader, error) {

) and fails to fetch any artifacts because they are not in the global minio instance. This results in no mplpipeline-metrics artifact being found by the ml-pipeline-persistenceagent when it asks mp-pipeline(apiserver) for it. Therefore the persistenceagent doe snot report the metrics into the mysql database mlpipeline in the table run_metrics. And of course the API server becomes useless for artifacts in general. A proper design would be if the apiserver would use the artifact proxy and everyone else, especially ml-pipeline-ui would use the apiserver only instead of doing stuff manually.

This means one either has to repair this structural deficit or use a global scalable minio instance with multiple buckets and different passwords per bucket. I will test whether the apiserver can get artifacts from different buckets per user.

[juliusvonkohout]

@Bobgy @ca-scribner @jacobmalmberg this is my design proposal:

We keep the single Minio instance and use bucket name = profile name = namespace name because of the apiserver limitations explained in #7219 (comment).

Detailed explanation:

pipelines/backend/src/common/util/workflow.go

Line 292 in 6b7adfa

func (w *Workflow) FindObjectStoreArtifactKeyOrEmpty(nodeID string, artifactName string) string {

searches for the artifact key "mlpipeline-metrics" in the workflow manifest.
This value e.g. 'artifacts/bash-pipeline-cqkb6/2022/01/27/bash-pipeline-cqkb6-310116140/mlpipeline-ui-metadata.tgz' is returned here #

pipelines/backend/src/apiserver/resource/resource_manager.go

Line 1106 in 6b7adfa

artifactPath := workflow.FindObjectStoreArtifactKeyOrEmpty(nodeID, artifactName)

So there is no bucket included, just the postfix

pipelines/backend/src/apiserver/storage/object_store.go

Line 81 in 6b7adfa

func (m *MinioObjectStore) GetFile(filePath string) ([]byte, error) {

It is set by default to the environment variable OBJECTSTORECONFIG_BUCKETNAME ("mlpipeline")

pipelines/backend/src/apiserver/client_manager.go

Line 397 in 6b7adfa

bucketName := common.GetStringConfigWithDefault("ObjectStoreConfig.BucketName", os.Getenv(pipelineBucketName))

pipelines/backend/src/apiserver/client_manager.go

Line 405 in 6b7adfa

return storage.NewMinioObjectStore(&storage.MinioClient{Client: minioClient}, bucketName, pipelinePath, disableMultipart)

So we have to change it somehow to use the name of the users profile/namespace (not the user which might contain forbidden letters like @) of the request as bucketname. This elegant solution does not need changes in the python SDK, we can keep the artifact keys without bucket names in the workflow manifest.

apiVersion: kubeflow.org/v1
kind: Profile
metadata:
  name: julius-vonkohout # this will be the namespace and bucket name
  resourceVersion: '463979994'
  uid: 372f46ea-ea7f-43dc-8cf6-c562a461ea86
spec:
  owner:
    kind: User
    name: [email protected]
  resourceQuotaSpec: {}

This apiserver log shows that the username and profile/namespace name is there for most requests. But of course not for reading artifacts... this is the most important API call that lacks the namespace specification...
https://www.kubeflow.org/docs/components/pipelines/reference/api/kubeflow-pipeline-api-spec/

I0127 15:27:00.857834       7 util.go:313] Getting user identity...
I0127 15:27:00.857858       7 util.go:323] User: [email protected], ResourceAttributes: &ResourceAttributes{Namespace:julius-vonkohout,Verb:get,Group:pipelines.kubeflow.org,Version:v1beta1,Resource:experiments,Subresource:,Name:default,}
I0127 15:27:00.857876       7 util.go:324] Authorizing request...
I0127 15:27:00.861652       7 util.go:331] Authorized user '[email protected]': &ResourceAttributes{Namespace:julius-vonkohout,Verb:get,Group:pipelines.kubeflow.org,Version:v1beta1,Resource:experiments,Subresource:,Name:default,}
I0127 15:27:00.864767       7 interceptor.go:37] /api.ExperimentService/GetExperiment handler finished
I0127 15:29:11.847320       7 interceptor.go:29] /api.RunService/ReadArtifact handler starting
I0127 15:29:11.856556       7 interceptor.go:37] /api.RunService/ReadArtifact handler finished
I0127 15:29:13.092830       7 interceptor.go:29] /api.RunService/ReadArtifact handler starting
I0127 15:29:13.100022       7 interceptor.go:37] /api.RunService/ReadArtifact handler finished
I0127 15:29:14.410155       7 interceptor.go:29] /api.RunService/ReadArtifact handler starting
I0127 15:29:14.419499       7 interceptor.go:37] /api.RunService/ReadArtifact handler finished
I0127 15:29:15.713893       7 interceptor.go:29] /api.RunService/ReadArtifact handler starting
I0127 15:29:15.721097       7 interceptor.go:37] /api.RunService/ReadArtifact handler finished

We really have to extend the API server artifacts API with the resource_reference_key.type=NAMESPACE and resource_reference_key.id=pavol-bauer. Somehow all other APIs do that properly, so it should be copy and paste. This is because we can share namespaces and e.g. Julius could ask for artifacts from namespace pavol-bauer (if it is shared) and julius-vonkohout. Trying to guess the right namespace/bucket then would be a nightmare.

So we just use the namespace ResourceAttribute from the request as bucket name as the other APIs do. If there is no namespace in the request we just use the default bucket from the environment variable (mlpipeline) for backwards compatibility or "shared" pipeline results. This is handled the same way for namespaced pipeline definitions (YAML files) in the API server. We need to slightly modify the persistenceagent too then. It must send the namespace of the workflow in the request to the API server when it wants to check for "mlpipeline-metrics" artifacts to report them later on as run_metrics. At some point in the future also those run metrics should be namespaced the same way we handle namespaced pipeline definitions.

At this point we still have to create the buckets, passwords and bucket policies manually.

We can merge the kubeflow-pipelines-profile-controller code in this pull request into https://github.com/kubeflow/kubeflow/tree/master/components/profile-controller to get rid of metacontroller with cluster-admin rights and two separate controllers as wanted by @thesuperzapper in #6629 (comment) (the profileresourcetemplate will make it way more simple in the long term). Of course we will ditch the local PVC and MinIO instance for the global one in the kubeflow namespace, but we will keep the namespace specific passwords and change the bucket name. The ml-pipeline-ui will continue to work properly anyway because it uses the artifact proxy in the users namespace. Hopefully we can also get rid of that in the future...

We just need to include a routine that creates an S3 bucket, user, policy, quota and password on profile creation and removes it again on profile deletion. The password generation can be obtained from the kubeflow-pipelines-profile-controller code in this pull request.

I examined the design proposal with a security advisor. If Alice shares a namespace with bob and then unshares it bob might have extracted and saved the minio, default-editor-token-xxxxx, default-viewer-token-xxxxx and other secrets. Bob could use it to still access alices minio bucket or the whole namespace with the serviceaccount. So ALL automatically (non-user) created secrets have to be deleted on rolebinding changes. Furthermore the minio password must not be deterministic.

[ca-scribner]

I've only thought about the proposal for a few minutes, but I like it so far. Would be good to have input from someone in the pipelines working group, but this sounds like a good idea to me.

[juliusvonkohout]

I've only thought about the proposal for a few minutes, but I like it so far. Would be good to have input from someone in the pipelines working group, but this sounds like a good idea to me.

@kimwnasptd @thesuperzapper @davidspek
I have found another severe security issue in kubeflow... If alice shares the namespace with bob, bob can start a jupyterlab and extract the serviceaccounttoken. If alice unshares the namespace bob can just use the extracted token to still do what he wants in alices namespace. He can even do so from outside of the cluster. So we also have to delete all default-editor-tokens on a rolebinding change. They will be automatically recreated by the reconciler.

I will implement this too for this PR.

[juliusvonkohout]

this is now handled in #7406