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

feat: support tls expression route. #4574

Merged
merged 3 commits into from
Oct 17, 2023
Merged

feat: support tls expression route. #4574

merged 3 commits into from
Oct 17, 2023

Conversation

rodman10
Copy link
Contributor

What this PR does / why we need it:

Which issue this PR fixes:

Fixes #4540

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@rodman10
Copy link
Contributor Author

rodman10 commented Aug 28, 2023 •
edited
Loading

I have struggled with the tls_passthrough intergration test all day, I found traditional flavor hadn't constrainted the net.protocol for stream subsystem strictly, so the tls_passthrough which will be converted into tcp will work properly, but in expression flavor, the conversion of tls_passthrough will cause expression match failed for gateway appending protocols field to the expression (for tls_passthrough is tcp) , but the scheme (aka protocols) will be set to tls in gateway preread phase, which will cause route not found, whether I have missed or misunderstood anything? @randmonkey

@rainest rainest mentioned this pull request Sep 8, 2023
Merged
1 task
@chronolaw chronolaw mentioned this pull request Sep 11, 2023
Merged
3 tasks
@randmonkey
Copy link
Contributor

@rodman10 confirmed with gateway team that there are some issue in the Kong router part to set the matching protocols, so tls_passthrough is not supported now. The Kong's PR Kong/kong#11538 is expected to fix this.

@rodman10
Copy link
Contributor Author

@rodman10 confirmed with gateway team that there are some issue in the Kong router part to set the matching protocols, so tls_passthrough is not supported now. The Kong's PR Kong/kong#11538 is expected to fix this.

Whether tls_pasthrough should be contained in this PR, or according to this thread, I can just ignore the test failure? @randmonkey

@randmonkey
Copy link
Contributor

randmonkey commented Sep 12, 2023 •
edited
Loading

@rodman10 confirmed with gateway team that there are some issue in the Kong router part to set the matching protocols, so tls_passthrough is not supported now. The Kong's PR Kong/kong#11538 is expected to fix this.

Whether tls_pasthrough should be contained in this PR, or according to this thread, I can just ignore the test failure? @randmonkey

I think we should hold this until Kong gateway has released a version to support tls_passthrough. Before Kong supports this, we cannot achieve the support of TLSRoute. While in your implementation, I think it is OK to ignore the test failures for now.

windmgc pushed a commit to Kong/kong that referenced this pull request Sep 18, 2023
@chronolaw
fix(router): fix tls_passthrough in expression flavor (#11538)
66795cd 
Fix an issue that protocol `tls_passthrough` can not work with expressions flavor

KAG-2561
See: Kong/kubernetes-ingress-controller#4574 (comment)
team-gateway-bot pushed a commit to Kong/kong that referenced this pull request Sep 18, 2023
@chronolaw @github-actions
fix(router): fix tls_passthrough in expression flavor (#11538)
617f97a 
Fix an issue that protocol `tls_passthrough` can not work with expressions flavor

KAG-2561
See: Kong/kubernetes-ingress-controller#4574 (comment)

(cherry picked from commit 66795cd)
windmgc pushed a commit to Kong/kong that referenced this pull request Sep 19, 2023
@chronolaw @windmgc
fix(router): fix tls_passthrough in expression flavor (#11538)
d8cf3aa 
Fix an issue that protocol `tls_passthrough` can not work with expressions flavor

KAG-2561
See: Kong/kubernetes-ingress-controller#4574 (comment)

(cherry picked from commit 66795cd)
@randmonkey
Copy link
Contributor

@rodman10 Now Kong 3.4.1 is released with the fix of supporting TLS passthrough. You can continue on this PR if you still want to do.

@rodman10
Copy link
Contributor Author

Ok, I will work with this pr this weekend.

@rodman10 rodman10 force-pushed the tls_exp branch 5 times, most recently from bb6673b to 18ee6c2 Compare October 15, 2023 13:37
@randmonkey
Copy link
Contributor

@rodman10 Is it ready for review now? I tried to run integration tests for TLSRoute and the tests passed.

@rodman10 rodman10 marked this pull request as ready for review October 17, 2023 10:56
@rodman10 rodman10 requested a review from a team as a code owner October 17, 2023 10:56
@rodman10
Copy link
Contributor Author

rodman10 commented Oct 17, 2023 •
edited
Loading

@rodman10 Is it ready for review now? I tried to run integration tests for TLSRoute and the tests passed.

Ok, the pr is ready for review, because I found kongintergration test was failed, I was trying to figure it out.

randmonkey
randmonkey reviewed Oct 17, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@randmonkey randmonkey merged commit a460cd9 into Kong:main Oct 17, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@programmer04 programmer04 programmer04 left review comments

@randmonkey randmonkey randmonkey approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support TLSRoute with expression router
3 participants
@rodman10 @randmonkey @programmer04