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

DNSControl treats a configured TTL of 0 as "not configured" #2444

Open
str4d opened this issue Jun 17, 2023 · 5 comments · May be fixed by #2475
Open

DNSControl treats a configured TTL of 0 as "not configured" #2444

str4d opened this issue Jun 17, 2023 · 5 comments · May be fixed by #2475
Labels

Comments

@str4d
Copy link
Contributor

str4d commented Jun 17, 2023

While discussing the fact that Linode's API explicitly permits and returns a TTL of 0 to represent "Default", I asked in #2440 (comment):

The question though is whether dnscontrol has a good way to represent "Default" in its own UX. Do any other providers have a similar concept? How do they handle a TTL of 0?

It turns out that DNSControl completely ignores any DefaultTTL(0) or TTL(0) settings, causing it to instead fall back on the global DNSControl default of 300 seconds. AFAICT the code in pkg/js/helpers.js handles things correctly, but when the JSON is parsed into Go, the resulting RecordConfig has its TTL replaced by the hard-coded global default of 300:

dnscontrol/models/record.go

Lines 377 to 379 in 5b3bb31

if rc.TTL == 0 {
rr.Header().Ttl = DefaultTTL
}

This prevents the LINODE provider (or any other provider) from setting or preserving a TTL of 0 (as was theoretically enabled by #2442).

@str4d
Copy link
Contributor Author

str4d commented Jun 17, 2023

AFAICT the code in pkg/js/helpers.js handles things correctly,

This is probably actually just by accident. I think the core issue here is that 0 in the TTL field is being treated as "falsey" in helpers.js (while not present on the pathways I'm using, there are conditionals of the form if (r.TTL)), and the TTL field is fundamentally being treated as an optional non-zero unsigned integer, rather than an optional unsigned integer. The Go code above is just enforcing that at parse time.

Fixing this would require changing both the JavaScript and Go types to separately represent None and Some(0). On the JavaScript side that could probably be slipped in via e.g. using -1 to mean None, while on the Go side a proper Optional type would help.

@str4d
Copy link
Contributor Author

str4d commented Jun 18, 2023

These are all the spots in the Go code that map 0 to DefaultTTL:

dnscontrol/models/record.go

Lines 377 to 379 in 5b3bb31

if rc.TTL == 0 {
rr.Header().Ttl = DefaultTTL
}

if rec.TTL == 0 {
rec.TTL = models.DefaultTTL
}

if z.DefaultTTL == 0 {
z.DefaultTTL = 300
}

if z.DefaultTTL == 0 {
z.DefaultTTL = 300
}

Commenting out the first two is sufficient to invert the meaining of 0 within dnscontrol preview: an explicit DefaultTTL(0) or TTL(0) gets through the LINODE provider and the corrections disappear, but by contrast all the other domains with no TTL settings no longer take up the implicit TTL of 300, and I see corrections of the form MODIFY ttl=300 -> ttl=0.

@str4d
Copy link
Contributor Author

str4d commented Jun 18, 2023

So, there are a few directions this could go:

  • Implicit "provider-default TTL".
    • Use that default for that provider if it exists, instead of the global DNSControl default.
    • Users can explicitly mark it with TTL("default"), but doing so is unnecessary.
    • If the provider exposes this default through their API via whatever sentinel value, map that to something that can be set explicitly.
  • Explicit "provider-default TTL".
    • TTL("default") (or similar) lets users opt into the provider's default instead of the DNSControl default.
    • TTL(0) is supported only for providers that explicitly support it via their API.
  • Remove the non-zero restriction on TTL
    • TTL(0) is supported for any provider, and rely on each provider to raise an error if it is unsupported.
    • Probably still want to reject "0m" etc as that doesn't visually map well to 0 being used as a default sentinel.
  • Choose not to support a TTL value of 0, or any kind of "provider-default TTL", but still improve the UX of DNSControl.
    • Providers that can see this value be returned from their API would need to map it to the corresponding default value inside DNSControl
    • Make it an error to set TTL(0) or DefaultTTL(0) (instead of it being silently ignored).
    • In preview / push, providers would need to add extra logic to check whether a record's DNSControl-configured TTL happens to match the default, and the only correction would be to change the DSP's record from 0 to an explicit number; if so, suppress this correction.
      • Maybe also suppress this kind of correction even if something else changes, i.e. only toggle from "Default" to "explicit" via either having pushed a non-default TTL, or having done so through the provider's regular web portal.

@str4d
Copy link
Contributor Author

str4d commented Jun 18, 2023

Another useful UX thing before, or in addition to, the above: change the way colour highlighting works, to only highlight the changed parts instead of the whole MODIFY line. That would make it easier to determine what is actually being changed.

str4d added a commit to str4d/dnscontrol that referenced this issue Jul 10, 2023
This unifies the logic for handling the dnscontrol-default TTL, and
frees up the TTL value 0 for use by providers (e.g. Linode, which uses
it as its sentinel for its default TTL).

Closes StackExchange#2444.
str4d added a commit to str4d/dnscontrol that referenced this issue Jul 10, 2023
This unifies the logic for handling the dnscontrol-default TTL, and
frees up the TTL value 0 for use by providers (e.g. Linode, which uses
it as its sentinel for its default TTL).

Closes StackExchange#2444.
str4d added a commit to str4d/dnscontrol that referenced this issue Jul 10, 2023
This unifies the logic for handling the dnscontrol-default TTL, and
frees up the TTL value 0 for use by providers (e.g. Linode, which uses
it as its sentinel for its default TTL).

Closes StackExchange#2444.
str4d added a commit to str4d/dnscontrol that referenced this issue Jul 10, 2023
This unifies the logic for handling the dnscontrol-default TTL, and
frees up the TTL value 0 for use by providers (e.g. Linode, which uses
it as its sentinel for its default TTL).

Closes StackExchange#2444.
str4d added a commit to str4d/dnscontrol that referenced this issue Jul 10, 2023
This unifies the logic for handling the dnscontrol-default TTL, and
frees up the TTL value 0 for use by providers (e.g. Linode, which uses
it as its sentinel for its default TTL).

Closes StackExchange#2444.
str4d added a commit to str4d/dnscontrol that referenced this issue Jul 10, 2023
This unifies the logic for handling the dnscontrol-default TTL, and
frees up the TTL value 0 for use by providers (e.g. Linode, which uses
it as its sentinel for its default TTL).

Closes StackExchange#2444.
@str4d
Copy link
Contributor Author

str4d commented Jul 10, 2023

I implemented "Remove the non-zero restriction on TTL" in #2475, which AFAICT (as a non-Go developer) was the simplest approach.

str4d added a commit to str4d/dnscontrol that referenced this issue Jul 22, 2023
This unifies the logic for handling the dnscontrol-default TTL, and
frees up the TTL value 0 for use by providers (e.g. Linode, which uses
it as its sentinel for its default TTL).

Closes StackExchange#2444.
str4d added a commit to str4d/dnscontrol that referenced this issue Aug 28, 2023
This unifies the logic for handling the dnscontrol-default TTL, and
frees up the TTL value 0 for use by providers (e.g. Linode, which uses
it as its sentinel for its default TTL).

Closes StackExchange#2444.
str4d added a commit to str4d/dnscontrol that referenced this issue Sep 6, 2023
This unifies the logic for handling the dnscontrol-default TTL, and
frees up the TTL value 0 for use by providers (e.g. Linode, which uses
it as its sentinel for its default TTL).

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

Successfully merging a pull request may close this issue.

2 participants