Makes the filled-in parts of ServiceNode more explicit.

consul/state/state_store.go

@@ -678,9 +678,11 @@ func (s *StateStore) ensureServiceTxn(tx *memdb.Txn, idx uint64, watches *DumbWa
 		return fmt.Errorf("failed service lookup: %s", err)
 	}
 
-	// Create the service node entry and populate the indexes. We leave the
-	// address blank and fill that in on the way out during queries.
-	entry := svc.ToServiceNode(node, "")
+	// Create the service node entry and populate the indexes. Note that
+	// conversion doesn't populate any of the node-specific information
+	// (Address and TaggedAddresses). That's always populated when we read
+	// from the state store.
+	entry := svc.ToServiceNode(node)
 	if existing != nil {
 		entry.CreateIndex = existing.(*structs.ServiceNode).CreateIndex
 		entry.ModifyIndex = idx
@@ -830,18 +832,23 @@ func (s *StateStore) parseServiceNodes(tx *memdb.Txn, services structs.ServiceNo
 	var results structs.ServiceNodes
 	for _, sn := range services {
 		// Note that we have to clone here because we don't want to
-		// modify the address field on the object in the database,
+		// modify the node-related fields on the object in the database,
 		// which is what we are referencing.
-		s := sn.Clone()
+		s := sn.PartialClone()
 
-		// Fill in the address of the node.
+		// Grab the corresponding node record.
 		n, err := tx.First("nodes", "id", sn.Node)
 		if err != nil {
 			return nil, fmt.Errorf("failed node lookup: %s", err)
 		}
+
+		// Populate the node-related fields. The tagged addresses may be
+		// used by agents to perform address translation if they are
+		// configured to do that.
 		node := n.(*structs.Node)
 		s.Address = node.Address
 		s.TaggedAddresses = node.TaggedAddresses
+
 		results = append(results, s)
 	}
 	return results, nil

consul/structs/structs.go

@@ -260,7 +260,11 @@ type Nodes []*Node
 // Maps service name to available tags
 type Services map[string][]string
 
-// ServiceNode represents a node that is part of a service
+// ServiceNode represents a node that is part of a service. Address and
+// TaggedAddresses are node-related fields that are always empty in the state
+// store and are filled in on the way out by parseServiceNodes(). This is also
+// why PartialClone() skips them, because we know they are blank already so it
+// would be a waste of time to copy them.
 type ServiceNode struct {
 	Node                     string
 	Address                  string
@@ -275,14 +279,16 @@ type ServiceNode struct {
 	RaftIndex
 }
 
-// Clone returns a clone of the given service node.
-func (s *ServiceNode) Clone() *ServiceNode {
+// PartialClone() returns a clone of the given service node, minus the node-
+// related fields that get filled in later, Address and TaggedAddresses.
+func (s *ServiceNode) PartialClone() *ServiceNode {
 	tags := make([]string, len(s.ServiceTags))
 	copy(tags, s.ServiceTags)
 
 	return &ServiceNode{
 		Node: s.Node,
-		Address:                  s.Address,
+		// Skip Address, see above.
+		// Skip TaggedAddresses, see above.
 		ServiceID:                s.ServiceID,
 		ServiceName:              s.ServiceName,
 		ServiceTags:              tags,
@@ -344,10 +350,11 @@ func (s *NodeService) IsSame(other *NodeService) bool {
 }
 
 // ToServiceNode converts the given node service to a service node.
-func (s *NodeService) ToServiceNode(node, address string) *ServiceNode {
+func (s *NodeService) ToServiceNode(node string) *ServiceNode {
 	return &ServiceNode{
 		Node: node,
-		Address:                  address,
+		// Skip Address, see ServiceNode definition.
+		// Skip TaggedAddresses, see ServiceNode definition.
 		ServiceID:                s.ID,
 		ServiceName:              s.Service,
 		ServiceTags:              s.Tags,

consul/structs/structs_test.go

@@ -110,6 +110,9 @@ func testServiceNode() *ServiceNode {
 	return &ServiceNode{
 		Node:    "node1",
 		Address: "127.0.0.1",
+		TaggedAddresses: map[string]string{
+			"hello": "world",
+		},
 		ServiceID:                "service1",
 		ServiceName:              "dogs",
 		ServiceTags:              []string{"prod", "v1"},
@@ -123,10 +126,20 @@ func testServiceNode() *ServiceNode {
 	}
 }
 
-func TestStructs_ServiceNode_Clone(t *testing.T) {
+func TestStructs_ServiceNode_PartialClone(t *testing.T) {
 	sn := testServiceNode()
 
-	clone := sn.Clone()
+	clone := sn.PartialClone()
+
+	// Make sure the parts that weren't supposed to be cloned didn't get
+	// copied over, then zero-value them out so we can do a DeepEqual() on
+	// the rest of the contents.
+	if clone.Address != "" || len(clone.TaggedAddresses) != 0 {
+		t.Fatalf("bad: %v", clone)
+	}
+
+	sn.Address = ""
+	sn.TaggedAddresses = nil
 	if !reflect.DeepEqual(sn, clone) {
 		t.Fatalf("bad: %v", clone)
 	}
@@ -140,7 +153,12 @@ func TestStructs_ServiceNode_Clone(t *testing.T) {
 func TestStructs_ServiceNode_Conversions(t *testing.T) {
 	sn := testServiceNode()
 
-	sn2 := sn.ToNodeService().ToServiceNode("node1", "127.0.0.1")
+	sn2 := sn.ToNodeService().ToServiceNode("node1")
+
+	// These two fields get lost in the conversion, so we have to zero-value
+	// them out before we do the compare.
+	sn.Address = ""
+	sn.TaggedAddresses = nil
 	if !reflect.DeepEqual(sn, sn2) {
 		t.Fatalf("bad: %v", sn2)
 	}