fix(ldap) cache key to work when plugin is configured globally #3354

bungle · 2018-03-28T20:14:41Z

Summary

Changes the way how LDAP cache key in constructed. Previously it didn't share cache with similarly configured LDAP plugins. And more importantly, it was broken when configured as a global plugin (that was reported with #3335)

Full changelog

Implement cache key based on plugin configuration

Issues resolved

Fix #3335

StephenLord · 2018-03-28T20:54:09Z

I will give it a go, may take a day or so to get there, my kong deployments are currently done by pulling a docker image, so I need to work out how to drop this into my setup.

Tieske · 2018-03-28T20:56:49Z

kong/plugins/ldap-auth/access.lua

 local function authenticate(conf, given_credentials)
  local given_username, given_password = retrieve_credentials(given_credentials, conf)
  if given_username == nil then
    return false
  end

-  local route_id = conf.route_id or conf.api_id
-  local cache_key = "ldap_auth_cache:" .. route_id .. ":" .. given_username
+  local cache_key = acl_cache_key(conf, given_username)


this is calculated on every request. We better cache the calculated key.

Not sure, the calculation is not very intensive. Maybe caching is even slower than calculating MD5 out of small string? Not sure if it is worth considering added complexity?

if you leave the username out of the hash, you can create the entire key and store it in a weak table. That would be a single table lookup, followed by the username concatenation.

Tieske · 2018-03-28T20:58:16Z

spec/03-plugins/21-ldap-auth/01-access_spec.lua

+    }, ":"))
+  }
+
+  return concat(cache_key, ":")
 end


maybe also add a test that reproduces the original problem?

Tieske

I like the hash solution for other plugin instances using the same ldap backend. More performant and less resources. 👍

Tieske · 2018-03-28T21:05:13Z

pinging @shashiranjan84 since he did the original work on this.

@bungle maybe add a small comment in the schema.lua file that if any properties are added/removed/updated, that the hash creation might need to be updated.

shashiranjan84 · 2018-03-28T21:41:12Z

kong/plugins/ldap-auth/access.lua

@@ -84,14 +86,31 @@ local function load_credential(given_username, given_password, conf)
  return {username = given_username, password = given_password}
 end

+
+local function acl_cache_key(conf, username)


Is this name intentional acl_cache_key, not a big deal though?

oops... how did I miss that... good catch

I copied that name from unit tests. I can rename both.

thibaultcha · 2018-03-28T21:53:46Z

kong/plugins/ldap-auth/access.lua

+    }, ":"))
+  }
+
+  return concat(cache_key, ":")


It is a lot more efficient to follow the approach taken by dao:cache_key() rather than paying the cost of 2 short-lived tables, the double concatenation operation, and the md5 hash. Any reason for now using a simple string.format instead? Combined with @Tieske's comment about caching the cache key prefix (that of the plugin configuration), and simply use the username as the dynamic component?

Sure, I will change that. A lot more in this case might be a few microseconds, but of course better to do it like that. Not sure though, should we use something else than MD5. Maybe requiring lua-resty-string and using SHA256 instead. Maybe not worth it, when I remove username from the hash.

Well, as always, it isn't about making that particular piece of the code run a few microseconds faster, but rather prevent the spreading of anti-patterns and ensure that overall, the proxy still performs relatively well in hot code paths. To avoid accumulating performance penalties when they can be avoided. It is a continuous effort.
In this case if we cache the key and only rotate the username part, we can get away with the current approach, sure.

I'm not sure why the hashing is required though?

I think the resulting string is a lot smaller?

That's the only benefit I am seeing as well (in which case md5 should be just fine?)

A key without hashing:

ldap_auth_cache:ec2-54-172-82-117.compute-1.amazonaws.com:389:ou=scientists,dc=ldap,dc=mashape,dc=com:uid:2:einstein

A key with hashing:

ldap_auth_cache:aec022a3d1d6590c4432b4bc65433e2e:einstein

bungle · 2018-03-28T23:39:13Z

@Tieske, @thibaultcha, @shashiranjan84,

Ok, I tried to fix all your concerns. Let me know what you think about it now.

thibaultcha · 2018-03-29T00:49:28Z

The CI build seems to consistently error on the following step:

spec-old-api/03-plugins/21-ldap-auth/01-access_spec.lua @ 326: Plugin: ldap-auth (access) caches LDAP Auth Credential

I restarted both builds to make sure that this wasn't a flaky setup.

Tieske · 2018-03-29T06:56:58Z

kong/plugins/ldap-auth/access.lua

+      conf.base_dn,
+      conf.attribute,
+      conf.cache_ttl
+    ))


we should remove case sensitivity from the cache-key for those elements that are not case sensitive (eg. hostname, haven't looked into the ldap specifc ones).

LDAP specific ones are usually case-insensitive, but you may possibly configure them to be case-sensitive.

then the safe way would be to keep them case-sensitive, so that only leaves the hostname to update.

http://ldapwiki.com/wiki/Distinguished%20Name%20Case%20Sensitivity

(I don't have a courage to make other than host case-insensitive, EVEN though in most sane cases they all are case-insensitive, including the username).

aren't the host and port properties only used to connect to the ldap server?

Tieske · 2018-03-29T07:15:37Z

@thibaultcha somewhat related to this, in the light of plugin-api spec, we could consider dropping the weak table pattern and use the config table instead.

currently the config table is considered read-only, hence we use the table as a key in a weak table to cache intermediary results.

we could define it as:

the plugin is the owner of the config table. Allowing the plugin to update the table.
Kongs responsibility will only be to fetch the plugin configuration and take care of invalidation and caching
Any changes made to the table will never be persisted, and are local to a worker process
The lifetime of the table may differ from the worker process based on lru or similar cache evictions

we might even consider to add a initialize_config() method to the plugins to create the intermediary updates right after fetching it, and forego on any runtime checks.

None of this for this PR though.

bungle · 2018-03-29T07:36:50Z

@Tieske, yes I was looking forward for that, but couldn't find a place currently to do that. Your other comments are good (case insensitivity), also I need to check that erroring test.

…3335

bungle added the pr/please review label Mar 28, 2018

bungle mentioned this pull request Mar 28, 2018

Cannot use ldap-auth as a global plugin #3335

Closed

bungle requested a review from Tieske March 28, 2018 20:18

Tieske reviewed Mar 28, 2018

View reviewed changes

Tieske requested changes Mar 28, 2018

View reviewed changes

Tieske added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/please review labels Mar 28, 2018

shashiranjan84 reviewed Mar 28, 2018

View reviewed changes

thibaultcha reviewed Mar 28, 2018

View reviewed changes

bungle force-pushed the fix/ldap-cache-key branch from ab9ae12 to cfec416 Compare March 28, 2018 23:37

bungle added pr/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Mar 28, 2018

bungle force-pushed the fix/ldap-cache-key branch from cfec416 to 401a820 Compare March 29, 2018 00:06

Tieske reviewed Mar 29, 2018

View reviewed changes

fix(ldap) cache key to work when plugin is configured globally, see #…

bf89d4d

…3335

bungle force-pushed the fix/ldap-cache-key branch from 401a820 to bf89d4d Compare March 29, 2018 10:42

Tieske approved these changes Mar 29, 2018

View reviewed changes

bungle merged commit 8318ad5 into master Mar 29, 2018

hishamhm deleted the fix/ldap-cache-key branch April 13, 2018 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ldap) cache key to work when plugin is configured globally #3354

fix(ldap) cache key to work when plugin is configured globally #3354

bungle commented Mar 28, 2018

StephenLord commented Mar 28, 2018

Tieske Mar 28, 2018

bungle Mar 28, 2018

Tieske Mar 28, 2018

Tieske Mar 28, 2018

Tieske left a comment

Tieske commented Mar 28, 2018

shashiranjan84 Mar 28, 2018 •

edited

Loading

Tieske Mar 28, 2018

bungle Mar 28, 2018

thibaultcha Mar 28, 2018 •

edited

Loading

bungle Mar 28, 2018 •

edited

Loading

thibaultcha Mar 28, 2018

Tieske Mar 28, 2018

thibaultcha Mar 28, 2018

bungle Mar 28, 2018

bungle commented Mar 28, 2018

thibaultcha commented Mar 29, 2018

Tieske Mar 29, 2018

bungle Mar 29, 2018

Tieske Mar 29, 2018

bungle Mar 29, 2018 •

edited

Loading

Tieske Mar 29, 2018

Tieske commented Mar 29, 2018

bungle commented Mar 29, 2018

fix(ldap) cache key to work when plugin is configured globally #3354

fix(ldap) cache key to work when plugin is configured globally #3354

Conversation

bungle commented Mar 28, 2018

Summary

Full changelog

Issues resolved

StephenLord commented Mar 28, 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

Tieske left a comment

Choose a reason for hiding this comment

Tieske commented Mar 28, 2018

shashiranjan84 Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

bungle Mar 28, 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

Choose a reason for hiding this comment

bungle commented Mar 28, 2018

thibaultcha commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bungle Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tieske commented Mar 29, 2018

bungle commented Mar 29, 2018

shashiranjan84 Mar 28, 2018 •

edited

Loading

thibaultcha Mar 28, 2018 •

edited

Loading

bungle Mar 28, 2018 •

edited

Loading

bungle Mar 29, 2018 •

edited

Loading