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

mayorova · 2018-03-22T18:31:23Z

An alternative (and a more generic) way to solve the issue with the upstream policy: #647

mikz

Looking pretty good but it might need a bit of reorganization to cleanup the API. Good job !

mikz · 2018-03-23T10:05:50Z

CHANGELOG.md

+
+### Changed
+
+- Changed the way `resty.balancer` sets the default port, in case no port is specified for the upstream [PR #662](https://github.com/3scale/apicast/pull/662)


The changelog should say how and not what. So it should say it uses default port for the scheme.

mikz · 2018-03-23T10:06:50Z

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) or nil


No need for or nil

mikz · 2018-03-23T10:09:49Z

gateway/src/resty/balancer.lua

@@ -87,7 +87,7 @@ function _M.select_peer(self, peers)
  return peer
 end

-function _M.set_peer(self, peers)
+function _M.set_peer(self, peers, default_port)
  local balancer = self.balancer


I’m not really convinced by the API.

Shouldn’t this function extract the default port?

Extract from where? :)

I think the best approach is to not use set_peer but rather select_peer + balancer.set_current_peer.

mikz · 2018-03-23T10:14:57Z

gateway/src/apicast/balancer.lua

 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 port = get_default_port(ngx.var.proxy_pass)


I checked the nginx docs and looks like there is no built in variable to get scheme of the upstream server. Pity.
We should evaluate this only when the port is missing.

mikz · 2018-04-03T21:52:48Z

gateway/src/apicast/balancer.lua

-    ngx.log(ngx.ERR, "failed to set current backend peer: ", err)
-    ngx.exit(ngx.status)
+    ngx.log(ngx.ERR, 'could not select peer: ', err)
+    exit_service_unavailable()


Better to signal it returns by doing return exit_service_unavailable() as recommended in the ngx.exit documentation.

Yeah, I thought about it, but for some reason didn't put it. Will fix, thanks.

mikz

👍

davidor · 2018-04-04T08:26:16Z

spec/balancer_spec.lua

+      assert.spy(b.balancer.set_current_peer).was.called_with('127.0.0.2', 443)
+    end)
+  end)
+end)


Nitpick: There's no newline at the end of the file.

davidor · 2018-04-04T08:32:29Z

gateway/src/apicast/balancer.lua

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


Just to clarify, I think this only works with http and https: https://github.com/3scale/lua-resty-url#default_port

Right. Do you think it needs to be documented somewhere in this PR?

Well spotted. I think it does not need to be documented (because we don't really support other protocols), but handled with a proper error message.

btw, set_current_peer will set port to 0 if not provided: https://github.com/openresty/lua-resty-core/blob/a39b8d68b3b1c93f22259f95925c9dc854d69c1c/lib/ngx/balancer.lua#L113

I guess then we can just leave it to that and just have a test to verify what it does when no port is provided.

If the balancer has a default and would return an error then there is no need to handle this case by us. And to verify that it works it is necessary to have a test. IMO it is better to have a test to see how it works than code to maintain.

I've tried it, but turns out that:

setting unsupported://example.com in the upstream policy config will fail here, because trying to parse a URL with unsupported scheme returns a nil.

If I force upstream to push the invalid URL forward, APIcast throws an error:
[error] 4373#0: *5 invalid URL prefix in "unsupported://upstream"

So, I am not sure how I can test it in either integration or unit test :/

@mikz @davidor thoughts?

@mayorova fail how? throw an exception? then it should be handled and we can test it is being handled by asserting an entry in the error log and some status.

@mikz 1. will throw an exception...
My point is – I don't see a way to test how balancer will act if the incorrect scheme is passed, because the execution will not even reach the balancer point, and will fail before that.
Or should I add a failing test for the upstream policy instead? (I would consider it as a different PR - handle exception in upstream policy and add test)

@mayorova does https://github.com/3scale/apicast/blob/dab3c183144fa4dc74e765e147ac56297cdcfa05/t/apicast-upstream-balancer.t help?

davidor · 2018-04-04T08:36:00Z

gateway/src/apicast/balancer.lua

+    port = get_default_port(ngx.var.proxy_pass)
+  end
+
+  local _


This looks strange.

Yeah :/
before it was an ok, but codeclimate complained about an unused variable.

Maybe err = balancer.balancer.set_current_peer(address, port)[2] ?... because we don't care about the ok value (or maybe we should)

Yes we should care about ok.

davidor · 2018-04-04T08:37:36Z

gateway/src/apicast/balancer.lua

-  local peer, err = balancer:set_peer(peers)
+  local peer, err = balancer:select_peer(peers)
+
+  local address, port


You can move this below the if.

davidor · 2018-04-04T08:39:16Z

gateway/src/apicast/balancer.lua

+  end
+
+  if not port then
+    port = get_default_port(ngx.var.proxy_pass)


I wonder if this can return nil because of the limitations of resty_url.default_port that I pointed out in another comment.
Not sure if that would be a problem later on.

I guess it can, but not sure what we should do in this case.
Defaulting to some specific port doesn't sound too good either. I guess here we can accept that apicast will return a 5xx error.

As pointed in #662 (comment) the port will be set to 0. So we just need to to have a test to see what it really does and we might not need to handle anything.

davidor · 2018-04-04T08:41:29Z

@mayorova The PR looks good 👍
My comments are about small issues.

…cer sets default port by scheme

mikz

Great job 👍 🥇

octobot assigned mayorova Mar 22, 2018

mayorova force-pushed the default-port-for-upstream branch from 714e914 to ab2ff03 Compare March 22, 2018 18:39

mayorova mentioned this pull request Mar 22, 2018

[upstream policy] use the default port (by scheme) for the upstream url #647

Closed

mayorova requested review from mikz and davidor March 22, 2018 18:49

mikz reviewed Mar 23, 2018

View reviewed changes

mayorova force-pushed the default-port-for-upstream branch from 65f52d9 to 3ae3e38 Compare April 3, 2018 17:22

mikz reviewed Apr 3, 2018

View reviewed changes

mayorova force-pushed the default-port-for-upstream branch 2 times, most recently from 9e6cbe8 to a121fdf Compare April 4, 2018 06:24

mikz approved these changes Apr 4, 2018

View reviewed changes

davidor reviewed Apr 4, 2018

View reviewed changes

mayorova force-pushed the default-port-for-upstream branch 2 times, most recently from 58adfba to 55e4339 Compare April 4, 2018 16:42

[balancer] resty.balancer doesn't fall back to port 80, apicast.balan…

d1a0937

…cer sets default port by scheme

mayorova force-pushed the default-port-for-upstream branch from 55e4339 to d1a0937 Compare April 4, 2018 17:00

mayorova added 3 commits April 23, 2018 12:37

[t] call actual apicast.balancer public method in the tests

28d5721

[t] Integration test for incorrect URI scheme

497acbe

luacheck fixes

904a6ee

mikz approved these changes Apr 23, 2018

View reviewed changes

davidor approved these changes Apr 23, 2018

View reviewed changes

davidor merged commit ce0045d into master Apr 23, 2018

davidor deleted the default-port-for-upstream branch April 23, 2018 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mayorova commented Mar 22, 2018

mikz left a comment

mikz Mar 23, 2018

mikz Mar 23, 2018

mikz Mar 23, 2018

mayorova Mar 23, 2018

mikz Apr 3, 2018

mikz Mar 23, 2018

mikz Apr 3, 2018

mayorova Apr 4, 2018

mikz left a comment

davidor Apr 4, 2018

mayorova Apr 4, 2018

davidor Apr 4, 2018

davidor Apr 4, 2018

mayorova Apr 4, 2018

mikz Apr 4, 2018

mayorova Apr 4, 2018

mikz Apr 4, 2018

mikz Apr 4, 2018

mayorova Apr 4, 2018

mikz Apr 6, 2018

mayorova Apr 6, 2018

mikz Apr 6, 2018

davidor Apr 4, 2018

mayorova Apr 4, 2018

mayorova Apr 4, 2018

mikz Apr 4, 2018

davidor Apr 4, 2018

davidor Apr 4, 2018

mayorova Apr 4, 2018

mikz Apr 4, 2018

davidor commented Apr 4, 2018

mikz left a comment


		### Changed

		- Changed the way `resty.balancer` sets the default port, in case no port is specified for the upstream [PR #662](https://github.com/3scale/apicast/pull/662)

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

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

Conversation

mayorova commented Mar 22, 2018

mikz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor commented Apr 4, 2018

mikz left a comment

Choose a reason for hiding this comment