kong icon indicating copy to clipboard operation
kong copied to clipboard

Caching/DAO: incorrect cache key computation at warmup (init phase) for unworkspaceable entities

Open marc-charpentier opened this issue 3 years ago • 1 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

Kong version ($ kong version)

2.8.1

Current Behavior

Hello, In a process of upgrading from Kong OSS 1.5.1 to 2.8.1, I found that our custom plugin's entities were no longer correctly warmed up, because the cache key computed at init phase was no longer the same as the one expected in request context. As a result, a database select is performed in request context, adding extra 50-100 ms to kong latency during access phase. Investigations involving numerous traces addition revealed that despite our custom entities' db table not supporting workspaces, the default workspace id is used to compute their cache key at warmup, whereas it is not in request context. Investigating further, I spotted another cause for this problem, and both are located in the kong/db/dao/init.lua file. The first one I spotted is in the DAO:cache_key function : if parameter key is a string and ws_id parameter is nil (as in request context in our case), the default workspace id isn't used, and if key is a table (as in warmup context while performing a 'find all' in db table), as a workspace id is present in the entity to cache (whatever its workspaceable status is), it is used :

[...]
  -- NOT IN CODE COMMENT:
  -- Here, key is a table (entity) but key.ws_id seems never to be nil
  if key.ws_id then
    ws_id = key.ws_id
  end
[...]

This last behavior led me to the second cause : when loading an entity from database, the DAO:row_to_entity function is executed and the following is performed in order to support blue-green migrations :

[...]
  -- NOT IN CODE COMMENT: row.ws_id is nil if there's no ws_id column in table
  local ws_id = row.ws_id
[...]
    entity.ws_id = ws_id

    -- special behavior for blue-green migrations
    -- NOT IN CODE COMMENTS:
    -- Is it intended to assign entity.ws_id with kong.default_workspace if ws_id is nil,
    -- whatever self.schema.workspaceable's value ?
    -- Or is it a logical operator precedence mistake ?
    if self.schema.workspaceable and ws_id == null or ws_id == nil then
     entity.ws_id = kong.default_workspace
    end
[...]

Do you confirm the two aforementioned behaviors are not intended as they are, or am I mistaken ? If mistaken, is it mandatory that our custom entities be migrated toward workspaceable model ? Else, I already have two fixes ready to be pushed in my forked repository in order to make a PR if agreed.

Thanks for taking a look.

Expected Behavior

In the way I understand workspaces :

  • Workspace id should not be taken into account in cache key computation for not workspaceable entities.
  • Workspace id should not be assigned a default value for not workspaceable entities.

Is it right ? Or must custom entities support workspaceability ?

Thank you.

Steps To Reproduce

Can be reproduced in our own environments only.

Anything else?

Kong running with a Cassandra database, configured to warmup (cache) custom entities.

marc-charpentier avatar Jun 16 '22 12:06 marc-charpentier

This looks like a fixed issue: #8553

But perhaps you need to migrate to 3.x to have this fixed.

StarlightIbuki avatar Feb 20 '23 09:02 StarlightIbuki

Dear contributor, We're closing this issue as there hasn't been any update to it for a long time. If the issue is still relevant in the latest version, please feel free to reopen it. We're more than happy to revisit it again. Your contribution is greatly appreciated! Please have a look at our pledge to the community for more information. Sincerely, Kong Gateway Team

StarlightIbuki avatar Oct 18 '23 08:10 StarlightIbuki