Merge pull request #1440 from 3scale/fix-upstrem-default-port

Fix upstream default port when HTTP_PROXY

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)

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

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/"
             }
           },
           {

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:

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

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,

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

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

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)

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)

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')