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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AlexanderYastrebov
Copy link
Member

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

For #1253

return err
} else {
listener = snet.NewShutdownListener(l)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work with upgrade connections like websocket?

Copy link
Member Author

@AlexanderYastrebov AlexanderYastrebov Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you create a follow up issue for that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to check this before we merge.

net/shutdown_listener.go Outdated Show resolved Hide resolved
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's way too often

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?
Maybe better expose a metric.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for debugging, IMO does not hurt

skipper.go Show resolved Hide resolved
return err
} else {
listener = snet.NewShutdownListener(l)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work with upgrade connections like websocket?

}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

testServerShutdown(t, o, protoH2C, testGracefulConnectionShutdown)

for i := 0; i < connectionShutdownChecks; i++ {
require.NoError(t, <-errc, "Expected to receive GOAWAY frame on shutdown")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not check that we received a GOAWAY frame?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we did not receive it will fail with EOF error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <[email protected]>
@AlexanderYastrebov
Copy link
Member Author

AlexanderYastrebov commented Jul 25, 2023

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

return &http.Client{
Transport: &http2.Transport{
// allow http scheme
AllowHTTP: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be even better to support a scheme

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

Successfully merging this pull request may close these issues.

2 participants