-
Notifications
You must be signed in to change notification settings - Fork 402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IGNORE()'d record deleted when other modifications are made (provider: MYTHICBEASTS) #3227
Comments
Ping @tomfitzhenry, the maintainer of the Mythic Beasts provider. |
I don't think this is a problem with just mythicbeasts. This might be a problem for any provider that uses the ByZone() function (that is, any updates require uploading the entire zone). I can think of some other edge-cases that aren't covered. We may have to disable IGNORE() for anything that uses ByZone() [autodns, bind, mythicbeasts, realtimeregister, sakuracloud]. And now I'm thinking of some edge cases that might fail for ByRecordSet() too [azuredns azureprivatedns gcloud gcore huaweicloud ns1 powerdns route53 transip] The fix would be to add any "ignored" records to the zone, that way they are uploaded with the zone refresh. Not sure if that's going to be easy or not. In the meanwhile, we should at least document this and possibly block the use of IGNORE*() with providers that use ByZone(). |
Thanks for digging into this, Tom! I'm assuming all variants of I had a quick skim of https://github.com/StackExchange/dnscontrol/blob/main/pkg/diff2/handsoff.go#L52 last night (with the caveat that Go isn't a language I'm familiar with!), and that seemed to describe a sensible approach - given the output text implied it knew of the ignored record's existence, I assumed it would also have the required state to copy it to the desired list and therefore send it back with the new zonefile? I note the caveat about BIND in https://github.com/StackExchange/dnscontrol/blob/main/documentation/language-reference/domain-modifiers/IGNORE.md?plain=1#L363, but I assumed that when working with an existing zonefile available, that wouldn't be in play. Given in my case it's only a single record that's updated outside of DNSControl, I may be able to work around it by delegating to a single-host sub zone and then dynamically updating the A record at the apex of that zone - I'll give that a try later! |
Excellent reading of the source code! Yes! That's exactly what we need to implement (adding ignored records to the state). Of course, since there's no transaction-level locking, it will be possible that a new record manually inserted at the exact right moment will be deleted, but that should be quire rare. The warning about BIND dates back to the original IGNORE implementation, which had tons of problems. How prescient that the same bug would appear in the rewrite. The first step should be to build some integration tests that demonstrate the bug. (I thought I had done that, which is why I was surprised by this bug but...) |
Odd. I'm not able to reproduce this with other DNS providers. I don't have an account on MYTHICBEASTS so I'll need to ask you to do some testing. I've added your example as an integration tests. They are in this PR: #3255 You'll need a test domain (all the records will be deleted. Please don't use a domain you need!) If you are unfamiliar with the testing system, here's how to checkout DNSControl, switch to the PR branch, and run the tests.
You'll see output like: https://github.com/StackExchange/dnscontrol/actions/runs/12362512717/job/34502276519?pr=3255 |
@tomfitzhenry Would you like to have mythic automatically tested in our CI/CD pipeline? Mythic will be tested with each PR, catching new bugs early. You'd need to set up a test account and domain and send me the creds. Here are the instructions: https://docs.dnscontrol.org/developer-info/byo-secrets |
By the way.. this is how I tested it in my own configuration: First I added these 2 lines to a domain:
Then I change them to this:
Then I changed them to this:
|
@tlimoncelli Thanks Tom! I don't yet have Go installed here but will try and spin up a test environment and run against that PR. (If Tom F isn't able to sort a test domain for the CI system, I'm happy to register one and fire over the creds.) |
Yes that sounds great. To issues credentials for a subdomain, I know that's supported, but I'll need to reach out to Mythic Beasts support for that. In the mean time, I'll work on reproducing this. |
OK, tests run in a container, and all the IGNORE ones pass:
But I can confirm your method in #3227 (comment), run manually, reproduces it with the MB provider (running with a release build of DNSControl, on Windows, and tested again with the latest 4.15.1 release after grabbing this output): Create the initial records:
Check that we get the expected records back from the MB API:
Change
Check we still get both records back from the MB API (yep):
Now change
And finally check that the
(Obviously this is the same result as originally, but reassuring to see it's not some magic about entries created through the DynDNS API or similar causing it!) |
Is there any way we can see what DNSControl is sending to the API? Is it
sending the entire zone or is it missing that one zone record?
Tom
|
Yep, I can grab the requests by proxying through Fiddler. (Some HTTP request headers redacted / omitted for clarity) Initial setup, creating the two A records:
Change first record to ignore, push again:
(No modifications made, so no PUT call) Change second record, leaving first as ignore:
|
EUREKA! Thanks, @rmc47! That output not only proves the bug, but let me create a better integration test that triggers the bug. If you look at the final "PUT", the "testignore" record is not included in the PUT. It should be, since the provider requires all records in the update. The good news is that this enabled me to create an integration test that triggers the bug. The trick is to include an extra step that verifies that all the records still exist at the provider. The new test is in this PR: #3261 You can see that it fails for all the diff2.ByZone() users and nobody else: https://github.com/StackExchange/dnscontrol/actions/runs/12396270182 Now that we can reproduce the issue, I'll take ownership for fixing this. |
Oh you star, thanks Tom! Offer still stands if you need a test domain for Mythic CI tests to target - very happy to contribute that (or if there's a preferred way I can support the project, shout). |
Thanks!
Yes, I'd love Mythic to join the ranks of the automatically tested. Instructions are here: https://docs.dnscontrol.org/developer-info/byo-secrets |
Will do - @tomfitzhenry, if you could shout if you're already looking at a test domain separately - I'm happy to register one if not, but no point us both doing so :-). |
Good idea to avoid duplicare work! It's on my todo list, and I was planning to look into it this weekend, but I'm happy if you do instead! :) My only thought is that it probably makes sense for the dnscontrol Mythic Beasts maintainer (currently me) to have this account, since it'll be useful for general maintenance/reproducing issues. Are you interested in being the dnscontrol Mythic Beasts maintainer? |
I don't think my Go skills are up to the task, but thanks for the offer! I'm happy instead to throw you some cash to cover, say, 5 years of a co.uk domain registration, or to register the domain in its own MB account and add you to that account, whichever works best - is the email address on your git commits good to use if so? |
I've merged the change but I'm leaving this open until I hear from you. |
Describe the bug
When
dnscontrol push
is used and results in modification to a zone (at least using MythicBeasts as the provider), a record which is marked withIGNORE()
is also deleted.To Reproduce
IGNORE("testignore"), A("testdefined", "9.9.9.9")
dnscontrol push
testignore
(for example, a record used for dynamic DNS, viahttps://ipv4.api.mythic-beasts.com/dns/v2/dynamic/testignore.example.com
)dnscontrol push
again - no modifications are made, and both records remain in place as seen in provider's control paneltestdefined
to point to a different address, and rundnscontrol push
again, resulting in the output belowtestignore
is now absent in the provider's control panel and DNS zoneSame zone definition:
Output of DNSControl push when modifying record is:
Expected behavior
The
IGNORE()
d record should still exist after the other modification is applied.DNS Provider
Docker image version: https://github.com/stackexchange/dnscontrol/pkgs/container/dnscontrol/302287665?tag=4.14.3
The text was updated successfully, but these errors were encountered: