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

feat(admission-controller,sysdig-deploy): add initial support for cert-manager #1791

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yurrriq
Copy link
Contributor

@yurrriq yurrriq commented Jun 26, 2024

What this PR does / why we need it:

Fixes #1761

Checklist

  • Title of the PR starts with type and scope, (e.g. feat(agent,node-analyzer,sysdig-deploy):)
  • Chart Version bumped for the respective charts
  • Variables are documented in the README.md (or README.tpl in some charts)
  • Check GithubAction checks (like lint) to avoid merge-check stoppers
  • All test files are added in the tests folder of their respective chart and have a "_test" suffix

Copy link
Contributor

Hi @yurrriq. Thanks for your PR.

After inspecting your changes someone with write access to this repo needs
to approve and run the workflow.

@yurrriq yurrriq force-pushed the cert-manager-for-admission-controller branch 3 times, most recently from 9dd2e86 to 4d3547b Compare June 26, 2024 23:12
Copy link
Contributor Author

@yurrriq yurrriq left a comment

Choose a reason for hiding this comment

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

Given an existing ClusterIssuer/selfsigning-isser:

apiVersion: cert-manager.io/v1
kind: ClusterIssuer
metadata:
  name: selfsigning-issuer
spec:
  selfSigned: {}

... and the following values to a release of sysdig-deploy:

admissionController:
  enabled: true
  webhook:
    ssl:
      certManager:
        enabled: true
        issuer:
          name: selfsigning-issuer

... the sysdig-deploy and admission-controller charts do what I'd expect:

  • create a Certificate for the ValidatingWebhookConfiguration, using the existing ClusterIssuer/selfsigning-issuer
  • don't include any caBundle in the webhooks
  • annotate the ValidatingWebhookConfiguration so cainjector will inject the cert bundle

@@ -411,7 +411,9 @@ webhooks:
name: {{ include "admissionController.webhook.fullname" . }}
path: /validate
port: {{ .Values.webhook.v2.service.port }}
caBundle: {{ .Cert }}
{{- with .Cert }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the laziest way I could think of to avoid reimplementing this template.

metadata:
annotations:
cert-manager.io/inject-ca-from: {{ .Release.Namespace }}/{{ include "admissionController.webhook.fullname" . }}
helm.sh/hook: "post-install, post-upgrade"
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'm not sure what the thinking is behind all the $existingVac logic, so I just skipped it here.

app.kubernetes.io/managed-by: Helm
name: {{ include "admissionController.webhook.fullname" . }}
namespace: {{ include "admissionController.namespace" . }}
{{- include "admissionController.webhookTemplate" . }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I don't inject any Cert value here so the resulting ValidatingWebhookConfiguration manifest won't contain any caBundle as per the _helper.tpl changes above.

{{- include "admissionController.webhook.labels" . | nindent 4 }}
name: {{ include "admissionController.webhook.fullname" . }}
namespace: {{ include "admissionController.namespace" . }}
spec:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a lot we could parameterize here, but I figure we should start simple and can always iterate.

- {{ include "admissionController.webhook.fullname" . }}
- {{ include "admissionController.webhook.fullname" . }}.{{ include "admissionController.namespace" . }}.svc
issuerRef:
{{- with .Values.webhook.ssl.certManager.issuer.group }}
Copy link
Contributor Author

@yurrriq yurrriq Jun 26, 2024

Choose a reason for hiding this comment

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

I've deliberately done it this we so we can easily add a boolean parameter like webhook.ssl.certManager.issuer.create later to determine whether we should create an issuer.

@yurrriq yurrriq force-pushed the cert-manager-for-admission-controller branch from 4d3547b to 2a63aee Compare June 26, 2024 23:26
@airadier
Copy link
Collaborator

airadier commented Jul 5, 2024

CC @mavimo @AlbertoBarba which team should review and merge? I assume we want to port the changes to the cluster-shield chart too

@airadier
Copy link
Collaborator

airadier commented Jul 5, 2024

@yurrriq thanks so much for this contribution! We are going to review internally and also port the changes to the cluster-shield chart, as this is now the official deployment method for Admission Controller.

Thanks again.

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.

Support using cert-manager for admission controller certs
2 participants