Add handling of updates to ACME email field in Issuers #1763

cheukwing · 2019-06-07T14:44:02Z

Signed-off-by: Michael Tsang michael.tsang@jetstack.io

What this PR does / why we need it:
When the email in the spec.acme field of an Issuer changes, compares this with the newly added field lastRegisteredEmail in status.acme.
If they are different then the registering process is begun, otherwise nothing.
On success the field is updated to the spec value, otherwise on failure or no difference it is kept the same.
As the status has changed then updateIssuerStatus will be called, and changes should be made to the actual Issuer.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1667

Release note:

Adds the handling of updates to the `spec.acme.email` field in Issuers

munnerz · 2019-06-11T14:07:00Z

pkg/issuer/acme/setup.go

@@ -166,6 +166,11 @@ func (a *Acme) Setup(ctx context.Context) error {

 	// registerAccount will also verify the account exists if it already
 	// exists.
+	if a.issuer.GetStatus().ACME.LastRegisteredEmail == a.issuer.GetSpec().ACME.Email {


I'm unsure whether we want to explicitly bail out/stop processing here just if the status == the spec (i.e. what if the users private key was deleted? In this instance we should be generating a new private key and re-registering an account).

Also, the proper way to 'handle' updating the email field is for us to actually update the email address associated with the ACME account. You can see the actual method you can use to do this here: https://github.com/jetstack/cert-manager/blob/9c6d81a92a192eb95be90084956dc04c0cb7552c/third_party/crypto/acme/acme.go#L371-L373

In terms of how the issuer's status.registeredEmail field is set, I think to make things clearer and easier to keep consistent, this field should always be set to the value on the Account object returned from the ACME server, instead of setting it to the value of spec.acme.email (as it could fall out of sync and we could be incorrectly reporting the status).

I'll have another look, thanks!

…t registered ACME account Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>

Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>

cheukwing · 2019-06-12T16:40:05Z

I've rewritten it and added an e2e test too.

munnerz

This looks brilliant 😄 a few comments, mostly small and non-functional!

munnerz · 2019-06-17T12:27:19Z

pkg/issuer/acme/setup.go

+	// if we got an account successfully, we must check if the registered
+	// email is the same as in the issuer spec
+	registeredEmail := ""
+	if err == nil {


Instead of wrapping this block in if err == nil, can we move it to after the err != nil check and thus assume that it is nil? We can then throw a distinct warning that updating the account failed if err != nil from: account, err = cl.UpdateAccount(ctx, account).

I wrote it before the err != nil block as it seemed that we would probably want to handle any errors from updating in a similar fashion to any from registering.
Should errors from updating just log a message and return the error, or should it behave as before but with all instances of verify changed to update?

Yeah I think we should duplicate the logic and update the messages.

We can also log an event to say we are updating the email address here too, which will help users be sure that the email address has been updated.

We should also keep the existing error code checking logic to handle known ACME error codes and skip retrying, to save DoSing the ACME API.

munnerz · 2019-06-17T12:28:24Z

pkg/issuer/acme/setup.go

@@ -195,6 +218,7 @@ func (a *Acme) Setup(ctx context.Context) error {
 	log.Info("verified existing registration with ACME server")
 	apiutil.SetIssuerCondition(a.issuer, v1alpha1.IssuerConditionReady, v1alpha1.ConditionTrue, successAccountRegistered, messageAccountRegistered)
 	a.issuer.GetStatus().ACMEStatus().URI = account.URL
+	a.issuer.GetStatus().ACMEStatus().LastRegisteredEmail = registeredEmail


👍 great, this means we will only ever set LastRegisteredEmail to the value received from the ACME server.

munnerz · 2019-06-17T12:29:55Z

pkg/issuer/acme/setup.go

+		}
+
+		// if they are different, we update the account
+		if registeredEmail != a.issuer.GetSpec().ACME.Email {


Maybe an edge case, but just calling it out to be sure you've considered it... if a user does not specify an email address at all, I guess the len(account.Contact) will be 0, meaning registeredEmail == "", meaning this whole thing should be skipped as expected. Fairly certain that is the case (especially now I've written it out here 😉), but just calling it out. Perhaps copy and paste some of this description into a comment? I know that future-us will appreciate it 😄

Yep, since "" is the zero-value of spec.acme.email.
I shall add a comment.

Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>

munnerz · 2019-06-18T13:58:20Z

/retest

munnerz · 2019-07-03T10:20:53Z

/lgtm
/approve

jetstack-bot · 2019-07-03T10:21:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheukwing, munnerz

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

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

retest-bot · 2019-07-03T11:36:10Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to jetstack).
Review the full test history for this PR.
Silence the bot with an /lgtm cancel comment for consistent failures.

jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Jun 7, 2019

jetstack-bot requested review from kragniz and munnerz June 7, 2019 14:44

jetstack-bot added area/acme Indicates a PR directly modifies the ACME Issuer code area/api Indicates a PR directly modifies the 'pkg/apis' directory size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 7, 2019

cheukwing force-pushed the issue-1667 branch from 3e90c8f to 1f7122f Compare June 7, 2019 14:57

jetstack-bot added the area/deploy Indicates a PR modifies deployment configuration label Jun 7, 2019

munnerz reviewed Jun 11, 2019

View reviewed changes

cheukwing added 2 commits June 12, 2019 09:42

Add LastRegisteredEmail field to ACMEIssuerStatus, to track the lates…

092e9c0

…t registered ACME account Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>

Check ACME issuer registered emails after first verifying

4b349f0

Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>

cheukwing force-pushed the issue-1667 branch from 1f7122f to 4b349f0 Compare June 12, 2019 13:51

jetstack-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 12, 2019

cheukwing changed the title ~~Add handling of updates to ACME email field in Issuers~~ WIP: Add handling of updates to ACME email field in Issuers Jun 12, 2019

jetstack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 12, 2019

Add e2e test for changing email

d2b350f

Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>

cheukwing changed the title ~~WIP: Add handling of updates to ACME email field in Issuers~~ Add handling of updates to ACME email field in Issuers Jun 12, 2019

munnerz reviewed Jun 17, 2019

View reviewed changes

Move email updating after register error handling

d7ce606

Signed-off-by: Michael Tsang <michael.tsang@jetstack.io>

munnerz added this to the v0.9 milestone Jul 3, 2019

jetstack-bot assigned munnerz Jul 3, 2019

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 3, 2019

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2019

jetstack-bot merged commit 8e54b32 into cert-manager:master Jul 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add handling of updates to ACME email field in Issuers #1763

Add handling of updates to ACME email field in Issuers #1763

cheukwing commented Jun 7, 2019

munnerz Jun 11, 2019

cheukwing Jun 11, 2019

cheukwing commented Jun 12, 2019

munnerz left a comment

munnerz Jun 17, 2019

cheukwing Jun 17, 2019

munnerz Jun 17, 2019

munnerz Jun 17, 2019

munnerz Jun 17, 2019

munnerz Jun 17, 2019

cheukwing Jun 17, 2019

munnerz commented Jun 18, 2019

munnerz commented Jul 3, 2019

jetstack-bot commented Jul 3, 2019

retest-bot commented Jul 3, 2019

Add handling of updates to ACME email field in Issuers #1763

Add handling of updates to ACME email field in Issuers #1763

Conversation

cheukwing commented Jun 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheukwing commented Jun 12, 2019

munnerz 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

munnerz commented Jun 18, 2019

munnerz commented Jul 3, 2019

jetstack-bot commented Jul 3, 2019

retest-bot commented Jul 3, 2019