skipper: support http2 over cleartext TCP (h2c) #2480

AlexanderYastrebov · 2023-07-24T18:05:58Z

Works around golang/go#26682 by introducing ShutdownListener that tracks active connections and implements graceful shutdown.

For #1253

AlexanderYastrebov · 2023-07-24T18:41:52Z

skipper.go

 		return err
+	} else {
+		listener = snet.NewShutdownListener(l)


This will also wait for hijacked connection when h2c is not enabled.
Need to think if there is any downside to it and maybe add a test.

Does it work with upgrade connections like websocket?

https://pkg.go.dev/net/http#Server.Shutdown

Shutdown does not attempt to close nor wait for hijacked connections such as WebSockets. The caller of Shutdown should separately notify such long-lived connections of shutdown and wait for them to close, if desired.

This is useful by itself to track hijacked connections (and maybe it should forcibly close and log remaining after timeout as currently Skipper will be killed after termination grace period (assuming cluster ingress mode)).

Can you create a follow up issue for that?

I plan to check this before we merge.

net/shutdown_listener.go

szuecs · 2023-07-24T20:10:21Z

net/shutdown_listener.go

+}
+
+func (l *ShutdownListener) Shutdown(ctx context.Context) error {
+	ticker := time.NewTicker(500 * time.Millisecond)


Why so often?
I would not ask if it was 1s.

Is it? Check out http.Server.Shutdown - it starts with 1ms and doubles until reaches 500ms, here I decided not to over-complicate things.

Oh that's way too often

szuecs · 2023-07-24T20:11:33Z

net/shutdown_listener.go

+	defer ticker.Stop()
+	for {
+		n := l.activeConns.Load()
+		log.Debugf("ShutdownListener Shutdown: %d connections", n)


We should have it as Infof and degrade if it's not needed. It's great to observe shutdown for a while.

net/shutdown_listener.go

szuecs · 2023-07-24T20:20:35Z

net/shutdown_listener.go

+
+func (l *ShutdownListener) registerConn() {
+	n := l.activeConns.Add(1)
+	log.Debugf("ShutdownListener registerConn: %d connections", n)


Do we need this?

For debugging, it does not hurt but I can remove.

szuecs · 2023-07-24T20:20:55Z

net/shutdown_listener.go

+
+func (l *ShutdownListener) unregisterConn() {
+	n := l.activeConns.Add(-1)
+	log.Debugf("ShutdownListener unregisterConn: %d connections", n)


Do we need this?
Maybe better expose a metric.

This is for debugging, IMO does not hurt

skipper.go

szuecs · 2023-07-24T20:25:17Z

skipper.go

 		return err
+	} else {
+		listener = snet.NewShutdownListener(l)


Does it work with upgrade connections like websocket?

szuecs · 2023-07-24T20:26:39Z

skipper.go

}
+

add also:
else {log.Info("shutdown done")}

We have log.Infof("done.") at the end

szuecs · 2023-07-25T06:44:31Z

skipper_test.go

+	testServerShutdown(t, o, protoH2C, testGracefulConnectionShutdown)
+
+	for i := 0; i < connectionShutdownChecks; i++ {
+		require.NoError(t, <-errc, "Expected to receive GOAWAY frame on shutdown")


Should this not check that we received a GOAWAY frame?

If we did not receive it will fail with EOF error

So we have another EOF case

Works around golang/go#26682 by introducing ShutdownListener that tracks active connections and implements graceful shutdown. For #1253 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>

AlexanderYastrebov · 2023-07-25T10:31:01Z

How to play with it:

$ GODEBUG=http2debug=2 bin/skipper -enable-http2-cleartext -inline-routes='* -> inlineContent("hello\n") -> <shunt>' -wait-for-healthcheck-interval=1s -application-log-level=DEBUG

then e.g. create h2c connection via patched h2i tool:

~$ /tmp/h2i -cleartext localhost:9090
Connecting to localhost:9090 ...
Connected to 127.0.0.1:9090
Sending: []
[FrameHeader SETTINGS len=30]
  [MAX_FRAME_SIZE = 1048576]
  [MAX_CONCURRENT_STREAMS = 250]
  [MAX_HEADER_LIST_SIZE = 1048896]
  [HEADER_TABLE_SIZE = 4096]
  [INITIAL_WINDOW_SIZE = 1048576]
[FrameHeader WINDOW_UPDATE len=4]
  Window-Increment = 983041
[FrameHeader SETTINGS flags=ACK len=0]
h2i>

terminate skipper

~$ pkill -TERM skipper

and observe shutdown logs:

[APP]INFO[0000] Expose metrics in codahale format            
[APP]DEBU[0000] filter "apiUsageMonitoring" is not enabled. spec returns `noop` filters.  filter=apiUsageMonitoring
[APP]INFO[0000] enable swarm: false                          
[APP]INFO[0000] Replacing tee filter specification           
[APP]INFO[0000] Replacing teenf filter specification         
[APP]INFO[0000] Replacing lua filter specification           
[APP]INFO[0000] support listener on :9911                    
[APP]INFO[0000] Dataclients are updated once, first load complete 
[APP]INFO[0000] Listen on :9090                              
[APP]INFO[0000] route settings, reset, route: : * -> inlineContent("hello\n") -> <shunt> 
[APP]INFO[0000] route settings received                      
[APP]INFO[0000] TLS settings not found, defaulting to HTTP   
[APP]INFO[0000] route settings applied                       
[APP]DEBU[0007] ShutdownListener registerConn: 1 active connections 
2023/07/25 12:28:17 h2c: attempting h2c with prior knowledge.
[APP]ERRO[0007] http2: server connection from 127.0.0.1:55312 on 0xc0000e62d0 
2023/07/25 12:28:17 http2: Framer 0xc00027e0e0: wrote SETTINGS len=30, settings: MAX_FRAME_SIZE=1048576, MAX_CONCURRENT_STREAMS=250, MAX_HEADER_LIST_SIZE=1048896, HEADER_TABLE_SIZE=4096, INITIAL_WINDOW_SIZE=1048576
2023/07/25 12:28:17 http2: Framer 0xc00027e0e0: read SETTINGS len=0
2023/07/25 12:28:17 http2: Framer 0xc00027e0e0: wrote WINDOW_UPDATE len=4 (conn) incr=983041
[APP]ERRO[0007] http2: server read frame SETTINGS len=0      
2023/07/25 12:28:17 http2: Framer 0xc00027e0e0: wrote SETTINGS flags=ACK len=0
[APP]INFO[0016] Got shutdown signal for Shutdown predicates  
[APP]INFO[0016] Got shutdown signal, wait 1s for health check 
[APP]INFO[0017] Start server shutdown                        
[APP]INFO[0017] Start listener shutdown                      
[APP]DEBU[0017] ShutdownListener Shutdown: 1 active connections 
2023/07/25 12:28:27 http2: Framer 0xc00027e0e0: wrote GOAWAY len=8 LastStreamID=0 ErrCode=NO_ERROR Debug=""
[APP]DEBU[0018] ShutdownListener Shutdown: 1 active connections 
[APP]DEBU[0018] ShutdownListener Shutdown: 1 active connections 
[APP]ERRO[0018] GOAWAY close timer fired; closing conn from 127.0.0.1:55312 
[APP]DEBU[0018] ShutdownListener unregisterConn: 0 active connections 
[APP]DEBU[0019] ShutdownListener Shutdown: 0 active connections 
[APP]INFO[0019] done.

and h2i logs:

...
[FrameHeader SETTINGS flags=ACK len=0]
[FrameHeader GOAWAY len=8]
  Last-Stream-ID = 0; Error-Code = NO_ERROR (0)
ReadFrame: EOF

With curl

$ curl -v --http2 http://localhost:9090
*   Trying 127.0.0.1:9090...
* Connected to localhost (127.0.0.1) port 9090 (#0)
> GET / HTTP/1.1
> Host: localhost:9090
> User-Agent: curl/8.1.2
> Accept: */*
> Connection: Upgrade, HTTP2-Settings
> Upgrade: h2c
> HTTP2-Settings: AAMAAABkAAQAoAAAAAIAAAAA
> 
< HTTP/1.1 101 Switching Protocols
< Connection: Upgrade
< Upgrade: h2c
* Received 101, Switching to HTTP/2
< HTTP/2 200 
< content-type: text/plain; charset=utf-8
< server: Skipper
< content-length: 6
< date: Tue, 25 Jul 2023 23:31:08 GMT
< 
hello
* Connection #0 to host localhost left intact

$ curl -v --http2-prior-knowledge http://localhost:9090
*   Trying 127.0.0.1:9090...
* Connected to localhost (127.0.0.1) port 9090 (#0)
* h2 [:method: GET]
* h2 [:scheme: http]
* h2 [:authority: localhost:9090]
* h2 [:path: /]
* h2 [user-agent: curl/8.1.2]
* h2 [accept: */*]
* Using Stream ID: 1 (easy handle 0x564601b134c0)
> GET / HTTP/2
> Host: localhost:9090
> User-Agent: curl/8.1.2
> Accept: */*
> 
< HTTP/2 200 
< content-type: text/plain; charset=utf-8
< server: Skipper
< content-length: 6
< date: Tue, 25 Jul 2023 23:31:27 GMT
< 
hello
* Connection #0 to host localhost left intact

szuecs · 2023-08-22T11:13:43Z

skipper_test.go

+		return &http.Client{
+			Transport: &http2.Transport{
+				// allow http scheme
+				AllowHTTP: true,


I guess we should do the same for skipper's net.Client.

Need to check if it continues to support http1 though. Otherwise we may think either about supporting h2c scheme for endpoints like h2c://192.168.0.1 or have a filter that'd configure backend transport/roundtripper.

That would be even better to support a scheme

AlexanderYastrebov mentioned this pull request Jul 24, 2023

Adds h2c support #1868

Closed

AlexanderYastrebov force-pushed the h2c-2 branch 2 times, most recently from bbd6747 to ff6f242 Compare July 24, 2023 18:15

AlexanderYastrebov mentioned this pull request Jul 24, 2023

x/net/http2/h2c: support closure of all connections and graceful shutdown golang/go#26682

Open

AlexanderYastrebov commented Jul 24, 2023

View reviewed changes

szuecs reviewed Jul 25, 2023

View reviewed changes

skipper: support http2 over cleartext TCP (h2c)

3a9dcac

Works around golang/go#26682 by introducing ShutdownListener that tracks active connections and implements graceful shutdown. For #1253 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>

AlexanderYastrebov force-pushed the h2c-2 branch from ff6f242 to 3a9dcac Compare July 25, 2023 10:22

szuecs reviewed Aug 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skipper: support http2 over cleartext TCP (h2c) #2480

skipper: support http2 over cleartext TCP (h2c) #2480

AlexanderYastrebov commented Jul 24, 2023

AlexanderYastrebov Jul 24, 2023

szuecs Jul 24, 2023

AlexanderYastrebov Jul 24, 2023 •

edited

szuecs Jul 25, 2023

AlexanderYastrebov Aug 22, 2023

szuecs Jul 24, 2023

AlexanderYastrebov Jul 25, 2023

szuecs Aug 1, 2023

szuecs Jul 24, 2023

szuecs Jul 24, 2023

AlexanderYastrebov Jul 25, 2023

szuecs Jul 24, 2023

AlexanderYastrebov Jul 25, 2023

szuecs Jul 24, 2023

szuecs Jul 24, 2023

AlexanderYastrebov Jul 25, 2023

szuecs Jul 25, 2023

AlexanderYastrebov Jul 25, 2023

szuecs Aug 1, 2023

AlexanderYastrebov commented Jul 25, 2023 •

edited

szuecs Aug 22, 2023

AlexanderYastrebov Aug 22, 2023

szuecs Aug 22, 2023

skipper: support http2 over cleartext TCP (h2c) #2480

Are you sure you want to change the base?

skipper: support http2 over cleartext TCP (h2c) #2480

Conversation

AlexanderYastrebov commented Jul 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderYastrebov Jul 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderYastrebov commented Jul 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexanderYastrebov Jul 24, 2023 •

edited

AlexanderYastrebov commented Jul 25, 2023 •

edited