fix(*): reject config if both deprecated and new field defined and their values mismatch #13565

nowNick · 2024-08-23T17:11:28Z

Summary

This PR adds a validation to deprecated fields that checks the case in which both new field and old field were defined; when that happens their values must match.

It introduces a new structure to deprecation definition by adding replaced_with information. This way field validation can verify if the replaced_with field has the same value as the old deprecated field. By default the comparison is done via translation through simple path but there might be instances where translation from old to new is more complicated and then the translation function should be provided in translation field.

The plan is to remove translate_backwards field in the future in favor of replaced_with.

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

KAG-5262

kong/db/schema/init.lua

nowNick · 2024-08-27T09:22:28Z

I jumped the gun a little bit - let me add one more test 👀

bungle

Just asking.

bungle · 2024-08-27T12:03:47Z

kong/plugins/acme/schema.lua

    translate_backwards = {'server_name'},
    deprecation = {
+      replaced_with = { { path = { 'server_name' } } },


We have translate_backwards and now replaced_with. what is the difference here and what is this path = ..., there is no path = with translate_backwards. Can we use single thing for both?

The plan is to use only one of them - I want to get rid of translate_backwards - however it would require quite a lot code removal so I wanted to get it in another PR. It's going to be a refactor PR so it's not going to block the release.
This addition - although it's a little bit of code duplication I think it's just safer / quicker path. I'll prepare cleanup PRs once this one is merged - I promise 😄

As a bonus I think we can even automate the message - I'm planning to still allow custom message but the way right now it's defined is rather repetitive and it looks like we have all the knowledge we need to generate those deprecation warnings automatically.

Plus... the more I work with it the more I hate the name translate_backwards - it's just confusing. replaced_with inside deprecation right now makes more sense to me.

The plan is to use only one of them

If both translate_backwards and replaced_with can fix the bug, we'd better stick with the translate_backwards and avoid introducing an extra metaschema in this PR.

The refactor and introduction of replaced_with can be made in another PR.

Unfortunately we cannot use only one of the since translate_backwards is a single just a table path and we need to have an array of table paths. The reason is that one field can be replaced by a few fields ex: redis.timeout and redis.connect_timeout / redis.send_timeout / redis.read_timeout.
We need to have a way to check against all 3 of these.

I've added a test case just for that. Maybe it explains it a little bit better: https://github.com/Kong/kong/pull/13565/files#diff-be157ebd3368ec7ae7c30ab60332aebbd18f627ec4fa680156e6d6cd1706f668R4233

kong/db/schema/init.lua

nowNick · 2024-08-28T09:00:41Z

I'm changing this back to draft since we've found some issues with this approach and gateway compatibility between the upcoming 3.8 and older versions. Feel free to review this PR but please hold off merging.

kong/db/schema/init.lua

…eir values mismatch This commit adds a validation to deprecated fields that checks if in case both new field and old field were defined - their values must match. Note that if one of the fields is null then the validation passes even if the other is not null. KAG-5262

jschmid1

1/n

kong/db/schema/init.lua

kong/db/schema/metaschema.lua

kong/db/schema/init.lua

… and their values mismatch PR fixes

StarlightIbuki

I'm OK with the current version

kikito

Approved with a small possible perf optimization when an error is detected

kong/db/schema/init.lua

kong/db/schema/metaschema.lua

… and their values mismatch PR fixes 2

…eir values mismatch (#13565) * fix(*): reject config if both deprecated and new field defined and their values mismatch This commit adds a validation to deprecated fields that checks if in case both new field and old field were defined - their values must match. Note that if one of the fields is null then the validation passes even if the other is not null. KAG-5262 * fixup! fix(*): reject config if both deprecated and new field defined and their values mismatch PR fixes * fixup! fix(*): reject config if both deprecated and new field defined and their values mismatch PR fixes 2 (cherry picked from commit d10408c)

team-gateway-bot · 2024-08-29T14:57:18Z

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-13565-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-13565-to-master-to-upstream
git checkout -b cherry-pick-13565-to-master-to-upstream
ancref=$(git merge-base 83de84393fbd7445cb68d72dea36d24890b2fc3a 125c653914ce0eb4f242fdc02ac8eb3dc35cb5c3)
git cherry-pick -x $ancref..125c653914ce0eb4f242fdc02ac8eb3dc35cb5c3

team-gateway-bot · 2024-08-29T14:57:22Z

Successfully created backport PR for release/3.8.x:

[backport -> release/3.8.x] fix(*): reject config if both deprecated and new field defined and their values mismatch #13594

…eir values mismatch (Kong#13565) * fix(*): reject config if both deprecated and new field defined and their values mismatch This commit adds a validation to deprecated fields that checks if in case both new field and old field were defined - their values must match. Note that if one of the fields is null then the validation passes even if the other is not null. KAG-5262 * fixup! fix(*): reject config if both deprecated and new field defined and their values mismatch PR fixes * fixup! fix(*): reject config if both deprecated and new field defined and their values mismatch PR fixes 2

pull-request-size bot added the size/L label Aug 23, 2024

github-actions bot assigned nowNick Aug 23, 2024

github-actions bot added plugins/rate-limiting core/db schema-change-noteworthy cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Aug 23, 2024

nowNick force-pushed the fix/reject-config-when-deprecated-field-mismatch-new-field branch from 27cab5b to 493588c Compare August 23, 2024 17:11

nowNick added the skip-changelog label Aug 23, 2024

bungle requested changes Aug 26, 2024

View reviewed changes

kong/db/schema/init.lua Outdated Show resolved Hide resolved

github-actions bot added plugins/acme plugins/response-ratelimiting labels Aug 26, 2024

nowNick force-pushed the fix/reject-config-when-deprecated-field-mismatch-new-field branch from 9621432 to c24de51 Compare August 27, 2024 07:16

nowNick added this to the 3.8.0 milestone Aug 27, 2024

nowNick added the backport release/3.8.x label Aug 27, 2024

nowNick marked this pull request as ready for review August 27, 2024 09:09

nowNick marked this pull request as draft August 27, 2024 09:22

nowNick force-pushed the fix/reject-config-when-deprecated-field-mismatch-new-field branch from c24de51 to 935ca82 Compare August 27, 2024 09:31

nowNick requested a review from bungle August 27, 2024 09:56

nowNick marked this pull request as ready for review August 27, 2024 09:56

nowNick requested a review from samugi August 27, 2024 09:56

nowNick force-pushed the fix/reject-config-when-deprecated-field-mismatch-new-field branch from 935ca82 to ea61bad Compare August 27, 2024 09:59

nowNick removed the skip-changelog label Aug 27, 2024

This comment was marked as resolved.

Sign in to view

bungle reviewed Aug 27, 2024

View reviewed changes

AndyZhang0707 requested review from windmgc, outsinre, ADD-SP and StarlightIbuki August 28, 2024 06:26

outsinre reviewed Aug 28, 2024

View reviewed changes

kong/db/schema/init.lua Outdated Show resolved Hide resolved

kong/db/schema/init.lua Outdated Show resolved Hide resolved

nowNick marked this pull request as draft August 28, 2024 08:59

StarlightIbuki reviewed Aug 29, 2024

View reviewed changes

kong/db/schema/init.lua Outdated Show resolved Hide resolved

kong/db/schema/init.lua Outdated Show resolved Hide resolved

nowNick force-pushed the fix/reject-config-when-deprecated-field-mismatch-new-field branch 2 times, most recently from 537cbab to a3ea812 Compare August 29, 2024 10:03

nowNick force-pushed the fix/reject-config-when-deprecated-field-mismatch-new-field branch from a3ea812 to eabcd44 Compare August 29, 2024 10:17

nowNick marked this pull request as ready for review August 29, 2024 10:49

jschmid1 reviewed Aug 29, 2024

View reviewed changes

kong/db/schema/init.lua Outdated Show resolved Hide resolved

kong/db/schema/metaschema.lua Outdated Show resolved Hide resolved

kong/db/schema/init.lua Outdated Show resolved Hide resolved

fixup! fix(*): reject config if both deprecated and new field defined…

e69e9fb

… and their values mismatch PR fixes

nowNick force-pushed the fix/reject-config-when-deprecated-field-mismatch-new-field branch from 2360ef6 to e69e9fb Compare August 29, 2024 12:22

StarlightIbuki approved these changes Aug 29, 2024

View reviewed changes

jschmid1 approved these changes Aug 29, 2024

View reviewed changes

kikito approved these changes Aug 29, 2024

View reviewed changes

kong/db/schema/init.lua Outdated Show resolved Hide resolved

bungle reviewed Aug 29, 2024

View reviewed changes

kong/db/schema/metaschema.lua Show resolved Hide resolved

fixup! fix(*): reject config if both deprecated and new field defined…

125c653

… and their values mismatch PR fixes 2

bungle approved these changes Aug 29, 2024

View reviewed changes

kikito merged commit d10408c into master Aug 29, 2024
28 checks passed

kikito deleted the fix/reject-config-when-deprecated-field-mismatch-new-field branch August 29, 2024 14:57

team-gateway-bot mentioned this pull request Aug 29, 2024

[backport -> release/3.8.x] fix(*): reject config if both deprecated and new field defined and their values mismatch #13594

Merged

3 tasks

github-actions bot added the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Aug 29, 2024

nowNick mentioned this pull request Sep 2, 2024

refactor(shorthand_fields): remove translate_backwards in favor of replaced_with #13604

Merged

3 tasks

kikito removed the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(*): reject config if both deprecated and new field defined and their values mismatch #13565

fix(*): reject config if both deprecated and new field defined and their values mismatch #13565

nowNick commented Aug 23, 2024 •

edited by kikito

Loading

nowNick commented Aug 27, 2024

This comment was marked as resolved.

bungle left a comment

bungle Aug 27, 2024

nowNick Aug 27, 2024

nowNick Aug 27, 2024

outsinre Aug 29, 2024

nowNick Aug 29, 2024

nowNick Aug 29, 2024

nowNick commented Aug 28, 2024

jschmid1 left a comment

StarlightIbuki left a comment

kikito left a comment

team-gateway-bot commented Aug 29, 2024

team-gateway-bot commented Aug 29, 2024

fix(*): reject config if both deprecated and new field defined and their values mismatch #13565

fix(*): reject config if both deprecated and new field defined and their values mismatch #13565

Conversation

nowNick commented Aug 23, 2024 • edited by kikito Loading

Summary

Checklist

Issue reference

nowNick commented Aug 27, 2024

This comment was marked as resolved.

bungle left a comment

Choose a reason for hiding this comment

bungle Aug 27, 2024

Choose a reason for hiding this comment

nowNick Aug 27, 2024

Choose a reason for hiding this comment

nowNick Aug 27, 2024

Choose a reason for hiding this comment

outsinre Aug 29, 2024

Choose a reason for hiding this comment

nowNick Aug 29, 2024

Choose a reason for hiding this comment

nowNick Aug 29, 2024

Choose a reason for hiding this comment

nowNick commented Aug 28, 2024

jschmid1 left a comment

Choose a reason for hiding this comment

StarlightIbuki left a comment

Choose a reason for hiding this comment

kikito left a comment

Choose a reason for hiding this comment

team-gateway-bot commented Aug 29, 2024

team-gateway-bot commented Aug 29, 2024

nowNick commented Aug 23, 2024 •

edited by kikito

Loading