SPF/TXT multipart handling

[weppos]

This PR introduces the multi-part SPF/TXT handling. The underlying RR types are already defined so that the value is a list, but we currently ignore it and we attempt to parse/split the incoming value.

This is related to https://github.com/dnsimple/dnsimple-business/issues/1765, in particular to issues related to the TXT implementation. The current implementation is designed to prefer the parts over the single value, whenever the JSON contains a valid element with the expected value.

Here's some examples.

1. TXT: Both txt and txts exists, txts is used

dig -p 8053 @127.0.0.1 txt2d.multipart.example TXT

txt2d.multipart.example. 120	IN	TXT	"string c" "string d"

Belongs to https://github.com/dnsimple/dnsimple-engineering/issues/288

Open issues

• Format TXT (and SPF) records appropriately (split at 255) — likely handled by dns_erlang
  • Add a test to verify encoding and decoding of big TXT sections dns_erlang#57
• Test the changes locally, and update zone fixture files if necessary

[NelsonVides]

I'm assuming than when https://github.com/dnsimple/zoneserver/pull/251 is merged and a new version is released, I can test locally with a docker image, otherwise, this PR looks ready to me.

[weppos]

I'm assuming than when dnsimple/zoneserver#251 is merged and a new version is released, I can test locally with a docker image, otherwise, this PR looks ready to me.

This is correct. We wanted to implement the ability to generate images from feature branches, but I don't believe we're there yet. I can probably merge and ship that change early on Monday.

Beware, however, that this is only half of the story. Zone changes done in the app will be propagated as RRSet from the dnsimple-app, and that portion is not implemented yet.

See https://github.com/dnsimple/dnsimple-engineering/issues/288

[weppos]

@NelsonVides I merged the PR, so now you have access to the updated container. Beware the behavior is disabled by default, you will need to set one or both ENV variables to true in the Docker Compose file to enable the flag.
https://github.com/dnsimple/zoneserver?tab=readme-ov-file#general

[NelsonVides]

This record

Gives me this response in dig:

;; ANSWER SECTION:
txt4.example5.com.      60      IN      TXT     "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad mi
nim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat." "Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nu
lla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."

and at runtime the encoding function is called with the right splitting:

     mfa = {dns,encode_message,2},
     data = [{dns_message,55750,true,0,true,false,true,false,
                          false,false,0,1,1,0,1,
                          [{dns_query,<<"txt4.example5.com">>,1,16}],
                          [{dns_rr,<<"txt4.example5.com">>,1,16,60,
                                   {dns_rrdata_txt,[<<"Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim
 ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.">>,
                                                    <<"Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupi
datat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.">>]}}],
                          [],
                          [{dns_optrr,1232,0,0,false,
                                      [{dns_opt_unknown,10,<<91,163,29,115,22,185,36,98>>}]}]},
             [{max_size,512}]],

However, traces from the modified erldns_zone_parser:json_record_to_erlang/1 don't seem to be getting any <<"txts">>, even after setting

x-zoneserver-env-vars: &zoneserver-env-vars
...
  - TXT2025_ENABLED=true
  - SPF2025_ENABLED=true

in https://github.com/dnsimple/dnsimple-engineering/blob/639f617ce9d2fefd88b01a7c00d53ae5f1274a2e/docker/compose.yml#L42, so not sure what is missing 🤔

[NelsonVides]

Now, when tracing a full start, the zoneserver is indeed passing the right payloads as it was implemented at https://github.com/dnsimple/zoneserver/pull/251.

And the erldns_txt module is never called:

3> tr:select(fun(#tr{mfa = {erldns_txt, _, _}} = Tr) -> Tr end).
[]

[weppos]

@NelsonVides have you tested this PR creating a TXT and a SPF longer than 255 chars? Is it correctly being split in the dig query?

I assume that would require somehow to propagate the dnsimple/dns_erlang#57 changes all the way to this PR, which requires also an update in erldns -> dns_erlang. Isn't it?

[NelsonVides]

@NelsonVides have you tested this PR creating a TXT and a SPF longer than 255 chars? Is it correctly being split in the dig query?

I assume that would require somehow to propagate the dnsimple/dns_erlang#57 changes all the way to this PR, which requires also an update in erldns -> dns_erlang. Isn't it?

Yes, the Lorem Ipsum has ~300 characters, and it splits well on both the wire (tracing) as well as the dig query.

The changes from dns_erlang, this PR has the dependency update too (see rebar.config).

[weppos]

Where is this change originating? Is it anyhow changing the current behavior?

[NelsonVides]

It comes from the new type requirements, requiring this element to be an integer. This is read when decoding, but in code it is never used during encoding as far as I could find. We can allow it to be an undefined if we update dns_erlang, but it seems to me from RFC4034 (https://datatracker.ietf.org/doc/html/rfc4034#section-3.1.6) that this is never supposed not to be an integer 🤔

[weppos]

@NelsonVides once this code is shipped, please update https://github.com/dnsimple/dnsimple-engineering/issues/269 with a note, so that I can have a platform team member (likely @DXTimer ) to enable it on a canary node, and run through the validation steps to confirm all is working before we make it the default option across the board.

[weppos]

@NelsonVides I can't provide a review as I'm unfortunately the original author. Please note I wanted to test the changes today, but I am blocked as since the new configuration changes the instructions to run the zone server locally are broken.

https://github.com/dnsimple/erldnsimple?tab=readme-ov-file#configuring is outdated, and I don't know how to proceed.

Please update the README (and CONTRIBUTING if necessary) with the correct instructions, and merge main into this branch. I will the re-run the review by running this branch directly on my local env, using the latest zoneserver and dnsimple-app images with the flag enabled.

[NelsonVides]

@NelsonVides I can't provide a review as I'm unfortunately the original author. Please note I wanted to test the changes today, but I am blocked as since the new configuration changes the instructions to run the zone server locally are broken.

https://github.com/dnsimple/erldnsimple?tab=readme-ov-file#configuring is outdated, and I don't know how to proceed.

Please update the README (and CONTRIBUTING if necessary) with the correct instructions, and merge main into this branch. I will the re-run the review by running this branch directly on my local env, using the latest zoneserver and dnsimple-app images with the flag enabled.

https://github.com/dnsimple/erldnsimple/pull/418 hopefully that should fix how to run locally now. Will look deeper into docs in a follow-up.