tlimoncelli
commented
1 year ago willpower232 I'd be glad to help.
Please include -v and -verbose for more verbosity (go test -v -verbose -provider NAMECHEAP -diff2).
Closed tlimoncelli closed 1 year ago
tlimoncelli
commented
1 year ago willpower232 I'd be glad to help.
Please include -v and -verbose for more verbosity (go test -v -verbose -provider NAMECHEAP -diff2).
willpower232
commented
1 year ago Ah I did not realise it was --diff2 for dnscontrol and -diff2 for tests but it seems the failures are still consistent.
=== RUN TestDNSProviders/willpower232testsdnscontrol.com/04:MX:Create_MX
integration_test.go:224:
GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
+ CREATE MX testmx.willpower232testsdnscontrol.com 5 foo.com. ttl=300]
integration_test.go:243: Expected 0 corrections on second run, but found 1.
integration_test.go:245: UNEXPECTED #0: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
± MODIFY MX testmx.willpower232testsdnscontrol.com: (5 ttl=300) -> (5 foo.com. ttl=300)]=== RUN TestDNSProviders/willpower232testsdnscontrol.com/07:manyTypesAtOnce:CreateManyTypesAtLabel
integration_test.go:224:
GENERATE_ZONE: willpower232testsdnscontrol.com (3 records)[
+ CREATE MX testmx.willpower232testsdnscontrol.com 100 bar.com. ttl=300
+ CREATE MX testmx.willpower232testsdnscontrol.com 5 foo.com. ttl=300
+ CREATE A www.willpower232testsdnscontrol.com 1.1.1.1 ttl=300]
integration_test.go:243: Expected 0 corrections on second run, but found 1.
integration_test.go:245: UNEXPECTED #0: GENERATE_ZONE: willpower232testsdnscontrol.com (3 records)[
± MODIFY MX testmx.willpower232testsdnscontrol.com: (100 ttl=300) -> (100 bar.com. ttl=300)
± MODIFY MX testmx.willpower232testsdnscontrol.com: (5 ttl=300) -> (5 foo.com. ttl=300)]=== RUN TestDNSProviders/willpower232testsdnscontrol.com/14:MX:Record_pointing_to_@
integration_test.go:224:
GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
+ CREATE MX foo.willpower232testsdnscontrol.com 8 willpower232testsdnscontrol.com. ttl=300]
integration_test.go:243: Expected 0 corrections on second run, but found 1.
integration_test.go:245: UNEXPECTED #0: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
± MODIFY MX foo.willpower232testsdnscontrol.com: (8 ttl=300) -> (8 willpower232testsdnscontrol.c=== RUN TestDNSProviders/willpower232testsdnscontrol.com/17:Case_Sensitivity:Create_CAPS
integration_test.go:224:
GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
+ CREATE MX bar.willpower232testsdnscontrol.com 5 bar.com. ttl=300]
integration_test.go:243: Expected 0 corrections on second run, but found 1.
integration_test.go:245: UNEXPECTED #0: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
± MODIFY MX bar.willpower232testsdnscontrol.com: (5 ttl=300) -> (5 bar.com. ttl=300)]=== RUN TestDNSProviders/willpower232testsdnscontrol.com/19:testByRecordSet:initial
integration_test.go:224:
GENERATE_ZONE: willpower232testsdnscontrol.com (5 records)[
+ CREATE A bar.willpower232testsdnscontrol.com 1.2.3.4 ttl=300
+ CREATE A foo.willpower232testsdnscontrol.com 2.3.4.5 ttl=300
+ CREATE A foo.willpower232testsdnscontrol.com 3.4.5.6 ttl=300
+ CREATE MX foo.willpower232testsdnscontrol.com 10 foo.willpower232testsdnscontrol.com. ttl=300
+ CREATE MX foo.willpower232testsdnscontrol.com 20 bar.willpower232testsdnscontrol.com. ttl=300]
integration_test.go:243: Expected 0 corrections on second run, but found 1.
integration_test.go:245: UNEXPECTED #0: GENERATE_ZONE: willpower232testsdnscontrol.com (5 records)[
± MODIFY MX foo.willpower232testsdnscontrol.com: (10 ttl=300) -> (10 foo.willpower232testsdnscontrol.com. ttl=300)
± MODIFY MX foo.willpower232testsdnscontrol.com: (20 ttl=300) -> (20 bar.willpower232testsdnscontrol.com. ttl=300)]Judging by those double spaces, it seems to fail to create (and cleanup) MX records, does that make sense with what got updated?
tlimoncelli
commented
1 year ago Ah I did not realise it was
--diff2fordnscontroland-diff2for tests but it seems the failures are still consistent.
Yeah, native Go utilities (go test) use a single hyphen because Rob Pike doesn't like double-hyphens (I can tell you more
Unix history if you are interested). dnscontrol uses double-hyphens.
4
=== RUN TestDNSProviders/willpower232testsdnscontrol.com/04:MX:Create_MX integration_test.go:224: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[ + CREATE MX testmx.willpower232testsdnscontrol.com 5 foo.com. ttl=300] integration_test.go:243: Expected 0 corrections on second run, but found 1. integration_test.go:245: UNEXPECTED #0: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[ ± MODIFY MX testmx.willpower232testsdnscontrol.com: (5 ttl=300) -> (5 foo.com. ttl=300)]Judging by those double spaces, it seems to fail to create (and cleanup) MX records, does that make sense with what got updated?
Yes, I agree with your analysis. My guess is that:
record.PopulateFromString(dnsHost.Type, dnsHost.Address, origin)should first check to see if it is an MX record and handle it as a special case. The comments in models/t_parse.go explain a recommended pattern. models/t_mx.go has some helper functions that you might find useful.
Tom
willpower232
commented
1 year ago I referenced some of the other providers and found a resolution as you described
switch dnsHost.Type {
case "MX":
record.SetTargetMX(uint16(dnsHost.MXPref), dnsHost.Address)
default:
record.PopulateFromString(dnsHost.Type, dnsHost.Address, origin)
}this did however re-introduce the previous fail
=== RUN TestDNSProviders/willpower232testsdnscontrol.com/04:MX:Change_MX_p
integration_test.go:224:
GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
± MODIFY MX testmx.willpower232testsdnscontrol.com: (5 bar.com. ttl=300) -> (100 bar.com. ttl=300)]
integration_test.go:243: Expected 0 corrections on second run, but found 1.
integration_test.go:245: UNEXPECTED #0: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
± MODIFY MX testmx.willpower232testsdnscontrol.com: (5 bar.com. ttl=300) -> (100 bar.com. ttl=300)]I think the issue might be that only the namecheap provider is making use of .MXPref, does that make sense? Is there are more appropriate property?
tlimoncelli
commented
1 year ago You're in the right direction! .MXPref is a field in a NameCheap struct. The DNSControl equivalent is models.RecordConfig.MxPreference. toRecords() converts namecheap native records to models.RecordConfig; generateRecords() goes the other direction. Both seem to do something with MX preferences.. maybe not the right thing?
It looks like the record is not being created properly. That would indicate a problem with generateRecords()
The way that the integration tests work is first they run the equivalent of "dnscontrol push" to make the desired change. Then it runs the equivalent of "dnscontrol preview" with the exact same parameters. The "preview" should show no changes. i.e. if the first one worked, the second one shouldn't have any work to do. It repeats that for each tc().
When you see "Expected 0 corrections on second run, but found 1." that means the next line is what change the 2nd run (the preview) output. In this case, its trying to make the exact same change as the push. That means the push didn't happen correctly.
So, you're properly creating MX records and changing the target. That is, changing the MX target from foo.com. to bar.com. worked. Its only changing the preference that failed.
On a whim, I'd try the following change in generateRecords():
case "CAA":case "CAA", "MX":I don't think that will work, but its worth a try.
If that doesn't work, revert the change and add some fmt.Printf's to generateRecords() and look at the value of rec.MXPref.
willpower232
commented
1 year ago I have added several printf's and verified that absolutely nothing is wrong with dnscontrol.
Thankfully namecheaps api supports GET as well as POST so I have been able to access it directly and confirm that changing an MX priority without changing another aspect of the record causes the change to not happen so the test failure is actually indicative of the real world!
Are you able to add the SetTargetMX yourself or would you like a PR or patch of some variety?
I'll open a ticket to namecheap in the meantime...
tlimoncelli
commented
1 year ago Wow! A real bug in the API! That's actually kind of rare!
Are you able to add the SetTargetMX yourself or would you like a PR or patch of some variety?
Please send a PR.
We can handle this a few ways:
Its up to you.
willpower232
commented
1 year ago Namecheap actually replied super fast which was nice. It seems that a "required" additional parameter called EmailType is only needed for this purpose and the request does not fail if it is missing.
Thankfully the library that is used in the namecheap provider is already aware of this
From what I can see, dnscontrol has the "latest" version of the library so is it possible that the type isn't being set correctly? I'm not sure how to fmt.Printf a third party library in go :sweat_smile:
willpower232
commented
1 year ago OK good news, it is definitely the API that is broken so I'll put that to one side for a minute whilst negotiating with namecheap.
Bad news, more tests have started failing since recent commits in the tlim_corrector branch.
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/14:MX:Null_MX (0.57s)
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/16:complex_TXT:TXT_with_1_double-quotes (1.02s)
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/25:CAA:CAA_change_flag (0.82s)Does this make sense or do you need more information?
Here is more information anyway
=== RUN TestDNSProviders/willpower232testsdnscontrol.com/14:MX:Null_MX
integration_test.go:230:
GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
+ CREATE MX willpower232testsdnscontrol.com 0 . ttl=300
- DELETE MX foo.willpower232testsdnscontrol.com 8 willpower232testsdnscontrol.com. ttl=300]
integration_test.go:234: Error 2050900: INVALID_ADDR : '.' should not be an IP/ URL for MX record.(host name: @)=== RUN TestDNSProviders/willpower232testsdnscontrol.com/16:complex_TXT:TXT_with_1_double-quotes
integration_test.go:230:
GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
+ CREATE TXT foodq.willpower232testsdnscontrol.com "quo\"te" ttl=300
- DELETE TXT foobt.willpower232testsdnscontrol.com "blah`blah" ttl=300]
integration_test.go:249: Expected 0 corrections on second run, but found 1.
integration_test.go:251: UNEXPECTED #0: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
± MODIFY TXT foodq.willpower232testsdnscontrol.com: ("quote" ttl=300) -> ("quo\"te" ttl=300)]=== RUN TestDNSProviders/willpower232testsdnscontrol.com/25:CAA:CAA_change_flag
integration_test.go:230:
GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
± MODIFY CAA willpower232testsdnscontrol.com: (0 issuewild "example.com" ttl=300) -> (128 issuewild "example.com" ttl=300)]
integration_test.go:249: Expected 0 corrections on second run, but found 1.
integration_test.go:251: UNEXPECTED #0: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[
± MODIFY CAA willpower232testsdnscontrol.com: (0 issuewild "example.com" ttl=300) -> (128 issuewild "example.com" ttl=300)]From what I can see, dnscontrol has the "latest" version of the library so is it possible that the type isn't being set correctly? I'm not sure how to fmt.Printf a third party library in go 😅
Modifying other libraries is possible but I'm not sure how. I think you clone it locally then edit go.mod to point to the local disk instead. I haven't tried.
The code is triggered by h.Type == "MX" ... can you verify that is getting set correctly?
tlimoncelli
commented
1 year ago OK good news, it is definitely the API that is broken so I'll put that to one side for a minute whilst negotiating with namecheap.
Bad news, more tests have started failing since recent commits in the tlim_corrector branch.
Glad to find these problems now instead of after release! :)
--- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/14:MX:Null_MX (0.57s) --- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/16:complex_TXT:TXT_with_1_double-quotes (1.02s) --- FAIL: TestDNSProviders/willpower232testsdnscontrol.com/25:CAA:CAA_change_flag (0.82s)Does this make sense or do you need more information?
Here is more information anyway
14
=== RUN TestDNSProviders/willpower232testsdnscontrol.com/14:MX:Null_MX integration_test.go:230: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[ + CREATE MX willpower232testsdnscontrol.com 0 . ttl=300 - DELETE MX foo.willpower232testsdnscontrol.com 8 willpower232testsdnscontrol.com. ttl=300] integration_test.go:234: Error 2050900: INVALID_ADDR : '.' should not be an IP/ URL for MX record.(host name: @)
Looks like the API doesn't permit an MX record pointing at .. It's easy to denote that this isn't allowed. Look at providers/loopia/auditrecords.go for an example of a.Add("MX", rejectif.MxNull)
16
=== RUN TestDNSProviders/willpower232testsdnscontrol.com/16:complex_TXT:TXT_with_1_double-quotes integration_test.go:230: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[ + CREATE TXT foodq.willpower232testsdnscontrol.com "quo\"te" ttl=300 - DELETE TXT foobt.willpower232testsdnscontrol.com "blah`blah" ttl=300] integration_test.go:249: Expected 0 corrections on second run, but found 1. integration_test.go:251: UNEXPECTED #0: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[ ± MODIFY TXT foodq.willpower232testsdnscontrol.com: ("quote" ttl=300) -> ("quo\"te" ttl=300)]
My guess? " isn't permitted in TXT records or they have to be escaped/quoted in a way that the API understands. I've never seen anyone actually put a double-quote in a TXT record outside of test cases. So, the easy route is to add auditrecords.go to add a.Add("TXT", rejectif.TxtHasDoubleQuotes) (See providers/msdns/auditrecords.go as an example)
25
=== RUN TestDNSProviders/willpower232testsdnscontrol.com/25:CAA:CAA_change_flag integration_test.go:230: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[ ± MODIFY CAA willpower232testsdnscontrol.com: (0 issuewild "example.com" ttl=300) -> (128 issuewild "example.com" ttl=300)] integration_test.go:249: Expected 0 corrections on second run, but found 1. integration_test.go:251: UNEXPECTED #0: GENERATE_ZONE: willpower232testsdnscontrol.com (1 records)[ ± MODIFY CAA willpower232testsdnscontrol.com: (0 issuewild "example.com" ttl=300) -> (128 issuewild "example.com" ttl=300)]
Sounds like updating just the first field is broken. It might be a problem with modifying the record, but it could be a problem with creating the record to: The test that works assigns a 0 to that field. Since 0 is the default value for that struct, it might be not working but we can't tell. (If that makes sense!)
Hope that helps!
Tom
willpower232
commented
1 year ago Well it passes 14 and 16 now but I think I have to put 25 under the category of what does namecheap think it is doing as its API (and UI) also refuses to accept the possibility of changing the byte flag in a CAA record.
I suggest you merge the PR and I'll continue to very very very slowly wake up namecheap support.
Whether you want to keep this issue open or not is up to you, I'm not very hopeful of getting anything useful out of namecheap to be honest.
tlimoncelli
commented
1 year ago Sounds good. Would you mind opening a new issue with the remaining work and closing this?
willpower232
commented
1 year ago I started a new issue but I can't close this one as it is not mine to close :sweat_smile:
These tests still fail