fix(schema): deprecated shorthand fields precedence #13486

bungle · 2024-08-09T13:12:25Z

Summary

The shorthand fields e.g. as used in (the url is a shorthand) take precedence:

http PUT :8001/services/test url=http://test.org/ port=2345

That means that port=2345 will be overwritten by http default port of 80 that is implicit in shorthand field url.

This is how it has been for a long long time.

In PR #12686 we added deprecation field to shorthand fields definition.

Some of our automatic migrations without database migrations magic use this functionality, but the precedence there does not good:

http -f PUT :8001/plugins/x name=x config.new=test config.old=should_be_ignored

In above the config.old is a shorthand field with a deprecation property. Thus it should not overwrite the config.new, but in current code base it does. This PR changes it so that it doesn't do it anymore.

KAG-5134 and https://kongstrong.slack.com/archives/C07AQH7SAF8/p1722589141558609

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

bungle · 2024-08-09T13:20:17Z

The actual fix is in this commit:
d77324a

nowNick · 2024-08-12T08:52:52Z

kong/db/dao/plugins.lua

+    if (entity.protocols and not entity.route)
+    or (entity.service and not entity.protocols)
+    or (entity.route and not entity.protocols)


What's the idea behind this logic? 🤔 Why protocols and routes are XORed etc.?

same question: shouldn't we check for "protocol match" even when both e.g. protocols and route were updated?

@nowNick, @samugi, yes we are checking, but in that case we are skipping RBW because we have information already to do the check.

you're right I misinterpreted the scope of that if block, my bad.

nowNick · 2024-08-12T08:53:58Z

kong/db/schema/init.lua

            for k, v in pairs(new_values) do
              if type(v) == "table" then
-                data[k] = tablex.merge(data[k] or {}, v, true)
-              else
+                data[k] = deprecation and table_merge(v, data[k])


Maybe let's add a comment here to explain why deprecation is tied to shorthands?

I am fine with asks like these. It is just that simple ask like this can create considerable amounts of work. Like restarting EE tests 10 times.

A comment was added.

samugi · 2024-08-12T09:28:35Z

kong/db/dao/plugins.lua

+    if (entity.protocols and not entity.route)
+    or (entity.service and not entity.protocols)
+    or (entity.route and not entity.protocols)


same question: shouldn't we check for "protocol match" even when both e.g. protocols and route were updated?

samugi · 2024-08-12T09:31:41Z

kong/api/routes/plugins.lua

are the changes in this file related to the fix? I am not sure I understand what changed here.

@samugi again, they are checking, just that we can skip additional RBW (read-before-write) -> we don't need to read database as the information that is needed for the check is already there.

The changes in this file are related to fix. But it also contains optimization (aka no rbw when not needed) - the generic DAO will do RBW on updates always (too). Actually on updates on admin API the admin api does RBW to read plugin name to get correct schema, the DAO does for protocols check and generic dao does it too. The middle one is optimized here to only happen when needed.

kong/db/schema/init.lua

…ome functions Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>

### Summary The shorthand fields e.g. as used in (the url is a shorthand) take precedence: ``` http PUT :8001/services/test url=http://test.org/ port=2345 ``` That means that `port=2345` will be overwritten by http default port of `80` that is implicit in shorthand field `url`. This is how it has been for a long long time. In PR #12686 we added `deprecation` field to shorthand fields definition. Some of our automatic migrations without database migrations magic use this functionality, but the precedence there does not good: ``` http -f PUT :8001/plugins/x name=x config.new=test config.old=should_be_ignored ``` In above the `config.old` is a shorthand field with a deprecation property. Thus it should not overwrite the `config.new`, but in current code base it does. This PR changes it so that it doesn't do it anymore. KAG-5134 and https://kongstrong.slack.com/archives/C07AQH7SAF8/p1722589141558609 Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>

