CLOUDFLAREAPI: Support Punycode for CF_REDIRECT/CF_TEMP_REDIRECT (with tests) (#1026)

* CLOUDFLAREAPI: CF_REDIRECT should support Punycode
* Add tests to CF_*REDIR
* CLOUDFLARE: DS records only permitted on children

integrationTest/integration_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"encoding/json"
 	"flag"
 	"fmt"
 	"os"
@@ -41,7 +42,18 @@ func getProvider(t *testing.T) (providers.DNSServiceProvider, string, map[int]bo
 		if *providerToRun != name {
 			continue
 		}
-		provider, err := providers.CreateDNSProvider(name, cfg, nil)
+
+		var metadata json.RawMessage
+		// CLOUDFLAREAPI tests related to CF_REDIRECT/CF_TEMP_REDIRECT
+		// requires metadata to enable this feature.
+		// In hindsight, I have no idea why this metadata flag is required to
+		// use this feature. Maybe because we didn't have the capabilities
+		// feature at the time?
+		if name == "CLOUDFLAREAPI" {
+			metadata = []byte(`{ "manage_redirects": true }`)
+		}
+
+		provider, err := providers.CreateDNSProvider(name, cfg, metadata)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -54,8 +66,10 @@ func getProvider(t *testing.T) (providers.DNSServiceProvider, string, map[int]bo
 				fails[i] = true
 			}
 		}
+
 		return provider, cfg["domain"], fails, cfg
 	}
+
 	t.Fatalf("Provider %s not found", *providerToRun)
 	return nil, "", nil, nil
 }
@@ -359,6 +373,18 @@ func azureAlias(name, aliasType, target string) *rec {
 	return r
 }
 
+func cfRedir(pattern, target string) *rec {
+	t := fmt.Sprintf("%s,%s", pattern, target)
+	r := makeRec("@", t, "CF_REDIRECT")
+	return r
+}
+
+func cfRedirTemp(pattern, target string) *rec {
+	t := fmt.Sprintf("%s,%s", pattern, target)
+	r := makeRec("@", t, "CF_TEMP_REDIRECT")
+	return r
+}
+
 func ns(name, target string) *rec {
 	return makeRec(name, target, "NS")
 }
@@ -981,7 +1007,7 @@ func makeTests(t *testing.T) []*TestGroup {
 
 		testgroup("DS (children only)",
 			requires(providers.CanUseDSForChildren),
-			not("CLOUDNS"),
+			not("CLOUDNS", "CLOUDFLAREAPI"),
 			// Use a valid digest value here, because GCLOUD (which implements this capability) verifies
 			// the value passed in is a valid digest. RFC 4034, s5.1.4 specifies SHA1 as the only digest
 			// algo at present, i.e. only hexadecimal values currently usable.
@@ -1001,7 +1027,7 @@ func makeTests(t *testing.T) []*TestGroup {
 
 		testgroup("DS (children only) CLOUDNS",
 			requires(providers.CanUseDSForChildren),
-			only("CLOUDNS"),
+			only("CLOUDNS", "CLOUDFLAREAPI"),
 			// Use a valid digest value here, because GCLOUD (which implements this capability) verifies
 			// the value passed in is a valid digest. RFC 4034, s5.1.4 specifies SHA1 as the only digest
 			// algo at present, i.e. only hexadecimal values currently usable.
@@ -1146,6 +1172,69 @@ func makeTests(t *testing.T) []*TestGroup {
 				cname("dev-system18", "ec2-54-91-33-155.compute-1.amazonaws.com."),
 			),
 		),
+
+		testgroup("CF_REDIRECT",
+			only("CLOUDFLAREAPI"),
+			tc("redir", cfRedir("cnn.**current-domain-no-trailing**/*", "https://www.cnn.com/$1")),
+			tc("change", cfRedir("cnn.**current-domain-no-trailing**/*", "https://change.cnn.com/$1")),
+			tc("changelabel", cfRedir("cable.**current-domain-no-trailing**/*", "https://change.cnn.com/$1")),
+			clear(),
+			tc("multipleA",
+				cfRedir("cnn.**current-domain-no-trailing**/*", "https://www.cnn.com/$1"),
+				cfRedir("msnbc.**current-domain-no-trailing**/*", "https://msnbc.cnn.com/$1"),
+			),
+			clear(),
+			tc("multipleB",
+				cfRedir("msnbc.**current-domain-no-trailing**/*", "https://msnbc.cnn.com/$1"),
+				cfRedir("cnn.**current-domain-no-trailing**/*", "https://www.cnn.com/$1"),
+			),
+			tc("change1",
+				cfRedir("msnbc.**current-domain-no-trailing**/*", "https://msnbc.cnn.com/$1"),
+				cfRedir("cnn.**current-domain-no-trailing**/*", "https://change.cnn.com/$1"),
+			),
+			tc("change1",
+				cfRedir("msnbc.**current-domain-no-trailing**/*", "https://msnbc.cnn.com/$1"),
+				cfRedir("cablenews.**current-domain-no-trailing**/*", "https://change.cnn.com/$1"),
+			),
+			// TODO(tlim): Fix this test case:
+			//clear(),
+			//tc("multiple3",
+			//	cfRedir("msnbc.**current-domain-no-trailing**/*", "https://msnbc.cnn.com/$1"),
+			//	cfRedir("cnn.**current-domain-no-trailing**/*", "https://www.cnn.com/$1"),
+			//	cfRedir("nytimes.**current-domain-no-trailing**/*", "https://www.nytimes.com/$1"),
+			//),
+
+			// Repeat the above using CF_TEMP_REDIR instead
+			clear(),
+			tc("tempredir", cfRedirTemp("cnn.**current-domain-no-trailing**/*", "https://www.cnn.com/$1")),
+			tc("tempchange", cfRedirTemp("cnn.**current-domain-no-trailing**/*", "https://change.cnn.com/$1")),
+			tc("tempchangelabel", cfRedirTemp("cable.**current-domain-no-trailing**/*", "https://change.cnn.com/$1")),
+			clear(),
+			tc("tempmultipleA",
+				cfRedirTemp("cnn.**current-domain-no-trailing**/*", "https://www.cnn.com/$1"),
+				cfRedirTemp("msnbc.**current-domain-no-trailing**/*", "https://msnbc.cnn.com/$1"),
+			),
+			clear(),
+			tc("tempmultipleB",
+				cfRedirTemp("msnbc.**current-domain-no-trailing**/*", "https://msnbc.cnn.com/$1"),
+				cfRedirTemp("cnn.**current-domain-no-trailing**/*", "https://www.cnn.com/$1"),
+			),
+			tc("tempchange1",
+				cfRedirTemp("msnbc.**current-domain-no-trailing**/*", "https://msnbc.cnn.com/$1"),
+				cfRedirTemp("cnn.**current-domain-no-trailing**/*", "https://change.cnn.com/$1"),
+			),
+			tc("tempchange1",
+				cfRedirTemp("msnbc.**current-domain-no-trailing**/*", "https://msnbc.cnn.com/$1"),
+				cfRedirTemp("cablenews.**current-domain-no-trailing**/*", "https://change.cnn.com/$1"),
+			),
+			// TODO(tlim): Fix this test case:
+			//clear(),
+			//tc("tempmultiple3",
+			//	cfRedirTemp("msnbc.**current-domain-no-trailing**/*", "https://msnbc.cnn.com/$1"),
+			//	cfRedirTemp("cnn.**current-domain-no-trailing**/*", "https://www.cnn.com/$1"),
+			//	cfRedirTemp("nytimes.**current-domain-no-trailing**/*", "https://www.nytimes.com/$1"),
+			//),
+		),
 	}
 
 	return tests

models/domain.go

@@ -67,26 +67,28 @@ func (dc *DomainConfig) Filter(f func(r *RecordConfig) bool) {
 // - Target (CNAME and MX only)
 func (dc *DomainConfig) Punycode() error {
 	for _, rec := range dc.Records {
+		// Update the label:
 		t, err := idna.ToASCII(rec.GetLabelFQDN())
 		if err != nil {
 			return err
 		}
 		rec.SetLabelFromFQDN(t, dc.Name)
+
+		// Set the target:
 		switch rec.Type { // #rtype_variations
 		case "ALIAS", "MX", "NS", "CNAME", "PTR", "SRV", "URL", "URL301", "FRAME", "R53_ALIAS":
 			// These rtypes are hostnames, therefore need to be converted (unlike, for example, an AAAA record)
 			t, err := idna.ToASCII(rec.GetTargetField())
-			rec.SetTarget(t)
 			if err != nil {
 				return err
 			}
+			rec.SetTarget(t)
+		case "CF_REDIRECT", "CF_TEMP_REDIRECT":
+			rec.SetTarget(rec.GetTargetField())
 		case "A", "AAAA", "CAA", "DS", "NAPTR", "SOA", "SSHFP", "TXT", "TLSA", "AZURE_ALIAS":
 			// Nothing to do.
 		default:
-			msg := fmt.Sprintf("Punycode rtype %v unimplemented", rec.Type)
+			return fmt.Errorf("Punycode rtype %v unimplemented", rec.Type)
-			panic(msg)
-			// We panic so that we quickly find any switch statements
-			// that have not been updated for a new RR type.
 		}
 	}
 	return nil

providers/cloudflare/cloudflareProvider.go

@@ -44,7 +44,7 @@ var features = providers.DocumentationNotes{
 	providers.CanUseSRV:              providers.Can(),
 	providers.CanUseTLSA:             providers.Can(),
 	providers.CanUseSSHFP:            providers.Can(),
-	providers.CanUseDS:               providers.Can(),
+	providers.CanUseDSForChildren:    providers.Can(),
 	providers.CanUseTXTMulti:         providers.Can(),
 	providers.DocCreateDomains:       providers.Can(),
 	providers.DocDualHost:            providers.Cannot("Cloudflare will not work well in situations where it is not the only DNS server"),
@@ -177,6 +177,10 @@ func (c *cloudflareProvider) GetDomainCorrections(dc *models.DomainConfig) ([]*m
 
 	if c.manageRedirects {
 		prs, err := c.getPageRules(id, dc.Name)
+		//fmt.Printf("GET PAGE RULES:\n")
+		//for i, p := range prs {
+		//	fmt.Printf("%03d: %q\n", i, p.GetTargetField())
+		//}
 		if err != nil {
 			return nil, err
 		}
@@ -218,9 +222,15 @@ func (c *cloudflareProvider) GetDomainCorrections(dc *models.DomainConfig) ([]*m
 				Msg: d.String(),
 				F:   func() error { return c.deletePageRule(ex.Original.(*pageRule).ID, id) },
 			})
-
 		} else {
-			corrections = append(corrections, c.deleteRec(ex.Original.(*cfRecord), id))
+			corr := c.deleteRec(ex.Original.(*cfRecord), id)
+			// DS records must always have a corresponding NS record.
+			// Therefore, we remove DS records before any NS records.
+			if d.Existing.Type == "DS" {
+				corrections = append([]*models.Correction{corr}, corrections...)
+			} else {
+				corrections = append(corrections, corr)
+			}
 		}
 	}
 	for _, d := range create {
@@ -231,7 +241,14 @@ func (c *cloudflareProvider) GetDomainCorrections(dc *models.DomainConfig) ([]*m
 				F:   func() error { return c.createPageRule(id, des.GetTargetField()) },
 			})
 		} else {
-			corrections = append(corrections, c.createRec(des, id)...)
+			corr := c.createRec(des, id)
+			// DS records must always have a corresponding NS record.
+			// Therefore, we create NS records before any DS records.
+			if d.Desired.Type == "NS" {
+				corrections = append(corr, corrections...)
+			} else {
+				corrections = append(corrections, corr...)
+			}
 		}
 	}