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

Multi target plan ( Page Not Found ) #404

Merged
merged 18 commits into from
Dec 14, 2017
Merged
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
- Every record managed by External DNS is now mapped to a kubernetes resource (service/ingress) @ideahitme
- New field is stored in TXT DNS record which reflects which kubernetes resource has acquired the DNS name
- Target of DNS record is changed only if corresponding kubernetes resource target changes
- If kubernetes resource is deleted, then another resource may acquire DNS name
- "Flapping" target issue is resolved by providing a consistent and defined mechanism for choosing a target

## v0.4.8 - 2017-11-22

- Allow filtering by source annotation via `--annotation-filter` (#354) @khrisrichardson
15 changes: 2 additions & 13 deletions endpoint/endpoint.go
Original file line number Diff line number Diff line change
@@ -22,8 +22,6 @@ import (
)

const (
// OwnerLabelKey is the name of the label that defines the owner of an Endpoint.
OwnerLabelKey = "owner"
// RecordTypeA is a RecordType enum value
RecordTypeA = "A"
// RecordTypeCNAME is a RecordType enum value
@@ -51,7 +49,7 @@ type Endpoint struct {
// TTL for the record
RecordTTL TTL
// Labels stores labels defined for the Endpoint
Labels map[string]string
Labels Labels
}

// NewEndpoint initialization method to be used to create an endpoint
@@ -65,20 +63,11 @@ func NewEndpointWithTTL(dnsName, target, recordType string, ttl TTL) *Endpoint {
DNSName: strings.TrimSuffix(dnsName, "."),
Target: strings.TrimSuffix(target, "."),
RecordType: recordType,
Labels: map[string]string{},
Labels: NewLabels(),
RecordTTL: ttl,
}
}

// MergeLabels adds keys to labels if not defined for the endpoint
func (e *Endpoint) MergeLabels(labels map[string]string) {
for k, v := range labels {
if e.Labels[k] == "" {
e.Labels[k] = v
}
}
}

func (e *Endpoint) String() string {
return fmt.Sprintf("%s %d IN %s %s", e.DNSName, e.RecordTTL, e.RecordType, e.Target)
}
12 changes: 0 additions & 12 deletions endpoint/endpoint_test.go
Original file line number Diff line number Diff line change
@@ -18,8 +18,6 @@ package endpoint

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestNewEndpoint(t *testing.T) {
@@ -36,13 +34,3 @@ func TestNewEndpoint(t *testing.T) {
t.Error("endpoint is not initialized correctly")
}
}

func TestMergeLabels(t *testing.T) {
e := NewEndpoint("abc.com", "1.2.3.4", "A")
e.Labels = map[string]string{
"foo": "bar",
"baz": "qux",
}
e.MergeLabels(map[string]string{"baz": "baz", "new": "fox"})
assert.Equal(t, map[string]string{"foo": "bar", "baz": "qux", "new": "fox"}, e.Labels)
}
99 changes: 99 additions & 0 deletions endpoint/labels.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
Copyright 2017 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package endpoint

import (
"errors"
"fmt"
"sort"
"strings"
)

var (
// ErrInvalidHeritage is returned when heritage was not found, or different heritage is found
ErrInvalidHeritage = errors.New("heritage is unknown or not found")
)

const (
heritage = "external-dns"
// OwnerLabelKey is the name of the label that defines the owner of an Endpoint.
OwnerLabelKey = "owner"
// ResourceLabelKey is the name of the label that identifies k8s resource which wants to acquire the DNS name
ResourceLabelKey = "resource"
Copy link
Member

Choose a reason for hiding this comment

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

Owner was supposed to identify the owner of a resource. For us it was to say it's "the cluster" (or rather the - usually one - instance of external-dns). However now the owner is a particular resource in a particular namespace in a particular cluster. I wonder if it's confusing to separate them this way.

I would rather clarify it with either a combined owner resource (owner=extdns1/default/my-service, is that even easier to code?) or two owner related labels (ownerInstance=extdns1, ownerResource=default/my-service, just a rename then).

Although there's the concern of backwards compatibility...

@ideahitme let me know what you think.

Copy link
Author

@ideahitme ideahitme Dec 13, 2017

Choose a reason for hiding this comment

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

yeah I agree, the naming is probably not the most descriptive.

However now the owner is a particular resource in a particular namespace in a particular cluster. I wonder if it's confusing to separate them this way.

I would still prefer them to be separated:

  • "owner" label signifies if the DNS Record can be modified at all
  • "resource" label helps to figure out how it should be updated

I also can see scenarios where "resource" label can be ignored altogether, so in my mind it is just a metadata, which allows to implement a "method" for determining resource-ownership, but it could be used for something else, or a different method could be implemented. That's why I thought it is better to keep the label name independent of how we use it.

With the previous statement though it is possibly better to rename "owner" label to something else, like "external-dns-instance", but since it would break compatibility, I would not do this change in this PR. However this change would be possible with some code change which would allow us to gradually migrate txt record values.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed!

)

// Labels store metadata related to the endpoint
// it is then stored in a persistent storage via serialization
type Labels map[string]string

// NewLabels returns empty Labels
func NewLabels() Labels {
return map[string]string{}
}

// NewLabelsFromString constructs endpoints labels from a provided format string
// if heritage set to another value is found then error is returned
// no heritage automatically assumes is not owned by external-dns and returns invalidHeritage error
func NewLabelsFromString(labelText string) (Labels, error) {
endpointLabels := map[string]string{}
labelText = strings.Trim(labelText, "\"") // drop quotes
tokens := strings.Split(labelText, ",")
foundExternalDNSHeritage := false
for _, token := range tokens {
if len(strings.Split(token, "=")) != 2 {
continue
}
key := strings.Split(token, "=")[0]
val := strings.Split(token, "=")[1]
Copy link
Member

@linki linki Dec 12, 2017

Choose a reason for hiding this comment

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

Can we use Kubernetes' label parsing functions? Seems easier to use: https://github.com/linki/chaoskube/blob/v0.6.1/main.go#L59 and we don't have to maintain this.

Copy link
Author

Choose a reason for hiding this comment

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

I will try it out :)

Copy link
Author

Choose a reason for hiding this comment

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

done :)

Copy link
Author

Choose a reason for hiding this comment

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

undone, because labels could not be parsed if "/" are present :(

if key == "heritage" && val != heritage {
return nil, ErrInvalidHeritage
}
if key == "heritage" {
foundExternalDNSHeritage = true
continue
}
if strings.HasPrefix(key, heritage) {
endpointLabels[strings.TrimPrefix(key, heritage+"/")] = val
}
}

if !foundExternalDNSHeritage {
return nil, ErrInvalidHeritage
}

return endpointLabels, nil
}

// Serialize transforms endpoints labels into a external-dns recognizable format string
// withQuotes adds additional quotes
func (l Labels) Serialize(withQuotes bool) string {
var tokens []string
tokens = append(tokens, fmt.Sprintf("heritage=%s", heritage))
var keys []string
for key := range l {
keys = append(keys, key)
}
sort.Strings(keys) // sort for consistency

for _, key := range keys {
tokens = append(tokens, fmt.Sprintf("%s/%s=%s", heritage, key, l[key]))
}
if withQuotes {
return fmt.Sprintf("\"%s\"", strings.Join(tokens, ","))
}
return strings.Join(tokens, ",")
}
92 changes: 92 additions & 0 deletions endpoint/labels_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/*
Copyright 2017 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package endpoint

import (
"fmt"
"testing"

"github.com/stretchr/testify/suite"
)

type LabelsSuite struct {
suite.Suite
foo Labels
fooAsText string
fooAsTextWithQuotes string
barText string
barTextAsMap Labels
noHeritageText string
noHeritageAsMap Labels
wrongHeritageText string
multipleHeritageText string //considered invalid
}

func (suite *LabelsSuite) SetupTest() {
suite.foo = map[string]string{
"owner": "foo-owner",
"resource": "foo-resource",
}
suite.fooAsText = "heritage=external-dns,external-dns/owner=foo-owner,external-dns/resource=foo-resource"
suite.fooAsTextWithQuotes = fmt.Sprintf(`"%s"`, suite.fooAsText)

suite.barTextAsMap = map[string]string{
"owner": "bar-owner",
"resource": "bar-resource",
"new-key": "bar-new-key",
}
suite.barText = "heritage=external-dns,,external-dns/owner=bar-owner,external-dns/resource=bar-resource,external-dns/new-key=bar-new-key,random=stuff,no-equal-sign,," //also has some random gibberish

suite.noHeritageText = "external-dns/owner=random-owner"
suite.wrongHeritageText = "heritage=mate,external-dns/owner=random-owner"
suite.multipleHeritageText = "heritage=mate,heritage=external-dns,external-dns/owner=random-owner"
}

func (suite *LabelsSuite) TestSerialize() {
suite.Equal(suite.fooAsText, suite.foo.Serialize(false), "should serializeLabel")
suite.Equal(suite.fooAsTextWithQuotes, suite.foo.Serialize(true), "should serializeLabel")
}

func (suite *LabelsSuite) TestDeserialize() {
foo, err := NewLabelsFromString(suite.fooAsText)
suite.NoError(err, "should succeed for valid label text")
suite.Equal(suite.foo, foo, "should reconstruct original label map")

foo, err = NewLabelsFromString(suite.fooAsTextWithQuotes)
suite.NoError(err, "should succeed for valid label text")
suite.Equal(suite.foo, foo, "should reconstruct original label map")

bar, err := NewLabelsFromString(suite.barText)
suite.NoError(err, "should succeed for valid label text")
suite.Equal(suite.barTextAsMap, bar, "should reconstruct original label map")

noHeritage, err := NewLabelsFromString(suite.noHeritageText)
suite.Equal(ErrInvalidHeritage, err, "should fail if no heritage is found")
suite.Nil(noHeritage, "should return nil")

wrongHeritage, err := NewLabelsFromString(suite.wrongHeritageText)
suite.Equal(ErrInvalidHeritage, err, "should fail if wrong heritage is found")
suite.Nil(wrongHeritage, "if error should return nil")

multipleHeritage, err := NewLabelsFromString(suite.multipleHeritageText)
suite.Equal(ErrInvalidHeritage, err, "should fail if multiple heritage is found")
suite.Nil(multipleHeritage, "if error should return nil")
}

func TestLabels(t *testing.T) {
suite.Run(t, new(LabelsSuite))
}
10 changes: 7 additions & 3 deletions internal/testutils/endpoint.go
Original file line number Diff line number Diff line change
@@ -16,8 +16,11 @@ limitations under the License.

package testutils

import "github.com/kubernetes-incubator/external-dns/endpoint"
import "sort"
import (
"sort"

"github.com/kubernetes-incubator/external-dns/endpoint"
)

/** test utility functions for endpoints verifications */

