Addresses additional state mutations.

Did a sweep of 84d6ac2d51
and checked them all.

agent/agent_endpoint.go

@@ -146,9 +146,11 @@ func (s *HTTPServer) AgentServices(resp http.ResponseWriter, req *http.Request)
 	}
 
 	// Use empty list instead of nil
-	for _, s := range services {
+	for id, s := range services {
 		if s.Tags == nil {
-			s.Tags = make([]string, 0)
+			clone := *s
+			clone.Tags = make([]string, 0)
+			services[id] = &clone
 		}
 	}
 

agent/catalog_endpoint.go

@@ -246,6 +246,15 @@ func (s *HTTPServer) CatalogNodeServices(resp http.ResponseWriter, req *http.Req
 		s.agent.TranslateAddresses(args.Datacenter, out.NodeServices.Node)
 	}
 
+	// TODO: The NodeServices object in IndexedNodeServices is a pointer to
+	// something that's created for each request by the state store way down
+	// in https://github.com/hashicorp/consul/blob/v1.0.4/agent/consul/state/catalog.go#L953-L963.
+	// Since this isn't a pointer to a real state store object, it's safe to
+	// modify out.NodeServices.Services in the loop below without making a
+	// copy here. Same for the Tags in each service entry, since that was
+	// created by .ToNodeService() which made a copy. This is safe as-is but
+	// this whole business is tricky and subtle. See #3867 for more context.
+
 	// Use empty list instead of nil
 	if out.NodeServices != nil {
 		for _, s := range out.NodeServices.Services {

agent/structs/structs.go

@@ -600,6 +600,8 @@ type IndexedServiceNodes struct {
 }
 
 type IndexedNodeServices struct {
+	// TODO: This should not be a pointer, see comments in
+	// agent/catalog_endpoint.go.
 	NodeServices *NodeServices
 	QueryMeta
 }