Updating amppackage.example.toml documentation. #361

banaag · 2019-10-24T21:15:30Z

Updating amppackage.example.toml documentation for newly added fields for cert autorenewal.
Addresses #93

googlebot · 2019-10-24T21:15:37Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

banaag · 2019-10-24T21:16:08Z

@googlebot

googlebot · 2019-10-24T21:16:12Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

twifkak

Thanks for the docs! Looks pretty good, just a bunch of nits.

twifkak · 2019-11-12T05:55:32Z

amppkg.example.toml

@@ -170,3 +188,75 @@ ForwardedRequestHeaders = []
  # Domain = "www.corp.amppackageexample.com"
  # PathRE = "/world/.*"
  # QueryRE = ""
+
+# ACME is a protocol that allows for automatic renewal of certificates. Amp packager uses an ACME library


s/Amp/AMP/ (ditto lines 229 and 237) and capitalize packager as Packager whenever it follows "AMP" (ditto below)

twifkak · 2019-11-12T05:58:44Z

amppkg.example.toml

@@ -170,3 +188,75 @@ ForwardedRequestHeaders = []
  # Domain = "www.corp.amppackageexample.com"
  # PathRE = "/world/.*"
  # QueryRE = ""
+
+# ACME is a protocol that allows for automatic renewal of certificates. Amp packager uses an ACME library
+# https://github.com/go-acme/lego to handle certificate renewal.


I would move details about the spec and the Go library to the bottom of this prologue comment. Focus on what it does, how to enable it, and any gotchas.

Fixed, let me know if the changes are satisfactory.

twifkak · 2019-11-13T22:47:16Z

amppkg.example.toml

@@ -47,6 +47,24 @@
 # SHA-256).
 CertFile = './pems/cert.pem'

+# The path to save a new cert retrieved from the CA if the current cert in
+# 'CertFile' above is still valid.
+# This is optional and is needed only if you have 'autorenewcert' # turned on.


Is this optional? My build complains if it's missing, and it looks like the code checks for it too:

amppackager/packager/certcache/certcache.go

Line 833 in e416691

if config.CertFile == "" || config.NewCertFile == "" {

.

Fixed in #372

twifkak · 2019-11-13T22:47:24Z

amppkg.example.toml

+#      https://www.digicert.com/csr-creation.htm?rid=011592
+#      https://www.ssl.com/how-to/manually-generate-a-certificate-signing-request-csr-using-openssl/
+#      https://geekflare.com/san-ssl-certificate/
+# This is optional and is needed only if you have 'autorenewcert' # turned on.


twifkak · 2019-11-13T22:48:54Z

amppkg.example.toml

@@ -47,6 +47,24 @@
 # SHA-256).
 CertFile = './pems/cert.pem'

+# The path to save a new cert retrieved from the CA if the current cert in
+# 'CertFile' above is still valid.
+# This is optional and is needed only if you have 'autorenewcert' # turned on.


Clarify in the docs: For multi-replica setups, does this need to be set on the non-autorenewcert replicas?

twifkak · 2019-11-13T23:07:33Z

amppkg.example.toml

+    # 	https://go-acme.github.io/lego/dns/
+    # DnsProvider = "gcloud"
+  # [ACMEConfig.Development]
+  # This config will ne used if 'autorenewcert' is turned on and 'development' is turned on.


What happens if -autorenewcert -development is turned on, but no [ACMEConfig.Development] is specified here? The above text says this is okay, but ISTR amppkg will refuse to start in this case.

The text just says this config will be used if those flags are on. I added more verbage that says if the flags are on and the config is missing, AMP Packager will fail to start.

twifkak · 2019-11-13T23:08:45Z

amppkg.example.toml

+# in 'CertFile' and 'NewCertFile' are located on a shared filesystem accessible by all AMP packager instances.
+# [ACMEConfig]
+  # [ACMEConfig.Production]
+  # This config will be used if 'autorenewcert' is turned on and 'development' is turned off.


Move this comment above [ACMEConfig.Production] to match the style in this file.

twifkak · 2019-11-13T23:09:06Z

amppkg.example.toml

+  # This config will ne used if 'autorenewcert' is turned on and 'development' is turned on.
+  # All the other fields below have the same semantics as the one in ACMEConfig.Production above.


Ditto, move this comment above [ACMEConfig.Development].

twifkak · 2019-11-13T23:11:09Z

amppkg.example.toml

+    # DnsProvider = "gcloud"
+  # [ACMEConfig.Development]
+  # This config will ne used if 'autorenewcert' is turned on and 'development' is turned on.
+  # All the other fields below have the same semantics as the one in ACMEConfig.Production above.


I would add a suggestion of Let's Encrypt in development, since it's free and development-mode doesn't require the SXG extension. (This lines up with how Matt suggested use of development-mode in the talk.)

twifkak · 2019-11-13T23:11:27Z

amppkg.example.toml

+  # [ACMEConfig.Development]
+  # This config will ne used if 'autorenewcert' is turned on and 'development' is turned on.
+  # All the other fields below have the same semantics as the one in ACMEConfig.Production above.
+    # DiscoURL = "development-acme.discovery.url"


Ditto make it look like a URL.

banaag · 2019-11-15T17:19:26Z

@twifkak comments fixed

alabiaga · 2019-11-21T18:38:08Z

cc\ @alabiaga

twifkak

LGTM % nit

twifkak · 2019-12-05T00:27:18Z

amppkg.example.toml

-    #
-    # DiscoURL = "production-acme.discovery.url"
-    #
+


Remove this blank line, so each comment is grouped with its field. So the format is like:

# Comment explaining Field. Blah blah lorem # ipsum blah. # Field = "foo" # Comment explaining OtherField. Oh wow so # interesting. # OtherField = true

twifkak · 2019-12-05T00:27:28Z

amppkg.example.toml

    # This is the email address you used to create an account with the Certificate Authority that is registered to
    # request signed exchange certificates.
-    #
+


Ditto this one.

alabiaga · 2019-12-05T00:42:36Z

since @banaag is away, any chances you can merge this @twifkak? and he can address those comments at another time. so I can make changes to #374, which needs to change because of this PR

twifkak · 2019-12-05T01:06:38Z

Wilco. It turns out I can just make the changes in this case.

First pass at updating amppkg.example.toml

e951e53

banaag requested a review from twifkak October 24, 2019 21:15

googlebot added the cla: no label Oct 24, 2019

banaag changed the base branch from releases to master October 24, 2019 21:15

googlebot added cla: yes and removed cla: no labels Oct 24, 2019

banaag requested a review from Gregable October 24, 2019 21:16

Added some more comments regarding challenges.

2732e42

twifkak requested changes Nov 13, 2019

View reviewed changes

ithinkihaveacat mentioned this pull request Nov 15, 2019

Example config missing NewCertFile key #370

Closed

Fix twifkak@ comments.

39e3980

twifkak approved these changes Dec 5, 2019

View reviewed changes

blank lines

5fe643e

twifkak merged commit d88c537 into master Dec 5, 2019

twifkak deleted the toml-branch branch December 5, 2019 01:11

This was referenced Dec 5, 2019

Support multiple certs in configuration #363

Open

multi domain cert/key support #374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating amppackage.example.toml documentation. #361

Updating amppackage.example.toml documentation. #361

banaag commented Oct 24, 2019

googlebot commented Oct 24, 2019

banaag commented Oct 24, 2019

googlebot commented Oct 24, 2019

twifkak left a comment

twifkak Nov 12, 2019

banaag Nov 14, 2019

twifkak Nov 12, 2019

banaag Nov 14, 2019

twifkak Nov 13, 2019

banaag Nov 14, 2019

ithinkihaveacat Nov 15, 2019

banaag Nov 15, 2019

twifkak Nov 13, 2019

banaag Nov 14, 2019

twifkak Nov 13, 2019

banaag Nov 15, 2019

twifkak Nov 13, 2019

banaag Nov 15, 2019

twifkak Nov 13, 2019

banaag Nov 15, 2019

twifkak Nov 13, 2019

banaag Nov 15, 2019

twifkak Nov 13, 2019

banaag Nov 15, 2019

twifkak Nov 13, 2019

banaag Nov 15, 2019

banaag commented Nov 15, 2019

alabiaga commented Nov 21, 2019

twifkak left a comment

twifkak Dec 5, 2019

twifkak Dec 5, 2019

alabiaga commented Dec 5, 2019

twifkak commented Dec 5, 2019

		# This config will ne used if 'autorenewcert' is turned on and 'development' is turned on.
		# All the other fields below have the same semantics as the one in ACMEConfig.Production above.

Updating amppackage.example.toml documentation. #361

Updating amppackage.example.toml documentation. #361

Conversation

banaag commented Oct 24, 2019

googlebot commented Oct 24, 2019

banaag commented Oct 24, 2019

googlebot commented Oct 24, 2019

twifkak 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

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

Choose a reason for hiding this comment

banaag commented Nov 15, 2019

alabiaga commented Nov 21, 2019

twifkak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alabiaga commented Dec 5, 2019

twifkak commented Dec 5, 2019