- 
                Notifications
    You must be signed in to change notification settings 
- Fork 612
fix: Do not add request-termination on TLSRoute / TCPRoute #7700
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. This does look good.
Just 1 minor nit w.r.t the build failing.
Most of the tests in CI will fail because we require a secret only available to in tree PRs. When we get the review done I'll handle cherry picking the change.
| "message": "no existing backendRef provided", | ||
| }, | ||
| }) | ||
| if strings.Contains(service.Service.Protocol, "http") || strings.Contains(service.Service.Protocol, "grpc") { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strings package has to be imported for the build to pass.
| Hello, | 
…ute (#7700) (#7720) * fix: Do not add request-termination on TLSRoute / TCPRoute * fix: add import of strings and check nil * fix unit tests and golden test cases * add changelog --------- Co-authored-by: Victor Durand <[email protected]>
…ute (#7700) (#7720) * fix: Do not add request-termination on TLSRoute / TCPRoute * fix: add import of strings and check nil * fix unit tests and golden test cases * add changelog --------- Co-authored-by: Victor Durand <[email protected]> (cherry picked from commit cc67509)
…ute (#7700) (#7720) * fix: Do not add request-termination on TLSRoute / TCPRoute * fix: add import of strings and check nil * fix unit tests and golden test cases * add changelog --------- Co-authored-by: Victor Durand <[email protected]>
…ute (#7700) (#7720) (#7729) * fix: Do not add request-termination on TLSRoute / TCPRoute * fix: add import of strings and check nil * fix unit tests and golden test cases * add changelog --------- (cherry picked from commit cc67509) Co-authored-by: Tao Yi <[email protected]> Co-authored-by: Victor Durand <[email protected]>
…ute (#7700) (#7720) (#7730) * fix: Do not add request-termination on TLSRoute / TCPRoute * fix: add import of strings and check nil * fix unit tests and golden test cases * add changelog --------- Co-authored-by: Victor Durand <[email protected]>
| Cherry-picked in #7720 | 
What this PR does / why we need it:
The request-termination was added to return 500 error when no backendRef is provided, this should not be done on TLS/TCP route as HTTP errors are not valid with those protocols. Also the request-termination plugin is only accepting HTTP/HTTPS/GRPC/GRPCS
https://developer.konghq.com/plugins/request-termination/reference/
Which issue this PR fixes:
#7699
#fix 7699
Special notes for your reviewer:
PR Readiness Checklist:
Complete these before marking the PR as
ready to review:CHANGELOG.mdrelease notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR