docs: Certificate Defaults Tutorial #1279

hawksight · 2023-09-04T15:11:25Z

Tutorial on setting Certificate Defaults based off of the exploration here from the original cert-manager issue.

Preview: https://deploy-preview-1279--cert-manager-website.netlify.app/docs/tutorials/certificate-defaults/

netlify · 2023-09-04T15:14:29Z

✅ Deploy Preview for cert-manager-website ready!

Name	Link
🔨 Latest commit	`56f3d0c`
🔍 Latest deploy log	https://app.netlify.com/sites/cert-manager-website/deploys/64f5f39f4905bd000847b909
😎 Deploy Preview	https://deploy-preview-1279--cert-manager-website.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2023-09-04T15:48:50Z

✅ Deploy Preview for cert-manager-website ready!

Name	Link
🔨 Latest commit	`111a387`
🔍 Latest deploy log	https://app.netlify.com/sites/cert-manager-website/deploys/65a5246703a99a00080b51fd
😎 Deploy Preview	https://deploy-preview-1279--cert-manager-website.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

hawksight · 2023-09-04T15:55:51Z

/cc @wallrj @maelvls

hawksight · 2023-09-04T16:57:18Z

@wallrj I added you for review as we started this exploration together so would be great to get your review here.

hawksight · 2023-09-05T12:20:40Z

/hold

hawksight · 2023-09-05T12:22:14Z

wallrj

Some initial comments. I haven't finished going through the tutorial yet.

content/docs/tutorials/certificate-defaults/README.md

wallrj · 2023-09-06T11:40:04Z

content/docs/tutorials/certificate-defaults/README.md

+      revisionHistoryLimit: 44
+      secretName: test-revision-override-cert
+    ```
+


Stopping here.

All those commands above work well.

I spotted another typo.

Perhaps this first part of the content can be merged now and the content below can be added to a separate PR after 1.13 is released with your fix for the mutating webhook.

Maybe write something about what "sane defaults" are recommended by the cert-manager team and explain why those are not the out-of-the-box defaults....here or in another PR?

Maybe write something about what "sane defaults" are recommended by the cert-manager team and explain why those are not the out-of-the-box defaults....here or in another PR?

I love that - will add a section. Might need your input after my first draft

Perhaps this first part of the content can be merged now and the content below can be added to a separate PR after 1.13 is released with your fix for the mutating webhook.

I have another section to add with ingress-shim. So I'll see when I complete this, probably after 1.13 so I can clean up that setion, or make reference to.. run 1.13 or read here ..

In the latest revision I am now able to move the <= v1.13 stuff to an appendix to make the tutorial flow better.
Added v1.14.X as a req for the tutorial.

hawksight · 2023-09-08T10:51:20Z

Content is now done and ready for a full review!

ingress-shim section (section 4)
A note on cert-manager having defaults and why they might not be suitable to you (intro section)
Commands a re now diffs which highlight the changes, or lack of when appropriate.
Spelling hopefully all correct now (or correct by US standards 😜 ) and caught all those Richard mentioned.

@maelvls / @wallrj the only thing I have not done is to change the file reference to be GitHub links. Eg.

kubectl apply -f yamls/cert-test-revision.yaml --dry-run=server -o yaml | diff -uZ yamls/cert-test-revision.yaml -
# This could be ..
kubectl apply -f https://raw.githubusercontent.com/cert-manager/website/a6c1ac93ed6b50c4bde8181b2b71f0ec53ee8841/content/docs/tutorials/certificate-defaults/yamls/cert-test-revision.yaml --dry-run=server -o yaml | diff -uZ https://raw.githubusercontent.com/cert-manager/website/a6c1ac93ed6b50c4bde8181b2b71f0ec53ee8841/content/docs/tutorials/certificate-defaults/yamls/cert-test-revision.yaml -

The other concern if to have that URL variablised to point to the "preview" or commit version, and then update to "master" or latest once this is merged. Without another PR. Eg. ${GITHUB_BRANCH} ?

I am happy to not do this, because the command is a bit ugly with URLs.
But it would make it easier to apply the resources and follow along?

Maybe a better idea would be to pull the yamls and patches directories to your machine and add a step for that? Opinions very welcome here.

hawksight · 2023-09-08T10:53:26Z

/unhold

wallrj

A couple of drive by remarks, but I'll do a more thorough review next week if you want.

I think it might be great to start with a "Use Cases" section E.g.

In this tutorial you will learn how to use Kyverno ClusterPolicy to encourage or to enforce best-practice and policy compliant settings for cert-manager Certificate resources.
Here are a few examples, but you can adapt these to meet your own specific requirements.

You want to avoid or ensure that CertificateRequest resources get cleaned up

Use a ClusterPolicy to set a sane default value for the Certificate.Spec.RevisionHistoryLimit field or to always override that field with 1 or 0.

You want to help your users choose secure default key settings for their Certificate resources.

Use a ClusterPolicy to set policy compliant default values for the Certificate.Spec.PrivateKey fields
or enforce your policy by overriding those settings.

I still think this document reads too much like a technical exploration, and that it can be made to sound more like a tutorial.

content/docs/tutorials/README.md

content/docs/tutorials/certificate-defaults/README.md

.spelling

content/docs/tutorials/certificate-defaults/README.md

hawksight · 2023-09-14T13:34:09Z

Thanks @wallrj & @jsoref for all the comments and review 🙌.
I'm going to try and work through them all in the next few days and come back with some revised wording.

maelvls · 2023-09-25T13:54:40Z

Hey, it looks like we are very close of having your tutorial published! How can I help?

hawksight · 2024-01-08T13:31:51Z

Based on my Q in the CNCF kyverno slack here,

I need to make a minor edit around the ingress section to not rely on metadata.annotations anymore.

It seems it was removed in 3.1.1 of kyverno, so I need to reference the logs instead.
Will do that shortly to avoid confusing users of this guide.

hawksight · 2024-01-09T11:05:08Z

Ok CI is passing, I have made the changes I wanted to. This is now good for a final review from my side.

@wallrj / @jsoref - might you be able to take another look please?.
Most of the steps are the same as before, but the restructure hopefully makes it feel much nicer to follow.

content/docs/tutorials/certificate-defaults/README.md

jsoref · 2024-01-09T12:45:39Z

content/docs/tutorials/certificate-defaults/README.md

+When the existing cert-manager `MutatingWebhookConfiguration` runs it will add the required field with an empty value, such as: `secretName: ""`, if there is no value in a required field. 
+The consequence of this action is that our Kyverno policies will not apply as an empty value is already present.
+
+Starting at v1.14.X the cert-manager-webhook `MutatingWebhookConfiguration` resource has been scoped back to only affect `CertificateRequest` resource. 


Suggested change

Starting at v1.14.X the cert-manager-webhook `MutatingWebhookConfiguration` resource has been scoped back to only affect `CertificateRequest` resource.

Starting with v1.14.x, the cert-manager-webhook `MutatingWebhookConfiguration` resource has been scoped to only affect `CertificateRequest` resources.

Agreed with the sentence. Left the .X in the version to keep consistency across the document though.

I should have called it out earlier, I prefer lowercase .x throughout.

I just thought, should I not just use v1.14 rather than v1.14.x?

content/docs/tutorials/certificate-defaults/README.md

hawksight · 2024-01-10T10:56:37Z

Todo

Redo the example outputs to ensure no kyverno annotations are added.
Address all review comments

hawksight · 2024-01-10T13:00:25Z

@jsoref thank you for all the time reviewing the changes. I resolved all the "simple" or accepted changes and left a few open where I had comments. Please let me know if there's anything I missed that you consider a show stopper. I think anything left is pretty minor.

wallrj

Thanks Peter.

I left a few suggestions below but this tutorial is going to be really nice.

Maybe rebase this on release-next branch and add some of your appendix notes to the release-notes-1.14.md document.

content/docs/tutorials/certificate-defaults/README.md

wallrj · 2024-01-10T15:49:28Z

content/docs/tutorials/certificate-defaults/README.md

+    In the `note` section you can identify that it was applied to the `Certificate` resource that was created by cert-manager based on the `Ingress` resource that we applied.
+
+Whilst you are not able to default the `secretName` and `issuerRef` fields when using the ingress-shim, you can default all other fields. 
+This is reasonable given that the `Ingress` specification needs to know what `Secret` to mount to the ingress controller.


This makes it sound like a short-coming when in fact it seems like a benefit that the secretName and issuerRef defaulting policy do not interfere with ingress-shim Certificate resources.

I'd be tempted to remove the content from here up to "You can optionally validate that only the "0-mutate-certificate-defaults" ClusterPolicy has been applied".
I don't think it needs mentioning

I've reworded it. My objective was that despite the Ingress part of this guide being after the defaulting of required fields, the rules for requried fields don't really play a part in the ingress section.

I hope the newer wording does a better job of explaining that.

wallrj · 2024-01-10T15:52:41Z

content/docs/tutorials/certificate-defaults/README.md

+
+> **Note**: The `ClusterIssuer` can be defaulted by supplying an install time parameter to cert-manager. 
+> See [cert-manager documentation](../../usage/ingress.md#optional-configuration) for full details. 
+> You would still need to provide at least one annotation, however this time it is static and doesn't need to be defaulted.


I find this a slightly confusing side note too. It doesn't seem relevant.
I suggest removing it.

Yes, I was thinking that this optional configuration was worth a mention, in that you could reduce a user's knowledge of Issuer and ClusterIssuers further by using just the kubernetes.io/tls-acme: "true" as opposed to the one I used here: cert-manager.io/cluster-issuer: "our-corp-issuer"

But again that's sort of an addendum to the tutorial and not really the core focus here.
Perhaps this is more operational best practice as the access to resources could tie into RBAC and how the platform team manages to Certificate provisioning lifecycle.

content/docs/tutorials/certificate-defaults/README.md

wallrj · 2024-01-10T15:56:15Z

content/docs/tutorials/certificate-defaults/README.md

+```
+🔗 <a href="cert-test-minimal.yaml">`cert-test-minimal.yaml`</a>
+
+With these policies we achieved our objective and have enabled users to submit minimal `Certifiate` resources, with only a single field contained within the specification, the `dnsNames` entry.


