Ports should be specified in CURLOPT_PORT, not in the host #782
Conversation
Rather than including the port in the host, which also makes the host header include the port, the port should be assigned using the CURLOPT_PORT option.
Fix tests so that they are no longer checking for the host to have the port in them as welll
Now that port isn't a part of the hostname, move it to be it's own member variable
Now that the port is being stored separately from the host, we should be asserting that the port is stored correctly.
The connection interface also needs the new port member variable and get function
Accidentally added a comment start
This fixes the failing ClientTest
|
On the run at the moment so haven't looked at this closely... but does it matter? My understanding is that regardless of where the port is specified (in the URL or in the curl options) the end result is the same? |
|
Ahh I see your comment in #548 (comment). Makes sense. Will try to take a closer look at this later today, or on monday at the latest. Thanks for the PR! |
|
Having the port in the host header is optional, it technically works, but can cause some problems in some setups. For my purposes, we use an HAProxy instance in front of the elasticsearch server. The HAProxy instance looks at the host header for routing, and having the port in the host header throws things off. This is just a more proper way of doing things, and prevents some confusion and frustration when the host header is important. Plus, having the port split out as it's own member variable is, in my opinion, cleaner than it is being in the host. |
|
Hello @polyfractal, May I ping you about this PR ? Is there anything missing before merging ? Regards, |
|
Hi @polyfractal , |
|
Maybe someone else can handle this PR. |
|
Thanks @nicomro for the remind. I'll review this PR asap and hopefully merge with 7.0.0 release. |
|
Just merged in master for 7.0.0 release. Thanks @afrozenpeach for this PR! |
Because the host is adding the port, the host header also has the port in it. Instead, the handler should be using CURLOPT_PORT to specify which port is the curl request is being sent on.
This also affects unit tests that no longer need to verify that the port is in the host name.