Page MenuHomePhabricator

[docs] Include documentation on running your own self-hosted keyserver through aws
ClosedPublic

Authored by will on Aug 19 2024, 3:29 AM.
Tags
None
Referenced Files
F3193244: D13105.id.diff
Fri, Nov 8, 10:36 PM
F3185849: D13105.diff
Fri, Nov 8, 1:44 PM
Unknown Object (File)
Fri, Nov 8, 8:13 AM
Unknown Object (File)
Thu, Nov 7, 11:32 PM
Unknown Object (File)
Thu, Nov 7, 11:53 AM
Unknown Object (File)
Tue, Oct 29, 9:01 AM
Unknown Object (File)
Tue, Oct 29, 4:52 AM
Unknown Object (File)
Mon, Oct 28, 4:14 AM
Subscribers

Details

Summary

Initial documentation on setting up your own self-hosted keyserver through aws

Test Plan

Was able to, from a clean aws account, deploy a fresh running keyserver following only directions from the docs

Diff Detail

Repository
rCOMM Comm
Branch
self_host_keyserver
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
docs/keyserver_aws_self_host.md
1 ↗(On Diff #43443)

Hmmm... shouldn't we have a header here, eg. "Self-hosting a keyserver on AWS"?

If we add that, I think all the headers should go down a level, eg. ## -> ###

15–17 ↗(On Diff #43443)

We have several prerequisites for running the scripts and Terraform logic that will help you manage your keyserver on AWS.

These prerequisites are all packaged together in our provided Nix development shell.

38 ↗(On Diff #43443)

It's weird that this is a subsection of "Create development shell". Can you rethink the "Table of Contents" here?

62 ↗(On Diff #43443)
64–66 ↗(On Diff #43443)

Not sure we need these lines

68 ↗(On Diff #43443)

What does "entered past" mean?

87 ↗(On Diff #43443)

Weird that the next line has a : but this line doesn't

90 ↗(On Diff #43443)

Nit: I'd drop the trailing slash

96 ↗(On Diff #43443)

I don't see what touch is doing here. I can't think of a text editor that cares whether a file exists prior to being edited. Why not just use vim, and suggest the user substitute with their desired text editor? You could also use $EDITOR

113 ↗(On Diff #43443)

What's this mean? What it explained earlier?

117 ↗(On Diff #43443)
119 ↗(On Diff #43443)
121 ↗(On Diff #43443)

Don't think we need this line

121–123 ↗(On Diff #43443)
127 ↗(On Diff #43443)

It's optional, right?

129 ↗(On Diff #43443)
135 ↗(On Diff #43443)

I think we can remove this line

137 ↗(On Diff #43443)
139 ↗(On Diff #43443)

Can we separate out the line of code?

143 ↗(On Diff #43443)

Think "above specified" is obvious

149 ↗(On Diff #43443)
154–162 ↗(On Diff #43443)

Don't we need more configs? How about all the license keys? I believe that I documented somewhere on Linear the list of params that I thought we needed on keyserver vs. web vs. landing... can you dig up that Linear comment, and try to make sure you cover everything the keyserver needs?

165 ↗(On Diff #43443)

Not all variables need to be modified, right? Eg. the CORS config

176 ↗(On Diff #43443)

It's weird to use inline code formatting for a separate line of code. Why not use the same three-backticks approach you use directly above?

178 ↗(On Diff #43443)
183 ↗(On Diff #43443)
189 ↗(On Diff #43443)

I thought ALIAS was non-standard. Don't some domain registrars frame this as a CNAME at the root? Do we need to explain the requirement of support for the non-standard ALIAS / root CNAME record earlier?

This revision now requires changes to proceed.Aug 20 2024, 12:24 PM
docs/keyserver_aws_self_host.md
38 ↗(On Diff #43443)

This might have been a misunderestanding. Create a development shell is just a comment. Removing it because The Nix development shell can be run with: is right above and the comment isn't necessary.

68 ↗(On Diff #43443)

Better word here wouldn've been skipped. Including in next rebase

87 ↗(On Diff #43443)

Going to keep this consistent by always include :

96 ↗(On Diff #43443)

Good point. This is specific to my habits. Going to just replace with suggesting they use their own desired text editor

113 ↗(On Diff #43443)

This rightfully could be confusing. Original instructions didn't have users set a region if they were using export to use their aws credentials. I'm going to change prior instructions to just require setting AWS_REGION by default.

127 ↗(On Diff #43443)

Currently not optional. terraform apply needs access to MariaDB to initialize the Comm database

189 ↗(On Diff #43443)

ALIAS is non-standard. I'll include a comment above about the need for ALIAS and root CNAME records with a recommendation to configure dns rules through route53 if their domain registrar does not support these records

will removed a reviewer: bartek. will added 1 blocking reviewer(s): ashoat.Sep 3 2024, 7:22 AM
ashoat requested changes to this revision.Sep 5 2024, 1:30 PM
ashoat added inline comments.
docs/keyserver_aws_self_host.md
1 ↗(On Diff #43822)

Let's make "keyserver" not capitalized. We seem to be inconsistent about this in top-level headings, but in general we don't capitalize words in headings unless they're the first word, or they're part of a proper noun

3 ↗(On Diff #43822)

This is the only second-level header. As requested in my last review, please re-think the table of contents

65 ↗(On Diff #43822)
112 ↗(On Diff #43822)

Why doesn't this paragraph have a line break above it, like all the other paragraphs?

116 ↗(On Diff #43822)

"Allows" is a really weird word choice for something that is required

145–153 ↗(On Diff #43822)

Don't we need more configs? How about all the license keys? I believe that I documented somewhere on Linear the list of params that I thought we needed on keyserver vs. web vs. landing... can you dig up that Linear comment, and try to make sure you cover everything the keyserver needs?

156 ↗(On Diff #43822)

Not all variables need to be modified, right? Eg. the CORS config

This revision now requires changes to proceed.Sep 5 2024, 1:30 PM
will marked 2 inline comments as done.

review feedback

ashoat requested changes to this revision.Sep 8 2024, 3:42 AM

Getting close, but still some comments here

docs/keyserver_aws_self_host.md
111 ↗(On Diff #43950)

Can you go through the doc and replace all the places where you used a single-quote character for an apostrophe with a proper apostrophe character? Like this: ’

147–148 ↗(On Diff #43950)
  1. Note the inline edit
  2. Is this the only place we use bullets? Does it look a little weird? Wondering if we should make it consistent with the other formats – have a subheading with database config, and a subheading with user credentials
156 ↗(On Diff #43950)

Should we skip the bullet point here? Seems unnecessary and inconsistent with the following sections

162 ↗(On Diff #43950)
168–172 ↗(On Diff #43950)

I looked around the keyserver code and I actually don't think this is used there... only for web / landing. Let's remove it

174 ↗(On Diff #43950)
180 ↗(On Diff #43950)
188 ↗(On Diff #43950)
192 ↗(On Diff #43950)
206 ↗(On Diff #43950)
211 ↗(On Diff #43950)

The spacing is different here from the other ones. Can you make the whitespace consistent across all the example configs that include JSON?

229 ↗(On Diff #43950)

Technically the user can pick whatever they want for this, right? Should we move it next to the other database credentials?

234 ↗(On Diff #43950)

Terraform is a proper noun. Please make sure you capitalize all proper nouns where appropriate

This revision now requires changes to proceed.Sep 8 2024, 3:42 AM
will marked 13 inline comments as done.

review feedback

typo. accidental uppercase

Please address inline comments before landing, and do a final sweep for apostrophes

docs/keyserver_aws_self_host.md
171 ↗(On Diff #44002)

Replace apostrophe

177 ↗(On Diff #44002)

Replace apostrophe

183 ↗(On Diff #44002)

Replace apostrophe

252–253 ↗(On Diff #44002)

Is the newline between these lines necessary?

This revision is now accepted and ready to land.Sep 9 2024, 2:25 PM
will marked 4 inline comments as done.

addressed apostrophes and unneeded newline in latest rebase