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

Updates to enable PodDefault with ODH #110

Open
wants to merge 1 commit into
base: v1.6-branch-openshift
Choose a base branch
from

Conversation

ykoyfman
Copy link

Which issue is resolved by this Pull Request:
Resolves opendatahub-io/kubeflow#61

Description of your changes:

Enable PodDefaults to work after being installed with kustomize build apps/admission-webhook/upstream/overlays/cert-manager | oc apply -f -

  • Create kubeflow namespace
  • Update namespace label used by admission webhook to match what's being created by ODH

Verified environment variables are being injected into notebook pods.

@openshift-ci openshift-ci bot requested review from LaVLaS and VaishnaviHire June 15, 2023 20:41
@openshift-ci
Copy link

openshift-ci bot commented Jun 15, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vaishnavihire for approval. For more information see the Kubernetes Code Review Process.

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

@openshift-ci
Copy link

openshift-ci bot commented Jun 15, 2023

Hi @ykoyfman. Thanks for your PR.

I'm waiting for a opendatahub-io 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.

@@ -16,7 +16,7 @@ webhooks:
name: $(podDefaultsDeploymentName).kubeflow.org
namespaceSelector:
matchLabels:
app.kubernetes.io/part-of: kubeflow-profile
opendatahub.io/dashboard: "true"

Choose a reason for hiding this comment

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

I know the dashboard team is looking at moving away from making this label required on "data science project" namespaces. This may cause issues in the future if they do move away from requiring this.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for letting me know. Is there a better alternative for a consistent matching label to use here?

Copy link

@shalberd shalberd Jun 20, 2023

Choose a reason for hiding this comment

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

They are mostly looking at moving away from labels on data science project namespaces because setting labels via self-provisioning type of users in projects / ProjectRequests is impossible and namespace-access for setting the labels after project creation is really too much. By the way, I don't understand why RedHat does not provide a way to set non-critical and non-system labels on project creation, be it via oc new-project or ProjectRequest / api.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could achieve that with project templates. There would be the standard default template for std projects, and a special template for DSPs that would set labels and anything else needed. That would be the template called by dashboard when creating new DSP, with:

projectRequestTemplate:
    name: <template_name>

in the specs of the project.

Copy link

@shalberd shalberd Jun 21, 2023

Choose a reason for hiding this comment

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

I in the past experienced trouble with anything but the standard few labels on a template:

https://docs.openshift.com/container-platform/4.10/applications/projects/configuring-project-creation.html#about-project-creation_configuring-project-creation

Same for annotations. The argument at RedHat is: both annotations and labels should not be left to self-provisioners. Leading to only display-name and description being set by the user using odh dashboard with self-provision role.

https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/api/k8s/projects.ts#L40

@andrewballantyne The annotation at level namespace is then set elsewhere.

https://github.com/opendatahub-io/odh-dashboard/blob/main/backend/src/routes/api/namespaces/namespaceUtils.ts#L58

Currently, it is either modify to all namespaces or none is allowed, with filtering not allowing openshift or kube namespaces purely on the dashboard code level.

At least in our enterprise, the ability to modify anything namespace-related is forbidden for developers, that is, all authenticated users, unless specified elsewhere. And annotations and labels have to be set at level namespace, not at level project. The ability to modify namespace-labels and annotations is also not implicit in ClusterRole Admin for a given project, nor is it in Clusterrole self-provisioners.

I cannot imagine many cluster-admins given such broad rights to ordinary users, especially since it is all-or-nothing in terms of namespaces.

From what gathered from our cluster admin folks, they have custom admission controllers for level namespace that react to a project being created with a certain prefix and with a certain requester id, in our case a serviceaccount. Then, the custom admission controller adds things like default permissions, labels, annotations to the namespace

If I understand dashboard code currectly currently, data science projects would not work for us in terms of creating projects from odh dashboard. We currently have to go with manual project provisioning and namespace label/annotation edit via a Gitops process. We also use custom node selector and taints and tolerations in namespaces, by the way. Our cluster is multi-tenant, with projects other than odh-related, too.

https://docs.openshift.com/container-platform/4.10/rest_api/authorization_apis/selfsubjectaccessreview-authorization-k8s-io-v1.html#spec-resourceattributes

Because of all those problems, currently, the odh-dashboard serviceaccount sets the label at level namespace, suboptimal.

@LaVLaS any thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, the discussion around making a UI that Trevor noted is the effort behind opendatahub-io/odh-dashboard#1417

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.

Add Support for PodDefault from Notebook Controller
5 participants