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

proposal: Guidelines for /contrib components #2286

Merged

Conversation

kimwnasptd
Copy link
Member

Create an initial proposal regarding the expectations we should have around the components in /contrib as well as a deprecation plan for unmaintained components.

I'd like to wait 2 weeks before merging this PR, to gather feedback on the proposal. Then once it's merged I'll do a pass on the components and create issues for the ones that don't meet the requirements. If there won't be any feedback in those issues, then the components will enter a deprecation phase.

@holdenk @Jeffwan @krishnadurai @yanniszark @juliusvonkohout @zijianjoy @gkcalat @zijianjoy @gkcalat @Jeffwan @woop @tedhtchang @Tomcli @animeshsingh @pvaneck @axsaucedo @adriangonz @cliveseldon @ryandawsonuk @ellistarn @yuzisun @cliveseldon @animeshsingh @deadeyegoodwin

cc-ing the release team as well @annajung @DomFleischmann @yubozhao @surajkota @DnPlas @jbottum

@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kimwnasptd

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

@kimwnasptd
Copy link
Member Author

/hold

Create an initial proposal regarding the expectations we should have
around the components in /contrib as well as a deprecation plan for
unmaintained components.

Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd kimwnasptd force-pushed the feature-kimwnasptd-contrib-guidelines branch from 9677d1d to e18e446 Compare September 27, 2022 11:20
@juliusvonkohout
Copy link
Member

juliusvonkohout commented Sep 27, 2022

I think "The K8s version and KF component versions that the component needs" is not always needed and can be ued as excuse to not maintain stuff. We should expect also /contrib to work with the recent Kubeflow version. So if you remove everything that has not been touched for 2 years and is most likely broken, you get

@sylus seems to have interest in upgrading seldon according to #2207 (comment)
and also @nithin8702 #2194 (comment)

@kimwnasptd kimwnasptd force-pushed the feature-kimwnasptd-contrib-guidelines branch from cfb01f9 to 7227249 Compare October 6, 2022 14:48
Signed-off-by: Kimonas Sotirchos <[email protected]>
@jbottum
Copy link

jbottum commented Oct 6, 2022

@kimwnasptd we have two related sessions at the Kubeflow Summit on Day 2, 1:00 pm - 1:30 pm - Manifests, and
1:30 pm - 2:00 pm - Components (How do we take care non core components). Would you please consider facilitating the Manifest session? I think this PR would be a good topic for these sessions and it would be great if you could review it in one of those two sessions.

@jbottum
Copy link

jbottum commented Oct 6, 2022

@kimwnasptd I like this proposal. Some comments in the Component Requirements section. Should we consider enhancing item 3. to include the need for a test plan and a verification that it has been executed for the latest release? Should there be an item 5 that would address how users might get support i.e. via a Kubeflow slack channel ?

@kimwnasptd
Copy link
Member Author

@jbottum

Would you please consider facilitating the Manifest session? I think this PR would be a good topic for these sessions and it would be great if you could review it in one of those two sessions

Yes, this can definitely be a topic to go through as well as how to make it easier for new components to integrate with Kubeflow.

Should we consider enhancing item 3. to include the need for a test plan and a verification that it has been executed for the latest release?

That's a pretty good suggestion actually, to expect a series of steps [can later become a script, that we can use to validate a component is working as expected. This also links to what @annajung proposed above https://github.com//pull/2286#discussion_r981484770

I'll add a new bullet for testing that asks maintainers to provide:

  1. A simple script that runs and ensures the component is working as expected. It can be something as simple as creating a CustomResource object and wait for it to become ready
  2. Work with the Manifests WG to ensure that there's some automation for that test

Should there be an item 5 that would address how users might get support i.e. via a Kubeflow slack channel ?

I think there's no need for this one. As long as the OWNERS file is up to date we can expect issues to be opened and we can ping the maintainers in these issues.

Signed-off-by: Kimonas Sotirchos <[email protected]>
@kimwnasptd
Copy link
Member Author

