Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Env variables for configuration #575

Closed
koalalorenzo opened this issue Oct 11, 2018 · 14 comments · Fixed by #596
Closed

Env variables for configuration #575

koalalorenzo opened this issue Oct 11, 2018 · 14 comments · Fixed by #596
Assignees
Labels
kind/enhancement A net-new feature or improvement to an existing feature

Comments

@koalalorenzo
Copy link

koalalorenzo commented Oct 11, 2018

This is a feature request.

The hardest part of maintaining ipfs-cluster is changing the configuration "on the fly" by changing the env variables. This is quiet common in docker containers which I am extensively using.

Most of the configuration now lives into its own file, and can't be changed after running the first init, and even by adding -f. For example if you need to change the CLUSTER_SECRET variable, and keep persistency, you need to force a new initialisation or delete the service.json file

What would be nice to have is the configuration to get updated based on the environment variables. The CLUSTER_SECRET is one quick-win, easy example, but it can be applied to several different cases (ex: bootstrapping vs pre-defined peers).

This can be implemented in different ways:

  • only for docker: forcing a -f init each restart
  • by implementing some more common practices. For example Kelsey Hightower's library can be quite useful to achieve this.
@hsanjuan
Copy link
Collaborator

@koalalorenzo so you want to override options from the configuration using env variables? would you also have cluster update the configuration with the new values then or just use them?

Would perhaps an ipfs-cluster-service config command help here? i.e. ipfs-cluster-service config secret=xxx would modify the config.

Are you mostly bothered by secret, or are there others you would rather use this for?

@hsanjuan hsanjuan added kind/enhancement A net-new feature or improvement to an existing feature needs feedback from submitter labels Oct 22, 2018
@koalalorenzo
Copy link
Author

koalalorenzo commented Oct 22, 2018

No, the ipfs-cluster-service config doesn't help at all. I am using a custom script to start go-ipfs and I find it very much against every cloud native standard, so I hope that ipfs-cluster-service will use the same techniques.

The idea is that (when possible) a value for the configuration is passed via env variable. This is including secret but it is not the only thing that could help.

Basically, if there is a env variable and the configuration file is present, the env variable should have priority. Changing the file doesn't matter as long as the software behaves as the env variables are specifying. Is this helping?

@lanzafame
Copy link
Contributor

@hsanjuan I think we could move to use something like Viper, which allows for reading from multiple different configuration sources.

@hsanjuan
Copy link
Collaborator

@koalalorenzo wait, is go-ipfs doing it right or not? I don't think it supports env variables.

@koalalorenzo
Copy link
Author

go-ipfs is NOT doing it right. I had to make a super hacky workaround to enable basic setup via env variables.

@lanzafame lanzafame self-assigned this Oct 26, 2018
@lanzafame
Copy link
Contributor

@koalalorenzo unfortunately, viper is too big of a jump from how we do config mngt in cluster atm. I am going to just add in reading from the environment with envconfig for now.

How do you currently manage the env vars of a process currently, i.e. Consul?

@hsanjuan
Copy link
Collaborator

@lanzafame no, don't do envconfig. It overlaps with urfave/cli which can also parse env variables to set flag values and doesn't bring anything special to the mix.

I think we should just have --secret and other select flags for the daemon command which take defaults from env variables.

@lanzafame
Copy link
Contributor

@hsanjuan we don't define flags for a lot of the service.json file though?

@hsanjuan
Copy link
Collaborator

hsanjuan commented Oct 26, 2018

No.. honestly it's still unclear to me why the cluster secret would need updating after init (which does take a CLUSTER_SECRET). The case of setting node_multiaddress is a bit better though.

Also, doing this per component makes service no longer be the place in charge of setting up the peer, and that starts depending on the components submodule being aware of the shell environment in which it's initialized, which I think confuses things a bit and is more difficult to document (vs having --help tell you automatically from the existing written code).

Overall I'm asking to take minimal a approach because I'm not sure what the best approach is in terms of functionality, documentation, code organization and maintainability (supporting all config via env variables is essentially supporting a parallel service.env file format).

@lanzafame
Copy link
Contributor

The way I implemented it in #596 is that at the time of load the service.json file, we check if there are any overriding env vars and use those instead of what is in the service.json. And it is just for the life of that instance, the override value doesn't get saved to the json file.

@hsanjuan
Copy link
Collaborator

If we do this, it should:

  • Define all variables prepended by CLUSTER_ in all components (except the main one which should not be CLUSTER_CLUSTER...)
  • Make sure things are like CLUSTER_CONSENSUS_RAFT_COMMIT_RETRIES and not CLUSTER_CONSENSUS_RAFT_COMMITRETRIES which means envconfig tags everywhere.

But doing this per component we cannot have a the full config overview that ipfs-cluster-service has and which we use to overwrite config values in some cases. i.e. leave_on_shutdown might be overwritten by an env var which might be overwritten by a flag, which may potentially take a default value from a better-named env variable. Also node_multiaddress will need special handling when we split the ipfs connector and the proxy.

@lanzafame
Copy link
Contributor

So to add some context, I want to be able to set the metrics and tracing configuration values prior to the invocation of the ipfs-cluster-service daemon command when setting up ipfs-cluster in kubernetes as the values of the endpoints exist within the context of the k8s cluster and can be accessed via env vars but adding to the flags used is difficult as passing args to docker containers is painful and fraught with trouble, especially if you want to pass double dash flags as docker parses them stupidly.

I have been thinking about the differences between file, env, and flag configuration and my current thoughts are that file and env configuration options should be the same and encompass the entire set of configuration options, whereas flag should be the subset of configuration options that change or add code paths to operation of the command, i.e. ipfs-cluster-service daemon --upgrade will run the upgrade code path as well as the standard daemon code path or --debug which adds lots of little extra code paths to the execution of the daemon command. Obviously, there are case where it will useful to pass values as flags but I think for the most part that is only useful if you are manually running the ipfs-cluster-{service,ctl} cmds and not so when you are using some form of automated/declarative deployment mechanism.

@hsanjuan
Copy link
Collaborator

@lanzafame yes ok, let's go ahead

@koalalorenzo
Copy link
Author

koalalorenzo commented Oct 29, 2018

No.. honestly it's still unclear to me why the cluster secret would need updating after init (which does take a CLUSTER_SECRET). The case of setting node_multiaddress is a bit better though.

Change the configuration from the env without any static config file, volumes or mounts. In case of security breach, changing just the env variables would be much easier than going inside each single volumes and changing the value of the files.

Is this a valid use case? There are more to point to

I would like to have at least CLUSTER_SECRET (plz 🙏 ). All the other configurations are also needed... but CLUSTER_SECRET is the most painful one as it broke constantly my clusters, and I have to manually put hands on the volumes. 😞

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants