Modify the ACME renewing logs level #2520

nmengin · 2017-12-04T14:02:51Z

What does this PR do?

This PR transforms the DEBUG logs generated during the ACME certificates renewing into INFO logs in the way to get more information about the process without getting all the others DEBUG logs.

Motivation

Related #2158 and slack support about this subject.

Additional Notes

This PR does not solve the problem but allows getting more information about it.
It seems to be a concurrent access problem.
A test is currently running in the way to repoduce the problem on a long period.

dtomcej · 2017-12-04T18:16:19Z

acme/acme.go

@@ -448,7 +477,7 @@ func dnsOverrideDelay(delay int) error {
 			return true, nil
 		}
 	} else if delay < 0 {
-		err = fmt.Errorf("Invalid negative DelayDontCheckDNS: %d", delay)
+		err = fmt.Errorf("invalid negative DelayDontCheckDNS: %d", delay)


How come some of these messages start with a capital letter, and others don't?

https://github.com/golang/go/wiki/CodeReviewComments#error-strings

dtomcej

LGTM

ldez · 2017-12-04T19:35:34Z

acme/acme.go

+	}
+	log.Infof("Commit Certificate %+v renewed in data store", certificateResource.Domains)
+	if err = transaction.Commit(account); err != nil {
+		return fmt.Errorf("error Saving ACME account %+v: %s", account, err.Error())


fmt.Errorf("error Saving ACME account %+v: %v", account, err)

ldez · 2017-12-04T19:36:29Z

acme/acme.go

-
-				if err = transaction.Commit(account); err != nil {
-					log.Errorf("Error Saving ACME account %+v: %s", account, err.Error())
+					log.Errorf("datastore cannot sync: %v", err)


could you starts with a upper-case letter.

ldez · 2017-12-04T19:38:06Z

acme/acme.go

-					PrivateKey:    certificateResource.Certificate.PrivateKey,
-					Certificate:   certificateResource.Certificate.Certificate,
-				}, true, OSCPMustStaple)
+				log.Infof("Renewing certificate %+v from LE", certificateResource.Domains)


log.Infof("Renewing certificate from LE: %+v", certificateResource.Domains)

ldez · 2017-12-04T19:38:45Z

acme/acme.go

 				if err != nil {
-					log.Errorf("Error renewing certificate: %v", err)
+					log.Errorf("Error renewing from LE certificate: %v", err)


log.Errorf("Error renewing certificate from LE: %v", err)

ldez · 2017-12-04T19:39:16Z

acme/acme.go

+	if err != nil {
+		return nil, err
+	}
+	log.Infof("Renewed certificate %+v from  LE", certificateResource.Domains)


log.Infof("Renewed certificate from LE: %+v", certificateResource.Domains)

ldez · 2017-12-04T19:39:46Z

acme/acme.go

+		return fmt.Errorf("Error during transaction initialization for renewing certificate: %v", err)
+	}
+	account = object.(*Account)
+	log.Infof("Renewing certificate %+v in data store", certificateResource.Domains)


log.Infof("Renewing certificate in data store: %+v", certificateResource.Domains)

ldez · 2017-12-04T19:40:23Z

acme/acme.go

+	log.Infof("Renewing certificate %+v in data store", certificateResource.Domains)
+	err = account.DomainsCertificate.renewCertificates(renewedACMECert, certificateResource.Domains)
+	if err != nil {
+		return fmt.Errorf("error renewing certificate: %v in datastore", err)


return fmt.Errorf("error renewing certificate in datastore: %v", err)

ldez · 2017-12-04T19:40:49Z

acme/acme.go

+	oldAccount := a.store.Get().(*Account)
+	for _, oldCertificateResource := range oldAccount.DomainsCertificate.Certs {
+		if oldCertificateResource.Domains.Main == certificateResource.Domains.Main && strings.Join(oldCertificateResource.Domains.SANs, ",") == strings.Join(certificateResource.Domains.SANs, ",") && certificateResource.Certificate != renewedACMECert {
+			return fmt.Errorf("renewed certificate %+v not stored", certificateResource.Domains)


return fmt.Errorf("renewed certificate not stored: %+v", certificateResource.Domains)

ldez · 2017-12-04T19:41:07Z

acme/acme.go

+			return fmt.Errorf("renewed certificate %+v not stored", certificateResource.Domains)
+		}
+	}
+	log.Infof("Certificate %+v successfully renewed in data store", certificateResource.Domains)


log.Infof("Certificate successfully renewed in data store: %+v", certificateResource.Domains)

ldez · 2017-12-04T19:51:38Z

acme/acme.go

+	if err != nil {
+		return fmt.Errorf("Error during transaction initialization for renewing certificate: %v", err)
+	}
+	account = object.(*Account)


Could you add blank lines in this function to aerate the code.

juliens

LGTM

ldez

LGTM

traefiker added area/acme size/S status/0-needs-triage labels Dec 4, 2017

traefiker added this to the 1.5 milestone Dec 4, 2017

nmengin added status/2-needs-review and removed status/0-needs-triage labels Dec 4, 2017

ldez added the kind/bug/fix a bug fix label Dec 4, 2017

dtomcej suggested changes Dec 4, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Dec 4, 2017

ldez removed the contributor/waiting-for-corrections label Dec 4, 2017

dtomcej approved these changes Dec 4, 2017

View reviewed changes

ldez requested changes Dec 4, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Dec 4, 2017

nmengin force-pushed the feature/add-logs-during-acme-renew branch from c85099d to b6095de Compare December 5, 2017 08:42

nmengin removed the contributor/waiting-for-corrections label Dec 5, 2017

juliens approved these changes Dec 5, 2017

View reviewed changes

ldez approved these changes Dec 5, 2017

View reviewed changes

ldez added status/3-needs-merge and removed status/2-needs-review labels Dec 5, 2017

traefiker added the status/4-merge-in-progress label Dec 5, 2017

nmengin added 2 commits December 5, 2017 14:28

Add logs INFO during ACME certificates renewing

8820923

Fix review

0681fe5

traefiker force-pushed the feature/add-logs-during-acme-renew branch from b6095de to 0681fe5 Compare December 5, 2017 14:28

traefiker removed the status/4-merge-in-progress label Dec 5, 2017

traefiker merged commit 6333bfe into traefik:v1.5 Dec 5, 2017

traefiker removed the status/3-needs-merge label Dec 5, 2017

nmengin mentioned this pull request Dec 6, 2017

Traefik stopped being able to renew TLS cert #2158

Closed

nmengin deleted the feature/add-logs-during-acme-renew branch August 3, 2018 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify the ACME renewing logs level #2520

Modify the ACME renewing logs level #2520

nmengin commented Dec 4, 2017

dtomcej Dec 4, 2017

ldez Dec 4, 2017

dtomcej left a comment

ldez Dec 4, 2017

ldez Dec 4, 2017

ldez Dec 4, 2017

ldez Dec 4, 2017

ldez Dec 4, 2017

ldez Dec 4, 2017

ldez Dec 4, 2017

ldez Dec 4, 2017

ldez Dec 4, 2017

ldez Dec 4, 2017

juliens left a comment

ldez left a comment

Modify the ACME renewing logs level #2520

Modify the ACME renewing logs level #2520

Conversation

nmengin commented Dec 4, 2017

What does this PR do?

Motivation

Additional Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtomcej 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

Choose a reason for hiding this comment

juliens left a comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment