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.
Details
- Reviewers
• jon ashoat - Commits
- rCOMM5e6f277fa1fb: [Docs] Document keyserver deployment
N/A, documentation
Diff Detail
- Repository
- rCOMM Comm
- Branch
- keyserver-docs (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
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. |
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:
|
17 ↗ | (On Diff #28983) |
|
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) |
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) |
I believe it's just a bash file being sourced
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 |
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.
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}'
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):
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. |
we can move the expansion of commapp_url details to another diff, as ashoat mentioned
docs/nix_keyserver_deployment.md | ||
---|---|---|
47–48 ↗ | (On Diff #29821) |
Let's skip this one. There's no reason for an at-home keyserver operator to configure the landing page.
Let's update this to:
Then we can break it down:
|
docs/nix_keyserver_deployment.md | ||
---|---|---|
50 ↗ | (On Diff #29844) | Missed replacing apostrophe |