Move cert-manager execution to a new pod #716
Conversation
kubevirt-bot
added
dco-signoff: yes
kubevirt-bot
added
release-note
qinqon
requested review from
rhrazdil and
RamLavi
and removed request for
yboaron
| ports: | ||
| - containerPort: 8443 | ||
| - containerPort: 9443 |
There was a problem hiding this comment.
Why are we changing this?
There was a problem hiding this comment.
We are going to use directly the webhook server from controller-runtime so we just use the defaults there since there are good enough for us and we don't have to add more code to change it to 8443, same happends to TLS cert directory.
|
/lgtm |
|
/hold Let's do a test PR at CNAO to verify that it's happy about this. |
kubevirt-bot
added
the
do-not-merge/hold
| ports: | ||
| - containerPort: 8443 | ||
| - containerPort: 9443 |
There was a problem hiding this comment.
will it have future value to put this in a env var (prevent future collisions)?
There was a problem hiding this comment.
we don't run it with network=host so is not going to collision, I doubt we are going to have multiple containers at the same pod with the same port.
| volumeMounts: | ||
| - name: tls-key-pair | ||
| readOnly: true | ||
| mountPath: /etc/webhook/certs | ||
| mountPath: /tmp/k8s-webhook-server/serving-certs/ |
There was a problem hiding this comment.
genereally speaking, why in /tmp/...?
There was a problem hiding this comment.
Is the default value from controller-runtime webhook server, even without /tmp/ if pod is restarted certs will be mounted too.
|
/retest |
kaw is just a little cert-manager specialized on webhooks, where or how do you deploy is user's responsability. |
qinqon
force-pushed
the
cert-manager-pod
branch
from
4ff33d9 to
ada5f0e
Compare
|
Testing it a CNAO here kubevirt/cluster-network-addons-operator#806 |
|
/hold cancel Test PR at CNAO is working fine kubevirt/cluster-network-addons-operator#806 |
kubevirt-bot
removed
the
do-not-merge/hold
deploy/handler/operator.yaml
Outdated
| app: kubernetes-nmstate | ||
| component: kubernetes-nmstate-cert-manager | ||
| spec: | ||
| replicas: 2 |
There was a problem hiding this comment.
nmstate already has quite a lot of moving parts. We don't mind downtime of couple of minutes in case one of the nodes running this goes down, do we? If not, could we keep it on a single replica? To save memory and CPU.
Currectly the cert-manager from kube-admission-webhook is running whithing the webhook server itself, this means that we need to create a placeholder secret and also a very specific readiness probe has to be used to ensure that secret is properly rotated at start up. This change introduces a new pod runing the cert-manager code so we don't need to create the placeholder secret (the webhook pod will not start until the secret is not created) and we don't need a readiness probess since secret will be correct form the beginning. Also start up is faster since webhook pods start at the very moment the secret is created by cert-manager pod. Signed-off-by: Quique Llorente <[email protected]>
qinqon
force-pushed
the
cert-manager-pod
branch
from
ada5f0e to
7405cd2
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: phoracek The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
|
@qinqon: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Is this a BUG FIX or a FEATURE ?:
What this PR does / why we need it:
Currectly the cert-manager from kube-admission-webhook is running
whithing the webhook server itself, this means that we need to create a
placeholder secret and also a very specific readiness probe has to be
used to ensure that secret is properly rotated at start up. This change
introduces a new pod runing the cert-manager code so we don't need to
create the placeholder secret (the webhook pod will not start until the
secret is not created) and we don't need a readiness probess since
secret will be correct form the beginning.
Also start up is faster since webhook pods start at the very moment the
secret is created by cert-manager pod.
Special notes for your reviewer:
Release note: