Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

autohttps: traefik's config now configurable and in YAML #1636

Merged
merged 5 commits into from
Apr 24, 2020

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Apr 19, 2020

PR summary

YAML instead of TOML

The autohttps deployment run Traefik, and we are in this PR switching from configuring it with YAML instead of TOML, for two reasons.

  1. It avoids introducing TOML as yet another markup language to understand to maintain this repo.
  2. Yaml is supported better by helm (fromYaml exists, but not fromToml), and I want to use this.

Extensible configuration in a sustainable way

The configuration options proxy.traefik.extraStaticConfig and .extraDynamicConfig are added to allow a chart user to add generic configuration into Traefik's config. By doing so, we setup for a more sustainable experience that doesn't require the Chart maintainers to add more and more templating options, while also allowing the chart user to configure whatever is of relevance.

The end goal of this work is to help for example BinderHub hook into our automatic HTTPS setup by adding endpoints that traefik can route to to get HTTPS through Z2JH's setup.

Implementation tricks explained

I've opted to create defined templates, one to render each Traefik configuration file. This allow me to take it and pass the rendered templates to fromYaml that transforms it back to an object as compared to big string of YAML. Following that, we can merge this object with .extra<Static|Dynamic>Config.

By doing this, we get to inject content from for example proxy.https.hosts into the configuration, and also allow the chart user to then do any change it likes.

@consideRatio consideRatio changed the title DX: TOML -> YAML to reduce language barriers WIP - DX: TOML -> YAML to reduce language barriers Apr 19, 2020
@consideRatio
Copy link
Member Author

consideRatio commented Apr 19, 2020

(removed to avoid clutter)

Traefik can be configured with both YAML and TOML, this transitioned the 
current TOML configuration to a YAML configuration.

- I added some comments while learning about the configuration as well.
- I made the log level become DEBUG if debug.enabled was configured
- We now explicitly set the ACME server to use and avoids using 
Traefik's default value.
@consideRatio consideRatio changed the title WIP - DX: TOML -> YAML to reduce language barriers autohttps: traefik's config now configurable and in YAML Apr 22, 2020
@consideRatio
Copy link
Member Author

consideRatio commented Apr 22, 2020

@yuvipanda and @GeorgianaElena, I make a trick I do in this PR to configure Traefik in a way that hopefully avoids a very common problem for Helm charts - that we end up adding more and more and more templating in order to allow the user to configure more and more.

The trick allow us to take a rendered Helm template and let the user merge any additional configuration into it!

  1. Render a template inside a defined function.
  2. Cast the rendered template (string) into an object using the fromYaml function.
  3. Merge the user provided configuration into the object.
  4. Render the object into a string using the toYaml function.
  5. Write the rendered string to a ConfigMap resource, so it can become a file such as traefik.yaml mounted in a container.

By this approach, I hope we can avoid a common Helm chart problem described in this video.

What do you think?

Copy link
Member

@GeorgianaElena GeorgianaElena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @consideRatio ❤️

I love the idea of letting users add their own custom config and that the static and dynamic config are separated into their own files. This seems less scary than having one big file and it's very nice that you don't need to restart whenever you modify dynamic config pieces.

LGTM 🎉

From the [RFC documentation](https://tools.ietf.org/html/rfc6797#section-7.2)
it seems like we "SHOULD" make the http->https redirect permanent (301)
instead of a temporary redirect (302), and that we "MUST NOT" include an
STS header in our redirect response over HTTP which is insecure.
A preload value in the STS header indicates that the webserver wants the
browser to add this website to a list which should be considered to be
HTTPS only that can influence other browsers that haven't even visisted
this webserver before.

One can also manually add oneself to such list from
https://hstspreload.org/ if one complies with their requirements, of
which one is that a preload value in the STS header is specified.
@consideRatio
Copy link
Member Author

After having read more about HSTS, I also added the option to specify the preload directive of the STS header. For more info, read at https://hstspreload.org/. I followed the RFC specification and didn't enable this by default according to their recommendation.

@consideRatio
Copy link
Member Author

I think this is ready for merge now!

@jcrist as a developer of the dask-gateway helm chart, I want to suggest usage of this pattern to configure your executables. Helm has an issue, and this pattern avoids it to a large degree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants