hotfix(cli) seed random number generator in CLI

[thibaultcha]

Summary

If spawning multiple nodes at once (making use of the CLI from different
host machines), we need to make sure none of them are using the same
seed. To enforce this, we make use of the patched math.randomseed()
function, which should greatly reduce the probability of seed collision.

To allow for this change, we need a special flag indicating our scripts
if we are running inside of our CLI, so that out math.randomseed()
does not complain about being called in resty-cli's 'timer' context.

Full changelog

• add ngx.RESTY_CLI flag in bin/kong
• add an edge case in our patched math.randomseed()
• apply kong.core.globalpatches to our CLI environment

Issues resolved

Fix #1592

[thibaultcha]

Here is a question I'm asking myself: would it be safer to use OpenSSL's int RAND_bytes(u_char *buf, int num); (as per lua-resty-random) to generate our seeds? As of today, if two Kong containers are spawned at the same time and both have the same hostname, same pids for the kong start command, then their seeds will be identical, and their serf id will too.

Are we safe with using OpenSSL's RAND_bytes (that is, on every platform such as Linux/BSD/OSX)? If so, I'll update this PR with a new seeding generation instead of:

seed = ngx.time() + ngx.worker.pid()

[Tieske]

look here; https://github.com/Mashape/kong/blob/ddc7230f69a4aa65c0885aab9171c5a39d2458d9/kong/core/globalpatches.lua

contains a test to check for resty-cli (based on global args table), and drop the error when reseeding. I'm depending on the resty dns lib, which for now still reseeds. So I have no alternative but to accept it, so all I can do is make the application immune for reseeding.

I'd prefer not to merge this, but to update it in the dns branch.

[thibaultcha]

contains a test to check for resty-cli (based on global args table)

The reason for not relying on this is that I find it too fragile, being outside of our real of control. If resty-cli changes their implementation, it would break our check.

I'd prefer not to merge this, but to update it in the dns branch.

We definitely need this for 0.9.2.

[Tieske]

contains a test to check for resty-cli (based on global args table)

The reason for not relying on this is that I find it too fragile, being outside of our real of control. If resty-cli changes their implementation, it would break our check.

Far fetched imo. Resty-cli will always have the args table. OpenResty won't get it because it doesn't have it, it's running in nginx. Never gets it's own arguments.
And even if they change it, an easy fix in that case.

We definitely need this for 0.9.2.

I can see that. But can you then change the implementation to at least functional equivalent of the dns branch? Meaning; stop throwing errors, simply skip reseeding. Dns will not be able to cope with errors while the resty-dns lib still reseeds.

[Tieske]

apply kong.core.globalpatches to our CLI environment

did I miss this in the code?

[Tieske]

regarding using OpenSSL for randomseeding;

See openssl docs, seems that it will error if not properly seeded, hence should be safe to use???

[thibaultcha]

Resty-cli will always have the args table.

So will every other interpreters. I just don't think it is resty-cli specific enough, and semantically does not have the same meaning as "I guarantee you we are running with resty-cli right now".

stop throwing errors, simply skip reseeding.

I think the best behavior is actually to print a log at the NOTICE level, updating the patch now.

[thibaultcha]

Actually hold on. The current behavior is better. It's throwing an error if not seeded and trying to seed in another context than init_worker.

If already seeded, we simply log at DEBUG lvl, but there is not error being thrown.

I'd rather merge this, and you can comment out the error() until resty.dns removes the seeding they;re doing at the module level. Ultimately, this behavior is better, but not compatible with your branch and your branch only. As such, next should not suffer from it, at least not until the DNS stuff is merged.

[thibaultcha]

(especially since there will be a 0.9.2 release next week, and this behavior should be the one included in the release, as it also prevents people with custom Kong/plugins from eventually messing up with it.)

[Tieske]

Testing for resty;

before I had this, later switched to just testing for arg;

local i = 1 
for k,v in pairs(arg or {}) do 
  if k<i then i = k end
end
local is_resty = ("/"..tostring((arg or {})[i]) or ""):match('/resty$') ~= nil

[Tieske]

better/simpler probably;

local is_resty = ngx and (type(arg) == "table")

[Tieske]

other commenst valid.

So for now, imo;

• drop the flag in the cmd line scripts, auto-detect in globalpatches.lua
• update seeding from RAND_bytes, OpenSSL

[thibaultcha]

In the end, between:

local is_resty = ngx and (type(arg) == "table")

and:

ngx.RESTY_CLI = true

One is so much simpler and also more reliable (because other interpreters also have arg), that I don't even see the point of having a more complex check that produces the same result: store a boolean. It's like trying to do it the "smart" way, but it's actually not 100% safe and more complicated. Why would we want that?

Let's not even talk about:

local i = 1 
for k,v in pairs(arg or {}) do 
  if k<i then i = k end
end
local is_resty = ("/"..tostring((arg or {})[i]) or ""):match('/resty$') ~= nil

Which is, totally, completely overkill for the sole purposes of setting a flag.

[thibaultcha]

I would be fine with this in kong.core.globalpatches, if you prefer:

local i = 1 
for k,v in pairs(arg or {}) do 
  if k<i then i = k end
end

ngx.IS_RESTY_CLI = ("/"..tostring((arg or {})[i]) or ""):match('/resty$') ~= nil

We keep our dirty hacks in there (ew), we rely on a "smart" detection because we require the global patches in both the CLI and ngx_lua's runtime. Ultimately that would be fine I think.

[thibaultcha]

Merging as-is for now (0.9.2), we can change the resty check or the patched-too-early behavior later and especially in other branches.