From 375aed5ed67248ed84e36f93e9ef863ee4813ca2 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 23 Dec 2020 12:50:28 -0500 Subject: [PATCH 1/4] xds: Pass in logger small cleanup in tests --- agent/agent.go | 2 +- agent/xds/clusters_test.go | 10 ++++------ agent/xds/endpoints_test.go | 8 +++----- agent/xds/listeners_test.go | 10 ++++------ agent/xds/routes_test.go | 10 ++++------ agent/xds/server.go | 11 +++++------ agent/xds/server_test.go | 15 +++++---------- 7 files changed, 26 insertions(+), 40 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index a05827bb4100..9d98d027689d 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -650,7 +650,7 @@ func (a *Agent) listenAndServeGRPC() error { } xdsServer := &xds.Server{ - Logger: a.logger, + Logger: a.logger.Named(logging.Envoy), CfgMgr: a.proxyConfig, ResolveToken: a.resolveToken, CheckFetcher: a, diff --git a/agent/xds/clusters_test.go b/agent/xds/clusters_test.go index 4883eae7a9f4..0e611bf5c006 100644 --- a/agent/xds/clusters_test.go +++ b/agent/xds/clusters_test.go @@ -10,12 +10,13 @@ import ( envoy "github.com/envoyproxy/go-control-plane/envoy/api/v2" "github.com/golang/protobuf/ptypes/wrappers" + testinf "github.com/mitchellh/go-testing-interface" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/agent/proxycfg" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/xds/proxysupport" "github.com/hashicorp/consul/sdk/testutil" - testinf "github.com/mitchellh/go-testing-interface" - "github.com/stretchr/testify/require" ) func TestClustersFromSnapshot(t *testing.T) { @@ -665,10 +666,7 @@ func TestClustersFromSnapshot(t *testing.T) { } // Need server just for logger dependency - logger := testutil.Logger(t) - s := Server{ - Logger: logger, - } + s := Server{Logger: testutil.Logger(t)} cInfo := connectionInfo{ Token: "my-token", diff --git a/agent/xds/endpoints_test.go b/agent/xds/endpoints_test.go index 0a741582e503..198f600f4dac 100644 --- a/agent/xds/endpoints_test.go +++ b/agent/xds/endpoints_test.go @@ -12,11 +12,12 @@ import ( envoy "github.com/envoyproxy/go-control-plane/envoy/api/v2" envoycore "github.com/envoyproxy/go-control-plane/envoy/api/v2/core" envoyendpoint "github.com/envoyproxy/go-control-plane/envoy/api/v2/endpoint" + testinf "github.com/mitchellh/go-testing-interface" + "github.com/hashicorp/consul/agent/proxycfg" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/xds/proxysupport" "github.com/hashicorp/consul/sdk/testutil" - testinf "github.com/mitchellh/go-testing-interface" ) func Test_makeLoadAssignment(t *testing.T) { @@ -579,10 +580,7 @@ func Test_endpointsFromSnapshot(t *testing.T) { } // Need server just for logger dependency - logger := testutil.Logger(t) - s := Server{ - Logger: logger, - } + s := Server{Logger: testutil.Logger(t)} cInfo := connectionInfo{ Token: "my-token", diff --git a/agent/xds/listeners_test.go b/agent/xds/listeners_test.go index 9e3d855f49ab..953480a13618 100644 --- a/agent/xds/listeners_test.go +++ b/agent/xds/listeners_test.go @@ -9,12 +9,13 @@ import ( envoy "github.com/envoyproxy/go-control-plane/envoy/api/v2" "github.com/envoyproxy/go-control-plane/pkg/wellknown" + testinf "github.com/mitchellh/go-testing-interface" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/agent/proxycfg" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/xds/proxysupport" "github.com/hashicorp/consul/sdk/testutil" - testinf "github.com/mitchellh/go-testing-interface" - "github.com/stretchr/testify/require" ) func TestListenersFromSnapshot(t *testing.T) { @@ -508,10 +509,7 @@ func TestListenersFromSnapshot(t *testing.T) { } // Need server just for logger dependency - logger := testutil.Logger(t) - s := Server{ - Logger: logger, - } + s := Server{Logger: testutil.Logger(t)} cInfo := connectionInfo{ Token: "my-token", diff --git a/agent/xds/routes_test.go b/agent/xds/routes_test.go index bb20fc5379c8..a5798937a496 100644 --- a/agent/xds/routes_test.go +++ b/agent/xds/routes_test.go @@ -9,14 +9,15 @@ import ( envoy "github.com/envoyproxy/go-control-plane/envoy/api/v2" envoyroute "github.com/envoyproxy/go-control-plane/envoy/api/v2/route" "github.com/golang/protobuf/ptypes" + testinf "github.com/mitchellh/go-testing-interface" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/agent/connect" "github.com/hashicorp/consul/agent/consul/discoverychain" "github.com/hashicorp/consul/agent/proxycfg" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/xds/proxysupport" "github.com/hashicorp/consul/sdk/testutil" - testinf "github.com/mitchellh/go-testing-interface" - "github.com/stretchr/testify/require" ) func TestRoutesFromSnapshot(t *testing.T) { @@ -256,10 +257,7 @@ func TestRoutesFromSnapshot(t *testing.T) { tt.setup(snap) } - logger := testutil.Logger(t) - s := Server{ - Logger: logger, - } + s := Server{Logger: testutil.Logger(t)} cInfo := connectionInfo{ Token: "my-token", ProxyFeatures: sf, diff --git a/agent/xds/server.go b/agent/xds/server.go index ed148da49528..e07595bdbde1 100644 --- a/agent/xds/server.go +++ b/agent/xds/server.go @@ -11,17 +11,17 @@ import ( envoycore "github.com/envoyproxy/go-control-plane/envoy/api/v2/core" envoydisco "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v2" "github.com/golang/protobuf/proto" - "github.com/hashicorp/consul/acl" - "github.com/hashicorp/consul/agent/proxycfg" - "github.com/hashicorp/consul/agent/structs" - "github.com/hashicorp/consul/logging" - "github.com/hashicorp/consul/tlsutil" "github.com/hashicorp/go-hclog" "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials" "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" + + "github.com/hashicorp/consul/acl" + "github.com/hashicorp/consul/agent/proxycfg" + "github.com/hashicorp/consul/agent/structs" + "github.com/hashicorp/consul/tlsutil" ) // ADSStream is a shorter way of referring to this thing... @@ -130,7 +130,6 @@ func (s *Server) Initialize() { if s.AuthCheckFrequency == 0 { s.AuthCheckFrequency = DefaultAuthCheckFrequency } - s.Logger = s.Logger.Named(logging.Envoy) } // StreamAggregatedResources implements diff --git a/agent/xds/server_test.go b/agent/xds/server_test.go index b2583fb26425..f619628079b8 100644 --- a/agent/xds/server_test.go +++ b/agent/xds/server_test.go @@ -89,7 +89,6 @@ func (m *testManager) AssertWatchCancelled(t *testing.T, proxyID structs.Service } func TestServer_StreamAggregatedResources_BasicProtocol(t *testing.T) { - logger := testutil.Logger(t) mgr := newTestManager(t) aclResolve := func(id string) (acl.Authorizer, error) { // Allow all @@ -99,7 +98,7 @@ func TestServer_StreamAggregatedResources_BasicProtocol(t *testing.T) { defer envoy.Close() s := Server{ - Logger: logger, + Logger: testutil.Logger(t), CfgMgr: mgr, ResolveToken: aclResolve, } @@ -430,7 +429,6 @@ func TestServer_StreamAggregatedResources_ACLEnforcement(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - logger := testutil.Logger(t) mgr := newTestManager(t) aclResolve := func(id string) (acl.Authorizer, error) { if !tt.defaultDeny { @@ -452,7 +450,7 @@ func TestServer_StreamAggregatedResources_ACLEnforcement(t *testing.T) { defer envoy.Close() s := Server{ - Logger: logger, + Logger: testutil.Logger(t), CfgMgr: mgr, ResolveToken: aclResolve, } @@ -513,7 +511,6 @@ func TestServer_StreamAggregatedResources_ACLTokenDeleted_StreamTerminatedDuring var validToken atomic.Value validToken.Store(token) - logger := testutil.Logger(t) mgr := newTestManager(t) aclResolve := func(id string) (acl.Authorizer, error) { if token := validToken.Load(); token == nil || id != token.(string) { @@ -526,7 +523,7 @@ func TestServer_StreamAggregatedResources_ACLTokenDeleted_StreamTerminatedDuring defer envoy.Close() s := Server{ - Logger: logger, + Logger: testutil.Logger(t), CfgMgr: mgr, ResolveToken: aclResolve, AuthCheckFrequency: 1 * time.Hour, // make sure this doesn't kick in @@ -608,7 +605,6 @@ func TestServer_StreamAggregatedResources_ACLTokenDeleted_StreamTerminatedInBack var validToken atomic.Value validToken.Store(token) - logger := testutil.Logger(t) mgr := newTestManager(t) aclResolve := func(id string) (acl.Authorizer, error) { if token := validToken.Load(); token == nil || id != token.(string) { @@ -621,7 +617,7 @@ func TestServer_StreamAggregatedResources_ACLTokenDeleted_StreamTerminatedInBack defer envoy.Close() s := Server{ - Logger: logger, + Logger: testutil.Logger(t), CfgMgr: mgr, ResolveToken: aclResolve, AuthCheckFrequency: 100 * time.Millisecond, // Make this short. @@ -698,7 +694,6 @@ func TestServer_StreamAggregatedResources_ACLTokenDeleted_StreamTerminatedInBack } func TestServer_StreamAggregatedResources_IngressEmptyResponse(t *testing.T) { - logger := testutil.Logger(t) mgr := newTestManager(t) aclResolve := func(id string) (acl.Authorizer, error) { // Allow all @@ -708,7 +703,7 @@ func TestServer_StreamAggregatedResources_IngressEmptyResponse(t *testing.T) { defer envoy.Close() s := Server{ - Logger: logger, + Logger: testutil.Logger(t), CfgMgr: mgr, ResolveToken: aclResolve, } From 2e2ee413900a41e9d8f7fa797f33a7c50405ba28 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 23 Dec 2020 12:59:05 -0500 Subject: [PATCH 2/4] xds: Fix data race TestEnvoy.Close used e.stream.recvCh == nil to indicate the channel had already been closed, so that TestEnvoy.Close can be called multiple times. The recvCh was not protected by a lock, so setting it to nil caused a data race with any goroutine trying to read from the channel. Instead set the stream to nil. The stream is guarded by a lock, so it does not race. This change allows us to test the agent/xds package using -race. --- .circleci/config.yml | 2 +- agent/xds/testing.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index a178514d5d3d..d095bd366261 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -237,7 +237,7 @@ jobs: command: | mkdir -p $TEST_RESULTS_DIR /tmp/jsonfile pkgs="$(go list ./... | \ - grep -E -v '^github.com/hashicorp/consul/agent(/consul|/local|/xds|/routine-leak-checker)?$' | \ + grep -E -v '^github.com/hashicorp/consul/agent(/consul|/local|/routine-leak-checker)?$' | \ grep -E -v '^github.com/hashicorp/consul/command/')" gotestsum \ --jsonfile /tmp/jsonfile/go-test-race.log \ diff --git a/agent/xds/testing.go b/agent/xds/testing.go index 8a635114a236..7fb9b224c704 100644 --- a/agent/xds/testing.go +++ b/agent/xds/testing.go @@ -189,7 +189,7 @@ func (e *TestEnvoy) Close() error { // unblock the recv chan to simulate recv error when client disconnects if e.stream != nil && e.stream.recvCh != nil { close(e.stream.recvCh) - e.stream.recvCh = nil + e.stream = nil } if e.cancel != nil { e.cancel() From 4b8b2a4291e894658d520dabda87336b386ac095 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 23 Dec 2020 13:08:23 -0500 Subject: [PATCH 3/4] xds: remove Server.Initialize Requiring a call to initialize to set a single field is not really substantially different from having to set that field to a value. --- agent/agent.go | 12 ++++++------ agent/xds/server.go | 7 ------- agent/xds/server_test.go | 5 ----- 3 files changed, 6 insertions(+), 18 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 9d98d027689d..79ec38fadf86 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -650,13 +650,13 @@ func (a *Agent) listenAndServeGRPC() error { } xdsServer := &xds.Server{ - Logger: a.logger.Named(logging.Envoy), - CfgMgr: a.proxyConfig, - ResolveToken: a.resolveToken, - CheckFetcher: a, - CfgFetcher: a, + Logger: a.logger.Named(logging.Envoy), + CfgMgr: a.proxyConfig, + ResolveToken: a.resolveToken, + CheckFetcher: a, + CfgFetcher: a, + AuthCheckFrequency: xds.DefaultAuthCheckFrequency, } - xdsServer.Initialize() var err error if a.config.HTTPSPort > 0 { diff --git a/agent/xds/server.go b/agent/xds/server.go index e07595bdbde1..d8a7ecbe5109 100644 --- a/agent/xds/server.go +++ b/agent/xds/server.go @@ -125,13 +125,6 @@ type Server struct { CfgFetcher ConfigFetcher } -// Initialize will finish configuring the Server for first use. -func (s *Server) Initialize() { - if s.AuthCheckFrequency == 0 { - s.AuthCheckFrequency = DefaultAuthCheckFrequency - } -} - // StreamAggregatedResources implements // envoydisco.AggregatedDiscoveryServiceServer. This is the ADS endpoint which is // the only xDS API we directly support for now. diff --git a/agent/xds/server_test.go b/agent/xds/server_test.go index f619628079b8..3d714e2d4ea3 100644 --- a/agent/xds/server_test.go +++ b/agent/xds/server_test.go @@ -102,7 +102,6 @@ func TestServer_StreamAggregatedResources_BasicProtocol(t *testing.T) { CfgMgr: mgr, ResolveToken: aclResolve, } - s.Initialize() sid := structs.NewServiceID("web-sidecar-proxy", nil) @@ -454,7 +453,6 @@ func TestServer_StreamAggregatedResources_ACLEnforcement(t *testing.T) { CfgMgr: mgr, ResolveToken: aclResolve, } - s.Initialize() errCh := make(chan error, 1) go func() { @@ -528,7 +526,6 @@ func TestServer_StreamAggregatedResources_ACLTokenDeleted_StreamTerminatedDuring ResolveToken: aclResolve, AuthCheckFrequency: 1 * time.Hour, // make sure this doesn't kick in } - s.Initialize() errCh := make(chan error, 1) go func() { @@ -622,7 +619,6 @@ func TestServer_StreamAggregatedResources_ACLTokenDeleted_StreamTerminatedInBack ResolveToken: aclResolve, AuthCheckFrequency: 100 * time.Millisecond, // Make this short. } - s.Initialize() errCh := make(chan error, 1) go func() { @@ -707,7 +703,6 @@ func TestServer_StreamAggregatedResources_IngressEmptyResponse(t *testing.T) { CfgMgr: mgr, ResolveToken: aclResolve, } - s.Initialize() sid := structs.NewServiceID("ingress-gateway", nil) From a04cefaa287c99d6d0d26efe6bfe4ca72039d80b Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 23 Dec 2020 13:13:49 -0500 Subject: [PATCH 4/4] Remove an unnecessary else --- agent/agent.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 79ec38fadf86..1d48cd85defd 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -658,14 +658,14 @@ func (a *Agent) listenAndServeGRPC() error { AuthCheckFrequency: xds.DefaultAuthCheckFrequency, } - var err error - if a.config.HTTPSPort > 0 { - // gRPC uses the same TLS settings as the HTTPS API. If HTTPS is - // enabled then gRPC will require HTTPS as well. - a.grpcServer, err = xdsServer.GRPCServer(a.tlsConfigurator) - } else { - a.grpcServer, err = xdsServer.GRPCServer(nil) + tlsConfig := a.tlsConfigurator + // gRPC uses the same TLS settings as the HTTPS API. If HTTPS is not enabled + // then gRPC should not use TLS. + if a.config.HTTPSPort <= 0 { + tlsConfig = nil } + var err error + a.grpcServer, err = xdsServer.GRPCServer(tlsConfig) if err != nil { return err }