cert-manager
Add support for generating certificates with helm
This removes the hard dependency on cert-manager by allowing users to choose not to create a Certificate.
The extremely long lifetime of the cert is to remove the chance of it expiring in the lifetime of a typical cluster. Users who want rotation would be well advised to use cert-manager, which remains the default
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: SgtCoDFish
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [SgtCoDFish]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@SgtCoDFish I had a quick go from your branch with:
kind create cluster --name trust
gh repo clone SgtCoDFish/cert-manager-trust-manager
cd cert-manager-trust-manager
gco gco isolatedcert
k create ns cert-manager
helm install -n cert-manager trust-manager ./deploy/charts/trust-manager --set issueIsolatedCert=true
# Get before secret
k get secret -n cert-manager trust-manager-tls -o yaml > install-cert.yaml
# check upgrade
helm diff upgrade -n cert-manager trust-manager ./deploy/charts/trust-manager/ --set issueIsolatedCert=true
# This also seems to give the same diff result
helm diff upgrade -n cert-manager trust-manager ./deploy/charts/trust-manager/ --reuse-values
# get after secret
k get secret -n cert-manager trust-manager-tls -o yaml > after-cert.yaml
When I go to upgrade it looks like it will generate a new CA / cert, from a helm diff:
- caBundle: "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURKakNDQWc2Z0F3SUJBZ0lSQVBDSllpWXpoWVFSOXNhYThiL29uSEF3RFFZSktvWklodmNOQVFFTEJRQXcKSFRFYk1Ca0dBMVVFQXd3U0tpNWpaWEowTFcxaGJtRm5aWEl1YzNaak1CNFhEVEl6TURneE1ERXdNemMxTkZvWApEVFE0TURnd016RXdNemMxTkZvd0hURWJNQmtHQTFVRUF3d1NLaTVqWlhKMExXMWhibUZuWlhJdWMzWmpNSUlCCklqQU5CZ2txaGtpRzl3MEJBUUVGQUFPQ0FROEFNSUlCQ2dLQ0FRRUFxMXI1ZFpGL1RRcEh5UW5Gb0U5cE1mcloKZU1jYUZJK1pLb2t2UEdaU3NtSHhSSEh3YlZKbDNOZlRreUllZkxyeU5vZHloeTI0S3AzN1lDRmpyWFFqRDkwTwpTQWJoRjNJU0dzUndoNFZWa3RPTS91ZjlhUU1JL0RWb3hjK0VmMEY4Q3B1Rkk1NFNzT3d2S2F1bHhDVkZNS3V2CnpQTnNCVkRmbWh4c2kvOGxybkZ4NHJEM3RsQVFaWHllcnRpdTFSNlAvdENpRGU2SzVFdWE4ZzlQdnFpdnJIVnYKMVB6REx0RDdkN2Z6MGpYYkVpd0xTenQwMGpIcWJjZTZXSEdDS0dETlUwQ2dZUEswMzExTHNhdGRMeWdqRFFkZgowdThnd1d1cWR3K2NtT21BVllVMmh6am82VW5sajduK1RoV2V6djRreHpKOUdpQnRsQTBNNGdLK3BTNEpRUUlECkFRQUJvMkV3WHpBT0JnTlZIUThCQWY4RUJBTUNBcVF3SFFZRFZSMGxCQll3RkFZSUt3WUJCUVVIQXdFR0NDc0cKQVFVRkJ3TUNNQThHQTFVZEV3RUIvd1FGTUFNQkFmOHdIUVlEVlIwT0JCWUVGRTc3SFdvQlIxWUpSL2VZTllkbApRZ3BrdHJPdE1BMEdDU3FHU0liM0RRRUJDd1VBQTRJQkFRQlhtekJuc2hDdW5oSkVWb1NUdkF2b1pJYU5peGI2Cjc0QTJQVzgzVzJoY1ZKbUlPTWFJdHk0VHA3Ymk5RVRQaGc4M1NtbFc4SlhkeTQxZWp3ZEU4MXA5azN3QlBEcEgKU1dRNG9iOXpnazJVUUhVdGVKQ09ISnR6REFNUkM3dDBlb1MvWlJLVTJRUi9Zd1EvL2wrL3RIck5BUjdPcGI5WAovOUZkdWNVRlZjY2h1UGxpb1pOc01mSkFKTU44SUJnbmIzUmFoeVJranlkdjBuaStpQTlGdEhJK1JjNWh2eWNGCi91UXByN2xyYi9rR2VKVmxmdFFZYys1VldCc1ZwVEEzckdFVGRoS1I1UWluVENXek1kb3NJYWp0Y1JRYW1mVk4KNGdyZDh5Z1dqdy9CTWdFMDM4Qm80U011TTFiaHp1TmF3eHlza2I1Snpoa3VrcUFiMTl1azhIZEkKLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo="
+ caBundle: "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURKakNDQWc2Z0F3SUJBZ0lSQVA1dWxUSGlPTEEwUjlJS2xNQWhlVmd3RFFZSktvWklodmNOQVFFTEJRQXcKSFRFYk1Ca0dBMVVFQXd3U0tpNWpaWEowTFcxaGJtRm5aWEl1YzNaak1CNFhEVEl6TURneE1ERXdNemt6TjFvWApEVFE0TURnd016RXdNemt6TjFvd0hURWJNQmtHQTFVRUF3d1NLaTVqWlhKMExXMWhibUZuWlhJdWMzWmpNSUlCCklqQU5CZ2txaGtpRzl3MEJBUUVGQUFPQ0FROEFNSUlCQ2dLQ0FRRUE1alJVbGJOVVNJd3NHdm1uNW5rYzZZa0wKMTcyaW9VSW0zLzg0UzhpNStFNEpQeUFlK2tOaEh2ZGZqRll4RnA4dmcwQ0RJQ1QwMi92MlNwNENzT0plSlhXTApjYzM0Z2NxQTlSbW5yTW5UK2pSbEI1dTZXLzRTcTlZRHpxNVZJMUlrQ3RiYmFSY1MyTHprRXpHR1pkRWxNT2MvCnBvZ0M1b1NuQzFsOVdQVEttckt2Ty9xSW9RaEpoZ1hIemV6cmVrMmVEQUh6OFBSK2JQbm1OVXVld2YzUEZtNzIKSTE3QUtUYlljK2tSOHRZdmFubWR4QVN1UXBuM2JvOEpWMHZiMmxKSm9LTWxuRmtWdlFRMWpwWDRVVE8wYStOKwoxVUtrMStpOTVnN1VrTlRWNWJuSEpwUEgxU01Xd0xNT0xjdkJucnFSZWJqTjkwMU12bldlQTlLemZlWEdTUUlECkFRQUJvMkV3WHpBT0JnTlZIUThCQWY4RUJBTUNBcVF3SFFZRFZSMGxCQll3RkFZSUt3WUJCUVVIQXdFR0NDc0cKQVFVRkJ3TUNNQThHQTFVZEV3RUIvd1FGTUFNQkFmOHdIUVlEVlIwT0JCWUVGTDJqaE16S2tRNFRKckEzbm0vYQovVUcxSXN6M01BMEdDU3FHU0liM0RRRUJDd1VBQTRJQkFRREUzZTA2cHZ4OS9JTnpjSVhIZFdTTHFSa2xoM044CjZmM3J2SG83UFI0QTZDb1Q3cGozQXNBRzdLM3F5Tko1NVdUMUhMR0FkcUYrWnV6S0tJay9jU2V4WkZBRXN2OG0KRHVvQWduTlpudTltb0RHY1d6SmlRbHFKZlVlUXJYNmM3MVRuMDZoV1o4dTcydngvT2NVdGtKM3JUMStJVkV0bwpxd25MNlBKZHFhaTJHbHFpTjJ1R09Fc2NXb0VjLzZCVWJLRU11Z0NTbWFWOTk5TThzTSttcjJVU0dTaVRtd0w0CjMzcEdCbytkTEE4NnF1TTdhUEdFbGgrckFRUW54cG5VRUNwTVZTcVp1SlpYUzllUE5xRWYxRnh5SzN5SGtEVnYKdUU5dnNwRWx2bmZsS1NCZnpBcmYvanVvcVgzOUozQUhiUlkwTGFrNXdCdTRlc3cwMzJLN0JmTisKLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo="
service:
name: trust-manager
namespace: "cert-manager"
path: /validate-trust-cert-manager-io-v1alpha1-bundle
cert-manager, trust-manager-tls, Secret (v1) has changed:
# Source: trust-manager/templates/webhook.yaml
apiVersion: v1
kind: Secret
metadata:
labels:
app.kubernetes.io/instance: trust-manager
app.kubernetes.io/managed-by: Helm
app.kubernetes.io/name: trust-manager
app.kubernetes.io/version: v0.5.0
helm.sh/chart: trust-manager-v0.5.0
name: trust-manager-tls
namespace: cert-manager
data:
- ca.crt: '-------- # (1151 bytes)'
- tls.crt: '-------- # (1229 bytes)'
- tls.key: '-------- # (1679 bytes)'
+ ca.crt: '++++++++ # (1151 bytes)'
+ tls.crt: '++++++++ # (1229 bytes)'
+ tls.key: '++++++++ # (1675 bytes)'
type: kubernetes.io/tls
Had a quick look at the ca.crt beore and after:
# before
Certificate:
Data:
Version: 3 (0x2)
Serial Number:
f0:89:62:26:33:85:84:11:f6:c6:9a:f1:bf:e8:9c:70
Signature Algorithm: sha256WithRSAEncryption
Issuer: CN = *.cert-manager.svc
Validity
Not Before: Aug 10 10:37:54 2023 GMT
Not After : Aug 3 10:37:54 2048 GMT
# after
Certificate:
Data:
Version: 3 (0x2)
Serial Number:
51:be:bc:4e:a1:a2:33:d1:f9:f8:62:79:b9:79:9e:85
Signature Algorithm: sha256WithRSAEncryption
Issuer: CN = *.cert-manager.svc
Validity
Not Before: Aug 10 10:44:06 2023 GMT
Not After : Aug 3 10:44:06 2048 GMT
I guess I have two questions:
- Does it matter if we have a new version every time we upgrade, functionally?
- Is there a way to not have it do this maybe, so on upgrade there is less visual change? (if the answer to 1 is yes)
I think this is a easy way to omit the cert-manager & ca-injector dependencies. I think it is great for development & testing. For production this is probably also ok for a good amount of use cases. I can see the following limitations currently (in what cases a user should not use this feature):
- no rotation -> no short lived tls certificates possible
- no non-selfsigned/ private pki certs
- private key is part of Helm state & static
- secret is dynamic (each Helm template call gives you another cert)
- is not supported in static yaml
Currently, other controllers often have a "create self-signed cert" option that allows them to create a secret that can be injected using ca-injector (so no need for cert-manager to be installed). https://github.com/cert-manager/cert-manager/blob/master/pkg/webhook/authority/authority.go https://github.com/cert-manager/approver-policy/blob/main/pkg/internal/webhook/tls/tls.go https://github.com/cert-manager/webhook-lib/blob/main/server/tls/dynamic_source.go#L37 Maybe we should also try to create a more consistent story across all our projects?
I guess I have two questions:
They're great questions 😁
Does it matter if we have a new version every time we upgrade, functionally?
It doesn't as long as we also update the caBundle on the ValidatingWebhookConfiguration, which it looks like we do. There's also a need to restart pods because of that behaviour, which I'll talk about below.
Is there a way to not have it do this maybe, so on upgrade there is less visual change? (if the answer to 1 is yes)
I'm not sure! I think it'd be preferable to not reissue (it's not like the cert's gonna expire) but I can't really see how to do it. That might just be because my Helm-foo isn't good enough though!
Restarting the Pod
Your super-helpful test script led me to realise there'd be an error here because the webhook would be updated but the pod would still be using the old cert. I'll post my modified test script below, then add an extra commit which fixes that problem.
#!/usr/bin/env bash
set -eu -o pipefail
kind delete clusters trust
kind create cluster --name trust
kubectl create ns cert-manager
helm install -n cert-manager trust-manager ./deploy/charts/trust-manager --set issueIsolatedCert=true --wait
# Get before secret
kubectl get secret -n cert-manager trust-manager-tls -o yaml > install-cert.yaml
kubectl apply -f - <<EOF
apiVersion: trust.cert-manager.io/v1alpha1
kind: Bundle
metadata:
name: mybundle
spec:
sources:
- useDefaultCAs: true
target:
configMap:
key: "target-key"
EOF
sleep 2
# check upgrade
helm upgrade -n cert-manager trust-manager ./deploy/charts/trust-manager/ --set issueIsolatedCert=true --wait
# get after secret
kubectl get secret -n cert-manager trust-manager-tls -o yaml > after-cert.yaml
kubectl apply -f - <<EOF
apiVersion: trust.cert-manager.io/v1alpha1
kind: Bundle
metadata:
name: mybundle-2
spec:
sources:
- useDefaultCAs: true
target:
configMap:
key: "target-key"
EOF
Oh, also - testing with a locally built chart will fail unless you've also built new containers. The v0.5.0 images aren't supported by the chart on master because of a change to the validation path, so I used this locally (temporarily, just for testing):
diff --git a/deploy/charts/trust-manager/templates/webhook.yaml b/deploy/charts/trust-manager/templates/webhook.yaml
index 382cb3e..0ca95ae 100644
--- a/deploy/charts/trust-manager/templates/webhook.yaml
+++ b/deploy/charts/trust-manager/templates/webhook.yaml
@@ -100,4 +100,5 @@ webhooks:
service:
name: {{ include "trust-manager.name" . }}
namespace: {{ .Release.Namespace | quote }}
- path: /validate-trust-cert-manager-io-v1alpha1-bundle
+ #path: /validate-trust-cert-manager-io-v1alpha1-bundle
+ path: /validate
I believe this PR will close https://github.com/cert-manager/trust-manager/issues/132 if merged.
PR needs rebase.
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've updated the commit in a few ways:
- Improved wording and descriptions, in line with Erik's review
- Fixed an issue with how annotations were added to pods in the deployment. Tested to confirm it works
- Changed the value to
helmCert.enabledas discussed - Added a check for whether approver-policy and helmCert are both enabled. If they are, the chart fails to install (since they don't make sense together).
I tested installing and templating locally using the following values.yaml file:
app:
podAnnotations:
abc: "123"
webhook:
tls:
helmCert:
enabled: true
image:
tag: v0.11.1
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: erikgb
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [erikgb]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
![cert-manager-prow[bot] avatar](https://avatars.githubusercontent.com/in/877404?v=4 "Published by a pub.dev verified publisher"){kind=small}
I think merging this PR should close https://github.com/cert-manager/trust-manager/issues/382 https://github.com/cert-manager/trust-manager/issues/132.
/lgtm
Maybe update the description to auto-close the relevant issues before merging?