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

feat(router/atc): extend route.snis to support wildcard for the leftmost or rightmost character with traditional-compatible flavor #12809

Merged
merged 18 commits into from
Apr 19, 2024

Conversation

catbro666
Copy link
Contributor

@catbro666 catbro666 commented Mar 29, 2024

Summary

  • Change the element type of route.snis from sni to wildcard_host.
  • Change the router accordingly to support wildcard snis because the sni is involved in route matching.

Note this is to be a hidden feature intentionally so we don't document it.

Additional Notes:

  • The expressions flavor doesn't support mTLS (mtls-auth plugin and tls-handshake-modifier plugin) theoretically. The reason is the current mTLS implementation is based on the route.snis, but at the ssl phase it is not yet known which route will match. As a workaround, it collects the sni set on routes that are associated with mtls plugins in advance. But for the expressions flavor, things are different. All the fields that were involved in route matching have been merged into the field expression. Without actually evaluating, we can't know in general if a sni will match a certain route. But again, you can't get all the parameters required for evaluation at the ssl phase. The correct solution is decoupling the mTLS logic from the routes entity by binding it to the snis entity, as we’ve discussed in https://konghq.atlassian.net/browse/KAG-3757
  • We don't distinguish the priority of plain-only-snis and contain-wildcard-snis for the traditional-compatible flavor because there are no reserved bits available. See https://github.com/Kong/kong/blob/master/kong/router/transform.lua#L515-L528

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

https://konghq.atlassian.net/browse/KAG-3832

@github-actions github-actions bot added core/router core/db schema-change-noteworthy cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Mar 29, 2024
@catbro666 catbro666 force-pushed the kag-3832-route-snis-support-wildcard branch 2 times, most recently from 721f49d to 18c2eec Compare March 29, 2024 05:11
@catbro666 catbro666 requested a review from chronolaw March 29, 2024 05:16
@chronolaw
Copy link
Contributor

Should we schedule a short offline meeting to discuss this PR? I am not very clear about the requirement.

Another thing, we are planing a big refactoring of router (#12667), perhaps you should take a look at it.

@chronolaw chronolaw marked this pull request as draft March 29, 2024 05:56
@chronolaw
Copy link
Contributor

chronolaw commented Mar 29, 2024

Convert to draft because we still have some discussions.

@catbro666 catbro666 requested review from bungle and dndx March 29, 2024 05:57
@catbro666 catbro666 added this to the 3.7.0 milestone Mar 29, 2024
@chronolaw
Copy link
Contributor

We should also add compatible check code for hybrid mode, See #12814

@catbro666 catbro666 changed the title feat(router): extend route.snis to support wildcard feat(router): extend route.snis to support wildcard for the leftmost or rightmost character Apr 1, 2024
@catbro666
Copy link
Contributor Author

catbro666 commented Apr 1, 2024

We should also add compatible check code for hybrid mode, See #12814

typedefs.sni and typedefs.wildcard_host are both of type string, just the validation functions are different. And validate_wildcard_host is stricter than validate_sni (which means those strings can pass validate_wildcard_host will certainly pass validate_sni), so I think there is no need to add the compatible check code.

@chronolaw
Copy link
Contributor

We should also add compatible check code for hybrid mode, See #12814

typedefs.sni and typedefs.wildcard_host are both of type string, just the validation functions are different. And validate_wildcard_host is stricter than validate_sni (which means those strings can pass validate_wildcard_host will certainly pass validate_sni), so I think there is no need to add the compatible check code.

No, atc-router in kong gateway lower than 3.7 does not understand wildcard sni, it only support sni == a.com.
If atc-router with flavor traditional_compatible get a expression sni == *.com it can not work as we wish.

@catbro666
Copy link
Contributor Author

We should also add compatible check code for hybrid mode, See #12814

typedefs.sni and typedefs.wildcard_host are both of type string, just the validation functions are different. And validate_wildcard_host is stricter than validate_sni (which means those strings can pass validate_wildcard_host will certainly pass validate_sni), so I think there is no need to add the compatible check code.

No, atc-router in kong gateway lower than 3.7 does not understand wildcard sni, it only support sni == a.com. If atc-router with flavor traditional_compatible get a expression sni == *.com it can not work as we wish.

Ah right. utils.normalize_ip does this limitation. Will add the compatible check code, Thanks!

@catbro666 catbro666 force-pushed the kag-3832-route-snis-support-wildcard branch from 73db9bb to 7e37da3 Compare April 1, 2024 03:57
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Apr 1, 2024
@catbro666 catbro666 force-pushed the kag-3832-route-snis-support-wildcard branch from 4003d47 to 40646a5 Compare April 1, 2024 07:35
kong/router/transform.lua Outdated Show resolved Hide resolved
kong/router/transform.lua Outdated Show resolved Hide resolved
kong/clustering/compat/checkers.lua Outdated Show resolved Hide resolved
kong/router/transform.lua Outdated Show resolved Hide resolved
@catbro666 catbro666 force-pushed the kag-3832-route-snis-support-wildcard branch 2 times, most recently from d61742b to 3459423 Compare April 1, 2024 09:19
kong/db/schema/entities/routes.lua Show resolved Hide resolved
spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
@catbro666 catbro666 force-pushed the kag-3832-route-snis-support-wildcard branch from 372741c to 6e96aac Compare April 18, 2024 05:20
@catbro666 catbro666 force-pushed the kag-3832-route-snis-support-wildcard branch from 6e96aac to dc84b12 Compare April 18, 2024 05:29
spec/01-unit/08-router_spec.lua Outdated Show resolved Hide resolved
spec/01-unit/08-router_spec.lua Outdated Show resolved Hide resolved
spec/01-unit/08-router_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
@chronolaw
Copy link
Contributor

Should we add back the change log file to tell the user that we have this new feature?

@chronolaw chronolaw changed the title feat(router): extend route.snis to support wildcard for the leftmost or rightmost character with traditional-compatible flavor feat(router/atc): extend route.snis to support wildcard for the leftmost or rightmost character with traditional-compatible flavor Apr 18, 2024
@catbro666 catbro666 force-pushed the kag-3832-route-snis-support-wildcard branch from 0873d08 to 51bccf1 Compare April 18, 2024 07:05
@catbro666
Copy link
Contributor Author

Should we add back the change log file to tell the user that we have this new feature?

Confirmed with @dndx that this would be a hidden feature so we don't document it.

spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
spec/02-integration/05-proxy/02-router_spec.lua Outdated Show resolved Hide resolved
catbro666

This comment was marked as off-topic.

Copy link
Contributor

@chronolaw chronolaw left a comment

Choose a reason for hiding this comment

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

Great job!

@catbro666
Copy link
Contributor Author

@chronolaw Thanks for the efforts in the review!

Copy link
Contributor

@jschmid1 jschmid1 left a comment

Choose a reason for hiding this comment

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

I have nothing further to add, thanks to the thorough reviews by my peers before me.

@ms2008 ms2008 dismissed dndx’s stale review April 19, 2024 04:20

Changes have been made.

@ms2008
Copy link
Contributor

ms2008 commented Apr 19, 2024

@dndx Can you please review this again?

@bungle bungle merged commit ea41756 into master Apr 19, 2024
27 checks passed
@bungle bungle deleted the kag-3832-route-snis-support-wildcard branch April 19, 2024 12:23
@team-gateway-bot
Copy link
Collaborator

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-12809-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-12809-to-master-to-upstream
git checkout -b cherry-pick-12809-to-master-to-upstream
ancref=$(git merge-base 00d1c3ff8afddb1b9c2600701d00c7655ef9f828 96e5d056cfe371b927473fff73308407e61bdc01)
git cherry-pick -x $ancref..96e5d056cfe371b927473fff73308407e61bdc01

@github-actions github-actions bot added the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Apr 19, 2024
@bungle
Copy link
Member

bungle commented Apr 19, 2024

@dndx damn it, I pushed rebase and merge instead of squash and merge. @catbro666 let's stop having PRs on review stage with unclean commits. See: https://github.com/Kong/kong/commits/master/

@kikito kikito removed the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.