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

CLOUDNS: add support for LOC records #3127

Merged
merged 2 commits into from
Oct 5, 2024

Conversation

hmoffatt
Copy link
Contributor

This PR adds support for NAPTR and LOC records on CLOUDNS.

@tlimoncelli
Copy link
Contributor

Thanks!

@tlimoncelli
Copy link
Contributor

Please add a comment with the output of the integration tests (redact as needed)

case "NAPTR":
rc.SetTargetNAPTRStrings(r.NaptrOrder, r.NaptrPref, r.NaptrFlag, r.NaptrParams, r.NaptrRegexp, r.NaptrReplace+".")
case "LOC":
loc := fmt.Sprintf("%s %s %s %s %s %s %s %s %s %s %s %s",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part should be redundant now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think specifically the part that strips the decimal part of altitude is redundant? The rest has to remain as I need to assemble the LOC record from the individual API parts.

Better to call calculateLOCFields directly, but then I would have to duplicate the string to int/float conversions as the API returns strings for everything, despite being JSON.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to call calculateLOCFields directly, but then I would have to duplicate the string to int/float conversions as the API returns strings for everything, despite being JSON.

🤦

I don't know what to say. But coercing as early as possible is safest, especially when working with numbers and maths.

@hmoffatt
Copy link
Contributor Author

Oops, I forgot the unit tests. This showed up an interesting quirk with CLOUDNS's handling of the size/precision fields - it quietly substitutes its own default values if you pass "0.00", even though those are documented as valid! I've put in a workaround.

@hmoffatt hmoffatt marked this pull request as draft September 25, 2024 01:58
@hmoffatt
Copy link
Contributor Author

All tests are passing now except the NAPTR test. The CLOUDNS API is rejecting records with both regexp and replacement set, which the test cases use. I couldn't find anywhere in the RFC that explicitly said this wasn't allowed, but none of the examples use both fields.

=== RUN   TestDNSProviders/dnscontrol-test.dev/35:NAPTR:NAPTR_record
    integration_test.go:248: 
        + CREATE NAPTR test.dnscontrol-test.dev 100 10 "U" "E2U+sip" "!^.*$!sip:customer-service@example.com!" example.foo.com. ttl=300
    integration_test.go:253: failed create record (ClouDNS): ClouDNS API error: You need to enter either Regular Expression or Replacement, not both

@hmoffatt hmoffatt force-pushed the cloudns-new-records branch 2 times, most recently from 0a88d4d to 3e795b4 Compare September 25, 2024 08:32
@systemcrash
Copy link
Contributor

Oops, I forgot the unit tests. This showed up an interesting quirk with CLOUDNS's handling of the size/precision fields - it quietly substitutes its own default values if you pass "0.00", even though those are documented as valid! I've put in a workaround.

Can you supply nothing?

helpers.js assumes 0 if the data source is DMS or DMM using a domain.js. But no assumptions or changes are made for records already deployed in a domain.

@systemcrash
Copy link
Contributor

systemcrash commented Sep 25, 2024

All tests are passing now except the NAPTR test. The CLOUDNS API is rejecting records with both regexp and replacement set, which the test cases use. I couldn't find anywhere in the RFC that explicitly said this wasn't allowed, but none of the examples use both fields.

=== RUN   TestDNSProviders/dnscontrol-test.dev/35:NAPTR:NAPTR_record
    integration_test.go:248: 
        + CREATE NAPTR test.dnscontrol-test.dev 100 10 "U" "E2U+sip" "!^.*$!sip:customer-service@example.com!" example.foo.com. ttl=300
    integration_test.go:253: failed create record (ClouDNS): ClouDNS API error: You need to enter either Regular Expression or Replacement, not both

Seems pretty wild. Both are necessary for a functioning NAPTR pointer. Flag an error with their support. Edit: actually, the regexp can do both. But which format don't they like from those available here?

https://docs.dnscontrol.org/language-reference/domain-modifiers/naptr

Recommend that you separate out NAPTR changes to a separate PR if you think this could take a while to resolve.

@hmoffatt
Copy link
Contributor Author

Can you supply nothing?

It turns out that the weird behaviour with "0.00" also applies to the altitude field. If I supply nothing, I get the default of 10,000m. 😕

@hmoffatt
Copy link
Contributor Author

Recommend that you separate out NAPTR changes to a separate PR if you think this could take a while to resolve.

Done. Once your #3130 is merged I will submit this PR.

@hmoffatt hmoffatt changed the title CLOUDNS: add support for NAPTR and LOC records CLOUDNS: add support for LOC records Sep 27, 2024
@hmoffatt
Copy link
Contributor Author

I've stripped out the NAPTR code for now, and updated this for @systemcrash 's decimal LOC changes.
cloudns-test.log

All the unit tests are passing, so I think this is ready to go! Thanks all for review and assistance.

@hmoffatt hmoffatt marked this pull request as ready for review September 27, 2024 23:30
@systemcrash
Copy link
Contributor

systemcrash commented Sep 28, 2024 via email

@hmoffatt
Copy link
Contributor Author

hmoffatt commented Oct 4, 2024

Hi @tlimoncelli I think this is ready to go now. Thanks.

@tlimoncelli
Copy link
Contributor

Thanks!

@tlimoncelli tlimoncelli merged commit d7f4d0e into StackExchange:main Oct 5, 2024
2 checks passed
@systemcrash
Copy link
Contributor

@hmoffatt have your changes proved beneficial?

@hmoffatt
Copy link
Contributor Author

hmoffatt commented Oct 8, 2024

My interest in this was academic tbh, just learning my way around Go and the dnscontrol codebase.

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

Successfully merging this pull request may close these issues.

3 participants