GetNameservers is inconsistent across providers (#655)

* Warn if GetNameservers returns FQDN+dot strings
* Simplify logic that covers for the inconsistency
* Fix azuredns, gcloud, bind, route53
* Clean up cloudflare, digitalocean, dnsimple, gandi_v5, namedotcom

models/dns.go

@@ -2,6 +2,7 @@ package models
 
 import (
 	"encoding/json"
+	"fmt"
 	"strings"
 )
 
@@ -49,6 +50,7 @@ type DNSProviderConfig struct {
 // Nameserver describes a nameserver.
 type Nameserver struct {
 	Name string `json:"name"` // Normalized to a FQDN with NO trailing "."
+	// NB(tlim): DomainConfig.Nameservers are stored WITH a trailing "." (Sorry!)
 }
 
 func (n *Nameserver) String() string {
@@ -56,6 +58,8 @@ func (n *Nameserver) String() string {
 }
 
 // StringsToNameservers constructs a list of *Nameserver structs using a list of FQDNs.
+// Deprecated. Please use ToNameservers, or maybe ToNameserversStripTD instead.
+// See https://github.com/StackExchange/dnscontrol/issues/491
 func StringsToNameservers(nss []string) []*Nameserver {
 	nservers := []*Nameserver{}
 	for _, ns := range nss {
@@ -64,7 +68,37 @@ func StringsToNameservers(nss []string) []*Nameserver {
 	return nservers
 }
 
-// NameserversToStrings constructs a list of lists from *Nameserver structs
+// ToNameservers turns a list of strings into a list of Nameservers.
+// It is an error if any string has a trailing dot. Either remove the
+// trailing dot before you call this or (much preferred) use ToNameserversStripTD.
+func ToNameservers(nss []string) ([]*Nameserver, error) {
+	nservers := []*Nameserver{}
+	for _, ns := range nss {
+		if strings.HasSuffix(ns, ".") {
+			return nil, fmt.Errorf("provider code leaves trailing dot on nameserver")
+			// If you see this error, maybe the provider should call
+			// ToNameserversStripTD instead.
+		}
+		nservers = append(nservers, &Nameserver{Name: ns})
+	}
+	return nservers, nil
+}
+
+// ToNameserversStripTD is like ToNameservers but strips the trailing
+// dot from each item. It is an error if there is no trailing dot.
+func ToNameserversStripTD(nss []string) ([]*Nameserver, error) {
+	nservers := []*Nameserver{}
+	for _, ns := range nss {
+		if !strings.HasSuffix(ns, ".") {
+			return nil, fmt.Errorf("provider code already removed nameserver trailing dot (%v)", ns)
+			// If you see this error, maybe the provider should call ToNameservers instead.
+		}
+		nservers = append(nservers, &Nameserver{Name: ns[0 : len(ns)-1]})
+	}
+	return nservers, nil
+}
+
+// NameserversToStrings constructs a list of strings from *Nameserver structs
 func NameserversToStrings(nss []*Nameserver) (s []string) {
 	for _, ns := range nss {
 		s = append(s, ns.Name)

models/tdwarn.go

@@ -0,0 +1,13 @@
+package models
+
+import "fmt"
+
+var dotwarned = map[string]bool{}
+
+func WarnNameserverDot(p, w string) {
+	if dotwarned[p] {
+		return
+	}
+	fmt.Printf("Warning: provider %s could be improved. See https://github.com/StackExchange/dnscontrol/issues/491 (%s)\n", p, w)
+	dotwarned[p] = true
+}

models/tonameservers_test.go

@@ -0,0 +1,51 @@
+package models
+
+import (
+	"testing"
+)
+
+func TestToNameservers(t *testing.T) {
+	nss, e := ToNameservers([]string{"example.com", "example2.tld"})
+	if e != nil {
+		t.Errorf("error e %v (%v)", e, nss)
+	}
+	if len(nss) != 2 {
+		t.Errorf("error len: %v", nss)
+	}
+	if nss[0].Name != "example.com" {
+		t.Errorf("error 0: %v", nss[0].Name)
+	}
+	if nss[1].Name != "example2.tld" {
+		t.Errorf("error 1: %v", nss[1].Name)
+	}
+}
+
+func TestToNameservers_neg(t *testing.T) {
+	nss, e := ToNameservers([]string{"example.com.", "example2.tld."})
+	if e == nil {
+		t.Errorf("error 3 (%v)", nss)
+	}
+}
+
+func TestToNameserversStripTD(t *testing.T) {
+	nss, e := ToNameserversStripTD([]string{"example.com.", "example2.tld."})
+	if e != nil {
+		t.Errorf("error e %v (%v)", e, nss)
+	}
+	if len(nss) != 2 {
+		t.Errorf("error len: %v", nss)
+	}
+	if nss[0].Name != "example.com" {
+		t.Errorf("error 0: %v", nss[0].Name)
+	}
+	if nss[1].Name != "example2.tld" {
+		t.Errorf("error 1: %v", nss[1].Name)
+	}
+}
+
+func TestToNameserversStripTD_neg(t *testing.T) {
+	nss, e := ToNameserversStripTD([]string{"example.com", "example2.tld"})
+	if e == nil {
+		t.Errorf("error e (%v)", nss)
+	}
+}

pkg/nameservers/nameservers.go

@@ -26,16 +26,21 @@ func DetermineNameservers(dc *models.DomainConfig) ([]*models.Nameserver, error)
 		if err != nil {
 			return nil, err
 		}
+		// Clean up the nameservers due to
+		// https://github.com/StackExchange/dnscontrol/issues/491
+		// In the far future, this warning will become a fatal error.
+		for i, _ := range nss {
+			if strings.HasSuffix(nss[i].Name, ".") {
+				models.WarnNameserverDot(dnsProvider.Name, fmt.Sprintf("DetermineNameservers (%s) (%s)", dc.Name, nss[i].Name))
+				nss[i].Name = strings.TrimSuffix(nss[i].Name, ".")
+			}
+		}
+
 		take := len(nss)
 		if n > 0 && n < take {
 			take = n
 		}
 		for i := 0; i < take; i++ {
-			nss[i].Name = strings.TrimRight(nss[i].Name, ".")
-			// FIXME(tlim): Rather than correct broken providers, we should print
-			// a warning that the provider should be updated to store the FQDN
-			// with no trailing dot.  See also providers/namedotcom/nameservers.go
-			// Bug https://github.com/StackExchange/dnscontrol/issues/491
 			ns = append(ns, nss[i])
 		}
 	}

pkg/normalize/validate.go

@@ -288,9 +288,14 @@ func NormalizeAndValidateConfig(config *models.DNSConfig) (errs []error) {
 
 		// Normalize Nameservers.
 		for _, ns := range domain.Nameservers {
-			ns.Name = dnsutil.AddOrigin(ns.Name, domain.Name)
-			ns.Name = strings.TrimRight(ns.Name, ".")
+			// NB(tlim): Like any target, NAMESERVER() is input by the user
+			// as a shortname or a FQDN+dot.  It is stored as FQDN+dot.
+			// Normalize it like we do any target to assure it is FQDN+dot
+			ns.Name = dnsutil.AddOrigin(ns.Name, domain.Name+".")
+			ns.Name = strings.TrimSuffix(ns.Name, ".")
+			checkTarget(ns.Name)
 		}
+
 		// Normalize Records.
 		models.PostProcessRecords(domain.Records)
 		for _, rec := range domain.Records {

providers/azuredns/azureDnsProvider.go

@@ -112,13 +112,13 @@ func (a *azureDnsProvider) GetNameservers(domain string) ([]*models.Nameserver,
 		return nil, errNoExist{domain}
 	}
 
-	var ns []*models.Nameserver
+	var nss []string
 	if zone.ZoneProperties != nil {
-		for _, azureNs := range *zone.ZoneProperties.NameServers {
-			ns = append(ns, &models.Nameserver{Name: azureNs})
+		for _, ns := range *zone.ZoneProperties.NameServers {
+			nss = append(nss, ns)
 		}
 	}
-	return ns, nil
+	return models.ToNameserversStripTD(nss)
 }
 
 func (a *azureDnsProvider) ListZones() ([]string, error) {

providers/bind/bindProvider.go

@@ -62,8 +62,13 @@ func initBind(config map[string]string, providermeta json.RawMessage) (providers
 			return nil, err
 		}
 	}
-	api.nameservers = models.StringsToNameservers(api.DefaultNS)
-	return api, nil
+	var nss []string
+	for _, ns := range api.DefaultNS {
+		nss = append(nss, ns[0:len(ns)-1])
+	}
+	var err error
+	api.nameservers, err = models.ToNameservers(nss)
+	return api, err
 }
 
 func init() {

providers/cloudflare/cloudflareProvider.go

@@ -91,7 +91,7 @@ func (c *CloudflareApi) GetNameservers(domain string) ([]*models.Nameserver, err
 	if !ok {
 		return nil, fmt.Errorf("Nameservers for %s not found in cloudflare account", domain)
 	}
-	return models.StringsToNameservers(ns), nil
+	return models.ToNameservers(ns)
 }
 
 func (c *CloudflareApi) ListZones() ([]string, error) {

providers/digitalocean/digitaloceanProvider.go

@@ -93,7 +93,7 @@ func (api *DoApi) EnsureDomainExists(domain string) error {
 
 // GetNameservers returns the nameservers for domain.
 func (api *DoApi) GetNameservers(domain string) ([]*models.Nameserver, error) {
-	return models.StringsToNameservers(defaultNameServerNames), nil
+	return models.ToNameservers(defaultNameServerNames)
 }
 
 // GetZoneRecords gets the records of a zone and returns them in RecordConfig format.

providers/dnsimple/dnsimpleProvider.go

@@ -55,7 +55,7 @@ type DnsimpleApi struct {
 
 // GetNameservers returns the name servers for a domain.
 func (c *DnsimpleApi) GetNameservers(domainName string) ([]*models.Nameserver, error) {
-	return models.StringsToNameservers(defaultNameServerNames), nil
+	return models.ToNameservers(defaultNameServerNames)
 }
 
 // GetZoneRecords gets the records of a zone and returns them in RecordConfig format.

providers/gandi_v5/gandi_v5Provider.go

@@ -310,7 +310,7 @@ func (client *api) GetNameservers(domain string) ([]*models.Nameserver, error) {
 	if err != nil {
 		return nil, err
 	}
-	return models.StringsToNameservers(nameservers), nil
+	return models.ToNameservers(nameservers)
 }
 
 // GetRegistrarCorrections returns a list of corrections for this registrar.

providers/gcloud/gcloudProvider.go

@@ -123,7 +123,7 @@ func (g *gcloud) GetNameservers(domain string) ([]*models.Nameserver, error) {
 	if err != nil {
 		return nil, err
 	}
-	return models.StringsToNameservers(zone.NameServers), nil
+	return models.ToNameserversStripTD(zone.NameServers)
 }
 
 type key struct {

providers/namedotcom/nameservers.go

@@ -28,7 +28,7 @@ func (n *NameCom) GetNameservers(domain string) ([]*models.Nameserver, error) {
 			toUse[idx] = matches[0]
 		}
 	}
-	return models.StringsToNameservers(toUse), nil
+	return models.ToNameservers(toUse)
 }
 
 func (n *NameCom) getNameserversRaw(domain string) ([]string, error) {
@@ -55,9 +55,6 @@ func (n *NameCom) GetRegistrarCorrections(dc *models.DomainConfig) ([]*models.Co
 	expected := []string{}
 	for _, ns := range dc.Nameservers {
 		expected = append(expected, ns.Name)
-		// FIXME(tlim): This should store a FQDN with no trailing ".".
-		// See pkg/nameservers/nameservers.go for details.
-		// Bug https://github.com/StackExchange/dnscontrol/issues/491
 	}
 	sort.Strings(expected)
 	expectedNameservers := strings.Join(expected, ",")

providers/route53/route53Provider.go

@@ -172,13 +172,14 @@ func (r *route53Provider) GetNameservers(domain string) ([]*models.Nameserver, e
 	if err != nil {
 		return nil, err
 	}
-	ns := []*models.Nameserver{}
+
+	var nss []string
 	if z.DelegationSet != nil {
 		for _, nsPtr := range z.DelegationSet.NameServers {
-			ns = append(ns, &models.Nameserver{Name: *nsPtr})
+			nss = append(nss, *nsPtr)
 		}
 	}
-	return ns, nil
+	return models.ToNameservers(nss)
 }
 
 // GetZoneRecords gets the records of a zone and returns them in RecordConfig format.