Config system #26

rubenseyer · 2018-02-15T15:02:42Z

Alright, so here's my suggestion for what the config system could look like. Sorry for being a bit spammy with the issue mentions--I didn't expect them to show up while I was still tidying history. I'm holding off on the Let's Encrypt implementation because I want to see how the tls-sni debacle ends before adding another HTTP server just to respond to the challenge.

I extended the pre-existing system in serverconf based on string maps. It is format independent - if it can encode string key-value pairs (and has a file format extension) it can be a config format.
The default config file generated is an .ini file. ~~If the config file is named murmur.ini an additional translation step is performed~~, including warnings for some unsupported features (but I won't promise I catch all of them). (The .ini support uses go-ini.)
I still dislike the JSON-with-comments idea, ~~but I quickly implemented it anyway to showcase the format independence. However, it would probably require a few good test cases for me to be confident it's usable.~~
The config files are only read, never written. The freeze system still acts as the persistent storage, which means that to revert a config value to defaults, you can't just comment it out, you have to specify it unset. I think this is an acceptable compromise.
The config system introduces one new feature: if the config format supports some form of subsets (sections in .ini, nested objects in .json) it is possible to set specific overrides for single virtual servers (including reverting to defaults). Not the best form of virtual server management, but at least it's something.

To make this meaningful, I also implemented some missing config keys that users migrating from Murmur probably would want. Features like databases, RPC and bandwidth limiting are still missing (but the latter should probably wait for web client support). I also added the SuperUser CLI flags from Murmur. I think the remaining unimplemented Murmur CLI flags would be meaningless for Grumble right now, at least.

mkrautz

Haven't had time to do a full review. I'll post what I have now.

Also, I don't think we need the JSON reader if we have one for INI. Though it may be worth keeping around. I don't know. I am too sleepy. :)

mkrautz · 2018-02-15T20:05:16Z

cmd/grumble/freeze.go

@@ -835,7 +834,7 @@ func (server *Server) UpdateFrozenBans(bans []ban.Ban) {
 }

 // Write an updated config value to the datastore.
-func (server *Server) UpdateConfig(key, value string) {
+/*func (server *Server) UpdateConfig(key, value string) {


Feel free to remove. We can restore from history if necessary.

mkrautz · 2018-02-15T20:05:20Z

cmd/grumble/freeze.go


 // Write to the freezelog that the config with key
 // has been reset to its default value.
-func (server *Server) ResetConfig(key string) {
+/*func (server *Server) ResetConfig(key string) {


Feel free to remove. We can restore from history if necessary.

rubenseyer · 2018-02-16T11:26:19Z

I don't mind whether you keep the JSON support or not. If there is no use-case for it we could just drop it.

ghost · 2018-02-24T12:46:39Z

cmd/grumble/args.go

@@ -30,6 +30,20 @@ var usageTmpl = `usage: grumble [options]
 --log <log-path> (default: $DATADIR/grumble.log)
     Log file path.

+ --config, --ini <config-path> (default: $DATADIR/grumble.ini)


murmur doesn't have --config, only --ini, so let's drop it from here

--ini by itself wouldn't make sense if we had multiple formats (although it should still be there for murmur compatibility), but we won't right now, so I'll get rid of it

ghost · 2018-02-24T13:00:09Z

pkg/serverconf/cipherlist.go

+		if ok {
+			ciphers = append(ciphers, c)
+		} else {
+			log.Printf("Ignoring invalid or unsupported cipher \"%v\"", v)


I dont like the log in this function. If you need to report in this, make the function a multi-return that contains the unsupported values.

ghost · 2018-02-24T13:01:10Z

pkg/serverconf/file.go

+	switch filepath.Ext(path) {
+	case ".ini":
+		f, err = newinicfg(path)
+	case ".json":


drop json support

ghost · 2018-02-24T13:02:00Z

pkg/serverconf/file.go

+func NewConfigFile(path string) (*ConfigFile, error) {
+	var f cfg
+	var err error
+	switch filepath.Ext(path) {


don't require the config file have a particular extension, just always parse as ini

As per #21 I'll still keep the multiple-format boilerplate around (separate NewConfigFile from newinicfg). Please tell me if you think I should get rid of it as well.

ghost · 2018-02-24T13:05:06Z

pkg/serverconf/file_ini.go

+	if !f.murmurCompat {
+		return f.file.Section("").KeysHash()
+	} else {
+		return TranslateMurmur(f.file.Section("").KeysHash())


i think it would be preferable if the ini file had as many common fields with the murmur config as possible. Can we rename all config values appropriately (e.g. Address -> host)

There are a few reasons I didn't do this.

Backwards compatibility with previous grumble freezes. The current way supports both the previous/current grumble key names and the previous murmur key names. (I don't know if this matters, and I actually accidentally broke it by renaming MaxChannelUsers to MaxUsersPerChannel. Oops! Fixing that when I have time to write a patch.)

I want a TranslateMurmur step and a list of supported murmur keys anyway to print messages regarding common unsupported configuration. (It also strips unsupported keys as to not pollute the freeze files, but that will be unnecessary later, see below.)

Subjective: My interpretation of Introduce compatibility with Murmur .ini files #21 is that the murmur support should be a non-default personality. (I chose to trigger it if the config file is named murmur.ini, but other solutions are possible, too.) This means that the normal configuration isn't dependent on murmur, so there's no guarantee everything works the same way in normal mode. (However, the murmur mode instead promises compatibility -- two sides of the same coin.) This means there is no need to rename things like Address either (it's had that name for 7 years). Rather, murmur configuration should be converted.

Because of the freeze persistence, grumble still wouldn't behave like murmur when you try to reconfigure murmur.ini to defaults. I didn't have time to fix that just yet, but I want the config to not persist in the freeze (overriding defaults, but not config set through future RPC).

This obviously does not preclude renaming the new keys closer to their murmur counterparts (like TLS* to SSL*, *Path to *File etc.), and fixing the persisting configs are on my to-do list. Even if you're not convinced about the field names, I'd still like to hear what you think about the translation/filter step.

ghost · 2018-02-24T13:05:22Z

pkg/serverconf/file_json.go

@@ -0,0 +1,154 @@
+package serverconf


delete file

ghost · 2018-02-24T13:05:59Z

pkg/serverconf/murmurcompat.go

+	"log"
+)
+
+var murmurCompatRules = map[string]string{


like mentioned above, drop this (file); use common names with murmur

mkrautz · 2018-03-26T21:45:36Z

Sorry for leaving this be.

I'll quickly sketch up my thoughts on the personalities in Grumble:

I think the code we're working with now should just be "grumble". If we do implement personalities, they'd probably be a separate command (separate package main). That seems cleaner to me. The way I imagine it would pan out is that we refactor the Server part into a package, with the hooks necessary to implement custom handling.

Anyway, in my mind, since Murmur doesn't have config files at the moment, we should probably just use the Murmur .ini format all the way through.

I guess that does make some sense: Grumble, as it is right now has no way to set config options. (I removed my JSON-RPC thing and/or SSH thing I had for a while).
With that in mind, I don't think there's much to lose from changing the config names Grumble uses already. Users of the current builds have no way to set them, right? Also, I don't think anyone really uses it in production, though I could be wrong, of course. So, there's no shame in dropping backwards compatibility with the freeze files.

NickSutton · 2018-04-14T15:00:31Z

pkg/serverconf/file_ini.go

+	"path/filepath"
+	"strconv"
+
+	"gopkg.in/ini.v1"


Getting a build error on this line, no package found

rubenseyer · 2018-04-15T10:26:12Z

Sorry for being inactive. I've been busy with other stuff and the ACME spec doesn't seem to be making a lot of progress anyway. I had some patches on my computer waiting for consensus that I am uploading now.

Seems like I am outvoted on the CamelCase point so I've dropped the compatibility layer design entirely and made grumble.ini configuration a superset of murmur.ini. (I still left some convenience features enabled in the .ini parser regarding quote escaping and boolean keys etc.).

I did keep the warnings for unsupported common Murmur configuration around but they are separate and easily removed now if you don't want them.

The branch history is a bit long now but I want to avoid rewriting the commits with issue mentions until the squash and merge.

quite · 2019-07-29T16:45:12Z

Is @mkrautz waiting for changes by @rubenseyer here, still? I only mean well. Looks like this PR could solve a number issues related to not having a proper configuration file system in place!

poVoq · 2020-04-14T21:39:07Z

Sorry to ask, but is there any news on this? I am about to setup a mumble server again, and would really like to use Grumble.

actown · 2020-04-15T02:19:34Z

Given this PR is a few years out of date, if you would like to rebase it on master and give it a test that would be awesome. If I have time this weekend I can take a look myself.

poVoq · 2020-04-15T02:31:36Z

If I have time this weekend I can take a look myself.

Would be awesome if you could have a look at it.
Anything basic will do, if some channels could be preconfigured like in uMurmur that's totally sufficient.

rubenseyer · 2020-04-15T08:28:28Z

Wow, I had forgotten about this! I'm still around, but probably too busy to do anything too soon. I'll try to rebase and update everything if I get time and no one else beats me.

Looking through what has happened since I'm gone, here's a few things off the top of my head:

Check that Add a configuration parameter making it possible to avoid the web port #51 works (also more on Grumble-specific configuration later)
Make it possible to change configuration parameters on a server #52 presents a design decision. No user in normal (that is, running Grumble itself) operation can currently set configuration in that way (with RPC unimplemented). However, unless you are careful, the settings persist in the freezelog and this may cause unexpected behaviour (or at least it worked that way back in 2018). The .ini files take care to not persist, but they will also override settings set in the freezelog on load for this reason (in case old configuration was left in), because I think that's more obvious for the user. We should think about what we want to happen.
Add support for server passwords #55 conflicts with this one (that's right, server password support was hidden here all along!), so we'll have to merge those changes -- notably, you can't be Murmur.ini-compatible and stored hashed passwords like for SU, so we need to special-case and hash incoming passwords from the config file (which is pointless because you'll still have them unhashed on disk anyway) or special-case and allow unhashed passwords to accept Murmur behaviour (but you might not like that for security reasons)
As you can probably read from the history of this pull request, these features have gone through a lot of redesign in terms of scope. I went in with some suggestions for enhancements for the configuration (json support, new configuration keys unsupported by Murmur, virtual server config without rpc) but in the end it was decided to scale back so that it essentially is literally murmur.ini. That meant no Grumble-specific configuration and renaming every key to Murmur names. This breaks every single existing server, so that configuration must be generated again, but when consensus made that decision in 2018 the reasoning was that nobody uses Grumble in production anyway. If someone actually is using this for real nowadays we ought to avoid renaming the keys, and probably just bring back the murmur.ini translation layer (but that would just be a matter of dropping some later commits, because I kept everything!). (Also, if those other features sound interesting this time and we consider that Grumble may diverge from Murmur in configuration feature set I might consider fixing the other suggestions too.)
Lastly, I added myself to AUTHORS because every contributor back then had done so, but this file has since gone entirely unused. I'd suggest dropping that commit from this pull request and just update it separately with every contributor, or getting rid of it.

Sorry for dropping a book on you, but I thought I'd write down some of these considerations so we can think about the decisions involved. Merging this is not far away in terms of code changes but it is an entirely new part to Grumble so it was always gonna be a matter of design problems first and foremost.

actown · 2020-04-16T02:33:44Z

It sounds like this PR brings up a bunch of questions that we should figure out the best way to handle.

I’m unsure if anyone is using Grumble in production and I’m unsure of the best way to determine that. Given the state of this repo I suspect that most people are still using the main server provided by their local package repo or via the static builds on the project website.
What’s the goal of this project? I have my views, please don’t take them as gospel. Personally I’d like to see this project be the default server backend for Mumble (a long way to go for this), splitting out the server from the client+QT base. Ideally doing so would allow the server and client to move in a direction that works best for each component. This also brings up some points that will need to be considered: Do we keep compatibility with older configs? ICE (please no)? gRPC (yes please)? Most of these can be worked around but will require some work. Would love to hear some opinions of this.
AUTHORS is fine, the main project uses this so we might as well follow suit. This should be its own task to get the file caught up.

rubenseyer · 2020-04-16T11:06:23Z

I think my views on this have remained unchanged since I first began this PR --- in essence, I agree.

I think that for Grumble to become a default choice we will need compatibility to allow easy migration (but compatibility need not mean that we enforce Grumble to have exactly the same feature set as Murmur). My solution for that was the translation layer which was a compromise between not breaking existing Grumble servers and allowing compatibility with existing configuration, because I have no problem with the real Grumble config not being exactly the same as murmur.ini (and apparently neither did the original author of serverconf) but I understand it seems like a lot of trouble just because we don't want to rename config keys.

(Also, we clearly would need gRPC for any major production user to consider switching. That was my idea for the next step after this PR back in 2018.)

(see also issue mumble-voip#21) Removes synchronizing set config keys to freezelog, since the system is the preferred way to persist configuration.

rubenseyer · 2020-04-16T20:31:55Z

I rebased everything and re-organized the slew of WIP commits into something more logical. I have changed no design decisions here compared to where I left off in 2018, so I do not expect this to be merged until everyone is satisfied on those. Please do test and review the changes, because I have only built it and nothing more.

Some notes:

Reading the code again made me realize I was wrong about what happens with Make it possible to change configuration parameters on a server #52... Configuration set using this method will indeed persist, but grumble.ini will not override them.
Add support for server passwords #55 should work if the password given in the config is in hashed format. Again, we should think about how we want this to work, because this breaks murmur.ini compatibility.

Also adds some Murmur CLI flags, see mumble-voip#25 Since the only way right now to set the SuperUser pw is through the CLI on start, this commit also drops the config updates through the freezelog (probably a remnant of RPC), which the SetSuperUserPassword function was the last user of.

MaxUsers: modifies existing sessionpool similar to Murmur MaxUsersPerChannel: already implemented, inconsistent name AllowPing: affects registration, too DefaultChannel RememberChannel ServerPassword SendOSInfo: already implemented, inconsistent name Config keys are renamed to conform to murmur.ini

streaps · 2020-05-28T09:39:06Z

I get this error on first start. The message is also missing a "\n".
Unable to open log file (/home/mumble/.grumble/grumble.log): open : no such file or directory

streaps · 2020-06-10T16:58:49Z

I'm not able to add @all permissions to the root channel, it only adds another SuperUser everytime I try. Is this a new problem with this PR or a known issue? I also cannot start the server anymore. Could this be related?

$ ./grumble
[G] 2020/06/10 16:55:12.632307 Grumble
[G] 2020/06/10 16:55:12.633083 Using data directory: /home/mumble/.grumble
[G] 2020/06/10 16:55:12.633692 Loading server 1
panic: assignment to entry in nil map

goroutine 1 [running]:
main.(*Channel).Unfreeze(0xc0000f4360, 0xc0000f4120)
        /home/mumble/repos/grumble/cmd/grumble/freeze.go:272 +0x4d2
main.NewServerFromFrozen(0xd015f1, 0x1, 0xc0000bec80, 0x0, 0x0)
        /home/mumble/repos/grumble/cmd/grumble/freeze.go:451 +0x808
main.main()
        /home/mumble/repos/grumble/cmd/grumble/grumble.go:245 +0x1494

rubenseyer · 2020-06-10T20:00:11Z

I can reproduce the same crash on both master and this PR:

Log in as SuperUser (on master you need to "cheat" to do this)
Make any change to the @all acl
Stop the server
Fail to start the server

I have filed PR #71 to fix it, because it is not really pertinent to this one (which is bogged down in years of reviews anyway). You should be able to easily rebase those changes onto this one if you want to combine them immediately -- this shouldn't touch those areas too much. I'll rebase here later if the other one is merged.

ghost requested review from a user and mkrautz February 15, 2018 15:32

mkrautz suggested changes Feb 16, 2018

View reviewed changes

ghost suggested changes Feb 24, 2018

View reviewed changes

ghost mentioned this pull request Apr 14, 2018

Building and running on Raspberry Pi #27

Open

NickSutton reviewed Apr 14, 2018

View reviewed changes

pkg/serverconf/file_ini.go

"path/filepath"

"strconv"

"gopkg.in/ini.v1"

Copy link

NickSutton Apr 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting a build error on this line, no package found

porjo mentioned this pull request Aug 27, 2018

What is the SuperUser password? #20

Open

rubenseyer mentioned this pull request Aug 27, 2018

fix log path #28

Closed

rubenseyer mentioned this pull request Nov 26, 2018

WebRTC #33

Open

rubenseyer mentioned this pull request Apr 18, 2019

Works directly with Grumble websockets? Johni0702/mumble-web#46

Open

Config system

40ef449

(see also issue mumble-voip#21) Removes synchronizing set config keys to freezelog, since the system is the preferred way to persist configuration.

rubenseyer force-pushed the config branch 2 times, most recently from 1ae1f08 to d7a0c46 Compare April 16, 2020 20:27

rubenseyer added 2 commits April 16, 2020 22:42

rubenseyer force-pushed the config branch from d7a0c46 to ae41a61 Compare April 16, 2020 20:43

toby63 mentioned this pull request May 3, 2020

WebRTC as a transport for voice mumble-voip/mumble#3561

Open

grumble.go: bad merge in log handling

76ff3fb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config system #26

Config system #26

rubenseyer commented Feb 15, 2018 •

edited

Loading

mkrautz left a comment

mkrautz Feb 15, 2018

mkrautz Feb 15, 2018

rubenseyer commented Feb 16, 2018

ghost Feb 24, 2018

rubenseyer Mar 2, 2018

ghost Feb 24, 2018

ghost Feb 24, 2018

ghost Feb 24, 2018

rubenseyer Mar 2, 2018

ghost Feb 24, 2018

rubenseyer Mar 2, 2018

ghost Feb 24, 2018

ghost Feb 24, 2018

rubenseyer Mar 2, 2018

mkrautz commented Mar 26, 2018

NickSutton Apr 14, 2018

rubenseyer commented Apr 15, 2018

quite commented Jul 29, 2019

poVoq commented Apr 14, 2020

actown commented Apr 15, 2020

poVoq commented Apr 15, 2020 •

edited

Loading

rubenseyer commented Apr 15, 2020 •

edited

Loading

actown commented Apr 16, 2020

rubenseyer commented Apr 16, 2020

rubenseyer commented Apr 16, 2020

streaps commented May 28, 2020

streaps commented Jun 10, 2020 •

edited

Loading

rubenseyer commented Jun 10, 2020

Config system #26

Are you sure you want to change the base?

Config system #26

Conversation

rubenseyer commented Feb 15, 2018 • edited Loading

mkrautz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenseyer commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkrautz commented Mar 26, 2018

Choose a reason for hiding this comment

rubenseyer commented Apr 15, 2018

quite commented Jul 29, 2019

poVoq commented Apr 14, 2020

actown commented Apr 15, 2020

poVoq commented Apr 15, 2020 • edited Loading

rubenseyer commented Apr 15, 2020 • edited Loading

actown commented Apr 16, 2020

rubenseyer commented Apr 16, 2020

rubenseyer commented Apr 16, 2020

streaps commented May 28, 2020

streaps commented Jun 10, 2020 • edited Loading

rubenseyer commented Jun 10, 2020

rubenseyer commented Feb 15, 2018 •

edited

Loading

poVoq commented Apr 15, 2020 •

edited

Loading

rubenseyer commented Apr 15, 2020 •

edited

Loading

streaps commented Jun 10, 2020 •

edited

Loading