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.

Sign up for GitHub

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

Jump to bottom

refactor(db/schema): improve expression validation #10100

Merged
merged 27 commits into from
Jul 6, 2023
Merged

refactor(db/schema): improve expression validation #10100

merged 27 commits into from
Jul 6, 2023

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Jan 11, 2023 •
edited
Loading

Summary

Why:

  • The atc-router library updates its build files, we should update too
  • The new validate API in atc-router could improve the validation performance.
  • The new rust.regex crate supports special escape, which will case some tests cases fail
  • The code in the schema is a bit hard to understand

When the atc-router releases a new version, this PR will be ready to merge.

KAG-1524

Checklist

Full changelog

  • localize atc-router module
  • cache atc-router matcher
  • rename to validate_entity_by_expression and change entrance
  • constant PATH_V1_DEPRECATION_MSG
  • check paths only in traditional_compatible
  • fix some regex path test cases for rust regex bump to 1.8
  • fix bazel build for latest atc-router updating

StarlightIbuki
StarlightIbuki reviewed Jan 16, 2023
View reviewed changes
Copy link
Contributor

@StarlightIbuki StarlightIbuki left a comment

Choose a reason for hiding this comment

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

Good to go after renaming is done.

kong/db/schema/entities/routes.lua Outdated Show resolved Hide resolved
kong/db/schema/entities/routes.lua Outdated Show resolved Hide resolved
@chronolaw chronolaw force-pushed the style/cache_atc_router_matcher branch 2 times, most recently from e17a2cc to 023f4ff Compare January 22, 2023 06:02
@chronolaw chronolaw marked this pull request as draft February 13, 2023 05:20
@chronolaw chronolaw force-pushed the style/cache_atc_router_matcher branch from 03e3512 to 6b3d2ec Compare February 22, 2023 07:32
@hbagdi hbagdi added the version-compatibility Incompatibility with older data plane versions label Mar 1, 2023
@chronolaw chronolaw force-pushed the style/cache_atc_router_matcher branch from a42de10 to 570729b Compare March 7, 2023 06:22
@dndx
Copy link
Member

dndx commented Mar 7, 2023

@chronolaw Is this ready for review?

@dndx
Copy link
Member

dndx commented Mar 7, 2023

Blocked by Kong/atc-router#39.

@mikefero
Copy link
Contributor

mikefero commented Mar 8, 2023

@hbagdi Can you elaborate on the version compatibility concerns here as this refactor looks to keep the schema intact?

@hbagdi
Copy link
Member

hbagdi commented Mar 9, 2023

No concerns @mikefero.

@mikefero
Copy link
Contributor

No concerns @mikefero.

None; the schema is intact (just moved) and only impacts validation of the expression router. All good here.

@hbagdi
Copy link
Member

hbagdi commented Mar 21, 2023

@chronolaw Any blockers on this PR?

@chronolaw
Copy link
Contributor Author

chronolaw commented Mar 22, 2023 •
edited
Loading

Yes, It still wait the atc-router's new release. See Kong/atc-router#39.

@chronolaw chronolaw force-pushed the style/cache_atc_router_matcher branch from 570729b to f771d3f Compare April 7, 2023 03:18
@mikefero
Copy link
Contributor

mikefero commented Apr 7, 2023

No concerns @mikefero.

None; the schema is intact (just moved) and only impacts validation of the expression router. All good here.

@hbagdi Taking this statement back now, it looks like we are removing a field.

@chronolaw What is the reasoning for removing a field in f771d3f? This will have a negative impact on version compatibility with older data planes.

@chronolaw
Copy link
Contributor Author

chronolaw commented Apr 7, 2023 •
edited
Loading

No concerns @mikefero.

None; the schema is intact (just moved) and only impacts validation of the expression router. All good here.

@hbagdi Taking this statement back now, it looks like we are removing a field.

@chronolaw What is the reasoning for removing a field in f771d3f? This will have a negative impact on version compatibility with older data planes.

No, the field id is only be passed to validation, and we don't use it any more with new api of atc-router.

OK, I will restore it.

@chronolaw chronolaw force-pushed the style/cache_atc_router_matcher branch from 44a8a7b to 141e494 Compare April 24, 2023 05:57
@chronolaw chronolaw force-pushed the style/cache_atc_router_matcher branch 2 times, most recently from 875bbb1 to 67c2559 Compare May 8, 2023 07:09
@chronolaw chronolaw added this to the 3.4.0 milestone May 8, 2023
@chronolaw chronolaw force-pushed the style/cache_atc_router_matcher branch from 4e43a0d to 7342f97 Compare July 6, 2023 07:20
@dndx dndx merged commit f60d681 into master Jul 6, 2023
@dndx dndx deleted the style/cache_atc_router_matcher branch July 6, 2023 09:49
curiositycasualty pushed a commit that referenced this pull request Oct 15, 2024
@raoxiaoyan
fix(runloop): throw unexpected error while updating the kong_admin (#…
1a9f0b5 
…10100)

https://konghq.atlassian.net/browse/KM-438
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StarlightIbuki StarlightIbuki StarlightIbuki left review comments

@dndx dndx dndx approved these changes

Assignees

@chronolaw chronolaw

Labels
build/bazel changelog core/db schema-change-noteworthy size/L version-compatibility Incompatibility with older data plane versions
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.
6 participants
@chronolaw @dndx @mikefero @hbagdi @StarlightIbuki @fffonion