3scale / APIcast

3scale API Gateway
Apache License 2.0
304 stars 171 forks source link

Fix upstream default port when HTTP_PROXY #1440

Closed eguzki closed 5 months ago

eguzki commented 5 months ago

What

When the gateway is configured to use http_proxy (i.e proxy between APIcast and some plain HTTP/1.1 upstream), and the port is not specified in api_backend service attribute, the request going from APIcast has a wrong absolute URI path

GET http://www.example.org:nil/pub/WWW/TheProject.html HTTP/1.1

Note that the port in the request line is nil.

~The issue is not happening for https_proxy based connections.~ It turns out that the camel use case is affected for https_proxy use case.

Verification Steps

Request should return 200

``` * Added get.example.com:8080:127.0.0.1 to DNS cache * Hostname get.example.com was found in DNS cache * Trying 127.0.0.1:8080... * Connected to get.example.com (127.0.0.1) port 8080 (#0) > GET /?user_key=123 HTTP/1.1 > Host: get.example.com:8080 > User-Agent: curl/7.81.0 > Accept: */* > * Mark bundle as not supporting multiuse < HTTP/1.1 200 OK < Server: openresty < Date: Wed, 31 Jan 2024 22:44:40 GMT < Content-Type: application/json < Content-Length: 251 < Connection: keep-alive < Via: 1.1 tinyproxy (tinyproxy/1.11.1) < Access-Control-Allow-Origin: * < Access-Control-Allow-Credentials: true < { "args": { "user_key": "123" }, "headers": { "Accept": "*/*", "Connection": "close", "Host": "example.com", "User-Agent": "curl/7.81.0" }, "origin": "192.168.64.3", "url": "http://example.com/get?user_key=123" } * Connection #0 to host get.example.com left intact ```
docker compose -p http-proxy-plain-http-upstream logs -f proxy

The request path should not contain nil port

proxy  | GET http://example.com/get?user_key=123 HTTP/1.1\r
eguzki commented 5 months ago

It turns out that the camel use case is affected. Fix added.

tkan145 commented 5 months ago

LGTM.

Please also update the CHANGELOG file

eguzki commented 5 months ago

Added CHANGELOG entry. Since there are new commits in the PR, I need new approval to be fair.

tkan145 commented 5 months ago

+1 feel free to merge when you are ready