Allow simple remote configuration files

[patcon]

Another run at #525

Curious about feedback to approach, but still needs work:

• Debug output says config is reloaded, but channels are not updated. Any advice?

Notes:

• Using PUT instead of POST, since this is replacing an existing resource in full (the config file)
• no public viper methods to get config file location, so had to store it
• not sure best place to store ConfigURL, as it's global and never bridge-specific.
• not sure best place to doc in sample config file. Seems like General, but then people will be reading API when they need to know about it, so put it in the latter place :)

If it's alright with you Wim, I'll do the rest of the docs inline once Swagger
PR is merged?

Related: #244

[patcon]

Hm. Something about the config.EVENT_REJOIN_CHANNELS, but still not clear if every bridge knows how to use that... Sending it from API resulted in a message from "system" in Slack that said "rejoin".

Will investigate more later

[patcon]

Also, with the "watching" approach, I don't think it's quick enough in the reload to know when to reload the channels, but we can prob just add in a large wait before sending the reconnect event through the gateways. At least that's my theorizing until I have time to try :)

[patcon]

Ah, ok, I think I found the real way: Using OnConfigChange to trigger something that rejoins channels or checks for new/removed channels

[patcon]

Hm. Or maybe the right approach is to give the router a way to shut down, and just create a new Router object from scratch? I'll admit that I can't figure out how to get everything to reload -- like the viper config is reloading, but it seems there's lots of state in the router/gateways that I don't know how to refresh

Any advice is appreciated!

[patcon]

Ok, I'm certain this is wrong, but I'm just pushing that last commit to start a conversation -- I really don't feel like I know a whole lot about how to do this elegantly...!

[42wim]

For now don't add any extra features to this PR, only reloading via remote URLs.
If you want to add extra stuff to reload beside the RELOADABLE settings open another PR.
All the extra stuff needs to work for all bridges and not only slack.

Debug output says config is reloaded, but channels are not updated. Any advice?

Yes that's correct, that's how it's implement, only the reloadable settings are reloaded.

[patcon]

only the reloadable settings are reloaded

So to confirm my understanding, updating of channels bridged to gateways (adding and removing them) is out of scope for this PR?

[42wim]

So to confirm my understanding, updating of channels bridged to gateways (adding and removing them) is out of scope for this PR?

Yes, that's a bigger change with a lot of impact, I'd like to keep the PR's small

[patcon]

ok right on. this one's ready for review. Tested it with some reloadable settings and works fine

[42wim]

I'm wondering, I think it's better to remove the ConfigURL and specify the reload url via the /api/reload and a JSON post like below.

curl --header "Content-Type: application/json" --request POST --data '{"url":"http://youserver/matterbridge.toml"}' http://host/api/reload

[patcon]

I'm down for that, but if we do that, I think we should enforce a Token for the API bridge and fail out if it's not set. Would that be fair?

[patcon]

On second thought... I know we're not building anything super-secure or privacy-minded here, but using POST data to set config url feels like unnecessary exposure... Can we allow environment variables too? As in, if someone knows that their config will always be at one spot, there's no need to add that extra level of exposure, if that makes sense :)

[42wim]

Well, talking about secure or privacy, the URL where you're exposing your configuration on is a greater risk if it's not on https or protected because that contains all your tokens/logins/passwords.

What do you mean by environment variables ?

Another option is that instead of the URL, you just post the whole actual config to the endpoint.

We should also add TLS support to the API (a new PR, not in this one! :), the original use-case was only for localhost and not meant to be available for the whole internet.

[patcon]

URL where you're exposing your configuration on is a greater risk if it's not on https or protected because that contains all your tokens/logins/passwords

Sorry, I don't understand: The tokens and any secrets are stored in environment variables on the matterbridge server (eg. MATTERBRIDGE_SLACK_MYTEAM_TOKEN), no one should be allowing their credentials to be fetched this way -- just the data on how the rooms are wired up :)

What do you mean by environment variables?

As in, setting MATTERBRIDGE_CONFIGURL will set the config url firmly from the server, so POSTing can't load any config file, but just reload the one pre-defined on the server.

And good call on TLS!

[42wim]

Sorry, I don't understand: The tokens and any secrets are stored in environment variables on the matterbridge server (eg. MATTERBRIDGE_SLACK_MYTEAM_TOKEN), no one should be allowing their credentials to be fetched this way -- just the data on how the rooms are wired up :)

Ok that wasn't clear to me, could you please write some examples about this on the wiki too, because this feature will be used wrong by users :)

[patcon]

Can do :) And I should test with BasicAuth before we merge.

Also, can we leave the "config file in payload" for anther PR? It's a feature that might be worth putting in, but it doesn't feel necessary to have it this PR, since this is already functional for a set of use-cases

[codeclimate]

Method Api.handleConfigReload has 6 return statements (exceeds 4 allowed).

[codeclimate]

Code Climate has analyzed commit 4e5a1f7 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

View more on Code Climate.

[42wim]

I don't think there is going to be any further work on this PR?
If so, please reopen, but closing for now.