Feat/match port #5102
Feat/match port #5102
Conversation
javierguerragiraldez
force-pushed
the
feat/match-port
branch
4 times, most recently
from
9bdd5fb to
abab7f8
Compare
|
@javierguerragiraldez in the DB-less testsuite, the error happening is that with your PR the error message has changed from |
javierguerragiraldez
force-pushed
the
feat/match-port
branch
4 times, most recently
from
5e57658 to
9e1b511
Compare
|
got a clean test on the branch, but for some reason, it still fails on the (projected) merge, even if git agrees it's identical to the branch, now that it's fully squashed and rebased on needless to say, i couldn't get the test to fail locally, even when pulling the merge commit from github's hidden space. |
|
@javierguerragiraldez you can safely ignore that failure. That test should be marked as flaky, the error happens because the balancer hasn't been completely created when the test runs. |
javierguerragiraldez
force-pushed
the
feat/match-port
branch
from
9e1b511 to
1e456a4
Compare
|
I have started reviewing this change, will get back to it in a few hours, please hold the merge for now, thank you! |
javierguerragiraldez
force-pushed
the
feat/match-port
branch
6 times, most recently
from
949d7e6 to
5489a7f
Compare
javierguerragiraldez
force-pushed
the
feat/match-port
branch
from
5489a7f to
27baa6b
Compare
javierguerragiraldez
force-pushed
the
feat/match-port
branch
from
1ef2bc7 to
f052838
Compare
Allow routes to specify a port in the host parameter. If present, it matches only requests to that specific port. If absent, requests to any port matches. Requests without explicit port are handled as if they included the appropriate default port (80 for HTTP, 443 for HTTPS). Routes with port have higher priority than those that those without; even for requests without explicit port.
javierguerragiraldez
force-pushed
the
feat/match-port
branch
from
f052838 to
3fb3356
Compare
|
Please hold before merging, thanks! |
| assert.same(nil, match_t.matches.method) | ||
| assert.same(nil, match_t.matches.uri) | ||
| assert.same(nil, match_t.matches.uri_captures) | ||
| end) |
There was a problem hiding this comment.
This was covered in a few test case, but a bit too indirectly, and I'm afraid the test may be updated in the future and not cover the case, so here is an isolated test case specifically for the submatching priority.
| @@ -770,8 +774,7 @@ do | |||
| local matchers = { | |||
| [MATCH_RULES.HOST] = function(route_t, ctx) | |||
| local req_host = ctx.hits.host or ctx.req_host | |||
| local host = route_t.hosts[req_host] | |||
| or route_t.hosts[split_port(req_host)] | |||
| local host = route_t.hosts[req_host] or route_t.hosts[ctx.host_no_port] | |||
There was a problem hiding this comment.
This was my last remaining concern: I removed the call to split_port on this hot path, without any issues so far storing host_no_port in the context.
| HAS_HOST_PORT = 0x04, | ||
| HAS_REGEX_URI = 0x01, | ||
| PLAIN_HOSTS_ONLY = 0x02, | ||
| HAS_WILDCARD_HOST_PORT = 0x04, |
There was a problem hiding this comment.
Renamed this submatching rule to be more precise: we only case about it when dealing with regex hosts.
thibaultcha
force-pushed
the
feat/match-port
branch
from
97e3b28 to
0b58992
Compare
Allow routes to specify a port in the host parameter. If present, it matches only requests to that specific port. If absent, requests to any port will match. Requests without explicit port are handled as if they included the appropriate default port (80 for HTTP, 443 for HTTPs). Routes with a port have higher priority than those without; even for requests without an explicit port in the Host header. From #5102
### Summary PR #5102 changed service entity where it added optional `port` to `service.host`. This is not actually used, and is wrong as the service also has `service.port` for that. Thus this PR returns that back to original, that is `service.host` cannot contain `port`.
### Summary PR #5102 changed service entity where it added optional `port` to `service.host`. This is not actually used, and is wrong as the service also has `service.port` for that. Thus this PR returns that back to original, that is `service.host` cannot contain `port`.
Summary
Allow routes to specify a port in the host parameter. If present, it matches only requests to that
specific port. If absent, requests to any port matches. Requests without explicit port are handled as if they included the appropriate default port (80 for HTTP, 443 for HTTPS). Routes with port have higher priority than those that those without; even for requests without explicit port.