This objective was not declared in the use-cases section above.
I suggest moving or copying it there.

Just trying to word this as an additional use case:

- **To enable users to request a certificate by providing only a domain name** Use a `ClusterPolicy` to set all other required `Certificate.spec` fields.

Would that suggestion cover it. Or should I be specific here and word it as:

- **To enable users to request a certificate by providing only a `Certificate.spec.dnsNames` field** Use a `ClusterPolicy` to set all other required `Certificate.spec` fields that cert-manager expects for `CertificateRequest` creation.

I am also struggling to come up with a good sentence. Maybe something more general, like:

Make application developers lives easier by allowing them to create secure X.509 TLS certificates with the minimum of configuration.

The application developer only needs to supply the field values that are relevant for their application, such as DNS names, extended usages and the target Secret name.
Use ClusterPolicy to set all the other required Certificate.spec fields to sane defaults.

Make application developers' lives easier by allowing them to create secure X.509 TLS certificates with the minimum of configuration.

Thanks, I took that title and added some sub-context.

content/docs/tutorials/certificate-defaults/README.md

hawksight · 2024-01-10T17:55:05Z

TY @wallrj - I have run out of steam and time today. I'll look to address those comments on Friday.
Given the tutorial needs 1.14 is that the reason for suggesting this goes into the release-next branch rather than master?

wallrj · 2024-01-12T15:59:36Z

Given the tutorial needs 1.14 is that the reason for suggesting this goes into the release-next branch rather than master?

Yes, that is my reasoning. Otherwise we will have to delay merging this tutorial until we release 1.14.

jsoref · 2024-01-12T16:28:15Z

