BUG: diff2 doesn't process IGNORE_TARGET() correctly (#2338)

Co-authored-by: Tom Limoncelli <tal@whatexit.org>

.github/workflows/build.yml

@@ -33,10 +33,10 @@ jobs:
     - uses: actions/upload-artifact@v3.1.2
       with:
         path: "/tmp/test-results"
-    - name: Build binaries
+    - name: Build binaries (if tagged)
       if: github.ref_type == 'tag'
       run: goreleaser build
-    - name: build binaries
+    - name: Build binaries (not tagged)
       if: github.ref_type != 'tag'
       run: goreleaser build --snapshot
   integration-test-providers:

integrationTest/integration_test.go

@@ -127,6 +127,11 @@ func getDomainConfigWithNameservers(t *testing.T, prv providers.DNSServiceProvid
 // error explaining why it is not.
 func testPermitted(t *testing.T, p string, f TestGroup) error {
 
+	// Does this test require "diff2"?
+	if f.diff2only && !diff2.EnableDiff2 {
+		return fmt.Errorf("test for diff2 only")
+	}
+
 	// not() and only() can't be mixed.
 	if len(f.only) != 0 && len(f.not) != 0 {
 		return fmt.Errorf("invalid filter: can't mix not() and only()")
@@ -222,7 +227,15 @@ func makeChanges(t *testing.T, prv providers.DNSServiceProvider, dc *models.Doma
 		if err != nil {
 			t.Fatal(fmt.Errorf("runTests: %w", err))
 		}
-		if (len(corrections) == 0 && expectChanges) && (tst.Desc != "Empty") {
+		if tst.Changeless {
+			if count := zonerecs.CountActionable(corrections); count != 0 {
+				t.Logf("Expected 0 corrections on FIRST run, but found %d.", count)
+				for i, c := range corrections {
+					t.Logf("UNEXPECTED #%d: %s", i, c.Msg)
+				}
+				t.FailNow()
+			}
+		} else if (len(corrections) == 0 && expectChanges) && (tst.Desc != "Empty") {
 			t.Fatalf("Expected changes, but got none")
 		}
 		for _, c := range corrections {
@@ -410,6 +423,7 @@ type TestGroup struct {
 	not       []string
 	trueflags []bool
 	tests     []*TestCase
+	diff2only bool
 }
 
 type TestCase struct {
@@ -418,6 +432,16 @@ type TestCase struct {
 	IgnoredNames   []*models.IgnoreName
 	IgnoredTargets []*models.IgnoreTarget
 	Unmanaged      []*models.UnmanagedConfig
+	Changeless     bool // set to true if any changes would be an error
+}
+
+func (tc *TestCase) ExpectNoChanges() *TestCase {
+	tc.Changeless = true
+	return tc
+}
+func (tg *TestGroup) Diff2Only() *TestGroup {
+	tg.diff2only = true
+	return tg
 }
 
 func SetLabel(r *models.RecordConfig, label, domain string) {
@@ -620,23 +644,25 @@ func tc(desc string, recs ...*models.RecordConfig) *TestCase {
 	var ignoredTargets []*models.IgnoreTarget
 	var unmanagedItems []*models.UnmanagedConfig
 	for _, r := range recs {
-		if r.Type == "IGNORE_NAME" {
+		switch r.Type {
+		case "IGNORE_NAME":
 			ignoredNames = append(ignoredNames, &models.IgnoreName{Pattern: r.GetLabel(), Types: r.GetTargetField()})
 			unmanagedItems = append(unmanagedItems, &models.UnmanagedConfig{
 				LabelPattern: r.GetLabel(),
 				RTypePattern: r.GetTargetField(),
 			})
-		} else if r.Type == "IGNORE_TARGET" {
-			rec := &models.IgnoreTarget{
+			continue
+		case "IGNORE_TARGET":
+			ignoredTargets = append(ignoredTargets, &models.IgnoreTarget{
 				Pattern: r.GetLabel(),
 				Type:    r.GetTargetField(),
-			}
-			ignoredTargets = append(ignoredTargets, rec)
+			})
 			unmanagedItems = append(unmanagedItems, &models.UnmanagedConfig{
 				RTypePattern:  r.GetTargetField(),
 				TargetPattern: r.GetLabel(),
 			})
-		} else {
+			continue
+		default:
 			records = append(records, r)
 		}
 	}
@@ -1719,21 +1745,32 @@ func makeTests(t *testing.T) []*TestGroup {
 			tc("Create some records",
 				txt("foo", "simple"),
 				a("foo", "1.2.3.4"),
-			),
-			tc("Add a new record - ignoring foo",
 				a("bar", "1.2.3.4"),
-				ignoreName("foo"),
 			),
+			tc("ignore foo",
+				ignoreName("foo"),
+				a("bar", "1.2.3.4"),
+			).ExpectNoChanges(),
+			clear(),
+			tc("Create some records",
+				txt("bar.foo", "simple"),
+				a("bar.foo", "1.2.3.4"),
+				a("bar", "1.2.3.4"),
+			),
+			tc("ignore *.foo",
+				ignoreName("*.foo"),
+				a("bar", "1.2.3.4"),
+			).ExpectNoChanges(),
 			clear(),
 			tc("Create some records",
 				txt("bar.foo", "simple"),
 				a("bar.foo", "1.2.3.4"),
 			),
-			tc("Add a new record - ignoring *.foo",
-				a("bar", "1.2.3.4"),
+			tc("ignore *.foo while we add 1",
 				ignoreName("*.foo"),
+				a("bar", "1.2.3.4"),
 			),
-		),
+		).Diff2Only(),
 
 		testgroup("IGNORE_NAME apex",
 			tc("Create some records",
@@ -1742,39 +1779,84 @@ func makeTests(t *testing.T) []*TestGroup {
 				txt("bar", "stringbar"),
 				a("bar", "2.4.6.8"),
 			),
+			tc("ignore apex",
+				ignoreName("@"),
+				txt("bar", "stringbar"),
+				a("bar", "2.4.6.8"),
+			).ExpectNoChanges(),
+			clear(),
 			tc("Add a new record - ignoring apex",
+				ignoreName("@"),
 				txt("bar", "stringbar"),
 				a("bar", "2.4.6.8"),
 				a("added", "4.6.8.9"),
-				ignoreName("@"),
-			),
 			),
+		).Diff2Only(),
 
-		testgroup("IGNORE_TARGET function",
+		testgroup("IGNORE_TARGET function CNAME",
 			tc("Create some records",
 				cname("foo", "test.foo.com."),
-				cname("bar", "test.bar.com."),
+				cname("keep", "keep.example.com."),
+			),
+			tc("ignoring CNAME=test.foo.com.",
+				ignoreTarget("test.foo.com.", "CNAME"),
+				cname("keep", "keep.example.com."),
+			).ExpectNoChanges(),
+			tc("ignoring CNAME=test.foo.com. and add",
+				ignoreTarget("test.foo.com.", "CNAME"),
+				cname("keep", "keep.example.com."),
+				a("adding", "1.2.3.4"),
+				cname("another", "www.example.com."),
+			),
+		),
+
+		testgroup("IGNORE_TARGET function CNAME*",
+			tc("Create some records",
+				cname("foo1", "test.foo.com."),
+				cname("foo2", "my.test.foo.com."),
+				cname("bar", "test.example.com."),
+			),
+			tc("ignoring CNAME=test.foo.com.",
+				ignoreTarget("*.foo.com.", "CNAME"),
+				cname("foo2", "my.test.foo.com."),
+				cname("bar", "test.example.com."),
+			).ExpectNoChanges(),
+			tc("ignoring CNAME=test.foo.com. and add",
+				ignoreTarget("*.foo.com.", "CNAME"),
+				cname("foo2", "my.test.foo.com."),
+				cname("bar", "test.example.com."),
+				a("adding", "1.2.3.4"),
+				cname("another", "www.example.com."),
+			),
+		),
+
+		testgroup("IGNORE_TARGET function CNAME**",
+			tc("Create some records",
+				cname("foo1", "test.foo.com."),
+				cname("foo2", "my.test.foo.com."),
+				cname("bar", "test.example.com."),
+			),
+			tc("ignoring CNAME=test.foo.com.",
+				ignoreTarget("**.foo.com.", "CNAME"),
+				cname("bar", "test.example.com."),
+			).ExpectNoChanges(),
+			tc("ignoring CNAME=test.foo.com. and add",
+				ignoreTarget("**.foo.com.", "CNAME"),
+				cname("bar", "test.example.com."),
+				a("adding", "1.2.3.4"),
+				cname("another", "www.example.com."),
+			),
+		),
+
+		testgroup("IGNORE_TARGET b2285",
+			tc("Create some records",
+				cname("foo", "redact1.acm-validations.aws."),
+				cname("bar", "redact2.acm-validations.aws."),
 			),
 			tc("Add a new record - ignoring test.foo.com.",
-				cname("bar", "bar.foo.com."),
-				ignoreTarget("test.foo.com.", "CNAME"),
+				ignoreTarget("**.acm-validations.aws.", "CNAME"),
+			).ExpectNoChanges(),
 		),
-			clear(),
-			tc("Create some records",
-				cname("bar.foo", "a.b.foo.com."),
-				a("test.foo", "1.2.3.4"),
-			),
-			tc("Add a new record - ignoring **.foo.com. targets",
-				a("bar", "1.2.3.4"),
-				ignoreTarget("**.foo.com.", "CNAME"),
-			),
-		),
-		// NB(tlim): We don't have a test for IGNORE_TARGET at the apex
-		// because IGNORE_TARGET only works on CNAMEs and you can't have a
-		// CNAME at the apex.  If we extend IGNORE_TARGET to support other
-		// types of records, we should add a test at the apex.
-
-		//
 
 		// Narrative: Congrats! You're done!  If you've made it this far
 		// you're very close to being able to submit your PR.  Here's

pkg/diff2/handsoff.go

@@ -243,7 +243,7 @@ func matchAny(uconfigs []*models.UnmanagedConfig, rec *models.RecordConfig) bool
 	for _, uc := range uconfigs {
 		if matchLabel(uc.LabelGlob, rec.GetLabel()) &&
 			matchType(uc.RTypeMap, rec.Type) &&
-			matchTarget(uc.TargetGlob, rec.GetLabel()) {
+			matchTarget(uc.TargetGlob, rec.GetTargetField()) {
 			return true
 		}
 	}

pkg/diff2/handsoff_test.go

@@ -88,8 +88,8 @@ func handsoffHelper(t *testing.T, existingZone, desiredJs string, noPurge bool,
 
 	if resultWanted != resultActual {
 		testifyrequire.Equal(t,
-			resultActual,
 			resultWanted,
+			resultActual,
 			"GOT =\n```\n%s```\nWANT=\n```%s```\nINPUTS=\n```\n%s\n```\n",
 			resultActual,
 			resultWanted,
@@ -230,7 +230,7 @@ D("f.com", "none",
 `
 	handsoffHelper(t, existingZone, desiredJs, true, `
 IGNORED:
-FOREIGN:
 _2222222222222222.cr CNAME _333333.nnn.acm-validations.aws.
+FOREIGN:
 	`)
 }