@@ -45,7 +48,8 @@ func (b byAllFields) Less(i, j int) bool {
// considers example.org. and example.org DNSName/Target as different endpoints
func SameEndpoint(a, b *endpoint.Endpoint) bool {
return a.DNSName == b.DNSName && a.Target == b.Target && a.RecordType == b.RecordType &&
a.Labels[endpoint.OwnerLabelKey] == b.Labels[endpoint.OwnerLabelKey] && a.RecordTTL == b.RecordTTL
a.Labels[endpoint.OwnerLabelKey] == b.Labels[endpoint.OwnerLabelKey] && a.RecordTTL == b.RecordTTL &&
a.Labels[endpoint.ResourceLabelKey] == b.Labels[endpoint.ResourceLabelKey]
}

// SameEndpoints compares two slices of endpoints regardless of order
70 changes: 70 additions & 0 deletions plan/conflict.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
Copyright 2017 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package plan

import (
"sort"

"github.com/kubernetes-incubator/external-dns/endpoint"
)

// ConflictResolver is used to make a decision in case of two or more different kubernetes resources
// are trying to acquire same DNS name
type ConflictResolver interface {
ResolveCreate(candidates []*endpoint.Endpoint) *endpoint.Endpoint
ResolveUpdate(current *endpoint.Endpoint, candidates []*endpoint.Endpoint) *endpoint.Endpoint
}

// PerResource allows only one resource to own a given dns name
type PerResource struct{}

// ResolveCreate is invoked when dns name is not owned by any resource
// ResolveCreate takes "minimal" (string comparison of Target) endpoint to acquire the DNS record
func (s PerResource) ResolveCreate(candidates []*endpoint.Endpoint) *endpoint.Endpoint {
var min *endpoint.Endpoint
for _, ep := range candidates {
if min == nil || s.less(ep, min) {
min = ep
}
}
return min
}

// ResolveUpdate is invoked when dns name is already owned by "current" endpoint
// ResolveUpdate uses "current" record as base and updates it accordingly with new version of same resource
// if it doesn't exist then pick min
func (s PerResource) ResolveUpdate(current *endpoint.Endpoint, candidates []*endpoint.Endpoint) *endpoint.Endpoint {
currentResource := current.Labels[endpoint.ResourceLabelKey] // resource which has already acquired the DNS
// TODO: sort candidates only needed because we can still have two endpoints from same resource here. We sort for consistency
// TODO: remove once single endpoint can have multiple targets
sort.SliceStable(candidates, func(i, j int) bool {
return s.less(candidates[i], candidates[j])
})
for _, ep := range candidates {
if ep.Labels[endpoint.ResourceLabelKey] == currentResource {
return ep
}
}
return s.ResolveCreate(candidates)
}

// less returns true if endpoint x is less than y
func (s PerResource) less(x, y *endpoint.Endpoint) bool {
return x.Target < y.Target
}

// TODO: with cross-resource/cross-cluster setup alternative variations of ConflictResolver can be used
137 changes: 137 additions & 0 deletions plan/conflict_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
/*
Copyright 2017 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package plan

import (
"testing"

"github.com/kubernetes-incubator/external-dns/endpoint"
"github.com/stretchr/testify/suite"
)

var _ ConflictResolver = PerResource{}

type ResolverSuite struct {
// resolvers
perResource PerResource
// endpoints
fooV1Cname *endpoint.Endpoint
fooV2Cname *endpoint.Endpoint
fooV2CnameDuplicate *endpoint.Endpoint
fooA5 *endpoint.Endpoint
bar127A *endpoint.Endpoint
bar192A *endpoint.Endpoint
bar127AAnother *endpoint.Endpoint
legacyBar192A *endpoint.Endpoint // record created in AWS now without resource label
suite.Suite
}

func (suite *ResolverSuite) SetupTest() {
suite.perResource = PerResource{}
// initialize endpoints used in tests
suite.fooV1Cname = &endpoint.Endpoint{
DNSName: "foo",
Target: "v1",
RecordType: "CNAME",
Labels: map[string]string{
endpoint.ResourceLabelKey: "ingress/default/foo-v1",
},
}
suite.fooV2Cname = &endpoint.Endpoint{
DNSName: "foo",
Target: "v2",
RecordType: "CNAME",
Labels: map[string]string{
endpoint.ResourceLabelKey: "ingress/default/foo-v2",
},
}
suite.fooV2CnameDuplicate = &endpoint.Endpoint{
DNSName: "foo",
Target: "v2",
RecordType: "CNAME",
Labels: map[string]string{
endpoint.ResourceLabelKey: "ingress/default/foo-v2-duplicate",
},
}
suite.fooA5 = &endpoint.Endpoint{
DNSName: "foo",
Target: "5.5.5.5",
RecordType: "A",
Labels: map[string]string{
endpoint.ResourceLabelKey: "ingress/default/foo-5",
},
}
suite.bar127A = &endpoint.Endpoint{
DNSName: "bar",
Target: "127.0.0.1",
RecordType: "A",
Labels: map[string]string{
endpoint.ResourceLabelKey: "ingress/default/bar-127",
},
}
suite.bar127AAnother = &endpoint.Endpoint{ //TODO: remove this once we move to multiple targets under same endpoint
DNSName: "bar",
Target: "8.8.8.8",
RecordType: "A",
Labels: map[string]string{
endpoint.ResourceLabelKey: "ingress/default/bar-127",
},
}
suite.bar192A = &endpoint.Endpoint{
DNSName: "bar",
Target: "192.168.0.1",
RecordType: "A",
Labels: map[string]string{
endpoint.ResourceLabelKey: "ingress/default/bar-192",
},
}
suite.legacyBar192A = &endpoint.Endpoint{
DNSName: "bar",
Target: "192.168.0.1",
RecordType: "A",
}
}

func (suite *ResolverSuite) TestStrictResolver() {
// test that perResource resolver picks min for create list
suite.Equal(suite.bar127A, suite.perResource.ResolveCreate([]*endpoint.Endpoint{suite.bar127A, suite.bar192A}), "should pick min one")
suite.Equal(suite.fooA5, suite.perResource.ResolveCreate([]*endpoint.Endpoint{suite.fooA5, suite.fooV1Cname}), "should pick min one")
suite.Equal(suite.fooV1Cname, suite.perResource.ResolveCreate([]*endpoint.Endpoint{suite.fooV2Cname, suite.fooV1Cname}), "should pick min one")

//test that perResource resolver preserves resource if it still exists
suite.Equal(suite.bar127A, suite.perResource.ResolveUpdate(suite.bar127A, []*endpoint.Endpoint{suite.bar127AAnother, suite.bar127A}), "should pick min for update when same resource endpoint occurs multiple times (remove after multiple-target support") // TODO:remove this test
suite.Equal(suite.bar127A, suite.perResource.ResolveUpdate(suite.bar127A, []*endpoint.Endpoint{suite.bar192A, suite.bar127A}), "should pick existing resource")
suite.Equal(suite.fooV2Cname, suite.perResource.ResolveUpdate(suite.fooV2Cname, []*endpoint.Endpoint{suite.fooV2Cname, suite.fooV2CnameDuplicate}), "should pick existing resource even if targets are same")
suite.Equal(suite.fooA5, suite.perResource.ResolveUpdate(suite.fooV1Cname, []*endpoint.Endpoint{suite.fooA5, suite.fooV2Cname}), "should pick new if resource was deleted")
// should actually get the updated record (note ttl is different)
newFooV1Cname := &endpoint.Endpoint{
DNSName: suite.fooV1Cname.DNSName,
Target: suite.fooV1Cname.Target,
Labels: suite.fooV1Cname.Labels,
RecordType: suite.fooV1Cname.RecordType,
RecordTTL: suite.fooV1Cname.RecordTTL + 1, // ttl is different
}
suite.Equal(newFooV1Cname, suite.perResource.ResolveUpdate(suite.fooV1Cname, []*endpoint.Endpoint{suite.fooA5, suite.fooV2Cname, newFooV1Cname}), "should actually pick same resource with updates")

// legacy record's resource value will not match any candidates resource label
// therefore pick minimum again
suite.Equal(suite.bar127A, suite.perResource.ResolveUpdate(suite.legacyBar192A, []*endpoint.Endpoint{suite.bar127A, suite.bar192A}), " legacy record's resource value will not match, should pick minimum")
}

func TestConflictResolver(t *testing.T) {
suite.Run(t, new(ResolverSuite))
}
137 changes: 91 additions & 46 deletions plan/plan.go
Original file line number Diff line number Diff line change
@@ -18,7 +18,6 @@ package plan

import (
"github.com/kubernetes-incubator/external-dns/endpoint"
log "github.com/sirupsen/logrus"
)

// Plan can convert a list of desired and current records to a series of create,
@@ -47,55 +46,102 @@ type Changes struct {
Delete []*endpoint.Endpoint
}

// Calculate computes the actions needed to move current state towards desired
// state. It then passes those changes to the current policy for further
// processing. It returns a copy of Plan with the changes populated.
func (p *Plan) Calculate() *Plan {
changes := &Changes{}
// planTable is a supplementary struct for Plan
// each row correspond to a dnsName -> (current record + all desired records)
/*
planTable: (-> = target)
--------------------------------------------------------
DNSName | Current record | Desired Records |
--------------------------------------------------------
foo.com | -> 1.1.1.1 | [->1.1.1.1, ->elb.com] | = no action
--------------------------------------------------------
bar.com | | [->191.1.1.1, ->190.1.1.1] | = create (bar.com -> 190.1.1.1)
--------------------------------------------------------
"=", i.e. result of calculation relies on supplied ConflictResolver
*/
type planTable struct {
rows map[string]*planTableRow
resolver ConflictResolver
}

// Ensure all desired records exist. For each desired record make sure it's
// either created or updated.
for _, desired := range p.Desired {
// Get the matching current record if it exists.
current, exists := recordExists(desired, p.Current)
func newPlanTable() planTable { //TODO: make resolver configurable
return planTable{map[string]*planTableRow{}, PerResource{}}
}

// If there's no current record create desired record.
if !exists {
changes.Create = append(changes.Create, desired)
continue
}
// planTableRow
// current corresponds to the record currently occupying dns name on the dns provider
// candidates corresponds to the list of records which would like to have this dnsName
type planTableRow struct {
current *endpoint.Endpoint
candidates []*endpoint.Endpoint
}

targetChanged := targetChanged(desired, current)
shouldUpdateTTL := shouldUpdateTTL(desired, current)
func (t planTable) addCurrent(e *endpoint.Endpoint) {
if _, ok := t.rows[e.DNSName]; !ok {
t.rows[e.DNSName] = &planTableRow{}
}
t.rows[e.DNSName].current = e
}

if !targetChanged && !shouldUpdateTTL {
log.Debugf("Skipping endpoint %v because nothing has changed", desired)
func (t planTable) addCandidate(e *endpoint.Endpoint) {
if _, ok := t.rows[e.DNSName]; !ok {
t.rows[e.DNSName] = &planTableRow{}
}
t.rows[e.DNSName].candidates = append(t.rows[e.DNSName].candidates, e)
}

// TODO: allows record type change, which might not be supported by all dns providers
func (t planTable) getUpdates() (updateNew []*endpoint.Endpoint, updateOld []*endpoint.Endpoint) {
for _, row := range t.rows {
if row.current != nil && len(row.candidates) > 0 { //dns name is taken
update := t.resolver.ResolveUpdate(row.current, row.candidates)
// compare "update" to "current" to figure out if actual update is required
if shouldUpdateTTL(update, row.current) || targetChanged(update, row.current) {
Copy link
Author

@ideahitme ideahitme Dec 4, 2017

Choose a reason for hiding this comment

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

should we maybe not allow record type change, or make it optional, or provide an option to disallow it ?

inheritOwner(row.current, update)
updateNew = append(updateNew, update)
updateOld = append(updateOld, row.current)
}
continue
}
}
return
}

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
func (t planTable) getCreates() (createList []*endpoint.Endpoint) {
for _, row := range t.rows {
if row.current == nil { //dns name not taken
createList = append(createList, t.resolver.ResolveCreate(row.candidates))
}
}
return
}

if !shouldUpdateTTL {
desired.RecordTTL = current.RecordTTL
func (t planTable) getDeletes() (deleteList []*endpoint.Endpoint) {
for _, row := range t.rows {
if row.current != nil && len(row.candidates) == 0 {
deleteList = append(deleteList, row.current)
}

changes.UpdateNew = append(changes.UpdateNew, desired)
}
return
}

// Calculate computes the actions needed to move current state towards desired
// state. It then passes those changes to the current policy for further
// processing. It returns a copy of Plan with the changes populated.
func (p *Plan) Calculate() *Plan {
t := newPlanTable()

// Ensure all undesired records are removed. Each current record that cannot
// be found in the list of desired records is removed.
for _, current := range p.Current {
if _, exists := recordExists(current, p.Desired); !exists {
changes.Delete = append(changes.Delete, current)
}
t.addCurrent(current)
}
for _, desired := range p.Desired {
t.addCandidate(desired)
}

// Apply policies to list of changes.
changes := &Changes{}
changes.Create = t.getCreates()
changes.Delete = t.getDeletes()
changes.UpdateNew, changes.UpdateOld = t.getUpdates()
for _, pol := range p.Policies {
changes = pol.Apply(changes)
}
@@ -109,6 +155,16 @@ func (p *Plan) Calculate() *Plan {
return plan
}

func inheritOwner(from, to *endpoint.Endpoint) {
if to.Labels == nil {
to.Labels = map[string]string{}
}
if from.Labels == nil {
from.Labels = map[string]string{}
}
to.Labels[endpoint.OwnerLabelKey] = from.Labels[endpoint.OwnerLabelKey]
}

func targetChanged(desired, current *endpoint.Endpoint) bool {
return desired.Target != current.Target
}
@@ -119,14 +175,3 @@ func shouldUpdateTTL(desired, current *endpoint.Endpoint) bool {
}
return desired.RecordTTL != current.RecordTTL
}

// 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 {
return record, true
}
}

return nil, false
}
462 changes: 316 additions & 146 deletions plan/plan_test.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion registry/registry.go
Original file line number Diff line number Diff line change
@@ -23,7 +23,7 @@ import (
)

// Registry is an interface which should enables ownership concept in external-dns
// Records() returns ALL records registered with DNS provider (TODO: for multi-zone support return all records)
// Records() returns ALL records registered with DNS provider
// each entry includes owner information
// ApplyChanges(changes *plan.Changes) propagates the changes to the DNS Provider API and correspondingly updates ownership depending on type of registry being used
type Registry interface {
65 changes: 34 additions & 31 deletions registry/txt.go
Original file line number Diff line number Diff line change
@@ -19,23 +19,13 @@ package registry
import (
"errors"

"fmt"
"regexp"
"strings"

"github.com/kubernetes-incubator/external-dns/endpoint"
"github.com/kubernetes-incubator/external-dns/plan"
"github.com/kubernetes-incubator/external-dns/provider"
)

const (
txtLabelFormat = "\"heritage=external-dns,external-dns/owner=%s\""
)

var (
txtLabelRegex = regexp.MustCompile("^\"heritage=external-dns,external-dns/owner=(.+)\"")
)

// TXTRegistry implements registry interface with ownership implemented via associated TXT records
type TXTRegistry struct {
provider provider.Provider
@@ -67,31 +57,40 @@ func (im *TXTRegistry) Records() ([]*endpoint.Endpoint, error) {
return nil, err
}

endpoints := make([]*endpoint.Endpoint, 0)
endpoints := []*endpoint.Endpoint{}

ownerMap := map[string]string{}
labelMap := map[string]endpoint.Labels{}

for _, record := range records {
if record.RecordType != endpoint.RecordTypeTXT {
endpoints = append(endpoints, record)
continue
}
ownerID := im.extractOwnerID(record.Target)
if ownerID == "" {
labels, err := endpoint.NewLabelsFromString(record.Target)
if err == endpoint.ErrInvalidHeritage {
//if no heritage is found or it is invalid
//case when value of txt record cannot be identified
//record will not be removed as it will have empty owner
endpoints = append(endpoints, record)
continue
}
if err != nil {
return nil, err
}
endpointDNSName := im.mapper.toEndpointName(record.DNSName)
ownerMap[endpointDNSName] = ownerID
labelMap[endpointDNSName] = labels
}

for _, ep := range endpoints {
ep.Labels[endpoint.OwnerLabelKey] = ownerMap[ep.DNSName]
if labels, ok := labelMap[ep.DNSName]; ok {
ep.Labels = labels
} else {
//this indicates that owner could not be identified, as there is no corresponding TXT record
ep.Labels = endpoint.NewLabels()
}
}

return endpoints, err
return endpoints, nil
}

// ApplyChanges updates dns provider with the changes
@@ -103,36 +102,40 @@ func (im *TXTRegistry) ApplyChanges(changes *plan.Changes) error {
UpdateOld: filterOwnedRecords(im.ownerID, changes.UpdateOld),
Delete: filterOwnedRecords(im.ownerID, changes.Delete),
}

for _, r := range filteredChanges.Create {
txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), im.getTXTLabel(), endpoint.RecordTypeTXT)
r.Labels[endpoint.OwnerLabelKey] = im.ownerID
txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), r.Labels.Serialize(true), endpoint.RecordTypeTXT)
filteredChanges.Create = append(filteredChanges.Create, txt)
}

for _, r := range filteredChanges.Delete {
txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), im.getTXTLabel(), endpoint.RecordTypeTXT)
txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), r.Labels.Serialize(true), endpoint.RecordTypeTXT)

// when we delete TXT records for which value has changed (due to new label) this would still work because
// !!! TXT record value is uniquely generated from the Labels of the endpoint. Hence old TXT record can be uniquely reconstructed
Copy link
Contributor

@szuecs szuecs Dec 11, 2017

Choose a reason for hiding this comment

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

the comment is here and in line 128. I am not sure if this makes sense to me. I think this is more a property of the ApplyChanges function of how it does this change or do I miss something?

Copy link
Author

Choose a reason for hiding this comment

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

I added the comment here because what is going on here is a bit of magic. With this PR change we cannot predict what is the value stored in the TXT record, and for the TXT record to be correctly deleted/updated we should make sure its value matches what is stored on DNS provider side. And this is done via regenerating txt record value from labels

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

filteredChanges.Delete = append(filteredChanges.Delete, txt)
}

// make sure TXT records are consistently updated as well
for _, r := range filteredChanges.UpdateNew {
Copy link
Contributor

Choose a reason for hiding this comment

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

All 3 for loops are basically the same, can you refactor them with "extract method" and call them 3 times, for example:

 filteredChanges.Delete = append(filteredChanges.Delete, yourNewFunction()... )
 filteredChanges.UpdateNew = append(filteredChanges.UpdateNew, yourNewFunction()... )
 filteredChanges.UpdateOld = append(filteredChanges.UpdateOld, yourNewFunction()... )

txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), r.Labels.Serialize(true), endpoint.RecordTypeTXT)
filteredChanges.UpdateNew = append(filteredChanges.UpdateNew, txt)
}
// make sure TXT records are consistently updated as well
for _, r := range filteredChanges.UpdateOld {
txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), r.Labels.Serialize(true), endpoint.RecordTypeTXT)
// when we updateOld TXT records for which value has changed (due to new label) this would still work because
// !!! TXT record value is uniquely generated from the Labels of the endpoint. Hence old TXT record can be uniquely reconstructed
filteredChanges.UpdateOld = append(filteredChanges.UpdateOld, txt)
}

