Loading new certs on calling the admin/reloadconfig endpoint

[lakshdeepsingheventstore]

Fixed : Calling the admin/reloadconfig endpoint only reloaded/updated the LogLevel and the certificates were not updated on reloading the config.

Fixes : https://linear.app/eventstore/issue/DB-166/new-certs-are-not-loaded-on-calling-the-adminreloadconfig-endpoint

As of now, as per the codebase, we only allow reloading of certificates or LogLevel to be readjusted on reloading the config file by calling the admin/reloadconfig endpoint.

[linear]

DB-166 New certs are not loaded on calling the admin/reloadconfig endpoint

Steps taken to diagnose the issue :

1. Started a single node ESDB instance in secure mode using certs generated from es-gencert-cli.
2. Edited the config file to update the LogLevel and the certificates details to load new certificates.
3. Saved the edited config file.
4. Made a POST request to the admin/reloadconfig endpoint.
5. Although, the change in the LogLevel setting was reflected upon reloading the config, the new certs, as updated in the config file in Step 2 were not loaded, and the already being used certs were being reloaded.

To add more context to this issue, we even tried reloading the expired certs. Following steps were followed :

1. Started a single node ESDB instance using valid certs. The node came up well.
2. Changed the certs settings in the config file to point to certificates which had already expired some time ago. Also, updated the LogLevel in the config file.
3. Made a call to the reloadconfig endpoint. Although the log level was updated, but the already being used certs were being reloaded (that is the valid ones with which the node had been setup), and the expired ones were not loaded, and hence no error was reported.
4. On the other hand, when tried to setup a node, by stopping ESDB and restarting with the expired certs, then the appropriate certificate errors were logged and the node did not come up.

All the certs used in above tests have been generated using es-gencert-cli tool.

[timothycoleman]

Do we know if this has been broken for long time? perhaps we broke it recently somehow

Would be good to see a test if possible

[lakshdeepsingheventstore]

Do we know if this has been broken for long time? perhaps we broke it recently somehow
Would be good to see a test if possible

@timothycoleman I tested this on v22.10.2, and observed the same behavior. The steps I followed were as mentioned here

[timothycoleman]

I mean a unit test :) so that we dont accidentally break it again later

[hayley-jean]

Can you also check whether this is broken in 21.10 @lakshdeepsingheventstore ? If it is, it would be better to fix and merge it before we cut the release

[lakshdeepsingheventstore]

Can you also check whether this is broken in 21.10 @lakshdeepsingheventstore ? If it is, it would be better to fix and merge it before we cut the release

Tested this in v21.10.8. It works as expected in v21.10. Looks like the Dev Certificate changes might have broken the expected functionality.

[timothycoleman]

do we need the change to this file? it adds a lot to the diff but im not sure it actually does anything

[lakshdeepsingheventstore]

The changes that were made here were indentation changes. If you see line number 10, it says "namespace EventStore.Core;", which did not make any sense, since we still had to import EventStore.Core. So we added a brace after "namespace EventStore.Core;"
All the changes you see in the file are indentation changes, automatically implemented by Rider on getting an additional pair of braces.

[timothycoleman]

would it be possible to have a unit test that checks that the reload works correctly now, or is that too awkward?
looks pretty good. i added a comment above to simplify reviewing of this pr

these commits can be squashed into one commit. It's nice if the commit summary message completes the sentence "When applied, this commit will..." and then the body of the commit message can extra explantion if necessary

here the commit message could be "Use reloaded options when reloading certificates". to me that makes it easy to understand why the dboptions have been moved from the constructor to the method call

[timothycoleman]

Adding a test of the sort I asked for above is a bit fiddly, I've added a separate ticket to cover this off as part of the other runtime db config changes project https://linear.app/eventstore/issue/DB-237/improve-tests-around-live-config-changes

[timothycoleman]

great, thanks 👍

[github-actions]

🚨 @pvanbuijtene Failed to create cherry Pick PR due to error:

RequestError [HttpError]: Merge conflict
   at /home/runner/work/_actions/EventStore/Automations/master/cherry-pick-pr-for-label/node_modules/@octokit/request/dist-node/index.js:66:23
   at processTicksAndRejections (node:internal/process/task_queues:96:5) {
 status: 409,
 headers: {
   'access-control-allow-origin': '*',
   'access-control-expose-headers': 'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset',
   connection: 'close',
   'content-length': '112',
   'content-security-policy': "default-src 'none'",
   'content-type': 'application/json; charset=utf-8',
   date: 'Tue, 11 Jul 2023 13:15:38 GMT',
   'referrer-policy': 'origin-when-cross-origin, strict-origin-when-cross-origin',
   server: 'GitHub.com',
   'strict-transport-security': 'max-age=31536000; includeSubdomains; preload',
   vary: 'Accept-Encoding, Accept, X-Requested-With',
   'x-content-type-options': 'nosniff',
   'x-frame-options': 'deny',
   'x-github-api-version-selected': '2022-11-28',
   'x-github-media-type': 'github.v3; format=json',
   'x-github-request-id': '8C09:888D:149CAAF:1539BE3:64AD55FA',
   'x-ratelimit-limit': '1000',
   'x-ratelimit-remaining': '990',
   'x-ratelimit-reset': '1689084934',
   'x-ratelimit-resource': 'core',
   'x-ratelimit-used': '10',
   'x-xss-protection': '0'
 },
 request: {
   method: 'POST',
   url: 'https://api.github.com/repos/EventStore/EventStore/merges',
   headers: {
     accept: 'application/vnd.github.v3+json',
     'user-agent': 'octokit-core.js/3.3.2 Node.js/16.16.0 (linux; x64)',
     authorization: 'token [REDACTED]',
     'content-type': 'application/json; charset=utf-8'
   },
   body: '{"base":"cherry-pick-cherry-pick/3868/lakshdeepsingh/db-166-new-certs-are-not-loaded-on-calling-the-adminreloadconfig-release/oss-v22.10-0efe0d99-5876-4d26-b654-176fa0da9120","commit_message":"Merge 4107e9cbc0bb2a22a6ef355beda1249b6b2403b1 into cherry-pick-cherry-pick/3868/lakshdeepsingh/db-166-new-certs-are-not-loaded-on-calling-the-adminreloadconfig-release/oss-v22.10-0efe0d99-5876-4d26-b654-176fa0da9120 [skip ci]\\n\\n\\nskip-checks: true\\n","head":"4107e9cbc0bb2a22a6ef355beda1249b6b2403b1"}',
   request: { agent: [Agent], hook: [Function: bound bound register] }
 },
 documentation_url: 'https://docs.github.com/rest/branches/branches#merge-a-branch'
}

🚨👉 Check https://github.com/EventStore/EventStore/actions/runs/5520432389