content/docs/tutorials/certificate-defaults/README.md


 1. Apply the policy to the cluster and check that it is ready:

    ```shell
-    kubectl apply -f cpol-mutate-certificate-defaults.yaml
+    kubectl apply -f cpol-mutate-certificates-0.yaml
    kubectl get cpol
    ```

    When the `ClusterPolicy` is ready the output should look like this:

    ```log
    NAME                            BACKGROUND   VALIDATE ACTION   READY   AGE   MESSAGE


did this line really not change its formatting to match the next line?

Just fixed a couple minor CI fails and I'll check the latest preview deploy.
I do need to redo the output examples anyway due to the condensing of policies into one file.

Signed-off-by: Peter Fiddes <[email protected]>

hawksight · 2024-01-15T12:35:08Z

@wallrj - I think that is everything addressed and now rebased too (thank to @aidy for git guidance)

I hope now ready for final review :)

wallrj

Thanks @hawksight

All the latest changes look good to me and thanks for rebasing on release-next.

/lgtm
/approve

jetstack-bot · 2024-01-16T09:55:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

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:

~~OWNERS~~ [wallrj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wallrj · 2024-02-08T17:21:31Z

content/docs/tutorials/certificate-defaults/README.md

+For further background reading around setting "defaults" or "presets", you can refer to [issue 2239](ttps://github.com/cert-manager/cert-manager/issues/2239).
+This tutorial came out of an investigation of that issue.


Broken link to CertificatePresets issue

@hawksight I just noticed this broken link while reading the live docs.
Sorry I missed it during review.

hawksight marked this pull request as ready for review September 4, 2023 15:53

jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2023

jetstack-bot assigned maelvls and wallrj and unassigned wallrj and maelvls Sep 4, 2023

jetstack-bot requested review from maelvls and wallrj September 4, 2023 15:55

hawksight mentioned this pull request Sep 4, 2023

Create a CertificatePreset resource type to allow configurable defaulting cert-manager/cert-manager#2239

Open

jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2023

wallrj requested changes Sep 5, 2023

View reviewed changes

wallrj requested changes Sep 6, 2023

View reviewed changes

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2023

hawksight requested a review from wallrj September 8, 2023 10:53

wallrj reviewed Sep 8, 2023

View reviewed changes

jsoref reviewed Sep 11, 2023

View reviewed changes

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2023

hawksight force-pushed the pf/kyverno-defaults branch from 482d0c9 to 1ae8688 Compare January 8, 2024 11:40

jsoref reviewed Jan 9, 2024

View reviewed changes

wallrj requested changes Jan 10, 2024

View reviewed changes

jsoref reviewed Jan 12, 2024

View reviewed changes

hawksight mentioned this pull request Jan 12, 2024

Create a section for sane Certificate defaults #1388

Open

hawksight force-pushed the pf/kyverno-defaults branch from bca8a1c to b4efce6 Compare January 15, 2024 12:08

hawksight changed the base branch from master to release-next January 15, 2024 12:18

hawksight force-pushed the pf/kyverno-defaults branch 2 times, most recently from bf8ebe9 to 3c021a1 Compare January 15, 2024 12:23

feat: Certificate defaults tutorial with Kyverno

111a387

Signed-off-by: Peter Fiddes <[email protected]>

hawksight force-pushed the pf/kyverno-defaults branch from 3c021a1 to 111a387 Compare January 15, 2024 12:26

hawksight requested review from wallrj and jsoref January 15, 2024 16:34

wallrj approved these changes Jan 16, 2024

View reviewed changes

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2024

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2024

jetstack-bot merged commit 33a3d73 into cert-manager:release-next Jan 16, 2024
4 checks passed

hawksight deleted the pf/kyverno-defaults branch January 16, 2024 10:02

This was referenced Jan 19, 2024

pf/tutorial kyverno version #1395

Closed

Release cert-manager 1.14 #1400

Merged

wallrj reviewed Feb 8, 2024

View reviewed changes

hawksight mentioned this pull request Feb 9, 2024

fix: Broken link due to missing character #1423

Merged

	Starting at v1.14.X the cert-manager-webhook `MutatingWebhookConfiguration` resource has been scoped back to only affect `CertificateRequest` resource.
	Starting with v1.14.x, the cert-manager-webhook `MutatingWebhookConfiguration` resource has been scoped to only affect `CertificateRequest` resources.

		For further background reading around setting "defaults" or "presets", you can refer to [issue 2239](ttps://github.com/cert-manager/cert-manager/issues/2239).
		This tutorial came out of an investigation of that issue.

docs: Certificate Defaults Tutorial #1279

docs: Certificate Defaults Tutorial #1279

Conversation

hawksight commented Sep 4, 2023 • edited by maelvls Loading

netlify bot commented Sep 4, 2023

✅ Deploy Preview for cert-manager-website ready!

netlify bot commented Sep 4, 2023 • edited Loading

✅ Deploy Preview for cert-manager-website ready!

hawksight commented Sep 4, 2023

hawksight commented Sep 4, 2023

hawksight commented Sep 5, 2023

hawksight commented Sep 5, 2023 • edited Loading

wallrj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawksight commented Sep 8, 2023

hawksight commented Sep 8, 2023

wallrj left a comment

Choose a reason for hiding this comment

You want to avoid or ensure that CertificateRequest resources get cleaned up

You want to help your users choose secure default key settings for their Certificate resources.

hawksight commented Sep 14, 2023

maelvls commented Sep 25, 2023

hawksight commented Jan 8, 2024 • edited Loading

hawksight commented Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawksight commented Jan 10, 2024 • edited Loading

Todo

hawksight commented Jan 10, 2024

wallrj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawksight commented Jan 10, 2024

wallrj commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawksight commented Jan 15, 2024

wallrj left a comment

Choose a reason for hiding this comment

jetstack-bot commented Jan 16, 2024

Choose a reason for hiding this comment

hawksight commented Sep 4, 2023 •

edited by maelvls

Loading

netlify bot commented Sep 4, 2023 •

edited

Loading

hawksight commented Sep 5, 2023 •

edited

Loading

hawksight commented Jan 8, 2024 •

edited

Loading

hawksight commented Jan 9, 2024 •

edited

Loading

hawksight commented Jan 10, 2024 •

edited

Loading