Updating the caBundle for the controller webhook #7022
Conversation
ytimocin
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
| @@ -32,7 +33,7 @@ webhooks: | |||
| - admissionReviewVersions: | |||
| - v1 | |||
| clientConfig: | |||
| caBundle: {{ b64enc $ca.Cert }} | |||
| caBundle: {{ $validatingWebhookCaBundle }} | |||
There was a problem hiding this comment.
@ytimocin Can you please make the code consistent with how UCP does? UCP way is more intuitive.
There was a problem hiding this comment.
In UCP's case, we don't need to select a caBundle from an array of webhook configurations: https://github.com/radius-project/radius/blob/main/deploy/Chart/templates/ucp/apiservice.yaml#L42.
The reason I did it this way is that we have a new webhook coming soon and that will also be added to the array of configs. We will need to map the right webhook configuration to the right caBundle.
The only thing that we will need to do for the next webhook is to call the function with the right webhook name.
Let me know if that makes sense @youngbupark.
There was a problem hiding this comment.
For mutatingAdmissionController, we will use existing controller service and share the same endpoint. Then I do not think we need additional cert for mutating controller. Please keep it simple and address what we need to solve.
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
ytimocin
force-pushed
the
ytimocin/updateWebhookCaBundle
branch
2 times, most recently
from
a9fbee4 to
2d1bf17
Compare
ytimocin
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
ytimocin
force-pushed
the
ytimocin/updateWebhookCaBundle
branch
from
2d1bf17 to
243a308
Compare
ytimocin
force-pushed
the
ytimocin/updateWebhookCaBundle
branch
from
243a308 to
46d782c
Compare
| {{ end }} | ||
| tls.crt: {{ include "secrets.lookup" (dict "secret" "controller-cert" "namespace" .Release.Namespace "key" "tls.crt" "defaultValue" $cert.Cert) }} | ||
| tls.key: {{ include "secrets.lookup" (dict "secret" "controller-cert" "namespace" .Release.Namespace "key" "tls.key" "defaultValue" $cert.Key) }} | ||
| ca.crt: {{ include "secrets.lookup" (dict "secret" "controller-cert" "namespace" .Release.Namespace "key" "ca.crt" "defaultValue" $ca.Cert) }} |
There was a problem hiding this comment.
Why do we need this ? this is dupe with tls.crt
There was a problem hiding this comment.
tls.crt uses $cert.Cert while ca.crt uses $ca.Cert. They are different.
There was a problem hiding this comment.
ca.crt is also added here: https://github.com/bitnami/charts/blob/882140cfc5318420fdf563b77201f7ee5f67ca8f/bitnami/rabbitmq-cluster-operator/templates/messaging-topology-operator/validating-webhook-configuration.yaml#L29. That is what we use for the caBundle.
We need a way to access existing ca.crt to use it in the caBundle.
| {{ if $existingSecret }}tls.key: {{ index $existingSecret.data "tls.key" }} | ||
| {{ else }}tls.key: {{ b64enc $cert.Key }} | ||
| {{ end }} | ||
| tls.crt: {{ include "secrets.lookup" (dict "secret" "controller-cert" "namespace" .Release.Namespace "key" "tls.crt" "defaultValue" $cert.Cert) }} |
There was a problem hiding this comment.
We do not need to create Secret if secret exists. So we can simplify it like below.
{{- if not $existingSecret }} // ---> UPDATE THIS
apiVersion: v1
kind: Secret
metadata:
name: controller-cert
namespace: {{ .Release.Namespace }}
labels:
app.kubernetes.io/name: controller
app.kubernetes.io/part-of: radius
data:
tls.crt: {{ b64enc $cert.Cert }}
tls.key: {{ b64enc $cert.Key }}
{{- end }}
There was a problem hiding this comment.
secrets.lookup function checks if there is already a Secret and, if there is, it uses the existing one. Like here.
secrets.lookup gets the secret name and the namespace that secret should be in, checks if it exists, if it does it returns the existing secret. If not, it returns the default value provided. In this case $cert.Cert, $cert.Key, and $ca.Cert.
There was a problem hiding this comment.
If we do it this ({{- if not $existingSecret }}) way, then Secret gets deleted when we do --reinstall. In the --reinstall case, the $existingSecret is not nil, that part is missing from the template. Let me know if that makes sense. Happy to sync offline.
And I did another test, when we do --reinstall Secret doesn't get recreated with this way: https://github.com/radius-project/radius/pull/7022/files#diff-dacf6ba48c5b451f0a3a55b9978b57991a50fb27fc2da1c4785e09e52adc4b09R18-R20.
ytimocin
force-pushed
the
ytimocin/updateWebhookCaBundle
branch
2 times, most recently
from
066d0e2 to
4fa6441
Compare
ytimocin
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
| Reuses the value from an existing secret, otherwise sets its value to a default value. | ||
|
|
||
| Usage: | ||
| {{ include "secrets.lookup" (dict "secret" "secret-name" "namespace" "ns-name" "key" "key-name" "defaultValue" "default-secret") }} |
There was a problem hiding this comment.
nit: perhaps ...."defaultValue" "default-secretvalue") or ...."defaultValue" "default-value") is more accurate
Signed-off-by: ytimocin <[email protected]>
ytimocin
force-pushed
the
ytimocin/updateWebhookCaBundle
branch
from
4fa6441 to
7847b14
Compare
ytimocin
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
# Description Updating the caBundle for the controller webhook ## Type of change - This pull request fixes a bug in Radius and has an approved issue (issue link required). Fixes: radius-project#6989 Signed-off-by: ytimocin <[email protected]> Signed-off-by: willdavsmith <[email protected]>
Description
Updating the caBundle for the controller webhook
Type of change
Fixes: "x509: certificate signed by unknown authority" error during webhook call #6989