team-gateway-bot · 2024-08-12T14:52:34Z

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-13486-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-13486-to-master-to-upstream
git checkout -b cherry-pick-13486-to-master-to-upstream
ancref=$(git merge-base 0960f882a944447df02f32390142447d95192d3e c8b1604fb71fdab969bea6f261c12b7a249d058f)
git cherry-pick -x $ancref..c8b1604fb71fdab969bea6f261c12b7a249d058f

bungle · 2024-08-12T14:53:06Z

The EE PR is here:
https://github.com/Kong/kong-ee/pull/9932

pull-request-size bot added the size/M label Aug 9, 2024

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

github-actions bot assigned bungle Aug 9, 2024

bungle force-pushed the fix/deprecation-merge-order branch from 8efd700 to 8275080 Compare August 9, 2024 13:17

bungle marked this pull request as ready for review August 9, 2024 13:18

bungle requested review from nowNick and samugi August 9, 2024 13:18

bungle force-pushed the fix/deprecation-merge-order branch from 8275080 to a123edc Compare August 9, 2024 15:26

github-actions bot added the core/admin-api label Aug 9, 2024

bungle force-pushed the fix/deprecation-merge-order branch 2 times, most recently from 711b888 to b077ce0 Compare August 9, 2024 16:18

pull-request-size bot added size/L and removed size/M labels Aug 9, 2024

bungle force-pushed the fix/deprecation-merge-order branch 4 times, most recently from 60ffb5e to d77324a Compare August 9, 2024 21:15

bungle changed the title ~~fix(scheme): deprecated shorthand fields precedence~~ fix(schema): deprecated shorthand fields precedence Aug 9, 2024

bungle added this to the 3.8.0 milestone Aug 10, 2024

nowNick reviewed Aug 12, 2024

View reviewed changes

samugi reviewed Aug 12, 2024

View reviewed changes

outsinre approved these changes Aug 12, 2024

View reviewed changes

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

chore(schema): use kong.tools.table instead of tablex, and localize s…

b50cf86

…ome functions Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>

bungle force-pushed the fix/deprecation-merge-order branch 3 times, most recently from 9b66421 to df32e55 Compare August 12, 2024 13:18

samugi approved these changes Aug 12, 2024

View reviewed changes

bungle force-pushed the fix/deprecation-merge-order branch from df32e55 to c8b1604 Compare August 12, 2024 13:21

bungle merged commit c2e9f99 into master Aug 12, 2024
27 checks passed

bungle deleted the fix/deprecation-merge-order branch August 12, 2024 14:52

bungle removed the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Aug 12, 2024

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

bungle removed the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Aug 12, 2024

AndyZhang0707 mentioned this pull request Aug 21, 2024

docs(changelog): move changelog files into the correct location #13534

Merged

3 tasks

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

[backport -> release/3.8.x] docs(changelog): move changelog files into the correct location #13535

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(schema): deprecated shorthand fields precedence #13486

fix(schema): deprecated shorthand fields precedence #13486

bungle commented Aug 9, 2024 •

edited

Loading

bungle commented Aug 9, 2024 •

edited

Loading

nowNick Aug 12, 2024

samugi Aug 12, 2024

bungle Aug 12, 2024 •

edited

Loading

samugi Aug 12, 2024

nowNick Aug 12, 2024

bungle Aug 12, 2024

bungle Aug 12, 2024

samugi Aug 12, 2024

samugi Aug 12, 2024

bungle Aug 12, 2024 •

edited

Loading

team-gateway-bot commented Aug 12, 2024

bungle commented Aug 12, 2024

fix(schema): deprecated shorthand fields precedence #13486

fix(schema): deprecated shorthand fields precedence #13486

Conversation

bungle commented Aug 9, 2024 • edited Loading

Summary

Checklist

bungle commented Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bungle Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bungle Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

team-gateway-bot commented Aug 12, 2024

bungle commented Aug 12, 2024

bungle commented Aug 9, 2024 •

edited

Loading

bungle commented Aug 9, 2024 •

edited

Loading

bungle Aug 12, 2024 •

edited

Loading

bungle Aug 12, 2024 •

edited

Loading