fix(ca_certificates): invalidate ca store caches when a ca cert is updated and prevent ca_certificates that are still being referenced by other entities from being deleted #11789

catbro666 · 2023-10-19T10:06:38Z

Summary

Currently, when a CA certificate is updated, the related CA certificate store caches won't be invalidated. And a CA certificate can be deleted even if it's still referenced by other entities. This PR fixes these issues.

link to #10120

Checklist

The Pull Request has tests
A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Full changelog

add :select_by_ca_certificate() method to kong.db.services and kong.db.plugins to select entities referencing a ca certificate.
add ca_certificates:delete(). Before deleting a ca certificate, check all the entities and plugins that may reference it and prevent deletion if any one is referencing it.
after updating a ca certificate, invalidate all the ca store caches that include this ca certificate.

Issue reference

Fix FTI-2060

kong/runloop/events.lua

kong/runloop/certificate.lua

updated and prevent ca_certificates that are still being referenced by other entities from being deleted. Fix [FTI-2060](https://konghq.atlassian.net/browse/FTI-2060)

ms2008

LGTM

tzssangglass · 2023-10-26T04:55:06Z

LGTM.

Just a question: the reference relationship of ca_certificates is established by traversing the full services data every time ca_certificates is updated. Will it become in the future:

Assuming there is no data in the database;
Incrementally update the reference relationship of ca_certificates every time an entity related to ca_certificates is created/updated/deleted.

catbro666 · 2023-10-26T07:40:23Z

@tzssangglass

Assuming there is no data in the database;

Maybe doing an initial traversal at the init phase instead.

Incrementally update the reference relationship of ca_certificates every time an entity related to ca_certificates is created/updated/deleted.

As mentioned in the ticket, this is another optional solution that will complicate the code a lot. Considering the CA certificates won't be updated frequently, not sure if it’s worthwhile.

BTW, the reason why we can't get the entities that reference ca_certificates efficiently is postgres doesn't support foreign key arrays. There was a proposed patch 6 years ago but it never got included because some major surgery is still required.

hanshuebner · 2023-10-26T13:03:24Z

BTW, the reason why we can't get the entities that reference ca_certificates efficiently is postgres doesn't support foreign key arrays.

This can be implemented with a before commit trigger that checks the relationship. This would be more efficient than traversing all certificates from the application end, in particular in large configurations.

kong/api/routes/ca_certificates.lua

flrgh · 2023-11-05T00:00:19Z

This can be implemented with a before commit trigger that checks the relationship. This would be more efficient than traversing all certificates from the application end, in particular in large configurations.

In the services case, I 100% agree. I would write a DELETE trigger function and call it a day. However, adapting the {{plugin}}.config.ca_certificates case to method is where it quickly gets more convoluted and starts to lose its appeal.

Idea: instead of performing all the filtering in Lua (expensive) or trying to enforce this constraint solely in the DB with schema/trigger changes (leaks plugin-specific business logic into the storage layer), what if we added some specialized :select_by_ca_certificate(cert_id) methods to kong.db.services and kong.db.plugins?

Something kinda like...

-- note: you might even be able to teach `kong/db/dao/init.lua` to
-- auto-generate this code if the type definition for the `ca_certificates`
-- field is annotated to reflect the foreign relationship.

function services:select_by_ca_certificate(cert_id)
  -- idk if this is correct at all
  local rows, err = self.strategy:query("SELECT * FROM services WHERE %s = ANY(ca_certificates)",
                                                  cert_id)
  -- ...
  return services
end

-- alternatively, one could craft a method that uses `SELECT COUNT(*) FROM ...`,
-- which would be more efficient given that we don't actually care about
-- retrieving the matching entities

This would negate the need to iterate and filter through all entities in Lua land, but it also avoids putting too much business logic into the DB.

team-gateway-bot · 2023-11-23T03:21:35Z