canonical / traefik-route-k8s-operator

Configuration charm for traefik-k8s.
https://charmhub.io/traefik-route-k8s
Apache License 2.0
0 stars 3 forks source link

Prefix strip middleware #6

Closed PietroPasotti closed 1 year ago

PietroPasotti commented 2 years ago

Added an optional config option to strip a PathPrefix prefix from the urls Traefik routes to.

PietroPasotti commented 2 years ago

How do we validate this new option in combination with the other ones? If you configure traefik-route with

root_url=http://foo.bar/baz/
strip_prefix=baz

should we forward the units the root_url with or without baz? should we raise an error? should we just forward the root_url as-is?

@mmanciop, thoughts?

wolsen commented 2 years ago

One thing that I have trouble with is the strip_prefix option and specifying the actual prefix to strip. The charm that requires the ingress relation to traefik (or traefik route) doesn't actually have intimate knowledge regarding what the prefix is, as this depends on a) the mode that the traefik charm is configured for and b) knowing the inner workings of how the traefik charm has actually configured the URLs.

wolsen commented 2 years ago

One thing that I have trouble with is the strip_prefix option and specifying the actual prefix to strip. The charm that requires the ingress relation to traefik (or traefik route) doesn't actually have intimate knowledge regarding what the prefix is, as this depends on a) the mode that the traefik charm is configured for and b) knowing the inner workings of how the traefik charm has actually configured the URLs.

This is actually available to the requiring/consuming charm as part of the relation data that comes back; though it has to parse the URL and determine how to handle this.

PietroPasotti commented 2 years ago

@wolsen so in conclusion, strip_prefix should be a boolean option, and traefik should know what it means to strip the prefix in both routing modes.

Suppose strip_prefix=True; then: In path routing mode the url is

unit_url = f"http://{host}:{self._port}/{prefix}"

you'd expect to see instead: f"http://{host}:{self._port}" instead (with or without trailing "/")

In subdomain routing mode the url is

unit_url = f"http://{prefix}.{host}:{self._port}/"

so you'd expect to see instead: f"http://{host}:{self._port}/" (with or without "http://"?)

correct?

wolsen commented 2 years ago

@wolsen so in conclusion, strip_prefix should be a boolean option, and traefik should know what it means to strip the prefix in both routing modes.

Suppose strip_prefix=True; then: In path routing mode the url is

unit_url = f"http://{host}:{self._port}/{prefix}"

you'd expect to see instead: f"http://{host}:{self._port}" instead (with or without trailing "/")

In subdomain routing mode the url is

unit_url = f"http://{prefix}.{host}:{self._port}/"

so you'd expect to see instead: f"http://{host}:{self._port}/" (with or without "http://"?)

correct?

Only the path_routing mode should be affected as the web-root (or path-prefix in traefik terms) only changes in this circumstance. I believe the default is wrong because the current default requires that the web-root on the backend service be changed in order to successfully service the request.

PietroPasotti commented 2 years ago

Summing up a conversation on MM:

test should pass:

def test_strip_prefix(strip_prefix: bool):
    harness = Harness(TraefikCharm)

    relation: ops.model.Relation = mock_setup_ipu(harness)
    mock_requirer_side(harness, relation, port=80, strip_prefix=strip_prefix)

    provider.publish_url(relation, "remote/0", "http://url.com/")

    assert (
        ingressed_charm.name in harness.charm._get_config_value_from_file("http.middlewares.stripprefix.stripPrefix.prefixes")
    ) == strip_prefix

New [IPA/IPU]Requirer signature has extra kwarg strip_prefix:bool=True Provider will assume strip_prefix=False if not explicitly passed by requirer (backwards comp).

simskij commented 1 year ago

This functionality has already been merged in a separate PR.