doc/CONFIG-NOTES.md: config proposal

This is my draft proposal to address #285 (closed). The text is identical to that in #285 (comment 2770848)

mentioned in issue #285 (closed)

assigned to @Diziet

I think this idea is interesting, though if we want to support it in the general case, we might to talk a bit about namespacing. (Does serde support multiple flattens like this? Can we re-serialize the combined files with this?)

Sure it does, and you can re-serialize: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=56529f2371f3c982850784b54e3bb412

And yes, there is a namespacing issue: you can't have sections that are provided by our code which have the same names as the caller's sections. serde's behaviour is to take the first when there are clashes. So the suggestion I am making causes our sections to override the caller's. That may not be right. But I think whichever way we choose for that we can address that with appropriate documentation.

I don't think that's what the default is now: it's currently in arti-config:

/// Return a filename for the default user configuration file.
pub fn default_config_file() -> Option<PathBuf> {
    CfgPath::new("${ARTI_CONFIG}/arti.toml".into()).path().ok()
}

So that should in theory use XDG dirs on XDG platforms.

I think there are multiple defaults at different layers. I'm sure I found a ./arti-config.toml somewhere :-). IMO there should be one default config path and yes, XDG dirs by default on Unix.

It is a mystery: git grep arti-config.toml finds nothing for me.

You might be thinking of this:

eta@euston ~/s/arti ((a9be6c9e…)) [1]> rg config.toml
crates/arti/src/main.rs
127:    let dflt_config = arti_config::default_config_file().unwrap_or_else(|| "./config.toml".into());

Yes, I think so. Well found.

So, right now it's in arti-config, on the theory that arti-config is where you look if you want your application to support the same configuratoin format as arti, and not otherwise. It sounds like in this proposal you're lowering part of arti-config into arti-client, but I'm not sure exactly how much.

Everything that knows about what's in TorClientCfg at least, I think, since we want all of that logic to be shared with everyone who uses arti-client.

I think what is left in arti-config is mostly the cmdline handling, and maybe some of the logging. If we want a principled distinction I think it's probably that arti-config is "stuff that is specific to the arti command line program, including its logging config, and parsing of config from the command line". So reuseable by an embedder other than arti, but we don't much mind if embedders do this stuff differently, so it can be off in another crate.

But, I am not wedded to the precise crate structure at this level. What I do think we want is that if an embedder has configurable Tor client, the easy path for them ends up with their Tor client configurable using the same files that arti uses. Whether the rest of their program's config is their own section in that config, or in a different file, or whatever, is something I don't think we need to have an opinion aobut.

What I do think we want is that if an embedder has configurable Tor client, the easy path for them ends up with their Tor client configurable using the same files that arti uses.

It's worth pointing out that this is very much already the case!

To make sure I understand: the rationale for this is to make it so the user doesn't need to care about separate builder types?

(open question)

Yes, indeed. I think the separate builder types, and the two layers of error reporting (once to get a FooConfig from a FooConfigBuilder, and once to get a TorClientCfg from its builder, is not helpful.

I'm okay with this if it works and doesn't require changes to our current configuration file formats. (Fortunately we have a test for that)

I think it shouldn't, because I think serde default and Option work near enough similarly. My hope is that it will bring the API and the config file closer.

I guess there might be minor differences.

I believe we do that now; I still think it's a good idea.

(I assume when you say FooConfig you only mean the tor-* FooConfig structures, not the ones currently in arti-config.)

I believe we do that now; I still think it's a good idea.

(This is apropros re-exporting.) I confess I didn't check. I'm not sure if all the crates are handled the same way, either.

(I assume when you say FooConfig you only mean the tor-* FooConfig structures, not the ones currently in arti-config.)

Yes.

I'm okay with this, though I wouldn't be okay with applying the same logic to the other FooConfig objects. I see below that we're in agreement about that.

This does mean that we can't easily rename a configuration section without an API break.

This does mean that we can't easily rename a configuration section without an API break.

Yes, but doing that is a config file compat break too, so let's do that very rarely.

I think that test-case for "(a)", or something like it, exists as arti_config::options::test::default_config.

(Another purpose of having arti_defaults.toml is to make sure that the defaults in the code match what we think they are, and that the encoding matches what we think it is.)

I'm okay with making things commented out by default.

OK, cool. Thanks for the ref to the test case.

I think I'm good with this design, modulo questions above, though of course I'd also like to see @eta's feedback.

Three things to think about before proceeding:

• Is there a way to do any of this incrementally as a series of smaller MRs? Ideally, we'd do the riskiest parts first, so that we can be sure that all of our ideas work out.
• What example code should we provide? (I think doing some example code for this would be a valuable way to confirm that the API is ergonomic.)
• How does this interact with the reconfigure() design?

Is there a way to do any of this incrementally as a series of smaller MRs?

There are a number of different things going on here and yes I think they can be split up. The most risky parts are (i) conflating the builder structs with what the API thinks of the config and (ii) the high-level changes about how the files are read. I think (i) can be tried out with one crate first without causing anything other than an API break. (ii) can be done separately from the rest.

What example code should we provide? (I think doing some example code for this would be a valuable way to confirm that the API is ergonomic.)

I'm hoping that arti will become even more trivial than it is already :-). arti then becomes the prototypical embedder.

How does this interact with the reconfigure() design?

I confess I didn't think about this properly but I don't think there are fundamental problems. I will go and do some more reading and update my proposal.

requested review from @eta

This is just a rewrap ?

this sentence randomly trails off?

Oops, will fix. What I have here is

And because the FooConfig is already partially validated, it just provides clutter in the caller's code.

changed this line in version 3 of the diff

So I don't think having builders is actually bad per se, although I've never understood the need to "validate" configuration when using them (this is more of a complaint against derive_builder). An ideal design would reject invalid values when calling the setter method, instead of when calling build() at the end, i.e.:

Foo::builder().field(...)?.build()

instead of

Foo::builder().field(...).build()?

In a lot of cases, though, I don't think there even are invalid values (and we might be able to constrain those that exist using the type system, so they're catchable at compile time) — which would let us get rid of the need to do this at all.

One issue is that there are sometimes settings that are individually allowable, but not mutually permissible. For example, there are some settings that we should only allow you to reconfigure if you are on a nonstandard Tor network (not with the default authorities).

(This isn't the case for most options, but it does come up.)

not mutually permissible

Right, so the thing with the builder methods necessarily has "invalid" states. I think that is OK. And given that, it is not necessary to completely validate the valid range (say) of every config parameter.

I guess I'm saying validating individual values early is rather more work and probably doesn't buy very much. I'm not against it but I'm not convinced that it is worth making every setter method on the builder fallible.

As for Deserialize, I think the Deserialize implementation should reject invalid values (possible via serde::de::Error::custom, probably by using deserialize_with on fields that require checking).

See above re validation of values. I think Deserialize and the builder methods should have the same semantics.

why would this throw / be fallible?

from_files definitely needs to be fallible. Perhaps the files can't be read because a fileserver is down or something.

I guess builtin_defaults doesn't need to be fallible but I don't want to tempt people to call that instead just to avoid having to do error handling :-)

This isn't actually clearer whatsoever, IMO: now I have to go and look at what that means ("throwing" is not a very familiar concept in Rust-land). It'd be better if everything just used Result.

OK, I was just writing a sketch. I will refrain from speaking in fehler around here in future.

I'm really unsure about this whole "the caller will want to merge their config file format with ours and load them from the same file" idea: it strikes me that almost no serious embedder would want to use such an API.

What might be more useful is an API that let you make a TorClientCfg from a serde_json::Value or similar.

What might be more useful is an API that let you make a TorClientCfg from a serde_json::Value or similar.

In my proposal TorClientCfg is Deserialize so you can indeed convert it (fallibly) from serde_json::Value.

I'm really unsure about this whole "the caller will want to merge their config file format with ours and load them from the same file" idea: it strikes me that almost no serious embedder would want to use such an API.

The arti executable itself wants it. Anyone who wants to do something "vaguely like arti" ought to be provided with the same facility. This is just a consequence of the general API principle that functionalities which people might want to use separately shouldn't be provided only via an API which welds them together.

Eg, maybe you want a version of arti which is also a web reverse proxy for onion services, so it will be "just like arti but with this extra config stuff for the web routing"

I think the intent behind this design doc is generally very good, although I think my ideal vision for what config should look like in Arti differs a bit.

Here are some general summary points I have of the proposal, as is (in addition to the more specific inline review comments):

• I agree with @Diziet that it's confusing having separate unvalidated and validated config formats.
  • However, I think the way to solve this is to do away with the entire concept of an unvalidated / incorrect configuration, at the type level.
  • For example, the Deserialize implementation for a configuration type should fail if presented with values of the correct type that are otherwise incorrect.
  • This could be implemented in practice by having a bunch of newtype wrappers, or by using #[serde(deserialize_with)].
• I'm not so sure how great the derive_builder crate is. Currently, I'm leaning towards the idea that replacing it with a bunch of manual boilerplate might be a good thing in the long run.
  • One specific problem: the fact that build() is fallible, instead of the individual setter methods being fallible. (See the relevant review thread about this.)
  • More generally, though, having that crate codegen a bunch of builder stuff for us that's hard for us to modify / tweak to our specific needs seems like a pretty large downside, especially given we care about API stability / design.
  • Autogenerating documentation also doesn't lead to the best results, either.

As for how I'd propose improving things myself: I think it could be a nice idea to get rid of the builder pattern and just move to regular old setter methods — instead of starting out with a blank slate and constructing things from there, users would start with a default config and call set_xyz to change things.

In addition, it might be nice to have a "merge configurations" function (as in, merge A with B, preferring A's settings if they both differ from the defaults, otherwise use the one that isn't the default), which might even replace a lot of what the config crate does for us.

• However, I think the way to solve this is to do away with the entire concept of an unvalidated / incorrect configuration, at the type level.

This might be possible, though I'm not sure.

(It won't do away with the need for validation at reconfiguration time, though, since not all transitions are going to be supported.)

• I'm not so sure how great the derive_builder crate is. Currently, I'm leaning towards the idea that replacing it with a bunch of manual boilerplate might be a good thing in the long run.

I'd lean towards this as a "long run" idea rather than a "right now" idea. One of the challenges with manual boilerplate is that it's easier to mess it up: setting the wrong option, or having the wrong return type, for instance.

Another possibility is to improve derive_builder to meet our needs, by adding support for manual documentation overrides and infallible build() methods. (I think it might already support fallible setters, but I could be wrong there.)

In addition, it might be nice to have a "merge configurations" function (as in, merge A with B, preferring A's settings if they both differ from the defaults, otherwise use the one that isn't the default), which might even replace a lot of what the config crate does for us.

It's actually important that we distinguish "has the default value" from "is explicitly set to the default value" here: If we want to apply configuration A on top of configuration B, then it matters whether A has left option X unset, or whether A has explicitly set it to the default value.

added 1 commit

• 6ba3686a - CONFIG-NOTES: Suggestions from MR review

Compare with previous version

do away with the entire concept of an unvalidated / incorrect configuration, at the type level

I think this is the most fundamental point here. I agree that making invalid states unrepresentable is generally a good idea.

But, here, I don't think it is practical or adviseable.

Firstly, it's not practical because of the point @nickm makes about combinations of settings possibly being invalid. If invalid combinations are unrepresentable then either setter methods would have to be called in a particular order (and, depending, there might not even be a possible order), or we would have to provide multi-setter methods, possibly in multiple combinations.

Secondly, it is actually useful to be able to represent a possibly-invalid configuration. That allows a program to read a configuration from disk and fix it up programmatically, for example. I think it will be easier to produce good error messages if we can have an in-memory representation of what everything is and where it came from - and if we do more of the validation in our own bespoke code, rather than in a Deserialize callback.

Stepping back: the principle of making invalid states unrepresentable is great when the invalidity might come from a bug (including a failure to check something). It allows the compiler to spot these bugs and render them statically impossible. But the scheme I propose already has separate types for unvalidated and validated config, so this is already the case within our own code. But at the API level, config data is not a complex data structure that is constructed programatically: it's very data-like, and often comes in from the outside. There is nothing wrong with having an ability to represent unvalidated data if you can be sure not to use it by mistake.

improve derive_builder

It's true that my proposal doesn't automatically give us good documentation for our config. The result will be tolerable, but it will leave people inferring the necessary TOML syntax by extrapolating from setter method names and types. Really we want a bespoke doc which describes the TOML format, but which is generated from the doc comments we write in-code (or some other single source). derive_builder can't do that right now. We might need to find or write a tool for this. But I don't think this is on a critical path for 0.1.0. Unlike Deserialize, the use of derive_builder doesn't become part of our API.

As for the build method being fallible or not: my suggestion is to hide this method entirely. Users would pass in a hopefully-valid TorClientCfg and it would be converted internally to a known-valid representation, using whatever combination of autogenerated and handwritten code is most convenient. Right now that would be a combination of the autogenerated build plus hand-rolled validation.

added 1 commit

• 85ed27f6 - CONFIG-NOTES: finish a sentence

Compare with previous version

approved this merge request

merged

mentioned in commit 4aa442de