Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(utils): add ClearUnmatchingDeprecations function to align configs #473

Merged
merged 5 commits into from
Nov 18, 2024

Conversation

nowNick
Copy link
Contributor

@nowNick nowNick commented Oct 22, 2024

Summary

When a new plugin configuration is sent that contains information about some deprecated fields in order to produce
correct diff we need to align the API response with given config.

Description

This PR aims to align the configs by doing two things:

  1. Adding: ClearUnmatchingDeprecations
  2. Modifying: fillConfigRecord

More details can be found in the functions documentation.

Important changes:

  • nil values are preserved and not changed when fillConfigRecord is run. Previously a config cluster_max_redirections: null would be "filled" with cluster_max_redirections: 5 if cluster_max_redirections had a default value of 5 defined. Right now sending explicit null preserves that. This reverts the change introduced in: https://github.com/Kong/go-kong/pull/309/files#diff-af8350ebad15aeeb0a2561433d1d0907140df8e5b8267a0d0f05bb53d0d53fcbR221-R249
  • shorthand values are not backfilled. If a new config contains only new fields fillConfigRecord will not try to fill deprecated fields in that config
  • if a deprecated field was defined fillConfigRecord will not populate the new fields that are linked to that deprecated field

Related PRs:

KAG-5577

@CLAassistant
Copy link

CLAassistant commented Oct 22, 2024

CLA assistant check
All committers have signed the CLA.

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 93.98907% with 11 lines in your changes missing coverage. Please review.

Project coverage is 55.15%. Comparing base (8678be1) to head (8b77ac2).

Files with missing lines Patch % Lines
kong/utils.go 93.98% 8 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #473      +/-   ##
==========================================
+ Coverage   54.04%   55.15%   +1.11%     
==========================================
  Files          71       71              
  Lines        5599     5691      +92     
