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

Jump to bottom

Multitarget #376

Closed
wants to merge 8 commits into from
Closed

Multitarget #376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ffe076d
support multiple targets by having one Endpoint per DNSName and Targe…
multi-io Oct 28, 2017
2d45bc2
cloudflare provider fixed for multi-target
multi-io Oct 30, 2017
e1dd79f
TXTRegistry: ownerID tracking (for multi-target records) via internal…
multi-io Oct 31, 2017
28fec84
tests adapted
multi-io Oct 31, 2017
2531925
inmemory provider fixed for multi-target
multi-io Nov 8, 2017
837f376
TXTRegistry multi-target tests
multi-io Nov 8, 2017
6ac1b1a
TestNoopRegistry fixed for multi-target
multi-io Nov 7, 2017
612236d
TXTRegistry: do not create records if the name existed already withou…
multi-io Nov 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 8 additions & 6 deletions controller/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,17 +94,19 @@ func TestRunOnce(t *testing.T) {
Target: "1.2.3.4",
},
{
DNSName: "update-record",
Target: "8.8.4.4",
DNSName: "update-record",
Target: "8.8.4.4",
RecordTTL: 100,
},
}, nil)

// Fake some existing records in our DNS provider and validate some desired changes.
provider := newMockProvider(
[]*endpoint.Endpoint{
{
DNSName: "update-record",
Target: "8.8.8.8",
DNSName: "update-record",
Target: "8.8.4.4",
RecordTTL: 50,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this test only work with TTL?
I think we should have a default defined in the zone, similar to DNS origin definition to default TTLs.

If it works, can we make this testcase to have with and without TTL tests

Copy link
Contributor Author

@multi-io multi-io Nov 9, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TTLs are there in some tests now to have record updates rather than creations/deletions. Basically, with one Endpoint per (DNSName, Target), to update a record rather than delete it and create another one, you must not change either DNSName or Target, so I'm changing the TTL instead. If you change the target, the record won't be updated, it will be deleted and another one with the new target created -- similar to what would happen if you change the name. I have modified Plan.Calculate() to operate this way. (that's really the crux of this PR)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thanks to clarify this

},
{
DNSName: "delete-record",
Expand All @@ -116,10 +118,10 @@ func TestRunOnce(t *testing.T) {
{DNSName: "create-record", Target: "1.2.3.4"},
},
UpdateNew: []*endpoint.Endpoint{
{DNSName: "update-record", Target: "8.8.4.4"},
{DNSName: "update-record", Target: "8.8.4.4", RecordTTL: 100},
},
UpdateOld: []*endpoint.Endpoint{
{DNSName: "update-record", Target: "8.8.8.8"},
{DNSName: "update-record", Target: "8.8.4.4", RecordTTL: 50},
},
Delete: []*endpoint.Endpoint{
{DNSName: "delete-record", Target: "4.3.2.1"},
Expand Down
20 changes: 4 additions & 16 deletions plan/plan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,24 +65,16 @@ func (p *Plan) Calculate() *Plan {
continue
}

targetChanged := targetChanged(desired, current)
shouldUpdateTTL := shouldUpdateTTL(desired, current)

if !targetChanged && !shouldUpdateTTL {
if !shouldUpdateTTL {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition skipped the record in case nothing was changed (target nor TTL). With this it skips the record whenever TTL was changed (but target could have) which seems wrong to me. Can you double-check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

desired and current are guaranteed to have the same target (see the recordExists() modification below), so targetChanged would always be false. That's why I removed it. As I said, with this PR you'd have one Endpoint per DNSName and Target rather than just per DNSName, so if the target changes, the corresponding endpoint will be removed and a new one with the same name and the new target created. So it treats target changes just like it always treated name changes -- the old Endpoint will be removed and a new one created. The TTL would be the only remaining thing in an Endpoint that can actually "change".

That's really the crux of this PR -- don't turn Endpoint.Target into an array, instead have one Endpoint per name and target. I did this because I figured it would require fewer (possibly no) changes to the providers, and I wanted to get the CF provider up and running with multiple targets quickly (I initially thought you'd basically get away with just changing plan.recordExists(), then saw that I should also modify the ownerId tracking in TXTRegistry). I guess we'd have to discuss whether this is the best way forward or not, since this is obviously incompatible with the other multitarget PRs that turn Endpoint.Target into an array.

log.Debugf("Skipping endpoint %v because nothing has changed", desired)
continue
}

changes.UpdateOld = append(changes.UpdateOld, current)
desired.MergeLabels(current.Labels) // inherit the labels from the dns provider, including Owner ID

if targetChanged {
desired.RecordType = current.RecordType // inherit the type from the dns provider
}

if !shouldUpdateTTL {
desired.RecordTTL = current.RecordTTL
}
desired.MergeLabels(current.Labels) // inherit the labels from the dns provider
desired.RecordType = current.RecordType // inherit the type from the dns provider

changes.UpdateNew = append(changes.UpdateNew, desired)
}
Expand All @@ -109,10 +101,6 @@ func (p *Plan) Calculate() *Plan {
return plan
}

func targetChanged(desired, current *endpoint.Endpoint) bool {
return desired.Target != current.Target
}

func shouldUpdateTTL(desired, current *endpoint.Endpoint) bool {
if !desired.RecordTTL.IsConfigured() {
return false
Expand All @@ -123,7 +111,7 @@ func shouldUpdateTTL(desired, current *endpoint.Endpoint) bool {
// recordExists checks whether a record can be found in a list of records.
func recordExists(needle *endpoint.Endpoint, haystack []*endpoint.Endpoint) (*endpoint.Endpoint, bool) {
for _, record := range haystack {
if record.DNSName == needle.DNSName {
if record.DNSName == needle.DNSName && record.Target == needle.Target {
return record, true
}
}
Expand Down
44 changes: 27 additions & 17 deletions plan/plan_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,34 +27,37 @@ import (
// TestCalculate tests that a plan can calculate actions to move a list of
// current records to a list of desired records.
func TestCalculate(t *testing.T) {
// we need different TTLs to create differing Endpoints with the same name and target
ttl := endpoint.TTL(300)
ttl2 := endpoint.TTL(50)

// empty list of records
empty := []*endpoint.Endpoint{}
// a simple entry
fooV1 := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v1", endpoint.RecordTypeCNAME)}
// the same entry but with different target
fooV2 := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v2", endpoint.RecordTypeCNAME)}
// the same entry as before but with varying TTLs
fooV2ttl1 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", endpoint.RecordTypeCNAME, ttl)}
fooV2ttl2 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", endpoint.RecordTypeCNAME, ttl2)}
// another simple entry
bar := []*endpoint.Endpoint{endpoint.NewEndpoint("bar", "v1", endpoint.RecordTypeCNAME)}

// test case with labels
noLabels := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v2", endpoint.RecordTypeCNAME)}
labeledV2 := []*endpoint.Endpoint{newEndpointWithOwner("foo", "v2", "123")}
labeledV1 := []*endpoint.Endpoint{newEndpointWithOwner("foo", "v1", "123")}
unlabeledTTL2 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", endpoint.RecordTypeCNAME, ttl2)}
labeledTTL1 := []*endpoint.Endpoint{newEndpointWithOwnerAndTTL("foo", "v2", "123", ttl)}
labeledTTL2 := []*endpoint.Endpoint{newEndpointWithOwnerAndTTL("foo", "v2", "123", ttl2)}

// test case with type inheritance
noType := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v2", "")}
typedV2 := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v2", endpoint.RecordTypeA)}
typedV1 := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v1", endpoint.RecordTypeA)}
untypedTTL2 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", "", ttl2)}
typedTTL1 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", endpoint.RecordTypeA, ttl)}
typedTTL2 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", endpoint.RecordTypeA, ttl2)}

// test case with TTL
ttl := endpoint.TTL(300)
ttl2 := endpoint.TTL(50)
// explicit TTL test cases
ttlV1 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v1", endpoint.RecordTypeCNAME, ttl)}
ttlV2 := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v1", endpoint.RecordTypeCNAME)}
ttlV3 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v1", endpoint.RecordTypeCNAME, ttl)}
ttlV4 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v1", endpoint.RecordTypeCNAME, ttl2)}
ttlV5 := []*endpoint.Endpoint{endpoint.NewEndpoint("foo", "v2", endpoint.RecordTypeCNAME)}
ttlV6 := []*endpoint.Endpoint{endpoint.NewEndpointWithTTL("foo", "v2", endpoint.RecordTypeCNAME, ttl)}

for _, tc := range []struct {
policies []Policy
Expand All @@ -69,24 +72,24 @@ func TestCalculate(t *testing.T) {
{[]Policy{&SyncPolicy{}}, fooV1, fooV1, empty, empty, empty, empty},
// Nothing is desired deletes the current.
{[]Policy{&SyncPolicy{}}, fooV1, empty, empty, empty, empty, fooV1},
// Current and desired match but Target is different triggers an update.
{[]Policy{&SyncPolicy{}}, fooV1, fooV2, empty, fooV1, fooV2, empty},
// Current and desired match but TTL is different triggers an update.
{[]Policy{&SyncPolicy{}}, fooV2ttl1, fooV2ttl2, empty, fooV2ttl1, fooV2ttl2, empty},
// Both exist but are different creates desired and deletes current.
{[]Policy{&SyncPolicy{}}, fooV1, bar, bar, empty, empty, fooV1},
// Same thing with current and desired only having different targets
{[]Policy{&SyncPolicy{}}, fooV1, fooV2, fooV2, empty, empty, fooV1},
// Nothing is desired but policy doesn't allow deletions.
{[]Policy{&UpsertOnlyPolicy{}}, fooV1, empty, empty, empty, empty, empty},
// Labels should be inherited
{[]Policy{&SyncPolicy{}}, labeledV1, noLabels, empty, labeledV1, labeledV2, empty},
{[]Policy{&SyncPolicy{}}, labeledTTL1, unlabeledTTL2, empty, labeledTTL1, labeledTTL2, empty},
// RecordType should be inherited
{[]Policy{&SyncPolicy{}}, typedV1, noType, empty, typedV1, typedV2, empty},
{[]Policy{&SyncPolicy{}}, typedTTL1, untypedTTL2, empty, typedTTL1, typedTTL2, empty},
// If desired TTL is not configured, do not update
{[]Policy{&SyncPolicy{}}, ttlV1, ttlV2, empty, empty, empty, empty},
// If desired TTL is configured but is the same as current TTL, do not update
{[]Policy{&SyncPolicy{}}, ttlV1, ttlV3, empty, empty, empty, empty},
// If desired TTL is configured and is not the same as current TTL, need to update
{[]Policy{&SyncPolicy{}}, ttlV1, ttlV4, empty, ttlV1, ttlV4, empty},
// If target changed and desired TTL is not configured, do not update TTL
{[]Policy{&SyncPolicy{}}, ttlV1, ttlV5, empty, ttlV1, ttlV6, empty},
} {
// setup plan
plan := &Plan{
Expand Down Expand Up @@ -187,3 +190,10 @@ func newEndpointWithOwner(dnsName, target, ownerID string) *endpoint.Endpoint {
e.Labels[endpoint.OwnerLabelKey] = ownerID
return e
}

func newEndpointWithOwnerAndTTL(dnsName, target, ownerID string, ttl endpoint.TTL) *endpoint.Endpoint {
e := endpoint.NewEndpoint(dnsName, target, endpoint.RecordTypeCNAME)
e.Labels[endpoint.OwnerLabelKey] = ownerID
e.RecordTTL = ttl
return e
}
2 changes: 1 addition & 1 deletion provider/cloudflare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ func (p *CloudFlareProvider) changesByZone(zones []cloudflare.Zone, changeSet []

func (p *CloudFlareProvider) getRecordID(records []cloudflare.DNSRecord, record cloudflare.DNSRecord) string {
for _, zoneRecord := range records {
if zoneRecord.Name == record.Name && zoneRecord.Type == record.Type {
if zoneRecord.Name == record.Name && zoneRecord.Type == record.Type && zoneRecord.Content == record.Content {
return zoneRecord.ID
}
}
Expand Down
68 changes: 42 additions & 26 deletions provider/inmemory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ func (im *InMemoryProvider) Records() ([]*endpoint.Endpoint, error) {
}

for _, record := range records {
endpoints = append(endpoints, endpoint.NewEndpoint(record.Name, record.Target, record.Type))
endpoints = append(endpoints, endpoint.NewEndpointWithTTL(record.Name, record.Target, record.Type, record.RecordTTL))
}
}

Expand Down Expand Up @@ -201,9 +201,10 @@ func convertToInMemoryRecord(endpoints []*endpoint.Endpoint) []*inMemoryRecord {
records := []*inMemoryRecord{}
for _, ep := range endpoints {
records = append(records, &inMemoryRecord{
Type: ep.RecordType,
Name: ep.DNSName,
Target: ep.Target,
Type: ep.RecordType,
Name: ep.DNSName,
Target: ep.Target,
RecordTTL: ep.RecordTTL,
})
}
return records
Expand Down Expand Up @@ -241,10 +242,12 @@ func (f *filter) EndpointZoneID(endpoint *endpoint.Endpoint, zones map[string]st
// Type - type of record
// Name - DNS name assigned to the record
// Target - target of the record
// RecordTTL - TTL of the record
type inMemoryRecord struct {
Type string
Name string
Target string
Type string
Name string
Target string
RecordTTL endpoint.TTL
}

type zone map[string][]*inMemoryRecord
Expand Down Expand Up @@ -303,18 +306,17 @@ func (c *inMemoryClient) ApplyChanges(zoneID string, changes *inMemoryChange) er
}
c.zones[zoneID][newEndpoint.Name] = append(c.zones[zoneID][newEndpoint.Name], newEndpoint)
}
for _, updateEndpoint := range changes.UpdateNew {
for _, rec := range c.zones[zoneID][updateEndpoint.Name] {
if rec.Type == updateEndpoint.Type {
rec.Target = updateEndpoint.Target
break
}
for i, updateOld := range changes.UpdateOld {
if rec := findRecord(updateOld, c.zones[zoneID][updateOld.Name]); rec != nil {
updateNew := changes.UpdateNew[i]
rec.Target = updateNew.Target
rec.RecordTTL = updateNew.RecordTTL
}
}
for _, deleteEndpoint := range changes.Delete {
newSet := make([]*inMemoryRecord, 0)
for _, rec := range c.zones[zoneID][deleteEndpoint.Name] {
if rec.Type != deleteEndpoint.Type {
if rec.Type != deleteEndpoint.Type || rec.Target != deleteEndpoint.Target {
newSet = append(newSet, rec)
}
}
Expand All @@ -323,15 +325,29 @@ func (c *inMemoryClient) ApplyChanges(zoneID string, changes *inMemoryChange) er
return nil
}

func (c *inMemoryClient) updateMesh(mesh map[string]map[string]bool, record *inMemoryRecord) error {
// mesh: map DNSName => map Type => map Target => bool
func (c *inMemoryClient) updateMesh(mesh map[string]map[string]map[string]bool, record *inMemoryRecord) error {
if _, exists := mesh[record.Name]; exists {
if mesh[record.Name][record.Type] {
return ErrDuplicateRecordFound
if _, exists := mesh[record.Name][record.Type]; exists {
if mesh[record.Name][record.Type][record.Target] {
return ErrDuplicateRecordFound
}

mesh[record.Name][record.Type][record.Target] = true
return nil
}

mesh[record.Name][record.Type] = map[string]bool{
record.Target: true,
}
mesh[record.Name][record.Type] = true
return nil
}
mesh[record.Name] = map[string]bool{record.Type: true}

mesh[record.Name] = map[string]map[string]bool{
record.Type: {
record.Target: true,
},
}
return nil
}

Expand All @@ -341,30 +357,30 @@ func (c *inMemoryClient) validateChangeBatch(zone string, changes *inMemoryChang
if !ok {
return ErrZoneNotFound
}
mesh := map[string]map[string]bool{}
mesh := map[string]map[string]map[string]bool{}
for _, newEndpoint := range changes.Create {
if c.findByType(newEndpoint.Type, curZone[newEndpoint.Name]) != nil {
if findRecord(newEndpoint, curZone[newEndpoint.Name]) != nil {
return ErrRecordAlreadyExists
}
if err := c.updateMesh(mesh, newEndpoint); err != nil {
return err
}
}
for _, updateEndpoint := range changes.UpdateNew {
if c.findByType(updateEndpoint.Type, curZone[updateEndpoint.Name]) == nil {
if findRecord(updateEndpoint, curZone[updateEndpoint.Name]) == nil {
return ErrRecordNotFound
}
if err := c.updateMesh(mesh, updateEndpoint); err != nil {
return err
}
}
for _, updateOldEndpoint := range changes.UpdateOld {
if rec := c.findByType(updateOldEndpoint.Type, curZone[updateOldEndpoint.Name]); rec == nil || rec.Target != updateOldEndpoint.Target {
if findRecord(updateOldEndpoint, curZone[updateOldEndpoint.Name]) == nil {
return ErrRecordNotFound
}
}
for _, deleteEndpoint := range changes.Delete {
if rec := c.findByType(deleteEndpoint.Type, curZone[deleteEndpoint.Name]); rec == nil || rec.Target != deleteEndpoint.Target {
if findRecord(deleteEndpoint, curZone[deleteEndpoint.Name]) == nil {
return ErrRecordNotFound
}
if err := c.updateMesh(mesh, deleteEndpoint); err != nil {
Expand All @@ -374,9 +390,9 @@ func (c *inMemoryClient) validateChangeBatch(zone string, changes *inMemoryChang
return nil
}

func (c *inMemoryClient) findByType(recordType string, records []*inMemoryRecord) *inMemoryRecord {
func findRecord(needle *inMemoryRecord, records []*inMemoryRecord) *inMemoryRecord {
for _, record := range records {
if record.Type == recordType {
if record.Type == needle.Type && record.Name == needle.Name && record.Target == needle.Target {
return record
}
}
Expand Down