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.

Sign up for GitHub

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
Merged

Google Provider Add Ttl from annotations #389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
- Google: Support configuring TTL by annotation: `external-dns.alpha.kubernetes.io/ttl`. (#389) @stealthybox

## v0.4.8 - 2017-11-09

- AWS: Added change batch limiting to a maximum of 4000 Route53 updates in one API call. Changes exceeding the limit will be dropped but all related changes by hostname are preserved within the limit. (#368) @bitvector2
Expand Down
18 changes: 17 additions & 1 deletion docs/ttl.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,23 @@ Providers
- [ ] Azure
- [ ] Cloudflare
- [ ] DigitalOcean
- [ ] Google
- [x] Google
- [ ] InMemory

PRs welcome!

Notes
=====
When the `external-dns.alpha.kubernetes.io/ttl` annotation is not provided, the Ttl will default to 0 seconds and `enpoint.TTL.isConfigured()` will be false.

### AWS Provider
The AWS Provider overrides the value to 300s when the Ttl is 0.
This value is a constant in the provider code.

### Google Provider
Previously with the Google Provider, Ttl's were hard-coded to 300s.
For safety, the Google Provider overrides the value to 300s when the Ttl is 0.
This value is a constant in the provider code.

For the moment, it is impossible to use a Ttl value of 0 with the AWS and Google Providers.
This behavior may change in the future.
12 changes: 11 additions & 1 deletion provider/google.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

)

type managedZonesCreateCallInterface interface {
Do(opts ...googleapi.CallOption) (*dns.ManagedZone, error)
}
Expand Down Expand Up @@ -319,10 +323,16 @@ func newRecord(ep *endpoint.Endpoint) *dns.ResourceRecordSet {
target = ensureTrailingDot(target)
}

// no annotation results in a Ttl of 0, default to 300 for backwards-compatability
var ttl int64 = googleRecordTTL
if ep.RecordTTL.IsConfigured() {
ttl = int64(ep.RecordTTL)
}

return &dns.ResourceRecordSet{
Name: ensureTrailingDot(ep.DNSName),
Rrdatas: []string{target},
Ttl: 300,
Ttl: ttl,
Type: ep.RecordType,
}
}
23 changes: 23 additions & 0 deletions provider/google_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,27 @@ func TestGoogleApplyChangesEmpty(t *testing.T) {
assert.NoError(t, provider.ApplyChanges(&plan.Changes{}))
}

func TestNewRecords(t *testing.T) {
records := newRecords([]*endpoint.Endpoint{
endpoint.NewEndpointWithTTL("update-test.zone-2.ext-dns-test-2.gcp.zalan.do", "8.8.4.4", endpoint.RecordTypeA, 1),
endpoint.NewEndpointWithTTL("delete-test.zone-2.ext-dns-test-2.gcp.zalan.do", "8.8.4.4", endpoint.RecordTypeA, 120),
endpoint.NewEndpointWithTTL("update-test-cname.zone-1.ext-dns-test-2.gcp.zalan.do", "bar.elb.amazonaws.com", endpoint.RecordTypeCNAME, 4000),
// test fallback to Ttl:300 when Ttl==0 :
endpoint.NewEndpointWithTTL("update-test.zone-1.ext-dns-test-2.gcp.zalan.do", "8.8.8.8", endpoint.RecordTypeA, 0),
endpoint.NewEndpoint("delete-test.zone-1.ext-dns-test-2.gcp.zalan.do", "8.8.8.8", endpoint.RecordTypeA),
endpoint.NewEndpoint("delete-test-cname.zone-1.ext-dns-test-2.gcp.zalan.do", "qux.elb.amazonaws.com", endpoint.RecordTypeCNAME),
})

validateChangeRecords(t, records, []*dns.ResourceRecordSet{
{Name: "update-test.zone-2.ext-dns-test-2.gcp.zalan.do.", Rrdatas: []string{"8.8.4.4"}, Type: "A", Ttl: 1},
{Name: "delete-test.zone-2.ext-dns-test-2.gcp.zalan.do.", Rrdatas: []string{"8.8.4.4"}, Type: "A", Ttl: 120},
{Name: "update-test-cname.zone-1.ext-dns-test-2.gcp.zalan.do.", Rrdatas: []string{"bar.elb.amazonaws.com."}, Type: "CNAME", Ttl: 4000},
{Name: "update-test.zone-1.ext-dns-test-2.gcp.zalan.do.", Rrdatas: []string{"8.8.8.8"}, Type: "A", Ttl: 300},
{Name: "delete-test.zone-1.ext-dns-test-2.gcp.zalan.do.", Rrdatas: []string{"8.8.8.8"}, Type: "A", Ttl: 300},
{Name: "delete-test-cname.zone-1.ext-dns-test-2.gcp.zalan.do.", Rrdatas: []string{"qux.elb.amazonaws.com."}, Type: "CNAME", Ttl: 300},
})
}

func TestSeparateChanges(t *testing.T) {
change := &dns.Change{
Additions: []*dns.ResourceRecordSet{
Expand Down Expand Up @@ -475,7 +496,9 @@ func validateChangeRecords(t *testing.T, records []*dns.ResourceRecordSet, expec

func validateChangeRecord(t *testing.T, record *dns.ResourceRecordSet, expected *dns.ResourceRecordSet) {
assert.Equal(t, expected.Name, record.Name)
assert.Equal(t, expected.Rrdatas, record.Rrdatas)
assert.Equal(t, expected.Ttl, record.Ttl)
assert.Equal(t, expected.Type, record.Type)
}

func newGoogleProvider(t *testing.T, domainFilter DomainFilter, dryRun bool, records []*endpoint.Endpoint) *GoogleProvider {
Expand Down