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

CLOUDFLAREAPI: Enable CanUseLOC #2799

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ConnorMcF
Copy link

Enables LOC records with the Cloudflare provider for #2798.

Draft PR as this currently doesn't work.

@tlimoncelli
Copy link
Contributor

Some advice: Update createRecDiff2() in rest.go to include LOC records. Create a function called cfLocData() similar to how SRV records are handled.

@tlimoncelli
Copy link
Contributor

CC @systemcrash

LocOrigAltitude int32 `json:"locorigaltitude,omitempty"`
LocOrigSize float32 `json:"locorigsize,omitempty"`
LocOrigHorizPre float32 `json:"locorighorizpre,omitempty"`
LocOrigVertPre float32 `json:"locorigvertpre,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this provider need new fields when multiple providers already support LOC without needing the extra fields?

Copy link
Author

@ConnorMcF ConnorMcF Jan 16, 2024

Choose a reason for hiding this comment

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

The Cloudflare API requires the original degrees, minutes, seconds values whereas to my knowledge the LOC model converts to decimal degrees, which doesn't fit into the request data Cloudflare asks for.
I'm trying to pass through both the DD and the original DMS values to the provider to allow either to be used.

https://developers.cloudflare.com/api/operations/dns-records-for-a-zone-create-dns-record (Body -> LOC Record)

I may be wrong here, or there may be a better way about this - but this is just what I've come to so far.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! So if I understand it correctly: Some of the fields are what the user input. Those inputs are then massaged and stored as another set of fields.

If that's the situation, then please be clear in the comments which are the user-input fields and which are the derived fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to have RecordConfig contain a field called LOC which is a pointer to a LOCConfig. All those fields would be moved to LOCConfig. That would conserve memory. (You don't have to make this change right now. First let's see if the extra RAM causes problems. If we really want to save memory, we should come up with a solution that works for Naptr, Loc, and other fields too.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Tim. I recall implementing LOC for what was necessary in the final result. I referenced the original RFC. There are several ways to write coordinates, with a few which are commonly accepted everywhere. I tried handling the common ones in the js import, so there might need to be a bit of work there also. So be mindful of the import process done via the Javascript files (and test coverage) if those routes are necessary.

Might not be a bad idea to pass through components from the original js config.

Copy link
Contributor

Choose a reason for hiding this comment

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

Refer to my original commit or PR to find the js bits. Good to see Cloud Flare filling out their portfolio with LOC. 🥳

LocLatDirection string `json:"loclatdirection,omitempty"`
LocLongDegrees uint8 `json:"loclongdegrees,omitempty"`
LocLongMinutes uint8 `json:"lotlongminutes,omitempty"`
LocLongSeconds float32 `json:"loclongseconds,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I couldn't see the LOC records at cloud flare link. So we assume only seconds will take decimal fractions ie floats. Degrees and minutes are whole integers.

@tlimoncelli
Copy link
Contributor

I've done some research and now I think I can give a (hopefully!) more useful recommendation.

Rather than derive those fields and store them in RecordConfig, those fields should be derived "on demand" when needed. That is, the cloudflare createRecDiff2() function should generate those additional fields.

This would have a few benefits:

  1. It doesn't increase the memory size of RecordConfig.
  2. It better supports other providers, who may have a different set of fields they need.
  3. It maintains the ideal that RecordConfig is the desired state, and each provider does what's needed for their native implementation. For example, TXT records are stored as one big string. If a provider wants them as 255-octet chunks, it's their responsibility to do the chunking and un-chunking behind the scenes. This converstion is usually done in functions with names like toNative() and toRC(). In Cloudflare's case, the conversion isn't isolated but it part of createRecDiff2() and other functions.
  4. This won't require additional CPU, and might require less CPU since the conversion will only be done if an LOC record changes.
  5. If there are helper functions that will be useful to many providers, put them in t_loc.go.

Hope that helps,
Tom

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

Successfully merging this pull request may close these issues.

None yet

4 participants