From 66a76c44c127f1415f669ca50b63881aa4978f8e Mon Sep 17 00:00:00 2001 From: Tom Limoncelli Date: Tue, 16 May 2023 10:42:08 -0400 Subject: [PATCH] Don't count diff2.REPORT (informational warnings) as a "correction" (#2361) Co-authored-by: Tom Limoncelli --- commands/commands.go | 2 -- commands/previewPush.go | 17 ++++++++- integrationTest/integration_test.go | 22 +++++++----- models/domain.go | 4 ++- pkg/acme/acme.go | 7 ++-- pkg/diff/diff2compat.go | 6 ++++ pkg/diff2/handsoff.go | 6 ++-- pkg/printer/printer.go | 8 +++++ pkg/zonerecs/zonerecords.go | 26 +++++++------- providers/route53/route53Provider.go | 53 ++++++++++++---------------- 10 files changed, 90 insertions(+), 61 deletions(-) diff --git a/commands/commands.go b/commands/commands.go index e8fb933f0..24b1f4fa2 100644 --- a/commands/commands.go +++ b/commands/commands.go @@ -26,8 +26,6 @@ var commands = []*cli.Command{} // These are set by/for goreleaser var ( version = "dev" - commit = "none" - date = "unknown" ) func cmd(cat string, c *cli.Command) bool { diff --git a/commands/previewPush.go b/commands/previewPush.go index bb13209d5..59cdfa68e 100644 --- a/commands/previewPush.go +++ b/commands/previewPush.go @@ -222,13 +222,16 @@ func run(args PreviewArgs, push bool, interactive bool, out printer.CLI) error { continue } - corrections, err := zonerecs.CorrectZoneRecords(provider.Driver, domain) + reports, corrections, err := zonerecs.CorrectZoneRecords(provider.Driver, domain) + printReports(domain.Name, provider.Name, reports, out, push, notifier) out.EndProvider(provider.Name, len(corrections), err) if err != nil { anyErrors = true return } totalCorrections += len(corrections) + // When diff1 goes away, the call to printReports() should be moved to HERE. + //printReports(domain.Name, provider.Name, reports, out, push, notifier) anyErrors = printOrRunCorrections(domain.Name, provider.Name, corrections, out, push, interactive, notifier) || anyErrors } @@ -543,3 +546,15 @@ func printOrRunCorrections(domain string, provider string, corrections []*models } return anyErrors } + +func printReports(domain string, provider string, reports []*models.Correction, out printer.CLI, push bool, notifier notifications.Notifier) (anyErrors bool) { + anyErrors = false + if len(reports) == 0 { + return false + } + for i, report := range reports { + out.PrintReport(i, report) + notifier.Notify(domain, provider, report.Msg, nil, !push) + } + return anyErrors +} diff --git a/integrationTest/integration_test.go b/integrationTest/integration_test.go index 0ad442260..a3db18e21 100644 --- a/integrationTest/integration_test.go +++ b/integrationTest/integration_test.go @@ -223,12 +223,12 @@ func makeChanges(t *testing.T, prv providers.DNSServiceProvider, dc *models.Doma } // get and run corrections for first time - corrections, err := zonerecs.CorrectZoneRecords(prv, dom) + _, corrections, err := zonerecs.CorrectZoneRecords(prv, dom) if err != nil { t.Fatal(fmt.Errorf("runTests: %w", err)) } if tst.Changeless { - if count := zonerecs.CountActionable(corrections); count != 0 { + if count := len(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) @@ -256,11 +256,11 @@ func makeChanges(t *testing.T, prv providers.DNSServiceProvider, dc *models.Doma } // run a second time and expect zero corrections - corrections, err = zonerecs.CorrectZoneRecords(prv, dom2) + _, corrections, err = zonerecs.CorrectZoneRecords(prv, dom2) if err != nil { t.Fatal(err) } - if count := zonerecs.CountActionable(corrections); count != 0 { + if count := len(corrections); count != 0 { t.Logf("Expected 0 corrections on second run, but found %d.", count) for i, c := range corrections { t.Logf("UNEXPECTED #%d: %s", i, c.Msg) @@ -351,10 +351,13 @@ func TestDualProviders(t *testing.T) { run := func() { dom, _ := dc.Copy() - cs, err := zonerecs.CorrectZoneRecords(p, dom) + rs, cs, err := zonerecs.CorrectZoneRecords(p, dom) if err != nil { t.Fatal(err) } + for i, c := range rs { + t.Logf("INFO#%d:\n%s", i+1, c.Msg) + } for i, c := range cs { t.Logf("#%d:\n%s", i+1, c.Msg) if err = c.F(); err != nil { @@ -373,14 +376,17 @@ func TestDualProviders(t *testing.T) { run() // run again to make sure no corrections t.Log("Running again to ensure stability") - cs, err := zonerecs.CorrectZoneRecords(p, dc) + rs, cs, err := zonerecs.CorrectZoneRecords(p, dc) if err != nil { t.Fatal(err) } - if count := zonerecs.CountActionable(cs); count != 0 { + if count := len(cs); count != 0 { t.Logf("Expect no corrections on second run, but found %d.", count) + for i, c := range rs { + t.Logf("INFO#%d:\n%s", i+1, c.Msg) + } for i, c := range cs { - t.Logf("#%d: %s", i, c.Msg) + t.Logf("#%d: %s", i+1, c.Msg) } t.FailNow() } diff --git a/models/domain.go b/models/domain.go index 14e8fb07e..ae9e5e5de 100644 --- a/models/domain.go +++ b/models/domain.go @@ -9,8 +9,10 @@ import ( ) const ( + // DomainUniqueName is the full `example.com!tag` name` DomainUniqueName = "dnscontrol_uniquename" - DomainTag = "dnscontrol_tag" + // DomainTag is the tag part of `example.com!tag` name + DomainTag = "dnscontrol_tag" ) // DomainConfig describes a DNS domain (technically a DNS zone). diff --git a/pkg/acme/acme.go b/pkg/acme/acme.go index 7316658e2..2b961d103 100644 --- a/pkg/acme/acme.go +++ b/pkg/acme/acme.go @@ -263,7 +263,7 @@ func (c *certManager) ensureNoPendingCorrections(d *models.DomainConfig) error { return nil } -// IgnoredProviders is a lit of provider names that should not be used to fill challenges. +// IgnoredProviders is a list of provider names that should not be used to fill challenges. var IgnoredProviders = map[string]bool{} func (c *certManager) getCorrections(d *models.DomainConfig) ([]*models.Correction, error) { @@ -276,10 +276,13 @@ func (c *certManager) getCorrections(d *models.DomainConfig) ([]*models.Correcti if err != nil { return nil, err } - corrections, err := zonerecs.CorrectZoneRecords(p.Driver, dc) + reports, corrections, err := zonerecs.CorrectZoneRecords(p.Driver, dc) if err != nil { return nil, err } + for _, c := range reports { + c.Msg = fmt.Sprintf("INFO[%s] %s", p.Name, strings.TrimSpace(c.Msg)) + } for _, c := range corrections { c.Msg = fmt.Sprintf("[%s] %s", p.Name, strings.TrimSpace(c.Msg)) } diff --git a/pkg/diff/diff2compat.go b/pkg/diff/diff2compat.go index 613bc8745..434d56484 100644 --- a/pkg/diff/diff2compat.go +++ b/pkg/diff/diff2compat.go @@ -62,7 +62,13 @@ func (d *differCompat) IncrementalDiff(existing []*models.RecordConfig) (unchang case diff2.REPORT: // Sadly the NewCompat function doesn't have an equivalent. We // just output the messages now. + fmt.Print("INFO: ") fmt.Println(inst.MsgsJoined) + + // TODO(tlim): When diff1 is deleted, IncremtntalDiff should add a + // parameter to list the REPORT messages. It can also eliminate the + // first parameter (existing) since nobody uses that in the diff2 + // world. case diff2.CREATE: cor.Desired = inst.New[0] toCreate = append(toCreate, cor) diff --git a/pkg/diff2/handsoff.go b/pkg/diff2/handsoff.go index 30b8420de..fdc9c4513 100644 --- a/pkg/diff2/handsoff.go +++ b/pkg/diff2/handsoff.go @@ -119,18 +119,18 @@ func handsoff( // Process UNMANAGE/IGNORE_* and NO_PURGE features: ignorable, foreign := processIgnoreAndNoPurge(domain, existing, desired, absences, unmanagedConfigs, noPurge) if len(foreign) != 0 { - msgs = append(msgs, fmt.Sprintf("INFO: %d records not being deleted because of NO_PURGE:", len(foreign))) + msgs = append(msgs, fmt.Sprintf("%d records not being deleted because of NO_PURGE:", len(foreign))) msgs = append(msgs, reportSkips(foreign, !printer.SkinnyReport)...) } if len(ignorable) != 0 { - msgs = append(msgs, fmt.Sprintf("INFO: %d records not being deleted because of IGNORE*():", len(ignorable))) + msgs = append(msgs, fmt.Sprintf("%d records not being deleted because of IGNORE*():", len(ignorable))) msgs = append(msgs, reportSkips(ignorable, !printer.SkinnyReport)...) } // Check for invalid use of IGNORE_*. conflicts := findConflicts(unmanagedConfigs, desired) if len(conflicts) != 0 { - msgs = append(msgs, fmt.Sprintf("INFO: %d records that are both IGNORE*()'d and not ignored:", len(conflicts))) + msgs = append(msgs, fmt.Sprintf("%d records that are both IGNORE*()'d and not ignored:", len(conflicts))) for _, r := range conflicts { msgs = append(msgs, fmt.Sprintf(" %s %s %s", r.GetLabelFQDN(), r.Type, r.GetTargetCombined())) } diff --git a/pkg/printer/printer.go b/pkg/printer/printer.go index 943413f84..f75935c0c 100644 --- a/pkg/printer/printer.go +++ b/pkg/printer/printer.go @@ -19,6 +19,7 @@ type CLI interface { StartRegistrar(name string, skip bool) PrintCorrection(n int, c *models.Correction) + PrintReport(n int, c *models.Correction) // Print corrections that are diff2.REPORT EndCorrection(err error) PromptToRun() bool } @@ -89,6 +90,13 @@ func (c ConsolePrinter) PrintCorrection(i int, correction *models.Correction) { fmt.Fprintf(c.Writer, "#%d: %s\n", i+1, correction.Msg) } +// PrintReport is called to print/format each non-mutating correction (diff2.REPORT). +func (c ConsolePrinter) PrintReport(i int, correction *models.Correction) { + // When diff1 is eliminated: + //fmt.Fprintf(c.Writer, "INFO#%d: %s\n", i+1, correction.Msg) + fmt.Fprintf(c.Writer, "INFO: %s\n", correction.Msg) +} + // PromptToRun prompts the user to see if they want to execute a correction. func (c ConsolePrinter) PromptToRun() bool { fmt.Fprint(c.Writer, "Run? (Y/n): ") diff --git a/pkg/zonerecs/zonerecords.go b/pkg/zonerecs/zonerecords.go index e56b92252..cb067da00 100644 --- a/pkg/zonerecs/zonerecords.go +++ b/pkg/zonerecs/zonerecords.go @@ -7,11 +7,11 @@ import ( // CorrectZoneRecords calls both GetZoneRecords, does any // post-processing, and then calls GetZoneRecordsCorrections. The // name sucks because all the good names were taken. -func CorrectZoneRecords(driver models.DNSProvider, dc *models.DomainConfig) ([]*models.Correction, error) { +func CorrectZoneRecords(driver models.DNSProvider, dc *models.DomainConfig) ([]*models.Correction, []*models.Correction, error) { existingRecords, err := driver.GetZoneRecords(dc.Name, dc.Metadata) if err != nil { - return nil, err + return nil, nil, err } // downcase @@ -23,7 +23,7 @@ func CorrectZoneRecords(driver models.DNSProvider, dc *models.DomainConfig) ([]* // dc.Records. dc, err = dc.Copy() if err != nil { - return nil, err + return nil, nil, err } // punycode @@ -31,18 +31,18 @@ func CorrectZoneRecords(driver models.DNSProvider, dc *models.DomainConfig) ([]* // FIXME(tlim) It is a waste to PunyCode every iteration. // This should be moved to where the JavaScript is processed. - return driver.GetZoneRecordsCorrections(dc, existingRecords) + everything, err := driver.GetZoneRecordsCorrections(dc, existingRecords) + reports, corrections := splitReportsAndCorrections(everything) + return reports, corrections, err } -// CountActionable returns the number of corrections that have -// actions. It is like `len(corrections)` but doesn't count any -// corrections that are purely informational. (i.e. `.F` is nil) -func CountActionable(corrections []*models.Correction) int { - count := 0 - for i := range corrections { - if corrections[i].F != nil { - count++ +func splitReportsAndCorrections(everything []*models.Correction) (reports, corrections []*models.Correction) { + for i := range everything { + if everything[i].F == nil { + reports = append(reports, everything[i]) + } else { + corrections = append(corrections, everything[i]) } } - return count + return reports, corrections } diff --git a/providers/route53/route53Provider.go b/providers/route53/route53Provider.go index 792e8d03f..5d9bd1ef3 100644 --- a/providers/route53/route53Provider.go +++ b/providers/route53/route53Provider.go @@ -496,7 +496,9 @@ func (r *route53Provider) GetZoneRecordsCorrections(dc *models.DomainConfig, exi return nil, err } instructions = reorderInstructions(instructions) - wasReport := false + var reports []*models.Correction + + //wasReport := false for _, inst := range instructions { instNameFQDN := inst.Key.NameFQDN instType := inst.Key.Type @@ -505,8 +507,12 @@ func (r *route53Provider) GetZoneRecordsCorrections(dc *models.DomainConfig, exi switch inst.Type { case diff2.REPORT: - chg = r53Types.Change{} - wasReport = true + // REPORTs are held in a separate list so that they aren't part of the batching process. + reports = append(reports, + &models.Correction{ + Msg: inst.MsgsJoined, + }) + continue case diff2.CREATE: fallthrough @@ -560,34 +566,19 @@ func (r *route53Provider) GetZoneRecordsCorrections(dc *models.DomainConfig, exi } addCorrection := func(msg string, req *r53.ChangeResourceRecordSetsInput) { - - if wasReport { - - // Add a "msg only" correction. - corrections = append(corrections, - &models.Correction{ - Msg: msg, - }) - - } else { - - // Add a function to execute. - corrections = append(corrections, - &models.Correction{ - Msg: msg, - F: func() error { - var err error - req.HostedZoneId = zone.Id - withRetry(func() error { - _, err = r.client.ChangeResourceRecordSets(context.Background(), req) - return err - }) + corrections = append(corrections, + &models.Correction{ + Msg: msg, + F: func() error { + var err error + req.HostedZoneId = zone.Id + withRetry(func() error { + _, err = r.client.ChangeResourceRecordSets(context.Background(), req) return err - }, - }) - - } - + }) + return err + }, + }) } // Send the changes in as few API calls as possible. @@ -605,7 +596,7 @@ func (r *route53Provider) GetZoneRecordsCorrections(dc *models.DomainConfig, exi return nil, err } - return corrections, nil + return append(reports, corrections...), nil }