Wrap diag service listener with multiplexer so it can work behind PROXY enabled loadbalancer/proxy. by AntonAM · Pull Request #39497 · gravitational/teleport

AntonAM · 2024-03-18T11:01:28Z

This PR wraps diag service listener with multiplexer. It now can accept simultaneously connections that are prepended with PROXY line or not. We also add HTTP() protocol listener to multiplexer and a flag that controls whether we should issue warnings about unspecified PROXY protocol mode for this listener - diag service will always run in unspecified mode to support both local access and access from behind a proxy/loadbalancer.

Fixes #39327

Changelog: Allow diagnostic endpoints to be accessed behind a PROXY protocol enabled loadbalancer/proxy.

…XY enabled loadbalancer/proxy. It accept simultaneously connections that are prepended with PROXY line or not. We also don't issue warnings about unspecified PROXY protocol mode for this listener.

espadolini

Can we at least gate this behind an option in metrics_service so we don't add load to the thing that's supposed to be used for super lightweight health checks and profiling?

AntonAM · 2024-03-18T13:26:47Z

@espadolini dedicated metrics_service shouldn't be affected, it's only affected if in legacy mode metrics are served by the diag service

webvictim

LGTM, but will defer to more experienced reviewers!

espadolini · 2024-03-18T16:50:38Z

@AntonAM I realized later that metrics_service doesn't actually serve healthz but only the prom metrics, lol

Can we put this behind a flag so in normal conditions the diag listener is only a regular OS stdlib listener handled by a plain http.Server?

AntonAM · 2024-03-19T11:01:23Z

@espadolini I apachebenched it and results were not that different, with 35k vs 40k reqs/sec (plain vs mux), memory consumption was relatively equal, with twice more garbage collections for the mux (125 vs 250). Considering that in real life that listener shouldn't see anywhere similar level of load, do you think it's worth it to introduce flag or something?

espadolini · 2024-03-20T11:11:43Z

The additional memory pressure and resource utilization is exactly what I'm worried about, considering that the pprof endpoints are generally used to diagnose situations where resource utilization is already exceeding the norm, but I suppose we'll just be able to rely on the unix socket for the console_service for that.

AntonAM · 2024-03-22T10:37:20Z

Yep, but that's what I meant with ab-testing result, only twice memory overhead over the plain listener (under specifically loading just this listener with thousands reqs per second) seems negligible for the real world usage, especially for the pprof handlers.

espadolini · 2024-03-25T11:17:34Z

+			}
+		}()
+
+		err = server.Serve(listenerHTTP)


With this change we close listenerHTTP rather than listener when we close server; I suspect that it's not a problem in practice (and it might be slightly more correct, even, considering that we want to manage the lifetime of the listener sockets in TeleportProcess rather than let random goroutines close them) but it might be worth noting in a comment in diagnostic.shutdown.

We now close multiplexer in diagnostic.shutdown, which closes listener. So I guess we can do without the comment, since we've got virtually the same situation?

espadolini · 2024-03-25T11:18:18Z

 	process.RegisterFunc("diagnostic.service", func() error {
-		err := server.Serve(listener)
-		if err != nil && err != http.ErrServerClosed {
+		muxListener, err := multiplexer.New(multiplexer.Config{


We should close muxListener either here in a defer or in diagnostic.shutdown.

Done 436d415

Co-authored-by: Gus Luxton <gus@goteleport.com>

Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>

public-teleport-github-review-bot · 2024-04-02T19:05:51Z

@AntonAM See the table below for backport results.

Branch	Result
branch/v13	Failed
branch/v14	Failed
branch/v15	Create PR

…XY enabled loadbalancer/proxy. (#39497) * Wrap diag service listener with multiplexer so it can work behind PROXY enabled loadbalancer/proxy. It accept simultaneously connections that are prepended with PROXY line or not. We also don't issue warnings about unspecified PROXY protocol mode for this listener. * Fix wording. Co-authored-by: Gus Luxton <gus@goteleport.com> * Use ExitContext instead of GracefulExitContext Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com> * Close diag multiplexer listener during diagnostic.shutdown event. * Refactor server.Serve() call * Move creation of muxListener outside of diagnostic.service event. * Combine declaration and usage Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com> --------- Co-authored-by: Gus Luxton <gus@goteleport.com> Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>

…XY enabled loadbalancer/proxy. (#39497) (#40139) * Wrap diag service listener with multiplexer so it can work behind PROXY enabled loadbalancer/proxy. It accept simultaneously connections that are prepended with PROXY line or not. We also don't issue warnings about unspecified PROXY protocol mode for this listener. * Fix wording. * Use ExitContext instead of GracefulExitContext * Close diag multiplexer listener during diagnostic.shutdown event. * Refactor server.Serve() call * Move creation of muxListener outside of diagnostic.service event. * Combine declaration and usage --------- Co-authored-by: Gus Luxton <gus@goteleport.com> Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>

…XY enabled loadbalancer/proxy. (#39497) (#40140) * Wrap diag service listener with multiplexer so it can work behind PROXY enabled loadbalancer/proxy. It accept simultaneously connections that are prepended with PROXY line or not. We also don't issue warnings about unspecified PROXY protocol mode for this listener. * Fix wording. * Use ExitContext instead of GracefulExitContext * Close diag multiplexer listener during diagnostic.shutdown event. * Refactor server.Serve() call * Move creation of muxListener outside of diagnostic.service event. * Combine declaration and usage --------- Co-authored-by: Gus Luxton <gus@goteleport.com> Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>

Wrap diag service listener with multiplexer so it can work behind PRO…

0f358b8

…XY enabled loadbalancer/proxy. It accept simultaneously connections that are prepended with PROXY line or not. We also don't issue warnings about unspecified PROXY protocol mode for this listener.

AntonAM changed the title ~~Wrap diag service listener with multipleexr so it can work behind PROXY enabled loadbalancer/proxy.~~ Wrap diag service listener with multiplexer so it can work behind PROXY enabled loadbalancer/proxy. Mar 18, 2024

AntonAM force-pushed the anton/diag_proxy_protocol branch from 9a7ab44 to 0f358b8 Compare March 18, 2024 11:20

AntonAM requested a review from webvictim March 18, 2024 11:20

AntonAM marked this pull request as ready for review March 18, 2024 11:21

github-actions Bot requested review from justinas and zmb3 March 18, 2024 11:21

github-actions Bot added the size/sm label Mar 18, 2024

espadolini reviewed Mar 18, 2024

View reviewed changes

webvictim reviewed Mar 18, 2024

View reviewed changes

Comment thread lib/multiplexer/multiplexer.go Outdated

rosstimothy requested a review from espadolini March 22, 2024 21:19

espadolini reviewed Mar 25, 2024

View reviewed changes

AntonAM and others added 3 commits April 1, 2024 22:33

Fix wording.

f691109

Co-authored-by: Gus Luxton <gus@goteleport.com>

Use ExitContext instead of GracefulExitContext

d224f23

Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>

Close diag multiplexer listener during diagnostic.shutdown event.

436d415

rosstimothy requested a review from espadolini April 2, 2024 15:00

rosstimothy approved these changes Apr 2, 2024

View reviewed changes

Comment thread lib/service/service.go Outdated

espadolini reviewed Apr 2, 2024

View reviewed changes

Comment thread lib/service/service.go

AntonAM added 2 commits April 2, 2024 18:57

Refactor server.Serve() call

3742038

Move creation of muxListener outside of diagnostic.service event.

76aac50

espadolini approved these changes Apr 2, 2024

View reviewed changes

Comment thread lib/service/service.go

Comment thread lib/service/service.go Outdated

public-teleport-github-review-bot Bot removed request for justinas and zmb3 April 2, 2024 17:14

Combine declaration and usage

f168c3c

Co-authored-by: Edoardo Spadolini <edoardo.spadolini@goteleport.com>

AntonAM added the backport/branch/v13 label Apr 2, 2024

AntonAM added backport/branch/v14 labels Apr 2, 2024

AntonAM added this pull request to the merge queue Apr 2, 2024

Merged via the queue into master with commit 73cbb75 Apr 2, 2024

AntonAM deleted the anton/diag_proxy_protocol branch April 2, 2024 19:04

Conversation

AntonAM commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

espadolini left a comment

Choose a reason for hiding this comment

Uh oh!

AntonAM commented Mar 18, 2024

Uh oh!

webvictim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

espadolini commented Mar 18, 2024

Uh oh!

AntonAM commented Mar 19, 2024

Uh oh!

espadolini commented Mar 20, 2024

Uh oh!

AntonAM commented Mar 22, 2024

Uh oh!

Uh oh!

espadolini Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

AntonAM Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

espadolini Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

AntonAM Apr 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Apr 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AntonAM commented Mar 18, 2024 •

edited

Loading