Read config values from env on init command

[roignpar]

@hsanjuan this is what I had in mind for #656.

In short, I split LoadJSON into another method, applyConfigJSON which only applies the configJSON values to the original Config and added the ApplyEnvVars method to Config which reads all env vars into a configJSON and overrides them back into Config.

All the other ComponentConfigs have empty ApplyEnvVars because they shouldn't read config from the env. At least that is what the docs say: https://cluster.ipfs.io/documentation/configuration/#using-environment-variables-to-overwrite-configuration-values

The options in the main configuration section (cluster) and the REST API section (restapi) can be overwritten by setting environment variables.

I now notice that LoadJSON assumed values to be set in the configJSON and thus will override config values with zero values if env vars are not set. Maybe we could have another method that allows all fields to be optional? Would it make sense to use only pointers for the configJSON fields to be able to distinguish fields that were set more easily (they would be nil)? i.e.:

type configJSON struct {
	ID                   *string   `json:"id"`
	Peername             *string   `json:"peername"`
	PrivateKey           *string   `json:"private_key"`
	Secret               *string   `json:"secret"`
	Peers                []string `json:"peers,omitempty"`     // DEPRECATED
	Bootstrap            []string `json:"bootstrap,omitempty"` // DEPRECATED
	LeaveOnShutdown      *bool     `json:"leave_on_shutdown"`
	ListenMultiaddress   *string   `json:"listen_multiaddress"`
	StateSyncInterval    *string   `json:"state_sync_interval"`
	IPFSSyncInterval     *string   `json:"ipfs_sync_interval"`
	ReplicationFactor    *int      `json:"replication_factor,omitempty"` // legacy
	ReplicationFactorMin *int      `json:"replication_factor_min"`
	ReplicationFactorMax *int      `json:"replication_factor_max"`
	MonitorPingInterval  *string   `json:"monitor_ping_interval"`
	PeerWatchInterval    *string   `json:"peer_watch_interval"`
	DisableRepinning     *bool     `json:"disable_repinning"`
	PeerstoreFile        *string   `json:"peerstore_file,omitempty"`
}

Have I missed anything?
What do you think?

[coveralls]

Coverage decreased (-0.5%) to 65.323% when pulling 06482e5 on roignpar:issue_656 into d49884b on ipfs:master.

[hsanjuan]

Have I missed anything?
What do you think?

I think that I'm stupid for not having realized you can set pointer fields on that struct to be able to distinguish between set and unset fields. And I have this SetIfNotDefault() (https://github.com/ipfs/ipfs-cluster/blob/master/config/util.go#L56) thing which is the workaround to that and which would become an if src != nil ... thing. Also probably simplifies the parseDuration things.

That, or there was really some issue that I have totally forgotten.

[hsanjuan]

All the other ComponentConfigs have empty ApplyEnvVars because they shouldn't read config from the env. At least that is what the docs say: https://cluster.ipfs.io/documentation/configuration/#using-environment-variables-to-overwrite-configuration-values

That's just because we started adding envvar support to a couple of components as we needed. every component should get it though.

[roignpar]

I just noticed that envconfig.Process is also called in ipfsproxy.Config.LoadJSON(). Should we implement a proper ApplyEnvVars? It's not part of the docs though. Should we just implement it for all component configs?

[hsanjuan]

I just noticed that envconfig.Process is also called in ipfsproxy.Config.LoadJSON(). Should we implement a proper ApplyEnvVars? It's not part of the docs though. Should we just implement it for all component configs?

Yes, please, implement for all.

[roignpar]

parseDuration is not simplified much; added the != 0 check where it's used because it returns 0 when the duration string is invalid, overwriting the default value. A way to optimize this would be to give parseDuration another argument, the default value, that would be returned when d == 0; a bit hacky, but would remove the != 0 check:

parseDuration := func(txt string, def time.Duration) time.Duration {
	d, _ := time.ParseDuration(txt)
	if d == 0 {
		logger.Warningf("%s is not a valid duration. Default will be used", txt)
                return def
	}
	return d
}

if jcfg.StateSyncInterval != nil {
	cfg.StateSyncInterval = parseDuration(*jcfg.StateSyncInterval, cfg.StateSyncInterval)
}

Any thoughts?

[roignpar]

Just noticed that ParseDurations exists in config/util.go. This could also be a solution to simplify the cluster config duration parsing. Is there a reason it isn't already used?

[roignpar]

Why is NodeHTTPS ignored in api/ipfsproxy/config.go ToJSON? Also, in jsonConfig it's string and bool in Config. Is this intentional?

[roignpar]

Also noticed that jsonConfig in ipfsconn/ipfshttp/config.go doesn't implement something like toNewFields and fields starting with Proxy are completely ignored.

[hsanjuan]

Just noticed that ParseDurations exists in config/util.go. This could also be a solution to simplify the cluster config duration parsing. Is there a reason it isn't already used?

Probably because it was introduced later and not backported to all the places where it could be used.

[hsanjuan]

Why is NodeHTTPS ignored in api/ipfsproxy/config.go ToJSON? Also, in jsonConfig it's string and bool in Config. Is this intentional?

I think it's a bug, it should be bool in jsonConfig and it should not be ignored in ToJSON() (the idea is that it remains "hidden" when generating the json config").

[hsanjuan]

Also noticed that jsonConfig in ipfsconn/ipfshttp/config.go doesn't implement something like toNewFields and fields starting with Proxy are completely ignored.

Yes, because these fields are kept around for compatibility and the json config is parsed with the api/ipfsproxy/configJSON object when the proxy section is not present. We should remove this soon anyway.

[hsanjuan]

Hmm I think things got uglier using pointers. I may have chosen not to use them because fields can still be set to empty values and then should be interpreted as "use the default value" in many cases.

Can we get this PR through without moving everything to pointers or is it a must?

If so, we should edit SetIfNotDefault to handle nil pointer values and try to introduce as little if jsoncfg.Value != nil as possible.

[roignpar]

Can we get this PR through without moving everything to pointers or is it a must?

We can leave out the pointer fields.

[roignpar]

My intention with introducing pointer fields was to reuse the code in applyConfigJSON methods.
I thought that since (the old) LoadJSON didn't use config.SetIfNotDefault() for all fields, the fields that are not checked for zero values are used directly and might put zero values in fields that were previously set. i.e.:

cfg := Config{}

// some values are now loaded from the given json
cfg.LoadJSON(rawJSON)

// the fields in `jsonConfig` that don't get any values from env will have zero values
// and will replace the fields that were set in the `rawJSON` used earlier
cfg.ApplyEnvVars()

I might be wrong, but this is how I understood it worked.

Using pointers made it easier to precisely figure out which fields had been set in both json and env with the same code.

As far as I can tell, we either need to adapt the existing code to take into account that any config field can be empty, or write new code that would only parse env variables. Is this not the case?

[roignpar]

A little detail:

type jsonMetricsConfig struct {
	EnableStats            bool   `json:"enable_stats"`
	PrometheusEndpoint     string `json:"prometheus_endpoint"`
	StatsReportingInterval string `json:"reporting_interval"`
}

Is it ok if we rename StatsReportingInterval to ReportingInterval to have consistent json and env variable names?

In MetricsConfig also?

TracingSamplingProb and TracingServiceName too?

type jsonTracingConfig struct {
	EnableTracing       bool    `json:"enable_tracing"`
	JaegerAgentEndpoint string  `json:"jaeger_agent_endpoint"`
	TracingSamplingProb float64 `json:"sampling_prob"`
	TracingServiceName  string  `json:"service_name"`
}

[roignpar]

Another option would be to use the envconfig tag:

type jsonMetricsConfig struct {
	EnableStats            bool   `json:"enable_stats"`
	PrometheusEndpoint     string `json:"prometheus_endpoint"`
	StatsReportingInterval string `json:"reporting_interval" envconfig:"reportinginterval"`
}

This requires less code changes.

[hsanjuan]

A little detail:

type jsonMetricsConfig struct {
	EnableStats            bool   `json:"enable_stats"`
	PrometheusEndpoint     string `json:"prometheus_endpoint"`
	StatsReportingInterval string `json:"reporting_interval"`
}

Is it ok if we rename StatsReportingInterval to ReportingInterval to have consistent json and env variable names?

In MetricsConfig also?

TracingSamplingProb and TracingServiceName too?

type jsonTracingConfig struct {
	EnableTracing       bool    `json:"enable_tracing"`
	JaegerAgentEndpoint string  `json:"jaeger_agent_endpoint"`
	TracingSamplingProb float64 `json:"sampling_prob"`
	TracingServiceName  string  `json:"service_name"`
}

Hello, yes please, rename these in a different PR so they match the JSON key and so we can merge them asap for the upcoming release that introduces tracing. Thanks!

cc ^^ @lanzafame

[hsanjuan]

My intention with introducing pointer fields was to reuse the code in applyConfigJSON methods.
I thought that since (the old) LoadJSON didn't use config.SetIfNotDefault() for all fields, the fields that are not checked for zero values are used directly and might put zero values in fields that were previously set. i.e.:

cfg := Config{}

// some values are now loaded from the given json
cfg.LoadJSON(rawJSON)

// the fields in `jsonConfig` that don't get any values from env will have zero values
// and will replace the fields that were set in the `rawJSON` used earlier
cfg.ApplyEnvVars()

I might be wrong, but this is how I understood it worked.

Using pointers made it easier to precisely figure out which fields had been set in both json and env with the same code.

As far as I can tell, we either need to adapt the existing code to take into account that any config field can be empty, or write new code that would only parse env variables. Is this not the case?

Right. Can we do the Config.ToJSONConfig() -> apply envconfig -> Config.FromJSONConfig() thing in ApplyEnvVars() ? At least for the moment, to simplify this changeset. We can do the make-things-pointers as a follow up, but it uglifies things a bit and I have the feeling we'll introduce some bugs or behaviour change that is not obvious at the moment.

[hsanjuan]

This is what it would look like if we don't use pointer fields. I think these are the minimum needed changes.

Keep in mind that booleans like restapi.cors_allow_credentials can never be set to false unless that is the default value. It is impossible to figure out if restapi jsonConfig.CORSAllowCredentials is false because it wasn't set or CLUSTER_RESTAPI_CORSALLOWCREDENTIALS was set to false by the user.

Can we do the Config.ToJSONConfig() -> apply envconfig -> Config.FromJSONConfig() thing in ApplyEnvVars()

Do you still want to do this?

Yes, see comment below. Doing that should workaround the problem above and be reasonably safe for the moment.

[roignpar]

Regarding smaller PRs, I got a bit carried away and wanted to fix everything in just one, but would you rather have the ipfsproxy NodeHTTPS fix in a separate PR? And one for using config.ParseDurations in cluster config?

[hsanjuan]

Regarding smaller PRs, I got a bit carried away and wanted to fix everything in just one, but would you rather have the ipfsproxy NodeHTTPS fix in a separate PR? And one for using config.ParseDurations in cluster config?

Those are fine here. Thanks!

[roignpar]

Instead of adding an ApplyEnvVars call after every LoadJSONFromFile call, I added a LoadJSONFileAndEnv method to keep the same behavior (loading env vars as part of LoadJSON).

[hsanjuan]

@roignpar is this good on your side? Looks ok to me!

[roignpar]

@hsanjuan Yep, I think it's done!

[hsanjuan]

Thanks so much!

[roignpar]

Thanks for all the feedback and patience!