-
Notifications
You must be signed in to change notification settings - Fork 360
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(policy): Add CTP support for TCP/TLS listeners #3337
Conversation
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
/retest |
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
/retest |
@@ -327,6 +324,14 @@ func validatePortOverlapForClientTrafficPolicy(l *ListenerContext, xds *ir.Xds, | |||
} | |||
} | |||
|
|||
var tcpIR *ir.TCPListener |
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.
tcpIR
is unused here, lets add it when needed ?
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.
Sure.
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
/retest |
/retest |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3337 +/- ##
==========================================
+ Coverage 67.41% 67.43% +0.02%
==========================================
Files 166 166
Lines 19186 19214 +28
==========================================
+ Hits 12934 12957 +23
- Misses 5322 5325 +3
- Partials 930 932 +2 ☔ View full report in Codecov by Sentry. |
@arkodg Do you think whether it's necessary to split this big PR to multiple small PRs for easier review ? Each small PR for single field in CTP. |
sorry for the delay in reviewing this one, on it ! |
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.
LGTM thanks !
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.
LGTM thanks for adding this!
What this PR does / why we need it:
Enable user to attach ClientTrafficPolicy 1 to TCP/TLS listeners, available fields are:
timeout(new field will be needed to specify tcp idle timeout in clientTimeout 2, raised in other issue feat: add support to configuring downstream idle timeout for TCP listener #3343)Which issue(s) this PR fixes:
Fixes #3163
Footnotes
https://gateway.envoyproxy.io/v1.0.1/api/extension_types/#clienttrafficpolicyspec ↩
https://gateway.envoyproxy.io/latest/api/extension_types/#clienttimeout ↩