Skip to content

Commit

Permalink
integration: 'go test -tags cluster_proxy -v ./integration/... ./clie…
Browse files Browse the repository at this point in the history
…ntv3/...' passes now.

The grpc-proxy test logic was assuming that the context associated to client is closed,
while in practice all tests called client.Close() without explicit context close.

The current testing strategy is complicated 2 fold:
  - grpc proxy works like man-in-the middle of each Connection issues
from integration tests and its lifetime is bound to the connection.
  - both connections (client -> proxy, and proxy -> etcd-server) are
represented by the same ClientV3 object instance (with substituted
implementations of KV or watcher).

The fix splits context representing proxy from context representing proxy -> etcd-server connection,
thus allowing cancelation of the proxy context.
  • Loading branch information
ptabor committed Sep 22, 2020
1 parent 205a656 commit 33b26b0
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 28 deletions.
7 changes: 4 additions & 3 deletions clientv3/naming/grpc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package naming
package naming_test

import (
"context"
Expand All @@ -21,6 +21,7 @@ import (
"testing"

etcd "go.etcd.io/etcd/v3/clientv3"
namingv3 "go.etcd.io/etcd/v3/clientv3/naming"
"go.etcd.io/etcd/v3/integration"
"go.etcd.io/etcd/v3/pkg/testutil"

Expand All @@ -33,7 +34,7 @@ func TestGRPCResolver(t *testing.T) {
clus := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 1})
defer clus.Terminate(t)

r := GRPCResolver{
r := namingv3.GRPCResolver{
Client: clus.RandClient(),
}

Expand Down Expand Up @@ -107,7 +108,7 @@ func TestGRPCResolverMulti(t *testing.T) {
t.Fatal(err)
}

r := GRPCResolver{c}
r := namingv3.GRPCResolver{c}

w, err := r.Resolve("foo")
if err != nil {
Expand Down
7 changes: 5 additions & 2 deletions etcdmain/grpc_proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -398,12 +398,15 @@ func newGRPCProxyServer(lg *zap.Logger, client *clientv3.Client) *grpc.Server {
}

kvp, _ := grpcproxy.NewKvProxy(client)
watchp, _ := grpcproxy.NewWatchProxy(lg, client)
watchp, _ := grpcproxy.NewWatchProxy(client.Ctx(), lg, client)
if grpcProxyResolverPrefix != "" {
grpcproxy.Register(lg, client, grpcProxyResolverPrefix, grpcProxyAdvertiseClientURL, grpcProxyResolverTTL)
}
clusterp, _ := grpcproxy.NewClusterProxy(lg, client, grpcProxyAdvertiseClientURL, grpcProxyResolverPrefix)
leasep, _ := grpcproxy.NewLeaseProxy(client)
leasep, _ := grpcproxy.NewLeaseProxy(client.Ctx(), client)

// grpcproxy.

mainp := grpcproxy.NewMaintenanceProxy(client)
authp := grpcproxy.NewAuthProxy(client)
electionp := grpcproxy.NewElectionProxy(client)
Expand Down
44 changes: 27 additions & 17 deletions integration/cluster_proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package integration

import (
"context"
"sync"

"go.etcd.io/etcd/v3/clientv3"
Expand All @@ -37,16 +38,23 @@ var (
const proxyNamespace = "proxy-namespace"

type grpcClientProxy struct {
grpc grpcAPI
wdonec <-chan struct{}
kvdonec <-chan struct{}
lpdonec <-chan struct{}
ctx context.Context
ctxCancel func()
grpc grpcAPI
wdonec <-chan struct{}
kvdonec <-chan struct{}
lpdonec <-chan struct{}
}

func toGRPC(c *clientv3.Client) grpcAPI {
pmu.Lock()
defer pmu.Unlock()

// dedicated context bound to 'grpc-proxy' lifetype
// (so in practice lifetime of the client connection to the proxy).
// TODO: Refactor to a separate clientv3.Client instance instead of the context alone.
ctx, ctxCancel := context.WithCancel(context.WithValue(context.TODO(), "_name", "grpcProxyContext"))

lg := zap.NewExample()

if v, ok := proxies[c]; ok {
Expand All @@ -59,8 +67,8 @@ func toGRPC(c *clientv3.Client) grpcAPI {
c.Lease = namespace.NewLease(c.Lease, proxyNamespace)
// test coalescing/caching proxy
kvp, kvpch := grpcproxy.NewKvProxy(c)
wp, wpch := grpcproxy.NewWatchProxy(lg, c)
lp, lpch := grpcproxy.NewLeaseProxy(c)
wp, wpch := grpcproxy.NewWatchProxy(ctx, lg, c)
lp, lpch := grpcproxy.NewLeaseProxy(ctx, c)
mp := grpcproxy.NewMaintenanceProxy(c)
clp, _ := grpcproxy.NewClusterProxy(lg, c, "", "") // without registering proxy URLs
authp := grpcproxy.NewAuthProxy(c)
Expand All @@ -77,20 +85,21 @@ func toGRPC(c *clientv3.Client) grpcAPI {
adapter.LockServerToLockClient(lockp),
adapter.ElectionServerToElectionClient(electp),
}
proxies[c] = grpcClientProxy{grpc: grpc, wdonec: wpch, kvdonec: kvpch, lpdonec: lpch}
proxies[c] = grpcClientProxy{ctx: ctx, ctxCancel: ctxCancel, grpc: grpc, wdonec: wpch, kvdonec: kvpch, lpdonec: lpch}
return grpc
}

type proxyCloser struct {
clientv3.Watcher
wdonec <-chan struct{}
kvdonec <-chan struct{}
lclose func()
lpdonec <-chan struct{}
proxyCtxCancel func()
wdonec <-chan struct{}
kvdonec <-chan struct{}
lclose func()
lpdonec <-chan struct{}
}

func (pc *proxyCloser) Close() error {
// client ctx is canceled before calling close, so kv and lp will close out
pc.proxyCtxCancel()
<-pc.kvdonec
err := pc.Watcher.Close()
<-pc.wdonec
Expand All @@ -110,11 +119,12 @@ func newClientV3(cfg clientv3.Config) (*clientv3.Client, error) {
lc := c.Lease
c.Lease = clientv3.NewLeaseFromLeaseClient(rpc.Lease, c, cfg.DialTimeout)
c.Watcher = &proxyCloser{
Watcher: clientv3.NewWatchFromWatchClient(rpc.Watch, c),
wdonec: proxies[c].wdonec,
kvdonec: proxies[c].kvdonec,
lclose: func() { lc.Close() },
lpdonec: proxies[c].lpdonec,
Watcher: clientv3.NewWatchFromWatchClient(rpc.Watch, c),
wdonec: proxies[c].wdonec,
kvdonec: proxies[c].kvdonec,
lclose: func() { lc.Close() },
lpdonec: proxies[c].lpdonec,
proxyCtxCancel: proxies[c].ctxCancel,
}
pmu.Unlock()
return c, nil
Expand Down
6 changes: 3 additions & 3 deletions proxy/grpcproxy/lease.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,13 @@ type leaseProxy struct {
wg sync.WaitGroup
}

func NewLeaseProxy(c *clientv3.Client) (pb.LeaseServer, <-chan struct{}) {
cctx, cancel := context.WithCancel(c.Ctx())
func NewLeaseProxy(ctx context.Context, c *clientv3.Client) (pb.LeaseServer, <-chan struct{}) {
cctx, cancel := context.WithCancel(ctx)
lp := &leaseProxy{
leaseClient: pb.NewLeaseClient(c.ActiveConnection()),
lessor: c.Lease,
ctx: cctx,
leader: newLeader(c.Ctx(), c.Watcher),
leader: newLeader(cctx, c.Watcher),
}
ch := make(chan struct{})
go func() {
Expand Down
6 changes: 3 additions & 3 deletions proxy/grpcproxy/watch.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,12 @@ type watchProxy struct {
lg *zap.Logger
}

func NewWatchProxy(lg *zap.Logger, c *clientv3.Client) (pb.WatchServer, <-chan struct{}) {
cctx, cancel := context.WithCancel(c.Ctx())
func NewWatchProxy(ctx context.Context, lg *zap.Logger, c *clientv3.Client) (pb.WatchServer, <-chan struct{}) {
cctx, cancel := context.WithCancel(ctx)
wp := &watchProxy{
cw: c.Watcher,
ctx: cctx,
leader: newLeader(c.Ctx(), c.Watcher),
leader: newLeader(cctx, c.Watcher),

kv: c.KV, // for permission checking
lg: lg,
Expand Down

0 comments on commit 33b26b0

Please sign in to comment.