Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(refactor) plugins to remove dependencies to kong.tools and use more PDK #3845

Merged
merged 9 commits into from Oct 20, 2018

Conversation

Projects
None yet
3 participants
@bungle
Copy link
Member

commented Oct 10, 2018

Summary

This contains changes to following plugins and their test suite? This is rebased on @hishamhm's https://github.com/Kong/kong/tree/feat/plugins-update-new-apis branch (#3844) which should be merged and REVIEWed before this.

  • acl
  • basic-auth
  • correlation-id
  • hmac-auth
  • jwt
  • ldap-auth
  • rate-limiting (still has some flaky tests)
  • request-termination

Also contains a small fix to a typo on key-auth plugin.

@bungle bungle force-pushed the feat/plugins-update-new-apis-pt2 branch 6 times, most recently from 42e7340 to 8174bf7 Oct 10, 2018

)
end,
}, { __index = function(_, generator)
kong.log.err("Invalid generator: " .. generator)

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 15, 2018

Member

This extra metatable that's added for error-checking only is effectively dead code, because the schema guarantees that conf.generator is always valid. I think we could remove this and make generators a simple table.

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 15, 2018

Member

(I realize this was in the original code already, just noticed that and thought we could clean it up)

@@ -28,6 +24,7 @@ local DIGEST = "digest"
local SIGNATURE_NOT_VALID = "HMAC signature cannot be verified"
local SIGNATURE_NOT_SAME = "HMAC signature does not match"


local new_tab

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 15, 2018

Member

new_tab can be removed and replaced by kong.table.new

-- parse the header to retrieve hamc parameters
if authorization_header then
local iterator, iter_err = ngx_gmatch(authorization_header, "\\s*[Hh]mac\\s*username=\"(.+)\",\\s*algorithm=\"(.+)\",\\s*headers=\"(.+)\",\\s*signature=\"(.+)\"")
local iterator, iter_err = re_gmatch(authorization_header,
"\\s*[Hh]mac\\s*username=\"(.+)\",\\s*algorithm=\"(.+)\",\\s*headers=\"(.+)\",\\s*signature=\"(.+)\"")

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 15, 2018

Member

you can reduce the line by breaking this long string into shorter ones connected by .. (don't worry, the compiler's constant-subexpression-elimination will make it into a single string)

local signature_2 = ngx_decode_base64(hmac_params.signature)

local function validate_signature(hmac_params)
local signature_1 = create_hash(ngx.var.request_uri, hmac_params)

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 15, 2018

Member

It's unfortunate we still need to rely on ngx.var.request_uri here, but I guess it's because we don't have an exact equivalent in the PDK, right? Would this be our best shot sticking to the current PDK API?

local function get_path_and_query()
  local p = ngx.request.get_path()
  local q = ngx.request.get_raw_query()
  return (q ~= "") and (p .. "?" .. q) or p
end

(which is inefficient because get_path strips the query from ngx.var.request_uri and then this adds it back, I know.)

Is this pattern common enough to warrant a kong.request.get_path_and_query function? (The aws-lambda plugin does the same as the above, but that's mostly for backwards compat with the "request_uri" info being forwarded)

local ctx = ngx.ctx
if ctx.delay_response and not ctx.delayed_response then
ctx.delayed_response = {
local shared_ctx = kong.ctx.shared

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 15, 2018

Member

The whole delayed_response logic (and flush function) can be replaced by kong.response.exit, which takes care of it. It is possible to just do

if content then
  return kong.response.exit(status, content, {
    ["Content-Type"] = conf.content_type
  })
end

instead of this whole block, also removing the flush function above.

end


local function get_timestamps(now)

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 15, 2018

Member

Copying these functions multiple times inside the same plugin feels wrong. If the idea is to move them out of kong.tools.* and into the plugin, then maybe create a timestamps utility module inside the plugin for them?

This comment has been minimized.

Copy link
@bungle

bungle Oct 16, 2018

Author Member

Only two times. Is that the threshold, when to make a new library and modification to rocks.

This comment has been minimized.

Copy link
@bungle

bungle Oct 16, 2018

Author Member

I can get back to just using kong.tools.timestamp module.

local fmt = string.format


local NULL_UUID = "00000000-0000-0000-0000-000000000000"
local log_err = kong and kong.log.err or function(...)

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 15, 2018

Member

Is this here because of tests where kong is not available? If that's the case, I'd rather just populate kong.log.err in the test case before testing this module instead of leaking ngx.log here.

@hishamhm
Copy link
Member

left a comment

I think the delayed_response in request-termination is the biggest issue (unless I missed something there that makes that code necessary.)

@bungle bungle force-pushed the feat/plugins-update-new-apis-pt2 branch 2 times, most recently from 7e7efbf to 44830e1 Oct 16, 2018

@bungle bungle force-pushed the feat/plugins-update-new-apis-pt2 branch from 44830e1 to f427b25 Oct 17, 2018

@bungle bungle force-pushed the feat/plugins-update-new-apis-pt2 branch 4 times, most recently from ffe251b to b16b0d3 Oct 17, 2018

ngx_log(ngx_error, "[acl plugin] Cannot identify the consumer, add an ",
"authentication plugin to use the ACL plugin")
return responses.send_HTTP_FORBIDDEN("You cannot consume this service")
kong.log.err("Cannot identify the consumer, add an authentication " ..

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Oct 18, 2018

Member

It is still preferred to use the variadic form of this function, (, instead of ..).

end

return re_find(str, uuid_regex, 'ioj') ~= nil
end

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Oct 18, 2018

Member

Why are we duplicating is_valid_uuid? This is a blocker. The scope of this work is not to get rid of kong.tools.utils.

Show resolved Hide resolved kong/plugins/hmac-auth/access.lua
return input
end


This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Oct 18, 2018

Member

Such vendor files ideally should be left untouched and are not subject to our coding style, as pulling fixes from upstream becomes harder.

LdapAuthHandler.PRIORITY = 1002
LdapAuthHandler.VERSION = "0.1.0"
LdapAuthHandler.VERSION = "0.1.1"

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Oct 18, 2018

Member

All of these massive updates warrant increasing the second digit (e.g. 0.2.0) in all of these plugins, instead of the patch digit.

@bungle bungle force-pushed the feat/plugins-update-new-apis-pt2 branch from b16b0d3 to 0ad4aa3 Oct 19, 2018

@thibaultcha thibaultcha merged commit e147b4c into next Oct 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@thibaultcha thibaultcha deleted the feat/plugins-update-new-apis-pt2 branch Oct 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.