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

[3.4] embed: fix nil pointer dereference when stopServer #16195

Merged
merged 2 commits into from
Jul 11, 2023
Merged
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
14 changes: 8 additions & 6 deletions embed/etcd.go
Original file line number Diff line number Diff line change
Expand Up @@ -424,18 +424,20 @@ func (e *Etcd) Close() {
}

func stopServers(ctx context.Context, ss *servers) {
shutdownNow := func() {
// first, close the http.Server
// first, close the http.Server
if ss.http != nil {
ss.http.Shutdown(ctx)
// then close grpc.Server; cancels all active RPCs
ss.grpc.Stop()
}

if ss.grpc == nil {
return
}
ahrtr marked this conversation as resolved.
Show resolved Hide resolved

// do not grpc.Server.GracefulStop with TLS enabled etcd server
// See https://github.com/grpc/grpc-go/issues/1384#issuecomment-317124531
// and https://github.com/etcd-io/etcd/issues/8916
if ss.secure && ss.http != nil {
shutdownNow()
ss.grpc.Stop()
return
}

Expand All @@ -453,7 +455,7 @@ func stopServers(ctx context.Context, ss *servers) {
case <-ctx.Done():
// took too long, manually close open transports
// e.g. watch streams
shutdownNow()
ss.grpc.Stop()

// concurrent GracefulStop should be interrupted
<-ch
Expand Down
10 changes: 10 additions & 0 deletions tests/e2e/cluster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ type etcdProcessClusterConfig struct {
WatchProcessNotifyInterval time.Duration

debug bool

stopSignal os.Signal
}

// newEtcdProcessCluster launches a new cluster from etcd processes, returning
Expand All @@ -151,12 +153,20 @@ func newEtcdProcessCluster(cfg *etcdProcessClusterConfig) (*etcdProcessCluster,
epc.Close()
return nil, err
}

epc.procs[i] = proc
}

if err := epc.Start(); err != nil {
return nil, err
}

// overwrite the default signal
if cfg.stopSignal != nil {
for _, proc := range epc.procs {
proc.WithStopSignal(cfg.stopSignal)
}
}
return epc, nil
}

Expand Down
14 changes: 12 additions & 2 deletions tests/e2e/cmux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"context"
"encoding/json"
"fmt"
"syscall"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -63,10 +64,19 @@ func TestConnectionMultiplexing(t *testing.T) {
} {
t.Run(tc.name, func(t *testing.T) {
ctx := context.Background()
cfg := etcdProcessClusterConfig{clusterSize: 1, clientTLS: tc.serverTLS, enableV2: true, clientHttpSeparate: tc.separateHttpPort}
cfg := etcdProcessClusterConfig{
clusterSize: 1,
clientTLS: tc.serverTLS,
enableV2: true,
clientHttpSeparate: tc.separateHttpPort,
stopSignal: syscall.SIGTERM, // check graceful stop
ahrtr marked this conversation as resolved.
Show resolved Hide resolved
}
clus, err := newEtcdProcessCluster(&cfg)
require.NoError(t, err)
defer clus.Close()

defer func() {
require.NoError(t, clus.Close())
}()

var clientScenarios []clientConnType
switch tc.serverTLS {
Expand Down