Go-ipfs: Separate identity values from configuration file

[lanzafame]

Version information:

go-ipfs version: 0.4.20-
Repo version: 7
System version: amd64/linux
Golang version: go1.12.4

Type:

Enhancement

Description:

The Identity section of the current configuration file should be moved to a separate file.

{
    "Identity": {
        "PeerID": <peer_id>,
        "PrivKey": <priv_key>
    },
    ...
}

The identity section prevents the simple configuration of ipfs nodes in cloud-native environments, i.e Kubernetes. Fundamentally, I want to be able to have an ipfs config.json in Consul/k8s ConfigMap and have all ipfs nodes spun up to use that, injected via an env var and using ipfs daemon --init flag to generate a key pair if one doesn't exist within IPFS_PATH. Currently, I have to run a k8s init container, which starts up before every ipfs container, to run ipfs init and then make the config changes, which are hardcoded in the script because there are no env vars for config values either. See here and here for code of what I just explained.

Related: #744, #783.

This obviously didn't happen or it did and the separation never occurred.

@whyrusleeping how should this move forward? To clearly state my agenda, I am not interested in 'where' the identity information goes per se as long as it is no longer in the configuration file.

Happy to make this change myself, I would just need some assistance in finding where to make this change, I am guessing go-ipfs-config but I am not sure about the discussion in #744 about keys being stored in the blockstore or #783 mentioning them going in the repo.

/cc @Stebalien

[Stebalien]

We absolutely should split identity from config but that part of the config has been stable since forever so this probably isn't the simplest fix (it'll require a config migration, announcement, light audit of downstream projects, etc.).

However, js-ipfs ran into a similar issue and came up with a different solution: https://github.com/ipfs/js-ipfsd-ctl/pull/303#discussion_r233553928

We already allow passing a config on init but, unfortunately, we don't actually try to _merge_ the config (which is why that won't work in both this case and js-ipfs's case. Would something like that work for you?

The one annoying part is that go-ipfs currently accepts ipfs init /path/to/config.json while js-ipfs accepts ipfs init {json config} and I'm pretty sure you want the former (well, you need ipfs daemon --init --config /path/to/config.json). We'll have to discuss that with the js-ipfs team.

cc @hugomrdias

[lanzafame]

well, you need ipfs daemon --init --config /path/to/config.json

Yes, this. Well really, in my head, and this may be incorrect due to some missing pieces, but I don't actually see the necessity of the init command once you separate the identity out of the config.

The logic of what I want the ipfs daemon command to do is as follows:

• check if there is an identity file,
  
  
  
  • if no, generate keys
  
  
  • if yes, use
  
  
• check if there is a configuration file,
  
  
  
  • if yes, start with the values of those two files
  
  
  • if no
    
    
    
    
    
    • check if there is an IPFS_CONFIG_FILE env var set
      
      
      
      
      
      
      
      • if yes, use that file
      
      
      
      
      • if no, generate a file with default values
      
      
      
      
    
    
    
  
  

Regarding, https://github.com/ipfs/js-ipfsd-ctl/pull/303#discussion_r233553928, I would have thought that having env vars for all configuration variables would be the easier option, instead of trying to json patch (merge) the configuration. To use @hugomrdias's example, what I am proposing would result in,

$ env $IPFS_CONFIG_FILE ipfs daemon

for passing in files. If we went whole hog and add env var support for the ipfs config, the

ipfs init '{"Addresses":{"Swarm":["/ip4/0.0.0.0/tcp/4003"]}}'
ipfs daemon

example would become

env ADDRESSES_SWARM=/ip4/0.0.0.0/tcp/4003 ipfs daemon

which looks ugly, in the context of invoking it manually, but in the context of a cloud environment where Consul or etcd or something similar is present, this is very clean.

[lanzafame]

We use envconfig in Cluster to handle having env vars overwrite the json configuration file as a side note.

[hugomrdias]

I'll try to explain the needs on the js side.
Tests are super super slow!!
One of the reason for this is how ipfsd-ctl handles start and stop of different types of daemons (go, js, in-process js).

The start fase goes like this:

• we need to know the current/default config, so we run ipfs config show and we get JSON
• after we need to change the config according to the user input ipfs config replace
• after we run ipfs init
• finally ipfs daemon

This is super slow because we spawn 4 child processes.

My personal end goal is to reduce this to 1 ipfs daemon --init --config '{"key":"value"}'.

The big problem here is if we merge the input with the defaults or not !?

If we don't merge i need to get the defaults and merge myself the only way is to run ipfs config show. I could keep a file inside ipfsd-ctl with the defaults but then this file would need to be kept up to date, js and go probably have conflicts here etc...

So, im fine with ipfs daemon --init --config /path/to/json but will this merge with the defaults? A deep merge with or without array concatenation is fine here really.

[lanzafame]

@hugomrdias have a look at JSON Patch? If it gives you the flexibility to make the on-the-fly changes that you need to make, it may be a viable option as a way of updating the config file.

[hugomrdias]

Yeah I know what json patch is but we don't normally use it in js.

But right now I just need ipfs daemon --init --config /path/to/json with or without mergeing because I found a sane way to keep a file with the defaults from go.

[djdv]

@hugomrdias
Just to be clear, is something along the lines of this what you're looking for?

https://github.com/djdv/go-ipfs/tree/feat/init-with-config-file

At current this just covers the daemon command, not init itself.

[hugomrdias]

Yes

ipfs init is suppose to already have --config right?

Just to be sure the config file overrides everything and doesn't merge with anything, right? So the file passed needs to have all the options needed to start a daemon?

[hugomrdias]

Wait the daemon doesn't stay up? I would like to init and start daemon (and keep it running) in one go

[djdv]

ipfs init is suppose to already have --config right?

it has --profile but no --config, I can add that though.
Edit: Correction, --config is a global parameter, which I believe uses that config for the request.

Just to be sure the config file overrides everything and doesn't merge with anything, right?

Yes, right now we just substitute the default config during init, with the one passed in to us.

Wait the daemon doesn't stay up? I would like to init and start daemon (and keep it running) in one go

This is interesting, it's supposed to remain. I need to see why it's going down right away.

Edit:
I was doing the initialization successfully, but returning early. Fixed in djdv@ce4fff8

[djdv]

ipfs daemon --init --init-config=${PATH} and ipfs init --config-file=${PATH} should be working now.
https://github.com/djdv/go-ipfs/commit/18d03498265990e9250ac494c80f0899010135a6

Names where picked arbitrarily and should probably be changed to something. Suggestions?
--config is reserved as a global command parameter so we can't use that nor -c.

[hugomrdias]

If init has --config-file, daemon should be --init-config-file

[djdv]

If init has --config-file, daemon should be --init-config-file

Current:
ipfs daemon --init --init-config-file=${PATH}
ipfs init --config-file=${PATH}
https://github.com/djdv/go-ipfs/commit/f1f726cd70ea7cf1c62d0c20a2a8613fb889b89e

[lanzafame]

@djdv Do these changes actually separate the identity from the .ipfs/config file? For the initial use case of this issue, cloud deploys of go-ipfs, the —init-config-file or —config-file flags do not solve that problem, unless you implemented merging?

[Stebalien]

Instead of a complete merge, we can do a single-level merge to get most of the way there. @djdv, could you submit this in a PR so we can discuss it there.

[djdv]

@lanzafame

Do these changes actually separate the identity from the .ipfs/config file?

Negative. I like the behaviour you laid out here https://github.com/ipfs/go-ipfs/issues/6262#issuecomment-486952412
But was requested for the config flag ;^)

I'd like to help push towards this separation though, and maybe move away from our explicit identity+config initialization for a more implicit one. (if exists, use, otherwise generate).

@Stebalien
Up: https://github.com/ipfs/go-ipfs/pull/6489

[olizilla]

@lanzafame's original issue still remains

Fundamentally, I want to be able to have an ipfs config.json in Consul/k8s ConfigMap and have all ipfs nodes spun up to use that, injected via an env var and using ipfs daemon --init flag to generate a key pair if one doesn't exist within IPFS_PATH. Currently, I have to run a k8s init container, which starts up before every ipfs container, to run ipfs init and then make the config changes

The common goal here is to be able to init and start the daemon with a single command, and be able to pass in some overrides, and let ipfs fill out the rest of the config, especially the Identity, which must be unique per node, so is a chore for the user to manage explicitly if the just want to spin up n nodes.

Pulling the Identity block out seems like a special of case of "Please generate some defaults for me and let me override some others

Would something like

$ ipfs daemon --init --init-config-overrides '{ "Datastore": { "StorageMax: 180GB" }, { " Swarm": "ConnMgr": { "HighWater": 2000 }}}'

get us to a less horrible DX for kubernetes et al if it created a default config with a generated Identity, and then merged in the overrides?

Or are there other reasons to extract the identity that are strong enough to warrent doing the work that @Stebalien highlighted

We absolutely should split identity from config but that part of the config has been stable since forever so this probably isn't the simplest fix (it'll require a config migration, announcement, light audit of downstream projects, etc.)

We aleady got towards an agreement on this...

Instead of a complete merge, we can do a single-level merge to get most of the way there. @djdv, could you submit this in a PR so we can discuss it there.

@lanzafame would a single level merge get us close enough to be useful? There is still the JSON merge vs env vars discussion to bottom out.