==========================================
+ Hits         3026     3139     +113     
+ Misses       1954     1935      -19     
+ Partials      619      617       -2     
Flag Coverage Δ
2.1 33.38% <44.26%> (-0.16%) ⬇️
2.2 43.85% <44.26%> (-0.33%) ⬇️
2.3 44.42% <44.26%> (-0.34%) ⬇️
2.4 44.49% <44.26%> (-0.34%) ⬇️
2.5 44.49% <44.26%> (-0.34%) ⬇️
2.6 44.49% <44.26%> (-0.34%) ⬇️
2.7 45.96% <44.26%> (-0.37%) ⬇️
2.8 45.96% <44.26%> (-0.37%) ⬇️
3.0 49.51% <44.26%> (-0.43%) ⬇️
3.1 50.85% <44.26%> (-0.45%) ⬇️
3.2 50.85% <44.26%> (-0.45%) ⬇️
3.3 50.85% <44.26%> (-0.45%) ⬇️
3.4 52.87% <44.26%> (-0.48%) ⬇️
3.5 51.02% <44.26%> (-0.45%) ⬇️
3.6 51.02% <44.26%> (-0.45%) ⬇️
3.7 51.02% <44.26%> (-0.45%) ⬇️
3.8 52.62% <93.98%> (+1.15%) ⬆️
community 41.60% <91.80%> (+1.26%) ⬆️
enterprise 53.75% <93.98%> (+1.13%) ⬆️
integration 55.15% <93.98%> (+1.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@nowNick nowNick force-pushed the fix/clear-unmatching-deprecated-fields branch 2 times, most recently from f2e13bd to 6e4a200 Compare October 23, 2024 16:35
@Kong Kong deleted a comment from CLAassistant Oct 23, 2024
@nowNick nowNick force-pushed the fix/clear-unmatching-deprecated-fields branch 2 times, most recently from df36ae0 to a7dc20e Compare October 23, 2024 16:49
@nowNick nowNick marked this pull request as ready for review October 23, 2024 17:05
@nowNick nowNick requested review from a team as code owners October 23, 2024 17:05
@nowNick nowNick self-assigned this Oct 23, 2024
kong/utils.go Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
kong/utils.go Show resolved Hide resolved
kong/utils.go Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
@nowNick nowNick force-pushed the fix/clear-unmatching-deprecated-fields branch 2 times, most recently from 836ef7b to 012122b Compare October 25, 2024 18:36
@nowNick nowNick marked this pull request as draft October 25, 2024 18:36
@nowNick
Copy link
Contributor Author

nowNick commented Oct 25, 2024

Thanks @Prashansa-K for the review. For now I've changed this PR back to draft because I've found a couple of issues.

@nowNick
Copy link
Contributor Author

nowNick commented Oct 29, 2024

Ok the corresponding PR is green and I think I've tackled the edge cases I've discovered (in the tests in this PR and the other one: Kong/go-database-reconciler#145 ). I think it's ready to be reviewed again.

@nowNick nowNick marked this pull request as ready for review October 29, 2024 13:53
@nowNick nowNick requested a review from samugi October 30, 2024 08:53
kong/utils.go Outdated Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
@nowNick nowNick dismissed Prashansa-K’s stale review October 31, 2024 07:39

The requested changes were addressed and incorporated.

@nowNick nowNick force-pushed the fix/clear-unmatching-deprecated-fields branch 4 times, most recently from 73b25d6 to f936e44 Compare November 7, 2024 12:58
@nowNick nowNick marked this pull request as ready for review November 7, 2024 15:07
@nowNick
Copy link
Contributor Author

nowNick commented Nov 7, 2024

I did some important changes to fillConfigRecord. @GGabriele @samugi could you guys take a look at it again? I've tried to document it as much as possible to ease reviewing process.

Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments on Go style etc.

kong/utils.go Outdated Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
When a new plugin configuration is sent that contains information
about some deprecated fields in order to produce correct diff
we need to align the API response with given config.

KAG-5577
@nowNick nowNick force-pushed the fix/clear-unmatching-deprecated-fields branch from f936e44 to c010578 Compare November 8, 2024 15:52
@nowNick nowNick requested a review from pmalek November 8, 2024 16:26
@Prashansa-K
Copy link
Collaborator

I tested these changes locally with an openid plugin (deck file added here)and noticed the following:

On gateway 3.8.1.0
No unnecessary diffs come up, except

+    "session_compressor": "none"
+    "session_cookie_maxsize": 4000
+    "session_cookie_renew": 600
+    "session_strategy": "default"

These fields don't have any translate_backwards or replaced_with property in the schema. So, not applicable with this change.

However, on Konnect, I noticed some diffs:

-    "session_redis_cluster_max_redirections": 5,
+    "session_redis_cluster_max_redirections": null,

-    "session_redis_connect_timeout": 2000,
+    "session_redis_connect_timeout": null,
    
-    "session_redis_read_timeout": 2000,
+    "session_redis_read_timeout": null,
-    "session_redis_send_timeout": 2000,
+    "session_redis_send_timeout": null,

decK and reconciler logic remains same for Konnect as far as I know. Is there anything different schema-wise for Konnect that could be causing this?

kong/utils.go Show resolved Hide resolved
@nowNick
Copy link
Contributor Author

nowNick commented Nov 12, 2024

@Prashansa-K it seems that it's a well known Koko issue. The reason why you see these diffs in Konnect is that Lua-CP and Koko treat null values differently. LuaCP preserves them but Koko falls back to default value if defined.

@nowNick nowNick force-pushed the fix/clear-unmatching-deprecated-fields branch from 83a17ed to 20c546e Compare November 13, 2024 13:09
… align configs

Fix for when nil config values are passed
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments. Other than that this looks solid.

One thought though: could we add an integration test that checks against the schema we receive from Gateway? Perhaps we can use

func (s *PluginService) GetFullSchema(ctx context.Context,
to get the schema and then use that in the test? We can use RunWhenKong() to constrain the versions against which we run these tests.

kong/utils.go Outdated Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
kong/utils_test.go Outdated Show resolved Hide resolved
Copy link
Member

@samugi samugi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I verified the "important changes" described in the summary, LGTM

…configs

PR Review - styling fixes, switch to integration tests
@nowNick
Copy link
Contributor Author

nowNick commented Nov 18, 2024

@pmalek I've made the changes according to your comments and I've switched to calling Kong API in tests (didn't know we can do this from go-kong). Does it look better now? (feel free to check the latest commit - I didn't force-push)

Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nits about the testing.

kong/utils_test.go Outdated Show resolved Hide resolved
kong/utils_test.go Outdated Show resolved Hide resolved
kong/utils_test.go Outdated Show resolved Hide resolved
kong/utils_test.go Outdated Show resolved Hide resolved
kong/utils_test.go Outdated Show resolved Hide resolved
kong/utils.go Outdated Show resolved Hide resolved
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@nowNick nowNick merged commit dc0aa75 into main Nov 18, 2024
91 checks passed
@nowNick nowNick deleted the fix/clear-unmatching-deprecated-fields branch November 18, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants