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

Extend https redirection tests, and fix incorrect behavior #3742

Merged
merged 4 commits into from Aug 14, 2018

Conversation

dtomcej
Copy link
Contributor

@dtomcej dtomcej commented Aug 7, 2018

What does this PR do?

Rebuilds https redirection paths after being modified by middlewares

Motivation

Fixes #3741

More

  • Add replacePath to context
  • Add tests
  • Add replacePathRegex to context
  • Add tests
  • Fix stripPathPrefix
  • Added/updated tests
  • Added/updated documentation - None needed, corrects intended behavior

@kachkaev
Copy link
Contributor

kachkaev commented Aug 7, 2018

Many thanks for putting this together @dtomcej! Does this PR also fix a double-trailing slash when the ingress path ends with a slash? #3741 (comment)

I looked through your changes, but could not find a new test case for this, so am wondering. Sorry in advance if that's a lame question – I don't know Go, so asking is the only option I have 😆

In any case, I'll be keen to test the result once the PR is merged! Is containous/traefik:experimental the right image to pull if I can't wait for the official 1.7.0 / 1.7.0-rc4?

@dtomcej
Copy link
Contributor Author

dtomcej commented Aug 7, 2018

@kachkaev Yessir. The trailing slash with a pathPrefixStrip rule is tested with this line:
(https://github.com/containous/traefik/pull/3742/files#diff-93727317485fd74c157e7a817d2e2186R33)

This means that in the new test suite, anything referring to example2.com is testing with a trailing slash on the pathPrefixStrip rule.

This PR increases the number of tests from ~15 to ~55 to improve coverage.

If you would like to test out the code right away, I have pushed the image to dtomcej/traefik:issue-3741.

@kachkaev
Copy link
Contributor

kachkaev commented Aug 7, 2018

I have just tried dtomcej/traefik:issue-3741 and can confirm that I no longer observe #3741 and #3741 (comment) 🎉

Many thanks for your work on this @dtomcej 🙌 💯

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

Few remarks ;)

@@ -751,114 +752,92 @@ func (s *HTTPSSuite) TestEntrypointHttpsRedirectAndPathModification(c *check.C)

testCases := []struct {
desc string
host string
host []string
Copy link
Member

Choose a reason for hiding this comment

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

Could you please rename host into hosts

},
}

for _, test := range testCases {
test := test
for _, subtest := range test.host {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please rename subtest into host

c.Assert(err, checker.IsNil)
defer resp.Body.Close()
location := resp.Header.Get("Location")
expected := strings.Replace(test.expectedURL, "<HOST>", subtest, 1)
Copy link
Member

Choose a reason for hiding this comment

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

You should maybe replace by

expected := fmt.Sprintf(test.expectedURL, host)

And replace all <HOST> occurrences in expectedURL by %s

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

👏

@@ -740,7 +741,7 @@ func (s *HTTPSSuite) TestEntrypointHttpsRedirectAndPathModification(c *check.C)
defer cmd.Process.Kill()

// wait for Traefik
err = try.GetRequest("http://127.0.0.1:8080/api/providers", 500*time.Millisecond, try.BodyContains("Host: example.com"))
err = try.GetRequest("http://127.0.0.1:8080/api/providers", 1000*time.Millisecond, try.BodyContains("Host: example.com"))

This comment was marked as outdated.

Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 👏

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.

None yet

7 participants