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.

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

Add IdleTimeout configuration to support use with (more) loadbalancers #317

Closed
betagan opened this issue Sep 29, 2020 · 2 comments · Fixed by #468
Closed

Add IdleTimeout configuration to support use with (more) loadbalancers #317

betagan opened this issue Sep 29, 2020 · 2 comments · Fixed by #468

Comments

@betagan
Copy link

betagan commented Sep 29, 2020

I'd like to propose that the following feature be added:

Proposal

Vouch Proxy should have a configuration parameter to set the IdleTimeout (and potentially Read/Write Timeout) that is passed to http.Server.

Background

Currently, that parameter is not set and thus defaults to ReadTimeout which is set to 15s as of now (in main.go).
This is problematic with some loadbalancers (like AWS ELB) and leads to 502 errors for clients.

Let me explain why I think this is problematic.

The IdleTimeout defines the maximum amount of time that http.Server will allow for a request to be made on an existing TCP session with keep-alive. That is, clients (or loadbalancers) can send follow-up requests to the same TCP session and get another response back.

For Loadbalancers like AWS ELB, the IdleTimeout has two effects: It defines

  • the keep-alive timeout: The loadbalancer will expect the upstream service to respond to requests on the same session for at least this long.
  • the max. request processing time: The loadbalancer will wait this long for an upstream service to respond (i.e. long-running requests) before it kills the connection.

This means, that we cannot set the Loadbalancers IdleTimeout to a very low value (less than VouchProxy's default 15s) as the other services sharing the loadbalancer might have long-running requests that require more than 15s. In real world scenarios, this IdleTimeout is probably set to something between 30 and 120 seconds.

What we are experiencing therefore is, that the loadbalancers sends a request on a reused TCP session after 15s and expects vouch proxy to respond. Vouch proxy does not respond (because the 15s have elapsed) and the loadbalancer thus generates a 502 http failure to the client.

@bnfinet
Copy link
Member

bnfinet commented Oct 2, 2020

@betagan that's exciting that you're able to integrate VP with AWS ELB. Is NGINX still involved? Would you care to provide any documentation related to that? I'd love to add that to the "Advanced Configurations" section of the README.

related: #180

PR welcome!

It shouldn't be too hard to add. Please set .defaults.yml to 15 and add a clear description to config/config.yml_example for vouch.server.idleTimeout as well as read/write. And please test with both an env var as well as the config entry.

@mig5
Copy link
Contributor

mig5 commented Mar 29, 2022

I recently launched 3 vouch containers at AWS ECS (where the AWS loadbalancer passes directly to the Vouch port - there is no Nginx middleware between the two). And I am seeing this problem rear its ugly head. It's most obvious on apps that make many HTTP requests, for example, AJAX-driven polling in the browser to an endpoint that is behind a Vouch auth request.

It would be great for this to be configurable. My AWS ALB has an idle_timeout of 60s (that's the default) and all my other non-Vouch apps, which use Nginx, have a keepalive_timeout of 65s. This combination works great, so I would love to set Vouch to 65 too.

Does vouch code need to change to consume such a parameter? I couldn't find in the code where any timeout is being set at all - perhaps it's inherited from an upstream Go HTTP dependency library? Found, fixed, and PR sent! #468

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants