plugin/forward: health_check needs to normalize a specified domain name (#5543)

* plugin/forward: convert the specified domain of health_check to Fqdn
* plugin/forward: update readme for health check

Signed-off-by: vanceli <vanceli@tencent.com>

3 files changed, 19 insertions, 8 deletions

diff --git a/plugin/forward/README.md b/plugin/forward/README.md
index 5cc85b409..0088c9c7c 100644
--- a/plugin/forward/README.md
+++ b/plugin/forward/README.md
@@ -48,7 +48,7 @@ forward FROM TO... {
     tls CERT KEY CA
     tls_servername NAME
     policy random|round_robin|sequential
-    health_check DURATION [no_rec] [domain DOMAIN]
+    health_check DURATION [no_rec] [domain FQDN]
     max_concurrent MAX
 }
 ~~~
@@ -88,8 +88,8 @@ forward FROM TO... {
   * `<duration>` - use a different duration for health checking, the default duration is 0.5s.
   * `no_rec` - optional argument that sets the RecursionDesired-flag of the dns-query used in health checking to `false`.
     The flag is default `true`.
-  * `domain DOMAIN` - optional arguments that sets the domain of the dns-query used in health checking.
-    If not configured, the requested domain name is `.`. `DOMAIN` is used to configure the domain name.
+  * `domain FQDN` - set the domain name used for health checks to **FQDN**.
+    If not configured, the domain name used for health checks is `.`.
 * `max_concurrent` **MAX** will limit the number of concurrent queries to **MAX**.  Any new query that would
   raise the number of concurrent queries above the **MAX** will result in a REFUSED response. This
   response does not count as a health failure. When choosing a value for **MAX**, pick a number
diff --git a/plugin/forward/setup.go b/plugin/forward/setup.go
index 0e317bf9f..dfae70d37 100644
--- a/plugin/forward/setup.go
+++ b/plugin/forward/setup.go
@@ -14,6 +14,8 @@ import (
 	"github.com/coredns/coredns/plugin/pkg/parse"
 	pkgtls "github.com/coredns/coredns/plugin/pkg/tls"
 	"github.com/coredns/coredns/plugin/pkg/transport"
+
+	"github.com/miekg/dns"
 )
 
 func init() { plugin.Register("forward", setup) }
@@ -204,7 +206,11 @@ func parseBlock(c *caddy.Controller, f *Forward) error {
 				if !c.NextArg() {
 					return c.ArgErr()
 				}
-				f.opts.hcDomain = c.Val()
+				hcDomain := c.Val()
+				if _, ok := dns.IsDomainName(hcDomain); !ok {
+					return fmt.Errorf("health_check: invalid domain name %s", hcDomain)
+				}
+				f.opts.hcDomain = plugin.Name(hcDomain).Normalize()
 			default:
 				return fmt.Errorf("health_check: unknown option %s", hcOpts)
 			}
diff --git a/plugin/forward/setup_test.go b/plugin/forward/setup_test.go
index 829e1f745..4b1743098 100644
--- a/plugin/forward/setup_test.go
+++ b/plugin/forward/setup_test.go
@@ -8,6 +8,8 @@ import (
 
 	"github.com/coredns/caddy"
 	"github.com/coredns/coredns/core/dnsserver"
+
+	"github.com/miekg/dns"
 )
 
 func TestSetup(t *testing.T) {
@@ -22,7 +24,7 @@ func TestSetup(t *testing.T) {
 	}{
 		// positive
 		{"forward . 127.0.0.1", false, ".", nil, 2, options{hcRecursionDesired: true, hcDomain: "."}, ""},
-		{"forward . 127.0.0.1 {\nhealth_check 0.5s domain example.org\n}\n", false, ".", nil, 2, options{hcRecursionDesired: true, hcDomain: "example.org"}, ""},
+		{"forward . 127.0.0.1 {\nhealth_check 0.5s domain example.org\n}\n", false, ".", nil, 2, options{hcRecursionDesired: true, hcDomain: "example.org."}, ""},
 		{"forward . 127.0.0.1 {\nexcept miek.nl\n}\n", false, ".", nil, 2, options{hcRecursionDesired: true, hcDomain: "."}, ""},
 		{"forward . 127.0.0.1 {\nmax_fails 3\n}\n", false, ".", nil, 3, options{hcRecursionDesired: true, hcDomain: "."}, ""},
 		{"forward . 127.0.0.1 {\nforce_tcp\n}\n", false, ".", nil, 2, options{forceTCP: true, hcRecursionDesired: true, hcDomain: "."}, ""},
@@ -243,13 +245,16 @@ func TestSetupHealthCheck(t *testing.T) {
 		{"forward . 127.0.0.1\n", false, true, ".", ""},
 		{"forward . 127.0.0.1 {\nhealth_check 0.5s\n}\n", false, true, ".", ""},
 		{"forward . 127.0.0.1 {\nhealth_check 0.5s no_rec\n}\n", false, false, ".", ""},
-		{"forward . 127.0.0.1 {\nhealth_check 0.5s no_rec domain example.org\n}\n", false, false, "example.org", ""},
-		{"forward . 127.0.0.1 {\nhealth_check 0.5s domain example.org\n}\n", false, true, "example.org", ""},
+		{"forward . 127.0.0.1 {\nhealth_check 0.5s no_rec domain example.org\n}\n", false, false, "example.org.", ""},
+		{"forward . 127.0.0.1 {\nhealth_check 0.5s domain example.org\n}\n", false, true, "example.org.", ""},
+		{"forward . 127.0.0.1 {\nhealth_check 0.5s domain .\n}\n", false, true, ".", ""},
+		{"forward . 127.0.0.1 {\nhealth_check 0.5s domain example.org.\n}\n", false, true, "example.org.", ""},
 		// negative
 		{"forward . 127.0.0.1 {\nhealth_check no_rec\n}\n", true, true, ".", "time: invalid duration"},
 		{"forward . 127.0.0.1 {\nhealth_check domain example.org\n}\n", true, true, "example.org", "time: invalid duration"},
 		{"forward . 127.0.0.1 {\nhealth_check 0.5s rec\n}\n", true, true, ".", "health_check: unknown option rec"},
 		{"forward . 127.0.0.1 {\nhealth_check 0.5s domain\n}\n", true, true, ".", "Wrong argument count or unexpected line ending after 'domain'"},
+		{"forward . 127.0.0.1 {\nhealth_check 0.5s domain example..org\n}\n", true, true, ".", "health_check: invalid domain name"},
 	}
 
 	for i, test := range tests {
@@ -275,7 +280,7 @@ func TestSetupHealthCheck(t *testing.T) {
 
 		f := fs[0]
 		if f.opts.hcRecursionDesired != test.expectedRecVal || f.proxies[0].health.GetRecursionDesired() != test.expectedRecVal ||
-			f.opts.hcDomain != test.expectedDomain || f.proxies[0].health.GetDomain() != test.expectedDomain {
+			f.opts.hcDomain != test.expectedDomain || f.proxies[0].health.GetDomain() != test.expectedDomain || !dns.IsFqdn(f.proxies[0].health.GetDomain()) {
 			t.Errorf("Test %d: expectedRec: %v, got: %v. expectedDomain: %s, got: %s. ", i, test.expectedRecVal, f.opts.hcRecursionDesired, test.expectedDomain, f.opts.hcDomain)
 		}
 	}