From ace2242daf83f896d1fe1c3de45f8e56f9081e1b Mon Sep 17 00:00:00 2001 From: Tom Limoncelli Date: Mon, 8 May 2023 22:29:58 -0400 Subject: [PATCH] BUG: diff2 doesn't process IGNORE_TARGET() correctly (#2338) Co-authored-by: Tom Limoncelli --- .github/workflows/build.yml | 4 +- integrationTest/integration_test.go | 148 +++++++++++++++++++++------- pkg/diff2/handsoff.go | 2 +- pkg/diff2/handsoff_test.go | 4 +- 4 files changed, 120 insertions(+), 38 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index ac0973ea3..b0b34dfe3 100644 --- a/.github/workflows/build.yml +++ b/.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: diff --git a/integrationTest/integration_test.go b/integrationTest/integration_test.go index 9b4b7c6e3..39604afac 100644 --- a/integrationTest/integration_test.go +++ b/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("Add a new record - ignoring test.foo.com.", - cname("bar", "bar.foo.com."), + tc("ignoring CNAME=test.foo.com.", ignoreTarget("test.foo.com.", "CNAME"), - ), - 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"), + 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."), ), ), - // 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. - // + 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.", + ignoreTarget("**.acm-validations.aws.", "CNAME"), + ).ExpectNoChanges(), + ), // 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 diff --git a/pkg/diff2/handsoff.go b/pkg/diff2/handsoff.go index cbe59898c..30b8420de 100644 --- a/pkg/diff2/handsoff.go +++ b/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 } } diff --git a/pkg/diff2/handsoff_test.go b/pkg/diff2/handsoff_test.go index 6b336ca13..b0856851d 100644 --- a/pkg/diff2/handsoff_test.go +++ b/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: `) }