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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ type Config struct {
ExpectContinueTimeoutBackend time.Duration `yaml:"expect-continue-timeout-backend"`
MaxIdleConnsBackend int `yaml:"max-idle-connection-backend"`
DisableHTTPKeepalives bool `yaml:"disable-http-keepalives"`
EnableHttp2Cleartext bool `yaml:"enable-http2-cleartext"`

// swarm:
EnableSwarm bool `yaml:"enable-swarm"`
Expand Down Expand Up @@ -523,6 +524,7 @@ func NewConfig() *Config {
flag.IntVar(&cfg.MaxIdleConnsBackend, "max-idle-connection-backend", 0, "sets the maximum idle connections for all backend connections")
flag.BoolVar(&cfg.DisableHTTPKeepalives, "disable-http-keepalives", false, "forces backend to always create a new connection")
flag.BoolVar(&cfg.KubernetesEnableTLS, "kubernetes-enable-tls", false, "enable using kubnernetes resources to terminate tls")
flag.BoolVar(&cfg.EnableHttp2Cleartext, "enable-http2-cleartext", false, "enables HTTP/2 connections over cleartext TCP")

// Swarm:
flag.BoolVar(&cfg.EnableSwarm, "enable-swarm", false, "enable swarm communication between nodes in a skipper fleet")
Expand Down Expand Up @@ -850,6 +852,7 @@ func (c *Config) ToOptions() skipper.Options {
MaxIdleConnsBackend: c.MaxIdleConnsBackend,
DisableHTTPKeepalives: c.DisableHTTPKeepalives,
KubernetesEnableTLS: c.KubernetesEnableTLS,
EnableHttp2Cleartext: c.EnableHttp2Cleartext,

// swarm:
EnableSwarm: c.EnableSwarm,
Expand Down
76 changes: 76 additions & 0 deletions net/shutdown_listener.go
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)
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 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)
}
50 changes: 41 additions & 9 deletions skipper.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
}
Expand Down Expand Up @@ -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)
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.

}

// making idleConnsCH and sigs optional parameters is required to be able to tear down a server
Expand All @@ -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)
}

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

close(idleConnsCH)
}()

Expand All @@ -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
}
Expand Down Expand Up @@ -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,
Expand Down
Loading