From ce89f7fb967fb4059494e6353999cb6f836bd66b Mon Sep 17 00:00:00 2001 From: Tom Limoncelli Date: Thu, 1 Jun 2023 13:11:36 -0400 Subject: [PATCH] CHORE: Eliminate SetTargetTXTString() (#2409) --- models/t_parse.go | 45 ++++++++++++++++------------ models/t_txt.go | 28 ----------------- providers/gcloud/gcloudProvider.go | 12 ++++++-- providers/hedns/hednsProvider.go | 2 ++ providers/route53/route53Provider.go | 18 +++++++---- 5 files changed, 51 insertions(+), 54 deletions(-) diff --git a/models/t_parse.go b/models/t_parse.go index 586b6fe67..374526e91 100644 --- a/models/t_parse.go +++ b/models/t_parse.go @@ -13,25 +13,29 @@ import ( // seems to quote them differently. // // Recommended calling convention: Process the exceptions first, then use the -// function for everything else. +// PopulateFromString function for everything else. // -// var err error -// switch rType { -// case "MX": -// // MX priority in a separate field. -// if err := rc.SetTargetMX(cr.Priority, target); err != nil { -// return nil, fmt.Errorf("unparsable MX record received from cloudflare: %w", err) -// } -// case "TXT": -// // TXT records are stored verbatim; no quoting/escaping to parse. -// err = rc.SetTargetTXT(target) -// // ProTip: Use rc.SetTargetTXTs(manystrings) if the API or parser returns a list of substrings. -// default: -// err = rec.PopulateFromString(rType, target, origin) -// } -// if err != nil { -// return nil, fmt.Errorf("unparsable record received from CHANGE_TO_PROVDER_NAME: %w", err) -// } +// rtype := FILL_IN_TYPE +// var err error +// rc := &models.RecordConfig{Type: rtype} +// rc.SetLabelFromFQDN(FILL_IN_NAME, origin) +// rc.TTL = uint32(FILL_IN_TTL) +// rc.Original = FILL_IN_ORIGINAL // The raw data received from provider (if needed later) +// switch rtype { +// case "MX": +// // MX priority in a separate field. +// err = rc.SetTargetMX(cr.Priority, target) +// case "TXT": +// // TXT records are stored verbatim; no quoting/escaping to parse. +// err = rc.SetTargetTXT(target) +// default: +// err = rc.PopulateFromString(rtype, target, origin) +// } +// if err != nil { +// return nil, fmt.Errorf("unparsable record type=%q received from PROVDER_NAME: %w", rtype, err) +// } +// return rc, nil + func (rc *RecordConfig) PopulateFromString(rtype, contents, origin string) error { if rc.Type != "" && rc.Type != rtype { panic(fmt.Errorf("assertion failed: rtype already set (%s) (%s)", rtype, rc.Type)) @@ -64,7 +68,10 @@ func (rc *RecordConfig) PopulateFromString(rtype, contents, origin string) error case "SOA": return rc.SetTargetSOAString(contents) case "SPF", "TXT": - return rc.SetTargetTXTString(contents) + // Parsing the contents may be unexpected. If your provider gives you a + // string that needs no further parsing, special case TXT and use + // rc.SetTargetTXT(target) like in the example above. + return rc.SetTargetTXTs(ParseQuotedTxt(contents)) case "SRV": return rc.SetTargetSRVString(contents) case "SSHFP": diff --git a/models/t_txt.go b/models/t_txt.go index 9decea13d..4a39c06c4 100644 --- a/models/t_txt.go +++ b/models/t_txt.go @@ -152,34 +152,6 @@ func (rc *RecordConfig) GetTargetTXTJoined() string { return strings.Join(rc.TxtStrings, "") } -// SetTargetTXTString is like SetTargetTXTs but accepts one big string, -// which is parsed into individual strings. -// Ex: -// -// foo << 1 string -// foo bar << 1 string -// "foo bar" << 1 string -// "foo" "bar" << 2 strings -// "f"oo" "bar" << 2 strings, one has a quote in it -// -// BUG: This function doesn't handle escaped quotes ("like \" this"). -// -// FIXME(tlim): This function is badly named. It obscures the fact -// that the string is parsed for quotes and stores a list of strings. -// -// Deprecated: This function has a confusing name. Most providers API -// return a single string, in which case you should use -// SetTargetTXT(). If your provider returns multiple strings, use -// SetTargetTXTs(). If your provider returns a single string that -// must be parsed to extract the individual strings, use -// SetTargetTXTfromRFC1035Quoted(). Sadly we have not figured out -// an integration test that will fail if you chose the wrong function. -// As a result, we recommend trying SetTargetTXT() before you try -// SetTargetTXTfromRFC1035Quoted(). -func (rc *RecordConfig) SetTargetTXTString(s string) error { - return rc.SetTargetTXTs(ParseQuotedTxt(s)) -} - // SetTargetTXTfromRFC1035Quoted parses a series of quoted strings // and sets .TxtStrings based on the result. // Note: Most APIs do notThis is rarely used. Try using SetTargetTXT() first. diff --git a/providers/gcloud/gcloudProvider.go b/providers/gcloud/gcloudProvider.go index fd0837379..a7333b47f 100644 --- a/providers/gcloud/gcloudProvider.go +++ b/providers/gcloud/gcloudProvider.go @@ -300,8 +300,16 @@ func nativeToRecord(set *gdns.ResourceRecordSet, rec, origin string) (*models.Re r := &models.RecordConfig{} r.SetLabelFromFQDN(set.Name, origin) r.TTL = uint32(set.Ttl) - if err := r.PopulateFromString(set.Type, rec, origin); err != nil { - return nil, fmt.Errorf("unparsable record received from GCLOUD: %w", err) + rtype := set.Type + var err error + switch rtype { + case "TXT": + err = r.SetTargetTXTs(models.ParseQuotedTxt(rec)) + default: + err = r.PopulateFromString(rtype, rec, origin) + } + if err != nil { + return nil, fmt.Errorf("unparsable record %q received from GCLOUD: %w", rtype, err) } return r, nil } diff --git a/providers/hedns/hednsProvider.go b/providers/hedns/hednsProvider.go index 83711f2f5..13aaa3746 100644 --- a/providers/hedns/hednsProvider.go +++ b/providers/hedns/hednsProvider.go @@ -367,6 +367,8 @@ func (c *hednsProvider) GetZoneRecords(domain string, meta map[string]string) (m // Convert to TXT record as SPF is deprecated rc.Type = "TXT" fallthrough + case "TXT": + err = rc.SetTargetTXTs(models.ParseQuotedTxt(data)) default: err = rc.PopulateFromString(rc.Type, data, domain) } diff --git a/providers/route53/route53Provider.go b/providers/route53/route53Provider.go index c51657349..2fe31a161 100644 --- a/providers/route53/route53Provider.go +++ b/providers/route53/route53Provider.go @@ -651,7 +651,7 @@ func nativeToRecords(set r53Types.ResourceRecordSet, origin string) ([]*models.R rtype = "TXT" fallthrough default: - ty := string(rtype) + rtypeString := string(rtype) val := *rec.Value // AWS Route53 has a bug. Sometimes it returns a target @@ -673,18 +673,26 @@ func nativeToRecords(set r53Types.ResourceRecordSet, origin string) ([]*models.R // first n pushes. It will seem odd but this is AWS's bug. // The UPSERT command only fixes the first record, even if // the UPSET received a list of corrections. - if ty == "CNAME" || ty == "MX" { + if rtypeString == "CNAME" || rtypeString == "MX" { if !strings.HasSuffix(val, ".") { val = val + "." } } + var err error rc := &models.RecordConfig{TTL: uint32(aws.ToInt64(set.TTL))} rc.SetLabelFromFQDN(unescape(set.Name), origin) - if err := rc.PopulateFromString(string(rtype), val, origin); err != nil { - return nil, fmt.Errorf("unparsable record received from R53: %w", err) - } rc.Original = set + switch rtypeString { + case "TXT": + err = rc.SetTargetTXTs(models.ParseQuotedTxt(val)) + default: + err = rc.PopulateFromString(rtypeString, val, origin) + } + if err != nil { + return nil, fmt.Errorf("unparsable record type=%q received from ROUTE53: %w", rtypeString, err) + } + results = append(results, rc) } }