Merge pull request #662 from 3scale/default-port-for-upstream

[balancer] Don't force 80 to be the default if port is not provided

CHANGELOG.md

@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Changed
 
 - descriptions in `oneOf`s in policy manifests have been replaced with titles [PR #663](https://github.com/3scale/apicast/pull/663)
+- `resty.balancer` doesn't fall back to the port `80` by default. If the port is missing, `apicast.balancer` sets the default port for the scheme of the `proxy_pass` URL [PR #662](https://github.com/3scale/apicast/pull/662)
 
 ## [3.2.0-beta3] - 2018-03-20
 

gateway/src/apicast/balancer.lua

@@ -1,19 +1,52 @@
 local round_robin = require 'resty.balancer.round_robin'
+local resty_url = require 'resty.url'
+local empty = {}
 
 local _M = { default_balancer = round_robin.new() }
 
+local function get_default_port(upstream_url)
+  local url = resty_url.split(upstream_url) or empty
+  local scheme = url[1] or 'http'
+  return resty_url.default_port(scheme)
+end
+
+local function exit_service_unavailable()
+  ngx.status = ngx.HTTP_SERVICE_UNAVAILABLE
+  ngx.exit(ngx.status)
+end
+
 function _M.call(_, _, balancer)
   balancer = balancer or _M.default_balancer
   local host = ngx.var.proxy_host -- NYI: return to lower frame
   local peers = balancer:peers(ngx.ctx[host])
 
-  local peer, err = balancer:set_peer(peers)
+  local peer, err = balancer:select_peer(peers)
 
   if not peer then
-    ngx.status = ngx.HTTP_SERVICE_UNAVAILABLE
-    ngx.log(ngx.ERR, "failed to set current backend peer: ", err)
-    ngx.exit(ngx.status)
+    ngx.log(ngx.ERR, 'could not select peer: ', err)
+    return exit_service_unavailable()
+  end
+
+  local address, port = peer[1], peer[2]
+
+  if not address then
+    ngx.log(ngx.ERR, 'peer missing address')
+    return exit_service_unavailable()
   end
+
+  if not port then
+    port = get_default_port(ngx.var.proxy_pass)
+  end
+
+  local ok
+  ok, err = balancer.balancer.set_current_peer(address, port)
+
+  if not ok then
+    ngx.log(ngx.ERR, 'failed to set current backend peer: ', err)
+    return exit_service_unavailable()
+  end
+
+  ngx.log(ngx.INFO, 'balancer set peer ', address, ':', port)
 end
 
 return _M

gateway/src/apicast/policy/token_introspection/token_introspection.lua

@@ -44,7 +44,7 @@ local function introspect_token(self, token)
     local token_info, decode_err = cjson.decode(res.body)
     if type(token_info) == 'table' then
       return token_info
-    else 
+    else
       ngx.log(ngx.ERR, 'failed to parse token introspection response:', decode_err)
       return { active = false }
     end

gateway/src/resty/balancer.lua

@@ -30,7 +30,7 @@ local function new_peer(server, port)
 
   return {
     address,
-    tonumber(server.port or port or 80, 10)
+    tonumber(server.port or port, 10)
   }
 end
 
@@ -41,7 +41,7 @@ local function convert_servers(servers, port)
   for i =1, #servers do
     local peer = new_peer(servers[i], port)
 
-    if peer and #peer == 2 then
+    if peer then
       insert(peers, peer)
     else
       ngx.log(ngx.INFO, 'skipping peer because it misses address or port')

spec/balancer_spec.lua

@@ -0,0 +1,23 @@
+local apicast_balancer = require 'apicast.balancer'
+local resty_balancer = require 'resty.balancer'
+
+describe('apicast.balancer', function()
+
+  describe('.call', function()
+    local b = resty_balancer.new(function(peers) return peers[1] end)
+    b.balancer = { }
+
+    ngx.var = {
+      proxy_host = 'upstream'
+    }
+
+    it('sets default port from scheme if no port is specified for peers', function()
+      b.peers = function() return { { '127.0.0.2' } } end
+      ngx.var.proxy_pass = 'https://example.com'
+
+      b.balancer.set_current_peer = spy.new(function() return true end)
+      apicast_balancer:call({},b)
+      assert.spy(b.balancer.set_current_peer).was.called_with('127.0.0.2', 443)
+    end)
+  end)
+end)

t/apicast-upstream-balancer.t

@@ -51,21 +51,12 @@ GET /t
     server 0.0.0.1:1;
 
     balancer_by_lua_block {
-      local round_robin = require 'resty.balancer.round_robin'
+      local balancer = require 'apicast.balancer'
 
-      local balancer = round_robin.new()
-      local servers = { { address = '127.0.0.1', port = $TEST_NGINX_SERVER_PORT } }
-      local peers = balancer:peers(servers)
+      ngx.ctx.upstream = { { address = '127.0.0.1', port = $TEST_NGINX_SERVER_PORT } }
 
-      local ok, err = balancer:set_peer(peers)
-
-      if not ok then
-        ngx.status = ngx.HTTP_SERVICE_UNAVAILABLE
-        ngx.log(ngx.ERR, "failed to set current peer: "..err)
-        ngx.exit(ngx.status)
-      end
+      local peers = balancer:call()
     }
-
     keepalive 32;
   }
 --- config
@@ -91,18 +82,8 @@ yay
     server 0.0.0.1:1;
 
     balancer_by_lua_block {
-      local round_robin = require 'resty.balancer.round_robin'
-
-      local balancer = round_robin.new()
-      local peers = balancer:peers(ngx.ctx.upstream)
-
-      local peer, err = balancer:set_peer(peers)
-
-      if not peer then
-        ngx.status = ngx.HTTP_SERVICE_UNAVAILABLE
-        ngx.log(ngx.ERR, "failed to set current peer: "..err)
-        ngx.exit(ngx.status)
-      end
+      local balancer = require 'apicast.balancer'
+      local peers = balancer:call()
     }
 
     keepalive 32;
@@ -134,3 +115,35 @@ GET /t
 --- response_body
 yay
 --- error_code: 200
+
+
+
+=== TEST 3: unsupported scheme
+Using an unsopported URI scheme causes an exception
+--- http_config
+  lua_package_path "$TEST_NGINX_LUA_PATH";
+
+  upstream upstream {
+    server 0.0.0.1:1;
+
+    balancer_by_lua_block {
+      local balancer = require 'apicast.balancer'
+      ngx.ctx.upstream = { { address = '127.0.0.1', port = $TEST_NGINX_SERVER_PORT } }
+      local peers = balancer:call()
+    }
+
+    keepalive 32;
+  }
+--- config
+location /api {
+  echo 'yay';
+}
+
+location /t {
+  set $proxy_pass "unsupported://upstream/api";
+  proxy_pass $proxy_pass;
+}
+--- request
+GET /t
+--- error_code: 500
+--- error_log: invalid URL prefix in