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

Let kublet plugin run privileged on OpenShift #76

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

empovit
Copy link
Contributor

@empovit empovit commented Feb 18, 2024

On OpenShift, the kublet plugin container won't run without applying the right
security settings that let it run privileged. We solve this by binding the privileged
security context constraints (SCC) role to the DRA driver's service account.

This is done only when the target system has the OpenShift SCC capability in order
to minimise the impact on non-OpenShift platforms.

@empovit empovit force-pushed the openshift-persmissions branch 2 times, most recently from bdf5dd5 to 7fe8b50 Compare February 21, 2024 15:10
@empovit empovit marked this pull request as ready for review February 21, 2024 15:14
Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

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

Thanks for this.

A minor nit, but looks good otherwise.

{{- if .Capabilities.APIVersions.Has "security.openshift.io/v1/SecurityContextConstraints" -}}
---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
Copy link
Collaborator

@klueska klueska Feb 23, 2024

Choose a reason for hiding this comment

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

So you need both this new RoleBinding and the normal ClusterRoleBinding on openshift?:
https://github.com/NVIDIA/k8s-dra-driver/blob/main/deployments/helm/k8s-dra-driver/templates/clusterrolebinding.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to balance between POLP, flexibility, and the intrusiveness of changes here. This solution seems good to me. E.g. the openshift stuff is kept separate and explicit.
I've discarded my previous changes to the ClusterRole.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My question was more about whether we should have the following in the normal ClusterRoleBinding:

{{- if not .Capabilities.APIVersions.Has "security.openshift.io/v1/SecurityContextConstraints" -}}
...
{{- end }}

or if you guys actually need both on openshift.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ClusterRoleBinding was there already, I believe it's required for the normal functioning of the plugin on any K8s-based platform. The new RoleBinding is where OpenShift differs from the others.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK

@empovit empovit force-pushed the openshift-persmissions branch 2 times, most recently from 58bbcc3 to 4632b53 Compare February 24, 2024 18:06
On OpenShift, the kublet plugin container won't run without applying the right
security settings that let it run privileged. We solve this by binding the privileged
security context constraints (SCC) role to the DRA driver's service account.

This is done only when the target system has the OpenShift SCC capability in order
to minimise the impact on non-OpenShift platforms.

Signed-off-by: Vitaliy Emporopulo <[email protected]>
@empovit empovit force-pushed the openshift-persmissions branch from 4632b53 to defe73d Compare February 24, 2024 18:07
@klueska klueska merged commit 887262d into NVIDIA:main Feb 29, 2024
5 checks passed
@empovit empovit deleted the openshift-persmissions branch September 5, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants