MAINT: PopulateFromString should use SetTargetTXT, not SetTargetTXTString

[tlimoncelli]

[NOTE: This is not urgent. I'm asking that everyone check their code in the next 4-5 weeks: I'd like to merge this on Aug 1, 2022]

The good news: PopulateFromString is very useful and a lot of people use it.

The bad news: PopulateFromString has a bug, and I need to check with everyone to make sure they aren't affected.

WHAT IS THE PROBLEM?

PopulateFromString calls SetTargetTXTString, which is almost never the right thing to do. It should use SetTargetTXT() instead.

SetTargetTXTString parses the string for quotes... incorrectly. Generally provider code receives a big TXT string that doesn't need to be parsed.

WHY CHANGE?

The integration tests do not detect if the incorrect SetTargetTXT* function is called. I can't figure out a test that would do that. Therefore, we have to make this change carefully.

Here is a checklist of who uses this function:

• [ ] akamaiedgedns @svernick
• [ ] autodns @arnoschoon
• [X] cloudflare @tresni
• [ ] desec @D3luxee
• [ ] dnsimple @aeden
• [x] dnsmadeeasy @vojtad
• [ ] exoscale pierre-emmanuelJ
• [x] gandiv5 @TomOnTime
• [x] gcloud @riyadhalnur
• [ ] hedns @rblenkinsopp
• [x] hetzner @das7pad
• [X] hexonet @papakai
• [x] hostingde @juliusrickert
• [ ] inwx @svenpeter42
• [ ] linode @koesie10
• [x] namecheap @captncraig
• [X] namedotcom
• [ ] ns1 @costasd
• [ ] oracle @kallsyms
• [x] ovh @masterzen
• [x] powerdns @jpbede
• [x] route53 @tresni
• [x] transip @blackshadev
• [X] vultr @pgaskin

WHAT SHOULD I DO?

Git clone this and run your integration tests.

If your integration tests work, let me know.

if the integration tests fail, I'd be glad to help you fix them.

I've written a longer explanation of how TXT records are handled in models/t_txt.go

[tlimoncelli]

If this change breaks your integration tests, I have a few suggestions:

You may need to handle TXT records using special code, not using PopulateFromString():

  case "TXT":
    // Do what you need to properly parse your TXT records.
    //err := rc.SetTargetTXT(native_txt_data)
    //err := rc.SetTargetTXTs(many_small_strings)
    //err := rc.SetTargetTXTs(myParser(native_txt_data))
    //err := rc.SetTargetTXTfromRFC1035Quoted(native_txt_data)
    return rc, err
  default:
    if err := rc.PopulateFromString(rType, ..., domain); err != nil {
      return nil, fmt.Errorf("unparsable record received from PROVIDERNAME: %w", err)
    }

After you fix your code, you may need to identify certain TXT strings that your code doesn't handle. For example, cscglobal needed these:

  if err := recordaudit.TxtNoStringsLen256orLonger(records); err != nil {
    return err
  } // Needed as of 2022-07-01

  if err := recordaudit.TxtNoMultipleStrings(records); err != nil {
    return err
  } // Needed as of 2022-07-01

More info is in models/t_txt.go

[masterzen]

Integrations test failures for ovh, because of double encoded quoted strings like:

=== RUN   TestDNSProviders/dnscontroltest.ovh/16:TXTMulti-same:Create_TXTMulti_1
    integration_test.go:220: CREATE TXT foo.dnscontroltest.ovh "simple" ttl=300
    integration_test.go:220: REFRESH zone dnscontroltest.ovh
    integration_test.go:239: Expected 0 corrections on second run, but found 2.
    integration_test.go:241: UNEXPECTED #0: MODIFY TXT foo.dnscontroltest.ovh: ("\"simple\"" ttl=300) -> ("simple" ttl=300)
    integration_test.go:241: UNEXPECTED #1: REFRESH zone dnscontroltest.ovh

