Refactor client & server Configs

[amaury1093]

Summary

Client and server configs are messy (viper vs no-viper, confusing env vars & flags binding...) We should refactor.

Problem Definition

A quick summary: right now, in the CLI command, we create 2 instances of configs, one client and one server, see simapp/simd/cmd.NewRootCmd(). Then, all CLI commands can call either GetServerContextFromCmd or GetClientContextFromCmd to get the relevant config.

The confusing part comes with the difference between the 2 configs:

client.Context

Is a custom struct, has an internal viper instance. This Viper instance is only used for reading from ~/.simapp/config/client.toml since #8953. This Viper instance does NOT read from flags, flags are handled separately by client/cmd.go#Read*CommandFlags.

server.Context

It has its own (separate) instance of Viper. This viper instance does multiple things: reads from Tendermint ~/.simapp/config/config.toml, reads from flags & env vars, reads from SDK's ~/.simapp/config/app.toml, and merge everything together.

Global viper

We also have a couple of instances where we use the global viper singleton: example.

Proposal

For one, we should remove usage of the global Viper singleton.

One proposal would be to keep client.Context and server.Context separate, each having its own Viper, and each Viper handling all {flags, env, config file} for its own context. We would remove client.Context manually handling flags.

Another idea is to not use Viper at all, maybe for client.Context and server.Context to use (separate instances of?) koanf that's used in Atlas.

ref:

• add back cli config option for flags #8529 (comment) (this issue is step 2)
• Are there too many logs by default? #8132 (comment)

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[amaury1093]

I wonder if @alexanderbez (who's worked on a separate design on Atlas) @alessio or @aaronc have more ideas.

[dwedul-figure]

I was about to submit a similar request, but found this one and thought it best to add to it.

First, I highly recommend keeping Viper. It makes a lot of things easily automatic (such as using environment variables instead of config files).

From what I can tell, Viper was mostly designed to only ever have one instance. The global one is erased and reset during viper.New, and there's no setter for the global Viper instance (i.e. you can't create a new instance without replacing the global one). I feel it's best to use a single instance shared among all contexts and config pieces. The only time it would cause a problem is if two config files use the same field name.

I also recommend not ever using the global viper instance or global viper.* functions that are just wrappers on the global viper instance. Mixing global and instance calls can lead to confusion and hard-to-identify bugs. And since this is a library, there's no promise that the global viper instance doesn't change in unexpected ways. Both the client and server contexts have a viper pointer, and those should always be what's used (and probably point to the same instance too). Similarly, calls to viper.New() should be minimized as they can be irreversibly destructive.

Here are my proposed changes:

1. Update client/config/toml.go:

• Make the following pubic: defaultConfigTemplate, writeConfigToFile and getClientConfig.
• SplitgetClientConfig into ReadClientConfigFile(path, viper) and ExtractClientConfig(viper).
• Take in the full file path, and use v.SetConfigFile instead of hard-coding the filename.
• Use v.MergeInConfig instead of v.ReadInConfig. This makes it easier to use a single viper instance.
• Define var DefaultClientConfigFilename = "client.toml".

2. Update client/config/config.go:

• Rename the constants, adding Default to the front of them.
• Make the constants into variables.
• From ReadFromClientConfig, extract the portion that applies the config to the context into its own ApplyClientConfigToContext function.

3. Have functions return errors instead of calling tmos.Exit (e.g. when writing a config file fails).
4. Remove the server/config/config.go function GetConfig and use ParseConfig in server/config/toml.go instead. The GetConfig function is a whole bunch of code that doesn't seem to have any benefit over just unmarshalling the config from viper like ParseConfig. The special attention to telemetry.global-labels done in GetConfig isn't needed (i.e. v.Unmarshal parses it just fine).
5. Define ValidateBasic() error functions for all of the different config structs.
6. Consider defining some standards that can be applied to the three configs (config.toml, app.toml, and client.toml). An example entry might be that the toml.go file should contain a const DefaultConfigTemplate (string), a func WriteConfigFile(configFilePath string, config *Config) error, and a func ParseConfig(v *viper.Viper) (*Config, error).

[alexanderbez]

ACK. I always found this part of the codebase pretty nasty. Using an actual config instance as opposed to a global is certainly the way to go.

[gjermundgaraba]

@alexanderbez @tac0turtle When diving through #12722 I discovered the problems outlined here. Instead of adding more code and corner cases to this problem, I would strongly prefer to fix this first (bonus, it would make #12722 way more straightforward). Do you agree with this approach?

If yes, is the design outlined by @dwedul-figure still relevant or should we discuss the design more first?

[alexanderbez]

... I would strongly prefer to fix this first (bonus, it would make #12722 way more straightforward). Do you agree with this approach?

If yes, is the design outlined by @dwedul-figure still relevant or should we discuss the design more first?

Absolutely, thank you so much for taking the initiative on this. Very thankful for the help on both issues!

It's been a while since I took a look at this so I'm looking over @dwedul-figure's comments :)

---

First, I highly recommend keeping Viper. It makes a lot of things easily automatic (such as using environment variables instead of config files).

I agree. We could use something like Konf too -- I chose this for my other projects, because it's riddled with less bugs and random tech debt/cruft, but I would rather not bike shed on this and just keep Viper (for now) as long as it's not a global!

From what I can tell, Viper was mostly designed to only ever have one instance. The global one is erased and reset during viper.New, and there's no setter for the global Viper instance (i.e. you can't create a new instance without replacing the global one). I feel it's best to use a single instance shared among all contexts and config pieces. The only time it would cause a problem is if two config files use the same field name.

I don't necessarily agree here. While Viper is typically shown to be used via global instance due to easy-of-use in CLI contexts, I think it's a really bad design flaw. We should really just assume viper doesn't even support globals -- use viper.New() where a viper instance is needed. Globals make things very difficult to test and debug. That being said, we can use a single non-global Viper instance. Maybe this is what you're suggesting? If so, I see nothing wrong with that 👍

Otherwise, for the most part, the rest of the proposal seems reasonable to me! Thanks so much @dwedul-figure and @bjaanes 🙏

[gjermundgaraba]

Making good progress on untangling this, but I have a couple of questions

1: On the behavior of the config command: Is it supposed to take flags and environment variables into account?

The way it works now, it reads from environment variables when outputting, but not when reading in. To me this is potentially confusing behavior:

$ NODE=http://localhost:1 gaiad config node http://localhost:2
# The content of the config file has node = "http://localhost:2"
# In other words, NODE env was ignored
$ NODE=http://localhost:1 gaiad config node
http://localhost:1
# In this case it does take NODE env variable into account, even though we just set something else.

2: Can you confirm the precedence we want of different values?
Viper does this by default (first is the highest priority): flag, env, config, default value (set by flags usually).

3: I am a bit torn on where to put these changes. It doesn't make a lot of sense to me to have a generic viper/config reader in client/config (which then does two things). I could put it into a separate package under client (or directly under client), but should server even depend on client? Let me know what you think.

[alexanderbez]

Hi @bjaanes, to answer your questions:

1/2. Yes. flags > environment variables > configuration file. So in this case for the config command, it should be flags > environment variables.
3. Who will be the consumers of this single configuration? Both server and client logic?

[gjermundgaraba]

1: So is the example I gave the correct behavior for the config command?

3: That is the idea; to standardize the setup of Viper

I've found multiple places where SetEnvPrefix is used differently and I think the only reason it works at all might be because the Tendermint codebase is using a global Viper. So it overrides the latest Viper instance made... It's a bit of a mess.

https://github.com/tendermint/tendermint/blob/3bd215313654ab4d125cb85d9a52b82f67e7b464/libs/cli/setup.go#L48-L55

[alexanderbez]

1. Yeah, I think so? The first command set it to http://localhost:2, correctly. The second command set it to http://localhost:1, correctly (since no arg was provided).

Yes, it's very messy. I can't think of a good place atm. I guess I'd have to see how it's all wired up and used to make a good argument. Perhaps, for now, we add it to a separate package under client/?

[gjermundgaraba]

Sounds good with the separate package.

As for 1: the second command doesn't set it. It just does output and it is a bit strange for me that the config command would take env variables into account. In the second command it ignores the configuration file instead of doing what I would expect which is just output value in the config file (since the config command is supposed to be about "Create or query an application CLI configuration file")
To illustrate more clearly:

$ NODE=http://localhost:1 gaiad config node http://localhost:2

$ NODE=http://localhost:1 gaiad config node
http://localhost:1
$ gaiad config node
http://localhost:2

[alexanderbez]

I believe the ENV var should set it.

[gjermundgaraba]

@alexanderbez I have spent some time trying to find a good holistic solution, but I might have gotten a bit blind on it, so would appreciate some feedback on where I am right now.

I pushed my current changes here: main...gjermundgaraba:cosmos-sdk:issue-9034

If you prefer I can create a draft PR instead.

It's a bit big atm and there is a lot of cleanup and tests left in any case, but I want to know if the approach is reasonable.

What I essentially have tried to do is make it all flags and configuration a bit more unified and straightforward. Some of the mess here comes from different ways of doing the same thing all over the place with environment variables prefixes being ignored some places and not others etc. There are almost certainly some potentially breaking changes here if someone depends on some of these strange behaviors (such as the server-related env variables always being prefixed with the name of the binary - this can be brought back if we split the viper instance in two).

Currently nothing will really work if a viper instance is not set. This shows in the number of test files I had to add .WithViper(viper.New()). I was not sure if this was OK. I suppose the alternative is to lazily set create one if it is nil.

Happy with feedback on small and big things here :)

[alexanderbez]

Nice, thanks for the amazing progress thus far!

A few points:

1. I wonder if we can drastically simplify our approach. Have $ <appd> config ONLY work with flags -- no env vars.
2. Re .WithViper(viper.New()) everywhere in tests, how is the Context initially created? Is it literals like Context{}? Or NewContext? If the latter, then we can just set Viper in the Context's constructor.

[gjermundgaraba]

1: Sure, that would make it a lot simpler.
2: Yeah, that part I am not so happy about, but yes they are all created as literals Context{}.

[alexanderbez]

Sigh. I think we have to use NewContext, but that still is a big diff, right?

[gjermundgaraba]

I suppose so. I don't think there is a NewContext for client context. We could obviously make one, but I as for the diff it would be big in any case :/

[gjermundgaraba]

I took a pause from this since I saw there was a lot of work happening for 0.47 and didn't want to introduce too much noise here. Timeline wise, when would it be wise to continue this work @tac0turtle (and/or others)?

[tac0turtle]

are you able to open smaller prs to get things through or need one large pr?

[gjermundgaraba]

It might be possible, but my experience so far tells me that it might get tricky because as soon as you start pulling on a thread here you get bananas, gorillas and jungles coming out very quickly.

I could try to create an initial PR that just creates an unused configuration setup. Then try to go as incrementally as possible from there. The initial approach I made can be seen here (ignore test files and it is not that big): gjermundgaraba@c2c443e#diff-66fb818e264f283f2651c29bb9363f3477b133dc0bd7841df19e2e53c9f21581

[gjermundgaraba]

@tac0turtle @alexanderbez I made a smaller draft PR here which we can use to discuss and see if this is a good approach: #14428

[gjermundgaraba]

I'll just step out this one for now. It's totally fine, I very much understand that it takes time and energy to follow up external contributors. :)

If you do want me to continue on this in any capacity, let me know. Or if there are other areas you might want some help instead. If not I'll go elsewhere to contribute :) (again, I get it and it's really no problem).