Skip to content

Integrate profiles with seaweedfs #3051

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 27, 2025
Merged

Integrate profiles with seaweedfs #3051

merged 4 commits into from
Apr 27, 2025

Conversation

pschoen-itsc
Copy link
Contributor

@pschoen-itsc pschoen-itsc commented Mar 14, 2025 •
edited
Loading

Pull Request Template for Kubeflow Manifests

✏️ Summary of Changes

Use ACLs in seaweedfs to isolate access to artifacts by profile. When a new profile is created, credentials for this profile will be created by seaweedfs and put as secret into the new namespace. ACLs will be set to limited to access from this profile to his subdir in the bucket.

📦 Dependencies

🐛 Related Issues

Don't know if there is currently an open issue

✅ Contributor Checklist

You can join the CNCF Slack and access our meetings at the Kubeflow Community website. Our channel on the CNCF Slack is here #kubeflow-platform.

@pschoen-itsc
Copy link
Contributor Author

@juliusvonkohout I need to do a bit more testing (for example UI components), but the integration between seaweedfs and kf-pipelines works that way.

@juliusvonkohout
Copy link
Member

/retest

juliusvonkohout
juliusvonkohout reviewed Mar 16, 2025
View reviewed changes
experimental/seaweedfs/base/seaweedfs/seaweedfs-networkpolicy.yaml
- namespaceSelector:
matchExpressions:
- key: kubernetes.io/metadata.name
operator: In
values:
- istio-system
- podSelector: {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ml-pipeline-ui and maybe others need it as well without the artifact proxy

juliusvonkohout
juliusvonkohout reviewed Mar 16, 2025
View reviewed changes
@juliusvonkohout juliusvonkohout marked this pull request as ready for review March 16, 2025 18:07
@juliusvonkohout
Copy link
Member

juliusvonkohout commented Mar 16, 2025 •
edited
Loading

@juliusvonkohout I need to do a bit more testing (for example UI components), but the integration between seaweedfs and kf-pipelines works that way.

Thank you very much for the PR. I have added a lot of comments and clarifications.

Now the question is why does the pipeline not stop running.

@juliusvonkohout juliusvonkohout force-pushed the integrate-profiles-with-seaweedfs branch from 29091b6 to 5c1a8ca Compare March 16, 2025 18:17
@google-oss-prow google-oss-prow bot added size/L and removed size/XL labels Apr 11, 2025
@juliusvonkohout
Copy link
Member

Do you mind squashing and signing your commits (DCO) and rebasing to master?

@juliusvonkohout
Copy link
Member

I think you can copy from our new pipeline install script. See also https://github.com/kubeflow/manifests/pull/3093/files and https://github.com/kubeflow/manifests/blob/master/.github/workflows/pipeline_test.yaml as reference.

@juliusvonkohout juliusvonkohout mentioned this pull request Apr 22, 2025
Closed
7 tasks
@pschoen-itsc pschoen-itsc force-pushed the integrate-profiles-with-seaweedfs branch from 06e4502 to 190b8f4 Compare April 25, 2025 15:00
@google-oss-prow google-oss-prow bot added size/L and removed size/XXL labels Apr 25, 2025
@juliusvonkohout
Copy link
Member

Thank you, i just re-enabled the tests

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Apr 25, 2025 •
edited
Loading

Ah i broke the DCO, but you can just squash my test enabling commit and take ownership of it if needed. Otherwise i will fix it later myself

@pschoen-itsc
Enable pipeline again
fc31f52 
Signed-off-by: Patrick Schönthaler <[email protected]>
@pschoen-itsc pschoen-itsc force-pushed the integrate-profiles-with-seaweedfs branch from 2084827 to fc31f52 Compare April 25, 2025 16:54
@google-oss-prow google-oss-prow bot added size/XL and removed size/L labels Apr 25, 2025
@juliusvonkohout
Copy link
Member

juliusvonkohout commented Apr 25, 2025 •
edited
Loading

I think https://github.com/pschoen-itsc/kf-manifests/blob/integrate-profiles-with-seaweedfs/tests/gh-actions/pipeline_test.py or https://github.com/kubeflow/manifests/actions/runs/14669563838/job/41172716531?pr=3051 needs some debugging.

@pschoen-itsc
Adjust pipeline test to new workflow
a02b9e0 
Signed-off-by: Patrick Schönthaler <[email protected]>
milinddethe15
milinddethe15 reviewed Apr 25, 2025
View reviewed changes
@pschoen-itsc
Change instance type for pipeline test
6d229e8 
Signed-off-by: Patrick Schönthaler <[email protected]>
@pschoen-itsc
Fix some formatting
9524ca0 
Signed-off-by: Patrick Schönthaler <[email protected]>
@pschoen-itsc
Copy link
Contributor Author

The End-to-End Integration test succeded before, now it fails without changing anything...
@juliusvonkohout can you just retrigger it?

@milinddethe15
Copy link
Member

/retest

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Apr 27, 2025 •
edited
Loading

Amazing, thank you very much. It it such an improvement of the status quo.

In a follow up PR we should

  1. check whether ubuntu-latest-16-cores is really needed instead of the smaller nodes
  2. test it also in https://github.com/kubeflow/manifests/blob/master/.github/workflows/full_kubeflow_integration_test.yaml
  3. Add security checks to fest for unauthorized access
  4. Adjust the Argo Workflow-Controller configuration map to achieve the same for V1 pipelines (easy, did so before)
  5. See how we can upstream that to KFP
    @akagami-harsh this could be interesting for you.

Thank you everyone!
I am pushing this for 4 years or so and even had google and redhat employees involved, back then even Amazon. It is fundamental for CVEs, maintainabiliy (minio is now stuck for 5 years or so) and hard multi-tenancy as basic requirement for a sane platform. We also had approaches there for several years with minio. it started all in 2020 here kubeflow/pipelines#4649 and went via kubeflow/pipelines#7725 (2022) and #2826 (October 2024) to #3051 (2025). Without that experimental and extended tests it would have been very hard to pull of and coordinate. I want to especially highlight @pschoen-itsc who spent his effort here for the public health sector in Germany where many insurances need hard multi-tenancy to process data.

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Apr 27, 2025
@google-oss-prow Google OSS Prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juliusvonkohout

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:

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

@google-oss-prow google-oss-prow bot merged commit de2bf1e into kubeflow:master Apr 27, 2025
8 of 9 checks passed
This was referenced Apr 27, 2025
Closed
Open
@juliusvonkohout
Copy link
Member

Tracked in #3119

@juliusvonkohout juliusvonkohout mentioned this pull request Jun 10, 2025
Merged
5 tasks
@NohaIhab NohaIhab mentioned this pull request Jun 17, 2025
Open
@pschoen-itsc pschoen-itsc mentioned this pull request Aug 6, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusvonkohout juliusvonkohout juliusvonkohout left review comments

@kimwnasptd kimwnasptd Awaiting requested review from kimwnasptd

+1 more reviewer

@milinddethe15 milinddethe15 milinddethe15 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@juliusvonkohout juliusvonkohout

Labels
approved lgtm size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pschoen-itsc @juliusvonkohout @milinddethe15