Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Allow toml configuration to be parsed from remote/local URL #525

Closed
wants to merge 4 commits into from

Conversation

patcon
Copy link
Contributor

@patcon patcon commented Oct 29, 2018

I'm thinking on how to create a web interface to manage the config, and my current thinking is just the to generate a small all app that access the config via basic auth.

Would you be opposed to this? That would allow me to start building adjacent things that don't need to impinge much on the larger project :)

@patcon patcon changed the title Allow configuration to be parse from remote/local URL Allow toml configuration to be parse from remote/local URL Oct 15, 2018
@patcon patcon changed the title Allow toml configuration to be parse from remote/local URL Allow toml configuration to be parsed from remote/local URL Oct 15, 2018
@42wim
Copy link
Owner

42wim commented Oct 15, 2018

just the generate a small all ?
Just to generate a small app? Sure, but at the moment matterbridge automatically checks the file on disks and reloads if it's changed. So if you implement something with remote/local URL it would also needs something like this :-)

@patcon
Copy link
Contributor Author

patcon commented Oct 16, 2018

EDIT: typos in original message fixed. sorry!

ah shoot, I could see this getting bloated, and that's unfortunate.

The idea was to use this to make translation channels more straightforward or Slack users. I was hoping to create another app that would auto-build config using a Slack token, from channel naming conventions, like this:

#foo    ;skipped
#bar-en ;skipped
#baz    ;added to bridge (not translation)
#baz-en ;added to bridge (all msgs translated to english)
#baz-hk ;added to bridge (all msgs translated to chinese)
#baz-es ;added to bridge (all msgs translated to spanish)

(The conventions could even be configured via regex, so others could adapt the little app to their needs.)

I was planning to write something to restart/reload matterbridge that didn't require adding code to this, but do you think it should be added here? If so, we could discuss adding an endpoint to force a reload.

Open to suggestions and feedback, as always :)

@patcon
Copy link
Contributor Author

patcon commented Oct 29, 2018

Ok, so if I'm understanding correctly, I've gotten far enough that I now need to create a PR for this upstream :)

Related: patcon/matterbridge-autoconfig#3

One Approach

Allow matterbridge to be run like so:

./matterbridge -conf "https://username:password@example.com/matterbridge.toml"

When an http config value is set rather than a local file:

  • disable the filesystem watch that viper uses
  • expose an endpoint that can be used to reload config
    • require a secret token to be set via envvar, to be POSTed in the payload

Another Variation

An alternative would be to allow properties for bridges (including tokens) to be set by envvar with the help of Viper, in which case we could avoid the need encourage basic auth, and so:

EDIT: Just realized that this is already supported :) Yay!

./matterbridge -conf "https://example.com/matterbridge.toml"

I actually like this variant more. It would involve changes so that instead of setting config like this:

# matterbridge.toml
[slack.myteam]
Token="xoxp-xxxxxxxxxxxxxxxxxx"

We could set it like this:
export MATTERBRIDGE_SLACK_MYTEAM_TOKEN=xoxp-xxxxxxxxxxxxxxxxxx

Conclusion

I'm actually inclined to work on support for the variation. Seems more robust. I suppose basic auth would still be helpful for entities who don't want any config exposed publicly, even unpriviledged. So happy to accomodate both!

Also, being able to configure bridge properties via envvar would be helpful for matterbridge-heroku as well :)

Eager to hear your thoughts on the webhook :)

@patcon
Copy link
Contributor Author

patcon commented Oct 29, 2018

Just realized that envvar support I hoped for is already there (sorry, typo'd in my testing: #545)

@patcon patcon changed the title Allow toml configuration to be parsed from remote/local URL [WIP] Allow toml configuration to be parsed from remote/local URL Oct 29, 2018
@patcon
Copy link
Contributor Author

patcon commented Oct 29, 2018

Ok, so I'm realizing that not all of matterbridge features can work on Heroku atm: heroku has web and worker processes. In short, only web can bind to a port and receive requests. But it also sleeps, and workers are the processes that stay active like matterbridge needs.

So this might not be something you'd be willing to bring into core, but I'm currently investigating using queues. Specifically, abstract messages queues that use go channels: https://medium.com/@matryer/introducing-vice-go-channels-across-many-machines-bcac1147d7e2

Channels are really new to me, so I'm curious your thoughts on all this. Thanks wim. Sorry for all the noise the last few days ;)

@42wim
Copy link
Owner

42wim commented Oct 31, 2018

Ok, some comments.

  • I think this fits better in the API bridge instead of creating another webhook..
  • Also not sure why you need to pull in redis.

@patcon
Copy link
Contributor Author

patcon commented Nov 3, 2018

heh, i was trying to create an app that could separate web and worker processes, as is advocated by heroku (and pressured by their free tier, due to limitations). A task queue (which i backed with redis) is required to pass tasks between processes.

But I can understand that that's a hassle. I'll use the API bridge as you suggest (good idea!), and just pay heroku so that I can run this as a single process

@patcon
Copy link
Contributor Author

patcon commented Nov 7, 2018

Ok, working on scaling this back in a local branch, but quick Q:

I think this fits better in the API bridge instead of creating another webhook..

I'm reading this to mean that each API bridge gets an /api/reload endpoint, and they trigger the same code path to do a reload of the configuration settings. Am I hearing you correctly?

@42wim
Copy link
Owner

42wim commented Nov 7, 2018

Well I would keep it very simple.

  • make an extra config option: configurationURL=https://somewhere.com
  • /API/reload downloads file, overwrites matterbridge.toml and normal configuration reloads kicks in. Keep all code in API package
  • extra option in config could be automatically download/reload after x time.

@patcon
Copy link
Contributor Author

patcon commented Nov 7, 2018

Sounds good to me. This would work for the common ephemeral filesystem setup of most PasS platforms, so long as we grab config as part of the process startup script in matterbridge-heroku:

re: https://devcenter.heroku.com/articles/dynos#ephemeral-filesystem

@patcon
Copy link
Contributor Author

patcon commented Nov 7, 2018

Ok, so I'm realizing that I'm confused about our config tactic. Should I store a ConfigURL at some deeper base level viper config (outside scope of General key), or just store it on General?

It seems like it's supposed to be the latter right now, but that might result in wonky effects if someone tried to set it per-gateway (e.g. gateways fighting over what url the config is written from)

@patcon
Copy link
Contributor Author

patcon commented Nov 7, 2018

Another potential issue of storing this url in config file itself (instead of setting as CLI flag or envvar)

It can easily set people up to find themselves in odd territory, because it fetches the file that points to the config that will be used on the next reload. So if someone is using a config file in a gist or github repo, the current config url will not be in the file -- only the last raw url from previous commits. And so trying to let config be managed in a git repo will result in the file "walking back in time" on each reload 😆

Can be avoided by encouraging setting via envvar, but just a heads up. I'll make sure I document favoured approaches

patcon added a commit to g0v-network/matterbridge that referenced this pull request Nov 7, 2018
@patcon
Copy link
Contributor Author

patcon commented Nov 7, 2018

Closing in favour of #565

@patcon patcon closed this Nov 7, 2018
@patcon
Copy link
Contributor Author

patcon commented Nov 7, 2018

Still curious about the above clarifications though

patcon added a commit to g0v-network/matterbridge that referenced this pull request Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants