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.

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

Updates to enable PodDefault with ODH #110

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

Updates to enable PodDefault with ODH #110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/admission-webhook/upstream/base/mutating-webhook-configuration.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ webhooks:
name: $(podDefaultsDeploymentName).kubeflow.org
namespaceSelector:
matchLabels:
app.kubernetes.io/part-of: kubeflow-profile
opendatahub.io/dashboard: "true"
Copy link

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 •
edited
Loading

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 •
edited
Loading

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

rules:
- apiGroups:
- ""
Expand Down
1 change: 1 addition & 0 deletions apps/admission-webhook/upstream/overlays/cert-manager/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ bases:

resources:
- certificate.yaml
- namespace.yaml

namespace: kubeflow

Expand Down
4 changes: 4 additions & 0 deletions apps/admission-webhook/upstream/overlays/cert-manager/namespace.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
apiVersion: v1
kind: Namespace
metadata:
name: kubeflow