Viper config loader

[doublerebel]

This PR is waiting on PRs spf13/viper#164 and magiconair/properties#9, but I thought I should open the discussion now.

Viper enables defaults to be set directly from a Config struct, and automatically matches flags and env vars. This removes boilerplate code from Fabio. Moving forward with Viper, new flags and new defaults can be added in one line each.

As a side effect, all filetypes Viper supports are supported for Fabio: JSON, HCL, YAML, TOML, and Java .properties.

Related: our discussion in #43.

I haven't flattened the commits so that the change is easy to follow. Once this PR meets your approval I will squash the commits and re-push.

Thanks!

EDIT: This does introduce one breaking change: Viper uses spf13/pflag (Posix flag), and I can't find any way to set single-dashed -cfg without Viper parsing that as shortflags -c -f -g. Instead we are left with double-dashed --cfg.

[CLAassistant]

All committers have accepted the CLA.

[magiconair]

Can you explain what problem this solves? I got the motivation for the ENV var overrides in #43 but I don't see the benefit of supporting N different config file formats.

[doublerebel]

This greatly simplifies Fabio configuration and setup, and ensures that it's easy to track config settings from the default Struct, env vars, and flags. The unified format means there is no guessing where the default is set, or which env var or flag corresponds with the Config struct.

For instance, the change to fix #48 introduced a bug, which overwrites my fix from #30. It introduces a new setting ServiceAddr, and no longer uses cfg.UI.Addr. The example properties file still lists the service registration address as defaulting to ui.addr, and also incorrectly lists the default setting as registry.consul.register.ip. The Config struct uses cfg.Consul.ServiceAddr, so it took me a while to figure out what had changed and what name of env var would override this setting.

As Fabio increases in complexity, naming and tracing settings will also become more complex. For instance, the PR for Google Cloud Platform backend introduces a new list of settings. Using Viper will eliminate the boilerplate code required to introduce new settings.

To iterate, test, and deploy Fabio, flags are even easier and quicker than env vars. Env vars are important for 12-factor apps, but the use of flags as an override is common and helps support many more use cases. With Viper we can also add flags with minimal additional code.

The extra file formats are not my primary goal -- but e.g. JSON is easy to generate from any language, which makes it simpler to automate deployment and configuration of Fabio.

EDIT: Added flag for registry.consul.serviceaddr, and cleaned up commits with gofmt.

[magiconair]

I don't think that any of the examples you list apply since they all stem from the information being distributed over several places. Regressions happen and so do refactorings for fixing issues (i.e. consul.serviceaddr). Unless I'm missing something, using viper would have not prevented any of these issues since I simply forgot to update the default config file.

Also, the FABIO_ prefix for the env vars probably breaks existing setups since the current implementation does not require this.

The reason I chose the properties format - besides the fact that I wrote the lib - is to have a commented default configuration file. That you won't have with JSON so HCL or TOML should be the choice and here the (IMO fruitless) config file format discussions begin. Also, fabio should have as little configuration as possible in the config file and it should be static so this should be something you touch rarely.

What you call boilerplate is what I call "one dependency less" since it is much simpler for me to fix a bug in my own code than in someone else's. To quote Pike: "A little copying is better than a little dependency." (https://go-proverbs.github.io)

I understand that you prefer the viper approach over homegrown code and I appreciate the effort you put into this. I also really appreciate you pointing out the current bugs and regressions. Right now I am just not convinced that using viper would have prevented and will prevent these things in the future.

I am not opposed to merging this but I'd like to do it for the right reasons. I'll think about it and your suggestion about using command line flags in addition to env vars and config files. Ideally, fabio should not require people to dig that deep unless there is a bug. You should download it and be able to use it right away.

[doublerebel]

Thanks very much for considering my patch. I know it is not a small change.

The FABIO_ prefix comes from the discussion in #43, I agree it's not required. Although since --cfg will also be a breaking change, it may be a good time to change if wanted.

I really like the intention of Fabio to be minimal config, so it can be downloaded and used right away. One of my goals is to simplify from a developer perspective, so that it's easier to extend Fabio right away. It might help if I explain how I am using Fabio and why I needed to extend it:

My architecture is designed to be reactive, as encouraged by Consul. I have Fabio running on n machines with >=3 networks attached per machine. The networks are defined as VLANs using Joyent's SDN. For security the ui.addr, proxy.localip, and proxy.addr are all on different networks. A machine may be detached and attached to any network at any time, or the image can be moved to another node. Using Solaris SMF, the Fabio service is defined as dependent on the network service, so Fabio will restart whenever the network is changed.

In order to set the different networks each time Fabio launches, I looked at a few solutions:

• Generate config using consul-template. This seemed like overkill but would work. However, I would have to setup another watch for the template, and state would still be stored in a file, leaving potential for bad state.
• Set an env var for every network setting. This is simple and can be done in the service init script. However, every service I use defines different env vars for their network settings. Now instead of storing state in 1 config file, state exists in >3 vars of different names, leaving potential for bad state.
• Extend Fabio to take flags. Flags are how I set networks for the majority of my services, and it is very clean. I can essentially do fabio --proxy.addr $(get_public_ip.sh), and state is not stored anywhere. This ensures that the networks are set directly from the nic values and correct at every launch.

For me the variable is IP addresses, but for other reactive architectures the variable could be any setting. That's why I support liberal acceptance of config via files, env vars, and flags. I did not see a way to add many flags without also creating a lot of code duplication and potential for me to make an error. Also debugging the config from all sources and saving to file became issues that I found already solved in viper.

Thanks again for your time, I really appreciate the discussion.

[magiconair]

So the real problem is not that the configuration is difficult but that the listeners are not dynamic enough. I am aware and want to change this. This actually has to change as soon as the dynamic TCP proxy is possible. Currently, the url prefix configuration and the static listener configuration are blocking this. However, given my current workload I don't know when this will happen so I'll probably merge this anyway.

Some questions:

1. is there a way to not introduce a breaking change for --cfg? Can't you parse the cmd line args manually? Isn't there a hook in viper?
2. what about PR#9 in magiconair/properties you have filed? Is that required for this?
3. Also, could you drop the FABIO_ prefix also for the reason of backwards compatibility. Is there a way to support both with and without prefix and deprecate the env vars without?
4. Could you have a look at the failing travis build for your change, please?

[doublerebel]

No problem, I have started working on these items:

1. I was able to manually keep -cfg flag by removing it from the arguments list. For reference, this is in commit dc6d851 so it may be removed later. I also have kept -v as shorthand for version.
2. Yes, magiconair/properties#9 is required since Viper does its own Unmarshal to Struct after merging defaults, env vars, flags, and config file.
3. Yes, I can patch vendored Viper to support both: env vars with FABIO_ prefix, and deprecated without.
4. I know the failed build is due to missing vendored packages, I will properly vendor all the dependencies and add them to this branch.

[magiconair]

I've left some comments on magiconair/properties#9. Lets address them so that we can move forward with this PR.

[magiconair]

This will be superseded by #79 unless I'm missing something substantial.