DNSIMPLE: Add compatibility with TXT changes#2745
Merged
tlimoncelli merged 11 commits intoStackExchange:masterfrom Jan 2, 2024
weppos:dnsimple/txt-quoting
Merged
DNSIMPLE: Add compatibility with TXT changes#2745tlimoncelli merged 11 commits intoStackExchange:masterfrom weppos:dnsimple/txt-quoting
tlimoncelli merged 11 commits intoStackExchange:masterfrom
weppos:dnsimple/txt-quoting
Conversation
Since December 2023 we store and return a RFC-compliant version of the string. https://support.dnsimple.com/articles/txt-record/#serialization It means the check for the ALIAS alternate TXT record should consider both possibilities, until the database is fully normalized.
Facilitate the transition until the database is fully encoded.
Artificially prefix the string with "legacy: " so that DNScontrol will find a diff, and attempt to store the string again.
Contributor
Author
|
Tests are passing: They were failing before, with the current version: |
philpennock
approved these changes
Dec 26, 2023
Contributor
philpennock
left a comment
philpennock
left a comment
There was a problem hiding this comment.
Works for me with our domains.
Would be good to have clarification in the DNSimple docs that the string quoting is the RFC-format -- it would be poor to have DNSControl and DNSimple disagree on quoting of characters.
Contributor
|
Looks good! I agree with point (1) and (2). (1) A little duplication that makes it easy to clean up later is better than complex code, (2) I like how you mutate the string to force an update... I might use that in the future for other projects. Best, CC @onlyhavecans for visibility |
Contributor
Author
|
Thanks for your feedback @tlimoncelli. I incorporated some additional changes from @philpennock proposal, I think this PR is now good to go. |
cafferata
reviewed
Dec 28, 2023
| // new client | ||
| client := dnsimpleapi.NewClient(tc) | ||
| client.SetUserAgent("DNSControl") | ||
| client.SetUserAgent(fmt.Sprintf("%s/%s", "dnscontrol", version.Banner())) |
Collaborator
There was a problem hiding this comment.
@tlimoncelli, @weppos better early than late, this change will conflict/break with #2737.
Contributor
Author
There was a problem hiding this comment.
Thanks for the heads up @cafferata, really appreciated.
I don't want to cause any delay in this or the other PR. Therefore, I reverted the change. It was not essential and could be easily applied later on.
Prevent conflict with #2737 See #2745 (review) This reverts commit 455c35b.
philpennock
approved these changes
Dec 28, 2023
Contributor
philpennock
left a comment
philpennock
left a comment
There was a problem hiding this comment.
Still LGTM but if this is an official DNSimple update then hopefully you can do better than my "assumed to be" text in the docs on the limitations. 😆
Contributor
|
I'm super excited to get a PR from the CTO of https://github.com/dnsimple himself! Thank you so much for contributing to this project. Please let me know if there's anything we can do to make DNSControl better for DNSimple users. If you have a OTE or testing account we can add DNSimple to the CICD pipeline. |
Contributor
Author
It's a fair request! 😉 I'll engage the team to review and clarify, we'll follow up with a new PR soon. I plan to cleanup the compatibility branching anyways after the transitioning period is over. |
Contributor
Author
Hi @tlimoncelli, thanks a lot for maintaining this project. I really appreciate the effort. I am using it for all my personal domains, and I'm really happy with it. I know other DNSimple team members also use it.
Absolutely! We have a sandbox environment (the equivalent of an OTE) you can use for integration tests: https://sandbox.dnsimple.com/dashboard You can find more information about the environment at this link, including the example of credit card numbers you can use to activate a stub subscription. Let me know if you have any questions, I'll be happy to assist you in setting up the integration. |
|
Hi there! As per @weppos' request, we have checked the code that's responsible for enforcing this limit on TXT records, and here are our insights:
Having said that, it's clear we should make this explicit in our docs so that we will be fixing that as soon as possible. On the other hand, we also wanted to take the opportunity to understand better the use case in |
Contributor
|
My use-case is DKIM keys, and when I was unbreaking, I needed to find the actual value to set as a limit, and I just found "characters". @tlimoncelli @weppos The documentation and the check will need updating. There are going to be open questions across providers around things like normalization forms and if strings are re-normalized on their end, to handle combining characters etc, but if we assume that DNS providers are going to be reasonable and use the string as given in an API call as the value to publish, without messing with it, then the UTF-8 encoding at least means we know how many runes there will be, and we can hopefully assume that's how the DNS providers who limit on "Unicode characters" are measuring things. So I suspect that:
|
Contributor
|
I have a question for @weppos about how the 1000 chars are counted. Some providers count the escape chars, not just the payload:
Right now the code tests the payload (7 and 256). Is that correct or should it count those as 10 and 260 respectively? As far as Unicode... I thought the RFCs (and I'll have to check) said that TXT records were just the printable 7-bit ascii chars. (The RFCs use the term "octets" to mean 8-bit bytes.... because they were written before the world standardized on 8 bits in a byte!) In that case, we should reject illegal chars at the UI level. I'd be glad to be corrected. |
Contributor
|
RFC 1035 just defines TXT as holding
So TXT can be anything, and there are no character set constraints. It doesn't have to be UTF-8. It can contain NUL characters, because it's length-prefixed. Thus my original false assumption that "character" would be octets. |
Contributor
|
Look up the TXT records in DNS, with a tcpdump running to see the actual hex codes and strings on the wire, for:
Using There's a lot more stress-tests in that zone-file. It's open AXFR from |
PJEilers
pushed a commit
to realtimeregister/dnscontrol
that referenced
this pull request
Jan 8, 2024
Co-authored-by: Tom Limoncelli <[email protected]>
Contributor
Author
|
@tlimoncelli @philpennock thanks for your research and questions. I was unable to follow up promptly in the last few weeks due to a busy schedule and some critical deadlines. I'll take a look at your questions next week, and will follow up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
I work at DNSimple, and I also happen to be a user of this library. 😉
We have recently introduced some TXT changes, that are partially modifying the behavior of TXT records via API. In particular, while we still support sending unquoted TXTs, we now always store RFC-compliant content and we return it.
If you submit an unquoted string, we will normalize and always return the quoted version.
In the past, we used to not normalize the input. You could either submit a quoted/unquoted string, and our system accepted both. The content was eventually sanitized at the name server stack, but occasionally this led to some unexpected conflicts.
The new changes and validations are described here:
https://support.dnsimple.com/articles/txt-record/#validation
Furthermore, we now support longer TXT strings.
Transition period
We rolled out the changes in Dec 2023, and we are now gong though a transitioning phase where we allow both quoted/non-quoted strings in our system. All new values are normalized, existing values are untouched.
We are currently notifying our customers, and we will normalize all entries in our system by February. This means most of the code in this PR that deals with conditional formatting will eventually go away once the content is fully normalized. This affects some of the implementation choices I made that are explained below.
Notes
I had to make a few choices a long the way, in order to implement these changes. They are described below. I am open to feedback:
Examples
This is an example of what you will see when you first run this version, and you have legacy (non-quoted) strings in your DNSimple account: