RFC Introduce structural configuration (Deprecate flags and env?) #532

JoelSpeed · 2020-05-08T11:13:48Z

Expected Behavior

Configuration should be simple and easy for users of the project to understand.

Current Behavior

We have lots of weird hacks that allow people to overload strings to force many different options within a single option.

For example, --whitelist-domains allows a URL such as example.com to match only that domain, but .example.com to match subdomains as well. Instead, if we had configuration that was structured, we could be move explicit such as:

whitelistDomains:
- host: example.com
  includeSubdomains: true|false

This would not only be more obvious to users when configuring, but would reduce complexity when parsing the config.

Possible Solution

I think we should implement a structured configuration file and break up options into sections and those which have been overloaded should be made into real objects with proper options so we don't have to parse lots of weird strings and interpret these.

We have already implemented Viper within the project, so we can use that to load one of many different structures. My suggestion would be YAML, but since viper can interpret lots, we could have an option for the config format and pass that through as well, allowing people to pick their favourite (I think that's too much personally though I'm willing to be persuaded).

As for how we do the transition, there are several options:

Just do it

This would involve, within a single release cycle, completely rewriting the configuration to include a structured configuration. This would be a major refactor of the codebase and if we did this, we would likely want to write some utility that loads the existing configuration and generates config files for the user to make this transition easier.

Do it slowly

This would involve introducing a new flag --structured-config, and over the period of a few releases, migrating small sections of the configuration to the new structured config, while leaving the old configuration as it is. Then once we have done the entire structure redesign, we deprecate the old format and help people to move.

This would slow down the pace of the migration but would add additional complexity within the project temporarily. As I see it, we would need two public structures (legacyConfig and structuredConfig) which would represent the two config file types users could use, plus an internal config type that would be used within the codebase and some way to merge the two public types into the internal type.

Slowly but still using a single configuration file

This would involve switching the format over in a breaking change, and then slowly making lots of breaking changes that adjust the format of the configuration file to eventually introduce the structure that we want.

Additional ideas

Env from config files

Currently, every option that we have can be specified via env. This wouldn't work if we had structure, how do you address a field within an object within a list for example.

Dex solved this issue by allowing users to write $ENV_VAR in their structured configuration and running the equivalent of an envsubst on the file before parsing it. This would allow those users who want to keep env vars for certain parts of their config (eg secrets) to specify their own env vars and have them loaded. This would also allow complex lists of complex objects to be written and include env vars where required. (Example in Dex docs)

Deprecate most flags

As above, with a structured configuration, we lose a lot of the ability to map 1:1 our options to flags. So I'm proposing that we basically deprecate all flags bar a very skeleton set (eg --config, --version) as part of this migration.

OpenAPI?

Another idea that crossed my mind, is that with a proper structured configuration, it might be good to include an OpenAPI schema. I have seen these used before for validation (which would be a win for us, less validation code, just farm that off to the OpenAPI schema validation) and also to be converted into autogenerated docs. I'm not sure exactly how this would be implemented, but generated docs would definitely make me a lot happier about the state of our documentation.

The text was updated successfully, but these errors were encountered:

rustyx · 2020-05-09T11:32:14Z

Good idea. YAML can be developed as an alternative way to populate the internal config, thus minimizing impact, and eventually replace the custom .cfg format.

Just make sure to define an extensible YAML schema, e.g. avoid lists of strings when possible. For schema definition Cue is also a possibility as an alternative to OpenAPI.

wonderhoss · 2020-05-09T11:51:49Z

I like the idea. FWIW, I would prefer a single-cycle breaking change that leaves me with the new config on the other end and removes all the old string parsing code at the same time.

As for format, I'd probably favour toml over yaml. Viper supports both, so it would be more about what's easiest for humans to read/write.

How do you propose to handle secrets going forward? Retain flags / env or require the config file as a whole to be secret so that I can contain those, too?

JoelSpeed · 2020-05-09T15:34:09Z

Just make sure to define an extensible YAML schema, e.g. avoid lists of strings when possible. For schema definition Cue is also a possibility as an alternative to OpenAPI.

@rustyx This is a good point about lists of strings. I think there are some cases where it might be ok (cookie domains springs to mind) but for the most part you're definitely right, by making lists of objects we can add new fields easily later

@gargath when you say single cycle, do you mean as in cutting over with a major change or one release sharing both configuration formats?

For format, we could potentially also include the ability for users to specify the format via a flag, allowing them all of vipers options? My only issue with that would be it's harder to read when someone posts their config for diagnosis

For secrets, my suggestion would be having env vars in the config and then us basically doing envsubst as Dex does. Alternatively, we could do flags still but I see this potentially causing issues where you need multiple complex objects in a list containing secrets, eg if we started supporting multiple providers simultaneously

wonderhoss · 2020-05-09T21:07:47Z

@JoelSpeed I meant changing over in a single release. Maybe a 6.x? Supporting both variants simultaneously might be quite painful to implement and is of limited benefit once the switchover is complete.
To me, the biggest benefit of the change is increasing security and reducing potentials for mistakes in the config. People will have to change at some point anyways...
Maybe instead of supporting both, how about committing to backporting security fixes to 5.x for a while so that people who don't want to change their config can stick with that?

As for secrets, I am not too familiar with how Dex does it. I would just prefer not having to keep the entire config file secret. If there is a way to somehow specify the actual secret values in env vars, that would be ideal. Don't really mind the specifics as long as it's documented.

As for the format: my 2 cents are that allowing all options Viper supports is probably overkill. Also makes documenting harder and I see your point about people posting theirs in different formats.
Another downside is that searching Google for keywords from your flavour format might not bring up all relevant results.
Finally, there's no other project out there - at least that I'm familiar with - where multiple flavours of config file are supported.
I'd say let's stick to one.

JoelSpeed · 2020-05-10T09:47:17Z

@JoelSpeed I meant changing over in a single release. Maybe a 6.x? Supporting both variants simultaneously might be quite painful to implement and is of limited benefit once the switchover is complete.
To me, the biggest benefit of the change is increasing security and reducing potentials for mistakes in the config. People will have to change at some point anyways...
Maybe instead of supporting both, how about committing to backporting security fixes to 5.x for a while so that people who don't want to change their config can stick with that?

This makes sense. My only concern is that in not having both versions supported, we will not only be asking users to upgrade (through various other changes) but also rewrite their config completely. In this case, how do they know that whether the code is broken now or just their config if they can't get the proxy working again?

As for secrets, I am not too familiar with how Dex does it. I would just prefer not having to keep the entire config file secret. If there is a way to somehow specify the actual secret values in env vars, that would be ideal. Don't really mind the specifics as long as it's documented.

I explained this briefly in the original comment (Env config from files), I think this solves the issue and as you say, will be documented

As for the format: my 2 cents are that allowing all options Viper supports is probably overkill. Also makes documenting harder and I see your point about people posting theirs in different formats.
Another downside is that searching Google for keywords from your flavour format might not bring up all relevant results.
Finally, there's no other project out there - at least that I'm familiar with - where multiple flavours of config file are supported.
I'd say let's stick to one.

Makes sense, I'll look into the formats it supports then and see what I think about each, hopefully others have opinions. Since we've already mentioned YAML and TOML, what makes you prefer TOML over YAML?

wonderhoss · 2020-05-10T09:59:16Z

The yaml spec is notoriously complex and can yield unexpected results. There have been quite frequently bugs in yaml parsers.
Given the choice and assuming it's expressive enough for what we need, I would prefer toml on those grounds.
That said, it is only my tuppence.
Maybe people (probably?) being more familiar with yaml might be a counter argument.

JoelSpeed · 2020-05-10T10:26:28Z

Maybe people (probably?) being more familiar with yaml might be a counter argument.

The popularity of YAML and prevalence within the devops space was my main reason for choosing it, it will be easier for people to be familiar with. Though I hear your concerns about the spec and bugs

karlskewes · 2020-05-12T18:37:50Z

Thanks very much for this.

Like the idea of envsubst mechanism for secrets (or anything) in the config file.
We would continue to hydrate via Sealed Secrets controller. Not having this ability is a pain in the Prometheus AlertManager world.

Prefer toml or ini because yaml indentation is annoying. Especially when yaml is indented already like when embedded in a ConfigMap.
See Ansible inventory file (with groups and vars) options for comparison between ini and yaml that might correlate to the "multiple provider" idea.

Could there be a configuration change only release to simplify migration?
It could have its own config related bug fixes (if any).
Fast followed by feature release perhaps but no pressure.

fnkr · 2020-05-22T10:10:31Z

+1 for structured configuration and YAML (I find TOML hard to read). Also +1 for releasing this as a single breaking-change release.

I think this gives the opportunity to lay groundwork for some frequently requested features such as:

multiple identity providers (Plan to handle multiple providers at once? #88)
rule-based access control (e.g. Implement subdomain-based routing #204, Add ability to skip authentication based on request headers #451, Add ability to authorize requests using arbitrary expressions on the user's claims #466)

It could look like this:

identity_providers:
  google:
    type: google
  org_gitlab:
    type: gitlab
    endpoint: https://gitlab.example.com
rules:
  pre_auth:
    # skip authentication for requests from internal network
    - if: cidr_match(request.remote_addr, '172.17.0.0/8')
      set_policy: skip_auth
    # use org_gitlab as sole identity provider for staff tools
    - if: request.host == 'staff.example.com'
      set_identity_providers: [org_gitlab]
  post_auth:
    # limit access to staff tools to employees
    - if: request.host == 'staff.example.com' && !ends_with(user.email, '@example.com')
      set_policy: deny

JoelSpeed · 2020-05-22T10:19:20Z

@fnkr thanks for linking those relevant issues. This is possibly one of my main drivers for suggesting structured configuration, to remove the weird syntaxes we are overloading in strings

I had a look into TOML to try and understand what our config might look like if we went that route, I have to say, I'm not a fan.

TOML array handling means that you can split an array anywhere within your config, entries in the array aren't necessarily grouped and to me, that seems wrong. I think that will make debugging config hard, it will be much easier to miss an array element with TOML than YAML

github-actions · 2020-09-11T00:15:06Z