clone the service under lock to avoid a data race (#11940)

* clone the service under lock to avoid a data race

* add change log

* create a struct and copy the pointer to mutate it to avoid a data race

* fix failing test

* revert added space

* add comments, to clarify the data race.
This commit is contained in:
Dhia Ayachi 2022-01-06 14:33:06 -05:00 committed by GitHub
parent 09688bdc38
commit 7e0b8354a5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 24 additions and 8 deletions

3
.changelog/11940.txt Normal file
View File

@ -0,0 +1,3 @@
```release-note:bug
Mutate `NodeService` struct properly to avoid a data race.
```

View File

@ -1082,27 +1082,34 @@ func (l *State) updateSyncState() error {
continue 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 // 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 // copy so that we don't retain a pointer to any actual state
// store info for in-memory RPCs. // store info for in-memory RPCs.
if ls.Service.EnableTagOverride { if svc.EnableTagOverride {
ls.Service.Tags = make([]string, len(rs.Tags)) svc.Tags = make([]string, len(rs.Tags))
copy(ls.Service.Tags, rs.Tags) copy(svc.Tags, rs.Tags)
} }
// Merge any tagged addresses with the consul- prefix (set by the server) // Merge any tagged addresses with the consul- prefix (set by the server)
// back into the local state. // back into the local state.
if !reflect.DeepEqual(ls.Service.TaggedAddresses, rs.TaggedAddresses) { if !reflect.DeepEqual(svc.TaggedAddresses, rs.TaggedAddresses) {
if ls.Service.TaggedAddresses == nil { if svc.TaggedAddresses == nil {
ls.Service.TaggedAddresses = make(map[string]structs.ServiceAddress) svc.TaggedAddresses = make(map[string]structs.ServiceAddress)
} }
for k, v := range rs.TaggedAddresses { for k, v := range rs.TaggedAddresses {
if strings.HasPrefix(k, structs.MetaKeyReservedPrefix) { if strings.HasPrefix(k, structs.MetaKeyReservedPrefix) {
ls.Service.TaggedAddresses[k] = v svc.TaggedAddresses[k] = v
} }
} }
} }
ls.InSync = ls.Service.IsSame(rs) ls.InSync = svc.IsSame(rs)
// replace the service pointer to the new mutated struct
ls.Service = &svc
} }
// Check which checks need syncing // Check which checks need syncing

View File

@ -415,6 +415,12 @@ func TestAgentAntiEntropy_Services_ConnectProxy(t *testing.T) {
// All the services should match // All the services should match
for id, serv := range services.NodeServices.Services { for id, serv := range services.NodeServices.Services {
serv.CreateIndex, serv.ModifyIndex = 0, 0 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 { switch id {
case "mysql-proxy": case "mysql-proxy":
assert.Equal(srv1, serv) assert.Equal(srv1, serv)