-
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(api): add idleTimeout to ClientTrafficPolicy for TCP listener #3345
Conversation
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
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.
Overall looks good me, just one comment on the doc
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.
Overall looks good me, just one comment on the doc
// Timeout settings for HTTP. | ||
// | ||
// +optional | ||
HTTP *HTTPClientTimeout `json:"http,omitempty"` | ||
} | ||
|
||
type TCPClientTimeout struct { | ||
// IdleTimeout for a TCP connection. Idle time is defined as a period in which there are no |
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.
I assume that this will be implemented for the envoy TCP Proxy, right? Maybe we should state that these settings only apply to tcproute
, tlsroute
resources, so that users don't expect this timeout to apply also in case of HTTP (over TCP) connections?
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.
Yeah, the state will be necessary to clear confusion. TCPClientTimeout can only be applied on listener whose protocol is TCP or TLS, it'll be used to set Envoy TCP Proxy's idle timeout eventually.
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
Signed-off-by: Dingkang Li <dingkang1743@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3345 +/- ##
==========================================
+ Coverage 66.97% 67.12% +0.15%
==========================================
Files 164 164
Lines 23882 23773 -109
==========================================
- Hits 15994 15958 -36
+ Misses 6964 6896 -68
+ Partials 924 919 -5 ☔ View full report in Codecov by Sentry. |
/retest |
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!
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 !
…tener (envoyproxy#3345)" This reverts commit 6912dc0.
…tener (envoyproxy#3345)" This reverts commit 6912dc0. Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
What this PR does / why we need it:
Add idleTimeout to ClientTrafficPolicy for TCP listener.
Which issue(s) this PR fixes:
Fixes #3343