-
Notifications
You must be signed in to change notification settings - Fork 350
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
package net | ||
|
||
import ( | ||
"context" | ||
"net" | ||
"sync" | ||
"sync/atomic" | ||
"time" | ||
|
||
log "github.com/sirupsen/logrus" | ||
) | ||
|
||
type ( | ||
ShutdownListener struct { | ||
net.Listener | ||
activeConns atomic.Int64 | ||
} | ||
|
||
shutdownListenerConn struct { | ||
net.Conn | ||
listener *ShutdownListener | ||
once sync.Once | ||
} | ||
) | ||
|
||
var _ net.Listener = &ShutdownListener{} | ||
|
||
func NewShutdownListener(l net.Listener) *ShutdownListener { | ||
return &ShutdownListener{Listener: l} | ||
} | ||
|
||
func (l *ShutdownListener) Accept() (net.Conn, error) { | ||
c, err := l.Listener.Accept() | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
l.registerConn() | ||
|
||
return &shutdownListenerConn{Conn: c, listener: l}, nil | ||
} | ||
|
||
func (l *ShutdownListener) Shutdown(ctx context.Context) error { | ||
ticker := time.NewTicker(500 * time.Millisecond) | ||
defer ticker.Stop() | ||
for { | ||
n := l.activeConns.Load() | ||
log.Debugf("ShutdownListener Shutdown: %d active connections", n) | ||
if n == 0 { | ||
return nil | ||
} | ||
select { | ||
case <-ctx.Done(): | ||
return ctx.Err() | ||
case <-ticker.C: | ||
} | ||
} | ||
} | ||
|
||
func (c *shutdownListenerConn) Close() error { | ||
err := c.Conn.Close() | ||
|
||
c.once.Do(c.listener.unregisterConn) | ||
|
||
return err | ||
} | ||
|
||
func (l *ShutdownListener) registerConn() { | ||
n := l.activeConns.Add(1) | ||
log.Debugf("ShutdownListener registerConn: %d active connections", n) | ||
} | ||
|
||
func (l *ShutdownListener) unregisterConn() { | ||
n := l.activeConns.Add(-1) | ||
log.Debugf("ShutdownListener unregisterConn: %d active connections", n) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,8 @@ import ( | |
ot "github.com/opentracing/opentracing-go" | ||
"github.com/prometheus/client_golang/prometheus" | ||
log "github.com/sirupsen/logrus" | ||
"golang.org/x/net/http2" | ||
"golang.org/x/net/http2/h2c" | ||
|
||
"github.com/zalando/skipper/circuit" | ||
"github.com/zalando/skipper/dataclients/kubernetes" | ||
|
@@ -42,7 +44,7 @@ import ( | |
"github.com/zalando/skipper/loadbalancer" | ||
"github.com/zalando/skipper/logging" | ||
"github.com/zalando/skipper/metrics" | ||
skpnet "github.com/zalando/skipper/net" | ||
snet "github.com/zalando/skipper/net" | ||
pauth "github.com/zalando/skipper/predicates/auth" | ||
"github.com/zalando/skipper/predicates/content" | ||
"github.com/zalando/skipper/predicates/cookie" | ||
|
@@ -381,6 +383,9 @@ type Options struct { | |
// a backend to always create a new connection. | ||
DisableHTTPKeepalives bool | ||
|
||
// EnableHttp2Cleartext enables HTTP/2 connections over cleartext TCP. | ||
EnableHttp2Cleartext bool | ||
|
||
// Flag indicating to ignore trailing slashes in paths during route | ||
// lookup. | ||
IgnoreTrailingSlash bool | ||
|
@@ -1107,6 +1112,10 @@ func (o *Options) tlsConfig(cr *certregistry.CertRegistry) (*tls.Config, error) | |
return nil, nil | ||
} | ||
|
||
if o.EnableHttp2Cleartext { | ||
return nil, fmt.Errorf("HTTP/2 connections over cleartext TCP are not supported when TLS is enabled") | ||
szuecs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
config := &tls.Config{ | ||
MinVersion: o.TLSMinVersion, | ||
} | ||
|
@@ -1233,11 +1242,27 @@ func listenAndServeQuit( | |
} | ||
} | ||
|
||
if o.EnableHttp2Cleartext { | ||
h2srv := &http2.Server{} | ||
srv.Handler = h2c.NewHandler(srv.Handler, h2srv) | ||
|
||
// Work around https://github.com/golang/go/issues/26682 | ||
// http2.ConfigureServer registers unexported h2srv graceful shutdown handler on srv shutdown - | ||
// it calls srv.RegisterOnShutdown(h2srv.state.startGracefulShutdown). | ||
// h2srv graceful shutdown handler sends GOAWAY frame to all connections and closes them after predefined delay. | ||
// | ||
// srv.Shutdown() runs h2srv shutdown handler in a goroutine so a special snet.ShutdownListener | ||
// waits until all connections are closed. | ||
http2.ConfigureServer(srv, h2srv) | ||
} | ||
|
||
log.Infof("Listen on %v", address) | ||
|
||
l, err := listen(o, address, mtr) | ||
if err != nil { | ||
var listener *snet.ShutdownListener | ||
if l, err := listen(o, address, mtr); err != nil { | ||
return err | ||
} else { | ||
listener = snet.NewShutdownListener(l) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will also wait for hijacked connection when h2c is not enabled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it work with upgrade connections like websocket? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://pkg.go.dev/net/http#Server.Shutdown
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)). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you create a follow up issue for that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I plan to check this before we merge. |
||
} | ||
|
||
// making idleConnsCH and sigs optional parameters is required to be able to tear down a server | ||
|
@@ -1258,10 +1283,16 @@ func listenAndServeQuit( | |
log.Infof("Got shutdown signal, wait %v for health check", o.WaitForHealthcheckInterval) | ||
time.Sleep(o.WaitForHealthcheckInterval) | ||
|
||
log.Info("Start shutdown") | ||
log.Info("Start server shutdown") | ||
if err := srv.Shutdown(context.Background()); err != nil { | ||
log.Errorf("Failed to graceful shutdown: %v", err) | ||
log.Errorf("Failed to gracefully shutdown: %v", err) | ||
} | ||
|
||
log.Info("Start listener shutdown") | ||
if err := listener.Shutdown(context.Background()); err != nil { | ||
log.Errorf("Failed to gracefully shutdown listener: %v", err) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add also: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have log.Infof("done.") at the end |
||
close(idleConnsCH) | ||
}() | ||
|
||
|
@@ -1281,20 +1312,21 @@ func listenAndServeQuit( | |
}() | ||
} | ||
|
||
if err := srv.ServeTLS(l, "", ""); err != http.ErrServerClosed { | ||
if err := srv.ServeTLS(listener, "", ""); err != http.ErrServerClosed { | ||
log.Errorf("ServeTLS failed: %v", err) | ||
return err | ||
} | ||
} else { | ||
log.Infof("TLS settings not found, defaulting to HTTP") | ||
|
||
if err := srv.Serve(l); err != http.ErrServerClosed { | ||
if err := srv.Serve(listener); err != http.ErrServerClosed { | ||
log.Errorf("Serve failed: %v", err) | ||
return err | ||
} | ||
} | ||
|
||
<-idleConnsCH | ||
|
||
log.Infof("done.") | ||
return nil | ||
} | ||
|
@@ -1580,13 +1612,13 @@ func run(o Options, sig chan os.Signal, idleConnsCH chan struct{}) error { | |
} | ||
|
||
var swarmer ratelimit.Swarmer | ||
var redisOptions *skpnet.RedisOptions | ||
var redisOptions *snet.RedisOptions | ||
log.Infof("enable swarm: %v", o.EnableSwarm) | ||
if o.EnableSwarm { | ||
if len(o.SwarmRedisURLs) > 0 || o.KubernetesRedisServiceName != "" || o.SwarmRedisEndpointsRemoteURL != "" { | ||
log.Infof("Redis based swarm with %d shards", len(o.SwarmRedisURLs)) | ||
|
||
redisOptions = &skpnet.RedisOptions{ | ||
redisOptions = &snet.RedisOptions{ | ||
Addrs: o.SwarmRedisURLs, | ||
Password: o.SwarmRedisPassword, | ||
HashAlgorithm: o.SwarmRedisHashAlgorithm, | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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