Backport of fix(v2dns): add node ttl to workloads, comment cleanup, a…

…nd changelog into release/1.18.x (#20650) * backport of commit 5ffa404 * backport of commit 17de0e4 --------- Co-authored-by: DanStough <[email protected]>
hashicorp · Feb 15, 2024 · 00a3f24 · 00a3f24
1 parent f986506
commit 00a3f24
Show file tree

Hide file tree

Showing 12 changed files with 34 additions and 47 deletions.
diff --git a/.changelog/20643.txt b/.changelog/20643.txt
@@ -0,0 +1,7 @@
+```release-note:feature
+dns: adds experimental support for a refactored DNS server that is v1 and v2 Catalog compatible. 
+Use `v2dns` in the `experiments` agent config to enable. 
+It will automatically be enabled when using the `resource-apis` (Catalog v2) experiment.
+The new DNS implementation will be the default in Consul 1.19.
+See the [Consul 1.18.x Release Notes](https://developer.hashicorp.com/consul/docs/release-notes/consul/v1_18_x) for deprecated DNS features.
+```
diff --git a/agent/discovery/discovery.go b/agent/discovery/discovery.go
@@ -20,7 +20,6 @@ var (
 
 // ECSNotGlobalError may be used to wrap an error or nil, to indicate that the
 // EDNS client subnet source scope is not global.
-// TODO (v2-dns): prepared queries errors are wrapped by this
 type ECSNotGlobalError struct {
 	error
 }

diff --git a/agent/discovery/query_fetcher_v1.go b/agent/discovery/query_fetcher_v1.go
@@ -58,7 +58,6 @@ type v1DataFetcherDynamicConfig struct {
 
 // V1DataFetcher is used to fetch data from the V1 catalog.
 type V1DataFetcher struct {
-	// TODO(v2-dns): store this in the config.
 	defaultEnterpriseMeta acl.EnterpriseMeta
 	dynamicConfig         atomic.Value
 	logger                hclog.Logger
@@ -275,7 +274,7 @@ func (f *V1DataFetcher) FetchRecordsByIp(reqCtx Context, ip net.IP) ([]*Result,
 	}
 
 	// nothing found locally, recurse
-	// TODO: (v2-dns) implement recursion
+	// TODO: (v2-dns) implement recursion (NET-7883)
 	//d.handleRecurse(resp, req)
 
 	return nil, fmt.Errorf("unhandled error in FetchRecordsByIp")

diff --git a/agent/discovery/query_fetcher_v1_test.go b/agent/discovery/query_fetcher_v1_test.go
@@ -103,7 +103,6 @@ func Test_FetchVirtualIP(t *testing.T) {
 						*reply = tc.expectedResult.Service.Address
 					}
 				})
-			// TODO (v2-dns): mock these properly
 			translateServicePortFunc := func(dc string, port int, taggedAddresses map[string]structs.ServiceAddress) int { return 0 }
 			rpcFuncForServiceNodes := func(ctx context.Context, req structs.ServiceSpecificRequest) (structs.IndexedCheckServiceNodes, cache.ResultMeta, error) {
 				return structs.IndexedCheckServiceNodes{}, cache.ResultMeta{}, nil
@@ -166,7 +165,6 @@ func Test_FetchEndpoints(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

diff --git a/agent/dns/parser_test.go b/agent/dns/parser_test.go
diff --git a/agent/dns/router.go b/agent/dns/router.go
@@ -57,16 +57,15 @@ type Context struct {
 
 // RouterDynamicConfig is the dynamic configuration that can be hot-reloaded
 type RouterDynamicConfig struct {
-	ARecordLimit          int
-	DisableCompression    bool
-	EnableDefaultFailover bool // TODO (v2-dns): plumbing required for this new V2 setting. This is the agent configured default
-	EnableTruncate        bool
-	NodeMetaTXT           bool
-	NodeTTL               time.Duration
-	Recursors             []string
-	RecursorTimeout       time.Duration
-	RecursorStrategy      structs.RecursorStrategy
-	SOAConfig             SOAConfig
+	ARecordLimit       int
+	DisableCompression bool
+	EnableTruncate     bool
+	NodeMetaTXT        bool
+	NodeTTL            time.Duration
+	Recursors          []string
+	RecursorTimeout    time.Duration
+	RecursorStrategy   structs.RecursorStrategy
+	SOAConfig          SOAConfig
 	// TTLRadix sets service TTLs by prefix, eg: "database-*"
 	TTLRadix *radix.Tree
 	// TTLStrict sets TTLs to service by full name match. It Has higher priority than TTLRadix
@@ -331,13 +330,6 @@ func getTTLForResult(name string, overrideTTL *uint32, query *discovery.Query, c
 	}
 
 	switch query.QueryType {
-	// TODO (v2-dns): currently have to do this related to the results type being changed to node whe
-	// the v1 data fetcher encounters a blank service address and uses the node address instead.
-	// we will revisiting this when look at modifying the discovery result struct to
-	// possibly include additional metadata like the node address.
-	case discovery.QueryTypeWorkload:
-		// TODO (v2-dns): we need to discuss what we want to do for workload TTLs
-		return 0
 	case discovery.QueryTypeService, discovery.QueryTypePreparedQuery:
 		ttl, ok := cfg.getTTLForService(name)
 		if ok {
@@ -554,7 +546,6 @@ func (r *Router) serializeQueryResults(req *dns.Msg, reqCtx Context,
 				// The datacenter should be empty during translation if it is a peering lookup.
 				// This should be fine because we should always prefer the WAN address.
 
-				// TODO (v2-dns): this needs a clean up so we're not assuming this everywhere.
 				address := ""
 				if result.Service != nil {
 					address = result.Service.Address
@@ -982,22 +973,20 @@ func (r *Router) getAnswerExtraAndNs(result *discovery.Result, port discovery.Po
 		}
 		answer = append(answer, ptr)
 	case qType == dns.TypeNS:
-		// TODO (v2-dns): fqdn in V1 has the datacenter included, this would need to be added to discovery.Result
 		resultType := result.Type
 		target := result.Node.Name
 		if parseRequestType(req) == requestTypeConsul && resultType == discovery.ResultTypeService {
 			resultType = discovery.ResultTypeNode
 		}
 		fqdn := canonicalNameForResult(resultType, target, domain, result.Tenancy, port.Name)
-		extraRecord := makeIPBasedRecord(fqdn, nodeAddress, ttl) // TODO (v2-dns): this is not sufficient, because recursion and CNAMES are supported
+		extraRecord := makeIPBasedRecord(fqdn, nodeAddress, ttl)
 
 		answer = append(answer, makeNSRecord(domain, fqdn, ttl))
 		extra = append(extra, extraRecord)
 	case qType == dns.TypeSOA:
-		// TODO (v2-dns): fqdn in V1 has the datacenter included, this would need to be added to discovery.Result
 		// to be returned in the result.
 		fqdn := canonicalNameForResult(result.Type, result.Node.Name, domain, result.Tenancy, port.Name)
-		extraRecord := makeIPBasedRecord(fqdn, nodeAddress, ttl) // TODO (v2-dns): this is not sufficient, because recursion and CNAMES are supported
+		extraRecord := makeIPBasedRecord(fqdn, nodeAddress, ttl)
 
 		ns = append(ns, makeNSRecord(domain, fqdn, ttl))
 		extra = append(extra, extraRecord)

diff --git a/agent/dns/router_test.go b/agent/dns/router_test.go
@@ -1874,6 +1874,7 @@ func Test_HandleRequest(t *testing.T) {
 							Name:   "api.port.foo.workload.consul.",
 							Rrtype: dns.TypeA,
 							Class:  dns.ClassINET,
+							Ttl:    123,
 						},
 						A: net.ParseIP("1.2.3.4"),
 					},
@@ -1932,6 +1933,7 @@ func Test_HandleRequest(t *testing.T) {
 							Name:   "foo.workload.consul.",
 							Rrtype: dns.TypeA,
 							Class:  dns.ClassINET,
+							Ttl:    123,
 						},
 						A: net.ParseIP("1.2.3.4"),
 					},
@@ -1994,6 +1996,7 @@ func Test_HandleRequest(t *testing.T) {
 							Name:   "foo.workload.bar.ns.baz.ap.dc3.dc.consul.",
 							Rrtype: dns.TypeA,
 							Class:  dns.ClassINET,
+							Ttl:    123,
 						},
 						A: net.ParseIP("1.2.3.4"),
 					},
@@ -2058,6 +2061,7 @@ func Test_HandleRequest(t *testing.T) {
 							Name:   "api.port.foo.workload.consul.",
 							Rrtype: dns.TypeCNAME,
 							Class:  dns.ClassINET,
+							Ttl:    123,
 						},
 						Target: "foo.example.com.",
 					},
@@ -2165,6 +2169,7 @@ func Test_HandleRequest(t *testing.T) {
 							Name:   "api.port.foo.workload.consul.",
 							Rrtype: dns.TypeCNAME,
 							Class:  dns.ClassINET,
+							Ttl:    123,
 						},
 						Target: "foo.example.com.",
 					},
@@ -2173,6 +2178,7 @@ func Test_HandleRequest(t *testing.T) {
 							Name:   "foo.example.com.",
 							Rrtype: dns.TypeA,
 							Class:  dns.ClassINET,
+							Ttl:    123,
 						},
 						A: net.ParseIP("1.2.3.4"),
 					},
@@ -2280,15 +2286,17 @@ func Test_HandleRequest(t *testing.T) {
 							Name:   "api.port.foo.workload.consul.",
 							Rrtype: dns.TypeCNAME,
 							Class:  dns.ClassINET,
+							Ttl:    123,
 						},
 						Target: "foo.example.com.",
 					},
-					// TODO (v2-dns): this next record is wrong per the RFC
+					// TODO (v2-dns): this next record is wrong per the RFC-1034 mentioned in the comment above (NET-8060)
 					&dns.A{
 						Hdr: dns.RR_Header{
 							Name:   "foo.example.com.",
 							Rrtype: dns.TypeCNAME,
 							Class:  dns.ClassINET,
+							Ttl:    123,
 						},
 						A: net.ParseIP("1.2.3.4"),
 					},
@@ -2383,7 +2391,7 @@ func Test_HandleRequest(t *testing.T) {
 				Answer: []dns.RR{
 					&dns.A{
 						Hdr: dns.RR_Header{
-							Name:   "foo.service.consul.", // TODO (v2-dns): verify this shouldn't include tenancy for workloads
+							Name:   "foo.service.consul.",
 							Rrtype: dns.TypeA,
 							Class:  dns.ClassINET,
 							Ttl:    uint32(123),
@@ -2882,7 +2890,7 @@ func Test_HandleRequest(t *testing.T) {
 						},
 						A: net.ParseIP("1.2.3.4"),
 					},
-					// TODO (v2-dns): This needs to be de-dupped
+					// TODO (v2-dns): This needs to be de-duplicated (NET-8064)
 					&dns.A{
 						Hdr: dns.RR_Header{
 							Name:   "foo-1.example.com.",

diff --git a/agent/dns/server_test.go b/agent/dns/server_test.go
diff --git a/agent/dns_node_lookup_test.go b/agent/dns_node_lookup_test.go
@@ -155,8 +155,7 @@ func TestDNS_NodeLookup_CaseInsensitive(t *testing.T) {
 	}
 }
 
-// TODO (v2-dns): NET-7632 - Fix node lookup when question name has a period in it.
-// Answer is coming back empty
+// V2 DNS does not support node names with a period. This will be deprecated.
 func TestDNS_NodeLookup_PeriodName(t *testing.T) {
 	if testing.Short() {
 		t.Skip("too slow for testing.Short")

diff --git a/agent/dns_service_lookup_test.go b/agent/dns_service_lookup_test.go
@@ -1989,7 +1989,7 @@ func TestDNS_ServiceLookup_CaseInsensitive(t *testing.T) {
 	}
 }
 
-// TODO (v2-dns): NET-7632 - Fix node and prepared query lookups when question name has a period in it
+// V2 DNS: we have deprecated support for service tags w/ periods
 func TestDNS_ServiceLookup_TagPeriod(t *testing.T) {
 	if testing.Short() {
 		t.Skip("too slow for testing.Short")

diff --git a/agent/dns_test.go b/agent/dns_test.go
@@ -3646,7 +3646,7 @@ func TestDNS_V1ConfigReload(t *testing.T) {
 
 }
 
-// TODO (v2-dns) add a test for checking the V2 DNS Server reloads the config
+// TODO (v2-dns) add a test for checking the V2 DNS Server reloads the config (NET-8056)
 
 func TestDNS_ReloadConfig_DuringQuery(t *testing.T) {
 	if testing.Short() {

diff --git a/agent/grpc-external/services/dns/server_v2.go b/agent/grpc-external/services/dns/server_v2.go
@@ -72,7 +72,7 @@ func (s *ServerV2) Query(ctx context.Context, req *pbdns.QueryRequest) (*pbdns.Q
 		return nil, status.Error(codes.Internal, fmt.Sprintf("failure decoding dns request: %s", err.Error()))
 	}
 
-	// TODO (v2-dns): parse token and other context metadata from the grpc request/metadata
+	// TODO (v2-dns): parse token and other context metadata from the grpc request/metadata (NET-7885)
 	reqCtx := agentdns.Context{
 		Token: s.TokenFunc(),
 	}