feat(admission-controller,sysdig-deploy): add initial support for cert-manager #1791
Conversation
|
Hi @yurrriq. Thanks for your PR. After inspecting your changes someone with write access to this repo needs |
yurrriq
force-pushed
the
cert-manager-for-admission-controller
branch
3 times, most recently
from
9dd2e86
to
4d3547b
Compare
There was a problem hiding this comment.
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
Certificatefor theValidatingWebhookConfiguration, using the existingClusterIssuer/selfsigning-issuer - don't include any
caBundlein thewebhooks - annotate the
ValidatingWebhookConfigurationsocainjectorwill 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 }} | |||
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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" . }} |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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 }} |
There was a problem hiding this comment.
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
force-pushed
the
cert-manager-for-admission-controller
branch
from
4d3547b
to
2a63aee
Compare
|
CC @mavimo @AlbertoBarba which team should review and merge? I assume we want to port the changes to the cluster-shield chart too |
|
@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. |
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
github-actions
bot
added
the
stale
|
This PR has been closed due to inactivity. |
github-actions
bot
removed
the
stale
What this PR does / why we need it:
Fixes #1761
Checklist
feat(agent,node-analyzer,sysdig-deploy):)