Page MenuHomePhabricator

[Docs] Document keyserver deployment
ClosedPublic

Authored by varun on Jul 19 2023, 3:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 7:18 AM
Unknown Object (File)
Sat, Nov 23, 4:18 AM
Unknown Object (File)
Sat, Nov 23, 4:18 AM
Unknown Object (File)
Tue, Nov 19, 5:45 AM
Unknown Object (File)
Tue, Nov 19, 5:45 AM
Unknown Object (File)
Tue, Nov 19, 5:45 AM
Unknown Object (File)
Tue, Nov 19, 5:45 AM
Unknown Object (File)
Tue, Nov 19, 5:45 AM
Subscribers

Details

Summary

While reviewing D8538, I had difficulty with deploying keyserver
as a docker image and pointing it to production identity service because
it required some additional steps to configure this properly.

https://linear.app/comm/issue/ENG-4416

Test Plan

N/A, documentation

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested changes to this revision.Jul 23 2023, 8:27 AM
varun added inline comments.
docs/nix_dev_env.md
139 ↗(On Diff #28870)

I don't think keyserver is considered a service. maybe it should have its own markdown file

docs/nix_services_deployment.md
5 ↗(On Diff #28870)
9 ↗(On Diff #28870)
17–18 ↗(On Diff #28870)
17–18 ↗(On Diff #28870)

maybe move this to the end of the .env file

This revision now requires changes to proceed.Jul 23 2023, 8:27 AM
jon marked 5 inline comments as done.

Apply feedback

docs/nix_dev_env.md
139 ↗(On Diff #28870)

It's a web server; so at least traditionally it can be thought of as a service which responds to http requests.

In comm terminology though, it's a device; so it kind of walks both lines.

Ultimately, I lumped it in as a service because we need to treat it like a service with deployment and monitoring.

docs/nix_services_deployment.md
17–18 ↗(On Diff #28870)

This documentation might be relevant in the future for "keyserver owners".

Also, "PRODUCTION ONLY!!!" seems very misleading as the fallout of https://phab.comm.dev/D8538 denotes that we should test connectivity against production.

17–18 ↗(On Diff #28870)

In the near future, this will hard fail if it's unable to contact an identity server. Whereas the rest of the file is situational.

varun added 1 blocking reviewer(s): ashoat.
varun added inline comments.
docs/nix_dev_env.md
139 ↗(On Diff #28870)

curious what @ashoat thinks

docs/nix_services_deployment.md
17–18 ↗(On Diff #28870)

This documentation might be relevant in the future for "keyserver owners".

Good point. How about # Required to connect to production Identity service

ashoat requested changes to this revision.Jul 27 2023, 12:06 PM
ashoat added inline comments.
docs/nix_dev_env.md
139 ↗(On Diff #28983)

Agree with @varun that keyserver shouldn't be considered one of our services

docs/nix_services_deployment.md
11 ↗(On Diff #28983)

You're missing URL config, which is pretty necessary in order to be able to test the keyserver:

COMM_JSONCONFIG_facts_landing_url='{"baseDomain":"http://localhost","basePath":"/commlanding/","baseRoutePath":"/commlanding/","https":false}'
COMM_JSONCONFIG_facts_commapp_url='{"baseDomain":"http://localhost:3000","basePath":"/comm/","https":false,"baseRoutePath":"/comm/","proxy":"none"}'
14–15 ↗(On Diff #28983)

Don't think we should say <located in keyserver/secrets/db_config.json>".. there's no requirement that this matches that value, and somebody should be able to configure the keyserver Docker image without previously having a db_config.json file

These values honestly just need to be set to something (can be whatever)

16 ↗(On Diff #28983)

It might be good to explain what these are, maybe outside of the code block. I could imagine some explanation outside of the code block of:

  1. One section to explain MariaDB config
  2. One section to explain identity config (identity_service_config and user_credentials)
  3. One section for ETH login
  4. One section for URL config
17 ↗(On Diff #28983)
  1. Does this comment format work in .env files? Good to know if so
  2. We should be consistent about spacing patterns. It looks like you mostly have an extra newline between sections, but this section doesn't have the extra newline
20 ↗(On Diff #28983)
24–25 ↗(On Diff #28983)

Honestly I think we can just omit this

28 ↗(On Diff #28983)

In the title, you outline three steps: configuration, build, and deploy. Should we reflect that this section handles two of the steps?

30 ↗(On Diff #28983)
This revision now requires changes to proceed.Jul 27 2023, 12:06 PM
jon marked 11 inline comments as done.

Address feedback

docs/nix_dev_env.md
139 ↗(On Diff #28983)

Gave it its own section

139 ↗(On Diff #28870)

We talked in the one-on-one. He would like for keyserver to be considered separate from the projects in services/

docs/nix_services_deployment.md
14–15 ↗(On Diff #28983)

reduced to <MariaDB user/password>

16 ↗(On Diff #28983)

Gave a breakdown of service dependencies and services provided by Keyserver. Not sure if it's what you want.

17 ↗(On Diff #28983)

Does this comment format work in .env files? Good to know if so

I believe it's just a bash file being sourced

We should be consistent about spacing patterns.

I wanted it under the "Mandatory" section. Gave it its own section, but included Mandatory again to reinforce that it's not optional.

17–18 ↗(On Diff #28870)

Sure

ashoat resigned from this revision.EditedAug 1 2023, 8:24 AM

I haven't reviewed the latest pass. Please add me before landing. By the time you add me, I should be able to simply accept the diff. See my comment here for details.

This revision is now accepted and ready to land.Aug 1 2023, 8:24 AM
ashoat removed a reviewer: ashoat. ashoat added 1 blocking reviewer(s): varun.Aug 1 2023, 8:24 AM
This revision now requires review to proceed.Aug 1 2023, 8:24 AM

One of the new FAQ items we're putting up on the landing page is "What happens if my keyserver is lost?"

I'd like to be able to reference this doc to address it. Can we add the following to the configuration section?

# Example backup config that stores up to 10 GiB of backups in /home/comm/backups
COMM_JSONCONFIG_facts_backups='{"enabled":true,"directory":"/home/comm/backups","maxDirSizeMiB":10240}'
varun edited reviewers, added: jon; removed: varun.

A couple nits inline

docs/nix_keyserver_deployment.md
31 ↗(On Diff #29821)
  1. Comma splice
  2. "in this case" what case?

Maybe just omit the stuff after the comma?

47–48 ↗(On Diff #29821)

This might not be specific enough but we can always improve it later

52 ↗(On Diff #29821)

I'd prefer "max size" to "size"

This revision is now accepted and ready to land.Aug 10 2023, 10:53 AM
jon added inline comments.
docs/nix_keyserver_deployment.md
47–48 ↗(On Diff #29821)

Seems a little vague, and not very helpful if you need to configure these values. You may want to mention that it will reflect a proxy (e.g. apache) configuration if in use. Not sure if defining baseDomain, basePath, baseRoutePath, and proxy is a goal, but they seem pretty "magical" as they required but not explained.

I think at least baseDomain should be explained. If someone wanted to use a domain for their keyserver, they would need to set this value. If someone wanted to host the keyserver at https://keyserver.mydomain.com/ then they would need:

{
  "baseDomain": "https://keyserver.mydomain.com",
  "basePath": "/comm/",
  "https": false,
  "baseRoutePath": "/",
  "proxy": "none"
}

As far as I understand (ashoat can correct me):

  • baseDomain: base domain used for constructing links
  • baseRoutePath: Endpoint added to all links. (e.g. keyserver in https://mydomain.com/keyserver/)
  • basePath: Endpoint used by web app
  • proxy: "none" | "apache" Checks request headers and protocol to determine if request is secure.
  • https: If true, attempts proxy check above, otherwise avoids https check altogether.

So if someone want to see chat/thread/256|123/ resource, then the rendered url to the user would be:

# Using bash-style substitution
"${baseDomain}${baseRoutePath}chat/thread/256|123/"

And the proxy or load balancer would need to hit:

"http://${IP_ADDRESS_AND_PORT}${basePath}chat/thread/256|123/

I'm sure I have some details off, but Ashoat would know for certain.

This revision now requires changes to proceed.Aug 10 2023, 11:42 AM

we can move the expansion of commapp_url details to another diff, as ashoat mentioned

This revision is now accepted and ready to land.Aug 10 2023, 11:51 AM
docs/nix_keyserver_deployment.md
47–48 ↗(On Diff #29821)
  • COMM_JSONCONFIG_facts_landing_url: URL details for the landing page. Adjust properties based on your setup.

Let's skip this one. There's no reason for an at-home keyserver operator to configure the landing page.

  • COMM_JSONCONFIG_facts_commapp_url: URL details for the Comm web app. Adjust properties based on your setup.

Let's update this to:

  • COMM_JSONCONFIG_facts_commapp_url: Your keyserver needs to know what it's externally-facing URL is in order to contruct links. It also needs to know if it's being proxied to that externally-facing URL, and what the internal route path is.

Then we can break it down:

  • baseDomain: Externally-facing domain. Used for constructing links.
  • basePath: Externally-facing path. Used for constructing links.
  • baseRoutePath: Internally-facing path. Same as basePath if no proxy. If there's a proxy, this is the local path (eg. http://localhost:3000/landing would correspond with /landing/)
  • proxy: "none" | "apache" Determines which request headers to use for HTTPS validation and IP address timezone detection.
  • https: If true, checks request headers to validate that HTTPS is in use.
This revision was landed with ongoing or failed builds.Aug 12 2023, 11:20 AM
This revision was automatically updated to reflect the committed changes.
docs/nix_keyserver_deployment.md
50

Missed replacing apostrophe