Apply environment variables during init

[hsanjuan]

Basic information

• Type (mark as appropiate):
  • Bug
  • Feature request
  • Enhancement

Description

Users find it weird that CLUSTER_SECRET can be used during init to set the CLUSTER_SECRET in the config, but that CLUSTER_LEAVEONSHUTDOWN does not do anythiing during init but takes effect when running the cluster peer.

We should apply the environment variables during init. Doing it in Default() makes Default() no longer use defaults, so maybe we can have ApplyEnvVars() or something called afterwards.

Source: https://discuss.ipfs.io/t/modifying-services-json-in-local-docker-cluster/4775/6

[roignpar]

I would like to help implement these changes.

I have a question though: ipfscluster.Config and ipfscluster.configJSON don't have the exact same fields. Peers and Bootstrap are part of configJSON, but not Config; Config.ListenAddr is named ListenMultiaddress in jsonConfig. Which environment variable names should be used, the ones from configJSON or Config?

As far as I can tell, configJSON is used when loading the cluster peer and env vars are applied to it:

// ipfs/ipfs-cluster/cluster_config.go

func (cfg *Config) LoadJSON(raw []byte) error {
        jcfg := &configJSON{}
	// ...

	// override json config with env var
	err = envconfig.Process(cfg.ConfigKey(), jcfg)
	if err != nil {
		return err
	}
        // ...
}

An easy solution would be to rename Config.ListenAddr to ListenMultiaddress and override env vars directly on Config. This would mean that CLUSTER_PEERS and CLUSTER_BOOTSTRAP env vars would be ignored though.

[hsanjuan]

Peers and Bootstrap are part of configJSON, but not Config

Yes, there is a comment next to them: // DEPRECATED. And used to print a warning when present.

The names of the env vars will be those of configJSON. The thing to do here essentially is to call envconfig like here https://github.com/ipfs/ipfs-cluster/blob/master/cluster_config.go#L309, but possibly towards the end of ToJSON().

The only problem is this will make envars always take precedence over whatever values Config{} has. Maybe we'd need a flag or a separate method (ToJSONWithEnv()) so that this behaviour can be only enabled with it's used for init. Right now it's not a big problem though, since init is the only moment we write the config.

[roignpar]

I do agree that adding a method like ToJSONWithEnv would be a quick fix, although I find it a bit peculiar logically speaking: there is some Config being used and manipulated in memory with a set of values, but when ToJSONWithEnv() is called, it gets encoded to JSON potentially with some other values taken from env overriding the ones from memory.

I find the first idea you proposed more robust: adding an ApplyEnvVars() method to config.ComponentConfig and config.Manager and call it when needed. In this case, right after setting the default values in init: https://github.com/ipfs/ipfs-cluster/blob/master/cmd/ipfs-cluster-service/main.go#L241

What do you think?

[hsanjuan]

I do agree that adding a method like ToJSONWithEnv would be a quick fix, although I find it a bit peculiar logically speaking: there is some Config being used and manipulated in memory with a set of values, but when ToJSONWithEnv() is called, it gets encoded to JSON potentially with some other values taken from env overriding the ones from memory.

I see this, the problem is you can't do envconfig with something other than jsonConfig because that's what has simple types that can be set directly from an env-var.

The ideal way is that env-vars get applied on Default(). But for this you need, at the end of Default(), to convert to jsonConfig{}, do envvars and re-parse from jsonConfig{}. For that you'll need to extract a toConfigJSON() *configJSON from ToJSON() and same for loadConfigJSON(jcfg *configJSON) from LoadJSON()`.

All in all it just seemed easier to stick a line in ToJSON, but it's def peculiar logically speaking.