Move towards file based configuration instead of flag based

[rolandshoemaker]

Ideally I'd like to manage trillian_log_server, trillian_log_signer, and ct_server as services but given that a large portion of their configuration is provided via flags this is somewhat of a pain to do. The majority of these flags could be moved into JSON/YAML configuration files to make this easier to accomplish.

[Martin2112]

What do you mean by 'service' in this context, what's your environment like?

[rolandshoemaker]

systemd, it would be entirely possible to create a management script for the current design but that means needing to customize that script for each different deployment to change things like MySQL/backend URIs etc instead of being able to manage those at the configuration file level with something like Salt.

[Martin2112]

OK. I don't write systemd configs or use Salt so I'm not clear whether we'd need a different main that reads a fixed config path or we could support both by adding a --config flag to the existing one.

Can you give an example of what would work best for your planned setup using e.g. log_server, which only has 4 flags?

[rolandshoemaker]

OK. I don't write systemd configs or use Salt so I'm not clear whether we'd need a different main that reads a fixed config path or we could support both by adding a --config flag to the existing one.

Adding a --config flag seems like the easiest option and work for our purposes, general design would be you would set a default path of something like /etc/trillian/log_config.json and then allow an override with the --config flag but I think it would also work fine to just set an empty default and only override the existing flags if a path was provided.

Can you give an example of what would work best for your planned setup using e.g. log_server, which only has 4 flags?

JSON would probably be the simplest configuration format since you wouldn't need any extra third-party dependencies, then you could have something basic like

{
  "http_endpoint": "localhost:8091",
  "rpc_endpoint": "localhost:8090",
  "mysql_uri": "test:zaphod@tcp(127.0.0.1:3306)/test",
  "log_dir": "/var/log/trillian",
  ...
}

For our purposes we would then generate this file from a template stored on our central configuration server whenever we brought up a new host/cluster and store it at some standard path we set in the --config flag.

[Martin2112]

Thanks, that's helpful.

I'm not overly keen on a default path for cross-platform compatibility reasons and the maybe minor risk of confusion by accidentally picking up an existing file and having your flags ignored.

Otherwise, I think it's worth doing. Would be great if you had some time to work on PRs to add it!

[rolandshoemaker]

Cool, I'll take a crack at it and see if I can get something up this afternoon.

[RJPercival]

If the suggestion is adding a --config flag, but leaving the existing flags in place as well, it might be best to just support reading flags from a file. This is easy to implement:

flags, err = ioutil.ReadFile("trillian_log_server.flags")
if err != nil { return err }
flag.CommandLine.Parse(strings.Split(string(flags), "\n"))

[pphaneuf]

It'd be great if this could be done in such a way that actual flags override the configuration file, but that's just a "nice to have" request. 😃

[codingllama]

@pphaneuf I've been thinking about this is as well, but I suspect the opposite is more likely to happen.

Whatever way we go, I'd like to have a clearly documented behavior of what happens when things get mixed. Ideally, I'd suggest that we don't allow it to be mixed at all; if you provided --config no other flags should be set, with an error happening if they did. It doesn't seem like this would be easy to do with the Go APIs, though. :/

[rolandshoemaker]

I think it is possible to allow reading from a config file and flags with the flags having precedence but it ends up being a bit hacky. You'd essentially define a config struct with all of your flags, then use flag.XXVar and use the pointer of the config struct field as the value. You then call flag.Parse and if a configPath (or whatever) flag is present you parse the flags from the file. You then call flag.Parse AGAIN and it should (if I've read the docs correctly) only overwrite the fields with values from provided flags. Like I said... hacky.

I think the way you'd do ONLY file or flags is once you've called flag.Parse if your --config flag has a value you check if len(os.Args[:1]) > 2 { fail }.

Thinking through this I'm not really sure what the best path forward is, all options seem a little bad... I definitely see the use in terms of testing of having flags have precedence over a config file but idk if it is worth having the hacky implementation :/

[codingllama]

Yep, the double flag.Parse is fishy. Even if it does work, I don't think it was intended to be used like that, which is not a good place to be.

As I stated above, I would prefer not allowing --config to mix with other flags. It's too easy to get things wrong if we allow both "regular" flags and --config to coexist. I'm OK with the os.Args length check. If folks think this is a bad we can go with a more straightforward approach, though.

[pphaneuf]

Isn't it kind of hard to make sure no other flag has been set?