Finishes up DNS compression by adding opt-out, tests, and documentation. Fixes trim routine.

command/agent/config.go

@@ -104,6 +104,11 @@ type DNSConfig struct {
 	// whose health checks are in any non-passing state. By
 	// default, only nodes in a critical state are excluded.
 	OnlyPassing bool `mapstructure:"only_passing"`
+
+	// DisableCompression is used to control whether DNS responses are
+	// compressed. In Consul 0.7 this was turned on by default and this
+	// config was added as an opt-out.
+	DisableCompression bool `mapstructure:"disable_compression"`
 }
 
 // Telemetry is the telemetry configuration for the server
@@ -1300,6 +1305,9 @@ func MergeConfig(a, b *Config) *Config {
 	if b.DNSConfig.OnlyPassing {
 		result.DNSConfig.OnlyPassing = true
 	}
+	if b.DNSConfig.DisableCompression {
+		result.DNSConfig.DisableCompression = true
+	}
 	if b.CheckUpdateIntervalRaw != "" || b.CheckUpdateInterval != 0 {
 		result.CheckUpdateInterval = b.CheckUpdateInterval
 	}

command/agent/config_test.go

@@ -608,6 +608,17 @@ func TestDecodeConfig(t *testing.T) {
 		t.Fatalf("bad: %#v", config)
 	}
 
+	// DNS disable compression
+	input = `{"dns_config": {"disable_compression": true}}`
+	config, err = DecodeConfig(bytes.NewReader([]byte(input)))
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	if !config.DNSConfig.DisableCompression {
+		t.Fatalf("bad: %#v", config)
+	}
+
 	// CheckUpdateInterval
 	input = `{"check_update_interval": "10m"}`
 	config, err = DecodeConfig(bytes.NewReader([]byte(input)))
@@ -1371,6 +1382,7 @@ func TestMergeConfig(t *testing.T) {
 		DNSConfig: DNSConfig{
 			AllowStale:         false,
 			EnableTruncate:     true,
+			DisableCompression: true,
 			MaxStale:           30 * time.Second,
 			NodeTTL:            10 * time.Second,
 			ServiceTTL: map[string]time.Duration{

command/agent/dns.go

@@ -180,7 +180,7 @@ func (d *DNSServer) handlePtr(resp dns.ResponseWriter, req *dns.Msg) {
 	// Setup the message response
 	m := new(dns.Msg)
 	m.SetReply(req)
-	m.Compress = true
+	m.Compress = !d.config.DisableCompression
 	m.Authoritative = true
 	m.RecursionAvailable = (len(d.recursors) > 0)
 
@@ -250,7 +250,7 @@ func (d *DNSServer) handleQuery(resp dns.ResponseWriter, req *dns.Msg) {
 	// Setup the message response
 	m := new(dns.Msg)
 	m.SetReply(req)
-	m.Compress = true
+	m.Compress = !d.config.DisableCompression
 	m.Authoritative = true
 	m.RecursionAvailable = (len(d.recursors) > 0)
 
@@ -506,10 +506,17 @@ func trimUDPAnswers(config *DNSConfig, resp *dns.Msg) (trimmed bool) {
 		resp.Answer = resp.Answer[:maxAnswers]
 	}
 
-	// This enforces the hard limit of 512 bytes per the RFC.
+	// This enforces the hard limit of 512 bytes per the RFC. Note that we
+	// temporarily switch to uncompressed so that we limit to a response
+	// that will not exceed 512 bytes uncompressed, which is more
+	// conservative and will allow our responses to be compliant even if
+	// some downstream server uncompresses them.
+	compress := resp.Compress
+	resp.Compress = false
 	for len(resp.Answer) > 0 && resp.Len() > 512 {
 		resp.Answer = resp.Answer[:len(resp.Answer)-1]
 	}
+	resp.Compress = compress
 
 	return len(resp.Answer) < numAnswers
 }
@@ -788,7 +795,11 @@ func (d *DNSServer) handleRecurse(resp dns.ResponseWriter, req *dns.Msg) {
 	for _, recursor := range d.recursors {
 		r, rtt, err = c.Exchange(req, recursor)
 		if err == nil {
-			r.Compress = true
+			// Compress the response; we don't know if the incoming
+			// response was compressed or not, so by not compressing
+			// we might generate an invalid packet on the way out.
+			r.Compress = !d.config.DisableCompression
+
 			// Forward the response
 			d.logger.Printf("[DEBUG] dns: recurse RTT for %v (%v)", q, rtt)
 			if err := resp.WriteMsg(r); err != nil {
@@ -804,7 +815,7 @@ func (d *DNSServer) handleRecurse(resp dns.ResponseWriter, req *dns.Msg) {
 		q, resp.RemoteAddr().String(), resp.RemoteAddr().Network())
 	m := &dns.Msg{}
 	m.SetReply(req)
-	m.Compress = true
+	m.Compress = !d.config.DisableCompression
 	m.RecursionAvailable = true
 	m.SetRcode(req, dns.RcodeServerFailure)
 	resp.WriteMsg(m)

command/agent/dns_test.go

@@ -3200,3 +3200,209 @@ func TestDNS_PreparedQuery_AgentSource(t *testing.T) {
 		}
 	}
 }
+
+func TestDNS_Compression_trimUDPAnswers(t *testing.T) {
+	config := &DefaultConfig().DNSConfig
+
+	m := dns.Msg{}
+	trimUDPAnswers(config, &m)
+	if m.Compress {
+		t.Fatalf("compression should be off")
+	}
+
+	// The trim function temporarily turns off compression, so we need to
+	// make sure the setting gets restored properly.
+	m.Compress = true
+	trimUDPAnswers(config, &m)
+	if !m.Compress {
+		t.Fatalf("compression should be on")
+	}
+}
+
+func TestDNS_Compression_Query(t *testing.T) {
+	dir, srv := makeDNSServer(t)
+	defer os.RemoveAll(dir)
+	defer srv.agent.Shutdown()
+
+	testutil.WaitForLeader(t, srv.agent.RPC, "dc1")
+
+	// Register a node with a service.
+	{
+		args := &structs.RegisterRequest{
+			Datacenter: "dc1",
+			Node:       "foo",
+			Address:    "127.0.0.1",
+			Service: &structs.NodeService{
+				Service: "db",
+				Tags:    []string{"master"},
+				Port:    12345,
+			},
+		}
+
+		var out struct{}
+		if err := srv.agent.RPC("Catalog.Register", args, &out); err != nil {
+			t.Fatalf("err: %v", err)
+		}
+	}
+
+	// Register an equivalent prepared query.
+	var id string
+	{
+		args := &structs.PreparedQueryRequest{
+			Datacenter: "dc1",
+			Op:         structs.PreparedQueryCreate,
+			Query: &structs.PreparedQuery{
+				Service: structs.ServiceQuery{
+					Service: "db",
+				},
+			},
+		}
+		if err := srv.agent.RPC("PreparedQuery.Apply", args, &id); err != nil {
+			t.Fatalf("err: %v", err)
+		}
+	}
+
+	// Look up the service directly and via prepared query.
+	questions := []string{
+		"db.service.consul.",
+		id + ".query.consul.",
+	}
+	for _, question := range questions {
+		m := new(dns.Msg)
+		m.SetQuestion(question, dns.TypeSRV)
+
+		addr, _ := srv.agent.config.ClientListener("", srv.agent.config.Ports.DNS)
+		conn, err := dns.Dial("udp", addr.String())
+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
+
+		// Do a manual exchange with compression on (the default).
+		srv.config.DisableCompression = false
+		if err := conn.WriteMsg(m); err != nil {
+			t.Fatalf("err: %v", err)
+		}
+		p := make([]byte, dns.MaxMsgSize)
+		compressed, err := conn.Read(p)
+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
+
+		// Disable compression and try again.
+		srv.config.DisableCompression = true
+		if err := conn.WriteMsg(m); err != nil {
+			t.Fatalf("err: %v", err)
+		}
+		unc, err := conn.Read(p)
+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
+
+		// We can't see the compressed status given the DNS API, so we
+		// just make sure the message is smaller to see if it's
+		// respecting the flag.
+		if compressed == 0 || unc == 0 || compressed >= unc {
+			t.Fatalf("'%s' doesn't look compressed: %d vs. %d", question, compressed, unc)
+		}
+	}
+}
+
+func TestDNS_Compression_ReverseLookup(t *testing.T) {
+	dir, srv := makeDNSServer(t)
+	defer os.RemoveAll(dir)
+	defer srv.agent.Shutdown()
+
+	testutil.WaitForLeader(t, srv.agent.RPC, "dc1")
+
+	// Register node.
+	args := &structs.RegisterRequest{
+		Datacenter: "dc1",
+		Node:       "foo2",
+		Address:    "127.0.0.2",
+	}
+	var out struct{}
+	if err := srv.agent.RPC("Catalog.Register", args, &out); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	m := new(dns.Msg)
+	m.SetQuestion("2.0.0.127.in-addr.arpa.", dns.TypeANY)
+
+	addr, _ := srv.agent.config.ClientListener("", srv.agent.config.Ports.DNS)
+	conn, err := dns.Dial("udp", addr.String())
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// Do a manual exchange with compression on (the default).
+	if err := conn.WriteMsg(m); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	p := make([]byte, dns.MaxMsgSize)
+	compressed, err := conn.Read(p)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// Disable compression and try again.
+	srv.config.DisableCompression = true
+	if err := conn.WriteMsg(m); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	unc, err := conn.Read(p)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// We can't see the compressed status given the DNS API, so we just make
+	// sure the message is smaller to see if it's respecting the flag.
+	if compressed == 0 || unc == 0 || compressed >= unc {
+		t.Fatalf("doesn't look compressed: %d vs. %d", compressed, unc)
+	}
+}
+
+func TestDNS_Compression_Recurse(t *testing.T) {
+	recursor := makeRecursor(t, []dns.RR{dnsA("apple.com", "1.2.3.4")})
+	defer recursor.Shutdown()
+
+	dir, srv := makeDNSServerConfig(t, func(c *Config) {
+		c.DNSRecursor = recursor.Addr
+	}, nil)
+	defer os.RemoveAll(dir)
+	defer srv.agent.Shutdown()
+
+	m := new(dns.Msg)
+	m.SetQuestion("apple.com.", dns.TypeANY)
+
+	addr, _ := srv.agent.config.ClientListener("", srv.agent.config.Ports.DNS)
+	conn, err := dns.Dial("udp", addr.String())
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// Do a manual exchange with compression on (the default).
+	if err := conn.WriteMsg(m); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	p := make([]byte, dns.MaxMsgSize)
+	compressed, err := conn.Read(p)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// Disable compression and try again.
+	srv.config.DisableCompression = true
+	if err := conn.WriteMsg(m); err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	unc, err := conn.Read(p)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+
+	// We can't see the compressed status given the DNS API, so we just make
+	// sure the message is smaller to see if it's respecting the flag.
+	if compressed == 0 || unc == 0 || compressed >= unc {
+		t.Fatalf("doesn't look compressed: %d vs. %d", compressed, unc)
+	}
+}

website/source/docs/agent/options.html.markdown

@@ -516,6 +516,10 @@ Consul will not enable TLS for the HTTP API unless the `https` port has been ass
   are considered. For example, if a node has a health check that is critical then all services on
   that node will be excluded because they are also considered critical.
 
+  * <a name="disable_compression"></a><a href="#disable_compression">`disable_compression`</a> If
+  set to true, DNS responses will not be compressed. Compression was added and enabled by default
+  in Consul 0.7.
+
   * <a name="udp_answer_limit"></a><a
   href="#udp_answer_limit">`udp_answer_limit`</a> - Limit the number of
   resource records contained in the answer section of a UDP-based DNS