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) diff --git a/dev-environments/http-proxy-plain-http-upstream/README.md b/dev-environments/http-proxy-plain-http-upstream/README.md index a2c2dacd6..bae329bb9 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 can 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: 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/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/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 748f53be9..ccf14b86b 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,28 @@ 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) + assert(type(uri) == 'table', 'the value of uri is not table') + 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 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/resty/url_helper_spec.lua b/spec/resty/url_helper_spec.lua new file mode 100644 index 000000000..a43806601 --- /dev/null +++ b/spec/resty/url_helper_spec.lua @@ -0,0 +1,99 @@ +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) + + 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) 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')