From 137c9c09735526fae4d166f7fdbc8bfadc91a2ed Mon Sep 17 00:00:00 2001 From: Dan Stough Date: Wed, 14 Feb 2024 12:40:38 -0500 Subject: [PATCH] [CE] Misc cleanup for V2 DNS (#20640) * chore: gitignore zed editor * chore(v2dns): remove ent/ce split from router * fix(v2dns): v2 workloads now have tenancy in output * feat(v2dns): support 'cluster' label * chore(v2dns): less chatty debug logs --- .gitignore | 1 + agent/discovery/query_fetcher_v1.go | 6 +- agent/discovery/query_fetcher_v1_ce.go | 2 +- agent/discovery/query_fetcher_v2.go | 2 +- agent/dns/parser.go | 2 +- agent/dns/recursor.go | 6 +- agent/dns/router.go | 51 ++- agent/dns/router_ce.go | 38 -- agent/dns/router_ce_test.go | 149 ------- ...uestion_test.go => router_service_test.go} | 5 +- agent/dns/router_test.go | 407 ++++++++++++++++-- agent/dns_catalogv2_test.go | 18 +- 12 files changed, 443 insertions(+), 244 deletions(-) delete mode 100644 agent/dns/router_ce.go delete mode 100644 agent/dns/router_ce_test.go rename agent/dns/{router_service_question_test.go => router_service_test.go} (98%) diff --git a/.gitignore b/.gitignore index 793354db02..9649c4a8cb 100644 --- a/.gitignore +++ b/.gitignore @@ -19,6 +19,7 @@ Thumbs.db __debug_bin coverage.out *.tmp +.zed # MacOS .DS_Store diff --git a/agent/discovery/query_fetcher_v1.go b/agent/discovery/query_fetcher_v1.go index 2b80bc9152..144c88a281 100644 --- a/agent/discovery/query_fetcher_v1.go +++ b/agent/discovery/query_fetcher_v1.go @@ -160,7 +160,7 @@ func (f *V1DataFetcher) FetchNodes(ctx Context, req *QueryPayload) ([]*Result, e // FetchEndpoints fetches records for A/AAAA/CNAME or SRV requests for services func (f *V1DataFetcher) FetchEndpoints(ctx Context, req *QueryPayload, lookupType LookupType) ([]*Result, error) { - f.logger.Debug(fmt.Sprintf("FetchEndpoints - req: %+v / lookupType: %+v", req, lookupType)) + f.logger.Trace(fmt.Sprintf("FetchEndpoints - req: %+v / lookupType: %+v", req, lookupType)) cfg := f.dynamicConfig.Load().(*v1DataFetcherDynamicConfig) return f.fetchService(ctx, req, cfg, lookupType) } @@ -525,7 +525,7 @@ RPC: func (f *V1DataFetcher) fetchService(ctx Context, req *QueryPayload, cfg *v1DataFetcherDynamicConfig, lookupType LookupType) ([]*Result, error) { - f.logger.Debug("fetchService", "req", req) + f.logger.Trace("fetchService", "req", req) if req.Tenancy.SamenessGroup == "" { return f.fetchServiceBasedOnTenancy(ctx, req, cfg, lookupType) } @@ -536,7 +536,7 @@ func (f *V1DataFetcher) fetchService(ctx Context, req *QueryPayload, // fetchServiceBasedOnTenancy is used to look up a service in the Consul catalog based on its tenancy or default tenancy. func (f *V1DataFetcher) fetchServiceBasedOnTenancy(ctx Context, req *QueryPayload, cfg *v1DataFetcherDynamicConfig, lookupType LookupType) ([]*Result, error) { - f.logger.Debug(fmt.Sprintf("fetchServiceBasedOnTenancy - req: %+v", req)) + f.logger.Trace(fmt.Sprintf("fetchServiceBasedOnTenancy - req: %+v", req)) if req.Tenancy.SamenessGroup != "" { return nil, errors.New("sameness groups are not allowed for service lookups based on tenancy") } diff --git a/agent/discovery/query_fetcher_v1_ce.go b/agent/discovery/query_fetcher_v1_ce.go index 101b57c225..0260b7a24a 100644 --- a/agent/discovery/query_fetcher_v1_ce.go +++ b/agent/discovery/query_fetcher_v1_ce.go @@ -30,7 +30,7 @@ func queryTenancyToEntMeta(_ QueryTenancy) acl.EnterpriseMeta { // fetchServiceFromSamenessGroup fetches a service from a sameness group. func (f *V1DataFetcher) fetchServiceFromSamenessGroup(ctx Context, req *QueryPayload, cfg *v1DataFetcherDynamicConfig, lookupType LookupType) ([]*Result, error) { - f.logger.Debug(fmt.Sprintf("fetchServiceFromSamenessGroup - req: %+v", req)) + f.logger.Trace(fmt.Sprintf("fetchServiceFromSamenessGroup - req: %+v", req)) if req.Tenancy.SamenessGroup == "" { return nil, errors.New("sameness groups must be provided for service lookups") } diff --git a/agent/discovery/query_fetcher_v2.go b/agent/discovery/query_fetcher_v2.go index 80dd211792..02e8fcaccc 100644 --- a/agent/discovery/query_fetcher_v2.go +++ b/agent/discovery/query_fetcher_v2.go @@ -224,7 +224,7 @@ func (f *V2DataFetcher) fetchResource(reqContext Context, req QueryPayload, kind }, } - f.logger.Debug("fetching "+kind.String(), "name", req.Name) + f.logger.Trace("fetching "+kind.String(), "name", req.Name) resourceCtx := metadata.AppendToOutgoingContext(context.Background(), "x-consul-token", reqContext.Token) // If the service is not found, return nil and an error equivalent to NXDOMAIN diff --git a/agent/dns/parser.go b/agent/dns/parser.go index dd23e91591..1a0f0a601d 100644 --- a/agent/dns/parser.go +++ b/agent/dns/parser.go @@ -33,7 +33,7 @@ func parseLabels(labels []string) (*parsedLabels, bool) { result.Namespace = labels[i] case "ap": result.Partition = labels[i] - case "dc": // TODO (v2-dns): This should also include "cluster" for the new notation. + case "dc", "cluster": result.Datacenter = labels[i] case "sg": result.SamenessGroup = labels[i] diff --git a/agent/dns/recursor.go b/agent/dns/recursor.go index 55b922f710..21ea94a6c8 100644 --- a/agent/dns/recursor.go +++ b/agent/dns/recursor.go @@ -31,7 +31,7 @@ func (r *recursor) handle(req *dns.Msg, cfgCtx *RouterDynamicConfig, remoteAddr network := "udp" defer func(s time.Time) { - r.logger.Debug("request served from client", + r.logger.Trace("request served from client", "question", q, "network", network, "latency", time.Since(s).String(), @@ -55,7 +55,7 @@ func (r *recursor) handle(req *dns.Msg, cfgCtx *RouterDynamicConfig, remoteAddr resp, rtt, err = c.Exchange(req, recurseAddr) // Check if the response is valid and has the desired Response code if resp != nil && (resp.Rcode != dns.RcodeSuccess && resp.Rcode != dns.RcodeNameError) { - r.logger.Debug("recurse failed for question", + r.logger.Trace("recurse failed for question", "question", q, "rtt", rtt, "recursor", recurseAddr, @@ -71,7 +71,7 @@ func (r *recursor) handle(req *dns.Msg, cfgCtx *RouterDynamicConfig, remoteAddr resp.Compress = !cfgCtx.DisableCompression // Forward the response - r.logger.Debug("recurse succeeded for question", + r.logger.Trace("recurse succeeded for question", "question", q, "rtt", rtt, "recursor", recurseAddr, diff --git a/agent/dns/router.go b/agent/dns/router.go index cca002793a..37e8743720 100644 --- a/agent/dns/router.go +++ b/agent/dns/router.go @@ -167,7 +167,7 @@ func (r *Router) HandleRequest(req *dns.Msg, reqCtx Context, remoteAddress net.A {Name: "type", Value: dns.Type(q.Qtype).String()}, }) - r.logger.Debug("request served from client", + r.logger.Trace("request served from client", "name", q.Name, "type", dns.Type(q.Qtype).String(), "class", dns.Class(q.Qclass).String(), @@ -681,18 +681,18 @@ func (r *Router) defaultAgentDNSRequestContext() Context { func (r *Router) resolveCNAME(cfgContext *RouterDynamicConfig, name string, reqCtx Context, remoteAddress net.Addr, maxRecursionLevel int) []dns.RR { // If the CNAME record points to a Consul address, resolve it internally - // Convert query to lowercase because DNS is case-insensitive; d.domain and - // d.altDomain are already converted + // Convert query to lowercase because DNS is case-insensitive; r.domain and + // r.altDomain are already converted if ln := strings.ToLower(name); strings.HasSuffix(ln, "."+r.domain) || strings.HasSuffix(ln, "."+r.altDomain) { if maxRecursionLevel < 1 { - //d.logger.Error("Infinite recursion detected for name, won't perform any CNAME resolution.", "name", name) + r.logger.Error("Infinite recursion detected for name, won't perform any CNAME resolution.", "name", name) return nil } req := &dns.Msg{} req.SetQuestion(name, dns.TypeANY) - // TODO: handle error response + // TODO: handle error response (this is a comment from the V1 DNS Server) resp := r.handleRequestRecursively(req, reqCtx, cfgContext, nil, maxRecursionLevel-1) return resp.Answer @@ -1426,3 +1426,44 @@ func makeTXTRecord(name string, result *discovery.Result, ttl uint32) []dns.RR { } return extra } + +// canonicalNameForResult returns the canonical name for a discovery result. +func canonicalNameForResult(resultType discovery.ResultType, target, domain string, + tenancy discovery.ResultTenancy, portName string) string { + switch resultType { + case discovery.ResultTypeService: + if tenancy.Namespace != "" { + return fmt.Sprintf("%s.%s.%s.%s.%s", target, "service", tenancy.Namespace, tenancy.Datacenter, domain) + } + return fmt.Sprintf("%s.%s.%s.%s", target, "service", tenancy.Datacenter, domain) + case discovery.ResultTypeNode: + if tenancy.PeerName != "" && tenancy.Partition != "" { + // We must return a more-specific DNS name for peering so + // that there is no ambiguity with lookups. + // Nodes are always registered in the default namespace, so + // the `.ns` qualifier is not required. + return fmt.Sprintf("%s.node.%s.peer.%s.ap.%s", + target, + tenancy.PeerName, + tenancy.Partition, + domain) + } + if tenancy.PeerName != "" { + // We must return a more-specific DNS name for peering so + // that there is no ambiguity with lookups. + return fmt.Sprintf("%s.node.%s.peer.%s", + target, + tenancy.PeerName, + domain) + } + // Return a simpler format for non-peering nodes. + return fmt.Sprintf("%s.node.%s.%s", target, tenancy.Datacenter, domain) + case discovery.ResultTypeWorkload: + // TODO (v2-dns): it doesn't appear this is being used to return a result. Need to investigate and refactor + if portName != "" { + return fmt.Sprintf("%s.port.%s.workload.%s.ns.%s.ap.%s", portName, target, tenancy.Namespace, tenancy.Partition, domain) + } + return fmt.Sprintf("%s.workload.%s.ns.%s.ap.%s", target, tenancy.Namespace, tenancy.Partition, domain) + } + return "" +} diff --git a/agent/dns/router_ce.go b/agent/dns/router_ce.go deleted file mode 100644 index 67cab00490..0000000000 --- a/agent/dns/router_ce.go +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -//go:build !consulent - -package dns - -import ( - "fmt" - - "github.com/hashicorp/consul/agent/discovery" -) - -// canonicalNameForResult returns the canonical name for a discovery result. -func canonicalNameForResult(resultType discovery.ResultType, target, domain string, - tenancy discovery.ResultTenancy, portName string) string { - switch resultType { - case discovery.ResultTypeService: - return fmt.Sprintf("%s.%s.%s.%s", target, "service", tenancy.Datacenter, domain) - case discovery.ResultTypeNode: - if tenancy.PeerName != "" { - // We must return a more-specific DNS name for peering so - // that there is no ambiguity with lookups. - return fmt.Sprintf("%s.node.%s.peer.%s", - target, - tenancy.PeerName, - domain) - } - // Return a simpler format for non-peering nodes. - return fmt.Sprintf("%s.node.%s.%s", target, tenancy.Datacenter, domain) - case discovery.ResultTypeWorkload: - if portName != "" { - return fmt.Sprintf("%s.port.%s.workload.%s", portName, target, domain) - } - return fmt.Sprintf("%s.workload.%s", target, domain) - } - return "" -} diff --git a/agent/dns/router_ce_test.go b/agent/dns/router_ce_test.go deleted file mode 100644 index 3dd63eeee6..0000000000 --- a/agent/dns/router_ce_test.go +++ /dev/null @@ -1,149 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -//go:build !consulent - -package dns - -import ( - "net" - "testing" - - "github.com/miekg/dns" - "github.com/stretchr/testify/mock" - "github.com/stretchr/testify/require" - - "github.com/hashicorp/consul/agent/discovery" -) - -func getAdditionalTestCases(t *testing.T) []HandleTestCase { - // PTR Lookups - return []HandleTestCase{ - // PTR Lookups - { - name: "PTR Lookup for node w/ peer name, query type is ANY", - request: &dns.Msg{ - MsgHdr: dns.MsgHdr{ - Opcode: dns.OpcodeQuery, - }, - Question: []dns.Question{ - { - Name: "4.3.2.1.in-addr.arpa", - Qtype: dns.TypeANY, - Qclass: dns.ClassINET, - }, - }, - }, - configureDataFetcher: func(fetcher discovery.CatalogDataFetcher) { - results := []*discovery.Result{ - { - Node: &discovery.Location{Name: "foo", Address: "1.2.3.4"}, - Type: discovery.ResultTypeNode, - Service: &discovery.Location{Name: "foo", Address: "foo"}, - Tenancy: discovery.ResultTenancy{ - Datacenter: "dc2", - PeerName: "peer1", - }, - }, - } - - fetcher.(*discovery.MockCatalogDataFetcher). - On("FetchRecordsByIp", mock.Anything, mock.Anything). - Return(results, nil). - Run(func(args mock.Arguments) { - req := args.Get(1).(net.IP) - - require.NotNil(t, req) - require.Equal(t, "1.2.3.4", req.String()) - }) - }, - response: &dns.Msg{ - MsgHdr: dns.MsgHdr{ - Opcode: dns.OpcodeQuery, - Response: true, - Authoritative: true, - }, - Compress: true, - Question: []dns.Question{ - { - Name: "4.3.2.1.in-addr.arpa.", - Qtype: dns.TypeANY, - Qclass: dns.ClassINET, - }, - }, - Answer: []dns.RR{ - &dns.PTR{ - Hdr: dns.RR_Header{ - Name: "4.3.2.1.in-addr.arpa.", - Rrtype: dns.TypePTR, - Class: dns.ClassINET, - }, - Ptr: "foo.node.peer1.peer.consul.", - }, - }, - }, - }, - { - name: "PTR Lookup for service, query type is PTR", - request: &dns.Msg{ - MsgHdr: dns.MsgHdr{ - Opcode: dns.OpcodeQuery, - }, - Question: []dns.Question{ - { - Name: "4.3.2.1.in-addr.arpa", - Qtype: dns.TypePTR, - Qclass: dns.ClassINET, - }, - }, - }, - configureDataFetcher: func(fetcher discovery.CatalogDataFetcher) { - results := []*discovery.Result{ - { - Node: &discovery.Location{Name: "foo", Address: "1.2.3.4"}, - Service: &discovery.Location{Name: "foo", Address: "foo"}, - Type: discovery.ResultTypeService, - Tenancy: discovery.ResultTenancy{ - Datacenter: "dc2", - }, - }, - } - - fetcher.(*discovery.MockCatalogDataFetcher). - On("FetchRecordsByIp", mock.Anything, mock.Anything). - Return(results, nil). - Run(func(args mock.Arguments) { - req := args.Get(1).(net.IP) - - require.NotNil(t, req) - require.Equal(t, "1.2.3.4", req.String()) - }) - }, - response: &dns.Msg{ - MsgHdr: dns.MsgHdr{ - Opcode: dns.OpcodeQuery, - Response: true, - Authoritative: true, - }, - Compress: true, - Question: []dns.Question{ - { - Name: "4.3.2.1.in-addr.arpa.", - Qtype: dns.TypePTR, - Qclass: dns.ClassINET, - }, - }, - Answer: []dns.RR{ - &dns.PTR{ - Hdr: dns.RR_Header{ - Name: "4.3.2.1.in-addr.arpa.", - Rrtype: dns.TypePTR, - Class: dns.ClassINET, - }, - Ptr: "foo.service.dc2.consul.", - }, - }, - }, - }, - } -} diff --git a/agent/dns/router_service_question_test.go b/agent/dns/router_service_test.go similarity index 98% rename from agent/dns/router_service_question_test.go rename to agent/dns/router_service_test.go index 76fce89107..cf78f8687a 100644 --- a/agent/dns/router_service_question_test.go +++ b/agent/dns/router_service_test.go @@ -8,10 +8,11 @@ import ( "testing" "time" - "github.com/hashicorp/consul/agent/discovery" "github.com/miekg/dns" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + + "github.com/hashicorp/consul/agent/discovery" ) func Test_HandleRequest_ServiceQuestions(t *testing.T) { @@ -159,8 +160,6 @@ func Test_HandleRequest_ServiceQuestions(t *testing.T) { }, } - testCases = append(testCases, getAdditionalTestCases(t)...) - for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { runHandleTestCases(t, tc) diff --git a/agent/dns/router_test.go b/agent/dns/router_test.go index ea19db9e4b..94ea7893dd 100644 --- a/agent/dns/router_test.go +++ b/agent/dns/router_test.go @@ -837,10 +837,18 @@ func Test_HandleRequest(t *testing.T) { { Node: &discovery.Location{Name: "server-one", Address: "1.2.3.4"}, Type: discovery.ResultTypeWorkload, + Tenancy: discovery.ResultTenancy{ + Namespace: resource.DefaultNamespaceName, + Partition: resource.DefaultPartitionName, + }, }, { Node: &discovery.Location{Name: "server-two", Address: "4.5.6.7"}, Type: discovery.ResultTypeWorkload, + Tenancy: discovery.ResultTenancy{ + Namespace: resource.DefaultNamespaceName, + Partition: resource.DefaultPartitionName, + }, }, }, nil). Run(func(args mock.Arguments) { @@ -892,7 +900,7 @@ func Test_HandleRequest(t *testing.T) { Class: dns.ClassINET, Ttl: 123, }, - Ns: "server-one.workload.consul.", + Ns: "server-one.workload.default.ns.default.ap.consul.", }, &dns.NS{ Hdr: dns.RR_Header{ @@ -901,13 +909,13 @@ func Test_HandleRequest(t *testing.T) { Class: dns.ClassINET, Ttl: 123, }, - Ns: "server-two.workload.consul.", + Ns: "server-two.workload.default.ns.default.ap.consul.", }, }, Extra: []dns.RR{ &dns.A{ Hdr: dns.RR_Header{ - Name: "server-one.workload.consul.", + Name: "server-one.workload.default.ns.default.ap.consul.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 123, @@ -916,7 +924,7 @@ func Test_HandleRequest(t *testing.T) { }, &dns.A{ Hdr: dns.RR_Header{ - Name: "server-two.workload.consul.", + Name: "server-two.workload.default.ns.default.ap.consul.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 123, @@ -959,11 +967,18 @@ func Test_HandleRequest(t *testing.T) { { Node: &discovery.Location{Name: "server-one", Address: "1.2.3.4"}, Type: discovery.ResultTypeWorkload, + Tenancy: discovery.ResultTenancy{ + Namespace: resource.DefaultNamespaceName, + Partition: resource.DefaultPartitionName, + }, }, { Node: &discovery.Location{Name: "server-two", Address: "4.5.6.7"}, Type: discovery.ResultTypeWorkload, - }, + Tenancy: discovery.ResultTenancy{ + Namespace: resource.DefaultNamespaceName, + Partition: resource.DefaultPartitionName, + }}, }, nil). Run(func(args mock.Arguments) { req := args.Get(1).(*discovery.QueryPayload) @@ -1014,7 +1029,7 @@ func Test_HandleRequest(t *testing.T) { Class: dns.ClassINET, Ttl: 123, }, - Ns: "server-one.workload.testdomain.", + Ns: "server-one.workload.default.ns.default.ap.testdomain.", }, &dns.NS{ Hdr: dns.RR_Header{ @@ -1023,13 +1038,13 @@ func Test_HandleRequest(t *testing.T) { Class: dns.ClassINET, Ttl: 123, }, - Ns: "server-two.workload.testdomain.", + Ns: "server-two.workload.default.ns.default.ap.testdomain.", }, }, Extra: []dns.RR{ &dns.A{ Hdr: dns.RR_Header{ - Name: "server-one.workload.testdomain.", + Name: "server-one.workload.default.ns.default.ap.testdomain.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 123, @@ -1038,7 +1053,7 @@ func Test_HandleRequest(t *testing.T) { }, &dns.A{ Hdr: dns.RR_Header{ - Name: "server-two.workload.testdomain.", + Name: "server-two.workload.default.ns.default.ap.testdomain.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 123, @@ -1070,10 +1085,18 @@ func Test_HandleRequest(t *testing.T) { { Node: &discovery.Location{Name: "server-one", Address: "1.2.3.4"}, Type: discovery.ResultTypeWorkload, + Tenancy: discovery.ResultTenancy{ + Namespace: resource.DefaultNamespaceName, + Partition: resource.DefaultPartitionName, + }, }, { Node: &discovery.Location{Name: "server-two", Address: "4.5.6.7"}, Type: discovery.ResultTypeWorkload, + Tenancy: discovery.ResultTenancy{ + Namespace: resource.DefaultNamespaceName, + Partition: resource.DefaultPartitionName, + }, }, }, nil). Run(func(args mock.Arguments) { @@ -1108,7 +1131,7 @@ func Test_HandleRequest(t *testing.T) { Class: dns.ClassINET, Ttl: 123, }, - Ns: "server-one.workload.consul.", // TODO (v2-dns): this format needs to be consistent with other workloads + Ns: "server-one.workload.default.ns.default.ap.consul.", }, &dns.NS{ Hdr: dns.RR_Header{ @@ -1117,13 +1140,13 @@ func Test_HandleRequest(t *testing.T) { Class: dns.ClassINET, Ttl: 123, }, - Ns: "server-two.workload.consul.", + Ns: "server-two.workload.default.ns.default.ap.consul.", }, }, Extra: []dns.RR{ &dns.A{ Hdr: dns.RR_Header{ - Name: "server-one.workload.consul.", + Name: "server-one.workload.default.ns.default.ap.consul.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 123, @@ -1132,7 +1155,7 @@ func Test_HandleRequest(t *testing.T) { }, &dns.A{ Hdr: dns.RR_Header{ - Name: "server-two.workload.consul.", + Name: "server-two.workload.default.ns.default.ap.consul.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 123, @@ -1175,10 +1198,18 @@ func Test_HandleRequest(t *testing.T) { { Node: &discovery.Location{Name: "server-one", Address: "1.2.3.4"}, Type: discovery.ResultTypeWorkload, + Tenancy: discovery.ResultTenancy{ + Namespace: resource.DefaultNamespaceName, + Partition: resource.DefaultPartitionName, + }, }, { Node: &discovery.Location{Name: "server-two", Address: "4.5.6.7"}, Type: discovery.ResultTypeWorkload, + Tenancy: discovery.ResultTenancy{ + Namespace: resource.DefaultNamespaceName, + Partition: resource.DefaultPartitionName, + }, }, }, nil). Run(func(args mock.Arguments) { @@ -1213,7 +1244,7 @@ func Test_HandleRequest(t *testing.T) { Class: dns.ClassINET, Ttl: 123, }, - Ns: "server-one.workload.testdomain.", + Ns: "server-one.workload.default.ns.default.ap.testdomain.", }, &dns.NS{ Hdr: dns.RR_Header{ @@ -1222,13 +1253,13 @@ func Test_HandleRequest(t *testing.T) { Class: dns.ClassINET, Ttl: 123, }, - Ns: "server-two.workload.testdomain.", + Ns: "server-two.workload.default.ns.default.ap.testdomain.", }, }, Extra: []dns.RR{ &dns.A{ Hdr: dns.RR_Header{ - Name: "server-one.workload.testdomain.", + Name: "server-one.workload.default.ns.default.ap.testdomain.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 123, @@ -1237,7 +1268,7 @@ func Test_HandleRequest(t *testing.T) { }, &dns.A{ Hdr: dns.RR_Header{ - Name: "server-two.workload.testdomain.", + Name: "server-two.workload.default.ns.default.ap.testdomain.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 123, @@ -1468,7 +1499,322 @@ func Test_HandleRequest(t *testing.T) { }, }, }, - // TODO (v2-dns): add a test to make sure only 3 records are returned + { + name: "[ENT] PTR Lookup for node w/ peer name in default partition, query type is ANY", + request: &dns.Msg{ + MsgHdr: dns.MsgHdr{ + Opcode: dns.OpcodeQuery, + }, + Question: []dns.Question{ + { + Name: "4.3.2.1.in-addr.arpa", + Qtype: dns.TypeANY, + Qclass: dns.ClassINET, + }, + }, + }, + configureDataFetcher: func(fetcher discovery.CatalogDataFetcher) { + results := []*discovery.Result{ + { + Node: &discovery.Location{Name: "foo", Address: "1.2.3.4"}, + Type: discovery.ResultTypeNode, + Service: &discovery.Location{Name: "foo-web", Address: "foo"}, + Tenancy: discovery.ResultTenancy{ + Datacenter: "dc2", + PeerName: "peer1", + Partition: "default", + }, + }, + } + + fetcher.(*discovery.MockCatalogDataFetcher). + On("FetchRecordsByIp", mock.Anything, mock.Anything). + Return(results, nil). + Run(func(args mock.Arguments) { + req := args.Get(1).(net.IP) + + require.NotNil(t, req) + require.Equal(t, "1.2.3.4", req.String()) + }) + }, + response: &dns.Msg{ + MsgHdr: dns.MsgHdr{ + Opcode: dns.OpcodeQuery, + Response: true, + Authoritative: true, + }, + Compress: true, + Question: []dns.Question{ + { + Name: "4.3.2.1.in-addr.arpa.", + Qtype: dns.TypeANY, + Qclass: dns.ClassINET, + }, + }, + Answer: []dns.RR{ + &dns.PTR{ + Hdr: dns.RR_Header{ + Name: "4.3.2.1.in-addr.arpa.", + Rrtype: dns.TypePTR, + Class: dns.ClassINET, + }, + Ptr: "foo.node.peer1.peer.default.ap.consul.", + }, + }, + }, + }, + { + name: "[ENT] PTR Lookup for service in default namespace, query type is PTR", + request: &dns.Msg{ + MsgHdr: dns.MsgHdr{ + Opcode: dns.OpcodeQuery, + }, + Question: []dns.Question{ + { + Name: "4.3.2.1.in-addr.arpa", + Qtype: dns.TypePTR, + Qclass: dns.ClassINET, + }, + }, + }, + configureDataFetcher: func(fetcher discovery.CatalogDataFetcher) { + results := []*discovery.Result{ + { + Node: &discovery.Location{Name: "foo", Address: "1.2.3.4"}, + Type: discovery.ResultTypeService, + Service: &discovery.Location{Name: "foo", Address: "foo"}, + Tenancy: discovery.ResultTenancy{ + Datacenter: "dc2", + Namespace: "default", + }, + }, + } + + fetcher.(*discovery.MockCatalogDataFetcher). + On("FetchRecordsByIp", mock.Anything, mock.Anything). + Return(results, nil). + Run(func(args mock.Arguments) { + req := args.Get(1).(net.IP) + + require.NotNil(t, req) + require.Equal(t, "1.2.3.4", req.String()) + }) + }, + response: &dns.Msg{ + MsgHdr: dns.MsgHdr{ + Opcode: dns.OpcodeQuery, + Response: true, + Authoritative: true, + }, + Compress: true, + Question: []dns.Question{ + { + Name: "4.3.2.1.in-addr.arpa.", + Qtype: dns.TypePTR, + Qclass: dns.ClassINET, + }, + }, + Answer: []dns.RR{ + &dns.PTR{ + Hdr: dns.RR_Header{ + Name: "4.3.2.1.in-addr.arpa.", + Rrtype: dns.TypePTR, + Class: dns.ClassINET, + }, + Ptr: "foo.service.default.dc2.consul.", + }, + }, + }, + }, + { + name: "[ENT] PTR Lookup for service in a non-default namespace, query type is PTR", + request: &dns.Msg{ + MsgHdr: dns.MsgHdr{ + Opcode: dns.OpcodeQuery, + }, + Question: []dns.Question{ + { + Name: "4.3.2.1.in-addr.arpa", + Qtype: dns.TypePTR, + Qclass: dns.ClassINET, + }, + }, + }, + configureDataFetcher: func(fetcher discovery.CatalogDataFetcher) { + results := []*discovery.Result{ + { + Node: &discovery.Location{Name: "foo-node", Address: "1.2.3.4"}, + Type: discovery.ResultTypeService, + Service: &discovery.Location{Name: "foo", Address: "foo"}, + Tenancy: discovery.ResultTenancy{ + Datacenter: "dc2", + Namespace: "bar", + Partition: "baz", + }, + }, + } + + fetcher.(*discovery.MockCatalogDataFetcher). + On("FetchRecordsByIp", mock.Anything, mock.Anything). + Return(results, nil). + Run(func(args mock.Arguments) { + req := args.Get(1).(net.IP) + + require.NotNil(t, req) + require.Equal(t, "1.2.3.4", req.String()) + }) + }, + response: &dns.Msg{ + MsgHdr: dns.MsgHdr{ + Opcode: dns.OpcodeQuery, + Response: true, + Authoritative: true, + }, + Compress: true, + Question: []dns.Question{ + { + Name: "4.3.2.1.in-addr.arpa.", + Qtype: dns.TypePTR, + Qclass: dns.ClassINET, + }, + }, + Answer: []dns.RR{ + &dns.PTR{ + Hdr: dns.RR_Header{ + Name: "4.3.2.1.in-addr.arpa.", + Rrtype: dns.TypePTR, + Class: dns.ClassINET, + }, + Ptr: "foo.service.bar.dc2.consul.", + }, + }, + }, + }, + { + name: "[CE] PTR Lookup for node w/ peer name, query type is ANY", + request: &dns.Msg{ + MsgHdr: dns.MsgHdr{ + Opcode: dns.OpcodeQuery, + }, + Question: []dns.Question{ + { + Name: "4.3.2.1.in-addr.arpa", + Qtype: dns.TypeANY, + Qclass: dns.ClassINET, + }, + }, + }, + configureDataFetcher: func(fetcher discovery.CatalogDataFetcher) { + results := []*discovery.Result{ + { + Node: &discovery.Location{Name: "foo", Address: "1.2.3.4"}, + Type: discovery.ResultTypeNode, + Service: &discovery.Location{Name: "foo", Address: "foo"}, + Tenancy: discovery.ResultTenancy{ + Datacenter: "dc2", + PeerName: "peer1", + }, + }, + } + + fetcher.(*discovery.MockCatalogDataFetcher). + On("FetchRecordsByIp", mock.Anything, mock.Anything). + Return(results, nil). + Run(func(args mock.Arguments) { + req := args.Get(1).(net.IP) + + require.NotNil(t, req) + require.Equal(t, "1.2.3.4", req.String()) + }) + }, + response: &dns.Msg{ + MsgHdr: dns.MsgHdr{ + Opcode: dns.OpcodeQuery, + Response: true, + Authoritative: true, + }, + Compress: true, + Question: []dns.Question{ + { + Name: "4.3.2.1.in-addr.arpa.", + Qtype: dns.TypeANY, + Qclass: dns.ClassINET, + }, + }, + Answer: []dns.RR{ + &dns.PTR{ + Hdr: dns.RR_Header{ + Name: "4.3.2.1.in-addr.arpa.", + Rrtype: dns.TypePTR, + Class: dns.ClassINET, + }, + Ptr: "foo.node.peer1.peer.consul.", + }, + }, + }, + }, + { + name: "[CE] PTR Lookup for service, query type is PTR", + request: &dns.Msg{ + MsgHdr: dns.MsgHdr{ + Opcode: dns.OpcodeQuery, + }, + Question: []dns.Question{ + { + Name: "4.3.2.1.in-addr.arpa", + Qtype: dns.TypePTR, + Qclass: dns.ClassINET, + }, + }, + }, + configureDataFetcher: func(fetcher discovery.CatalogDataFetcher) { + results := []*discovery.Result{ + { + Node: &discovery.Location{Name: "foo", Address: "1.2.3.4"}, + Service: &discovery.Location{Name: "foo", Address: "foo"}, + Type: discovery.ResultTypeService, + Tenancy: discovery.ResultTenancy{ + Datacenter: "dc2", + }, + }, + } + + fetcher.(*discovery.MockCatalogDataFetcher). + On("FetchRecordsByIp", mock.Anything, mock.Anything). + Return(results, nil). + Run(func(args mock.Arguments) { + req := args.Get(1).(net.IP) + + require.NotNil(t, req) + require.Equal(t, "1.2.3.4", req.String()) + }) + }, + response: &dns.Msg{ + MsgHdr: dns.MsgHdr{ + Opcode: dns.OpcodeQuery, + Response: true, + Authoritative: true, + }, + Compress: true, + Question: []dns.Question{ + { + Name: "4.3.2.1.in-addr.arpa.", + Qtype: dns.TypePTR, + Qclass: dns.ClassINET, + }, + }, + Answer: []dns.RR{ + &dns.PTR{ + Hdr: dns.RR_Header{ + Name: "4.3.2.1.in-addr.arpa.", + Rrtype: dns.TypePTR, + Class: dns.ClassINET, + }, + Ptr: "foo.service.dc2.consul.", + }, + }, + }, + }, // V2 Workload Lookup { name: "workload A query w/ port, returns A record", @@ -2151,7 +2497,7 @@ func Test_HandleRequest(t *testing.T) { Weight: 2, Priority: 1, Port: 5678, - Target: "api.port.foo-1.workload.consul.", // TODO (v2-dns): verify this shouldn't include tenancy for workloads + Target: "api.port.foo-1.workload.default.ns.default.ap.consul.", }, &dns.SRV{ Hdr: dns.RR_Header{ @@ -2163,7 +2509,7 @@ func Test_HandleRequest(t *testing.T) { Weight: 3, Priority: 1, Port: 5678, - Target: "api.port.foo-2.workload.consul.", // TODO (v2-dns): verify this shouldn't include tenancy for workloads + Target: "api.port.foo-2.workload.default.ns.default.ap.consul.", }, &dns.SRV{ Hdr: dns.RR_Header{ @@ -2175,13 +2521,13 @@ func Test_HandleRequest(t *testing.T) { Weight: 3, Priority: 1, Port: 21000, - Target: "mesh.port.foo-2.workload.consul.", // TODO (v2-dns): verify this shouldn't include tenancy for workloads + Target: "mesh.port.foo-2.workload.default.ns.default.ap.consul.", }, }, Extra: []dns.RR{ &dns.A{ Hdr: dns.RR_Header{ - Name: "api.port.foo-1.workload.consul.", + Name: "api.port.foo-1.workload.default.ns.default.ap.consul.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: uint32(123), @@ -2190,7 +2536,7 @@ func Test_HandleRequest(t *testing.T) { }, &dns.A{ Hdr: dns.RR_Header{ - Name: "api.port.foo-2.workload.consul.", + Name: "api.port.foo-2.workload.default.ns.default.ap.consul.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: uint32(123), @@ -2199,7 +2545,7 @@ func Test_HandleRequest(t *testing.T) { }, &dns.A{ Hdr: dns.RR_Header{ - Name: "mesh.port.foo-2.workload.consul.", + Name: "mesh.port.foo-2.workload.default.ns.default.ap.consul.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: uint32(123), @@ -2282,13 +2628,13 @@ func Test_HandleRequest(t *testing.T) { Weight: 3, Priority: 1, Port: 21000, - Target: "mesh.port.foo-2.workload.consul.", // TODO (v2-dns): verify this shouldn't include tenancy for workloads + Target: "mesh.port.foo-2.workload.default.ns.default.ap.consul.", }, }, Extra: []dns.RR{ &dns.A{ Hdr: dns.RR_Header{ - Name: "mesh.port.foo-2.workload.consul.", + Name: "mesh.port.foo-2.workload.default.ns.default.ap.consul.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: uint32(123), @@ -2637,7 +2983,7 @@ func Test_HandleRequest(t *testing.T) { }, Question: []dns.Question{ { - Name: "foo.query.consul.", + Name: "foo.query.dc1.cluster.consul.", Qtype: dns.TypeA, Qclass: dns.ClassINET, }, @@ -2678,6 +3024,7 @@ func Test_HandleRequest(t *testing.T) { Run(func(args mock.Arguments) { req := args.Get(1).(*discovery.QueryPayload) require.Equal(t, "foo", req.Name) + require.Equal(t, "dc1", req.Tenancy.Datacenter) }) }, validateAndNormalizeExpected: true, @@ -2690,7 +3037,7 @@ func Test_HandleRequest(t *testing.T) { Compress: true, Question: []dns.Question{ { - Name: "foo.query.consul.", + Name: "foo.query.dc1.cluster.consul.", Qtype: dns.TypeA, Qclass: dns.ClassINET, }, @@ -2698,7 +3045,7 @@ func Test_HandleRequest(t *testing.T) { Answer: []dns.RR{ &dns.A{ Hdr: dns.RR_Header{ - Name: "foo.query.consul.", + Name: "foo.query.dc1.cluster.consul.", Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 1, @@ -2710,8 +3057,6 @@ func Test_HandleRequest(t *testing.T) { }, } - testCases = append(testCases, getAdditionalTestCases(t)...) - for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { runHandleTestCases(t, tc) diff --git a/agent/dns_catalogv2_test.go b/agent/dns_catalogv2_test.go index 4bc205ecf3..c806e59101 100644 --- a/agent/dns_catalogv2_test.go +++ b/agent/dns_catalogv2_test.go @@ -244,7 +244,7 @@ func TestDNS_CatalogV2_Basic(t *testing.T) { if client.Net == "tcp" { for portName, port := range dbWorkloadPorts { for workloadName, workload := range dbWorkloads { - workloadTarget := fmt.Sprintf("%s.port.%s.workload.consul.", portName, workloadName) + workloadTarget := fmt.Sprintf("%s.port.%s.workload.default.ns.default.ap.consul.", portName, workloadName) workloadHost := workload.Addresses[0].Host srvRec := findSrvAnswerForTarget(t, in, workloadTarget) @@ -261,9 +261,9 @@ func TestDNS_CatalogV2_Basic(t *testing.T) { require.Equal(t, 9, len(in.Answer), "answer count did not match expected\n\n%s", in.String()) require.Equal(t, 9, len(in.Extra), "extra answer count did not match expected\n\n%s", in.String()) } else { - // Expect 1 result per port, per workload, up to the default limit of 3. - require.Equal(t, 3, len(in.Answer), "answer count did not match expected\n\n%s", in.String()) - require.Equal(t, 3, len(in.Extra), "extra answer count did not match expected\n\n%s", in.String()) + // Expect 1 result per port, per workload, up to the default limit of 3. In practice the results are truncated at 2. + require.Equal(t, 2, len(in.Answer), "answer count did not match expected\n\n%s", in.String()) + require.Equal(t, 2, len(in.Extra), "extra answer count did not match expected\n\n%s", in.String()) } } @@ -272,7 +272,7 @@ func TestDNS_CatalogV2_Basic(t *testing.T) { question := fmt.Sprintf("%s.port.db.service.consul.", portName) for workloadName, workload := range dbWorkloads { - workloadTarget := fmt.Sprintf("%s.port.%s.workload.consul.", portName, workloadName) + workloadTarget := fmt.Sprintf("%s.port.%s.workload.default.ns.default.ap.consul.", portName, workloadName) workloadHost := workload.Addresses[0].Host m := new(dns.Msg) @@ -361,10 +361,10 @@ func TestDNS_CatalogV2_Basic(t *testing.T) { "db-3": dns.TypeAAAA, } { for _, question := range []string{ - fmt.Sprintf("%s.workload.consul.", workloadName), - fmt.Sprintf("tcp.port.%s.workload.consul.", workloadName), - fmt.Sprintf("admin.port.%s.workload.consul.", workloadName), - fmt.Sprintf("mesh.port.%s.workload.consul.", workloadName), + fmt.Sprintf("%s.workload.default.ns.default.ap.consul.", workloadName), + fmt.Sprintf("tcp.port.%s.workload.default.ns.default.ap.consul.", workloadName), + fmt.Sprintf("admin.port.%s.workload.default.ns.default.ap.consul.", workloadName), + fmt.Sprintf("mesh.port.%s.workload.default.ns.default.ap.consul.", workloadName), } { workload := dbWorkloads[workloadName] workloadHost := workload.Addresses[0].Host