fix(globalpathces) ngx socket connect to unix domain socket #3633

rucciva · 2018-07-19T02:26:18Z

Summary

When connecting to unix domain socket, port parameter should not
be passed to tcpsock:connect or udpsock:connect

Full changelog

globalpatches of ngx.socket to connect to unix domain socket

Issues resolved

When connecting to unix domain socket, port parameter should not be passed to tcpsock:connect or udpsock:connect Fix Kong#3626

When connecting to unix domain socket, port parameter should not be passed to tcpsock:connect or udpsock:connect Patch of the previous commit

rucciva · 2018-07-19T05:51:00Z

Weird, the test succeded on the forked repository

Tieske · 2018-07-19T07:18:18Z

@rucciva one testsuite (PDK) failed. It ran fine after restarting.

Tieske

Problem solved, but the code is a mess this way (not your fault, but because of all the branches/if-then).

I think it would be cleaner to implement a helper function to strip the trailing nils. Something like:

    -- need to do the extra check here: https://github.com/openresty/lua-nginx-module/issues/860
    local function strip_nils(f, sock, host, port, opts)
      if opts then
        return f(sock, host, port, opts)
      elseif port then
        return f(sock, host, port)
      end
      return f(sock, host)
    end

This can then be used from both the TCP as well as the UDP functions, which would only have to do the unix: check, and then simply call the helper function

Tieske · 2018-07-19T07:49:58Z

actually this would be better:

    -- need to do the extra check here: https://github.com/openresty/lua-nginx-module/issues/860
    local function strip_nils(port, opts)
      if opts then
        return port, opts
      elseif port then
        return port
      end
    end

    -- to be called as:
    old_tcp_connect(sock, target_ip, strip_nils(port, opts))

rucciva · 2018-07-19T08:17:49Z

this failed when opts is not nil but the port is nil

   -- need to do the extra check here: https://github.com/openresty/lua-nginx-module/issues/860
    local function strip_nils(port, opts)
      if opts then
        return port, opts
      elseif port then
        return port
      end
    end

    -- to be called as:
    old_tcp_connect(sock, target_ip, strip_nils(port, opts))

i think it should be

local function strip_nils(port, opts)
      if opts and port then
        return port, opts
      elseif opts then
        return opts
      elseif port then
        return port
      end
    end

and the caller will be like this

    local function tcp_resolve_connect(sock, host, port, sock_opts)
      local target_ip, target_port
      if sub(host, 1, 5) == "unix:" then
        target_ip = host
      else
        target_ip, target_port = toip(host, port)
      end
      if not target_ip then
        return nil, "[toip() name lookup failed]: " .. tostring(target_port)
      end

      return old_tcp_connect(sock, target_ip, strip_nils(target_port, sock_opts))
    end

    local function udp_resolve_setpeername(sock, host, port)
      local target_ip, target_port
      if sub(host, 1, 5) == "unix:" then
        target_ip = host
      else
        target_ip, target_port = toip(host, port)
      end
      if not target_ip then
        return nil, "[toip() name lookup failed]: " .. tostring(target_port) -- err
      end

      return old_udp_setpeername(sock, target_ip, strip_nils(target_port))
    end

is this fine @Tieske ?

rucciva · 2018-07-19T08:22:11Z

or refactor further?

  -- need to do the extra check here: https://github.com/openresty/lua-nginx-module/issues/860
    local function strip_nils(port, opts)
      if opts and port then
        return port, opts
      elseif opts then
        return opts
      elseif port then
        return port
      end
    end

    local function resolve_connect(f, sock, host, port, sock_opts)
      local target_ip, target_port
      if sub(host, 1, 5) == "unix:" then
        target_ip = host
      else
        target_ip, target_port = toip(host, port)
      end
      if not target_ip then
        return nil, "[toip() name lookup failed]: " .. tostring(target_port)
      end
      return f(sock, target_ip, strip_nils(target_port, sock_opts))
    end

    local function tcp_resolve_connect(sock, host, port, sock_opts)
      return resolve_connect(old_tcp_connect, sock, host, port, sock_opts)
    end

    local function udp_resolve_setpeername(sock, host, port)
      return resolve_connect(old_udp_setpeername, sock, host, port)
    end

Tieske · 2018-07-19T09:07:54Z

@rucciva that's looking great (the last version). Thx. please adjust the PR. I'l comment on the update in line

Tieske · 2018-07-19T10:11:47Z

kong/globalpatches.lua

+      if port and opts then
+        return port, opts
+      elseif opts then
+        return opts


here you are "shifting" a parameter? is that what's intended? So in case of a unix socket, the proper call would be tcp_connect(sock, name, opts) instead of tcp_connect(sock, name, nil, opts)?

and you are correct, sorry for the noise. https://github.com/openresty/lua-nginx-module#tcpsockconnect

eh, no, you are not correct.

Since the function should be compatible with the ngx.socket.tcp calls, in the case of a unix socket, the caller would do connect(sock, name, opts), so in the code, the opts table would ba carried in the port variable actually.

Since the function should be compatible with the ngx.socket.tcp calls, in the case of a unix socket, the caller would do connect(sock, name, opts), so in the code, the opts table would ba carried in the port variable actually.

isn't that what will happens? if the port is nil then strip_nils will return only the opts,
so return f(sock, target_ip, strip_nils(target_port, sock_opts)) equals return f(sock, target_ip, sock_opts) when target_port is nil

oh i got what you means, sorry

since the third argument can be port or opts (and will be confusing), can i do it like this?

local function strip_nils(first, second) if second then return first, second elseif first then return first end end local function resolve_connect(f, sock, host, port_or_opts, opts) if sub(host, 1, 5) == "unix:" then opts = port_or_opts return f(sock, host, strip_nils(opts)) end local port = port_or_opts local target_ip, target_port = toip(host, port) if not target_ip then return nil, "[toip() name lookup failed]: " .. tostring(target_port) end return f(sock, target_ip, strip_nils(target_port, opts)) end local function tcp_resolve_connect(sock, host, port_or_opts, opts) return resolve_connect(old_tcp_connect, sock, host, port_or_opts, opts) end local function udp_resolve_setpeername(sock, host, port) return resolve_connect(old_udp_setpeername, sock, host, port) end

yes, seems appropriate

Tieske · 2018-07-19T10:12:34Z

kong/globalpatches.lua

      end
+      if not target_ip then


this check can be moved in the else block above, since it doesn't apply to unix sockets

Tieske · 2018-07-19T10:15:11Z

kong/globalpatches.lua

      if sub(host, 1, 5) == "unix:" then
-        target_ip = host -- unix domain socket, so just maintain the named values
-
+        target_ip = host


please do not remove the vertical whitspace, see also style guide

like this one right?

local function resolve_connect(f, sock, host, port, sock_opts) local target_ip, target_port if sub(host, 1, 5) == "unix:" then target_ip = host else target_ip, target_port = toip(host, port) if not target_ip then return nil, "[toip() name lookup failed]: " .. tostring(target_port) end end return f(sock, target_ip, strip_nils(target_port, sock_opts)) end

i removed the blank line in the last commit since in the style guide, it says For one-line blocks, blank lines are not necessary:

This is fine, no need for the blank line between the two end statements. In general, check the rest of the code.

rucciva · 2018-07-19T10:42:32Z

btw, the test result are not the same again

- tcpsock:connect(host, port, options_table?) - tcpsock:connect("unix:/path/to/unix-domain.socket", options_table?) - udpsock:setpeername(host, port) - udpsock:setpeername("unix:/path/to/unix-domain.socket")

Tieske · 2018-07-19T13:29:03Z

kong/globalpatches.lua

-      end
-
-      return old_tcp_connect(sock, target_ip, target_port, sock_opts)
+      return f(sock, target_ip, strip_nils(target_port, opts))


the code is correct, but I think you're overdoing the variable naming to explain some intricate details of how the socket functions work, which complicates the code and reduces code readability. Can't we just drop the local variables and stick to:

local function resolve_connect(f, sock, host, port, opts) if sub(host, 1, 5) ~= "unix:" then host, port = toip(host, port) if not host then return nil, "[toip() name lookup failed]: " .. tostring(port) end end return f(sock, host, strip_nils(port, opts)) end

seems ok for me too. in the end, this is a patch and users might not expect that the original ngx.socket.* are modified by kong, thus they will probably lookup the original ngx lua to find the definition.

Tieske · 2018-07-19T13:29:54Z

kong/globalpatches.lua

+      end
+    end
+
+    -- need to do the extra check here: https://github.com/openresty/lua-nginx-module/issues/860


this comment belongs to the function above: strip_nils

rucciva added 2 commits July 19, 2018 09:17

fix(globalpathces) ngx socket connect to unix domain socket

6abfe96

When connecting to unix domain socket, port parameter should not be passed to tcpsock:connect or udpsock:connect Fix Kong#3626

fix(globalpathces) ngx socket connect to unix domain socket

9b60d2a

When connecting to unix domain socket, port parameter should not be passed to tcpsock:connect or udpsock:connect Patch of the previous commit

53204f8

Tieske requested changes Jul 19, 2018

View reviewed changes

Tieske mentioned this pull request Jul 19, 2018

connect to unix socked failed #3626

Closed

refactor patch

2c0d30b

Tieske requested changes Jul 19, 2018

View reviewed changes

the patched module should be compatible with the original

866bb16

- tcpsock:connect(host, port, options_table?) - tcpsock:connect("unix:/path/to/unix-domain.socket", options_table?) - udpsock:setpeername(host, port) - udpsock:setpeername("unix:/path/to/unix-domain.socket")

Tieske requested changes Jul 19, 2018

View reviewed changes

shorter patch for better readability

3de9231

Tieske approved these changes Jul 19, 2018

View reviewed changes

Tieske merged commit d895296 into Kong:master Jul 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(globalpathces) ngx socket connect to unix domain socket #3633

fix(globalpathces) ngx socket connect to unix domain socket #3633

rucciva commented Jul 19, 2018

rucciva commented Jul 19, 2018

Tieske commented Jul 19, 2018

Tieske left a comment

Tieske commented Jul 19, 2018

rucciva commented Jul 19, 2018

rucciva commented Jul 19, 2018 •

edited

Loading

Tieske commented Jul 19, 2018

Tieske Jul 19, 2018

Tieske Jul 19, 2018

Tieske Jul 19, 2018

rucciva Jul 19, 2018

rucciva Jul 19, 2018

rucciva Jul 19, 2018 •

edited

Loading

Tieske Jul 19, 2018

Tieske Jul 19, 2018

Tieske Jul 19, 2018

rucciva Jul 19, 2018 •

edited

Loading

Tieske Jul 19, 2018

rucciva commented Jul 19, 2018

Tieske Jul 19, 2018

rucciva Jul 19, 2018

Tieske Jul 19, 2018

fix(globalpathces) ngx socket connect to unix domain socket #3633

fix(globalpathces) ngx socket connect to unix domain socket #3633

Conversation

rucciva commented Jul 19, 2018

Summary

Full changelog

Issues resolved

rucciva commented Jul 19, 2018

Tieske commented Jul 19, 2018

Tieske left a comment

Choose a reason for hiding this comment

Tieske commented Jul 19, 2018

rucciva commented Jul 19, 2018

rucciva commented Jul 19, 2018 • edited Loading

Tieske commented Jul 19, 2018

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

rucciva Jul 19, 2018 • edited Loading

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

rucciva Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rucciva commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rucciva commented Jul 19, 2018 •

edited

Loading

rucciva Jul 19, 2018 •

edited

Loading

rucciva Jul 19, 2018 •

edited

Loading