Route.Host -matching will ignore any provided port from getHost(), if… by cognusion · Pull Request #447 · gorilla/mux

cognusion · 2019-02-13T00:26:02Z

… a port isn't specified in the template

In lieu of checking the template pattern on every Match request, a bool is added to the routeRegexp, and set if the routeRegexp is a host AND there is no ":" in the template. I dislike extending the type, but I'd dislike doing a string match on every single Match, even more.

All existing mux tests pass. Additionally my application-level (Convey) positive and negative tests pass. Feel free to comment: You know your code better than I do.

=== RUN   TestPathHostPortUnset

  When a request is made, with a host and port specified, the correct match is found (no port specified) ✔✔✔✔✔


16 total assertions

--- PASS: TestPathHostPortUnset (0.00s)
=== RUN   TestPathHostPortSet

  When a request is made, with a host and port specified, and the port does match, a match is found ✔✔✔✔✔


21 total assertions

--- PASS: TestPathHostPortSet (0.00s)
=== RUN   TestPathHostPortSetNope

  When a request is made, with a host and port specified, and the port does not match, a match is NOT found ✔✔✔


24 total assertions

--- PASS: TestPathHostPortSetNope (0.00s)

… a port isn't specified in the template In lieu of checking the template pattern on every Match request, a bool is added to the routeRegexp, and set if the routeRegexp is a host AND there is no ":" in the template. I dislike extending the type, but I'd dislike doing a string match on every single Match, even more.

yogo1212 · 2019-03-12T11:23:08Z

works for me

stale · 2019-05-11T12:18:48Z

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

elithrar · 2019-05-11T19:58:37Z

Sorry, I missed this: can you show application code before/after that explains this better?

I understand what the change does, but not why.

yogo1212 · 2019-05-12T15:22:31Z

@elithrar
Without this change, r.Host("test.host.com") won't match requests with the Host: test.host.com:1234.
In order to make that case work, this PR assumes that, unless a port specified (e.g. r.Host("test.host.com:1234")), any port is allowed to match.

cognusion · 2019-05-16T18:07:26Z

@elithrar ,

As @yogo1212 properly described, a previous PR #383 horrifically broke a good swath of the Gorilla-using Internet by deciding that if you didn't specify a port in the route, you can't specify a port on the Request and have it match. This PR allows that PR to exist in-spirit, without the awful requirement of specifying ports for every route. Without either reverting #383 or implementing this patch, if your route is r.Host("a.b.c") then http://a.b.c:80/ won't work but http://a.b.c/ will. With this patch, unless you specify the port on the route, it assumes you don't give a flying leap what port the request tacks on.

elithrar · 2019-05-16T18:57:04Z

Understood - #383 broke more than we anticipated (users should pin + test deps, but we also need to do a better job of checking impact).

Reviewing the PR today and can (hopefully) merge + tag a new version tonight.

elithrar · 2019-05-17T00:30:08Z

@cognusion See https://github.com/gorilla/mux/releases/tag/v1.7.2

yogo1212 · 2019-05-17T11:21:01Z

@elithrar thank you for the notice <3

full diff: gorilla/mux@v1.7.0...v1.7.2 includes: - gorilla/mux#457 adding Router.Name to create new Route - gorilla/mux#447 host:port matching does not require a :port to be specified Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: gorilla/mux@v1.7.0...v1.7.2 includes: - gorilla/mux#457 adding Router.Name to create new Route - gorilla/mux#447 host:port matching does not require a :port to be specified Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 25b451e01b8a1b44f45a67cb4731ff61d7b1ebc1 Component: engine

full diff: gorilla/mux@v1.7.0...v1.7.2 includes: - gorilla/mux#457 adding Router.Name to create new Route - gorilla/mux#447 host:port matching does not require a :port to be specified Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 25b451e) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: gorilla/mux@v1.7.0...v1.7.2 includes: - gorilla/mux#457 adding Router.Name to create new Route - gorilla/mux#447 host:port matching does not require a :port to be specified Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 81e3457c23630de470c42d909e79dfb159833f7a Component: cli

full diff: gorilla/mux@v1.7.0...v1.7.2 includes: - gorilla/mux#457 adding Router.Name to create new Route - gorilla/mux#447 host:port matching does not require a :port to be specified Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: gorilla/mux@v1.7.0...v1.7.2 includes: - gorilla/mux#457 adding Router.Name to create new Route - gorilla/mux#447 host:port matching does not require a :port to be specified Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 25b451e) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: gorilla/mux@v1.7.0...v1.7.2 includes: - gorilla/mux#457 adding Router.Name to create new Route - gorilla/mux#447 host:port matching does not require a :port to be specified Signed-off-by: Sebastiaan van Stijn <github@gone.nl> (cherry picked from commit 25b451e01b8a1b44f45a67cb4731ff61d7b1ebc1) Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 486953e2ff065883897746317c318b0bb8d3406d Component: engine

full diff: gorilla/mux@v1.7.0...v1.7.2 includes: - gorilla/mux#457 adding Router.Name to create new Route - gorilla/mux#447 host:port matching does not require a :port to be specified Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: zach <Zachary.Joyner@linux.com>

harshavardhana

This PR actually caused a regression one more change needed is missing in this PR

git diff
diff --git a/regexp.go b/regexp.go
index 96dd94a..0144842 100644
--- a/regexp.go
+++ b/regexp.go
@@ -325,6 +325,12 @@ func (v routeRegexpGroup) setMatch(req *http.Request, m *RouteMatch, r *Route) {
        // Store host variables.
        if v.host != nil {
                host := getHost(req)
+               if v.host.wildcardHostPort {
+                       // Don't be strict on the port match
+                       if i := strings.Index(host, ":"); i != -1 {
+                               host = host[:i]
+                       }
+               }
                matches := v.host.regexp.FindStringSubmatchIndex(host)
                if len(matches) > 0 {
                        extractVars(host, matches, v.host.varsN, m.Vars)

Continuing from PR gorilla#447 we have to add extra check to ignore the port as well

Continuing from PR gorilla#447 we have to add extra check to ignore the port as well add tests to cover this case

Continuing from PR #447 we have to add extra check to ignore the port as well add tests to cover this case

cognusion mentioned this pull request Feb 13, 2019

fix #2, return full host:port info from getHost #383

Merged

stale bot added the stale label May 11, 2019

stale bot removed the stale label May 11, 2019

elithrar self-assigned this May 16, 2019

elithrar added enhancement minor-rev-change labels May 16, 2019

elithrar merged commit ed099d4 into gorilla:master May 17, 2019

thaJeztah mentioned this pull request Jun 20, 2019

bump gorilla/mux v1.7.2 docker/cli#1958

Merged

thaJeztah mentioned this pull request Jun 20, 2019

bump gorilla/mux v1.7.2 moby/moby#39381

Merged

thaJeztah mentioned this pull request Jun 26, 2019

[19.03 backport] bump gorilla/mux v1.7.2 docker-archive/engine#289

Merged

harshavardhana reviewed Jul 10, 2020

View reviewed changes

harshavardhana mentioned this pull request Jul 10, 2020

[resolved] Virtual-host style is broken with latest release minio/minio#10007

Closed

harshavardhana added a commit to harshavardhana/mux that referenced this pull request Jul 10, 2020

fix: regression in vars extract for wildcard host

d2eaf19

Continuing from PR gorilla#447 we have to add extra check to ignore the port as well

harshavardhana mentioned this pull request Jul 10, 2020

fix: regression in vars extract for wildcard host #579

Merged

harshavardhana added a commit to harshavardhana/mux that referenced this pull request Jul 10, 2020

fix: regression in vars extract for wildcard host

8769a15

Continuing from PR gorilla#447 we have to add extra check to ignore the port as well add tests to cover this case

elithrar pushed a commit that referenced this pull request Jul 11, 2020

fix: regression in vars extract for wildcard host (#579)

98cb6bf

Continuing from PR #447 we have to add extra check to ignore the port as well add tests to cover this case

chrislusf mentioned this pull request Jul 28, 2020

请问S3网关如何绑定域名而URL中不含bucket路径 seaweedfs/seaweedfs#1405

Closed

dependabot bot mentioned this pull request Mar 11, 2021

Bump github.com/gorilla/mux from 1.7.0 to 1.8.0 awslabs/amazon-ecs-local-container-endpoints#55

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Route.Host -matching will ignore any provided port from getHost(), if…#447

Route.Host -matching will ignore any provided port from getHost(), if…#447
elithrar merged 1 commit intogorilla:masterfrom
cognusion:feature-hostportwildcard

cognusion commented Feb 13, 2019

Uh oh!

yogo1212 commented Mar 12, 2019

Uh oh!

stale bot commented May 11, 2019

Uh oh!

elithrar commented May 11, 2019

Uh oh!

yogo1212 commented May 12, 2019

Uh oh!

cognusion commented May 16, 2019 •

edited

Loading

Uh oh!

elithrar commented May 16, 2019

Uh oh!

elithrar commented May 17, 2019

Uh oh!

yogo1212 commented May 17, 2019

Uh oh!

harshavardhana left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cognusion commented Feb 13, 2019

Uh oh!

yogo1212 commented Mar 12, 2019

Uh oh!

stale bot commented May 11, 2019

Uh oh!

elithrar commented May 11, 2019

Uh oh!

yogo1212 commented May 12, 2019

Uh oh!

cognusion commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elithrar commented May 16, 2019

Uh oh!

elithrar commented May 17, 2019

Uh oh!

yogo1212 commented May 17, 2019

Uh oh!

harshavardhana left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cognusion commented May 16, 2019 •

edited

Loading