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
Separate identity values from configuration file #6262
Comments
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: ipfs/js-ipfsd-ctl#303 (comment) 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 cc @hugomrdias |
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 The logic of what I want the
Regarding, ipfs/js-ipfsd-ctl#303 (comment), 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,
for passing in files. If we went whole hog and add env var support for the ipfs config, the
example would become
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. |
We use |
I'll try to explain the needs on the js side. The start fase goes like this:
This is super slow because we spawn 4 child processes. My personal end goal is to reduce this to 1 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 So, im fine with |
@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. |
Yeah I know what json patch is but we don't normally use it in js. But right now I just need |
@hugomrdias https://github.com/djdv/go-ipfs/tree/feat/init-with-config-file At current this just covers the |
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? |
Wait the daemon doesn't stay up? I would like to init and start daemon (and keep it running) in one go |
Yes, right now we just substitute the default config during init, with the one passed in to us.
This is interesting, it's supposed to remain. I need to see why it's going down right away. Edit: |
Names where picked arbitrarily and should probably be changed to something. Suggestions? |
If init has --config-file, daemon should be --init-config-file |
Current: |
@djdv Do these changes actually separate the identity from the |
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. |
Negative. I like the behaviour you laid out here #6262 (comment) 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 |
@lanzafame's original issue still remains
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 aleady got towards an agreement on this...
@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. |
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.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 withinIPFS_PATH
. Currently, I have to run a k8s init container, which starts up before every ipfs container, to runipfs 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
The text was updated successfully, but these errors were encountered: