-
Notifications
You must be signed in to change notification settings - Fork 282
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
proxy: add idle timeout #2319
proxy: add idle timeout #2319
Conversation
Code Climate has analyzed commit 677f681 and detected 0 issues on this pull request. View more on Code Climate. |
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.
How do these policy timeout settings interact with the option timeouts?
Lines 108 to 113 in 41a2622
// Timeout settings : https://github.com/pomerium/pomerium/issues/40 | |
ReadTimeout time.Duration `mapstructure:"timeout_read" yaml:"timeout_read,omitempty"` | |
WriteTimeout time.Duration `mapstructure:"timeout_write" yaml:"timeout_write,omitempty"` | |
IdleTimeout time.Duration `mapstructure:"timeout_idle" yaml:"timeout_idle,omitempty"` | |
In particular, IdleTimeout
?
It looks like WriteTimeout
and ReadTimeout
are no longer used as originally designed (as part of the old underlying net.Conn
for proxy)?
config/policy.go
Outdated
@@ -188,7 +193,15 @@ type PolicyRedirect struct { | |||
|
|||
// NewPolicyFromProto creates a new Policy from a protobuf policy config route. | |||
func NewPolicyFromProto(pb *configpb.Route) (*Policy, error) { | |||
timeout, _ := ptypes.Duration(pb.GetTimeout()) | |||
var timeout, idleTimeout *time.Duration |
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.
same declaration is seperated in ToProto
. Personally I prefer splitting it, and keeping the variable close to the assignment as in ToProto
but I think we should be consistent either way.
edit: removed nit
. Let's be consistent.
my reading of https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-field-config-route-v3-routeaction-idle-timeout: per-route
and |
Summary
timeout
andidle_timeout
optional pointerstimeout
if unset andidle_timeout
is setRelated issues
Fixes https://github.com/pomerium/internal/issues/445
Checklist
improvement
/bug
/ etc)