[services] Tunnelbroker - Add the ability to change the default config files directory to run in Nix
ClosedPublic
Actions

Authored by • max on Aug 15 2022, 4:32 PM.

Details

Reviewers

• karol
tomek
varun
• jon

Commits

rCOMM22c5aac90661: [services] Tunnelbroker - Add the ability to change the default config files…

Summary

By default, the Tunnelbroker looks for the config files in a ~/tunnelbroker/ directory. To add the ability to run the Tunnelbroker in Nix or any other environment we should add the ability to change the default directory for config files location. The ability to change the default config files directory is added by using the TUNNELBROKER_CONFIG_FILE_DIRECTORY exported environment variable.

This task introduces a few small related changes:

Constants for the config files location were split into directory and file names for the production and sandbox configs.
Tunnelbroker's tunnelbroker-dev.ini config renamed to tunnelbroker-sandbox.ini.
Add the ability to overwrite the config files directory by using of exported TUNNELBROKER_CONFIG_FILE_DIRECTORY environment variable in a ConfigManager::load() method.

Related Linear task: ENG-1589

Test Plan

Docker:

Running of the yarn run-tunnelbroker-service-in-sandbox successfully built the app and the ~/tunnelbroker/ directory is used as the config files location.

Nix:

Enter the nix environment by using nix develop command.
Run cmake -B build . && make -C build -j4 in the services/tunnelbroker directory to build the service.
Export the TUNNELBROKER_CONFIG_FILE_DIRECTORY environment variable with the directory of the service location (services/tunnelbroker).
Running of build/bin/tunnelbroker binary will not produce any config-related errors.

Exporting the wrong location using the TUNNELBROKER_CONFIG_FILE_DIRECTORY and running of build/bin/tunnelbroker will produce the config loading error on startup.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Aug 15 2022, 4:32 PM

• max held this revision as a draft.

Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptAug 15 2022, 4:32 PM

Harbormaster failed remote builds in B11366: Diff 15631!Aug 15 2022, 4:33 PM

• max retitled this revision from [services] Tunnelbroker - Add the ability to change the config file directory to [services] Tunnelbroker - Add the ability to change the default config file directory to run in Nix.Aug 15 2022, 4:50 PM

• max edited the summary of this revision. (Show Details)

• max edited the test plan for this revision. (Show Details)

• max added reviewers: • karol, tomek, varun.

• max edited the summary of this revision. (Show Details)

Keyserver build error is not related to these changes.

Adding @jon since it's relevant to his work on Nix

• jon requested changes to this revision.Aug 16 2022, 9:37 AM

• jon added inline comments.

services/tunnelbroker/src/Constants.h
53–54 ↗	(On Diff #15631)	We should really be using something like https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html. In this case `$HOME/.config/tunnelbroker/` should be our directory. This helps to prevent over-pollution of a users environment with tooling-specific configuration. On MacOS, the canonical path would be `$HOME/Library/Application Support/app.comm.tunnelbroker/` For a lot of the nix shell work, I've been just providing defaults: https://github.com/CommE2E/comm/blob/091b31930a107ddbbb73be29ccdf8649235e2e62/nix/start-powerline.sh#L10-L14 Taking a step back, I don't think these configuration values should be in Constants.h. Configuration isn't really constant. We should be putting this into a function which is able to do some logic about how to find the configuration, do some logging, etc.
services/tunnelbroker/src/Tools/ConfigManager.cpp
41–43 ↗	(On Diff #15631)	Not relevant to the diff itself. But I think this is an anti pattern. We should probably have something like `mode = sandbox` inside of the configuration file and unify where the configuration should be located.

This revision now requires changes to proceed.Aug 16 2022, 9:37 AM

• max planned changes to this revision.Aug 16 2022, 12:35 PM

@jon suggested good ideas and I've put them into a possible follow-up for 0.5 as an ENG-1680 task because these changes a breaking and not really relevant to this diff.
The main purpose of this diff is to fix the running of the Tunnelbroker Nix dev environment without breaking changes in a 0.4 version (which is "frozen" for such breaking updates).
But we should consider making the following updates.

services/tunnelbroker/src/Constants.h
53–54 ↗	(On Diff #15631)	We should really be using something like https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html. In this case `$HOME/.config/tunnelbroker/` should be our directory. This helps to prevent over-pollution of a users environment with tooling-specific configuration. On MacOS, the canonical path would be `$HOME/Library/Application Support/app.comm.tunnelbroker/` For a lot of the nix shell work, I've been just providing defaults: https://github.com/CommE2E/comm/blob/091b31930a107ddbbb73be29ccdf8649235e2e62/nix/start-powerline.sh#L10-L14 Taking a step back, I don't think these configuration values should be in Constants.h. Configuration isn't really constant. We should be putting this into a function which is able to do some logic about how to find the configuration, do some logging, etc. This is a good idea @jon ! I agree with you, but not sure we should use `$HOME/Library/Application Support/app.comm.tunnelbroker/` for a macOS.
services/tunnelbroker/src/Tools/ConfigManager.cpp
41–43 ↗	(On Diff #15631)	Not relevant to the diff itself. But I think this is an anti pattern. We should probably have something like `mode = sandbox` inside of the configuration file and unify where the configuration should be located. Yes, I think we should consider using this in a follow-up 0.5. It looks good.

• max requested review of this revision.Aug 22 2022, 5:55 AM

• max marked 2 inline comments as done.

The main purpose of this diff is to fix the running of the Tunnelbroker Nix dev environment without breaking changes in a 0.4 version (which is "frozen" for such breaking updates).

Yea, sounds good. I'm fine with pushing the changes to another goal post.

Having an ENV var for now to get unblocked seems okay.

tomek accepted this revision.Aug 23 2022, 7:36 AM

This revision is now accepted and ready to land.Aug 23 2022, 7:36 AM