Validation Refactor (#52)

* Cleaning up (and enforcing) validation

* Some more style fixes to validation

* comment

main.go

@@ -90,8 +90,17 @@ func main() {
 	errs := normalize.NormalizeAndValidateConfig(dnsConfig)
 	if len(errs) > 0 {
 		fmt.Printf("%d Validation errors:\n", len(errs))
-		for i, err := range errs {
-			fmt.Printf("%d: %s\n", i+1, err)
+		fatal := false
+		for _, err := range errs {
+			if _, ok := err.(normalize.Warning); ok {
+				fmt.Printf("WARNING: %s\n", err)
+			} else {
+				fatal = true
+				fmt.Printf("ERROR: %s\n", err)
+			}
+		}
+		if fatal {
+			log.Fatal("Exiting due to validation errors")
 		}
 	}
 

normalize/validate.go

@@ -11,30 +11,8 @@ import (
 	"github.com/miekg/dns/dnsutil"
 )
 
-// Returns false if label does not validate.
-func assert_no_enddot(label string) error {
-	if label == "@" {
-		return nil
-	}
-	if len(label) < 1 {
-		return fmt.Errorf("WARNING: null label.")
-	}
-	if label[len(label)-1] == '.' {
-		return fmt.Errorf("WARNING: label (%v) ends with a (.)", label)
-	}
-	return nil
-}
-
-// Returns false if label does not validate.
-func assert_no_underscores(label string) error {
-	if strings.ContainsRune(label, '_') {
-		return fmt.Errorf("WARNING: label (%v) contains an underscore", label)
-	}
-	return nil
-}
-
 // Returns false if target does not validate.
-func assert_valid_ipv4(label string) error {
+func checkIPv4(label string) error {
 	if net.ParseIP(label).To4() == nil {
 		return fmt.Errorf("WARNING: target (%v) is not an IPv4 address", label)
 	}
@@ -42,31 +20,31 @@ func assert_valid_ipv4(label string) error {
 }
 
 // Returns false if target does not validate.
-func assert_valid_ipv6(label string) error {
+func checkIPv6(label string) error {
 	if net.ParseIP(label).To16() == nil {
 		return fmt.Errorf("WARNING: target (%v) is not an IPv6 address", label)
 	}
 	return nil
 }
 
-// assert_valid_cname_target returns 1 if target is not valid for cnames.
-func assert_valid_target(target string) error {
+// make sure target is valid reference for cnames, mx, etc.
+func checkTarget(target string) error {
 	if target == "@" {
 		return nil
 	}
 	if len(target) < 1 {
-		return fmt.Errorf("WARNING: null target.")
+		return fmt.Errorf("empty target")
 	}
 	// If it containts a ".", it must end in a ".".
 	if strings.ContainsRune(target, '.') && target[len(target)-1] != '.' {
-		return fmt.Errorf("WARNING: target (%v) must end with a (.) [Required if target is not single label]", target)
+		return fmt.Errorf("target (%v) must end with a (.) [Required if target is not single label]", target)
 	}
 	return nil
 }
 
 // validateRecordTypes list of valid rec.Type values. Returns true if this is a real DNS record type, false means it is a pseudo-type used internally.
-func validateRecordTypes(rec *models.RecordConfig, domain_name string) error {
-	var valid_types = map[string]bool{
+func validateRecordTypes(rec *models.RecordConfig, domain string) error {
+	var validTypes = map[string]bool{
 		"A":                true,
 		"AAAA":             true,
 		"CNAME":            true,
@@ -76,84 +54,104 @@ func validateRecordTypes(rec *models.RecordConfig, domain_name string) error {
 		"NS":               true,
 	}
 
-	if _, ok := valid_types[rec.Type]; !ok {
-		return fmt.Errorf("Unsupported record type (%v) domain=%v name=%v", rec.Type, domain_name, rec.Name)
+	if _, ok := validTypes[rec.Type]; !ok {
+		return fmt.Errorf("Unsupported record type (%v) domain=%v name=%v", rec.Type, domain, rec.Name)
 	}
 	return nil
 }
 
-// validateTargets returns true if rec.Target is valid for the rec.Type.
-func validateTargets(rec *models.RecordConfig, domain_name string) (errs []error) {
+// underscores in names are often used erroneously. They are valid for dns records, but invalid for urls.
+// here we list common records expected to have underscores. Anything else containing an underscore will print a warning.
+var expectedUnderscores = []string{"_domainkey", "_dmarc"}
+
+func checkLabel(label string, rType string, domain string) error {
+	if label == "@" {
+		return nil
+	}
+	if len(label) < 1 {
+		return fmt.Errorf("empty %s label in %s", rType, domain)
+	}
+	if label[len(label)-1] == '.' {
+		return fmt.Errorf("label %s.%s ends with a (.)", label, domain)
+	}
+
+	//underscores are warnings
+	if strings.ContainsRune(label, '_') {
+		//unless it is in our exclusion list
+		ok := false
+		for _, ex := range expectedUnderscores {
+			if strings.Contains(label, ex) {
+				ok = true
+				break
+			}
+		}
+		if !ok {
+			return Warning{fmt.Errorf("label %s.%s contains an underscore", label, domain)}
+		}
+	}
+	return nil
+}
+
+// checkTargets returns true if rec.Target is valid for the rec.Type.
+func checkTargets(rec *models.RecordConfig, domain string) (errs []error) {
 	label := rec.Name
 	target := rec.Target
 	check := func(e error) {
 		if e != nil {
-			errs = append(errs, e)
+			err := fmt.Errorf("In %s %s.%s: %s", rec.Type, rec.Name, domain, e.Error())
+			if _, ok := e.(Warning); ok {
+				err = Warning{err}
+			}
+			errs = append(errs, err)
 		}
 	}
-	check(assert_no_enddot(label))
 	switch rec.Type {
 	case "A":
-		check(assert_no_underscores(label))
-		check(assert_valid_ipv4(target))
+		check(checkIPv4(target))
 	case "AAAA":
-		check(assert_no_underscores(label))
-		check(assert_valid_ipv6(target))
+		check(checkIPv6(target))
 	case "CNAME":
-		// NOTE(tlim): In theory a CNAME's label should abide by the assertions
-		// of the record type it points to.
-		// i.e. a If the CNAME target points at an MX record, the label should
-		// be validated as if it is an MX record.
-		// However, that's rather difficult to do, and impossible if the CNAME
-		// points at a record outside of dnscontrol's control.  Therefore we do
-		// simple hacks to guess the target type.
-		// Assumption: If the label contains "._domainkey", this is an TXT.
-		check(assert_valid_target(target))
-		if !strings.Contains(label, "._domainkey") {
-			check(assert_no_underscores(label))
-		}
+		check(checkTarget(target))
 	case "MX":
-		check(assert_no_underscores(label))
-		check(assert_valid_target(target))
+		check(checkTarget(target))
 	case "NS":
-		check(assert_no_underscores(label))
-		check(assert_valid_target(target))
+		check(checkTarget(target))
 		if label == "@" {
 			check(fmt.Errorf("cannot create NS record for bare domain. Use NAMESERVER instead"))
 		}
 	case "TXT", "IMPORT_TRANSFORM":
 	default:
 		errs = append(errs, fmt.Errorf("Unimplemented record type (%v) domain=%v name=%v",
-			rec.Type, domain_name, rec.Name))
+			rec.Type, domain, rec.Name))
 	}
 	return
 }
 
-func transform_cname(target, old_domain, new_domain string) string {
-	// Canonicalize. If it isn't a FQDN, add the new_domain.
-	result := dnsutil.AddOrigin(target, old_domain)
+func transformCNAME(target, oldDomain, newDomain string) string {
+	// Canonicalize. If it isn't a FQDN, add the newDomain.
+	result := dnsutil.AddOrigin(target, oldDomain)
 	if dns.IsFqdn(result) {
 		result = result[:len(result)-1]
 	}
-	return dnsutil.AddOrigin(result, new_domain) + "."
+	return dnsutil.AddOrigin(result, newDomain) + "."
 }
 
 // import_transform imports the records of one zone into another, modifying records along the way.
-func import_transform(src_domain, dst_domain *models.DomainConfig, transforms []transform.IpConversion, ttl uint32) error {
-	// Read src_domain.Records, transform, and append to dst_domain.Records:
+func importTransform(srcDomain, dstDomain *models.DomainConfig, transforms []transform.IpConversion, ttl uint32) error {
+	// Read srcDomain.Records, transform, and append to dstDomain.Records:
 	// 1. Skip any that aren't A or CNAMEs.
-	// 2. Append dest_domainname to the end of the label.
-	// 3. For CNAMEs, append dest_domainname to the end of the target.
+	// 2. Append destDomainname to the end of the label.
+	// 3. For CNAMEs, append destDomainname to the end of the target.
 	// 4. For As, change the target as described the transforms.
 
-	for _, rec := range src_domain.Records {
-		if dst_domain.HasRecordTypeName(rec.Type, rec.NameFQDN) {
+	for _, rec := range srcDomain.Records {
+		if dstDomain.HasRecordTypeName(rec.Type, rec.NameFQDN) {
 			continue
 		}
 		newRec := func() *models.RecordConfig {
 			rec2, _ := rec.Copy()
 			rec2.Name = rec2.NameFQDN
-			rec2.NameFQDN = dnsutil.AddOrigin(rec2.Name, dst_domain.Name)
+			rec2.NameFQDN = dnsutil.AddOrigin(rec2.Name, dstDomain.Name)
 			if ttl != 0 {
 				rec2.TTL = ttl
 			}
@@ -168,12 +166,12 @@ func import_transform(src_domain, dst_domain *models.DomainConfig, transforms []
 			for _, tr := range trs {
 				r := newRec()
 				r.Target = tr.String()
-				dst_domain.Records = append(dst_domain.Records, r)
+				dstDomain.Records = append(dstDomain.Records, r)
 			}
 		case "CNAME":
 			r := newRec()
-			r.Target = transform_cname(r.Target, src_domain.Name, dst_domain.Name)
-			dst_domain.Records = append(dst_domain.Records, r)
+			r.Target = transformCNAME(r.Target, srcDomain.Name, dstDomain.Name)
+			dstDomain.Records = append(dstDomain.Records, r)
 		case "MX", "NS", "TXT":
 			// Not imported.
 			continue
@@ -195,12 +193,15 @@ func deleteImportTransformRecords(domain *models.DomainConfig) {
 	}
 }
 
-func NormalizeAndValidateConfig(config *models.DNSConfig) (errs []error) {
-	// TODO(tlim): Before any changes are made, we should check the rules
-	// such as MX/CNAME/NS .Target must be a single word, "@", or FQDN.
-	// Validate and normalize
-	for _, domain := range config.Domains {
+// Warning is a wrapper around error that can be used to indicate it should not
+// stop execution, but is still likely a problem.
+type Warning struct {
+	error
+}
 
+func NormalizeAndValidateConfig(config *models.DNSConfig) (errs []error) {
+
+	for _, domain := range config.Domains {
 		// Normalize Nameservers.
 		for _, ns := range domain.Nameservers {
 			ns.Name = dnsutil.AddOrigin(ns.Name, domain.Name)
@@ -216,7 +217,10 @@ func NormalizeAndValidateConfig(config *models.DNSConfig) (errs []error) {
 			if err := validateRecordTypes(rec, domain.Name); err != nil {
 				errs = append(errs, err)
 			}
-			if errs2 := validateTargets(rec, domain.Name); errs2 != nil {
+			if err := checkLabel(rec.Name, rec.Type, domain.Name); err != nil {
+				errs = append(errs, err)
+			}
+			if errs2 := checkTargets(rec, domain.Name); errs2 != nil {
 				errs = append(errs, errs2...)
 			}
 
@@ -238,7 +242,7 @@ func NormalizeAndValidateConfig(config *models.DNSConfig) (errs []error) {
 					errs = append(errs, err)
 					continue
 				}
-				err = import_transform(config.FindDomain(rec.Target), domain, table, rec.TTL)
+				err = importTransform(config.FindDomain(rec.Target), domain, table, rec.TTL)
 				if err != nil {
 					errs = append(errs, err)
 				}
@@ -256,29 +260,6 @@ func NormalizeAndValidateConfig(config *models.DNSConfig) (errs []error) {
 			errs = append(errs, err)
 		}
 	}
-
-	// Verify all labels are FQDN ending with ".":
-	for _, domain := range config.Domains {
-		for _, rec := range domain.Records {
-			// .Name must NOT end in "."
-			if rec.Name[len(rec.Name)-1] == '.' {
-				errs = append(errs, fmt.Errorf("Should not happen: Label ends with '.': %v %v %v %v %v",
-					domain.Name, rec.Name, rec.NameFQDN, rec.Type, rec.Target))
-			}
-			// .NameFQDN must NOT end in "."
-			if rec.NameFQDN[len(rec.NameFQDN)-1] == '.' {
-				errs = append(errs, fmt.Errorf("Should not happen: FQDN ends with '.': %v %v %v %v %v",
-					domain.Name, rec.Name, rec.NameFQDN, rec.Type, rec.Target))
-			}
-			// .Target MUST end in "."
-			if rec.Type == "CNAME" || rec.Type == "NS" || rec.Type == "MX" {
-				if rec.Target[len(rec.Target)-1] != '.' {
-					errs = append(errs, fmt.Errorf("Should not happen: Target does NOT ends with '.': %v %v %v %v %v",
-						domain.Name, rec.Name, rec.NameFQDN, rec.Type, rec.Target))
-				}
-			}
-		}
-	}
 	return errs
 }
 

normalize/validate_test.go

@@ -6,7 +6,7 @@ import (
 	"github.com/StackExchange/dnscontrol/models"
 )
 
-func Test_assert_no_enddot(t *testing.T) {
+func TestCheckLabel(t *testing.T) {
 	var tests = []struct {
 		experiment string
 		isError    bool
@@ -16,10 +16,12 @@ func Test_assert_no_enddot(t *testing.T) {
 		{"foo.bar", false},
 		{"foo.", true},
 		{"foo.bar.", true},
+		{"foo_bar", true},
+		{"_domainkey", false},
 	}
 
 	for _, test := range tests {
-		err := assert_no_enddot(test.experiment)
+		err := checkLabel(test.experiment, "A", "foo.com")
 		checkError(t, err, test.isError, test.experiment)
 	}
 }
@@ -33,24 +35,6 @@ func checkError(t *testing.T, err error, shouldError bool, experiment string) {
 	}
 }
 
-func Test_assert_no_underscores(t *testing.T) {
-	var tests = []struct {
-		experiment string
-		isError    bool
-	}{
-		{"@", false},
-		{"foo", false},
-		{"_foo", true},
-		{"foo_", true},
-		{"fo_o", true},
-	}
-
-	for _, test := range tests {
-		err := assert_no_underscores(test.experiment)
-		checkError(t, err, test.isError, test.experiment)
-	}
-}
-
 func Test_assert_valid_ipv4(t *testing.T) {
 	var tests = []struct {
 		experiment string
@@ -63,7 +47,7 @@ func Test_assert_valid_ipv4(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		err := assert_valid_ipv4(test.experiment)
+		err := checkIPv4(test.experiment)
 		checkError(t, err, test.isError, test.experiment)
 	}
 }
@@ -81,7 +65,7 @@ func Test_assert_valid_target(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		err := assert_valid_target(test.experiment)
+		err := checkTarget(test.experiment)
 		checkError(t, err, test.isError, test.experiment)
 	}
 }
@@ -99,7 +83,7 @@ func Test_transform_cname(t *testing.T) {
 	}
 
 	for _, test := range tests {
-		actual := transform_cname(test.experiment, "old.com", "new.com")
+		actual := transformCNAME(test.experiment, "old.com", "new.com")
 		if test.expected != actual {
 			t.Errorf("%v: expected (%v) got (%v)\n", test.experiment, test.expected, actual)
 		}
@@ -109,12 +93,12 @@ func Test_transform_cname(t *testing.T) {
 func TestNSAtRoot(t *testing.T) {
 	//do not allow ns records for @
 	rec := &models.RecordConfig{Name: "test", Type: "NS", Target: "ns1.name.com."}
-	errs := validateTargets(rec, "foo.com")
+	errs := checkTargets(rec, "foo.com")
 	if len(errs) > 0 {
 		t.Error("Expect no error with ns record on subdomain")
 	}
 	rec.Name = "@"
-	errs = validateTargets(rec, "foo.com")
+	errs = checkTargets(rec, "foo.com")
 	if len(errs) != 1 {
 		t.Error("Expect error with ns record on @")
 	}