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

Wrong HTTPS redirect when also using AddPrefix #2024

Closed
schoren opened this issue Aug 29, 2017 · 21 comments
Closed

Wrong HTTPS redirect when also using AddPrefix #2024

schoren opened this issue Aug 29, 2017 · 21 comments
Labels
area/provider/k8s/ingress kind/bug/possible a possible bug that needs analysis before it is confirmed or fixed. status/5-frozen-due-to-age

Comments

@schoren
Copy link

schoren commented Aug 29, 2017

Do you want to request a feature or report a bug?

Bug

What did you do?

I'm using traefik as a kubernetes ingress controller. I have configured HTTPS redirect as follows in the TOML config file:

[entryPoints]
  [entryPoints.http]
  address = ":80"
  [entryPoints.http.redirect]
    regex = "^http://(.*)"
    replacement = "https://$1"
  [entryPoints.https]
  address = ":443"
  [entryPoints.https.tls]

I have also configured an ingress like this:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: test
  annotations:
    kubernetes.io/tls-acme: "true"
    kubernetes.io/ingress.class: "traefik"
    traefik.frontend.rule.type: "AddPrefix"
spec:
  tls:
  - hosts:
    - demo.example.com
    secretName: tls
  rules:
  - host: demo.example.com
    http:
      paths:
      - path: /path
        backend:
          serviceName: myservice
          servicePort: 80

What did you expect to see?

When accessing http://demo.example.com, I wanted it to redirect to https://demo.example.com.

What did you see instead?

When accessing http://demo.example.com, I wanted it to redirect to https://demo.example.com/path.

Output of traefik version: (What version of Traefik are you using?)

Version:      v1.3.7
Codename:     raclette
Go version:   go1.8.3
Built:        2017-08-25_08:56:06PM
OS/Arch:      linux/amd64

What is your environment & configuration (arguments, toml, provider, platform, ...)?

defaultEntryPoints = ["http","https"]
[entryPoints]
  [entryPoints.http]
  address = ":80"
  [entryPoints.http.redirect]
    regex = "^http://(.*)"
    replacement = "https://$1"
  [entryPoints.https]
  address = ":443"
  [entryPoints.https.tls]
[acme]
email = "certmaster@example.com"
storage = "/acme/acme.json"
entryPoint = "https"
onDemand = false
OnHostRule = true
acmelogging = true
caServer = "https://acme-v01.api.letsencrypt.org/directory"
acmeLogging = true
@dtomcej
Copy link
Contributor

dtomcej commented Aug 29, 2017

Hello @schoren

In your report, you have 2 conflicting statements:

What did you expect to see?

When accessing http://demo.example.com, I wanted it to redirect to https://demo.example.com.

What did you see instead?

When accessing http://demo.example.com, I wanted it to redirect to https://demo.example.com/path.

Can you please confirm which was the intended, and which was the actual?

@schoren
Copy link
Author

schoren commented Aug 29, 2017

I'm really sorry, this is the right report:

What did you expect to see?

When accessing http://demo.example.com, I wanted it to redirect to https://demo.example.com.

What did you see instead?

When accessing http://demo.example.com, I got redirected to https://demo.example.com/path.

@ldez
Copy link
Member

ldez commented Aug 29, 2017

May related to #1957

@ldez ldez added kind/bug/possible a possible bug that needs analysis before it is confirmed or fixed. and removed status/0-needs-triage labels Aug 29, 2017
@schoren
Copy link
Author

schoren commented Aug 29, 2017

Yes, I have already saw that. I have some experience in Go and would like to contribute to this project. Could you point me to a good starting point to investigate this?

@ldez
Copy link
Member

ldez commented Aug 29, 2017

First read https://github.com/containous/traefik/blob/master/CONTRIBUTING.md

All the code of the K8s provider is here: https://github.com/containous/traefik/tree/master/provider/kubernetes

@kachkaev
Copy link
Contributor

kachkaev commented Sep 4, 2017

Hi @schoren! Did you have a chance to investigate? What could be causing this behaviour? Totally agree with @ldez that this is likely to be related to #1957.

@schoren
Copy link
Author

schoren commented Sep 4, 2017

Hi @ldez, I have been looking at the code but so far couldn't find the problem. Will update here if I can make any progress

@schoren
Copy link
Author

schoren commented Sep 4, 2017

BTW, what's the best way to test or debug the code? I thin using a minikube could work, but it would require to rebuild the image and reload the minikube for each change.

Do you guys have a method for this?

@ldez
Copy link
Member

ldez commented Sep 4, 2017

@schoren you can simply rebuild the binary: make binary

perhaps @timoreimann have a better method.

@timoreimann
Copy link
Contributor

@schoren the fastest feedback cycles can be achieved if you can reproduce the problem on something simpler than Kubernetes, like the file provider.

