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

fix(db/declarative): fix TTL not working in DB-less and Hybrid mode #11464

Merged
merged 28 commits into from
Sep 7, 2023

Conversation

chobits
Copy link
Contributor

@chobits chobits commented Aug 28, 2023

Summary

  • Previously, in DB-less and Hybrid mode, the ttl/updated_at fields were not copied from the original entities to the flattened entities. As a result, the entities were loaded without the TTL field.
  • Additionally, for loading the TTL field, the "off" DB strategy (lmdb) did not support automatic calculation of the remaining expiration time.

Checklist

Full changelog

  • copy the ttl field from original entities to flatten entities
  • transform the absolute ttl value to relative within the new function each_for_export(), then declarative config exports its configuration using for ... in each_for_export instead of for ... in each()
  • add DAO:each_for_export() based on DAO:each() to export the entity with the absolute ttl
    • add the options.export field to enable this feature in pg:page()
    • add extra pg SQL statements "page_XXX_for_export{_global}"
  • the off strategy:
    • turn the absolute ttl to relative for the upper user (cred.ttl) in off:select()/:page()
    • does not return the expired entity(cred) to the upper user, which makes it identical with the PG strategy.

Issue reference

Fix FTI-4512

@CLAassistant

This comment was marked as outdated.

@chobits

This comment was marked as resolved.

@chobits chobits requested review from StarlightIbuki and removed request for bungle August 28, 2023 03:57
@chobits chobits force-pushed the fix_ttl branch 2 times, most recently from 2ca8583 to 8e7daba Compare August 28, 2023 05:05
StarlightIbuki

This comment was marked as resolved.

kong/db/schema/others/declarative_config.lua Outdated Show resolved Hide resolved
kong/db/schema/others/declarative_config.lua Outdated Show resolved Hide resolved
kong/db/strategies/off/init.lua Outdated Show resolved Hide resolved
@chronolaw
Copy link
Contributor

We also need a change log entry.

@chronolaw chronolaw changed the title fix(declarative): fix TTL not working in DB-less and Hybrid mode fix(db/declarative): fix TTL not working in DB-less and Hybrid mode Aug 28, 2023
@pull-request-size pull-request-size bot added size/S and removed size/M labels Aug 28, 2023
@chobits chobits requested a review from bungle August 28, 2023 06:24
@pull-request-size pull-request-size bot added size/L and removed size/S labels Aug 29, 2023
@chobits

This comment was marked as outdated.

@chobits chobits marked this pull request as ready for review August 31, 2023 02:12
@@ -142,7 +142,7 @@ local function export_from_db_impl(emitter, skip_ws, skip_disabled_entities, exp
if db[name].pagination then
page_size = db[name].pagination.max_page_size
end
for row, err in db[name]:each(page_size, GLOBAL_QUERY_OPTS) do
for row, err in db[name]:each_for_export(page_size, GLOBAL_QUERY_OPTS) do
Copy link
Contributor Author

@chobits chobits Aug 31, 2023

Choose a reason for hiding this comment

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

As discussed with bungle, we have used :each_for_export here to export the entity with the absolute ttl while keeping original :each unchanged. :each will continue to give the relative.

CHANGELOG.md Outdated Show resolved Hide resolved
https://github.com/Kong/kong/blob/master/kong/db/strategies/postgres/init.lua
-> add each_for_export

https://github.com/Kong/kong/blob/master/kong/db/declarative/export.lua#L145
-> use :each_for_export

https://github.com/Kong/kong/blob/master/kong/db/declarative/import.lua#L167
-> ensure that ttl is properly stored in lmdb (absolute value)

https://github.com/Kong/kong/blob/master/kong/db/strategies/off/init.lua
-> make sure that TTL is respected (expired items are not returned), and
turn absolute ttl to relative, and ensure that it is returned (the
cred.ttl)
We have already introduced CHANGELOG/unreleased/kong/11464.yaml.
* sleep before attempting unnecessary requests
* decrease the ttl to expedite the case's execution
Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

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

Looks good!

@bungle bungle merged commit 48dc2ef into Kong:master Sep 7, 2023
23 checks passed
windmgc pushed a commit that referenced this pull request Jan 24, 2024
…11464)

* fix(declarative): fix TTL not working in DB-less and Hybrid mode