At this point I believe we've addressed all the provided feedback. Also thank you everyone for your valuable input!

I think the documented requirements will really help ensure our components will be able to stand the test of time. Next work items here will be to expose more documentation on what are the actual technical details for a component to be "integrated" with the rest of the KF components. I.e.

  • Namespace isolation
  • User permissions on user-namespaces, for any new CustomResources that will be created
  • Servers that act on behalf of users will need to respect the user in the kubeflow-userid header, and use SubjectAccessReviews
  • Integration with the CentralDashboard
  • Possibility for a KFP component [if needed]

I'll keep this PR open for 2 more days to see if we'll have any last feedback and then I'll merge the PR. Thank you for your time everyone!

@juliusvonkohout
Copy link
Member

At this point I believe we've addressed all the provided feedback. Also thank you everyone for your valuable input!

I think the documented requirements will really help ensure our components will be able to stand the test of time. Next work items here will be to expose more documentation on what are the actual technical details for a component to be "integrated" with the rest of the KF components. I.e.

* Namespace isolation

* User permissions on user-namespaces, for any new CustomResources that will be created

* Servers that act on behalf of users will need to respect the user in the `kubeflow-userid` header, and use SubjectAccessReviews

* Integration with the CentralDashboard

* Possibility for a KFP component [if needed]

I'll keep this PR open for 2 more days to see if we'll have any last feedback and then I'll merge the PR. Thank you for your time everyone!

Yes, definitely no root containers / runasnonroot. They must run with the offical kubernetes podsecuritystandards restricted set https://kubernetes.io/docs/concepts/security/pod-security-standards/#restricted. That is currently the case if you use istio-cni and all other official components. I worked over the last two years with almost all WGs to make this possible. Only this enables us to significantly harden Kubeflow in the future and make it ready for enterprise consumption.

@juliusvonkohout
Copy link
Member

juliusvonkohout commented Oct 19, 2022

@kimwnasptd https://kubernetes.io/docs/concepts/workloads/pods/user-namespaces/ will be incredible exiting. I am still discussing with security researchers, but it could once and for all remove the runasnonroot problem. The PVC support will come in the future https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/127-user-namespaces/README.md#phase-2-pods-with-volumes. But then in around a year, when modern distributions are using that, we could at least consider runasnonroot exceptions e.g. for MLflow from infinstor in the Kubeflow profile namespaces. Maybe i will use kyverno then and add two policies into contrib: The offical restricted PSS and and another one which allows root (not privileged) and enforces hostUsers=false.

@kimwnasptd
Copy link
Member Author

At this point I believe we can merge this PR and start cleaning up the /contrib dir

/cc @kubeflow/wg-manifests-leads

/hold cancel

@google-oss-prow google-oss-prow bot requested a review from a team October 19, 2022 16:10
@elikatsis
Copy link
Member

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Oct 19, 2022
@google-oss-prow google-oss-prow bot merged commit 9616c2d into kubeflow:master Oct 19, 2022
@kimwnasptd kimwnasptd deleted the feature-kimwnasptd-contrib-guidelines branch October 19, 2022 16:56
kevin85421 pushed a commit to juliusvonkohout/manifests that referenced this pull request Feb 28, 2023
* proposal: Guidelines for /contrib components

Create an initial proposal regarding the expectations we should have
around the components in /contrib as well as a deprecation plan for
unmaintained components.

Signed-off-by: Kimonas Sotirchos <[email protected]>

* review: Document that components should be working with current KF

Signed-off-by: Kimonas Sotirchos <[email protected]>

* review: Expect link to component's documentation

Signed-off-by: Kimonas Sotirchos <[email protected]>

* review: Expect an UPGRADE.md file as well

Signed-off-by: Kimonas Sotirchos <[email protected]>

* review: Fix typos

Signed-off-by: Kimonas Sotirchos <[email protected]>

* review: Add some testing requirements

Signed-off-by: Kimonas Sotirchos <[email protected]>

Signed-off-by: Kimonas Sotirchos <[email protected]>
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.

7 participants