return im.provider.ApplyChanges(filteredChanges)
}

/**
TXT registry specific private methods
*/

func (im *TXTRegistry) getTXTLabel() string {
return fmt.Sprintf(txtLabelFormat, im.ownerID)
}

func (im *TXTRegistry) extractOwnerID(txtLabel string) string {
if matches := txtLabelRegex.FindStringSubmatch(txtLabel); len(matches) == 2 {
return matches[1]
}
return ""
}

/**
nameMapper defines interface which maps the dns name defined for the source
to the dns name which TXT record will be created with
29 changes: 20 additions & 9 deletions registry/txt_test.go
Original file line number Diff line number Diff line change
@@ -132,6 +132,7 @@ func testTXTRegistryRecordsPrefixed(t *testing.T) {

r, _ := NewTXTRegistry(p, "txt.", "owner")
records, _ := r.Records()

assert.True(t, testutils.SameEndpoints(records, expectedRecords))
}

@@ -142,7 +143,7 @@ func testTXTRegistryRecordsNoPrefix(t *testing.T) {
Create: []*endpoint.Endpoint{
newEndpointWithOwner("foo.test-zone.example.org", "foo.loadbalancer.com", endpoint.RecordTypeCNAME, ""),
newEndpointWithOwner("bar.test-zone.example.org", "my-domain.com", endpoint.RecordTypeCNAME, ""),
newEndpointWithOwner("txt.bar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""),
newEndpointWithOwner("txt.bar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""),
newEndpointWithOwner("txt.bar.test-zone.example.org", "baz.test-zone.example.org", endpoint.RecordTypeCNAME, ""),
newEndpointWithOwner("qux.test-zone.example.org", "random", endpoint.RecordTypeTXT, ""),
newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, ""),
@@ -173,7 +174,8 @@ func testTXTRegistryRecordsNoPrefix(t *testing.T) {
Target: "baz.test-zone.example.org",
RecordType: endpoint.RecordTypeCNAME,
Labels: map[string]string{
endpoint.OwnerLabelKey: "owner",
endpoint.OwnerLabelKey: "owner",
endpoint.ResourceLabelKey: "ingress/default/my-ingress",
},
},
{
@@ -233,32 +235,34 @@ func testTXTRegistryApplyChangesWithPrefix(t *testing.T) {

changes := &plan.Changes{
Create: []*endpoint.Endpoint{
newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", "", ""),
newEndpointWithOwnerResource("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", "", "", "ingress/default/my-ingress"),
},
Delete: []*endpoint.Endpoint{
newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"),
},
UpdateNew: []*endpoint.Endpoint{
newEndpointWithOwner("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"),
newEndpointWithOwnerResource("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress-2"),
},
UpdateOld: []*endpoint.Endpoint{
newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"),
},
}
expected := &plan.Changes{
Create: []*endpoint.Endpoint{
newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", "", ""),
newEndpointWithOwner("txt.new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""),
newEndpointWithOwnerResource("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", "", "owner", "ingress/default/my-ingress"),
newEndpointWithOwner("txt.new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress\"", endpoint.RecordTypeTXT, ""),
},
Delete: []*endpoint.Endpoint{
newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"),
newEndpointWithOwner("txt.foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""),
},
UpdateNew: []*endpoint.Endpoint{
newEndpointWithOwner("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"),
newEndpointWithOwnerResource("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner", "ingress/default/my-ingress-2"),
newEndpointWithOwner("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner,external-dns/resource=ingress/default/my-ingress-2\"", endpoint.RecordTypeTXT, ""),
},
UpdateOld: []*endpoint.Endpoint{
newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"),
newEndpointWithOwner("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""),
},
}
p.OnApplyChanges = func(got *plan.Changes) {
@@ -300,7 +304,7 @@ func testTXTRegistryApplyChangesNoPrefix(t *testing.T) {

changes := &plan.Changes{
Create: []*endpoint.Endpoint{
newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", "", ""),
newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, ""),
},
Delete: []*endpoint.Endpoint{
newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"),
@@ -314,7 +318,7 @@ func testTXTRegistryApplyChangesNoPrefix(t *testing.T) {
}
expected := &plan.Changes{
Create: []*endpoint.Endpoint{
newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", "", ""),
newEndpointWithOwner("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", endpoint.RecordTypeCNAME, "owner"),
newEndpointWithOwner("new-record-1.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""),
},
Delete: []*endpoint.Endpoint{
@@ -354,3 +358,10 @@ func newEndpointWithOwner(dnsName, target, recordType, ownerID string) *endpoint
e.Labels[endpoint.OwnerLabelKey] = ownerID
return e
}

func newEndpointWithOwnerResource(dnsName, target, recordType, ownerID, resource string) *endpoint.Endpoint {
e := endpoint.NewEndpoint(dnsName, target, recordType)
e.Labels[endpoint.OwnerLabelKey] = ownerID
e.Labels[endpoint.ResourceLabelKey] = resource
return e
}
7 changes: 7 additions & 0 deletions source/ingress.go
Original file line number Diff line number Diff line change
@@ -105,6 +105,7 @@ func (sc *ingressSource) Endpoints() ([]*endpoint.Endpoint, error) {
}

log.Debugf("Endpoints generated from ingress: %s/%s: %v", ing.Namespace, ing.Name, ingEndpoints)
sc.setResourceLabel(ing, ingEndpoints)
endpoints = append(endpoints, ingEndpoints...)
}

@@ -197,6 +198,12 @@ func (sc *ingressSource) filterByAnnotations(ingresses []v1beta1.Ingress) ([]v1b
return filteredList, nil
}

func (sc *ingressSource) setResourceLabel(ingress v1beta1.Ingress, endpoints []*endpoint.Endpoint) {
for _, ep := range endpoints {
ep.Labels[endpoint.ResourceLabelKey] = fmt.Sprintf("ingress/%s/%s", ingress.Namespace, ingress.Name)
}
}

// endpointsFromIngress extracts the endpoints from ingress object
func endpointsFromIngress(ing *v1beta1.Ingress) []*endpoint.Endpoint {
var endpoints []*endpoint.Endpoint
38 changes: 38 additions & 0 deletions source/ingress_test.go
Original file line number Diff line number Diff line change
@@ -28,12 +28,50 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
)

// Validates that ingressSource is a Source
var _ Source = &ingressSource{}

type IngressSuite struct {
suite.Suite
sc Source
fooWithTargets *v1beta1.Ingress
}

func (suite *IngressSuite) SetupTest() {
fakeClient := fake.NewSimpleClientset()
var err error

suite.sc, err = NewIngressSource(
fakeClient,
"",
"",
"{{.Name}}",
)
suite.NoError(err, "should initialize ingress source")

suite.fooWithTargets = (fakeIngress{
name: "foo-with-targets",
namespace: "default",
dnsnames: []string{"foo"},
ips: []string{"8.8.8.8"},
hostnames: []string{"v1"},
}).Ingress()
_, err = fakeClient.Extensions().Ingresses(suite.fooWithTargets.Namespace).Create(suite.fooWithTargets)
suite.NoError(err, "should succeed")
}

func (suite *IngressSuite) TestResourceLabelIsSet() {
endpoints, _ := suite.sc.Endpoints()
for _, ep := range endpoints {
suite.Equal("ingress/default/foo-with-targets", ep.Labels[endpoint.ResourceLabelKey], "should set correct resource label")
}
}

func TestIngress(t *testing.T) {
suite.Run(t, new(IngressSuite))
t.Run("endpointsFromIngress", testEndpointsFromIngress)
t.Run("Endpoints", testIngressEndpoints)
}
7 changes: 7 additions & 0 deletions source/service.go
Original file line number Diff line number Diff line change
@@ -115,6 +115,7 @@ func (sc *serviceSource) Endpoints() ([]*endpoint.Endpoint, error) {
}

log.Debugf("Endpoints generated from service: %s/%s: %v", svc.Namespace, svc.Name, svcEndpoints)
sc.setResourceLabel(svc, svcEndpoints)
endpoints = append(endpoints, svcEndpoints...)
}

@@ -210,6 +211,12 @@ func (sc *serviceSource) filterByAnnotations(services []v1.Service) ([]v1.Servic
return filteredList, nil
}

func (sc *serviceSource) setResourceLabel(service v1.Service, endpoints []*endpoint.Endpoint) {
for _, ep := range endpoints {
ep.Labels[endpoint.ResourceLabelKey] = fmt.Sprintf("service/%s/%s", service.Namespace, service.Name)
}
}

func (sc *serviceSource) generateEndpoints(svc *v1.Service, hostname string) []*endpoint.Endpoint {
var endpoints []*endpoint.Endpoint

53 changes: 53 additions & 0 deletions source/service_test.go
Original file line number Diff line number Diff line change
@@ -28,9 +28,62 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
)

type ServiceSuite struct {
suite.Suite
sc Source
fooWithTargets *v1.Service
}

func (suite *ServiceSuite) SetupTest() {
fakeClient := fake.NewSimpleClientset()
var err error

suite.sc, err = NewServiceSource(
fakeClient,
"",
"",
"{{.Name}}",
"",
false,
)
suite.fooWithTargets = &v1.Service{
Spec: v1.ServiceSpec{
Type: v1.ServiceTypeLoadBalancer,
},
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
Name: "foo-with-targets",
Annotations: map[string]string{},
},
Status: v1.ServiceStatus{
LoadBalancer: v1.LoadBalancerStatus{
Ingress: []v1.LoadBalancerIngress{
{IP: "8.8.8.8"},
{Hostname: "foo"},
},
},
},
}

suite.NoError(err, "should initialize service source")

_, err = fakeClient.CoreV1().Services(suite.fooWithTargets.Namespace).Create(suite.fooWithTargets)
suite.NoError(err, "should successfully create service")

}

func (suite *ServiceSuite) TestResourceLabelIsSet() {
endpoints, _ := suite.sc.Endpoints()
for _, ep := range endpoints {
suite.Equal("service/default/foo-with-targets", ep.Labels[endpoint.ResourceLabelKey], "should set correct resource label")
}
}

func TestServiceSource(t *testing.T) {
suite.Run(t, new(ServiceSuite))
t.Run("Interface", testServiceSourceImplementsSource)
t.Run("NewServiceSource", testServiceSourceNewServiceSource)
t.Run("Endpoints", testServiceSourceEndpoints)