-
Notifications
You must be signed in to change notification settings - Fork 402
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
Add a TTL model #2475
base: main
Are you sure you want to change the base?
Add a TTL model #2475
Conversation
With this change, I am able to fully resolve #2440 by adding an explicit It will likely be necessary for providers to be checked for previously-hidden assumptions about the value 0; before when parsing this from the provider into |
if rc.TTL != 0 { | ||
ttl := int64(rc.TTL) | ||
if rc.TTL.Value() != 0 { | ||
ttl := int64(rc.TTL.Value()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example of where a provider was explicitly checking for 0, in a place where it could never be reached (as dnscontrol
always replaced 0
with models.DefaultTTL
before reaching this point). I presumed that this check was intentional and thus left it in place (as it is now triggerable).
@@ -277,7 +277,7 @@ func toReq(dc *models.DomainConfig, rc *models.RecordConfig) (*recordEditRequest | |||
Type: rc.Type, | |||
Name: rc.GetLabel(), | |||
Target: rc.GetTargetField(), | |||
TTL: int(rc.TTL), | |||
TTL: int(rc.TTL.Value()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what fully fixes #2440:
- When no TTL is set, this evaluates to
models.DefaultTTL
(300). - When
DefaultTTL(0)
is used explicitly, this evaluates to 0.
Hi there! I'm on vacation so please expect delays. Thank you for taking the time to work on this issue. The proposed change is pretty fundamental and will require extensive discussion and testing. I have a number of over-arching concerns:
Code-specific suggestions:
|
Yep, as expected! I will say that I'm not claiming this PR is the optimal approach (I am not a Go or JS developer, and have fumbled my way into something that compiles and works with a specific provider), but my hope is that it helps us to coalesce around a viable approach.
In this PR I didn't do that because I was trying to preserve If the global As an aside, my first local attempt at this PR did not use pointers, instead opting for an
I personally don't particularly care whether you want to support a default TTL or not. My own issue is "solved" by this PR, in that I'm wanting to move my DNS settings away from Linode, and all I wanted was to confirm that my However, there doesn't appear to be a clear decision one way or the other regarding default TTLs, and the implicit decision to map
Done.
IDK, I can go with whatever is standard Go naming convention here. In Rust we use
Thanks, I couldn't figure out how to run the tests. It took me a bit to figure out that I actually had to set an environment variable to get this to run:
And the test failure was because Force-pushed with changes 1 and 3. |
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.
e0b50a5
to
ed1bf34
Compare
I appreciate all the effort you've put into this. While this solution will work, I think there are other solutions that are more simple that will also work. I always favor the more simple solution. I don't think we can solve this globally until first we decide how we want TTLs to work. Most of the TTL-handling code was written in 2016-ish when DNSControl had fewer formal design specifications (translation: we were guessing our way through most design decisions and a lot of specifics were just implemented without a formal design). As a result, each provider handles things slightly differently. Eventually we should do an RFC and get feedback from the community about how TTLs should work, get consensus, then implement a solution based on that feedback. In the meanwhile, I think we should fix this just for Linode rather than doing a global change that affects nearly all providers. Therefore, I'm asking you to start a different PR that fixes this for Linode only. Thanks for understanding. |
@tlimoncelli If you know of a simpler solution, I am all ears! The core problem is the aliasing everywhere of Lines 100 to 109 in af91e37
Lines 251 to 257 in af91e37
Lines 1048 to 1051 in af91e37
Lines 222 to 228 in af91e37
The Go model Lines 86 to 92 in af91e37
Additionally, individual providers never see dnscontrol/pkg/normalize/validate.go Lines 348 to 354 in af91e37
Delaying or changing this normalization would require changes to every single provider to handle the fact that their assumptions about default TTL handling have been broken. So I can't alter that either if I am to satisfy your request. As a result of the above two implementation details, there is currently no way for the Linode provider to distinguish these three cases:
I'll be honest, I do not see a simpler solution than "change the type of
If this approach is along the lines of what you were thinking, then I'll go ahead and implement it. And if there is some aspect of |
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 #2444.