NET-7165 - fix address and target setting (#20403)

agent/discovery/query_fetcher_v1.go

@@ -127,6 +127,10 @@ func (f *V1DataFetcher) FetchNodes(ctx Context, req *QueryPayload) ([]*Result, e
 		Type:     ResultTypeNode,
 		Metadata: node.Meta,
 		Target:   node.Node,
+		Tenancy: ResultTenancy{
+			EnterpriseMeta: cfg.defaultEntMeta,
+			Datacenter:     cfg.datacenter,
+		},
 	})
 
 	return results, nil
@@ -358,19 +362,10 @@ func (f *V1DataFetcher) fetchServiceBasedOnTenancy(ctx Context, req *QueryPayloa
 	out.Nodes.Shuffle()
 	results := make([]*Result, 0, len(out.Nodes))
 	for _, node := range out.Nodes {
-		target := node.Service.Address
+		address, target, resultType := getAddressTargetAndResultType(node)
-		resultType := ResultTypeService
+
-		// TODO (v2-dns): IMPORTANT!!!!:  this needs to be revisited in how dns v1 utilizes
-		// the nodeaddress when the service address is an empty string.  Need to figure out
-		// if this can be removed and dns recursion and process can work with only the
-		// address set to the node.address and the target set to the service.address.
-		// We may have to look at modifying the discovery result if more metadata is needed to send along.
-		if target == "" {
-			target = node.Node.Node
-			resultType = ResultTypeNode
-		}
 		results = append(results, &Result{
-			Address:  node.Node.Address,
+			Address:  address,
 			Type:     resultType,
 			Target:   target,
 			Weight:   uint32(findWeight(node)),
@@ -386,6 +381,37 @@ func (f *V1DataFetcher) fetchServiceBasedOnTenancy(ctx Context, req *QueryPayloa
 	return results, nil
 }
 
+// getAddressTargetAndResultType returns the address, target and result type for a check service node.
+func getAddressTargetAndResultType(node structs.CheckServiceNode) (string, string, ResultType) {
+	// Set address and target
+	// if service address is present, set target and address based on service.
+	// otherwise get it from the node.
+	address := node.Service.Address
+	target := node.Service.Service
+	resultType := ResultTypeService
+
+	addressIP := net.ParseIP(address)
+	if addressIP == nil {
+		resultType = ResultTypeNode
+		if node.Service.Address != "" {
+			// cases where service address is foo or foo.node.consul
+			// For usage in DNS, these discovery results necessitate a CNAME record.
+			// These cases can be inferred from the discovery result when Type is Node and
+			// target is not an IP.
+			target = node.Service.Address
+		} else {
+			// cases where service address is empty and the service is bound to
+			// node with an address.  These do not require a CNAME record in.
+			// For usage in DNS, these discovery results do not require a CNAME record.
+			// These cases can be inferred from the discovery result when Type is Node and
+			// target is not an IP.
+			target = node.Node.Node
+		}
+		address = node.Node.Address
+	}
+	return address, target, resultType
+}
+
 // findWeight returns the weight of a service node.
 func findWeight(node structs.CheckServiceNode) int {
 	// By default, when only_passing is false, warning and passing nodes are returned

agent/discovery/query_fetcher_v1_test.go

@@ -21,7 +21,7 @@ import (
 	"github.com/hashicorp/consul/sdk/testutil"
 )
 
-// Test_FetchService tests the FetchService method in scenarios where the RPC
+// Test_FetchVirtualIP tests the FetchVirtualIP method in scenarios where the RPC
 // call succeeds and fails.
 func Test_FetchVirtualIP(t *testing.T) {
 	// set these to confirm that RPC call does not use them for this particular RPC
@@ -119,3 +119,193 @@ func Test_FetchVirtualIP(t *testing.T) {
 		})
 	}
 }
+
+// Test_FetchEndpoints tests the FetchEndpoints method in scenarios where the RPC
+// call succeeds and fails.
+func Test_FetchEndpoints(t *testing.T) {
+	// set these to confirm that RPC call does not use them for this particular RPC
+	rc := &config.RuntimeConfig{
+		DNSAllowStale:  true,
+		DNSMaxStale:    100,
+		DNSUseCache:    true,
+		DNSCacheMaxAge: 100,
+	}
+	tests := []struct {
+		name                   string
+		queryPayload           *QueryPayload
+		context                Context
+		rpcFuncForServiceNodes func(ctx context.Context, req structs.ServiceSpecificRequest) (structs.IndexedCheckServiceNodes, cache.ResultMeta, error)
+		expectedResults        []*Result
+		expectedErr            error
+	}{
+		{
+			name: "when service address is IPv4, result type is service, address is service address and target is service name",
+			queryPayload: &QueryPayload{
+				Name: "service-name",
+				Tenancy: QueryTenancy{
+					EnterpriseMeta: defaultEntMeta,
+				},
+			},
+			rpcFuncForServiceNodes: func(ctx context.Context, req structs.ServiceSpecificRequest) (structs.IndexedCheckServiceNodes, cache.ResultMeta, error) {
+				return structs.IndexedCheckServiceNodes{
+					Nodes: []structs.CheckServiceNode{
+						{
+							Node: &structs.Node{
+								Address: "node-address",
+								Node:    "node-name",
+							},
+							Service: &structs.NodeService{
+								Address: "127.0.0.1",
+								Service: "service-name",
+							},
+						},
+					},
+				}, cache.ResultMeta{}, nil
+			},
+			context: Context{
+				Token: "test-token",
+			},
+			expectedResults: []*Result{
+				{
+					Address: "127.0.0.1",
+					Target:  "service-name",
+					Type:    ResultTypeService,
+					Weight:  1,
+				},
+			},
+			expectedErr: nil,
+		},
+		{
+			name: "when service address is IPv6, result type is service, address is service address and target is service name",
+			queryPayload: &QueryPayload{
+				Name: "service-name",
+				Tenancy: QueryTenancy{
+					EnterpriseMeta: defaultEntMeta,
+				},
+			},
+			rpcFuncForServiceNodes: func(ctx context.Context, req structs.ServiceSpecificRequest) (structs.IndexedCheckServiceNodes, cache.ResultMeta, error) {
+				return structs.IndexedCheckServiceNodes{
+					Nodes: []structs.CheckServiceNode{
+						{
+							Node: &structs.Node{
+								Address: "node-address",
+								Node:    "node-name",
+							},
+							Service: &structs.NodeService{
+								Address: "2001:db8:1:2:cafe::1337",
+								Service: "service-name",
+							},
+						},
+					},
+				}, cache.ResultMeta{}, nil
+			},
+			context: Context{
+				Token: "test-token",
+			},
+			expectedResults: []*Result{
+				{
+					Address: "2001:db8:1:2:cafe::1337",
+					Target:  "service-name",
+					Type:    ResultTypeService,
+					Weight:  1,
+				},
+			},
+			expectedErr: nil,
+		},
+		{
+			name: "when service address is not IP but is not empty, result type is node, address is node address, and target is service address",
+			queryPayload: &QueryPayload{
+				Name: "service-name",
+				Tenancy: QueryTenancy{
+					EnterpriseMeta: defaultEntMeta,
+				},
+			},
+			rpcFuncForServiceNodes: func(ctx context.Context, req structs.ServiceSpecificRequest) (structs.IndexedCheckServiceNodes, cache.ResultMeta, error) {
+				return structs.IndexedCheckServiceNodes{
+					Nodes: []structs.CheckServiceNode{
+						{
+							Node: &structs.Node{
+								Address: "node-address",
+								Node:    "node-name",
+							},
+							Service: &structs.NodeService{
+								Address: "foo",
+								Service: "service-name",
+							},
+						},
+					},
+				}, cache.ResultMeta{}, nil
+			},
+			context: Context{
+				Token: "test-token",
+			},
+			expectedResults: []*Result{
+				{
+					Address: "node-address",
+					Target:  "foo",
+					Type:    ResultTypeNode,
+					Weight:  1,
+				},
+			},
+			expectedErr: nil,
+		},
+		{
+			name: "when service address is empty, result type is node, address is node address, and target is node name",
+			queryPayload: &QueryPayload{
+				Name: "service-name",
+				Tenancy: QueryTenancy{
+					EnterpriseMeta: defaultEntMeta,
+				},
+			},
+			rpcFuncForServiceNodes: func(ctx context.Context, req structs.ServiceSpecificRequest) (structs.IndexedCheckServiceNodes, cache.ResultMeta, error) {
+				return structs.IndexedCheckServiceNodes{
+					Nodes: []structs.CheckServiceNode{
+						{
+							Node: &structs.Node{
+								Address: "node-address",
+								Node:    "node-name",
+							},
+							Service: &structs.NodeService{
+								Address: "",
+								Service: "service-name",
+							},
+						},
+					},
+				}, cache.ResultMeta{}, nil
+			},
+			context: Context{
+				Token: "test-token",
+			},
+			expectedResults: []*Result{
+				{
+					Address: "node-address",
+					Target:  "node-name",
+					Type:    ResultTypeNode,
+					Weight:  1,
+				},
+			},
+			expectedErr: nil,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			logger := testutil.Logger(t)
+			mockRPC := cachetype.NewMockRPC(t)
+			// TODO (v2-dns): mock these properly
+			translateServicePortFunc := func(dc string, port int, taggedAddresses map[string]structs.ServiceAddress) int { return 0 }
+			rpcFuncForSamenessGroup := func(ctx context.Context, req *structs.ConfigEntryQuery) (structs.SamenessGroupConfigEntry, cache.ResultMeta, error) {
+				return structs.SamenessGroupConfigEntry{}, cache.ResultMeta{}, nil
+			}
+			getFromCacheFunc := func(ctx context.Context, t string, r cache.Request) (interface{}, cache.ResultMeta, error) {
+				return nil, cache.ResultMeta{}, nil
+			}
+
+			df := NewV1DataFetcher(rc, acl.DefaultEnterpriseMeta(), getFromCacheFunc, mockRPC.RPC, tc.rpcFuncForServiceNodes, rpcFuncForSamenessGroup, translateServicePortFunc, logger)
+
+			results, err := df.FetchEndpoints(tc.context, tc.queryPayload, LookupTypeService)
+			require.Equal(t, tc.expectedErr, err)
+			require.Equal(t, tc.expectedResults, results)
+		})
+	}
+}

agent/dns/dns_address.go

@@ -83,5 +83,5 @@ func (a *dnsAddress) IsInternalFQDNOrIP(domain string) bool {
 
 // IsExternalFQDN returns true if the address is a FQDN and is external to the domain.
 func (a *dnsAddress) IsExternalFQDN(domain string) bool {
-	return !a.IsIP() && a.IsFQDN() && !strings.HasSuffix(a.FQDN(), domain)
+	return !a.IsIP() && a.IsFQDN() && strings.Count(a.FQDN(), ".") > 1 && !strings.HasSuffix(a.FQDN(), domain)
 }

agent/dns/dns_address_test.go

@@ -95,6 +95,20 @@ func Test_dnsAddress(t *testing.T) {
 				isInternalFQDNOrIP: true,
 			},
 		},
+		{
+			name:  "server name",
+			input: "web.",
+			expectedResults: expectedResults{
+				isIp:               false,
+				stringResult:       "web.",
+				fqdn:               "web.",
+				isFQDN:             true,
+				isEmptyString:      false,
+				isExternalFQDN:     false,
+				isInternalFQDN:     false,
+				isInternalFQDNOrIP: false,
+			},
+		},
 		{
 			name:  "external FQDN without trailing period",
 			input: "web.service.vault",

agent/dns/query_locality.go

@@ -1,42 +0,0 @@
-// Copyright (c) HashiCorp, Inc.
-// SPDX-License-Identifier: BUSL-1.1
-
-package dns
-
-import "github.com/hashicorp/consul/acl"
-
-// queryLocality is the locality parsed from a DNS query.
-type queryLocality struct {
-	// datacenter is the datacenter parsed from a label that has an explicit datacenter part.
-	// Example query: <service>.virtual.<namespace>.ns.<partition>.ap.<datacenter>.dc.consul
-	datacenter string
-
-	// peer is the peer name parsed from a label that has explicit parts.
-	// Example query: <service>.virtual.<namespace>.ns.<peer>.peer.<partition>.ap.consul
-	peer string
-
-	// peerOrDatacenter is parsed from DNS queries where the datacenter and peer name are
-	// specified in the same query part.
-	// Example query: <service>.virtual.<peerOrDatacenter>.consul
-	//
-	// Note that this field should only be a "peer" for virtual queries, since virtual IPs should
-	// not be shared between datacenters. In all other cases, it should be considered a DC.
-	peerOrDatacenter string
-
-	acl.EnterpriseMeta
-}
-
-// EffectiveDatacenter returns the datacenter parsed from a query, or a default
-// value if none is specified.
-func (l queryLocality) EffectiveDatacenter(defaultDC string) string {
-	// Prefer the value parsed from a query with explicit parts: <namespace>.ns.<partition>.ap.<datacenter>.dc
-	if l.datacenter != "" {
-		return l.datacenter
-	}
-	// Fall back to the ambiguously parsed DC or Peer.
-	if l.peerOrDatacenter != "" {
-		return l.peerOrDatacenter
-	}
-	// If all are empty, use a default value.
-	return defaultDC
-}

agent/dns/query_locality_ce.go

@@ -1,57 +0,0 @@
-// Copyright (c) HashiCorp, Inc.
-// SPDX-License-Identifier: BUSL-1.1
-
-//go:build !consulent
-
-package dns
-
-import (
-	"github.com/hashicorp/consul/acl"
-	"github.com/hashicorp/consul/agent/config"
-)
-
-// ParseLocality can parse peer name or datacenter from a DNS query's labels.
-// Peer name is parsed from the same query part that datacenter is, so given this ambiguity
-// we parse a "peerOrDatacenter". The caller or RPC handler are responsible for disambiguating.
-func ParseLocality(labels []string, defaultEnterpriseMeta acl.EnterpriseMeta, _ enterpriseDNSConfig) (queryLocality, bool) {
-	locality := queryLocality{
-		EnterpriseMeta: defaultEnterpriseMeta,
-	}
-
-	switch len(labels) {
-	case 2, 4:
-		// Support the following formats:
-		// - [.<datacenter>.dc]
-		// - [.<peer>.peer]
-		for i := 0; i < len(labels); i += 2 {
-			switch labels[i+1] {
-			case "dc":
-				locality.datacenter = labels[i]
-			case "peer":
-				locality.peer = labels[i]
-			default:
-				return queryLocality{}, false
-			}
-		}
-		// Return error when both datacenter and peer are specified.
-		if locality.datacenter != "" && locality.peer != "" {
-			return queryLocality{}, false
-		}
-		return locality, true
-	case 1:
-		return queryLocality{peerOrDatacenter: labels[0]}, true
-
-	case 0:
-		return queryLocality{}, true
-	}
-
-	return queryLocality{}, false
-}
-
-// enterpriseDNSConfig is the configuration for enterprise DNS.
-type enterpriseDNSConfig struct{}
-
-// getEnterpriseDNSConfig returns the enterprise DNS configuration.
-func getEnterpriseDNSConfig(conf *config.RuntimeConfig) enterpriseDNSConfig {
-	return enterpriseDNSConfig{}
-}

agent/dns/query_locality_ce_test.go

@@ -1,60 +0,0 @@
-// Copyright (c) HashiCorp, Inc.
-// SPDX-License-Identifier: BUSL-1.1
-
-//go:build !consulent
-
-package dns
-
-import (
-	"github.com/hashicorp/consul/acl"
-)
-
-func getTestCases() []testCaseParseLocality {
-	testCases := []testCaseParseLocality{
-		{
-			name:                "test [.<datacenter>.dc]",
-			labels:              []string{"test-dc", "dc"},
-			enterpriseDNSConfig: enterpriseDNSConfig{},
-			expectedResult: queryLocality{
-				EnterpriseMeta: acl.EnterpriseMeta{},
-				datacenter:     "test-dc",
-			},
-			expectedOK: true,
-		},
-		{
-			name:                "test [.<peer>.peer]",
-			labels:              []string{"test-peer", "peer"},
-			enterpriseDNSConfig: enterpriseDNSConfig{},
-			expectedResult: queryLocality{
-				EnterpriseMeta: acl.EnterpriseMeta{},
-				peer:           "test-peer",
-			},
-			expectedOK: true,
-		},
-		{
-			name:                "test 1 label",
-			labels:              []string{"test-peer"},
-			enterpriseDNSConfig: enterpriseDNSConfig{},
-			expectedResult: queryLocality{
-				EnterpriseMeta:   acl.EnterpriseMeta{},
-				peerOrDatacenter: "test-peer",
-			},
-			expectedOK: true,
-		},
-		{
-			name:                "test 0 labels",
-			labels:              []string{},
-			enterpriseDNSConfig: enterpriseDNSConfig{},
-			expectedResult:      queryLocality{},
-			expectedOK:          true,
-		},
-		{
-			name:                "test 3 labels returns not found",
-			labels:              []string{"test-dc", "dc", "test-blah"},
-			enterpriseDNSConfig: enterpriseDNSConfig{},
-			expectedResult:      queryLocality{},
-			expectedOK:          false,
-		},
-	}
-	return testCases
-}

agent/dns/query_locality_test.go

@@ -1,74 +0,0 @@
-// Copyright (c) HashiCorp, Inc.
-// SPDX-License-Identifier: BUSL-1.1
-
-package dns
-
-import (
-	"testing"
-
-	"github.com/hashicorp/consul/acl"
-	"github.com/stretchr/testify/require"
-)
-
-type testCaseParseLocality struct {
-	name                string
-	labels              []string
-	defaultMeta         acl.EnterpriseMeta
-	enterpriseDNSConfig enterpriseDNSConfig
-	expectedResult      queryLocality
-	expectedOK          bool
-}
-
-func Test_ParseLocality(t *testing.T) {
-	testCases := getTestCases()
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			actualResult, actualOK := ParseLocality(tc.labels, tc.defaultMeta, tc.enterpriseDNSConfig)
-			require.Equal(t, tc.expectedOK, actualOK)
-			require.Equal(t, tc.expectedResult, actualResult)
-
-		})
-	}
-
-}
-
-func Test_EffectiveDatacenter(t *testing.T) {
-	type testCase struct {
-		name          string
-		queryLocality queryLocality
-		defaultDC     string
-		expected      string
-	}
-	testCases := []testCase{
-		{
-			name: "return datacenter first",
-			queryLocality: queryLocality{
-				datacenter:       "test-dc",
-				peerOrDatacenter: "test-peer",
-			},
-			defaultDC: "default-dc",
-			expected:  "test-dc",
-		},
-		{
-			name: "return peerOrDatacenter second",
-			queryLocality: queryLocality{
-				peerOrDatacenter: "test-peer",
-			},
-			defaultDC: "default-dc",
-			expected:  "test-peer",
-		},
-		{
-			name:          "return defaultDC as fallback",
-			queryLocality: queryLocality{},
-			defaultDC:     "default-dc",
-			expected:      "default-dc",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			got := tc.queryLocality.EffectiveDatacenter(tc.defaultDC)
-			require.Equal(t, tc.expected, got)
-		})
-	}
-}

agent/dns/router.go

@@ -195,17 +195,25 @@ func (r *Router) handleRequestRecursively(req *dns.Msg, reqCtx discovery.Context
 		return resp
 	}
 
+	// Need to pass the question name to properly support recursion and the
+	// trimming of the domain suffixes.
+	qName := dns.CanonicalName(req.Question[0].Name)
+	if maxRecursionLevel < maxRecursionLevelDefault {
+		// Get the QName without the domain suffix
+		qName = r.trimDomain(qName)
+	}
+
 	reqType := parseRequestType(req)
-	results, query, err := r.getQueryResults(req, reqCtx, reqType, configCtx)
+	results, query, err := r.getQueryResults(req, reqCtx, reqType, configCtx, qName)
 	switch {
 	case errors.Is(err, errNameNotFound):
-		r.logger.Error("name not found", "name", req.Question[0].Name)
+		r.logger.Error("name not found", "name", qName)
 
 		ecsGlobal := !errors.Is(err, discovery.ErrECSNotGlobal)
 		return createAuthoritativeResponse(req, configCtx, responseDomain, dns.RcodeNameError, ecsGlobal)
 	// TODO (v2-dns): there is another case here where the discovery service returns "name not found"
 	case errors.Is(err, discovery.ErrNoData):
-		r.logger.Debug("no data available", "name", req.Question[0].Name)
+		r.logger.Debug("no data available", "name", qName)
 
 		ecsGlobal := !errors.Is(err, discovery.ErrECSNotGlobal)
 		return createAuthoritativeResponse(req, configCtx, responseDomain, dns.RcodeSuccess, ecsGlobal)
@@ -224,6 +232,21 @@ func (r *Router) handleRequestRecursively(req *dns.Msg, reqCtx discovery.Context
 	return resp
 }
 
+// trimDomain trims the domain from the question name.
+func (r *Router) trimDomain(questionName string) string {
+	longer := r.domain
+	shorter := r.altDomain
+
+	if len(shorter) > len(longer) {
+		longer, shorter = shorter, longer
+	}
+
+	if strings.HasSuffix(questionName, "."+strings.TrimLeft(longer, ".")) {
+		return strings.TrimSuffix(questionName, longer)
+	}
+	return strings.TrimSuffix(questionName, shorter)
+}
+
 // getTTLForResult returns the TTL for a given result.
 func getTTLForResult(name string, query *discovery.Query, cfg *RouterDynamicConfig) uint32 {
 	switch {
@@ -243,7 +266,8 @@ func getTTLForResult(name string, query *discovery.Query, cfg *RouterDynamicConf
 }
 
 // getQueryResults returns a discovery.Result from a DNS message.
-func (r *Router) getQueryResults(req *dns.Msg, reqCtx discovery.Context, reqType requestType, cfg *RouterDynamicConfig) ([]*discovery.Result, *discovery.Query, error) {
+func (r *Router) getQueryResults(req *dns.Msg, reqCtx discovery.Context,
+	reqType requestType, cfg *RouterDynamicConfig, qName string) ([]*discovery.Result, *discovery.Query, error) {
 	var query *discovery.Query
 	switch reqType {
 	case requestTypeConsul:
@@ -272,9 +296,9 @@ func (r *Router) getQueryResults(req *dns.Msg, reqCtx discovery.Context, reqType
 		}
 		return results, query, nil
 	case requestTypeIP:
-		ip := dnsutil.IPFromARPA(req.Question[0].Name)
+		ip := dnsutil.IPFromARPA(qName)
 		if ip == nil {
-			r.logger.Error("error building IP from DNS request", "name", req.Question[0].Name)
+			r.logger.Error("error building IP from DNS request", "name", qName)
 			return nil, nil, errNameNotFound
 		}
 		results, err := r.processor.QueryByIP(ip, reqCtx)
@@ -742,8 +766,10 @@ func buildAddressResults(req *dns.Msg) ([]*discovery.Result, error) {
 
 // getAnswerAndExtra creates the dns answer and extra from discovery results.
 func (r *Router) getAnswerExtraAndNs(result *discovery.Result, req *dns.Msg, reqCtx discovery.Context,
-	query *discovery.Query, cfg *RouterDynamicConfig, domain string, remoteAddress net.Addr, maxRecursionLevel int) (answer []dns.RR, extra []dns.RR, ns []dns.RR) {
+	query *discovery.Query, cfg *RouterDynamicConfig, domain string, remoteAddress net.Addr,
-	address, target := getAddressAndTargetFromDiscoveryResult(result, r.domain)
+	maxRecursionLevel int) (answer []dns.RR, extra []dns.RR, ns []dns.RR) {
+	target := newDNSAddress(result.Target)
+	address := newDNSAddress(result.Address)
 	qName := req.Question[0].Name
 	ttlLookupName := qName
 	if query != nil {
@@ -781,17 +807,20 @@ func (r *Router) getAnswerExtraAndNs(result *discovery.Result, req *dns.Msg, req
 	case qType == dns.TypeSRV:
 		// We put A/AAAA/CNAME records in the additional section for SRV requests
 		a, e := r.getAnswerExtrasForAddressAndTarget(address, target, req, reqCtx,
-			result, ttl, remoteAddress, maxRecursionLevel)
+			result, ttl, remoteAddress, cfg, maxRecursionLevel)
 		answer = append(answer, a...)
 		extra = append(extra, e...)
 
-		cfg := r.dynamicConfig.Load().(*RouterDynamicConfig)
 		if cfg.NodeMetaTXT {
-			extra = append(extra, makeTXTRecord(target.FQDN(), result, ttl)...)
+			name := target.FQDN()
+			if !target.IsInternalFQDN(r.domain) && !target.IsExternalFQDN(r.domain) {
+				name = canonicalNameForResult(result, r.domain)
+			}
+			extra = append(extra, makeTXTRecord(name, result, ttl)...)
 		}
 	default:
 		a, e := r.getAnswerExtrasForAddressAndTarget(address, target, req, reqCtx,
-			result, ttl, remoteAddress, maxRecursionLevel)
+			result, ttl, remoteAddress, cfg, maxRecursionLevel)
 		answer = append(answer, a...)
 		extra = append(extra, e...)
 	}
@@ -801,65 +830,74 @@ func (r *Router) getAnswerExtraAndNs(result *discovery.Result, req *dns.Msg, req
 // getAnswerExtrasForAddressAndTarget creates the dns answer and extra from address and target dnsAddress pairs.
 func (r *Router) getAnswerExtrasForAddressAndTarget(address *dnsAddress, target *dnsAddress, req *dns.Msg,
 	reqCtx discovery.Context, result *discovery.Result, ttl uint32, remoteAddress net.Addr,
-	maxRecursionLevel int) (answer []dns.RR, extra []dns.RR) {
+	cfg *RouterDynamicConfig, maxRecursionLevel int) (answer []dns.RR, extra []dns.RR) {
 	qName := req.Question[0].Name
 	reqType := parseRequestType(req)
 
-	cfg := r.dynamicConfig.Load().(*RouterDynamicConfig)
 	switch {
+	// Virtual IPs and Address requests
+	// both return IPs with empty targets
+	case (reqType == requestTypeAddress || result.Type == discovery.ResultTypeVirtual) &&
+		target.IsEmptyString() && address.IsIP():
+		a, e := getAnswerExtrasForIP(qName, address, req.Question[0], reqType,
+			result, ttl)
+		answer = append(a, answer...)
+		extra = append(e, extra...)
 
-	// There is no target and the address is a FQDN (external service)
+	// Address is a FQDN and requires a CNAME lookup.
 	case address.IsFQDN():
 		a, e := r.makeRecordFromFQDN(address.FQDN(), result, req, reqCtx,
 			cfg, ttl, remoteAddress, maxRecursionLevel)
 		answer = append(a, answer...)
 		extra = append(e, extra...)
 
-	// The target is a FQDN (internal or external service name)
+	// Target is FQDN that point to IP
-	case result.Type != discovery.ResultTypeNode && target.IsFQDN():
+	case target.IsFQDN() && address.IsIP():
-		a, e := r.makeRecordFromFQDN(target.FQDN(), result, req, reqCtx,
+		var a, e []dns.RR
-			cfg, ttl, remoteAddress, maxRecursionLevel)
+		if result.Type == discovery.ResultTypeNode {
-		answer = append(answer, a...)
+			// if it is a node record it means the service address pointed to a node
-		extra = append(extra, e...)
+			// and the node address was used.  So we create an A record for the node address,
-
+			// as well as a CNAME for the service to node mapping.
-	// There is no target and the address is an IP
+			name := target.FQDN()
-	case address.IsIP():
+			if !target.IsInternalFQDN(r.domain) && !target.IsExternalFQDN(r.domain) {
-		// TODO (v2-dns): Do not CNAME node address in case of WAN address.
+				name = canonicalNameForResult(result, r.domain)
-		ipRecordName := target.FQDN()
+			} else if target.IsInternalFQDN(r.domain) {
-		if maxRecursionLevel < maxRecursionLevelDefault || ipRecordName == "" {
+				answer = append(answer, makeCNAMERecord(qName, canonicalNameForResult(result, r.domain), ttl))
-			ipRecordName = qName
+			}
+			a, e = getAnswerExtrasForIP(name, address, req.Question[0], reqType,
+				result, ttl)
+		} else {
+			// if it is a service record, it means that the service address had the IP directly
+			// and there was not a need for an intermediate CNAME.
+			a, e = getAnswerExtrasForIP(qName, address, req.Question[0], reqType,
+				result, ttl)
 		}
-		a, e := getAnswerExtrasForIP(ipRecordName, address, req.Question[0], reqType, result, ttl)
-		answer = append(answer, a...)
-		extra = append(extra, e...)
-
-	// The target is an IP
-	case target.IsIP():
-		a, e := getAnswerExtrasForIP(qName, target, req.Question[0], reqType, result, ttl)
-		answer = append(answer, a...)
-		extra = append(extra, e...)
-
-	// The target is a CNAME for the service we are looking
-	// for. So we use the address.
-	case target.FQDN() == req.Question[0].Name && address.IsIP():
-		a, e := getAnswerExtrasForIP(qName, address, req.Question[0], reqType, result, ttl)
 		answer = append(answer, a...)
 		extra = append(extra, e...)
 
 	// The target is a FQDN (internal or external service name)
 	default:
-		a, e := r.makeRecordFromFQDN(target.FQDN(), result, req, reqCtx, cfg, ttl, remoteAddress, maxRecursionLevel)
+		a, e := r.makeRecordFromFQDN(target.FQDN(), result, req, reqCtx, cfg,
+			ttl, remoteAddress, maxRecursionLevel)
 		answer = append(a, answer...)
 		extra = append(e, extra...)
 	}
 	return
 }
 
-// getAddressAndTargetFromDiscoveryResult returns the address and target from a discovery result.
+// getAnswerExtrasForIP creates the dns answer and extra from IP dnsAddress pairs.
-func getAnswerExtrasForIP(name string, addr *dnsAddress, question dns.Question, reqType requestType, result *discovery.Result, ttl uint32) (answer []dns.RR, extra []dns.RR) {
+func getAnswerExtrasForIP(name string, addr *dnsAddress, question dns.Question,
-	record := makeIPBasedRecord(name, addr, ttl)
+	reqType requestType, result *discovery.Result, ttl uint32) (answer []dns.RR, extra []dns.RR) {
 	qType := question.Qtype
 
+	// Have to pass original question name here even if the system has recursed
+	// and stripped off the domain suffix.
+	recHdrName := question.Name
+	if qType == dns.TypeSRV {
+		recHdrName = name
+	}
+	record := makeIPBasedRecord(recHdrName, addr, ttl)
+
 	isARecordWhenNotExplicitlyQueried := record.Header().Rrtype == dns.TypeA && qType != dns.TypeA && qType != dns.TypeANY
 	isAAAARecordWhenNotExplicitlyQueried := record.Header().Rrtype == dns.TypeAAAA && qType != dns.TypeAAAA && qType != dns.TypeANY
 
@@ -970,7 +1008,7 @@ MORE_REC:
 	}
 
 	answers := []dns.RR{
-		makeCNAMERecord(result, q.Name, ttl),
+		makeCNAMERecord(q.Name, result.Target, ttl),
 	}
 	answers = append(answers, additional...)
 
@@ -978,15 +1016,15 @@ MORE_REC:
 }
 
 // makeCNAMERecord returns a CNAME record for the given name and target.
-func makeCNAMERecord(result *discovery.Result, qName string, ttl uint32) *dns.CNAME {
+func makeCNAMERecord(name string, target string, ttl uint32) *dns.CNAME {
 	return &dns.CNAME{
 		Hdr: dns.RR_Header{
-			Name:   qName,
+			Name:   name,
 			Rrtype: dns.TypeCNAME,
 			Class:  dns.ClassINET,
 			Ttl:    ttl,
 		},
-		Target: dns.Fqdn(result.Target),
+		Target: dns.Fqdn(target),
 	}
 }
 
@@ -1047,13 +1085,3 @@ func makeTXTRecord(name string, result *discovery.Result, ttl uint32) []dns.RR {
 	}
 	return extra
 }
-
-// getAddressAndTargetFromCheckServiceNode returns the address and target for a given discovery.Result
-func getAddressAndTargetFromDiscoveryResult(result *discovery.Result, domain string) (*dnsAddress, *dnsAddress) {
-	target := newDNSAddress(result.Target)
-	if !target.IsEmptyString() && !target.IsInternalFQDNOrIP(domain) {
-		target.SetAddress(canonicalNameForResult(result, domain))
-	}
-	address := newDNSAddress(result.Address)
-	return address, target
-}