feat(request-aware-table) add request aware table #11017

samugi · 2023-06-06T16:27:28Z

Summary

Add a request-aware table able to detect accesses from different requests.

Checklist

The Pull Request has tests
(na) There's an entry in the CHANGELOG
(na) There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

KAG-1570

bungle · 2023-06-07T05:52:43Z

What is request aware table in comparison to ngx.ctx or kong.ctx.plugin|shared?

samugi · 2023-06-07T09:25:07Z

What is request aware table in comparison to ngx.ctx or kong.ctx.plugin|shared?

I wondered that myself @bungle, thanks for bringing it up.

The main difference is that this table can detect race conditions and throw an error if they happen, instead of using separate scopes for each request to prevent them.
Race condition checks (which currently require an access to ngx.var per request) would only be turned on during testing, to ensure the table is used properly (i.e. cleared before it's accessed by another request). This allows defining a table that is local to the module where it is instantiated, can be cleared at will, and does not rely on a shared context.

However, if the performance impact of accessing this table compared to using the context is not significant, we could solve the same problem just as in this PR, using kong.ctx.plugin to prevent race conditions in the first place.

I'd be curious to hear @dndx 's opinion as well

kong/tools/request_aware_table.lua

kong/templates/nginx_kong.lua

AndyZhang0707 · 2023-09-14T23:49:38Z

@dndx could you take a look at this PR again? thx

kong/tools/request_aware_table.lua

dndx · 2023-09-26T08:21:19Z

kong/tools/request_aware_table.lua

+
+-- Check if access is allowed for table, based on the request ID
+local function enforce_sequential_access(table)
+  if next(table.__data) == nil or not get_request() then


Do we need the empty __data table check? I thought just checking __allowed_request_id below is enough.

the empty __data check is used to reset __allowed_request_id when the table is cleared. If there is no data it means the table was emptied. I removed it now that I reintroduced the :clear() method

@samugi I see. :clear would be a safer approach and I think we should keep that. It also makes the clearing intention more explicit.

dndx · 2023-09-26T08:22:14Z

kong/tools/request_aware_table.lua

+end
+
+return {
+  new = new,


Do we need a :clear method as well? How to clear __data?

I initially included a :clear method but that would mean the table has a different interface when in debug_mode compared to when it is not (in which case we just return a regular table).

One alternative I implemented now is to always include the :clear method for request_aware_table, the difference being that in non debug_mode the metatable with concurrency checks is not applied, wdyt?

I see what you are saying. We can do this:

With debug mode on:
Use the proxy and metatable.

With debug mode on:
Use a different metatable _mt_direct, no proxy in this case. Define the :clear on this metatable only, not table instance.

This avoids you having to hack the "clear" method on the table each time, and prevents it being accidentally cleared by the user using table.clear manually.

ok, I like that.

samugi · 2023-09-27T10:56:36Z

kong/plugins/rate-limiting/policies/init.lua

usage in a plugin

samugi · 2023-09-27T11:05:13Z

kong/plugins/rate-limiting/policies/init.lua

-    [cache_key] = <integer>
-  --]]
-}
+local cur_usage = request_aware_table.new()


we might want to preallocate some nrec, what could be a good value?

nowNick

Nice work! A clever idea and neat implementation 🚀

I left a few code style comments but it's entirely up to you. I'm happy with the way the code looks right now 👏

spec/02-integration/05-proxy/33-request-aware-table_spec.lua

spec/fixtures/custom_plugins/kong/plugins/request-aware-table/handler.lua

spec/02-integration/05-proxy/33-request-aware-table_spec.lua

kong/tools/request_aware_table.lua

add a request-aware table able to detect accesses from different requests.

* refactor * only use static log level to turn concurrency checks on or off * use single metatable, keep data and request_id in the instance * use table.new -style initialization * reintroduce :clear() method Co-authored-by: Datong Sun <datong.sun@konghq.com>

* small refactors here and there

pull-request-size bot added the size/L label Jun 6, 2023

samugi marked this pull request as draft June 6, 2023 16:27

github-actions bot assigned samugi Jun 6, 2023

github-actions bot added the core/templates label Jun 6, 2023

samugi force-pushed the feat/request-aware-table branch 2 times, most recently from 35086ad to 95706cd Compare June 6, 2023 17:38

samugi force-pushed the feat/request-aware-table branch 3 times, most recently from 0db9d43 to 7530d1d Compare June 8, 2023 18:12

dndx reviewed Aug 14, 2023

View reviewed changes

kong/tools/request_aware_table.lua Outdated Show resolved Hide resolved

kong/tools/request_aware_table.lua Outdated Show resolved Hide resolved

samugi commented Aug 31, 2023

View reviewed changes

kong/tools/request_aware_table.lua Outdated Show resolved Hide resolved

samugi force-pushed the feat/request-aware-table branch from 717117e to 09d2481 Compare August 31, 2023 10:16

samugi commented Aug 31, 2023

View reviewed changes

kong/templates/nginx_kong.lua Show resolved Hide resolved

samugi force-pushed the feat/request-aware-table branch from 09d2481 to 4bf980a Compare August 31, 2023 10:21

samugi marked this pull request as ready for review August 31, 2023 10:23

samugi force-pushed the feat/request-aware-table branch 5 times, most recently from 85553b9 to 862d76d Compare September 8, 2023 07:30

samugi force-pushed the feat/request-aware-table branch 2 times, most recently from 6f69d98 to d83b6d3 Compare September 12, 2023 14:29

dndx requested changes Sep 21, 2023

View reviewed changes

kong/tools/request_aware_table.lua Outdated Show resolved Hide resolved

samugi force-pushed the feat/request-aware-table branch 2 times, most recently from ddf396f to fede892 Compare September 25, 2023 16:40

samugi commented Sep 25, 2023

View reviewed changes

kong/tools/request_aware_table.lua Outdated Show resolved Hide resolved

dndx reviewed Sep 26, 2023

View reviewed changes

samugi force-pushed the feat/request-aware-table branch 3 times, most recently from e5e3d6a to 385dffb Compare September 27, 2023 10:55

github-actions bot added the plugins/rate-limiting label Sep 27, 2023

samugi commented Sep 27, 2023

View reviewed changes

kong/plugins/rate-limiting/policies/init.lua

Copy link

Member Author

samugi Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usage in a plugin

samugi commented Sep 27, 2023

View reviewed changes

samugi force-pushed the feat/request-aware-table branch from 385dffb to 85f093a Compare September 27, 2023 17:24

samugi requested a review from dndx September 27, 2023 17:25

nowNick approved these changes Sep 29, 2023

View reviewed changes

nowNick reviewed Sep 29, 2023

View reviewed changes

kong/tools/request_aware_table.lua Outdated Show resolved Hide resolved

samugi force-pushed the feat/request-aware-table branch from afe83aa to 2901f39 Compare October 4, 2023 15:36

samugi and others added 9 commits October 11, 2023 22:43

feat(request-aware-table) add request aware table

0e32f57

add a request-aware table able to detect accesses from different requests.

tests(request-aware-table): integration

4e172da

feat(request-aware-table): enable checks when log_level is debug

391173d

feat(request-aware-table): add to rate-limiting

3604e3d

Update request_aware_table.lua

bb1c7eb

Update request_aware_table.lua

cc9b6d8

feat(request-aware-table): PR feedback

b98fa42

* small refactors here and there

fix styles

e17992f

dndx force-pushed the feat/request-aware-table branch from 546a7a3 to e17992f Compare October 12, 2023 03:43

dndx approved these changes Oct 12, 2023

View reviewed changes

dndx merged commit 39a545d into master Oct 12, 2023
23 checks passed

dndx deleted the feat/request-aware-table branch October 12, 2023 04:33

ADD-SP mentioned this pull request Oct 12, 2023

perf(tracing/request-id): introduce $kong_request_id #11725

Merged

3 tasks

samugi mentioned this pull request Oct 12, 2023

fix(rate-limiting): revert request-aware-table usage #11746

Merged

3 tasks

chronolaw mentioned this pull request Oct 12, 2023

refactor(tools): use or to simplify table_new() #11749

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(request-aware-table) add request aware table #11017

feat(request-aware-table) add request aware table #11017

samugi commented Jun 6, 2023 •

edited

Loading

bungle commented Jun 7, 2023

samugi commented Jun 7, 2023 •

edited

Loading

AndyZhang0707 commented Sep 14, 2023

dndx Sep 26, 2023

samugi Sep 27, 2023 •

edited

Loading

dndx Sep 27, 2023

dndx Sep 26, 2023 •

edited

Loading

samugi Sep 27, 2023 •

edited

Loading

dndx Sep 27, 2023

samugi Sep 27, 2023

samugi Sep 27, 2023

samugi Sep 27, 2023

nowNick left a comment

feat(request-aware-table) add request aware table #11017

feat(request-aware-table) add request aware table #11017

Conversation

samugi commented Jun 6, 2023 • edited Loading

Summary

Checklist

Issue reference

bungle commented Jun 7, 2023

samugi commented Jun 7, 2023 • edited Loading

AndyZhang0707 commented Sep 14, 2023

dndx Sep 26, 2023

Choose a reason for hiding this comment

samugi Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

dndx Sep 27, 2023

Choose a reason for hiding this comment

dndx Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

samugi Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

dndx Sep 27, 2023

Choose a reason for hiding this comment

samugi Sep 27, 2023

Choose a reason for hiding this comment

samugi Sep 27, 2023

Choose a reason for hiding this comment

samugi Sep 27, 2023

Choose a reason for hiding this comment

nowNick left a comment

Choose a reason for hiding this comment

samugi commented Jun 6, 2023 •

edited

Loading

samugi commented Jun 7, 2023 •

edited

Loading

samugi Sep 27, 2023 •

edited

Loading

dndx Sep 26, 2023 •

edited

Loading

samugi Sep 27, 2023 •

edited

Loading