If Kubernetes is needed, the second best thing you can do is to configure minikube to use your host Docker daemon. That way, you only need to rebuild the Docker image and kill your Traefik pods for changes to become effective.

What I have never tried myself but should be possible theoretically is to launch a development container into Kubernetes, kubectl exec into it, and develop / build from there. It seems more like a hack though.

Finally, there's Microsoft's Draft tool which promises to streamline the Kubernetes development process as well. I yet have to give it a whirl too, but what I have read and seen looks promising.

Hope this helps.

@schoren
Copy link
Author

schoren commented Sep 5, 2017

@timoreimann thank you, I was thinking that exactly. I'll do some debuggin tonight to see if the same issue happens without kubernetes.

@schoren
Copy link
Author

schoren commented Sep 6, 2017

Quick update. I have been able to reproduce the problem with a basic file provider. The bug seems to be that the Rewrite middleware is being applied after the AddPath middleware.
I'm trying to switch the order and see if that works (and doesn't break anything).

Any ideas are welcome :)

@kachkaev
Copy link
Contributor

@schoren have you discovered anything else since your last comment? Just curious.

@fabiorauber @Arcticdolphin you mentioned that you also faced the same issue in #1738 and #1957. If you know Go at least a bit, could you please have a look at what could be causing this? I have no experience in this language and despite that I tried browsing through the sources, I could not dig out anything that could help solving this 😞

Permanent http to https redirects are pretty common these days and keeping different containers behind different paths becomes popular too. So looks like the bug might affect quite a few users.

@schoren
Copy link
Author

schoren commented Sep 14, 2017

@kachkaev I have been doing some tests with the load order of mddlewares. The problem here is that the Route middlewares (such as addPrefix, stripPath, etc) are being executed before the entrypoint middlewares. I tried inverting the order in which they are loaded, but it seemed to have no effect.

The issues you mention are very likely to be related to this.

Still working, any help from someone more experienced with Negroni and Traefik would be great

@schoren
Copy link
Author

schoren commented Sep 15, 2017

I have finally understood the problem. Traefik has two kinds of middlewares: 1) entryPoint middlewares, and 2) frontend middlewares.
EntryPoint middlewares are implemented as Negroni middlewares, while frontend middlewares are implemented as http.Handler interface that wrap around one after another, and finally wrap the Negroni handler. This can be seen in the server.wireFrontendBackend

The effect of this is that, in spite of the load order of the negroni middlewares, frontend middlewares are always executed before. Thus, the URL that the middleware.Rewrite receives has already been modified.

I did a test to create a temporary handler that suppports order of execution (having pre and post handlers), but the result is that the Request never gets forwarded to the actual backend. I am not sure if this is the expected behavior.

IMHO, the most intuitive behavior would be to have the entryPoint middlewares executed first, and have the frontend middlewares modifiy the internal forwarded route, without the client noticing it.

I think there are two ways of fixing this issue:

  1. We could have the rewrite handler reverse ingeneering any rewrites made on the Request URL, so it acts on the original requested url. This needs the least code refactoring, athought it doesn't feel like a good solution

  2. We could rewrite the frontend middlewares in order to act as regular Negroni middlewares, and then comply with the loading order logic. I'm not really sure if this is possible because I don't fully understand how the requests are forwarded to the backend.

@ldez do you have any input about this?

@fore5fire
Copy link

Any update on this? I'm still experiencing this issue on v1.5.0, and I can't figure out a good work around.

@kachkaev
Copy link
Contributor

kachkaev commented Jan 25, 2018

@LSmith130 check out a discussion in #1957 – these issues seem to be related. I'm also hoping that the problem is fixed soon.

@schoren
Copy link
Author

schoren commented Jan 26, 2018

Yes, #1957 is totally related. The problem is in the execution order of the middlewares. However, it will require some thinking to fix this without introducing regressions and/or new bugs.

@dtomcej
Copy link
Contributor

dtomcej commented Jul 16, 2018

Addprefix is designed to add to the backend request, not to build a custom redirect.

We now have full redirect capabilities, and modifier/matcher capabilities in 1.7.

We have rewritten the matchers and modifiers to allow this to work properly.

I will go ahead and close this, as it may confuse other issues moving forward.

@dtomcej
Copy link
Contributor

dtomcej commented Jul 20, 2018

Re-opening this issue, because even with a proper rule + modifier, the redirect is still incorrect, #3631 should resolve.

@dtomcej
Copy link
Contributor

dtomcej commented Oct 25, 2018

Closing this issue, as #3742 should have resolved any outstanding issues. Any further issues will need to be noted in a new ticket.

Thanks!

@dtomcej dtomcej closed this as completed Oct 25, 2018
@traefik traefik locked and limited conversation to collaborators Sep 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/provider/k8s/ingress kind/bug/possible a possible bug that needs analysis before it is confirmed or fixed. status/5-frozen-due-to-age
Projects
None yet
Development

No branches or pull requests

7 participants