Configurable issuer duration and renewBefore by vdesjardins · Pull Request #520 · cert-manager/cert-manager

vdesjardins · 2018-04-28T04:46:35Z

What this PR does / why we need it:
This pull request makes possible to configure the certificate validity duration and the renewal before duration per issuer.
Which issue this PR fixes:
#437
Special notes for your reviewer:
Split from Vault issuer pull request #292

Release note:

Configurable certificate duration and renewal duration using fields Issuer.spec.duration and 
Issuer.spec.renewBefore respectively.
The CA Issuer default duration is now 90 days instead of 365 days to make all issuers more consistent when configured without explicit duration.

munnerz · 2018-04-28T07:01:10Z

/ok-to-test

…

On Sat, 28 Apr 2018 at 05:46, jetstack-bot ***@***.***> wrote: [APPROVALNOTIFIER] This PR is *NOT APPROVED* This pull-request has been approved by: To fully approve this pull request, please assign additional approvers. We suggest the following additional approver: *kragniz* Assign the PR to them by writing /assign @kragniz in a comment when ready. The full list of commands accepted by this bot can be found here <https://go.k8s.io/bot-commands>. The pull request process is described here <https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process> Needs approval from an approver in each of these files: - *OWNERS <https://github.com/jetstack/cert-manager/blob/master/OWNERS>* Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#520 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMbP-ob4cwRGtV_tFIw2Nn9UB0cyiWmks5ts_Q5gaJpZM4TrPmN> .

munnerz · 2018-05-01T12:41:31Z

pkg/controller/certificates/sync.go

 	messageCertificateRenewed = "Certificate renewed successfully"
+
+	messageWarningCertificateDuration = "Certificate duration received from issuer is %s which is lower than the issuer configured duration of %s"
+	messageWarningScheduleModified    = "Certificate renewal requested schedule cannot be honored. Specified renewBefore of %s is greater than certificate total duration of %s"


This sounds like a hard error to me - perhaps rephrase to explain that we've instead changed the renewal date?

I'd also make this a non-warning (i.e. EventTypeNormal), as some users will take a warning to be an error and starting opening issues 😬

munnerz · 2018-05-01T12:42:32Z

pkg/issuer/acme/setup.go

 const (
 	errorAccountRegistrationFailed = "ErrRegisterACMEAccount"
 	errorAccountVerificationFailed = "ErrVerifyACMEAccount"
+	errorDurationInvalid           = "ErrACMEDurationInvalid"


Needn't be an ACME specific error. ErrDurationInvalid should suffice, and can be reused across issuers.

munnerz · 2018-05-01T12:43:28Z

pkg/issuer/acme/setup.go

 	messageAccountVerificationFailed = "Failed to verify ACME account: "
 	messageAccountRegistered         = "The ACME account was registered with the ACME server"
 	messageAccountVerified           = "The ACME account was verified with the ACME server"
+	messageDurationInvalid           = "ACME "


Remove this var - it doesn't really add any context for the user. They are already aware that it is an ACME certificate. We can instead just set the conditions message to err.Error()

munnerz · 2018-05-01T12:43:50Z

pkg/issuer/acme/setup.go

+	if err != nil {
+		s := messageDurationInvalid + err.Error()
+		a.issuer.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorDurationInvalid, s)
+		return fmt.Errorf(s)


Can get rid of s and just return err here instead.

munnerz · 2018-05-01T12:45:00Z

pkg/issuer/ca/setup.go

 	messageKeyPairVerified = "Signing CA verified"
+
+	messageDurationInvalid = "CA "
+	errorDurationInvalid   = "ErrCADurationInvalid"


Can get rid of both of these vars

munnerz · 2018-05-01T12:45:08Z

pkg/issuer/ca/setup.go

+		s := messageDurationInvalid + err.Error()
+		glog.Info(s)
+		c.issuer.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorDurationInvalid, s)
+		return fmt.Errorf(s)


Same comment as ACME issuer

This addresses comments in PR cert-manager#520. To make the Duration check more generic it was moved into the Issuer factory, allowing all Issuers to benifit.

MadVikingGod · 2018-05-08T18:17:35Z

Hi, this is a feature that I am interested in using. Is there anything that I can do to help shepherd this along it's path?

vdesjardins · 2018-05-09T03:09:30Z

I just pushed an update to the pull request with fixes for the code review and support for the Vault issuer.

The main remaining issue that I can think of right now is what we should do with the ACMEv2 issuer. The ACMEv2 issuer receives a validation error from Let's Encrypt (Boulder) when a custom notBefore and notAfter is specified in the order request. The reason is that the Boulder server do not implement that particular feature of the spec.

Since Boulder (and Pebble) are the only servers implementing the ACMEv2 protocol that I'm aware of maybe we could do one of these:

Remove support for custom duration instead of generating an error. We could generate an status condition to let the user know that we do not support custom duration for this issuer.
Recover from the error and update the certificate status with a warning/info about the fact that the issuer does not support custom duration.

MadVikingGod · 2018-05-10T15:16:54Z

It's an optional field in the acme spec. I would just put it in as a warning that it's not supported by the ACME server.

munnerz · 2018-05-24T13:22:33Z

/cc @vdesjardins

jetstack-bot · 2018-05-24T13:22:35Z

@munnerz: GitHub didn't allow me to request PR reviews from the following users: vdesjardins.

Note that only jetstack members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @vdesjardins

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.

vdesjardins · 2018-06-06T02:30:35Z

/retest

jetstack-bot · 2018-06-09T18:42:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: wallrj

Assign the PR to them by writing /assign @wallrj in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

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

jetstack-bot · 2018-06-15T16:35:50Z

@vdesjardins: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
cert-manager-e2e-v1-9	`516be06`	link	`/test e2e v1.9`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

munnerz · 2018-06-16T17:52:53Z

pkg/issuer/acme/acme.go

+			// Clearing the specified validity period in the request and try again
+			order.NotBefore = time.Time{}
+			order.NotAfter = time.Time{}
+			createdOrder, err = cl.CreateOrder(ctx, order)


I think we need some way to mark a certification configuration as invalid in order to prevent re-processing of a Certificate in this case, instead of re-attempting to create ourselves.

This will require us somehow hashing the 'input values' (i.e. the Duration field right now) and recording on the Certificate resource that the particular configuration is invalid. When a re-sync happens (i.e. when the resource changes), we will have to compare that hash to the hash of the new resource in order to determine if a user has updated their configuration, in which case we retry CreateOrder with the new configuration.

I think there are a number of places where this could be useful, at least for ACME. Perhaps for other Issuers (or other parts of the ACME issuer) in future too. It's sort of 'runtime validation' against the actual Issuer, and not just validating the resources are valid 😄

Yes I agree. Perhaps we can store it in a label like the hash computed by the deployment resource? In the pod resource this field is called pod-template-hash.

#761 will allow us to skip processing Certificate resources with particular fields set if they reference ACME issuers.

This should allow us to just blanket not support setting these fields with all ACME issuers. We can then explore at a later time an implementation that's more complex without making a breaking change 😄

#761 has now merged, so we should be able to add in this check now 😄

@vdesjardins are you still in a position to rebase and get this PR up to date? There's been a few changes that may effect your implementation slightly!

I should have some time hopefully to start looking at it the coming days!

jetstack-bot · 2018-06-26T15:01:38Z

@vdesjardins: PR needs rebase.

Details

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.

munnerz · 2018-10-12T10:57:20Z

Closing this in favour of #893 😄

jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 28, 2018

jetstack-bot requested a review from kragniz April 28, 2018 04:46

jetstack-bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 28, 2018

vdesjardins force-pushed the cert-duration branch 4 times, most recently from de26bbc to 178e943 Compare April 28, 2018 18:25

munnerz reviewed May 1, 2018

View reviewed changes

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2018

vdesjardins force-pushed the cert-duration branch from 178e943 to b0a6b4a Compare May 9, 2018 02:26

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2018

vdesjardins force-pushed the cert-duration branch from b0a6b4a to d57696b Compare May 9, 2018 02:39

munnerz added this to the v0.4 milestone May 9, 2018

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2018

vdesjardins force-pushed the cert-duration branch from d57696b to 0058f94 Compare May 19, 2018 02:06

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2018

vdesjardins force-pushed the cert-duration branch 2 times, most recently from 9b8e239 to ee86bb6 Compare May 30, 2018 01:14

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2018

vdesjardins force-pushed the cert-duration branch from ee86bb6 to 191ce58 Compare June 9, 2018 18:41

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2018

wallrj mentioned this pull request Jun 12, 2018

Add self signed Issuer type #637

Merged

vdesjardins force-pushed the cert-duration branch from 191ce58 to 9e1951f Compare June 15, 2018 13:19

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2018

vdesjardins added 2 commits June 15, 2018 15:51

Configuration of certificate duration and renewBefore in issuers.

7230151

ACME: retry order if certificate duration not supported

516be06

vdesjardins force-pushed the cert-duration branch from 9e1951f to 516be06 Compare June 15, 2018 16:01

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 15, 2018

munnerz reviewed Jun 16, 2018

View reviewed changes

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2018

munnerz mentioned this pull request Jul 18, 2018

'Runtime' validation support #735

Closed

munnerz mentioned this pull request Jul 26, 2018

Allow configurable expiration for CA issuer. #767

Closed

munnerz modified the milestones: v0.4, v0.5 Jul 28, 2018

munnerz mentioned this pull request Aug 17, 2018

Set Organization in Certificates #838

Merged

Queuecumber mentioned this pull request Sep 12, 2018

Configurable issuer duration and renewBefore Take 2 #893

Merged

munnerz modified the milestones: v0.5, v0.6 Sep 13, 2018

munnerz closed this Oct 12, 2018

Conversation

vdesjardins commented Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

munnerz commented Apr 28, 2018 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MadVikingGod commented May 8, 2018

Uh oh!

vdesjardins commented May 9, 2018

Uh oh!

MadVikingGod commented May 10, 2018

Uh oh!

munnerz commented May 24, 2018

Uh oh!

jetstack-bot commented May 24, 2018

Uh oh!

vdesjardins commented Jun 6, 2018

Uh oh!

jetstack-bot commented Jun 9, 2018

Uh oh!

jetstack-bot commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jetstack-bot commented Jun 26, 2018

Uh oh!

munnerz commented Oct 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vdesjardins commented Apr 28, 2018 •

edited

Loading

jetstack-bot commented Jun 15, 2018 •

edited

Loading