From 7e0b8354a570efe6020f0e3801abf6a27897bb5c Mon Sep 17 00:00:00 2001
From: Dhia Ayachi <dhia@hashicorp.com>
Date: Thu, 6 Jan 2022 14:33:06 -0500
Subject: [PATCH] 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.
---
 .changelog/11940.txt      |  3 +++
 agent/local/state.go      | 23 +++++++++++++++--------
 agent/local/state_test.go |  6 ++++++
 3 files changed, 24 insertions(+), 8 deletions(-)
 create mode 100644 .changelog/11940.txt

diff --git a/.changelog/11940.txt b/.changelog/11940.txt
new file mode 100644
index 000000000..22278079f
--- /dev/null
+++ b/.changelog/11940.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+ Mutate `NodeService` struct properly to avoid a data race.
+```
diff --git a/agent/local/state.go b/agent/local/state.go
index 03db0badb..d8ad529f7 100644
--- a/agent/local/state.go
+++ b/agent/local/state.go
@@ -1082,27 +1082,34 @@ 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 ls.Service.EnableTagOverride {
-			ls.Service.Tags = make([]string, len(rs.Tags))
-			copy(ls.Service.Tags, rs.Tags)
+		if svc.EnableTagOverride {
+			svc.Tags = make([]string, len(rs.Tags))
+			copy(svc.Tags, rs.Tags)
 		}
 
 		// Merge any tagged addresses with the consul- prefix (set by the server)
 		// back into the local state.
-		if !reflect.DeepEqual(ls.Service.TaggedAddresses, rs.TaggedAddresses) {
-			if ls.Service.TaggedAddresses == nil {
-				ls.Service.TaggedAddresses = make(map[string]structs.ServiceAddress)
+		if !reflect.DeepEqual(svc.TaggedAddresses, rs.TaggedAddresses) {
+			if svc.TaggedAddresses == nil {
+				svc.TaggedAddresses = make(map[string]structs.ServiceAddress)
 			}
 			for k, v := range rs.TaggedAddresses {
 				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
diff --git a/agent/local/state_test.go b/agent/local/state_test.go
index f2c51a281..679adba84 100644
--- a/agent/local/state_test.go
+++ b/agent/local/state_test.go
@@ -415,6 +415,12 @@ 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)