From 6a4e8606c4c0bcf028b7ae4fcbd7916e71d3836c Mon Sep 17 00:00:00 2001 From: Eguzki Astiz Lezaun Date: Wed, 31 Jan 2024 19:52:54 +0100 Subject: [PATCH 1/6] add default port to request path to the proxy --- gateway/src/apicast/http_proxy.lua | 14 ++------------ gateway/src/resty/url_helper.lua | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/gateway/src/apicast/http_proxy.lua b/gateway/src/apicast/http_proxy.lua index 54d3783f5..ef90584d1 100644 --- a/gateway/src/apicast/http_proxy.lua +++ b/gateway/src/apicast/http_proxy.lua @@ -6,6 +6,7 @@ local ngx_http_version = ngx.req.http_version local ngx_send_headers = ngx.send_headers local resty_url = require "resty.url" +local url_helper = require('resty.url_helper') local resty_resolver = require 'resty.resolver' local http_proxy = require 'resty.http.proxy' local file_reader = require("resty.file").file_reader @@ -46,17 +47,6 @@ local function resolve_servers(uri) return resolver:get_servers(uri.host, uri) end -local function absolute_url(uri) --- target server requires hostname not IP and DNS resolution is left to the proxy itself as specified in the RFC #7231 --- https://httpwg.org/specs/rfc7231.html#CONNECT - return format('%s://%s:%s%s', - uri.scheme, - uri.host, - uri.port, - uri.path or '/' - ) -end - local function forward_https_request(proxy_uri, uri, proxy_opts) local body, err local sock @@ -228,7 +218,7 @@ function _M.request(upstream, proxy_uri) if err then ngx.log(ngx.WARN, "HTTP proxy is set, but no servers have been resolved, err: ", err) end - upstream.uri.path = absolute_url(uri) + upstream.uri.path = url_helper.absolute_url(uri) upstream:rewrite_request() return elseif uri.scheme == 'https' then diff --git a/gateway/src/resty/url_helper.lua b/gateway/src/resty/url_helper.lua index 748f53be9..08443b08a 100644 --- a/gateway/src/resty/url_helper.lua +++ b/gateway/src/resty/url_helper.lua @@ -1,4 +1,5 @@ local tonumber = tonumber +local format = string.format local resty_url = require('resty.url') local core_base = require('resty.core.base') @@ -40,4 +41,27 @@ function _M.parse_url(url) return uri end +-- absolute_url formats an absolute URI from a table containing the fields: scheme, host, port and path +-- From https://datatracker.ietf.org/doc/html/rfc7230#section-5.3.2 +-- a client MUST send the target URI in absolute-form as the request-target +-- An example absolute-form of request-line would be: +-- GET http://www.example.org/pub/WWW/TheProject.html HTTP/1.1 +-- @param uri the table +-- @return absolute URI +function _M.absolute_url(uri) + local port = uri.port + local default_port = resty_url.default_port(uri.scheme) + + local host = uri.host + if port and port ~= default_port then + host = format('%s:%s', uri.host, port) + end + + return format('%s://%s%s', + uri.scheme, + host, + uri.path or '/' + ) +end + return _M From f39f064829877d441286be64b63e6496fb92c23f Mon Sep 17 00:00:00 2001 From: Eguzki Astiz Lezaun Date: Wed, 31 Jan 2024 19:53:36 +0100 Subject: [PATCH 2/6] http-proxy-plain-http-upstream dev environment: inspect traffic between apicast and proxy --- .../http-proxy-plain-http-upstream/README.md | 15 +++++++++++---- .../apicast-config.json | 2 +- .../docker-compose.yml | 9 +++++++++ 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/dev-environments/http-proxy-plain-http-upstream/README.md b/dev-environments/http-proxy-plain-http-upstream/README.md index a2c2dacd6..745b5c91f 100644 --- a/dev-environments/http-proxy-plain-http-upstream/README.md +++ b/dev-environments/http-proxy-plain-http-upstream/README.md @@ -18,18 +18,25 @@ Running custom apicast image make gateway IMAGE_NAME=quay.io/3scale/apicast:latest ``` -Traffic between the proxy and upstream can be inspected looking at logs from `example.com` service +Traffic between APIcast and the proxy ucan be inspected looking at logs from `proxy` service ``` -docker compose -p http-proxy-plain-http-upstream logs -f example.com +docker compose -p http-proxy-plain-http-upstream logs -f proxy ``` -Proxy can be inspected looking at logs from `proxy` service +Proxy logs from `actual.proxy` service ``` -docker compose -p http-proxy-plain-http-upstream logs -f proxy +docker compose -p http-proxy-plain-http-upstream logs -f actual.proxy ``` +Traffic between the proxy and upstream can be inspected looking at logs from `example.com` service + +``` +docker compose -p http-proxy-plain-http-upstream logs -f example.com +``` + + ## Testing `GET` request diff --git a/dev-environments/http-proxy-plain-http-upstream/apicast-config.json b/dev-environments/http-proxy-plain-http-upstream/apicast-config.json index 668dbf625..daa6967c3 100644 --- a/dev-environments/http-proxy-plain-http-upstream/apicast-config.json +++ b/dev-environments/http-proxy-plain-http-upstream/apicast-config.json @@ -14,7 +14,7 @@ { "name": "apicast.policy.http_proxy", "configuration": { - "http_proxy": "http://proxy:443/" + "http_proxy": "http://proxy:8080/" } }, { diff --git a/dev-environments/http-proxy-plain-http-upstream/docker-compose.yml b/dev-environments/http-proxy-plain-http-upstream/docker-compose.yml index 97ed3c9b9..f1e461fba 100644 --- a/dev-environments/http-proxy-plain-http-upstream/docker-compose.yml +++ b/dev-environments/http-proxy-plain-http-upstream/docker-compose.yml @@ -5,6 +5,7 @@ services: image: ${IMAGE_NAME:-apicast-test} depends_on: - proxy + - actual.proxy - example.com - two.upstream environment: @@ -23,6 +24,14 @@ services: volumes: - ./apicast-config.json:/tmp/config.json proxy: + image: alpine/socat:1.7.4.4 + container_name: proxy + command: "-d -v -d TCP-LISTEN:8080,reuseaddr,fork TCP:actual.proxy:443" + expose: + - "8080" + restart: unless-stopped + actual.proxy: + container_name: actual.proxy build: dockerfile: ./tinyproxy.Dockerfile expose: From 88e1061724a07b120bfe0ffec56d15dd1572fe23 Mon Sep 17 00:00:00 2001 From: Eguzki Astiz Lezaun Date: Wed, 31 Jan 2024 23:02:01 +0100 Subject: [PATCH 3/6] spec/resty/url_helper_spec.lua unittest for absolute_url --- spec/resty/url_helper_spec.lua | 79 ++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 spec/resty/url_helper_spec.lua diff --git a/spec/resty/url_helper_spec.lua b/spec/resty/url_helper_spec.lua new file mode 100644 index 000000000..6377c66bb --- /dev/null +++ b/spec/resty/url_helper_spec.lua @@ -0,0 +1,79 @@ +local _M = require 'resty.url_helper' + + +describe('URL parser', function() + describe('.absolute_url', function() + local absolute_url = _M.absolute_url + local config = '{}' + + it('when port is specified and does not match default port for http', function() + local uri = { + scheme = "http", + host = "example.com", + port = 8080, + path = "/some/path", + } + + assert.same('http://example.com:8080/some/path', + absolute_url(uri)) + end) + + it('when port is specified and matches default port for http', function() + local uri = { + scheme = "http", + host = "example.com", + port = 80, + path = "/some/path", + } + + assert.same('http://example.com/some/path', + absolute_url(uri)) + end) + + it('when port is specified and does not match default port for https', function() + local uri = { + scheme = "https", + host = "example.com", + port = 8443, + path = "/some/path", + } + + assert.same('https://example.com:8443/some/path', + absolute_url(uri)) + end) + + it('when port is specified and matches default port for https', function() + local uri = { + scheme = "https", + host = "example.com", + port = 443, + path = "/some/path", + } + + assert.same('https://example.com/some/path', + absolute_url(uri)) + end) + + it('when port is not specified for http', function() + local uri = { + scheme = "http", + host = "example.com", + path = "/some/path", + } + + assert.same('http://example.com/some/path', + absolute_url(uri)) + end) + + it('when port is not specified for https', function() + local uri = { + scheme = "https", + host = "example.com", + path = "/some/path", + } + + assert.same('https://example.com/some/path', + absolute_url(uri)) + end) + end) +end) From 9e0ee50905bb9623c8531018ad09c70ad6978aff Mon Sep 17 00:00:00 2001 From: Eguzki Astiz Lezaun Date: Wed, 31 Jan 2024 23:15:43 +0100 Subject: [PATCH 4/6] remove unused code --- gateway/src/apicast/upstream.lua | 10 ---------- spec/proxy_spec.lua | 6 ------ spec/upstream_spec.lua | 15 --------------- 3 files changed, 31 deletions(-) diff --git a/gateway/src/apicast/upstream.lua b/gateway/src/apicast/upstream.lua index 43c395c68..8900f33ac 100644 --- a/gateway/src/apicast/upstream.lua +++ b/gateway/src/apicast/upstream.lua @@ -86,16 +86,6 @@ function _M:resolve() return res end ---- Return port to use when connecting to upstream. ---- @treturn number port number -function _M:port() - if not self or not self.uri then - return nil, 'not initialized' - end - - return self.uri.port or resty_url.default_port(self.uri.scheme) -end - local root_uri = { ['/'] = true, [''] = true, diff --git a/spec/proxy_spec.lua b/spec/proxy_spec.lua index ad9349b98..e99a114e6 100644 --- a/spec/proxy_spec.lua +++ b/spec/proxy_spec.lua @@ -52,12 +52,6 @@ describe('Proxy', function() local get_upstream before_each(function() get_upstream = proxy.get_upstream end) - it('sets correct upstream port', function() - assert.same(443, get_upstream({ api_backend = 'https://example.com' }):port()) - assert.same(80, get_upstream({ api_backend = 'http://example.com' }):port()) - assert.same(8080, get_upstream({ api_backend = 'http://example.com:8080' }):port()) - end) - it("on invalid api_backend return error", function() local upstream, err = get_upstream({ api_backend = 'test.com' }) assert.falsy(upstream) diff --git a/spec/upstream_spec.lua b/spec/upstream_spec.lua index 6ab12e432..b24b8f4c3 100644 --- a/spec/upstream_spec.lua +++ b/spec/upstream_spec.lua @@ -52,21 +52,6 @@ describe('Upstream', function() end) end) - describe(':port', function() - it('returns port from the URI', function() - assert.same(8090, Upstream.new('http://host:8090'):port()) - end) - - it('returns default port for the scheme when none is provided', function() - assert.same(443, Upstream.new('https://example.com'):port()) - end) - - it('returns nil when port is unknown', function() - assert.is_nil(Upstream.new('ftp://example.com'):port()) - end) - end) - - describe(':append_path', function() it('return valid path when is not set', function() local up = Upstream.new('http://host:8090') From 9e67f3f440c42a7a1c8660fcadf1422d4bee7b77 Mon Sep 17 00:00:00 2001 From: Eguzki Astiz Lezaun Date: Fri, 2 Feb 2024 14:54:32 +0100 Subject: [PATCH 5/6] add default port to request path to the proxy for camel case --- .../http-proxy-plain-http-upstream/README.md | 2 +- gateway/src/resty/http/proxy.lua | 5 +++-- gateway/src/resty/url_helper.lua | 1 + spec/resty/url_helper_spec.lua | 20 +++++++++++++++++++ 4 files changed, 25 insertions(+), 3 deletions(-) diff --git a/dev-environments/http-proxy-plain-http-upstream/README.md b/dev-environments/http-proxy-plain-http-upstream/README.md index 745b5c91f..bae329bb9 100644 --- a/dev-environments/http-proxy-plain-http-upstream/README.md +++ b/dev-environments/http-proxy-plain-http-upstream/README.md @@ -18,7 +18,7 @@ Running custom apicast image make gateway IMAGE_NAME=quay.io/3scale/apicast:latest ``` -Traffic between APIcast and the proxy ucan be inspected looking at logs from `proxy` service +Traffic between APIcast and the proxy can be inspected looking at logs from `proxy` service ``` docker compose -p http-proxy-plain-http-upstream logs -f proxy diff --git a/gateway/src/resty/http/proxy.lua b/gateway/src/resty/http/proxy.lua index 8f644869d..b236aba04 100644 --- a/gateway/src/resty/http/proxy.lua +++ b/gateway/src/resty/http/proxy.lua @@ -3,6 +3,7 @@ local http = require 'resty.resolver.http' local resty_url = require 'resty.url' local resty_env = require 'resty.env' +local url_helper = require('resty.url_helper') local format = string.format local _M = { @@ -105,11 +106,11 @@ local function connect_proxy(httpc, request, skip_https_connect) request.path = format('%s://%s:%s%s', uri.scheme, uri.host, uri.port, uri.path or '/') return httpc elseif uri.scheme == 'https' and skip_https_connect then - request.path = format('%s://%s:%s%s', uri.scheme, uri.host, uri.port, request.path or '/') + local custom_uri = { scheme = uri.scheme, host = uri.host, port = uri.port, path = request.path } + request.path = url_helper.absolute_url(custom_uri) return _connect_tls_direct(httpc, request, uri.host, uri.port) elseif uri.scheme == 'https' then return _connect_proxy_https(httpc, request, uri.host, uri.port) - else return nil, 'invalid scheme' end diff --git a/gateway/src/resty/url_helper.lua b/gateway/src/resty/url_helper.lua index 08443b08a..ccf14b86b 100644 --- a/gateway/src/resty/url_helper.lua +++ b/gateway/src/resty/url_helper.lua @@ -49,6 +49,7 @@ end -- @param uri the table -- @return absolute URI function _M.absolute_url(uri) + assert(type(uri) == 'table', 'the value of uri is not table') local port = uri.port local default_port = resty_url.default_port(uri.scheme) diff --git a/spec/resty/url_helper_spec.lua b/spec/resty/url_helper_spec.lua index 6377c66bb..a43806601 100644 --- a/spec/resty/url_helper_spec.lua +++ b/spec/resty/url_helper_spec.lua @@ -75,5 +75,25 @@ describe('URL parser', function() assert.same('https://example.com/some/path', absolute_url(uri)) end) + + it('when uri is nil, asserts', function() + local uri = nil + local res, err = pcall(absolute_url, uri) + + assert.is_falsy(res) + assert.is_truthy(err) + end) + + it('when uri is not a table, asserts', function() + local uri = "some string" + local res, err = pcall(absolute_url, uri) + assert.is_falsy(res) + assert.is_truthy(err) + + uri = 1 + local res, err = pcall(absolute_url, uri) + assert.is_falsy(res) + assert.is_truthy(err) + end) end) end) From a3d1630a873b726415ef16d317478e0d7c7196d1 Mon Sep 17 00:00:00 2001 From: Eguzki Astiz Lezaun Date: Mon, 5 Feb 2024 09:52:09 +0100 Subject: [PATCH 6/6] Update CHANGELOG --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c9535be1..f6e5a5fb2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fixed issue with URL was not correctly escaped when using the JWT claim check policy [THREESCALE-10308](https://issues.redhat.com/browse/THREESCALE-10308) [PR #1428](https://github.com/3scale/APIcast/pull/1428) +- Fix upstream default port when HTTP_PROXY [PR #1440](https://github.com/3scale/APIcast/pull/1440) + ### Added - Detect number of CPU shares when running on Cgroups V2 [PR #1410](https://github.com/3scale/apicast/pull/1410) [THREESCALE-10167](https://issues.redhat.com/browse/THREESCALE-10167)