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

Add support for configuration via file/stdin #63

Open
Tracked by #15
jacobheun opened this issue Feb 5, 2019 · 14 comments
Open
Tracked by #15

Add support for configuration via file/stdin #63

jacobheun opened this issue Feb 5, 2019 · 14 comments

Comments

@jacobheun
Copy link
Contributor

As the daemon grows our configuration flag options are likely to become cumbersome pretty quickly. I was talking with @bigs and @vasco-santos about needing to add support for specifying the multiaddrs the daemons libp2p node listens on. We could, and probably should, add a comma delimited string flag similar to how we add bootstrap addresses. Over time though, reading through all of the flags you have enabled for a node could get overwhelming.

I'd like to propose adding support for piping a config file to the daemon. For example:

$ cat config.json | p2p daemon

This would also have the nice benefit of being able to have node type configurations pre made for users; DHT booster, relay, rendezvous, etc.

If this sounds good, I can write up a spec for the json configuration.

@bigs
Copy link
Contributor

bigs commented Feb 5, 2019

👍 i think it should be as simple as trying to read stdin, and ignoring any command line options if data exists

@raulk
Copy link
Member

raulk commented Feb 5, 2019

See #35 for a discussion on configuration flags precisely!

@vyzo
Copy link
Collaborator

vyzo commented Feb 5, 2019

or we can simply pass the config file as argument, better than reading from stdin.

@jacobheun
Copy link
Contributor Author

or we can simply pass the config file as argument, better than reading from stdin.

I'm good with this approach

@bigs
Copy link
Contributor

bigs commented Feb 6, 2019

yeah, fine by me. makes reproducibility easier too.

@bigs
Copy link
Contributor

bigs commented Feb 7, 2019

we should add, one of the motivating factors behind this was wanting to easily add lists (i.e. how we already do bootstrap nodes) and eventually add listen addresses to the configuration

@anacrolix
Copy link
Contributor

If the configuration becomes too complex to represent with flags (😢 ), consider the merits of libp2p/go-libp2p#526. If you're able to represent your configuration as a tree (explicit configuration structs with appropriate embedding), you can marshal the root of your configuration type to and from your configuration file format of choice (JSON/yaml etc.).

type Libp2pDaemonConfig struct {
    Host host.Config
    Dht dht.Config
    SomeMultiaddrsCozWhyNot []ma.Multiaddrs
}

Ultimately lists lack the structure to represent complex configuration. Maybe nip it in the bud before it gets out of hand?

@anacrolix
Copy link
Contributor

It looks like @jacobheun already essentially suggested this, but libp2p/go-libp2p#526 does add some context. It would be unfortunate to reexport a bunch of config in other packages in this way when they could be exposing it like that already.

@bigs
Copy link
Contributor

bigs commented Feb 8, 2019

interesting. the daemon ultimately has a different set of configuration options, but we are heavily leaning towards reading it from a JSON file.

@anacrolix
Copy link
Contributor

@lgierth

@ghost
Copy link

ghost commented Feb 11, 2019

or we can simply pass the config file as argument, better than reading from stdin.

But it'd also mean introducing a dependency on the filesystem. We'd make one-off invocations slightly more concise, but virtualization and automation more-than-slightly more involved.

But well I guess we can also have both ¯\_(ツ)_/¯ many unix tools accept a - filename meaning stdin.

As long as we make sure not to require stuff in the filesystem (apart from the control socket obviously, but that's not something the user needs to create themselves).


Another approach not mentioned here yet is to configure the daemon over its own API.

Disclaimer: I'm not yet very familiar with the daemon, or with all the teams that already have a need for the daemon (and it offering complex functionality). That need is there right now, so it's good to make small, swift, pragmatic steps. All I'd ask is we do steps that are in line with an overall longterm design goal and don't push ourselves into a corner that we eventually don't get out of. Things that trigger me: big config structs, god objects, long lists of cli flags, (bidirectionally) leaking abstractions between UIs and "business" code :P

There'd be a command running the daemon (let's say run), and a command starting and configuring it (let's say start). And I guess there would also be commands reloading/stopping/killing it. start would pass through run's stdout and stderr, so to the user it could look much the same as it does right now.

The handling of the user interface (CLI (and config file)) would be nicely confined to one command and decoupled from the actual work. We can focus mainly on API design instead. There's probably even a named design pattern for this.

Most importantly it'd help much on the way towards hot-reloading, which I think is super-desirable for anything networking-focused. Long way to get there, but very desirable, and as we get more networky down the roadmap, networking people will expect it.

It'll require pretty hefty design and refactoring work, but if we're going to refactor Host/Swarm in Go anyway... Don't need to have 100% hot-reloading ability right away, can do it step by step.

@raulk
Copy link
Member

raulk commented Feb 11, 2019

But it'd also mean introducing a dependency on the filesystem. We'd make one-off invocations slightly more concise, but virtualization and automation more-than-slightly more involved.

But well I guess we can also have both ¯\_(ツ)_/¯ many unix tools accept a - filename meaning stdin.

Exactly my thoughts. I'm not sure if - is the right interface here. I've seen it in vim - and others that expect a filename as an CLI arg, not as an option flag. If we want to do p2pd <config file> it should be ok, but I'm thinking we probably want to pass the config file as an option. So maybe -o <filename> for reading from a file, and -O for reading from stdin.

Anyway, it should be trivial to support both inputs, and agree it's the best path forward.

Most importantly it'd help much on the way towards hot-reloading, which I think is super-desirable for anything networking-focused. Long way to get there, but very desirable, and as we get more networky down the roadmap, networking people will expect it.

I'd shy away from watching the file and hot-reloading when it changes. It creates a significant security hole, and also it's not very polite. I'd propose reacting to SIGSTOP or another unused signal by re-loading the config file, or ignoring if we're reading from stdin.

@raulk raulk changed the title Add support for piping configuration Add support for configuration via file/stdin Feb 11, 2019
@ghost
Copy link

ghost commented Feb 11, 2019

So maybe -o <filename> for reading from a file, and -O for reading from stdin.

Yeah sounds good, or -f/-i (-o sounds like output)

I'd shy away from watching the file and hot-reloading when it changes. It creates a significant security hole, and also it's not very polite. I'd propose reacting to SIGSTOP or another unused signal by re-loading the config file, or ignoring if we're reading from stdin.

Agree on not watching the file and handling a signal instead, but SIGHUP or SIGUSR1 are most common for reloading. I've only ever seen SIGSTOP used for log rotation (in combination with SIGCONT), I think the process doesn't even get it (same as SIGKILL).

@Kubuxu
Copy link
Member

Kubuxu commented Feb 11, 2019

SIGHUP is used for config reload for daemons (detached from a console) as a signal that the console closed doesn't make sense for daemons (go-ipfs is not a daemon in the traditional scheme). So if you are planning this I would go with SIGUSR1.

Also, as it is a config, maybe just go with --config flag.

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

No branches or pull requests

6 participants