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

[traefik-route] middleware in router tls is removed #335

Open
hemanthnakkina opened this issue Apr 25, 2024 · 9 comments
Open

[traefik-route] middleware in router tls is removed #335

hemanthnakkina opened this issue Apr 25, 2024 · 9 comments

Comments

@hemanthnakkina
Copy link

Bug Description

The charm integrates with traefik via traefik-route interface.
And the charm updates relation data with the dynamic traefik configuration that includes middlewares.
The middlewares added in router non-tls are retained in the file /opt/traefik/juju/ where as the middlewares specified in router tls section are removed.

This is easily reproducible by unit tests. Patch file with unit test attached

To Reproduce

  1. Clone the latest traefik-k8s-operator code
  2. Apply patch file attached test-traefik-route-middleware-patch.txt
  3. Run unit test cases tox -e unit

Environment

Juju 3.4 + microk8s v1.28.7
traefik charm : 1.0/stable 164

However this is reproducible on latest charm as well.

Relevant log output

Unit test assert failure output:

  Differing items:
  {'http': {'middlewares': {'custom-headers-http': {'headers': {'customRequestHeaders': {'X-Forwarded-Proto': 'http'}}},...foo-service', 'tls': {'domains': [{...}]}}}, 'services': {'juju-foo-service': {'loadBalancer': {'servers': [{...}]}}}}} != {'http': {'middlewares': {'custom-headers-http': {'headers': {'customRequestHeaders': {'X-Forwarded-Proto': 'http'}}},...th`)', 'service': 'juju-foo-service', ...}}, 'services': {'juju-foo-service': {'loadBalancer': {'servers': [{...}]}}}}}
  
  Full diff:
    {
        'http': {
            'middlewares': {
                'custom-headers-http': {
                    'headers': {
                        'customRequestHeaders': {
                            'X-Forwarded-Proto': 'http',
                        },
                    },
                },
                'custom-headers-https': {
                    'headers': {
                        'customRequestHeaders': {
                            'X-Forwarded-Proto': 'https',
                        },
                    },
                },
            },
            'routers': {
                'juju-foo-router': {
                    'entryPoints': [
                        'web',
                    ],
                    'middlewares': [
                        'custom-headers-http',
                    ],
                    'rule': 'PathPrefix(`/path`)',
                    'service': 'juju-foo-service',
                },
                'juju-foo-router-tls': {
                    'entryPoints': [
                        'websecure',
                    ],
  -                 'middlewares': [
  -                     'custom-headers-https',
  -                 ],
                    'rule': 'PathPrefix(`/path`)',
                    'service': 'juju-foo-service',
                    'tls': {
                        'domains': [
                            {
                                'main': 'testhostname',
                                'sans': [
                                    '*.testhostname',
                                ],
                            },
                        ],
                    },
                },
            },
            'services': {
                'juju-foo-service': {
                    'loadBalancer': {
                        'servers': [
                            {
                                'url': 'http://foo.testmodel-endpoints.local:8080',
                            },
                        ],
                    },
                },
            },
        },
    }

Additional context

test-traefik-route-middleware-patch.txt

@PietroPasotti
Copy link
Contributor

So this happens because traefik charm generates a tls config for the route and overwrites the one you passed.

A fix could be: if the user provided a tls config, respect that instead of overwriting.
Only issue is: how can traefik know that a router definition is a tls one?

For now, I suggest you try a workaround: rename your tls router to a different prefix.
Traefik uses "{router_name}-tls" for the tls router, if you name your router "{router_name}-https" or something else, traefik will not overwrite it.
However, it will configure traefik with three routers, of which two for tls, and only one of them will have the middlewares you passed, and I'm not sure that will work.

@PietroPasotti
Copy link
Contributor

a somewhat more sustainable solution could be to extend the route interface with a tls-middlewares arg so that traefik knows what middlewares to inject in the tls definition.
Which still means you leave to traefik the task of configuring a tls route for your entrypoint, which I'm not sure it's a good practice either. Perhaps we should say: if you're using traefik route, you know what you're doing and we're not going to touch your config. Why are we injecting tls stuff in it?

@simskij or @sed-i do you have an opinion here?

@hemanthnakkina
Copy link
Author

There is another problem with traefik_route interface itself.

The related charm is unaware of the traefik configurations but constructing traefik config based on assumptions. I believe the interface itself needs to pass the relevant traefik configuration to the charm via the interface if it expects the charm to provide complete configuration

@PietroPasotti
Copy link
Contributor

that's not scalable though, you incur precedence issues if you have multiple charms using traefik-route
I think the interface is fine: a requirer charm gives traefik a valid dynamic config file.
The question is: should traefik patch that file or take it as-is?

@hemanthnakkina
Copy link
Author

@PietroPasotti It works with the workaround you mentioned if i set a priority for "{router_name}-https"

@sed-i
Copy link
Contributor

sed-i commented Apr 25, 2024

  • In traefik, would we be able to easily include user middlewares, @PietroPasotti?
  • @hemanthnakkina, what are you using traefik route for? The only use case I am aware of is "ingress-to-leader" for grafana, which we want to replace with a proper charm lib in the near future. According to charmhub, there are no other uses.

@hemanthnakkina
Copy link
Author

@sed-i I am not sure how the charmhub populates its list but its definitely missing some charms. I looked at ingress interface as well and it does not list any charms owned by OpenStack Charmers.

Couple of sunbeam charms (nova-k8s, heat-k8s) are using traefik route interface predominantly for 2 reasons:

  • Multiple ports per charm need to be exposed via traefik ( ingress interface allows only single app single port )
  • Use middleware in traefik configuration

Note: Sunbeam charms starts multiple apps as separate containers within a pod

@simskij
Copy link
Member

simskij commented Apr 26, 2024

We're seeing this in other places as well, even in our own charms, and I think we might need to start thinking about how to properly support multi-route or multi-port ingressing over the ingress interface. @gregory-schiano, do you have a similar concept in nginx?

@gregory-schiano
Copy link

Well we don't have multi-port usecase for now, but multi-route is one. The way I intend to solve the multi-route is by using multiple ingress relation on the requirer charm (one per route) and the custom-url-routing we've discussed here canonical/charm-relation-interfaces#79 and in Riga
Note that multi-port might resolved using multiple ingress relation with the requirer charm since the port attribute is already supported.

My overall though is I think summed up is the following comment canonical/charm-relation-interfaces#79 (comment)

@dstathis dstathis added Checked and removed Checked labels Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants