Improve tagged domain handling in support of Split Horizon feature (#3444)

Co-authored-by: Tom Limoncelli <tlimoncelli@stackoverflow.com>

commands/commands.go

@@ -304,13 +304,37 @@ func (args *FilterArgs) flags() []cli.Flag {
 	}
 }
 
+// domainInList takes a domain and a list of domains and returns true if the
+// domain is in the list, accounting for wildcards and tags.
 func domainInList(domain string, list []string) bool {
 	for _, item := range list {
+		if item == domain {
+			return true
+		}
 		if strings.HasPrefix(item, "*") && strings.HasSuffix(domain, item[1:]) {
 			return true
 		}
-		if item == domain {
-			return true
+		filterDom, filterTag, isFilterTagged := strings.Cut(item, "!")
+		splitDom, domainTag, isDomainTagged := strings.Cut(domain, "!")
+		if splitDom == filterDom {
+			if isDomainTagged {
+				if filterTag == "*" {
+					return true
+				}
+				if domainTag == "" && !isFilterTagged {
+					// domain example.com! == filter example.com
+					return true
+				}
+				if isFilterTagged && domainTag == filterTag {
+					return true
+				}
+			}
+			if isFilterTagged {
+				if filterTag == "" && !isDomainTagged {
+					// filter example.com! == domain example.com
+					return true
+				}
+			}
 		}
 	}
 	return false

commands/commands_test.go

@@ -52,6 +52,70 @@ func Test_domainInList(t *testing.T) {
 			},
 			want: false,
 		},
+		{
+			name: "tagged",
+			args: args{
+				domain: "foo.com!bar",
+				list:   []string{"foo.com"},
+			},
+			want: false,
+		},
+		{
+			name: "taggedWildcard",
+			args: args{
+				domain: "foo.com!bar",
+				list:   []string{"foo.com!*"},
+			},
+			want: true,
+		},
+		{
+			name: "taggedWildcardMatchesEmpty",
+			args: args{
+				domain: "foo.com!",
+				list:   []string{"foo.com!*"},
+			},
+			want: true,
+		},
+		{
+			name: "taggedWildcardNotMatchUntagged",
+			args: args{
+				domain: "foo.com",
+				list:   []string{"foo.com!*"},
+			},
+			want: false,
+		},
+		{
+			name: "taggedEmtpy",
+			args: args{
+				domain: "foo.com",
+				list:   []string{"foo.com!"},
+			},
+			want: true,
+		},
+		{
+			name: "domainTaggedEmtpy",
+			args: args{
+				domain: "foo.com!",
+				list:   []string{"foo.com"},
+			},
+			want: true,
+		},
+		{
+			name: "filterTaggedNoMatch",
+			args: args{
+				domain: "foo.com",
+				list:   []string{"foo.com!foo"},
+			},
+			want: false,
+		},
+		{
+			name: "domainTaggedNoMatch",
+			args: args{
+				domain: "foo.com!foo",
+				list:   []string{"foo.com"},
+			},
+			want: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

commands/ppreviewPush.go

@@ -346,6 +346,10 @@ func prun(args PPreviewArgs, push bool, interactive bool, out printer.CLI, repor
 //	return r
 //}
 
+// whichZonesToProcess takes a list of DomainConfigs and a filter string and
+// returns a list of DomainConfigs whose metadata[DomainUniqueName] matched the
+// filter. The filter string is a comma-separated list of domain names. If the
+// filter string is empty or "all", all domains are returned.
 func whichZonesToProcess(domains []*models.DomainConfig, filter string) []*models.DomainConfig {
 	if filter == "" || filter == "all" {
 		return domains
@@ -354,7 +358,7 @@ func whichZonesToProcess(domains []*models.DomainConfig, filter string) []*model
 	permitList := strings.Split(filter, ",")
 	var picked []*models.DomainConfig
 	for _, domain := range domains {
-		if domainInList(domain.Name, permitList) {
+		if domainInList(domain.GetUniqueName(), permitList) {
 			picked = append(picked, domain)
 		}
 	}

commands/ppreviewPush_test.go

@@ -0,0 +1,180 @@
+package commands
+
+import (
+	"testing"
+
+	"github.com/StackExchange/dnscontrol/v4/models"
+)
+
+func Test_whichZonesToProcess(t *testing.T) {
+
+	dcNoTag := &models.DomainConfig{Name: "example.com"}
+	dcNoTag2 := &models.DomainConfig{Name: "example.net"}
+	dcTaggedEmpty := &models.DomainConfig{Name: "example.com!"}
+	dcTaggedGeorge := &models.DomainConfig{Name: "example.com!george"}
+	dcTaggedJohn := &models.DomainConfig{Name: "example.com!john"}
+
+	allDC := []*models.DomainConfig{
+		dcNoTag,
+		dcNoTag2,
+		dcTaggedGeorge,
+		dcTaggedJohn,
+		dcTaggedEmpty,
+	}
+
+	for _, dc := range allDC {
+		dc.UpdateSplitHorizonNames()
+	}
+
+	type args struct {
+		dc     []*models.DomainConfig
+		filter string
+	}
+
+	tests := []struct {
+		name string
+		why  string
+		args args
+		want []*models.DomainConfig
+	}{
+		{
+			name: "testAllFilter",
+			why:  "Should return all domain configs",
+			args: args{
+				dc:     allDC,
+				filter: "all",
+			},
+			want: allDC,
+		},
+		{
+			name: "testNoFilter",
+			why:  "Should return all domain configs",
+			args: args{
+				dc:     allDC,
+				filter: "",
+			},
+			want: allDC,
+		},
+		{
+			name: "testFilterTagged",
+			why:  "Should return one tagged domain",
+			args: args{
+				dc:     allDC,
+				filter: "example.com!george",
+			},
+			want: []*models.DomainConfig{dcTaggedGeorge},
+		},
+		{
+			name: "testMultiFilterTagged",
+			why:  "Should return two tagged domains",
+			args: args{
+				dc:     allDC,
+				filter: "example.com!george,example.com!john",
+			},
+			want: []*models.DomainConfig{dcTaggedGeorge, dcTaggedJohn},
+		},
+		{
+			name: "testMultiFilterTaggedNoMatch",
+			why:  "Should return nothing",
+			args: args{
+				dc:     allDC,
+				filter: "example.com!ringo",
+			},
+			want: []*models.DomainConfig{},
+		},
+		{
+			name: "testMultiFilterTaggedWildcard",
+			why:  "Should return all matching tagged domains",
+			args: args{
+				dc:     allDC,
+				filter: "example.com!*",
+			},
+			want: []*models.DomainConfig{dcTaggedGeorge, dcTaggedJohn},
+		},
+		{
+			name: "testFilterNoTag",
+			why:  "Should return untagged and empty tagged domain",
+			args: args{
+				dc:     allDC,
+				filter: "example.com",
+			},
+			want: []*models.DomainConfig{dcNoTag, dcTaggedEmpty},
+		},
+		{
+			name: "testFilterEmptyTag",
+			why:  "Should return untagged and empty tagged domain",
+			args: args{
+				dc:     allDC,
+				filter: "example.com!",
+			},
+			want: []*models.DomainConfig{dcNoTag, dcTaggedEmpty},
+		},
+		{
+			name: "testFilterEmptyTagAndNoTag",
+			why:  "Should return untagged and empty tagged domain",
+			args: args{
+				dc:     allDC,
+				filter: "example.com!,example.com",
+			},
+			want: []*models.DomainConfig{dcNoTag, dcTaggedEmpty},
+		},
+		{
+			name: "testFilterNoTagTagged",
+			why:  "Should return the tagged and untagged domains",
+			args: args{
+				dc:     allDC,
+				filter: "example.com!george,example.com",
+			},
+			want: []*models.DomainConfig{dcTaggedGeorge, dcNoTag, dcTaggedEmpty},
+		},
+		{
+			name: "testFilterDuplicates2",
+			why:  "Should return one untagged domain",
+			args: args{
+				dc:     allDC,
+				filter: "example.net,example.net",
+			},
+			want: []*models.DomainConfig{dcNoTag2},
+		},
+		{
+			name: "testFilterNoTagNoMatch",
+			why:  "Should return nothing",
+			args: args{
+				dc:     []*models.DomainConfig{dcTaggedGeorge, dcTaggedJohn},
+				filter: "example.com",
+			},
+			want: []*models.DomainConfig{},
+		},
+		{
+			name: "testFilterTaggedNoMatch",
+			why:  "Should return nothing",
+			args: args{
+				dc:     []*models.DomainConfig{dcNoTag},
+				filter: "example.com!george",
+			},
+			want: []*models.DomainConfig{},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := whichZonesToProcess(tt.args.dc, tt.args.filter)
+			if len(got) != len(tt.want) {
+				t.Errorf("whichZonesToProcess() %s: %s", tt.name, tt.why)
+				for i := range got {
+					t.Errorf("got[%d]: %s", i, got[i].GetUniqueName())
+				}
+				for i := range tt.want {
+					t.Errorf("want[%d]: %s", i, tt.want[i].GetUniqueName())
+				}
+				return
+			}
+			for i := range got {
+				if got[i].Name != tt.want[i].Name {
+					t.Errorf("whichZonesToProcess() %s: %s", tt.name, tt.why)
+					return
+				}
+			}
+		})
+	}
+}

commands/types/dnscontrol.d.ts

@@ -690,11 +690,12 @@ declare function CNAME(name: string, target: string, ...modifiers: RecordModifie
  * six months? You get the idea.
  *
  * DNSControl command line flag `--domains` matches the full name (with the "!").  If you
- * define domains `example.com!george` and `example.com!john` then:
+ * define domains `example.com!john`, `example.com!paul`, and `example.com!george` then:
  *
- * * `--domains=example.com` will not match either domain.
- * * `--domains='example.com!george'` will match only match the first.
- * * `--domains='example.com!george,example.com!john'` will match both.
+ * * `--domains=example.com` will not match any of the three.
+ * * `--domains='example.com!george'` will only match george.
+ * * `--domains='example.com!george,example.com!john'` will match george and john.
+ * * `--domains='example.com!*'` will match all three.
  *
  * NOTE: The quotes are required if your shell treats `!` as a special
  * character, which is probably does.  If you see an error that mentions

documentation/language-reference/top-level-functions/D.md

@@ -86,11 +86,12 @@ may have noticed this mistake, but will your coworkers?  Will you in
 six months? You get the idea.
 
 DNSControl command line flag `--domains` matches the full name (with the "!").  If you
-define domains `example.com!george` and `example.com!john` then:
+define domains `example.com!john`, `example.com!paul`, and `example.com!george` then:
 
-* `--domains=example.com` will not match either domain.
-* `--domains='example.com!george'` will match only match the first.
-* `--domains='example.com!george,example.com!john'` will match both.
+* `--domains=example.com` will not match any of the three.
+* `--domains='example.com!george'` will only match george.
+* `--domains='example.com!george,example.com!john'` will match george and john.
+* `--domains='example.com!*'` will match all three.
 
 {% hint style="info" %}
 **NOTE**: The quotes are required if your shell treats `!` as a special

documentation/preview-push.md

@@ -49,10 +49,19 @@ OPTIONS:
 
 * `--domains value`
   * Specifies a comma-separated list of domains to include.
-    Typically all domains are included in `preview`/`push`. Wildcards are not
-    permitted except `*` at the start of the entry. For example, `--domains
-    example.com,*.in-addr.arpa` would include `example.com` plus all reverse lookup
-    domains.
+    Example: `--domains example.com,myexample.net`
+  * Domains may include a wildcard at the beginning.
+    For example, `--domains example.com,*.in-addr.arpa` would include
+    `example.com` plus all IPv4 reverse lookup domains.
+  * Matching includes tags. If the domains are `example.com!foo` and
+    `example.com!bar`, then `--domains example.com!foo` would match the first
+    one, and `--domains example.com` will not match either.
+  * A wildcard tag is permitted and indicates all configured tags of that domain
+    should be selected. Example: `--domains=example.com!*` would match
+    `example.com!foo` and  `example.com!bar` but not `example.com`.
+  * If `--domains` is not specified, the default is all domains.
+  * NOTE: An empty tag is considered equivalent to the untagged domain.
+    For example, `--domains=example.com!` will match `example.com` and `example.com!`
 
 * `--v foo=bar`
   * Sets the variable `foo` to the value `bar` prior to

models/domain.go

@@ -81,6 +81,10 @@ func (dc *DomainConfig) UpdateSplitHorizonNames() {
 			name = l[0]
 			tag = l[1]
 		}
+		if tag == "" {
+			// ensure empty tagged domain is treated as untagged
+			unique = name
+		}
 	}
 
 	dc.Name = name

models/domain_test.go

@@ -0,0 +1,68 @@
+package models
+
+import (
+	"testing"
+)
+
+func Test_UpdateSplitHorizonNames(t *testing.T) {
+	tests := []struct {
+		name     string
+		dc       *DomainConfig
+		expected *DomainConfig
+	}{
+		{
+			name: "testNoTag",
+			dc: &DomainConfig{
+				Name: "example.com",
+			},
+			expected: &DomainConfig{
+				Name: "example.com",
+				Metadata: map[string]string{
+					DomainUniqueName: "example.com",
+					DomainTag:        "",
+				},
+			},
+		},
+		{
+			name: "testEmptyTag",
+			dc: &DomainConfig{
+				Name: "example.com!",
+			},
+			expected: &DomainConfig{
+				Name: "example.com",
+				Metadata: map[string]string{
+					DomainUniqueName: "example.com",
+					DomainTag:        "",
+				},
+			},
+		},
+		{
+			name: "testWithTag",
+			dc: &DomainConfig{
+				Name: "example.com!john",
+			},
+			expected: &DomainConfig{
+				Name: "example.com",
+				Metadata: map[string]string{
+					DomainUniqueName: "example.com!john",
+					DomainTag:        "john",
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tt.dc.UpdateSplitHorizonNames()
+			if tt.dc.Name != tt.expected.Name {
+				t.Errorf("expected name %s, got %s", tt.expected.Name, tt.dc.Name)
+			}
+			if tt.dc.Metadata[DomainUniqueName] != tt.expected.Metadata[DomainUniqueName] {
+				t.Errorf("expected unique name %s, got %s", tt.expected.Metadata[DomainUniqueName], tt.dc.Metadata[DomainUniqueName])
+			}
+			if tt.dc.Metadata[DomainTag] != tt.expected.Metadata[DomainTag] {
+				t.Errorf("expected tag %s, got %s", tt.expected.Metadata[DomainTag], tt.dc.Metadata[DomainTag])
+			}
+		})
+	}
+}

pkg/js/helpers.js

@@ -148,8 +148,20 @@ function D(name, registrar) {
         var m = arguments[i];
         processDargs(m, domain);
     }
+
+    // handle the empty tag ("example.com!" -> "example.com")
+    // replace name with result of removing the empty tag if it exists
+    // keep track so we can explain the situation in the error message
+    var withoutEmptyTag = _removeEmptyTag(name);
+    name = withoutEmptyTag[0];
+    var tagWasRemoved = withoutEmptyTag[1];
+
     if (conf.domain_names.indexOf(name) !== -1) {
-        throw name + ' is declared more than once';
+        var message = name + ' is declared more than once';
+        if (tagWasRemoved) {
+            message += ' (check empty tags)';
+        }
+        throw message;
     }
     conf.domains.push(domain);
     conf.domain_names.push(name);
@@ -188,15 +200,31 @@ function D_EXTEND(name) {
     conf.domains[domain.id] = domain.obj; // let's overwrite the object.
 }
 
+// _removeEmptyTag(domain): Remove empty tag.
+function _removeEmptyTag(name) {
+    var tagWasRemoved = false;
+    if (name.slice(-1) === '!') {
+        name = name.slice(0, name.length - 1);
+        tagWasRemoved = true;
+    }
+    return [name, tagWasRemoved];
+}
+
 // _getDomainObject(name): This implements the domain matching
 // algorithm used by D_EXTEND(). Candidate matches are an exact match
 // of the domain's name, or if name is a proper subdomain of the
 // domain's name. The longest match is returned.
 function _getDomainObject(name) {
+    var nameTrimmedTag = _removeEmptyTag(name);
+    name = nameTrimmedTag[0];
     var domain = null;
     var domainLen = 0;
     for (var i = 0; i < conf.domains.length; i++) {
         var thisName = conf.domains[i]['name'];
+        // check for empty tag
+        var thisNameTrimmedTag = _removeEmptyTag(thisName);
+        thisName = thisNameTrimmedTag[0];
+
         var desiredSuffix = '.' + thisName;
         var foundSuffix = name.substr(-desiredSuffix.length);
         // If this is an exact match or the suffix matches...

pkg/js/js_test.go

@@ -49,9 +49,9 @@ func TestParsedFiles(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
-			// for _, dc := range conf.Domains {
-			// 	normalize.UpdateNameSplitHorizon(dc)
-			// }
+			for _, dc := range conf.Domains {
+				dc.UpdateSplitHorizonNames()
+			}
 
 			errs := normalize.ValidateAndNormalizeConfig(conf)
 			if len(errs) != 0 {
@@ -114,7 +114,13 @@ func TestParsedFiles(t *testing.T) {
 
 			var dCount int
 			for _, dc := range conf.Domains {
-				zoneFile := filepath.Join(testDir, testName, dc.Name+".zone")
+				var zoneFile string
+				dc.UpdateSplitHorizonNames()
+				if dc.Metadata[models.DomainTag] != "" {
+					zoneFile = filepath.Join(testDir, testName, dc.GetUniqueName()+".zone")
+				} else {
+					zoneFile = filepath.Join(testDir, testName, dc.Name+".zone")
+				}
 				expectedZone, err := os.ReadFile(zoneFile)
 				if err != nil {
 					continue

pkg/js/parse_tests/037-splithor.js

@@ -22,3 +22,37 @@ D_EXTEND("example.com",
 D_EXTEND("example.com!inside",
     A("main", "11.11.11.11"),
 );
+
+D("example.net", REG, DnsProvider(DNS_OUTSIDE),
+    A("www", "203.0.113.1"),
+);
+
+D_EXTEND("example.net!",
+    A("main", "203.0.113.12"),
+);
+
+D("example.net!inside", REG, DnsProvider(DNS_INSIDE),
+    INCLUDE("example.net!"),
+    A("main", "192.0.2.1"),
+);
+
+D("example.net!outside", REG, DnsProvider(DNS_OUTSIDE),
+    INCLUDE("example.net"),
+    A("main", "203.0.113.1"),
+);
+
+D("empty.example.net", REG, DnsProvider(DNS_OUTSIDE),
+    A("www", "203.0.113.2"),
+);
+
+D_EXTEND("empty.example.net!",
+    A("main", "203.0.113.22"),
+);
+
+D("example-b.net!", REG, DnsProvider(DNS_OUTSIDE),
+    A("www", "203.0.113.1"),
+);
+
+D_EXTEND("example-b.net",
+    A("main", "203.0.113.12"),
+);

pkg/js/parse_tests/037-splithor.json

@@ -82,6 +82,143 @@
         }
       ],
       "registrar": "Third-Party"
+    },
+    {
+      "dnsProviders": {
+        "bind": -1
+      },
+      "meta": {
+        "dnscontrol_tag": "",
+        "dnscontrol_uniquename": "example.net"
+      },
+      "name": "example.net",
+      "records": [
+        {
+          "name": "main",
+          "target": "203.0.113.12",
+          "ttl": 300,
+          "type": "A"
+        },
+        {
+          "name": "www",
+          "target": "203.0.113.1",
+          "ttl": 300,
+          "type": "A"
+        }
+      ],
+      "registrar": "Third-Party"
+    },
+    {
+      "dnsProviders": {
+        "Cloudflare": -1
+      },
+      "meta": {
+        "dnscontrol_tag": "inside",
+        "dnscontrol_uniquename": "example.net!inside"
+      },
+      "name": "example.net",
+      "records": [
+        {
+          "name": "main",
+          "target": "192.0.2.1",
+          "ttl": 300,
+          "type": "A"
+        },
+        {
+          "name": "main",
+          "target": "203.0.113.12",
+          "ttl": 300,
+          "type": "A"
+        },
+        {
+          "name": "www",
+          "target": "203.0.113.1",
+          "ttl": 300,
+          "type": "A"
+        }
+      ],
+      "registrar": "Third-Party"
+    },
+    {
+      "dnsProviders": {
+        "bind": -1
+      },
+      "meta": {
+        "dnscontrol_tag": "outside",
+        "dnscontrol_uniquename": "example.net!outside"
+      },
+      "name": "example.net",
+      "records": [
+        {
+          "name": "main",
+          "target": "203.0.113.1",
+          "ttl": 300,
+          "type": "A"
+        },
+        {
+          "name": "main",
+          "target": "203.0.113.12",
+          "ttl": 300,
+          "type": "A"
+        },
+        {
+          "name": "www",
+          "target": "203.0.113.1",
+          "ttl": 300,
+          "type": "A"
+        }
+      ],
+      "registrar": "Third-Party"
+    },
+    {
+      "dnsProviders": {
+        "bind": -1
+      },
+      "meta": {
+        "dnscontrol_tag": "",
+        "dnscontrol_uniquename": "empty.example.net"
+      },
+      "name": "empty.example.net",
+      "records": [
+        {
+          "name": "main",
+          "target": "203.0.113.22",
+          "ttl": 300,
+          "type": "A"
+        },
+        {
+          "name": "www",
+          "target": "203.0.113.2",
+          "ttl": 300,
+          "type": "A"
+        }
+      ],
+      "registrar": "Third-Party"
+    },
+    {
+      "dnsProviders": {
+        "bind": -1
+      },
+      "meta": {
+        "dnscontrol_tag": "",
+        "dnscontrol_uniquename": "example-b.net"
+      },
+      "name": "example-b.net",
+      "records": [
+        {
+          "name": "main",
+          "target": "203.0.113.12",
+          "ttl": 300,
+          "type": "A"
+        },
+        {
+          "name": "www",
+          "target": "203.0.113.1",
+          "ttl": 300,
+          "type": "A"
+        }
+      ],
+      "registrar": "Third-Party"
     }
   ],
   "registrars": [

pkg/js/parse_tests/037-splithor/empty.example.net.zone

@@ -0,0 +1,3 @@
+$TTL 300
+main             IN A     203.0.113.22
+www              IN A     203.0.113.2

pkg/js/parse_tests/037-splithor/example-b.net.zone

@@ -0,0 +1,3 @@
+$TTL 300
+main             IN A     203.0.113.12
+www              IN A     203.0.113.1

pkg/js/parse_tests/037-splithor/example.com!inside.zone

@@ -0,0 +1,3 @@
+$TTL 300
+main             IN A     1.1.1.1
+                 IN A     11.11.11.11

pkg/js/parse_tests/037-splithor/example.com!outside.zone

@@ -0,0 +1,2 @@
+$TTL 300
+main             IN A     8.8.8.8

pkg/js/parse_tests/037-splithor/example.com.zone

@@ -0,0 +1,3 @@
+$TTL 300
+main             IN A     3.3.3.3
+www              IN A     33.33.33.33

pkg/js/parse_tests/037-splithor/example.net!inside.zone

@@ -0,0 +1,4 @@
+$TTL 300
+main             IN A     192.0.2.1
+                 IN A     203.0.113.12
+www              IN A     203.0.113.1

pkg/js/parse_tests/037-splithor/example.net!outside.zone

@@ -0,0 +1,4 @@
+$TTL 300
+main             IN A     203.0.113.1
+                 IN A     203.0.113.12
+www              IN A     203.0.113.1

pkg/js/parse_tests/037-splithor/example.net.zone

@@ -0,0 +1,3 @@
+$TTL 300
+main             IN A     203.0.113.12
+www              IN A     203.0.113.1

pkg/normalize/validate.go

@@ -564,6 +564,8 @@ func processSplitHorizonDomains(config *models.DNSConfig) error {
 	seen := map[string]bool{}
 	for _, d := range config.Domains {
 		uniquename := d.GetUniqueName()
+		// empty tag == untagged ("example.com!" -> "example.com")
+		uniquename = strings.TrimSuffix(uniquename, "!")
 		if seen[uniquename] {
 			return fmt.Errorf("duplicate domain name: %q", uniquename)
 		}