From de226f26e468d2af9bc9ed13313fa373790fc619 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 a05827bb4..9d98d0276 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 4883eae7a..0e611bf5c 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 0a741582e..198f600f4 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 9e3d855f4..953480a13 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 bb20fc537..a5798937a 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 ed148da49..e07595bdb 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 b2583fb26..f61962807 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 bbf1a116f6cbed04715f00d63e1ae5671f67e0a3 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 a178514d5..d095bd366 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 8a635114a..7fb9b224c 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 f6543b1651948a5bbbf888111cedfdc3b09462d4 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 9d98d0276..79ec38fad 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 e07595bdb..d8a7ecbe5 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 f61962807..3d714e2d4 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 bcfb444a778f83c922921e3f5b23fed66ecfaf05 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 79ec38fad..1d48cd85d 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 }