-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Google Provider Add Ttl from annotations #389
Conversation
82d0009
to
48bdc62
Compare
@@ -33,6 +33,10 @@ import ( | |||
"github.com/kubernetes-incubator/external-dns/plan" | |||
) | |||
|
|||
const ( | |||
googleRecordTTL = 300 |
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.
Much better than the hardcoded value 300, but do we have a default in the GoogleDNS zone already?
I thought about having no TTL set if there is no TTL specified, such that you can use the default in the origin configuration, which can be adjusted centrally, but this constant not.
I am fine with the PR, so we can also do it later, but it would be nice in general to have it set by the DNS zone itself, which is already the default TTL.
What do you think?
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.
I don't know of a way to configure default TTL in the Cloud DNS zone. Do you know how to do this?
I tried omitting Ttl:
return &dns.ResourceRecordSet{
Name: ensureTrailingDot(ep.DNSName),
Rrdatas: []string{target},
// Ttl: ttl,
Type: ep.RecordType,
}
The resulting DNS records in gcloud have 0-value TTL's:
NAME TYPE TTL DATA
rightdns.com. NS 21600 ns-cloud-b1.googledomains.com.,ns-cloud-b2.googledomains.com.,ns-cloud-b3.googledomains.com.,ns-cloud-b4.googledomains.com.
rightdns.com. SOA 21600 ns-cloud-b1.googledomains.com. cloud-dns-hostmaster.google.com. 1 21600 3600 259200 300
nginx.rightdns.com. A 0 10.128.0.7
nginx.rightdns.com. TXT 0 "heritage=external-dns,external-dns/owner=external-dns"
nginx2.rightdns.com. A 0 10.128.0.7
nginx2.rightdns.com. TXT 0 "heritage=external-dns,external-dns/owner=external-dns"
In the future, I believe we should be able to define an overrideable global default TTL for all endpoints in external-dns as well as min and max.
endpoint.TTL.isConfigured()
would probably need to be refactored as well.
This is out of scope as it would affect all providers.
For now, I prefer the explicit behavior in this PR.
It matches the AWS provider implementation: {code}
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.
ACK. Thanks for your test and detailed comment!
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.
Further discussion about this functionality in #390
👍 @linki @ideahitme please merge, as discussed with @stealthybox in slack, I am fine with the current PR. |
Added changelog -- thanks @szuecs |
4e680f6
to
7a607a1
Compare
Use `int64(ep.RecordTTL)` in `newRecord()` Fallback to hardcoded 300s for backwards-compat Add `TestNewRecords()` Add notes in *ttl.md*
7a607a1
to
65f4239
Compare
/lgtm |
Google Provider Add Ttl from annotations
Use
int64(ep.RecordTTL)
innewRecord()
Fallback to hardcoded 300s for backwards-compat
Add
TestNewRecords()
Add notes in ttl.md