Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(v2dns): add node ttl to workloads, comment cleanup, and changelog #20643

Merged
merged 2 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changelog/20643.txt
Original file line number Diff line number Diff line change
@@ -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.
```
1 change: 0 additions & 1 deletion agent/discovery/discovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
3 changes: 1 addition & 2 deletions agent/discovery/query_fetcher_v1.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand Down
2 changes: 0 additions & 2 deletions agent/discovery/query_fetcher_v1_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 0 additions & 6 deletions agent/dns/parser_test.go

This file was deleted.

33 changes: 11 additions & 22 deletions agent/dns/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
14 changes: 11 additions & 3 deletions agent/dns/router_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
},
Expand Down Expand Up @@ -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"),
},
Expand Down Expand Up @@ -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"),
},
Expand Down Expand Up @@ -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.",
},
Expand Down Expand Up @@ -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.",
},
Expand All @@ -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"),
},
Expand Down Expand Up @@ -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"),
},
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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.",
Expand Down
6 changes: 0 additions & 6 deletions agent/dns/server_test.go

This file was deleted.

3 changes: 1 addition & 2 deletions agent/dns_node_lookup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
2 changes: 1 addition & 1 deletion agent/dns_service_lookup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1981,7 +1981,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")
Expand Down
2 changes: 1 addition & 1 deletion agent/dns_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3623,7 +3623,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() {
Expand Down
2 changes: 1 addition & 1 deletion agent/grpc-external/services/dns/server_v2.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
}
Expand Down
Loading