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

Upstream PROXY protocol results in unbound number of connection pools #16682

Closed
ggreenway opened this issue May 26, 2021 · 4 comments · Fixed by #16948 or #17403
Closed

Upstream PROXY protocol results in unbound number of connection pools #16682

ggreenway opened this issue May 26, 2021 · 4 comments · Fixed by #16948 or #17403
Assignees

Comments

@ggreenway
Copy link
Contributor

When using upstream PROXY protocol with tcp_proxy, a new connection pool is created in the cluster for each unique combination of downstream IP:port and upstream host.

For many typical use cases, where the downstream clients are not from a tightly constrained set of IP addresses, this results in a nearly-infinite number of connection pools, which wastes memory. Even if the clients are from a very small set of IPs, each could typically use 30,000 ephemeral ports, which could still result in a very large number of connection pools. The connection pools are never removed until the cluster is removed (via CDS, or process shutdown).

#13061 would have fixed this, but it was never completed/merged.

@ggreenway ggreenway added bug triage Issue requires triage area/cluster and removed triage Issue requires triage labels May 26, 2021
@ggreenway ggreenway self-assigned this May 26, 2021
@klarose
Copy link
Contributor

klarose commented Jun 2, 2021

I wonder if the original_src filter work I did a while back is susceptible to this as well. I thought that I had implemented some cleanup and reuse logic here (https://github.com/envoyproxy/envoy/blob/57976d142005cda6ce54ec215cafceebe4fc1aef/source/common/upstream/conn_pool_map_impl.h), but it's possible I missed it.

I bring this up, because it has similar constraints to the proxy protocol: a given upstream connection can only really carry requests from a given downstream, so we essentially need a connection pool per downstream connection.

@ggreenway
Copy link
Contributor Author

If the circuit breaker for number of pools is set, that will set a bound on the number of http pools. However:

  • If the number of pools is large, performance won't be very good because we have to do a linear search everytime we want to free a pool to add another
  • tcp conn pools don't honor that setting at all, so tcp_proxy configurations don't have any way to limit the number of pools

@github-actions
Copy link

github-actions bot commented Jul 3, 2021

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 3, 2021
@ggreenway
Copy link
Contributor Author

Not stale; PR in progress.

@ggreenway ggreenway removed the stale stalebot believes this issue/PR has not been touched recently label Jul 6, 2021
ggreenway added a commit that referenced this issue Jul 9, 2021
Delete connection pools when they have no connections anymore. This fixes unbounded memory use for cases where a new connection pool is needed for each downstream connection, such as when using upstream PROXY protocol.

Fixes #16682

Signed-off-by: Greg Greenway <ggreenway@apple.com>
Co-authored-by: Craig Radcliffe <craig.radcliffe@broadcom.com>
ggreenway added a commit to ggreenway/envoy that referenced this issue Jul 19, 2021
Delete connection pools when they have no connections anymore. This
fixes unbounded memory use for cases where a new connection pool is
needed for each downstream connection, such as when using upstream
PROXY protocol.

Fixes envoyproxy#16682

This reverts commit b7bc539.
This reverts PR envoyproxy#17319, by re-adding envoyproxy#17302 and envoyproxy#16948.

Signed-off-by: Greg Greenway <ggreenway@apple.com>
Co-authored-by: Craig Radcliffe <craig.radcliffe@broadcom.com>
@ggreenway ggreenway reopened this Jul 19, 2021
leyao-daily pushed a commit to leyao-daily/envoy that referenced this issue Sep 30, 2021
Delete connection pools when they have no connections anymore. This fixes unbounded memory use for cases where a new connection pool is needed for each downstream connection, such as when using upstream PROXY protocol.

Fixes envoyproxy#16682

Signed-off-by: Greg Greenway <ggreenway@apple.com>
Co-authored-by: Craig Radcliffe <craig.radcliffe@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants