From 9d471269d8dc786dee0979481b96c94f08c22091 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 9 Jun 2021 12:14:36 -0400 Subject: [PATCH] agent: fix a data race in a test The test was modifying a pointer to a struct that had been passed to another goroutine. Instead create a new struct to modify. ``` WARNING: DATA RACE Write at 0x00c01407c3c0 by goroutine 832: github.com/hashicorp/consul/agent.TestServiceManager_PersistService_API() /home/daniel/pers/code/consul/agent/service_manager_test.go:446 +0x1d86 testing.tRunner() /usr/lib/go/src/testing/testing.go:1193 +0x202 Previous read at 0x00c01407c3c0 by goroutine 938: reflect.typedmemmove() /usr/lib/go/src/runtime/mbarrier.go:177 +0x0 reflect.Value.Set() /usr/lib/go/src/reflect/value.go:1569 +0x13b github.com/mitchellh/copystructure.(*walker).Primitive() /home/daniel/go/pkg/mod/github.com/mitchellh/copystructure@v1.0.0/copystructure.go:289 +0x190 github.com/mitchellh/reflectwalk.walkPrimitive() /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:252 +0x31b github.com/mitchellh/reflectwalk.walk() /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:179 +0x24d github.com/mitchellh/reflectwalk.walkStruct() /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:386 +0x4ec github.com/mitchellh/reflectwalk.walk() /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:188 +0x656 github.com/mitchellh/reflectwalk.walkStruct() /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:386 +0x4ec github.com/mitchellh/reflectwalk.walk() /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:188 +0x656 github.com/mitchellh/reflectwalk.Walk() /home/daniel/go/pkg/mod/github.com/mitchellh/reflectwalk@v1.0.1/reflectwalk.go:92 +0x164 github.com/mitchellh/copystructure.Config.Copy() /home/daniel/go/pkg/mod/github.com/mitchellh/copystructure@v1.0.0/copystructure.go:69 +0xe7 github.com/mitchellh/copystructure.Copy() /home/daniel/go/pkg/mod/github.com/mitchellh/copystructure@v1.0.0/copystructure.go:13 +0x84 github.com/hashicorp/consul/agent.mergeServiceConfig() /home/daniel/pers/code/consul/agent/service_manager.go:362 +0x56 github.com/hashicorp/consul/agent.(*serviceConfigWatch).handleUpdate() /home/daniel/pers/code/consul/agent/service_manager.go:279 +0x250 github.com/hashicorp/consul/agent.(*serviceConfigWatch).runWatch() /home/daniel/pers/code/consul/agent/service_manager.go:246 +0x2d4 Goroutine 832 (running) created at: testing.(*T).Run() /usr/lib/go/src/testing/testing.go:1238 +0x5d7 testing.runTests.func1() /usr/lib/go/src/testing/testing.go:1511 +0xa6 testing.tRunner() /usr/lib/go/src/testing/testing.go:1193 +0x202 testing.runTests() /usr/lib/go/src/testing/testing.go:1509 +0x612 testing.(*M).Run() /usr/lib/go/src/testing/testing.go:1417 +0x3b3 main.main() _testmain.go:1181 +0x236 Goroutine 938 (running) created at: github.com/hashicorp/consul/agent.(*serviceConfigWatch).start() /home/daniel/pers/code/consul/agent/service_manager.go:223 +0x4e4 github.com/hashicorp/consul/agent.(*ServiceManager).AddService() /home/daniel/pers/code/consul/agent/service_manager.go:98 +0x344 github.com/hashicorp/consul/agent.(*Agent).addServiceLocked() /home/daniel/pers/code/consul/agent/agent.go:1942 +0x2e4 github.com/hashicorp/consul/agent.(*Agent).AddService() /home/daniel/pers/code/consul/agent/agent.go:1929 +0x337 github.com/hashicorp/consul/agent.TestServiceManager_PersistService_API() /home/daniel/pers/code/consul/agent/service_manager_test.go:400 +0x17c4 testing.tRunner() /usr/lib/go/src/testing/testing.go:1193 +0x202 ``` --- agent/service_manager_test.go | 44 +++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/agent/service_manager_test.go b/agent/service_manager_test.go index 876d9f144..b6c14f7f0 100644 --- a/agent/service_manager_test.go +++ b/agent/service_manager_test.go @@ -8,11 +8,12 @@ import ( "path/filepath" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func TestServiceManager_RegisterService(t *testing.T) { @@ -330,26 +331,27 @@ func TestServiceManager_PersistService_API(t *testing.T) { testrpc.WaitForLeader(t, a.RPC, "dc1") - // Now register a sidecar proxy via the API. - svc := &structs.NodeService{ - Kind: structs.ServiceKindConnectProxy, - ID: "web-sidecar-proxy", - Service: "web-sidecar-proxy", - Port: 21000, - Proxy: structs.ConnectProxyConfig{ - DestinationServiceName: "web", - DestinationServiceID: "web", - LocalServiceAddress: "127.0.0.1", - LocalServicePort: 8000, - Upstreams: structs.Upstreams{ - { - DestinationName: "redis", - DestinationNamespace: "default", - LocalBindPort: 5000, + newNodeService := func() *structs.NodeService { + return &structs.NodeService{ + Kind: structs.ServiceKindConnectProxy, + ID: "web-sidecar-proxy", + Service: "web-sidecar-proxy", + Port: 21000, + Proxy: structs.ConnectProxyConfig{ + DestinationServiceName: "web", + DestinationServiceID: "web", + LocalServiceAddress: "127.0.0.1", + LocalServicePort: 8000, + Upstreams: structs.Upstreams{ + { + DestinationName: "redis", + DestinationNamespace: "default", + LocalBindPort: 5000, + }, }, }, - }, - EnterpriseMeta: *structs.DefaultEnterpriseMeta(), + EnterpriseMeta: *structs.DefaultEnterpriseMeta(), + } } expectState := &structs.NodeService{ @@ -385,6 +387,7 @@ func TestServiceManager_PersistService_API(t *testing.T) { EnterpriseMeta: *structs.DefaultEnterpriseMeta(), } + svc := newNodeService() svcID := svc.CompoundServiceID() svcFile := filepath.Join(a.Config.DataDir, servicesDir, svcID.StringHash()) @@ -443,6 +446,7 @@ func TestServiceManager_PersistService_API(t *testing.T) { } // Updates service definition on disk + svc = newNodeService() svc.Proxy.LocalServicePort = 8001 require.NoError(a.addServiceFromSource(svc, nil, true, "mytoken", ConfigSourceRemote)) requireFileIsPresent(t, svcFile)