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

added a dynamic file provider and config options (traefik_qs_exposedbydefault, traefik_qs_tls_options, traefik_qs_middlewares) #5

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

foxcris
Copy link

@foxcris foxcris commented Jan 6, 2021

  • added config option traefik_qs_exposedbydefault: If set to false,services that don't have a traefik.enable=true label will be ignored from the resulting routing configuration.
  • added config option traefik_qs_tls_options: If set to true, three different setups for tls options (modern, intermediate, old) are created according to https://ssl-config.mozilla.org/#server=traefik.
  • added config option traefik_qs_middlewares: If set to true, setup default middleware config for hsts-header, xssfilter-header
  • added a dynamic file provider:
    A dynamic file provider is setup by default. It watches the directory dynamic_conf which is placed in the data directory for traefik (controlled by the variable traefik_dir). You can simply add your own config here. By setting traefik_qs_tls_options and/or traefik_qs_middlewares to true config files are autogenerated qs_traefik_tls_options.yml and/or qs_traefik_middlewares.yml.

… services that don't have a traefik.enable=true label will be ignored from the resulting routing configuration.

- added config option `traefik_qs_tls_options`: If set to true, three different setups for tls options (modern, intermediate, old) are created  according to https://ssl-config.mozilla.org/#server=traefik.
- added config option `traefik_qs_middlewares`: If set to true, setup default middleware config for hsts-header, xssfilter-header
- added a dynamic file provider:
A dynamic file provider is setup by default. It watches the directory `dynamic_conf` which is placed in the data directory for traefik (controlled by the variable `traefik_dir`). You can simply add your own config here. By setting `traefik_qs_tls_options` and/or `traefik_qs_middlewares` to true config files are autogenerated `qs_traefik_tls_options.yml` and/or `qs_traefik_middlewares.yml`.
@foxcris
Copy link
Author

foxcris commented Jan 10, 2021

Can you give an advice why the travis checks are failing? Everything is working on my side.

@foxcris
Copy link
Author

foxcris commented Feb 15, 2021

What are the next steps to get this pull request included on your side?

@mleutenegger
Copy link
Member

Hi, sry for my late comment, this somehow went under my radar. I will have a look at this and give feedback asap

- added ipv6 switch
@foxcris
Copy link
Author

foxcris commented Mar 1, 2021

No problem. In the mean time also added support for ipv6.

- name: 'setup : dynamic file config'
become: true
copy:
dest: '{{ traefik_dir }}/traefik_dynamic_file_config.yml'

Choose a reason for hiding this comment

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

should this be in the dynamic_conf directory?

@mleutenegger mleutenegger self-assigned this Mar 25, 2021
@mleutenegger mleutenegger added the enhancement New feature or request label Mar 25, 2021
@mleutenegger
Copy link
Member

Phew, I finally came around having a good look at it.

First off, thanks for the work and the PR 🥳 !

Second, the molecule tests seem to be failing because of a missing dependency, I have updated this in another PR (#6) which you can sync once its merged into master. It may also completely abolish travis in favour of github actions.

And lastly, about the PR itself:
Good work on defining the additional _qs_ variables. Especially a valid default TLS config is beneficial. Also the Idea of integrating a dynamic file is not a bad idea either. I have however a two things to note about the implementation before we can proceed with merging.

1. Dynamic file provider is somewhat redundant when opt-out and traefik_dynamic_file_config is redundant
While I do think that a dynamic file provider option makes sense, I would only see it as an opt-in feature. This role primarily sets up a traefik container on a host and configures it to use docker as provider. Therefore adding in more dynamic providers adds more sources of errors. I can however see usecases that rely on this provider, for example if you want to dynamically change settings depending on some external source during runtime. The other thing is, that traefik_dynamic_file_config is redundant, as you can already set any config in the static config. Additionally, a dynamic config file set by ansible would not be any different from a static config entry, both would only be updated at the playbook run.

2. TLS and middleware options should use the static config
The Iidea behind a simple setup of some hardening is great. But I think it should follow the overall structure of the role by injecting them into the static config in the config step, like the https config:

- name: "config : generate https entrypoint config"
set_fact:
traefik_int_conf_entryPoints: "{{
traefik_int_conf_entryPoints |
combine(traefik_int_conf_entryPoints_https)
}}"
when: traefik_qs_https
- name: "config : generate https redirect config"
set_fact:
traefik_int_conf_entryPoints: "{{ traefik_int_conf_entryPoints |
combine(traefik_int_conf_entryPoints_https_redirect, recursive=True) }}"
when: traefik_qs_https and traefik_qs_https_redirect

This has the main benefit, that the config can be overwritten by a hoastvar if needed, and there is no cleanup of a stale file to be done if the _qs_ option is set to false again.

So to summarize what I think is necessary for merging:
👉 make the dynamic file provider opt-in and remove traefik_dynamic_file_config
👉 inject the middleware and tls config into the static config if _qs_ variable is set.

Again, thanks for the PR and effort!

@foxcris
Copy link
Author

foxcris commented Apr 5, 2021

Hi, thanks for your comments. I think you are right with your remarks and i will rework the PR to use the static config in favour of the dynamic config.

@foxcris
Copy link
Author

foxcris commented Apr 5, 2021

I tried to adapt my setup but i get problems using the defined tls and middleware setups from the static config. Till now i am assigning them using <name>@file format. @file is now not valid anymore as this can only be used with configuration provided by the dynamic file provider. How do i address the tls and middleware if i defined them in the static config? I could not find anything regarding this issue.

