Portability refactor #3074
Portability refactor #3074
Conversation
Used by some openresty packages, e.g. https://aur.archlinux.org/packages/openresty/
|
@daurnimator Thanks for the PR! No major complaint but some minor stuff on my side. I would really like @shashiranjan84 and @p0pr0ck5 to take a look at this as well (since you contributed those plugins and/or maintain the building pipeline for our Kong release artifacts, any consequences you can foresee? Should we give this branch a try internally?). |
|
Seems reasonable from a build perspective at first glance. I have a few minor comments as wel. Will give this a spin through our artifact pipeline in the next few days. |
| @@ -9,6 +9,7 @@ local fmt = string.format | |||
| local nginx_bin_name = "nginx" | |||
| local nginx_search_paths = { | |||
| "/usr/local/openresty/nginx/sbin", | |||
| "/opt/openresty/nginx/sbin", | |||
There was a problem hiding this comment.
This is almost certainly undesirable to be hardcoded. I can definitely see a case where this could be configurable, though. Maybe a separate scope of change?
There was a problem hiding this comment.
I think it's already configurable; however there's much to be said for sensible defaults.
/opt/openresty/nginx/sbin is the location on a few distros, so I don't know why we wouldn't add it to places we search
There was a problem hiding this comment.
Hmm, I don't see this local var being defined anywhere else? Is it in a config in forgetting about? (I suspect so, I seem to be doing that a lot lately ;)).
To me it makes the most sense to make this user-configurable if appropriate. Otherwise it just becomes an opinionated list of where we think the bin might live (the current definition is the only location that's used in officially supported distros, fwiw)
There was a problem hiding this comment.
Hmm, I don't see this local var being defined anywhere else? Is it in a config in forgetting about? (I suspect so, I seem to be doing that a lot lately ;)).
It's implicitly configurable via $PATH
There was a problem hiding this comment.
Otherwise it just becomes an opinionated list of where we think the bin might live
Is that not the whole point of a function called find_nginx_bin?
kong/plugins/basic-auth/crypto.lua
Outdated
| local format = string.format | ||
|
|
||
| local function tohex(s) |
There was a problem hiding this comment.
There are bindings in lua-resty-string to ngx_hex_dump. Probably worth using that existing call than rolling our own here
There was a problem hiding this comment.
kong doesn't currently have a dependency on lua-resty-string. Should I add it?
There was a problem hiding this comment.
lua-resty-string is built with all modern versions of openresty. Seems unnecessary?
There was a problem hiding this comment.
K. pushed/updated.
kong/plugins/basic-auth/crypto.lua
Outdated
| @@ -16,6 +23,6 @@ return { | |||
| -- @return hash of the salted credential's password | |||
| encrypt = function(credential) | |||
| local salted = salt_password(credential) | |||
| return crypto.digest("sha1", salted) | |||
| return tohex(openssl_digest.new("sha1"):final(salted)) | |||
There was a problem hiding this comment.
Let's just use the built-in binding for sha1 composition that openresty makes available here. Using an openssl binding at all is not necessary
daurnimator
force-pushed
the
refactor/daurn-portability
branch
2 times, most recently
from
e1e5f96 to
2e03769
Compare
|
Ran this through our build pipeline and didn't see any issues. |
|
@p0pr0ck5 Awesome! Thanks! |
|
I'll have a look at this on OS X today and will merge if nothing comes up, so it can go out in our upcoming 0.12.0 release. |
| @@ -24,7 +24,7 @@ dependencies = { | |||
| "luatz == 0.3", | |||
| "lua_system_constants == 0.1.2", | |||
| "lua-resty-iputils == 0.3.0", | |||
| "luacrypto == 0.3.2", | |||
| "luaossl == 20171028", | |||
There was a problem hiding this comment.
@daurnimator Would you mind adding the CRYPTO_DIR variable in the install rule of the Makefile as part of the same commit? It seems luaossl requires both OPENSSL_DIR and CRYPTO_DIR (previously, we only needed OPENSSL_DIR. It can probably just be:
@luarocks make OPENSSL_DIR=$(OPENSSL_DIR) CRYPTO_DIR=$(OPENSSL_DIR)
Used by some OpenResty packages, e.g. https://aur.archlinux.org/packages/openresty/ From #3074
|
Awesome, manually merged to next and scheduled for 0.12.0 @daurnimator! Thanks! |
Used by some OpenResty packages, e.g. https://aur.archlinux.org/packages/openresty/ From #3074
Used by some OpenResty packages, e.g. https://aur.archlinux.org/packages/openresty/ From #3074
No description provided.