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

Google Provider Add Ttl from annotations #389

Merged
merged 1 commit into from
Nov 17, 2017

Conversation

stealthybox
Copy link
Contributor

Use int64(ep.RecordTTL) in newRecord()
Fallback to hardcoded 300s for backwards-compat
Add TestNewRecords()
Add notes in ttl.md

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 14, 2017
@stealthybox stealthybox changed the title provider=google: Add Ttl from annotations: Google Provider Add Ttl from annotations Nov 14, 2017
@@ -33,6 +33,10 @@ import (
"github.com/kubernetes-incubator/external-dns/plan"
)

const (
googleRecordTTL = 300
Copy link
Contributor

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?

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 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}

Copy link
Contributor

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!

Copy link
Contributor Author

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

@szuecs
Copy link
Contributor

szuecs commented Nov 14, 2017

👍 @linki @ideahitme please merge, as discussed with @stealthybox in slack, I am fine with the current PR.

@stealthybox
Copy link
Contributor Author

stealthybox commented Nov 14, 2017

Added changelog -- thanks @szuecs

@stealthybox stealthybox force-pushed the google-ttl branch 3 times, most recently from 4e680f6 to 7a607a1 Compare November 14, 2017 21:43
Use `int64(ep.RecordTTL)` in `newRecord()`
Fallback to hardcoded 300s for backwards-compat
Add `TestNewRecords()`
Add notes in *ttl.md*
@ideahitme
Copy link

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2017
@njuettner njuettner merged commit eb51f7f into kubernetes-sigs:master Nov 17, 2017
ffledgling pushed a commit to ffledgling/external-dns that referenced this pull request Jan 18, 2018
Google Provider Add Ttl from annotations
lou-lan pushed a commit to lou-lan/external-dns that referenced this pull request May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants