Merge branch 'master' into 1920-client-find

dnsforward/config.go

@@ -249,6 +249,7 @@ func (s *Server) prepareUpstreamSettings() error {
 	} else {
 		upstreams = s.conf.UpstreamDNS
 	}
+	upstreams = filterOutComments(upstreams)
 	upstreamConfig, err := proxy.ParseUpstreamsConfig(upstreams, s.conf.BootstrapDNS, DefaultTimeout)
 	if err != nil {
 		return fmt.Errorf("DNS: proxy.ParseUpstreamsConfig: %s", err)

dnsforward/dnsforward_http.go

@@ -117,12 +117,10 @@ func (s *Server) handleSetConfig(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if js.Exists("upstream_dns") {
-		if len(req.Upstreams) != 0 {
-			err = ValidateUpstreams(req.Upstreams)
-			if err != nil {
-				httpError(r, w, http.StatusBadRequest, "wrong upstreams specification: %s", err)
-				return
-			}
+		err = ValidateUpstreams(req.Upstreams)
+		if err != nil {
+			httpError(r, w, http.StatusBadRequest, "wrong upstreams specification: %s", err)
+			return
 		}
 	}
 
@@ -256,6 +254,14 @@ type upstreamJSON struct {
 
 // ValidateUpstreams validates each upstream and returns an error if any upstream is invalid or if there are no default upstreams specified
 func ValidateUpstreams(upstreams []string) error {
+	// No need to validate comments
+	upstreams = filterOutComments(upstreams)
+
+	// Consider this case valid because defaultDNS will be used
+	if len(upstreams) == 0 {
+		return nil
+	}
+
 	var defaultUpstreamFound bool
 	for _, u := range upstreams {
 		d, err := validateUpstream(u)
@@ -397,6 +403,10 @@ func (s *Server) handleTestUpstreamDNS(w http.ResponseWriter, r *http.Request) {
 }
 
 func checkDNS(input string, bootstrap []string) error {
+	if !isUpstream(input) {
+		return nil
+	}
+
 	// separate upstream from domains list
 	input, defaultUpstream, err := separateUpstream(input)
 	if err != nil {
@@ -404,7 +414,7 @@ func checkDNS(input string, bootstrap []string) error {
 	}
 
 	// No need to check this DNS server
-	if input == "#" || !defaultUpstream {
+	if !defaultUpstream {
 		return nil
 	}
 

dnsforward/dnsforward_test.go

@@ -961,31 +961,35 @@ func TestValidateUpstream(t *testing.T) {
 }
 
 func TestValidateUpstreamsSet(t *testing.T) {
+	// Empty upstreams array
+	var upstreamsSet []string
+	err := ValidateUpstreams(upstreamsSet)
+	assert.Nil(t, err, "empty upstreams array should be valid")
+
+	// Comment in upstreams array
+	upstreamsSet = []string{"# comment"}
+	err = ValidateUpstreams(upstreamsSet)
+	assert.Nil(t, err, "comments should not be validated")
+
 	// Set of valid upstreams. There is no default upstream specified
-	upstreamsSet := []string{"[/host.com/]1.1.1.1",
+	upstreamsSet = []string{"[/host.com/]1.1.1.1",
 		"[//]tls://1.1.1.1",
 		"[/www.host.com/]#",
 		"[/host.com/google.com/]8.8.8.8",
 		"[/host/]sdns://AQMAAAAAAAAAFDE3Ni4xMDMuMTMwLjEzMDo1NDQzINErR_JS3PLCu_iZEIbq95zkSV2LFsigxDIuUso_OQhzIjIuZG5zY3J5cHQuZGVmYXVsdC5uczEuYWRndWFyZC5jb20",
 	}
-	err := ValidateUpstreams(upstreamsSet)
-	if err == nil {
-		t.Fatalf("there is no default upstream")
-	}
+	err = ValidateUpstreams(upstreamsSet)
+	assert.NotNil(t, err, "there is no default upstream")
 
 	// Let's add default upstream
 	upstreamsSet = append(upstreamsSet, "8.8.8.8")
 	err = ValidateUpstreams(upstreamsSet)
-	if err != nil {
-		t.Fatalf("upstreams set is valid, but doesn't pass through validation cause: %s", err)
-	}
+	assert.Nilf(t, err, "upstreams set is valid, but doesn't pass through validation cause: %s", err)
 
 	// Let's add invalid upstream
 	upstreamsSet = append(upstreamsSet, "dhcp://fake.dns")
 	err = ValidateUpstreams(upstreamsSet)
-	if err == nil {
-		t.Fatalf("there is an invalid upstream in set, but it pass through validation")
-	}
+	assert.NotNil(t, err, "there is an invalid upstream in set, but it pass through validation")
 }
 
 func TestIpFromAddr(t *testing.T) {

dnsforward/util.go

@@ -85,3 +85,18 @@ func matchDNSName(dnsNames []string, sni string) bool {
 	}
 	return false
 }
+
+// Is not comment
+func isUpstream(line string) bool {
+	return !strings.HasPrefix(line, "#")
+}
+
+func filterOutComments(lines []string) []string {
+	var filtered []string
+	for _, l := range lines {
+		if isUpstream(l) {
+			filtered = append(filtered, l)
+		}
+	}
+	return filtered
+}