Include port in authority computed from absolute-uri #1484
Conversation
Fix the misuse of the UF_* bitfields correctly.
|
@alyssawilk you mind reviewing? |
| Buffer::OwnedImpl buffer( | ||
| "GET http://www.somewhere.com:4532/foo/bar HTTP/1.1\r\nHost: bah\r\n\r\n"); | ||
| expectHeadersTest(Protocol::Http11, true, buffer, expected_headers); | ||
| } |
There was a problem hiding this comment.
One early suggestion, can you add an integration test with a port as well? I assume we'd want route config with host "www.foo.com" to match "www.foo.com:80" and I'm not confident this would work end-to-end
I think you can just tweak the config you added in your prior patch to not have a * domain and then add or copy-and-extend your current absolute-uri test to make sure we match correctly.
There was a problem hiding this comment.
Though looking more at the code, if that doesn't work end to end it's more likely a problem with exiting route picking than this particular PR. So I'd still encourage testing and either fixing here or in a follow-up since ports are much more common with absolute URIs and this fix won't do much for you if you can't pick routes :-)
My only other comment is it'd be nice to have regression tests for the bit-shifting fix if it's doable But the code here looks totally solid.
There was a problem hiding this comment.
I'll add the integration test and see what happens.
- Test the difference in behavior with port numbers included in the uri and not
|
@alyssawilk your question is somewhat but not entirely related to #886. I honestly don't have a super strong opinion on this. On one hand I think it's simpler to just strip port like you mentioned. On the other hand perhaps people want to do this? Will defer to you on what you think is best (I've honestly never encountered a production use case where this has mattered for me personally either way). |
|
I think the question of how mapping http://www.foo.com:80 -> http://www.foo.com is somewhat unrelated, but useful to plan for. Current absolute uri support doesn't allow clients to reach more than one port per hostname unless the configuration also uses prefix. |
|
I don't have strong feelings so if no one speaks up in favor I'd skip for
now as it makes this feature simpler, and we can always add it later.
If I'm reading #886 <#886> if we strip
ports that would address that problem since http://host and http://host:80
will behave the same way, right?
…On Tue, Aug 22, 2017 at 10:28 AM, Matt Klein ***@***.***> wrote:
@alyssawilk <https://github.com/alyssawilk> your question is somewhat but
not entirely related to #886 <#886>.
I honestly don't have a super strong opinion on this. On one hand I think
it's simpler to just strip port like you mentioned. On the other hand
perhaps people want to do this? Will defer to you on what you think is best
(I've honestly never encountered a production use case where this has
mattered for me personally either way).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1484 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ARYFvVmxc4TqvzQCzmeb6JJl7EyKtg2bks5sauVxgaJpZM4O6WJb>
.
|
I think partially it would help, but that person seems to want us to rewrite with 443 in the port. I'm not sure that I would want to do that at all. To me just stripping ports (at least optionally) seems like a better option. |
|
Yeah, as long as we handle ports consistently, fixing that is orthogonal to
this PR
…On Tue, Aug 22, 2017 at 12:08 PM, Matt Klein ***@***.***> wrote:
***@***.**** approved this pull request.
LGTM. @alyssawilk <https://github.com/alyssawilk> OK with you for now?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1484 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ARYFvQDBM0lnjSnEnrDqYTwDU1-a5LRLks5sav0BgaJpZM4O6WJb>
.
|
Fix the misuse of the UF_* bitfields correctly.