@foxcris
Copy link
Author

foxcris commented Apr 5, 2021

Accoding to https://doc.traefik.io/traefik/getting-started/configuration-overview/#the-dynamic-configuration
everything except

  • entrypoints
  • providers
  • resolvers (i could not make them work with dynamic config)
    should be setup with a dynamic configuration. Or am i missunderstanding something?

@foxcris
Copy link
Author

foxcris commented Apr 5, 2021

I just stated the dashboard and all tls and middleware configurations from the static config are not shown. Thus i cannot reference them.

…nfkey_tls

- removed unused dynamic fileconfig file (a directory is used)
@mleutenegger
Copy link
Member

Just had a look at the Traefik docs to make sure I am (still) understanding everything correctly.

According to the docs, middlewares defined in the static entrypoint definition are applied to each router associated with the entrypoint. Have you tried to do something like this:
Variables:

# in config/main.yml
traefik_int_conf_http_middlewares:
    https:
      http:
        middlewares:
          hsts-header:
            headers:
              STSSeconds: 63072000
          xssfilter-header:
            headers:
              browserXssFilter: true

and then merge:

# tasks/0_config.yml
- name: "config : generate https xss & hsts"
  set_fact:
      traefik_int_conf_entryPoints: "{{ traefik_int_conf_entryPoints |
        combine(traefik_int_conf_http_middlewares, recursive=True) }}"
  when: traefik_qs_https and traefik_qs_middlewares

If you want to add them to the http entrypoint (:80), you would need to make another task which merges that one specifically

@foxcris
Copy link
Author

foxcris commented Apr 6, 2021

With dynamic file config i had the middlewares defined independend from the entrypoint. I then used docker lables to assign them to the required router.
The way you refering to might work (i haven't tried till now) however i don't think its a good idea to assign such middleware and tls options to all entrypoints. There is no way for the user to disbale the assignment for specific routers. Especially the xss and hsts middlewares are mostly not required for all routers.

So i propose to stay with defining a dynamic file config per default and using it for such middleware configuration.
Above you said that the dynamic file config would make treafik more errore prune but i do not think so. This will only happen to users which want to extend the configuration with self made config parts. An then they now what they are doing.

@mleutenegger
Copy link
Member

So what you are proposing would be to remove the quick-setup for tls, hsts and xss and just go with a dynamic config option, handing configuration over to the user? I'd be on board with that, as it removes the necessity to maintain a good tls config in this role.

Concerning the "making traefik more error prone" part, I think I might not have stated my intentions clear enough. Adding more dynamic providers would not make traefik more error prone, as it is very mutch designed to be used with multiple dynamic config sources. What I was referring to was, that adding a dynamic file provider which is then used by the role as a default way of configuring traefik might introduce maintenance problems later down the line when updating the role or changing config around. Something like this would require much more effort (e.g. tracking and removing config files) to keep the role idempotent and remove any stale config. This is why I very much agree with the point you make:

This will only happen to users which want to extend the configuration with self made config parts. An then they now what they are doing.

With that in mind, I would argue that the configuration of hsts, xss and tls is very much in the responsibility of the role user. The responsibility of this role is to give its users the ability to implement these things according to their needs. This would mean, that the role should provide:

  • A switch to enable/disable the interpretation of dynamic config (i.e. `traefik_use_dynamic_file_config)
  • A variable to define the source directory (i.e. traefik_dynamic_file_config_dir)
  • Some documentation on how to configure custom dynamic config with examples for hsts, xss and tls

This should suffice to achieve any specific middleware config by either rendering it on-the-fly on the server or by using a simple file or template task in a playbook, rendering a client specific file to the traefik_dynamic_file_config_dir.
Do you think that would be OK with your usecase?

Again to be clear: I do like the Idea of adding dynamic file config, I just need to make sure that the role can be updated cleanly and doues not introduce problems in the future.

@foxcris
Copy link
Author

foxcris commented Apr 6, 2021

I think your summary:


    A switch to enable/disable the interpretation of dynamic config (i.e. `traefik_use_dynamic_file_config)
    A variable to define the source directory (i.e. traefik_dynamic_file_config_dir)
    Some documentation on how to configure custom dynamic config with examples for hsts, xss and tls

is correct.
But i would extend it:

  • add a way for the user to provide custom dynamic config parts which are then automatically setup/installed by the role

@mleutenegger
Copy link
Member

Agreed, that would be beneficial, too. So it would then be:

  • A switch to enable/disable the interpretation of dynamic config (i.e. `traefik_use_dynamic_file_config)
  • A variable to define the source directory (i.e. traefik_dynamic_file_config_dir)
  • A variable which will produce a file when traefik_use_dynamic_file_config: true which is always named the same containing the yaml config (_ansible_conf.yml and traefik_dynamic_file_config)
  • Some documentation on how to configure custom dynamic config with examples for hsts, xss and tls

Does that sound ok and doable?

@foxcris
Copy link
Author

foxcris commented Apr 6, 2021

Yes, thats sounds good. I will report back as soon as i am ready :-).

…mic_file_config

- introduced switch traefik_use_dynamic_file_config to use dynamic file configuration provider
- added example playbook to setup traefik
- added variable to configure directory to use with dynamic file provider
@foxcris
Copy link
Author

foxcris commented Feb 20, 2022

Sorry for the delay. I have just commited the discussed changes.

Looking forward for your review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants