fix(core): remove and restore nulls before and after transformations #12284

outsinre · 2024-01-03T03:34:50Z

Current declarative config uncondtionally removes nulls before loading into LMDB, which would reset relavant config fields to their default values.

If their default values are nil and the code fails to dectect it, Kong will error! For example, config update may not be applied to core entities or plugins.

This PR removes nulls only if relevant schemas have transformations defined, and restore those nulls after transformation. Therefore, when loaded, entities preserve their nulls.

This fix does not prevent other components removing nulls accidentally. So please always check both nil and null for code robustness.

This PR will skip transformations if there is not transformation defitions. As most schemas do not have transformations, this would greatly improve performance.

Addtionally, this PR change recursive function to be iterative to boost performance.

Local debug info.

2024/01/04 11:36:11 [error] 38655#0: *2 [lua] import.lua:357: load_into_cache(): preserve-nulls config = {
  config = {
    large = <userdata 1>,
    request_header = "Hello-Foo",
    response_header = "Bye-Bar",
    ttl = <userdata 1>
  },
  consumer = <userdata 1>,
  consumer_group = <userdata 1>,
  created_at = 1704339371,
  enabled = true,
  id = "4833cef7-0628-502f-a417-84775ab7d49d",
  instance_name = <userdata 1>,
  name = "preserve-nulls",
  ordering = <userdata 1>,
  protocols = { "grpc", "grpcs", "http", "https",
    <metatable> = {}
  },
  route = {
    id = "2a69e506-6ca5-57db-8bda-fa910f3d2f17"
  },
  service = <userdata 1>,
  tags = <userdata 1>,
  updated_at = 1704339371,
  ws_id = "0dc6f45b-8f8d-40d2-a504-473544ee190b"
}, context: ngx.timer
2024/01/04 11:36:11 [error] 38655#0: *2 [lua] import.lua:526: load_into_cache(): preserve-nulls from lmdb = {
  config = {
    large = <userdata 1>,
    request_header = "Hello-Foo",
    response_header = "Bye-Bar",
    ttl = <userdata 1>
  },
  consumer = <userdata 1>,
  consumer_group = <userdata 1>,
  created_at = 1704339371,
  enabled = true,
  id = "4833cef7-0628-502f-a417-84775ab7d49d",
  instance_name = <userdata 1>,
  name = "preserve-nulls",
  ordering = <userdata 1>,
  protocols = { "grpc", "grpcs", "http", "https" },
  route = {
    id = "2a69e506-6ca5-57db-8bda-fa910f3d2f17"
  },
  service = <userdata 1>,
  tags = <userdata 1>,
  updated_at = 1704339371,
  ws_id = "0dc6f45b-8f8d-40d2-a504-473544ee190b"
}, context: ngx.timer
●
1 success / 0 failures / 0 errors / 0 pending : 1.197774 seconds
(kong-dev) 11:36:12 zachary@Zacharys-MacBook-Pro ~/workspace/kong-ee

Summary

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

Issue reference

Fix #FTI-5260

outsinre · 2024-01-03T03:36:56Z

kong/db/schema/init.lua

@@ -930,6 +930,7 @@ function Schema:validate_field(field, value)
    local field_schema = get_field_schema(field)
    -- TODO return nested table or string?
    local copy = field_schema:process_auto_fields(value, "insert")
+    -- Changes in custom entity check only applies to copy, not to value


Not sure if this is intended or bug. Leave a comment for reminder.

I think as this is validate_field function. The process auto fields is just executed so that the validation can pass or something. Thus I don't think we need to add this comment. Or if we add the comment, the comment should explain why copy, and not to point something that might be totally false. Thus I lean removing this comment as bad comment is worse than no comment at all. Perhaps a more generic "-- TODO: explain why we need to make a copy?"

Suggested change

-- Changes in custom entity check only applies to copy, not to value

Agree.

I replaced the comment with:

TODO: explain why we need to make a copy?"

The issue is, field_schema:validate(copy) may have config customizations. If so, the customizations only go to the copy, not the original value.

@outsinre, yes, the customizations I think are fine. This is validation time, the validation should not change the entitity I think. So that's why it is probably copied so that it does not change the original entity on validation. Perhaps it does the change on some other time.

outsinre · 2024-01-03T15:10:13Z

First CI green: https://github.com/Kong/kong/actions/runs/7397227994?pr=12284

kong/db/strategies/off/init.lua

outsinre · 2024-01-04T03:03:12Z

Second job green: https://github.com/Kong/kong/actions/runs/7399005978?pr=12284

outsinre · 2024-01-08T02:48:53Z

CI green 3rd time https://github.com/Kong/kong/actions/runs/7419465451/job/20189942923?pr=12284

bungle

Some change requests.

kong/db/declarative/import.lua

bungle · 2024-01-11T08:32:35Z

kong/db/schema/init.lua

@@ -930,6 +930,7 @@ function Schema:validate_field(field, value)
    local field_schema = get_field_schema(field)
    -- TODO return nested table or string?
    local copy = field_schema:process_auto_fields(value, "insert")
+    -- Changes in custom entity check only applies to copy, not to value


I think as this is validate_field function. The process auto fields is just executed so that the validation can pass or something. Thus I don't think we need to add this comment. Or if we add the comment, the comment should explain why copy, and not to point something that might be totally false. Thus I lean removing this comment as bad comment is worse than no comment at all. Perhaps a more generic "-- TODO: explain why we need to make a copy?"

Suggested change

-- Changes in custom entity check only applies to copy, not to value

spec/02-integration/20-wasm/09-filter-meta_spec.lua

Current declarative config uncondtionally removes nulls before loading into LMDB, which would reset relavant config fields to their default values. If their default values are `nil` and the code fails to dectect it, Kong will error! For example, config update may not be applied to core entities or plugins. This PR removes nulls only if relevant schemas have transformations defined, and restore those nulls after transformation. Therefore, when loaded, entities preserve their nulls. This fix does not prevent other components removing nulls accidentally. So please always check both `nil` and `null` for code robustness. This PR will skip transformations if there is not transformation defitions. As most schemas do not have transformations, this would greatly improve performance. Addtionally, this PR change recursive function to be iterative to boost performance. Signed-off-by: Zachary Hu <zachary.hu@konghq.com>

Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>

outsinre · 2024-01-15T08:30:14Z

4th CI green: https://github.com/Kong/kong/actions/runs/7524934481?pr=12284

spec/01-unit/01-db/11-declarative_lmdb_spec.lua

pull-request-size bot added the size/L label Jan 3, 2024

outsinre requested a review from hishamhm January 3, 2024 03:35

github-actions bot added core/db schema-change-noteworthy labels Jan 3, 2024

github-actions bot assigned outsinre Jan 3, 2024

outsinre requested a review from bungle January 3, 2024 03:35

outsinre commented Jan 3, 2024

View reviewed changes

outsinre force-pushed the FTI-5260/ce/fix-declaractive-config-null branch 3 times, most recently from 99b2929 to 7484fde Compare January 3, 2024 08:07

outsinre marked this pull request as draft January 3, 2024 08:31

outsinre force-pushed the FTI-5260/ce/fix-declaractive-config-null branch 3 times, most recently from d773d55 to 84ae9ee Compare January 3, 2024 12:26

outsinre marked this pull request as ready for review January 3, 2024 14:11

outsinre commented Jan 3, 2024

View reviewed changes

kong/db/strategies/off/init.lua Show resolved Hide resolved

kong/db/strategies/off/init.lua Show resolved Hide resolved

outsinre force-pushed the FTI-5260/ce/fix-declaractive-config-null branch from 4bd0659 to c8c93b7 Compare January 3, 2024 15:13

outsinre removed the request for review from hishamhm January 4, 2024 03:07

outsinre requested a review from flrgh January 8, 2024 02:59

bungle requested changes Jan 11, 2024

View reviewed changes

outsinre and others added 4 commits January 15, 2024 13:58

fix(core): remove unnecessary process_auto_fields on read time

4ca6abe

Update kong/db/declarative/import.lua

92d2921

Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>

chore(*): add a to-do comment on field validation

cfbfb5c

outsinre force-pushed the FTI-5260/ce/fix-declaractive-config-null branch from 5ed05f8 to cfbfb5c Compare January 15, 2024 05:58

bungle approved these changes Jan 15, 2024

View reviewed changes

outsinre removed the request for review from flrgh January 15, 2024 08:30

bungle approved these changes Jan 15, 2024

View reviewed changes

chobits reviewed Jan 17, 2024

View reviewed changes

spec/01-unit/01-db/11-declarative_lmdb_spec.lua Show resolved Hide resolved

tests(*): purge cache after creation

e52ba26

chobits approved these changes Jan 17, 2024

View reviewed changes

hanshuebner merged commit 2bfb15d into master Jan 18, 2024
23 checks passed

hanshuebner deleted the FTI-5260/ce/fix-declaractive-config-null branch January 18, 2024 02:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): remove and restore nulls before and after transformations #12284

fix(core): remove and restore nulls before and after transformations #12284

outsinre commented Jan 3, 2024 •

edited

Loading

outsinre Jan 3, 2024

bungle Jan 11, 2024 •

edited

Loading

outsinre Jan 15, 2024 •

edited

Loading

bungle Jan 15, 2024

outsinre commented Jan 3, 2024 •

edited

Loading

outsinre commented Jan 4, 2024

outsinre commented Jan 8, 2024

bungle left a comment

bungle Jan 11, 2024 •

edited

Loading

outsinre commented Jan 15, 2024

fix(core): remove and restore nulls before and after transformations #12284

fix(core): remove and restore nulls before and after transformations #12284

Conversation

outsinre commented Jan 3, 2024 • edited Loading

Summary

Checklist

Issue reference

outsinre Jan 3, 2024

Choose a reason for hiding this comment

bungle Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

outsinre Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

bungle Jan 15, 2024

Choose a reason for hiding this comment

outsinre commented Jan 3, 2024 • edited Loading

outsinre commented Jan 4, 2024

outsinre commented Jan 8, 2024

bungle left a comment

Choose a reason for hiding this comment

bungle Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

outsinre commented Jan 15, 2024

outsinre commented Jan 3, 2024 •

edited

Loading

bungle Jan 11, 2024 •

edited

Loading

outsinre Jan 15, 2024 •

edited

Loading

outsinre commented Jan 3, 2024 •

edited

Loading

bungle Jan 11, 2024 •

edited

Loading