Configurable issuer duration and renewBefore Take 2 by Queuecumber · Pull Request #893 · cert-manager/cert-manager

Queuecumber · 2018-09-12T14:59:27Z

This is me commandeering #520 because it is clearly not being worked on

Plan right now is to skip 516be06 in favor of validation which disallows duration for ACME issuers.

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:

TODO

~~Salvage code from 7230151~~
~~Make the existing unit and e2e tests work~~
~~Use duration config in self-signed issuer~~
~~General code quality updates~~
~~Validate that duration is not set for ACME certs (w/ unit test)~~
~~Test everything in a real cluster~~
~~Validate durations in validation and not issuer setup~~

Release note:

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

Queuecumber · 2018-09-12T15:00:21Z

/hold

This is just to get the ball roling and get some early feedback, e2e tests had a lot of changes since the original commit and need to be reworked. Docs are not updated yet and validation still needs to be written.

Queuecumber · 2018-09-12T15:06:57Z

pkg/issuer/issuer.go


+ const (
+ 	// Error reason generated when duration or renewBefore is invalid
+	ErrorDurationInvalid = "ErrDurationInvalid"


A lot of things to do with constants seemed to move since the original commit, so I stuck this here, Is there a better place for it?

From what I can tell, this var isn't actually used? Unless GitHub is collapsing things in the diff for me! 😄

Yup you're right, this is a holdover from when we were triggering events for stuff (I think) so I'll remove it

Queuecumber · 2018-09-12T15:08:07Z

pkg/controller/certificates/sync.go

 )

+// to help testing
+var now = time.Now


I kind of hate this line, and I don't understand it's purpose in the original commit. I know that it is used in the test (next file down), but there must be a better way to handle it.

munnerz · 2018-09-13T10:02:51Z

/ok-to-test

Queuecumber · 2018-09-13T17:59:57Z

@munnerz Can you tell me what version of dep the CI server uses? I dont think the new Bazel system prints it out like the old hack script used to and for a while now I get the following error when I run bazel run hack://update-deps

(81/93) Wrote github.com/hashicorp/errwrap@master: hash of vendored tree didn't match digest in Gopkg.lock
grouped write of manifest, lock and vendor: failed to export bitbucket.org/ww/goautoneg: 
        (1) hg is not installed: 
        (2) hg is not installed: 
        (3) hg is not installed: 
        (4) failed to list versions for https://bitbucket.org/ww/goautoneg: fatal: repository 'https://bitbucket.org/ww/goautoneg/' not found
: exit status 128
        (5) failed to list versions for ssh://git@bitbucket.org/ww/goautoneg: not a git repository.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
: exit status 128
        (6) failed to list versions for git://bitbucket.org/ww/goautoneg: fatal: unable to connect to bitbucket.org:
bitbucket.org[0: 18.205.93.2]: errno=Connection timed out
bitbucket.org[1: 18.205.93.0]: errno=Connection timed out
bitbucket.org[2: 18.205.93.1]: errno=Connection timed out

: exit status 128
        (7) failed to list versions for http://bitbucket.org/ww/goautoneg: fatal: repository 'http://bitbucket.org/ww/goautoneg/' not found
: exit status 128

Queuecumber · 2018-09-13T18:02:21Z

OK well nvm it looks like it really did just need the hg binary to complete I dont know why thats surprising to me

munnerz · 2018-09-13T18:03:43Z

OK well nvm it looks like it really did just need the hg binary to complete I dont know why thats surprising to me

This is down to that one bitbucket dependency afaict. We've had this issue for a while, and it's really annoying! 😬

Glad you got it resolved. FWIW, I think you need to run:

bazel run //hack:update-bazel
bazel run //hack:update-codegen

😄

munnerz · 2018-09-13T18:04:10Z

I also DM'd you on slack just now - feel free to reach out directly if you run into more issues. The new Bazel build system is still early on and there could be a few rough edges!

Queuecumber · 2018-09-14T17:18:46Z

pkg/apis/certmanager/v1alpha1/const.go

+
+	// Default duration before certificate expiration if  Issuer.spec.renewBefore is not set
+	DefaultRenewBefore = time.Hour * 24 * 30
+)


Is this the best place for these constants to go?

👍 I think so - this ultimately does become part of our public API, and given we are still alpha, it does not preclude a breaking change if needs be 😄

test/e2e/certificate/certificate_vault.go

Queuecumber · 2018-09-15T14:19:33Z

@munnerz Couple of things that I was thinking about that I'd like your opinion on

Currently duration and renewBefore are properties of the issuers. Should there also be properties on the certificates that override these? Or do we treat the different issuers as QoS levels, so if you want a longer duration cert then ask for one from the issuer that gives longer durations?
Should there be an option to not renew a certificate? For example (real example that I am working on today), I have someone who needs access to our site for a month, so I want to issue him a certificate for one month but not renew it. I could just delete the cert resource and/or not send him the renewed certificate, but maybe there should be options for this?

I think both of these are probably future work in the space of defaulting policies and certificate delivery that you had mentioned in previous discussions.

Queuecumber · 2018-09-15T19:09:24Z

/hold cancel

This passed a few tests I ran in a real cluster this morning, issued certs correctly have expirations set, etc. and I was able to issue such a client cert to someone who needs short term access to my cluster. So I think this is at your mercy @munnerz

Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

Queuecumber · 2018-11-12T18:18:52Z

@munnerz Ok well, it seems to pass e2e tests now, but I'm not exactly happy with the solution which was to just not set the duration and renewBefore fields on ACME tests. I still have no idea why it was failing though, and only for HTTP01. DNS01 tests worked fine, and both HTTP01 and DNS01 worked on my live cluster, so its a bit of a mystery. Your call how we proceed.

…be caught as any other too-small value Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

Queuecumber · 2018-11-12T21:15:45Z

/hold

cleaning up some of the e2e tests

…te API calls Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

Queuecumber · 2018-11-13T00:21:29Z

/hold cancel

I think it's done now

Signed-off-by: James Munnelly <james@munnelly.eu>

munnerz · 2018-11-14T22:29:23Z

I added a few commits on top to clean up a few things, to save you having to go through more rounds of review and back-and-forth!

I cleaned up the Certificate controller Sync function, fixing up a rebase issue
Allowed ACME certificates to specify renewBefore
Updated the docs examples
Renamed calculateTimeBeforeExpiry

I also removed some dead/irrelevant code that I think has been brought forward from rebases.

Thanks for being so persistent with this, I know it's been a pain!

/lgtm
/approve

jetstack-bot · 2018-11-14T22:29:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

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~~ [munnerz]

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

Queuecumber · 2018-11-14T23:18:23Z

Truly a momentous occasion

That's pretty much all I've got for the foreseeable future, good luck with the project

Btw how long until this is available in an image?

jetstack-bot requested review from kragniz and munnerz September 12, 2018 14:59

jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 12, 2018

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

Queuecumber commented Sep 12, 2018

View reviewed changes

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

Queuecumber force-pushed the cert-duration branch from fc65bce to 7a873bf Compare September 13, 2018 16:51

jetstack-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 13, 2018

Queuecumber force-pushed the cert-duration branch from 7a873bf to a3aa97d Compare September 13, 2018 17:29

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

jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 14, 2018

Queuecumber commented Sep 14, 2018

View reviewed changes

test/e2e/certificate/certificate_vault.go Outdated Show resolved Hide resolved

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

Rerunning hacks after rebase

9706517

Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

Queuecumber force-pushed the cert-duration branch from bbb1f33 to 9706517 Compare November 8, 2018 21:20

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Nov 8, 2018

Queuecumber added 2 commits November 8, 2018 16:39

Fix bad function call in new e2e test

88902cf

Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

Try not setting duration and renewbefore on acme e2e tests

6ff6ce6

Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

Queuecumber force-pushed the cert-duration branch from b3865a4 to 6ff6ce6 Compare November 11, 2018 20:27

Queuecumber added 2 commits November 12, 2018 16:11

Zero checks should really just be replaced by nil tests, zero should …

e3571e3

…be caught as any other too-small value Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

Fixed a missing nil check that got away

96820f2

Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2018

Queuecumber added 3 commits November 12, 2018 18:43

Change e2e duration test format to use pointer times to better simula…

720edfe

…te API calls Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

Fix sync unit test to match e2e test format

cf7d260

Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

Fix vault e2e test

4be0b2f

Signed-off-by: Max Ehrlich <max.ehr@gmail.com>

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2018

Revert changes to Certificate sync function

9e8a0ca

Signed-off-by: James Munnelly <james@munnelly.eu>

munnerz added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 14, 2018

Remove selfsigned e2e issuer.go

3a32527

Signed-off-by: James Munnelly <james@munnelly.eu>

munnerz force-pushed the cert-duration branch from b641468 to 3a32527 Compare November 14, 2018 14:05

munnerz added 4 commits November 14, 2018 14:19

Don't use ACME issuer in duration example and tidy up line endings

f278cb6

Signed-off-by: James Munnelly <james@munnelly.eu>

Allow renewBefore to be set on ACME certificates

1f447e6

Signed-off-by: James Munnelly <james@munnelly.eu>

Update renewBefore ACME docs. Remove unused fields.

ab795b5

Signed-off-by: James Munnelly <james@munnelly.eu>

Rename calculateTimeBeforeExpiry to calculateDurationUntilRenew

b7d124c

Signed-off-by: James Munnelly <james@munnelly.eu>

jetstack-bot assigned munnerz Nov 14, 2018

munnerz mentioned this pull request Jan 10, 2019

feature request: Add custom renewal time for certificates #437

Closed

Conversation

Queuecumber commented Sep 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Queuecumber commented Sep 12, 2018

Uh oh!

Queuecumber Sep 12, 2018

Choose a reason for hiding this comment

Uh oh!

munnerz Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

Queuecumber Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

Queuecumber Sep 12, 2018

Choose a reason for hiding this comment

Uh oh!

munnerz commented Sep 13, 2018

Uh oh!

Queuecumber commented Sep 13, 2018

Uh oh!

Queuecumber commented Sep 13, 2018

Uh oh!

munnerz commented Sep 13, 2018

Uh oh!

munnerz commented Sep 13, 2018

Uh oh!

Queuecumber Sep 14, 2018

Choose a reason for hiding this comment

Uh oh!

munnerz Oct 29, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Queuecumber commented Sep 15, 2018

Uh oh!

Queuecumber commented Sep 15, 2018

Uh oh!

Queuecumber commented Nov 12, 2018

Uh oh!

Queuecumber commented Nov 12, 2018

Uh oh!

Queuecumber commented Nov 13, 2018

Uh oh!

munnerz commented Nov 14, 2018

Uh oh!

jetstack-bot commented Nov 14, 2018

Uh oh!

Queuecumber commented Nov 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Queuecumber commented Sep 12, 2018 •

edited

Loading