Skip to content
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

Open
rmc47 opened this issue Dec 8, 2024 · 19 comments · Fixed by #3263
Open

IGNORE()'d record deleted when other modifications are made (provider: MYTHICBEASTS) #3227

rmc47 opened this issue Dec 8, 2024 · 19 comments · Fixed by #3263
Assignees

Comments

@rmc47
Copy link

rmc47 commented Dec 8, 2024

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 with IGNORE() is also deleted.

To Reproduce

  1. Establish zone with MythicBeasts as the DNS provider
  2. In the zone definition, specify records: IGNORE("testignore"), A("testdefined", "9.9.9.9")
  3. dnscontrol push
  4. Outside of DNSControl, create an A record for testignore (for example, a record used for dynamic DNS, via https://ipv4.api.mythic-beasts.com/dns/v2/dynamic/testignore.example.com)
  5. dnscontrol push again - no modifications are made, and both records remain in place as seen in provider's control panel
  6. Modify the definition of testdefined to point to a different address, and run dnscontrol push again, resulting in the output below
  7. Observe that testignore is now absent in the provider's control panel and DNS zone

Same zone definition:

D("example.com", REG_NONE, DnsProvider(DNS_MYTHIC),
  IGNORE("testignore"),
  A("testdefined", "8.8.8.8")
);

Output of DNSControl push when modifying record is:

******************** Domain: example.com
----- Getting nameservers from: mythic
----- DNS Provider: mythic...
1 correction (mythic)
#1: 1 records not being deleted because of IGNORE*():
    testignore.example.com. A 8.8.8.8
± MODIFY testdefined.example.com A (9.9.9.9 ttl=300) -> (8.8.8.8 ttl=300)
SUCCESS!
----- Registrar: none...
0 corrections (none)

Expected behavior
The IGNORE()d record should still exist after the other modification is applied.

DNS Provider

  • MYTHICBEASTS

Docker image version: https://github.com/stackexchange/dnscontrol/pkgs/container/dnscontrol/302287665?tag=4.14.3

@cafferata
Copy link
Collaborator

Ping @tomfitzhenry, the maintainer of the Mythic Beasts provider.

@tlimoncelli
Copy link
Contributor

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().

@tlimoncelli tlimoncelli self-assigned this Dec 9, 2024
@rmc47
Copy link
Author

rmc47 commented Dec 9, 2024

Thanks for digging into this, Tom! I'm assuming all variants of IGNORE are likely to be similarly affected?

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!

@tlimoncelli
Copy link
Contributor

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...)

@tlimoncelli
Copy link
Contributor

CC @tomfitzhenry

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.

cd /somewhere
git clone https://github.com/StackExchange/dnscontrol.git
git checkout tlim_3227_ignorebroke
cd integrationTest
export MYTHICBEASTS_KEYID="fill-in"
export MYTHICBEASTS_SECRET="fill-in"
export MYTHICBEASTS_DOMAIN=example.com    (The test domain. All records will be deleted.)
go test -timeout 1h -v -verbose -provider MYTHICBEASTS

You'll see output like: https://github.com/StackExchange/dnscontrol/actions/runs/12362512717/job/34502276519?pr=3255

@tlimoncelli
Copy link
Contributor

@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

@tlimoncelli
Copy link
Contributor

By the way.. this is how I tested it in my own configuration:

First I added these 2 lines to a domain:

  // Startup: Load up the records
  A("testdefined", "8.8.8.8"),
  A("testignore", "1.2.3.4"),

Then I change them to this:

  // Ignore without a change:
  A("testdefined", "8.8.8.8"),
 IGNORE("testignore"),

Then I changed them to this:

  // Ignore WITH a change:
A("testdefined", "9.9.9.9"),
IGNORE("testignore"),

@rmc47
Copy link
Author

rmc47 commented Dec 16, 2024

@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.)

@tomfitzhenry
Copy link
Contributor

tomfitzhenry commented Dec 16, 2024

@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

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.

@rmc47
Copy link
Author

rmc47 commented Dec 16, 2024

OK, tests run in a container, and all the IGNORE ones pass:

=== RUN   TestDNSProviders/m0vfc.com/Clean_Slate:Empty#34
=== RUN   TestDNSProviders/m0vfc.com/72:IGNORE_main:Create_some_records
    integration_test.go:248:
        + CREATE bar.m0vfc.com A 5.5.5.5 ttl=300
        + CREATE foo.m0vfc.com A 1.2.3.4 ttl=300
        + CREATE foo.m0vfc.com A 2.3.4.5 ttl=300
        + CREATE foo.m0vfc.com TXT "simple" ttl=300
        + CREATE mail.m0vfc.com CNAME ghs.googlehosted.com. ttl=300
=== RUN   TestDNSProviders/m0vfc.com/72:IGNORE_main:ignore_label
=== RUN   TestDNSProviders/m0vfc.com/72:IGNORE_main:ignore_label,type
=== RUN   TestDNSProviders/m0vfc.com/72:IGNORE_main:ignore_label,type,target
=== RUN   TestDNSProviders/m0vfc.com/72:IGNORE_main:ignore_type
=== RUN   TestDNSProviders/m0vfc.com/72:IGNORE_main:ignore_type,target
=== RUN   TestDNSProviders/m0vfc.com/72:IGNORE_main:ignore_target
=== RUN   TestDNSProviders/m0vfc.com/72:IGNORE_main:ignore_manytypes
=== RUN   TestDNSProviders/m0vfc.com/72:IGNORE_main:ignore_label,type,target=*
=== RUN   TestDNSProviders/m0vfc.com/Clean_Slate:Empty#35
    integration_test.go:248:
        - DELETE bar.m0vfc.com A 5.5.5.5 ttl=300
        - DELETE foo.m0vfc.com A 1.2.3.4 ttl=300
        - DELETE foo.m0vfc.com A 2.3.4.5 ttl=300
        - DELETE foo.m0vfc.com TXT "simple" ttl=300
        - DELETE mail.m0vfc.com CNAME ghs.googlehosted.com. ttl=300
=== RUN   TestDNSProviders/m0vfc.com/73:IGNORE_apex:Create_some_records
    integration_test.go:248:
        + CREATE m0vfc.com A 1.2.3.4 ttl=300
        + CREATE m0vfc.com A 2.3.4.5 ttl=300
        + CREATE m0vfc.com TXT "simple" ttl=300
        + CREATE bar.m0vfc.com A 5.5.5.5 ttl=300
        + CREATE mail.m0vfc.com CNAME ghs.googlehosted.com. ttl=300
=== RUN   TestDNSProviders/m0vfc.com/73:IGNORE_apex:ignore_label
=== RUN   TestDNSProviders/m0vfc.com/73:IGNORE_apex:ignore_label,type
=== RUN   TestDNSProviders/m0vfc.com/73:IGNORE_apex:ignore_label,type,target
=== RUN   TestDNSProviders/m0vfc.com/73:IGNORE_apex:ignore_type
=== RUN   TestDNSProviders/m0vfc.com/73:IGNORE_apex:ignore_type,target
=== RUN   TestDNSProviders/m0vfc.com/73:IGNORE_apex:ignore_target
=== RUN   TestDNSProviders/m0vfc.com/73:IGNORE_apex:ignore_manytypes
=== RUN   TestDNSProviders/m0vfc.com/Clean_Slate:Empty#36
    integration_test.go:248:
        - DELETE m0vfc.com A 1.2.3.4 ttl=300
        - DELETE m0vfc.com A 2.3.4.5 ttl=300
        - DELETE m0vfc.com TXT "simple" ttl=300
        - DELETE bar.m0vfc.com A 5.5.5.5 ttl=300
        - DELETE mail.m0vfc.com CNAME ghs.googlehosted.com. ttl=300
=== RUN   TestDNSProviders/m0vfc.com/74:IGNORE_cross:Create_some_records
    integration_test.go:248:
        + CREATE m0vfc.com A 2.2.2.2 ttl=300
        + CREATE m0vfc.com TXT "asimple" ttl=300
        + CREATE foo.m0vfc.com A 1.2.3.4 ttl=300
        + CREATE foo.m0vfc.com TXT "simple" ttl=300
=== RUN   TestDNSProviders/m0vfc.com/74:IGNORE_cross:ignore_label=apex
=== RUN   TestDNSProviders/m0vfc.com/74:IGNORE_cross:ignore_label=apex#01
=== RUN   TestDNSProviders/m0vfc.com/Clean_Slate:Empty#37
    integration_test.go:248:
        - DELETE m0vfc.com A 2.2.2.2 ttl=300
        - DELETE m0vfc.com TXT "asimple" ttl=300
        - DELETE foo.m0vfc.com A 1.2.3.4 ttl=300
        - DELETE foo.m0vfc.com TXT "simple" ttl=300
=== RUN   TestDNSProviders/m0vfc.com/75:IGNORE_wilds:Create_some_records
    integration_test.go:248:
        + CREATE bar.bat.m0vfc.com A 5.5.5.5 ttl=300
        + CREATE foo.bat.m0vfc.com A 1.2.3.4 ttl=300
        + CREATE foo.bat.m0vfc.com A 2.3.4.5 ttl=300
        + CREATE foo.bat.m0vfc.com TXT "simple" ttl=300
        + CREATE mail.bat.m0vfc.com CNAME ghs.googlehosted.com. ttl=300
=== RUN   TestDNSProviders/m0vfc.com/75:IGNORE_wilds:ignore_label=foo.*
=== RUN   TestDNSProviders/m0vfc.com/75:IGNORE_wilds:ignore_label=foo.bat,type
=== RUN   TestDNSProviders/m0vfc.com/75:IGNORE_wilds:ignore_target=*.domain
=== RUN   TestDNSProviders/m0vfc.com/Clean_Slate:Empty#38
    integration_test.go:248:
        - DELETE bar.bat.m0vfc.com A 5.5.5.5 ttl=300
        - DELETE foo.bat.m0vfc.com A 1.2.3.4 ttl=300
        - DELETE foo.bat.m0vfc.com A 2.3.4.5 ttl=300
        - DELETE foo.bat.m0vfc.com TXT "simple" ttl=300
        - DELETE mail.bat.m0vfc.com CNAME ghs.googlehosted.com. ttl=300
=== RUN   TestDNSProviders/m0vfc.com/76:IGNORE_TARGET_b2285:Create_some_records
    integration_test.go:248:
        + CREATE bar.m0vfc.com CNAME redact2.acm-validations.aws. ttl=300
        + CREATE foo.m0vfc.com CNAME redact1.acm-validations.aws. ttl=300
=== RUN   TestDNSProviders/m0vfc.com/76:IGNORE_TARGET_b2285:Add_a_new_record_-_ignoring_test.foo.com.
=== RUN   TestDNSProviders/m0vfc.com/Clean_Slate:Empty#39
    integration_test.go:248:
        - DELETE bar.m0vfc.com CNAME redact2.acm-validations.aws. ttl=300
        - DELETE foo.m0vfc.com CNAME redact1.acm-validations.aws. ttl=300
=== RUN   TestDNSProviders/m0vfc.com/77:IGNORE_everything_b2822:Create_some_records
    integration_test.go:248:
        + CREATE dyndns-city1.m0vfc.com A 91.42.1.1 ttl=300
        + CREATE dyndns-city1.m0vfc.com AAAA 2003:dd:d7ff::fe71:ce77 ttl=300
        + CREATE dyndns-city2.m0vfc.com A 91.42.1.2 ttl=300
        + CREATE dyndns-city2.m0vfc.com AAAA 2003:dd:d7ff::fe71:ce78 ttl=300
=== RUN   TestDNSProviders/m0vfc.com/77:IGNORE_everything_b2822:ignore_them_all
=== RUN   TestDNSProviders/m0vfc.com/Clean_Slate:Empty#40
    integration_test.go:248:
        - DELETE dyndns-city1.m0vfc.com A 91.42.1.1 ttl=300
        - DELETE dyndns-city1.m0vfc.com AAAA 2003:dd:d7ff::fe71:ce77 ttl=300
        - DELETE dyndns-city2.m0vfc.com A 91.42.1.2 ttl=300
        - DELETE dyndns-city2.m0vfc.com AAAA 2003:dd:d7ff::fe71:ce78 ttl=300
=== RUN   TestDNSProviders/m0vfc.com/78:IGNORE_w/change_b3227:Create_some_records
    integration_test.go:248:
        + CREATE testdefined.m0vfc.com A 9.9.9.9 ttl=300
        + CREATE testignore.m0vfc.com A 8.8.8.8 ttl=300
=== RUN   TestDNSProviders/m0vfc.com/78:IGNORE_w/change_b3227:ignore
=== RUN   TestDNSProviders/m0vfc.com/78:IGNORE_w/change_b3227:ignore_with_change
    integration_test.go:248:
        1 records not being deleted because of IGNORE*():
            testignore.m0vfc.com. A 8.8.8.8
        ± MODIFY testdefined.m0vfc.com A (9.9.9.9 ttl=300) -> (2.2.2.2 ttl=300)
=== RUN   TestDNSProviders/m0vfc.com/79:structured_TXT_***SKIPPED(disabled_by_only)***:Empty
    integration_test.go:248:
        - DELETE testdefined.m0vfc.com A 2.2.2.2 ttl=300
=== RUN   TestDNSProviders/m0vfc.com/80:structured_TXT_as_native_records_***SKIPPED(disabled_by_only)***:Empty

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:

.\bin\dnscontrol.exe push --domains m0vfc.com
[INFO: Diff2 algorithm in use. Welcome to the future!]
INFO: In dnsconfig.js NewRegistrar("none", "NONE") can be simplified to NewRegistrar("none") (See https://docs.dnscontrol.org/commands/creds-json#cleanup)
******************** Domain: m0vfc.com
1 correction (mythic_rmc)
#1: + CREATE testchange.m0vfc.com A 2.2.2.2 ttl=300
+ CREATE testignore.m0vfc.com A 1.1.1.1 ttl=300
SUCCESS!
Done. 1 corrections.

Check that we get the expected records back from the MB API:

curl -H "Authorization: Bearer $($env:MBTOKEN)" https://api.mythic-beasts.com/dns/v2/zones/m0vfc.com/records
{"records":[{"data":"ns1.mythic-beasts.com.","host":"@","ttl":300,"type":"NS"},{"data":"ns2.mythic-beasts.com.","host":"@","ttl":300,"type":"NS"},{"data":"2.2.2.2","host":"testchange","ttl":300,"type":"A"},{"data":"1.1.1.1","host":"testignore","ttl":300,"type":"A"}]}

Change testignore to be IGNORE("testignore"), then push again with no change to testchange:

.\bin\dnscontrol.exe push --domains m0vfc.com
[INFO: Diff2 algorithm in use. Welcome to the future!]
INFO: In dnsconfig.js NewRegistrar("none", "NONE") can be simplified to NewRegistrar("none") (See https://docs.dnscontrol.org/commands/creds-json#cleanup)
******************** Domain: m0vfc.com
Done. 0 corrections.

Check we still get both records back from the MB API (yep):

curl -H "Authorization: Bearer $($env:MBTOKEN)" https://api.mythic-beasts.com/dns/v2/zones/m0vfc.com/records
{"records":[{"data":"ns1.mythic-beasts.com.","host":"@","ttl":300,"type":"NS"},{"data":"ns2.mythic-beasts.com.","host":"@","ttl":300,"type":"NS"},{"data":"2.2.2.2","host":"testchange","ttl":300,"type":"A"},{"data":"1.1.1.1","host":"testignore","ttl":300,"type":"A"}]}

Now change testchange:

.\bin\dnscontrol.exe push --domains m0vfc.com
[INFO: Diff2 algorithm in use. Welcome to the future!]
INFO: In dnsconfig.js NewRegistrar("none", "NONE") can be simplified to NewRegistrar("none") (See https://docs.dnscontrol.org/commands/creds-json#cleanup)
******************** Domain: m0vfc.com
1 correction (mythic_rmc)
#1: 1 records not being deleted because of IGNORE*():
    testignore.m0vfc.com. A 1.1.1.1
± MODIFY testchange.m0vfc.com A (2.2.2.2 ttl=300) -> (3.3.3.3 ttl=300)
SUCCESS!
Done. 1 corrections.

And finally check that the IGNORE()'d record has vanished:

curl -H "Authorization: Bearer $($env:MBTOKEN)" https://api.mythic-beasts.com/dns/v2/zones/m0vfc.com/records
{"records":[{"data":"ns1.mythic-beasts.com.","host":"@","ttl":300,"type":"NS"},{"data":"ns2.mythic-beasts.com.","host":"@","ttl":300,"type":"NS"},{"data":"3.3.3.3","host":"testchange","ttl":300,"type":"A"}]}

(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!)

@tlimoncelli
Copy link
Contributor

tlimoncelli commented Dec 17, 2024 via email

@rmc47
Copy link
Author

rmc47 commented Dec 17, 2024

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:

GET https://api.mythic-beasts.com/dns/v2/zones/m0vfc.com/records HTTP/1.1
HTTP/1.1 200 OK
Date: Tue, 17 Dec 2024 23:34:34 GMT
Server: Apache/2.4.62 (Debian)
Vary: Accept-Encoding
Content-Type: text/html; charset=utf-8
X-Mythic-Backend: pyapi_pyapi_mer_a
X-Varnish: 201720068
Age: 0
Via: 1.1 varnish (Varnish/7.1)
X-Clacks-Overhead: GNU Terry Pratchett
X-Mythic-Frontend: www-mer-a.mythic-beasts.com
X-Mythic-Proxy: 93.93.129.174
Strict-Transport-Security: max-age=31536000
Content-Length: 112
Connection: keep-alive

@                      300 NS    ns1.mythic-beasts.com.
@                      300 NS    ns2.mythic-beasts.com.
PUT https://api.mythic-beasts.com/dns/v2/zones/m0vfc.com/records HTTP/1.1
Host: api.mythic-beasts.com
User-Agent: Go-http-client/1.1
Content-Length: 166
Accept: text/dns
Authorization: <redacted>
Content-Type: text/dns
Accept-Encoding: gzip

testignore.m0vfc.com.	300	IN	A	1.1.1.1
testchange.m0vfc.com.	300	IN	A	3.3.3.3
m0vfc.com.	300	IN	NS	ns1.mythic-beasts.com.
m0vfc.com.	300	IN	NS	ns2.mythic-beasts.com.
HTTP/1.1 200 OK
Date: Tue, 17 Dec 2024 23:34:34 GMT
Server: Apache/2.4.62 (Debian)
Content-Length: 68
Content-Type: application/json
X-Mythic-Backend: pyapi_pyapi_cam_a
X-Varnish: 201720070
Age: 0
Via: 1.1 varnish (Varnish/7.1)
X-Clacks-Overhead: GNU Terry Pratchett
X-Mythic-Frontend: www-mer-a.mythic-beasts.com
X-Mythic-Proxy: 93.93.129.174
Strict-Transport-Security: max-age=31536000
Connection: keep-alive

{"message":"4 records added","records_added":4,"records_removed":2}

Change first record to ignore, push again:

GET https://api.mythic-beasts.com/dns/v2/zones/m0vfc.com/records HTTP/1.1
HTTP/1.1 200 OK
Date: Tue, 17 Dec 2024 23:37:02 GMT
Server: Apache/2.4.62 (Debian)
Vary: Accept-Encoding
Content-Type: text/html; charset=utf-8
X-Mythic-Backend: pyapi_pyapi_cam_a
X-Varnish: 200293866
Age: 0
Via: 1.1 varnish (Varnish/7.1)
X-Clacks-Overhead: GNU Terry Pratchett
X-Mythic-Frontend: www-mer-a.mythic-beasts.com
X-Mythic-Proxy: 46.235.225.189
Strict-Transport-Security: max-age=31536000
Connection: keep-alive
Content-Length: 194

@                      300 NS    ns1.mythic-beasts.com.
@                      300 NS    ns2.mythic-beasts.com.
testchange             300 A     3.3.3.3
testignore             300 A     1.1.1.1

(No modifications made, so no PUT call)

Change second record, leaving first as ignore:

GET https://api.mythic-beasts.com/dns/v2/zones/m0vfc.com/records HTTP/1.1
HTTP/1.1 200 OK
Date: Tue, 17 Dec 2024 23:37:47 GMT
Server: Apache/2.4.62 (Debian)
Vary: Accept-Encoding
Content-Type: text/html; charset=utf-8
X-Mythic-Backend: pyapi_pyapi_cam_a
X-Varnish: 195104553
Age: 0
Via: 1.1 varnish (Varnish/7.1)
X-Clacks-Overhead: GNU Terry Pratchett
X-Mythic-Frontend: www-mer-a.mythic-beasts.com
X-Mythic-Proxy: 46.235.225.189
Strict-Transport-Security: max-age=31536000
Content-Length: 194
Connection: keep-alive

@                      300 NS    ns1.mythic-beasts.com.
@                      300 NS    ns2.mythic-beasts.com.
testchange             300 A     3.3.3.3
testignore             300 A     1.1.1.1
PUT https://api.mythic-beasts.com/dns/v2/zones/m0vfc.com/records HTTP/1.1
Host: api.mythic-beasts.com
User-Agent: Go-http-client/1.1
Content-Length: 127
Accept: text/dns
Authorization: <redacted>
Content-Type: text/dns
Accept-Encoding: gzip

testchange.m0vfc.com.	300	IN	A	3.3.3.2
m0vfc.com.	300	IN	NS	ns1.mythic-beasts.com.
m0vfc.com.	300	IN	NS	ns2.mythic-beasts.com.
HTTP/1.1 200 OK
Date: Tue, 17 Dec 2024 23:37:48 GMT
Server: Apache/2.4.62 (Debian)
Content-Length: 68
Content-Type: application/json
X-Mythic-Backend: pyapi_pyapi_mer_a
X-Varnish: 195104555
Age: 0
Via: 1.1 varnish (Varnish/7.1)
X-Clacks-Overhead: GNU Terry Pratchett
X-Mythic-Frontend: www-mer-a.mythic-beasts.com
X-Mythic-Proxy: 46.235.225.189
Strict-Transport-Security: max-age=31536000
Connection: keep-alive

{"message":"3 records added","records_added":3,"records_removed":4}

@tlimoncelli
Copy link
Contributor

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.

@rmc47
Copy link
Author

rmc47 commented Dec 18, 2024

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).

@tlimoncelli
Copy link
Contributor

Oh you star, thanks Tom!

Thanks!

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).

Yes, I'd love Mythic to join the ranks of the automatically tested. Instructions are here: https://docs.dnscontrol.org/developer-info/byo-secrets

@rmc47
Copy link
Author

rmc47 commented Dec 18, 2024

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 :-).

@tomfitzhenry
Copy link
Contributor

tomfitzhenry commented Dec 18, 2024

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?

@rmc47
Copy link
Author

rmc47 commented Dec 18, 2024

Are you interested in being the dnscontrol Mythic Beasts maintainer?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?

@tlimoncelli
Copy link
Contributor

I've merged the change but I'm leaving this open until I hear from you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants