Confusing behavior with missing (required) configuration values

[bluekeyes]

Describe the bug

(This is arguably 3 distinct bugs, but they are related and each led to the next.)

(1) The documentation suggested to me that configuration properties that are not set in a configuration file will use the documented defaults; this is the behavior if environment variables are used for configuration. Instead, any properties not explicitly defined in the configuration file use the zero values for their types.

(2) For some properties (I hit it on DownloadMode, but may apply to others), the zero value is invalid and the property is effectively required, but is not marked for validation.

(3) When the the DownloadMode property is missing (i.e. it is an empty string), the logged error does not clearly indicate the problem or what the cause was, printing a vague Bad Request error (even though no network request was made.)

Error Message

2019-08-12 20:11:55.325066 I | error adding proxy routes (Bad Request)

To Reproduce

1. Create a configuration file that does not specify a DownloadMode
2. Start the server using the configuration file

Expected behavior

1. Values not explicitly set in a configuration file use the documented defaults
2. Failing that, all required properties fail validation with an error showing they are required
3. Failing that, logged errors include details about the operation/code that failed

Environment:

• OS: Docker on Ubuntu 16.04
• Go version: ?
• Proxy version: v0.5.0
• Storage (fs/mongodb/s3 etc.): s3

[arschles]

@bluekeyes Do you think a log to indicate what's missing, and removing docs that talk about defaults be a decent start to solving these 3 problems?

[bluekeyes]

I think logging some of the additional details attached to errors and making sure all required properties are marked with the required tag for validation would be a good start: that existing validation was pretty helpful when I first started the server with my minimal configuration file.

I'm not sure what the best way to improve the docs is, since the defaults are still relevant for people using environment variables. On a closer reading, I realized some parts say that you should copy the development configuration file and modify it as needed, so maybe that could be emphasized on the "Installing Athens" or "Configuring Athens" sections?

[twexler]

I also just ran into this when handwriting my own config, I think the easy change is to make DownloadMode required, but I think the correct fix is to actually set defaults when they should be.

[marwan-at-work]

The config file takes precedence over the defaults, and therefore if you're using your own config file, you want to make sure it's a modified version of the config.dev.toml file. That said, the error is definitely confusing and we should either default to Sync for DownloadMode, or tell the user that they are not using it correctly and error out.

[arschles]

@twexler Do you think we can close this issue, and you can open 3 new ones, one for each of the items in your description above? I think it will be easier to work on each in parallel that way. I will pick up some of them.

Also, I saw that #1365 fixes some of the problems above, but only for a single config value.

[twexler]

@arschles I've sort of lost context on this, I believe you're talking about the 3 items in @bluekeyes' description though?

[arschles]

@twexler yup, that's what I meant

cc/ @bluekeyes

[bluekeyes]

Sure, I've created #1490 and #1491 for issues (1) and (2) in my original list.

The problem in (3) was mostly fixed by #1365, although I see that errors.KindBadRequest is still used in some situations. I'll need to look at that one a bit more to see if there is still a separate issue to report related to error handling.

[arschles]

@bluekeyes that's great, thank you. I am working on making our configuration error messages better at the moment, which may help with #1491. I'll submit a draft pull request and tag you to get your opinion.

Thanks again 💚