Decouple the internal Config Go struct from the user config file in .ipfs/conf: use a subset of user overrides instead

[schomatis]

This issue is about fixing our internal technical debt without changing external UX. Some internal APIs will need to be broken but that can be minimized in a progressive deployment.

Problem

We currently map the JSON data of the user configuration file (UCF) in .ipfs/conf to our internal Config Go struct. This brings many problems:

• System defaults that might change over releases are hard-coded and locked in the UCF without the user realizing it (or even wanting to manage that particular configuration option).
• This causes the UCF to be bloated with default configuration options not set by the user, making it harder to realize which options have actually been changed or added. This makes the UCF harder to parse (both for the user and the developer triaging a new issue) with a giant blob of configuration options.
• There is no Go native way to mark a variable as unset. Variables always default to a Go value which might be different from the configuration default we may want that value to encode (the typical example is misinterpreting an unset limit with its default integer value of 0, instead of the expected "no limit" option). To avoid exposing the entire Config struct in the UCF we end up relying on stopgaps like the json:",omitempty" option, but that ends up creating even more confusion as it only just hides the underlying coupling that remains between these two sources of configuration.

The last point in turn has given rise to "optional" variables, OptionalInteger and related, to handle the JSON-GO mismatch of unspecified configuration options with the following drawbacks:

• Increased code bloat and associated maintaining costs, especially because we need one of this Go structures for each variable type the configuration encodes: Flag, Strings, Priority, OptionalDuration, OptionalInteger, OptionalString, and the list just keeps growing.
• Its WithDefault() API scatters system default values in the places that access these values, instead of centralizing them in the config package where they are defined. These system defaults end up hard-coded deep in the code base, or we flat-out copy an external default structure field by field like:

https://github.com/ipfs/go-ipfs/blob/7162a63e9629258bac0b9d135acae4c3c1b9fb33/core/node/libp2p/relay.go#L28-L40

(Looking at the above list now, it seems like the copy/paste used to write the last 4 lines left the text relayOpts.MaxReservations untouched without adding the PerPeer, PerIP, and PerASN suffixes needed. This is a perfect example of the motivation for fixing technical debt like this one.)

Proposed solution

We should clearly differentiate the two sources of configuration:

• System configuration: what the go-ipfs node is running with, encoded in a Config Go struct. It defaults to an internal set of values that depend on the node version.

• User overrides: Stored in .ipfs/conf, contains a JSON map of a subset of the configuration options the user wants to impose over the system defaults.

The user overrides are not the JSON-serialized version of the system configuration. We should not leak Go implementation details; for the user the only thing that exists is the JSON file (only internally do we use the Config struct to validate if those attributes are actually system-defined configuration options).

When the node starts it should load the Config with its internally-defined default values and only change the options specified in the user overrides, nothing else. The user overrides .ipfs/conf should be a bare-bones file after ipfs init.

All that is currently hard-coded in the .ipfs/conf file because of the exposure through the direct serialization of the Config struct, which at the moment is slowly and painfully being hidden through the cumbersome "optional" variables, will remain only in the Config struct as system-defined defaults (subject to change with the release version).

With this decoupling there is no more ambiguity on unspecified user options in the .ipfs/conf file since all options will be set in the internal Config struct: an option the user doesn't specify just leaves in place the system default (no more json:",omitempty").

Implementation

The progressive road to implementation is illustrated in this branch. The Repo interface is mostly maintained, what changes is that the Config struct (and related API calls) is no longer just the serialized user JSON file but instead the merge between the system defaults and JSON overrides (which are never serialized directly into the Config struct). The main objective of the first iteration is identifying which API consumers were relying on having all the configuration stored in the JSON file and start clearly differentiating between the user overrides and the internal system defaults: we should eliminate as much as possible the expectation of a user configuration (as it now exists) that contains all the configuration needed to run the node.

[lidel]

Quick thoughts:

• tldr: my initial reaction is 👍
  • Your approach in this branch feels backward-compatible, existing JSONs will "just work", so low risk, high return
  • In my mind, the main value added here is decreased maintenance cost for Stewards and contributors
    • We no longer need to do additional QA ensuring config changes are backward-compatible and we won't pollute JSON with empty structs just to avoid panics.
    • UX is also improved (right now we are in-between where some things are in JSON produced by ipfs init, some are not, and there is no clear convention why)
• Minimal $IPFS_PATH/config being only the diff sgtm
  • There would still be things like Addresses and Identity which we want to solidify during ipfs init, but the rest is just noise at this point
• Keeping implicit defaults in one place also sgtm
  • I can confirm that grepping Foo.WithDefault( every time I want to find where the implicit default is defined is tedious.
    • It is also error-prone: found myself in situation where I defined different default than the one that was already in other place, and had to refactor/bloat code to make sure only one default is defined at higher layers

[schomatis]

Thanks for the feedback @lidel. Will move ahead then with this approach prioritizing backward-compatibility.