diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index 7eead01c5..855eed077 100644 --- a/agent/agent_endpoint.go +++ b/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 } } diff --git a/agent/catalog_endpoint.go b/agent/catalog_endpoint.go index 2eae35a2a..ca781b36f 100644 --- a/agent/catalog_endpoint.go +++ b/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 { diff --git a/agent/structs/structs.go b/agent/structs/structs.go index b50f912f8..907a7a4ab 100644 --- a/agent/structs/structs.go +++ b/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 }