I'm going to test with your suggested changes.

[riyadhalnur]

=== RUN   TestDNSProviders/dnscontrol-gcloud.com/12:complex_TXT:TXT_with_0-octel_string
    integration_test.go:220: CREATE TXT foo1.dnscontrol-gcloud.com "" ttl=300
        
    integration_test.go:239: Expected 0 corrections on second run, but found 1.
    integration_test.go:241: UNEXPECTED #0: MODIFY TXT foo1.dnscontrol-gcloud.com: ("\"\"" ttl=300) -> ("" ttl=300)

Multiple integration test cases also failed for GCLOUD (attached above is 1 such example). I will get back to you on this thread once I add the changes suggested.

[masterzen]

Integrations test failures for ovh, because of double encoded quoted strings like: [snipped]

I'm going to test with your suggested changes.

So, with OVH using SetTargetTXTfromRFC1035Quoted works a bit better, but ultimately fail those two tests:

 --- FAIL: TestDNSProviders/dnscontroltest.ovh/11:long_TXT:Create_long_TXT (1.15s) 
 --- FAIL: TestDNSProviders/dnscontroltest.ovh/14:long_TXT:Create_a_505_TXT (0.93s)

I'll dig deeper about those failures, but regarding quoted strings, I've been noting the following:

• the OVH UI adds double quotes to the record value when creating a new TXT record
• the OVH API allows bare strings to be stored verbatim

[jpbede]

PDNS also failed on several integration tests. This has been fixed with #1569.

[tlimoncelli]

I notice a lot of people have implemented similar parse functions:

• providers/powerdns/convert.go line 34 func parseTxt(content string) (result []string)
• providers/namedotcom/records.go line 208 func decodeTxt(s string) []string

Maybe we should pick the best and provide it as an exported helper function, similar to SetTargetTXTfromRFC1035Quoted?

[jpbede]

The parser function of namedotcom would also work for powerdns. All integration tests and unit tests are passed with it.

[riyadhalnur]

The parser under namedotcom along with the SetTargetTXTs() method works for GCLOUD as well. I would opt for moving the parser under the txtutil package. If we're on the same page with that, I can go ahead and open a PR with the relevant changes later.

[masterzen]

I've fixed OVH with this change:

diff --git i/providers/ovh/ovhProvider.go w/providers/ovh/ovhProvider.go
index e6e91325..c6771d9f 100644
--- i/providers/ovh/ovhProvider.go
+++ w/providers/ovh/ovhProvider.go
@@ -190,7 +190,16 @@ func nativeToRecord(r *Record, origin string) (*models.RecordConfig, error) {
   }

   rec.SetLabel(r.SubDomain, origin)
-  if err := rec.PopulateFromString(rtype, r.Target, origin); err != nil {
+
+  var err error
+  switch rtype {
+  case "TXT":
+     err = rec.SetTargetTXTs(models.ParseQuotedTxt(r.Target))
+  default:
+     err = rec.PopulateFromString(rtype, r.Target, origin)
+  }
+
+  if err != nil {
      return nil, fmt.Errorf("unparsable record received from ovh: %w", err)
   }

Tests also pass with namedotcom decodeTxts. Let me know there's a standard solution that should be used for the providers.

[tlimoncelli]

@masterzen Of77 If both work, use the decodeTxts version.

[tlimoncelli]

Folks, Just to let you know... I think I may not be explaining this all correctly. I don't want to send you down the wrong road.

I think this is the decision flow-chart:

• Step 1: Does the API take/send 1 string or a list of strings?

  • List of strings: Your nativeToRc() should call SetTargetTXTs(many []string); your Corrections code should copy the individual strings from rc.TxtStrings (which is []string). Once this is implemented you are done.
  • 1 string: Go to Step 2.

• Step 2: Go to the provider's website and manually create a TXT record. Does it permit you to enter 1 string or a list of strings?

  • 1 string: Your nativeToRc() should call SetTargetTXT(s string); your Corrections code should read the field using rc.GetTargetTXTJoined(). Once this is implemented you are done.
  • List of strings: Go to Step 3.

• Step 3: At this point, we can conclude that the string the API gives you needs to be parsed into many separate strings.

  • Your nativeToRc() should call SetTargetTXTs(PARSER(s)); your Corrections code should read the field using rc.GetTargetRFC1035Quoted(). PARSER should be one of three things:
    • models.ParseQuotedEscaped() -- Handles strings like: "one" "two" "in"side"
    • models.ParseQuotedFields() -- Handles quotes, doesn't allow escaped chars
    • ParseSetTargetTXTfromRFC1035Quoted() -- Uses miekg/dns's parser, which leaves backslashes intact.

I'm going to move those parsers to a separate module and give them more meaningful names. However, i have to run to a meeting so that will have to wait.

Again... I'm not sure if I'm explaining myself very well. Please accept my apologies for the confusion. It will get less confusing once I'm less confused!

[masterzen]

Folks, Just to let you know... I think I may not be explaining this all correctly. I don't want to send you down the wrong road.

[snip] I'm going to move those parsers to a separate module and give them more meaningful names. However, i have to run to a meeting so that will have to wait.

Again... I'm not sure if I'm explaining myself very well. Please accept my apologies for the confusion. It will get less confusing once I'm less confused!

Your flowchart made plenty of sense, except that it doesn't seem to work with OVH:

1. Step 1: OVH accepts a unique string in the API -> go to 2.
2. Step 2: OVH UI accepts only a string, and encloses in double-quotes as if it was a zone file. If I add " around the string, they are kept (it's not double-double-quoted). I can't put a straight " in the middle of a string. I can enter strings of any size, there's no splitting at all.

From your flowchart, I'm supposed to use SetTargetTXT, but then that doesn't unquote the string I might have entered into their web UI. Note that OVH API returns the quoted string (from the example entered in the UI posted above):

{

    "id": 5237495109
    "ttl": 0
    "target": ""test""
    "subDomain": "test2"
    "fieldType": "TXT"
    "zone": "dnscontroltest.ovh"
}

The API is much more forgiving, as it is possible to store strings quoted and unquoted, which makes things more complex.

Based on the premise we need to be able to modify records entered by the UI, I need to unquote in nativetoRc with something along of models.ParseQuotedFields (even though there's only one field) and quote in the result of GetTargetTXTJoined (because GetTargetRFC1035Quoted could produce multiple quoted strings).

Is that OK?

[tlimoncelli]

Based on the premise we need to be able to modify records entered by the UI, I need to unquote in nativetoRc with something along of models.ParseQuotedFields (even though there's only one field) and quote in the result of GetTargetTXTJoined (because GetTargetRFC1035Quoted could produce multiple quoted strings).

Is that OK?

Perfect!

[masterzen]

Based on the premise we need to be able to modify records entered by the UI, I need to unquote in nativetoRc with something along of models.ParseQuotedFields (even though there's only one field) and quote in the result of GetTargetTXTJoined (because GetTargetRFC1035Quoted could produce multiple quoted strings). Is that OK?

Perfect!

Shouldn't it also be split (in 255 bytes long chunks) to be stored in models.Record.TxtStrings or is it ok to have only one long string in the models.Record? (I must admit I'm a bit loss, I need to read some code I guess :D )

[tlimoncelli]

Each string can be any length. Often len(.TxtStrings) == 1 and len (.TxtStrings[0]) is very very long.

[tlimoncelli]

Update: I've moved the parsers to a package called pks/decode to make them easier to reuse.

Some are old (QuoteEscapedFields is what namedotcom uses) and some are new. Of course, you can write your own decoder.

func QuotedFields(s string) ([]string, error) {
func QuoteEscapedFields(s string) ([]string, error) {
func MiekgDNSFields(s string) ([]string, error) {
func RFC1035Fields(s string) ([]string, error) {

The first two require the first and last octet be ".

The good news is that I retrofitted GCLOUD, PACKETFRAME, and DIGITALOCEAN. If @riyadhalnur, @hamptonmoore and @Deraen can try running the integration tests and report back, I think we might have the solution.

Sorry for the big change. You'll need to rebase.

[jpbede]

I've successfully rerun the integration tests with decode.QuoteEscapedFields for POWERDNS. Should I create a PR merging into your branch?

[TomOnTime]

On Wed, Jun 22, 2022 at 2:46 PM Jan-Philipp Benecke < @.***> wrote:

I've successfully rerun the integration tests with decode.QuotedFields for POWERDNS. Should I create a PR merging into your branch?

Yes please

-- Sent from Gmail Mobile. Autocorrect is my co-author.

[riyadhalnur]

Integration tests for GCLOUD run successfully

[TomOnTime]

@riyadhalnur Excellent!

[TomOnTime]

FYI: I'm going camping and won't be back until Monday, June 27. I'll be offline until then.

[das7pad]

Thanks for the heads up, Tom. I've opened https://github.com/StackExchange/dnscontrol/pull/1578 with the necessary changes for the HETZNER provider. Together with https://github.com/StackExchange/dnscontrol/pull/1577 the integration tests are passing.

[tlimoncelli]

Thanks for the heads up, Tom. I've opened #1578 with the necessary changes for the HETZNER provider. Together with #1577 the integration tests are passing.

Thanks! I've merged them.

[rblenkinsopp]

HEDNS provider is not passing integration tests as-is, I will look to apply the fixes, I'm however away this weekend so I won't be able to look at this till next week.

[vojtad]

I've opened #1579 which fixes parsing TXT records for DNSMADEEASY.

[blackshadev]

Transip failed, I have opened #1581 with the fix. With that the integration tests succeed.

Thanks @tlimoncelli for the heads up, work and clear communication!

[masterzen]

I'm able to fix the tests with the following patch:

@@ -190,7 +193,18 @@ func nativeToRecord(r *Record, origin string) (*models.RecordConfig, error) {
   }

   rec.SetLabel(r.SubDomain, origin)
-  if err := rec.PopulateFromString(rtype, r.Target, origin); err != nil {
+
+  var err error
+  switch r.FieldType {
+  case "TXT":
+     var result []string
+     if result, err = decode.QuotedFields(r.Target); err == nil {
+        err = rec.SetTargetTXTs(result)
+     }
+  default:
+     err = rec.PopulateFromString(rtype, r.Target, origin)
+  }
+  if err != nil {
      return nil, fmt.Errorf("unparsable record received from ovh: %w", err)
   }

Except that I don't think it is correct for OVH. I can enter "bar" "baz" with the OVH API (which takes only one string), but then it is transformed to "bar baz" when I query (through DNS), even though the UI still shows "bar" "baz" :) So I have the feeling in fact OVH doesn't support multiple TXT strings, and only arbitrary long strings, and we should have recordaudit.TxtNoMultipleStrings defined for this provider.

[masterzen]

As said in my previous comment, I've produced an implementation that seems correct to me in #1582. Let me know if there's anything that needs to be done.

[tlimoncelli]

Sounds good! I’m camping:traveling and without much internet access. I’ll look into this on the weekend.-- Sent from Gmail Mobile

[tlimoncelli]

Hey folks! I'm back from my travel.

First, I had a realization while traveling: I could have made this a lot easier for people if I had just renamed the old function to be PopulateFromStringOld() and asked people to submit individual PRs at your own pace. The way I did this, requiring everyone to comment on one big PR, was not the best way to do it. I apologize for making this so complex. Often the only way to see the right way is to do it the wrong way first. Mea culpa! I'll do better next time!

Second, I've added some helper functions so you don't have to call decode.Parse*() directly.