diff --git a/agent/local/state.go b/agent/local/state.go index d8ad529f71..527a9b04a9 100644 --- a/agent/local/state.go +++ b/agent/local/state.go @@ -1082,34 +1082,31 @@ func (l *State) updateSyncState() error { continue } - // to avoid a data race with the service struct, - // We copy the Service struct, mutate it and replace the pointer - svc := *ls.Service - // If our definition is different, we need to update it. Make a // copy so that we don't retain a pointer to any actual state // store info for in-memory RPCs. - if svc.EnableTagOverride { - svc.Tags = make([]string, len(rs.Tags)) - copy(svc.Tags, rs.Tags) + if ls.Service.EnableTagOverride { + ls.Service.Tags = make([]string, len(rs.Tags)) + copy(ls.Service.Tags, rs.Tags) } // Merge any tagged addresses with the consul- prefix (set by the server) // back into the local state. - if !reflect.DeepEqual(svc.TaggedAddresses, rs.TaggedAddresses) { - if svc.TaggedAddresses == nil { - svc.TaggedAddresses = make(map[string]structs.ServiceAddress) + if !reflect.DeepEqual(ls.Service.TaggedAddresses, rs.TaggedAddresses) { + // Make a copy of TaggedAddresses to prevent races when writing + // since other goroutines may be reading from the map + m := make(map[string]structs.ServiceAddress) + for k, v := range ls.Service.TaggedAddresses { + m[k] = v } for k, v := range rs.TaggedAddresses { if strings.HasPrefix(k, structs.MetaKeyReservedPrefix) { - svc.TaggedAddresses[k] = v + m[k] = v } } + ls.Service.TaggedAddresses = m } - ls.InSync = svc.IsSame(rs) - - // replace the service pointer to the new mutated struct - ls.Service = &svc + ls.InSync = ls.Service.IsSame(rs) } // Check which checks need syncing diff --git a/agent/local/state_test.go b/agent/local/state_test.go index 679adba84b..7deb2893f9 100644 --- a/agent/local/state_test.go +++ b/agent/local/state_test.go @@ -373,36 +373,41 @@ func TestAgentAntiEntropy_Services_ConnectProxy(t *testing.T) { // We should have 5 services (consul included) assert.Len(services.NodeServices.Services, 5) - // All the services should match + // Check that virtual IPs have been set vips := make(map[string]struct{}) - srv1.TaggedAddresses = nil - srv2.TaggedAddresses = nil - for id, serv := range services.NodeServices.Services { - serv.CreateIndex, serv.ModifyIndex = 0, 0 + for _, serv := range services.NodeServices.Services { if serv.TaggedAddresses != nil { serviceVIP := serv.TaggedAddresses[structs.TaggedAddressVirtualIP].Address assert.NotEmpty(serviceVIP) vips[serviceVIP] = struct{}{} } - serv.TaggedAddresses = nil - switch id { - case "mysql-proxy": - assert.Equal(srv1, serv) - case "redis-proxy": - assert.Equal(srv2, serv) - case "web-proxy": - assert.Equal(srv3, serv) - case "cache-proxy": - assert.Equal(srv5, serv) - case structs.ConsulServiceID: - // ignore - default: - t.Fatalf("unexpected service: %v", id) - } } - assert.Len(vips, 4) - assert.Nil(servicesInSync(a.State, 4, structs.DefaultEnterpriseMetaInDefaultPartition())) + + // All the services should match + // Retry to mitigate data races between local and remote state + retry.Run(t, func(r *retry.R) { + require.NoError(r, a.State.SyncFull()) + for id, serv := range services.NodeServices.Services { + serv.CreateIndex, serv.ModifyIndex = 0, 0 + switch id { + case "mysql-proxy": + require.Equal(r, srv1, serv) + case "redis-proxy": + require.Equal(r, srv2, serv) + case "web-proxy": + require.Equal(r, srv3, serv) + case "cache-proxy": + require.Equal(r, srv5, serv) + case structs.ConsulServiceID: + // ignore + default: + r.Fatalf("unexpected service: %v", id) + } + } + }) + + assert.NoError(servicesInSync(a.State, 4, structs.DefaultEnterpriseMetaInDefaultPartition())) // Remove one of the services a.State.RemoveService(structs.NewServiceID("cache-proxy", nil)) @@ -415,12 +420,6 @@ func TestAgentAntiEntropy_Services_ConnectProxy(t *testing.T) { // All the services should match for id, serv := range services.NodeServices.Services { serv.CreateIndex, serv.ModifyIndex = 0, 0 - if serv.TaggedAddresses != nil { - serviceVIP := serv.TaggedAddresses[structs.TaggedAddressVirtualIP].Address - assert.NotEmpty(serviceVIP) - vips[serviceVIP] = struct{}{} - } - serv.TaggedAddresses = nil switch id { case "mysql-proxy": assert.Equal(srv1, serv)