* Previously, in DB-less and Hybrid mode, the ttl/updated_at fields were
  not copied from the original entities to the flattened entities. As a
  result, the entities were loaded without the TTL field.
* Additionally, for loading the TTL field, the "off" DB strategy
  (lmdb) did not properly filter expired items, nor returned right
  TTL value for DAO.

FTI-4512

* fix coding style
* fix coding style: improved function name
* added test case: hybrid mode for key-auth
* fix test case warnings
* fixed test case consumer domain
* export ttl as absolute value
* delete unused defination
* move ttl-fixing logic into row_to_entity()
* still use pg to caculate relative value
* clean code
* add changelog entry
* fixed test cases
* fixed test cases warning
* fixed test failure
* fix test case issue: ttl expiration
* fix test case: unsed local variable
* add an entry in CHANGELOG.md
* fix changelog scope
* remove release-related information in CHANGELOG.md
* fix test case: sleep before attempting unnecessary requests
* sleep before attempting unnecessary requests
* decrease the ttl to expedite the case's execution
* fix CHANGELOG typo
* fix the tense problem of changelog entry
* add export options for "page_*_for_export" sql statement
* fix warning: setting non-standard global variable
* fix error reporting: options is nil
* fix an issue where the off strategy returned the expired entity
* run ttl processing before schema:process_auto_fields()
windmgc pushed a commit that referenced this pull request Mar 7, 2024
…11464)

* fix(declarative): fix TTL not working in DB-less and Hybrid mode

* Previously, in DB-less and Hybrid mode, the ttl/updated_at fields were
  not copied from the original entities to the flattened entities. As a
  result, the entities were loaded without the TTL field.
* Additionally, for loading the TTL field, the "off" DB strategy
  (lmdb) did not properly filter expired items, nor returned right
  TTL value for DAO.

FTI-4512

* fix coding style
* fix coding style: improved function name
* added test case: hybrid mode for key-auth
* fix test case warnings
* fixed test case consumer domain
* export ttl as absolute value
* delete unused defination
* move ttl-fixing logic into row_to_entity()
* still use pg to caculate relative value
* clean code
* add changelog entry
* fixed test cases
* fixed test cases warning
* fixed test failure
* fix test case issue: ttl expiration
* fix test case: unsed local variable
* add an entry in CHANGELOG.md
* fix changelog scope
* remove release-related information in CHANGELOG.md
* fix test case: sleep before attempting unnecessary requests
* sleep before attempting unnecessary requests
* decrease the ttl to expedite the case's execution
* fix CHANGELOG typo
* fix the tense problem of changelog entry
* add export options for "page_*_for_export" sql statement
* fix warning: setting non-standard global variable
* fix error reporting: options is nil
* fix an issue where the off strategy returned the expired entity
* run ttl processing before schema:process_auto_fields()
windmgc pushed a commit that referenced this pull request Mar 8, 2024
…11464)

* fix(declarative): fix TTL not working in DB-less and Hybrid mode

* Previously, in DB-less and Hybrid mode, the ttl/updated_at fields were
  not copied from the original entities to the flattened entities. As a
  result, the entities were loaded without the TTL field.
* Additionally, for loading the TTL field, the "off" DB strategy
  (lmdb) did not properly filter expired items, nor returned right
  TTL value for DAO.

FTI-4512

* fix coding style
* fix coding style: improved function name
* added test case: hybrid mode for key-auth
* fix test case warnings
* fixed test case consumer domain
* export ttl as absolute value
* delete unused defination
* move ttl-fixing logic into row_to_entity()
* still use pg to caculate relative value
* clean code
* add changelog entry
* fixed test cases
* fixed test cases warning
* fixed test failure
* fix test case issue: ttl expiration
* fix test case: unsed local variable
* add an entry in CHANGELOG.md
* fix changelog scope
* remove release-related information in CHANGELOG.md
* fix test case: sleep before attempting unnecessary requests
* sleep before attempting unnecessary requests
* decrease the ttl to expedite the case's execution
* fix CHANGELOG typo
* fix the tense problem of changelog entry
* add export options for "page_*_for_export" sql statement
* fix warning: setting non-standard global variable
* fix error reporting: options is nil
* fix an issue where the off strategy returned the expired entity
* run ttl processing before schema:process_auto_fields()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants