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

Add basic CSI driver for the spiffe-helper #166

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft

Add basic CSI driver for the spiffe-helper #166

wants to merge 22 commits into from

Conversation

kfox1111
Copy link
Collaborator

This adds a prototype level helm chart for a virtual csi driver that makes it significantly easier to integrate existing software running on Kubernetes with SPIRE.

This adds a prototype level helm chart for a virtual csi driver that
makes it significantly easier to integrate existing software running
on Kubernetes with SPIRE.

Signed-off-by: Kevin Fox <[email protected]>
@faisal-memon faisal-memon changed the title Add basic CSI driver for the spire-helper Add basic CSI driver for the spiffe-helper Jan 9, 2024
@faisal-memon faisal-memon added this to the 0.18.0 milestone Jan 9, 2024
@kfox1111 kfox1111 added prototype review ready Ready for review but not merge labels Jan 22, 2024
@faisal-memon faisal-memon modified the milestones: 0.18.0, 0.20.0 Mar 7, 2024
@faisal-memon faisal-memon modified the milestones: 0.20.0, 0.22.0 May 14, 2024
{
"name": "kyverno",
"repo": "https://kyverno.github.io/kyverno",
"version": "3.1.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not slip in a policy engine as an out-of-scope "extra" on a commit intended to add a CSI driver.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The whole hook is implemented as a kyverno mutating policy. Its needed to make it work at all.

Totally open to it needing its own golang based webhook implementation in the future, along with associated git repositories, a container, etc, to make it not depend on kyverno, but was looking at getting an implementation of the concept working in as minimal amount of time possible, and that lead to using an existing webhook engine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The expansion of the charts to include Kyverno is something that probably should be discussed with a majority of the maintainers. I understand that it's part of this implementation, but its inclusion should be done with consensus, and at least some commentary on what the alternatives might be.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Completely agree.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I 100% believe Kyverno is NOT the right solution in the long run.

The right place for it though is not clear. There are several potential places

  1. kubernetes is working on gaining cel support for MutatingAdmissionPolicy. This would be the very best place to put it, I think. provided it supports all the needed functionality (unclear). But it isn't supported in k8s yet, let alone in all versions of k8s we support.
  2. It could be added to spire-controller-manager. Another webhook wouldn't be required then.
  3. Its own standalone repo/webhook - needs ssc support

I also don't think we should block users playing around with the concept and getting the api correct so that when we do identify a better way of implementing that api, we can switch out the kyverno based implementation with a better one without affecting users of it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For what it is worth, in our own clusters we manage Kyverno ourself, meaning I probably need a way to disable it here, for the remainder I understand the reason why you are adding it now.

Probably it should be something managed in the controller itself to have the webhook implemented there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It makes use of Kyverno via a policy in the chart, but it doesn't install Kyvverno other then in the tests, to test the policy.

Longer term, it will be switched to kubernetes/kubernetes#127134 once it is a thing.

Copy link
Collaborator

@edwbuck edwbuck left a comment

Choose a reason for hiding this comment

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

A good start. Needs a little cleanup here and there. I'm a bit confused why we need to apply values to clusterpolicy.yaml in code, when it likely should be handled by templating.

@edwbuck
Copy link
Collaborator

edwbuck commented Jun 18, 2024

partially blocked on spiffe/spiffe-helper#118

We need a deeper discussion on whether to bring in Kyverno

@kfox1111
Copy link
Collaborator Author

Ideally, the webhook will be implemented in go or kubernetes mutating cel. A home for that needs to be established.

The api, tests, and chart would be unchanged and should be good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prototype review ready Ready for review but not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants