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 1 commit
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
Prev Previous commit
Next Next commit
make labels have its own type, add serialization as a method
ideahitme committed Nov 30, 2017
commit b543159883b607ded321747402bb9560cd1b677e
17 changes: 2 additions & 15 deletions endpoint/endpoint.go
Original file line number Diff line number Diff line change
@@ -22,10 +22,6 @@ import (
)

const (
// 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"
// RecordTypeA is a RecordType enum value
RecordTypeA = "A"
// RecordTypeCNAME is a RecordType enum value
@@ -53,7 +49,7 @@ type Endpoint struct {
// TTL for the record
RecordTTL TTL
// Labels stores labels defined for the Endpoint
Labels map[string]string //TODO: this should be a struct of its own, to have easier validation
Labels Labels
}

// NewEndpoint initialization method to be used to create an endpoint
@@ -67,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)
}
61 changes: 37 additions & 24 deletions registry/labels.go → endpoint/labels.go
Original file line number Diff line number Diff line change
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package registry
package endpoint

import (
"errors"
@@ -24,37 +24,30 @@ import (
)

var (
errInvalidHeritage = errors.New("heritage is unknown or not found")
ErrInvalidHeritage = errors.New("heritage is unknown or not found")
)

// known keys
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!

)

// serializeLabel transforms endpoints labels into a external-dns format string
func serializeLabel(labels map[string]string, surroundQuotes bool) string {
var tokens []string
tokens = append(tokens, fmt.Sprintf("heritage=%s", heritage))
var keys []string
for key := range labels {
keys = append(keys, key)
}
sort.Strings(keys) // sort for consistency
// Labels store metadata related to the endpoint
// it is then stored in a persistent storage via serialization
type Labels map[string]string

for _, key := range keys {
tokens = append(tokens, fmt.Sprintf("%s/%s=%s", heritage, key, labels[key]))
}
if surroundQuotes {
return fmt.Sprintf("\"%s\"", strings.Join(tokens, ","))
}
return strings.Join(tokens, ",")
// NewLabels returns empty Labels
func NewLabels() Labels {
return map[string]string{}
}

// deserializeLabel constructs endpoints labels from a provided format 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
func deserializeLabel(labelText string) (map[string]string, error) {
// 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, ",")
@@ -66,7 +59,7 @@ func deserializeLabel(labelText string) (map[string]string, error) {
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
return nil, ErrInvalidHeritage
}
if key == "heritage" {
foundExternalDNSHeritage = true
@@ -78,8 +71,28 @@ func deserializeLabel(labelText string) (map[string]string, error) {
}

if !foundExternalDNSHeritage {
return nil, errInvalidHeritage
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, ",")
}
45 changes: 22 additions & 23 deletions registry/labels_test.go → endpoint/labels_test.go
Original file line number Diff line number Diff line change
@@ -14,30 +14,29 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package registry
package endpoint

import (
"testing"

"fmt"
"testing"

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

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

func (suite *ParserSuite) SetupTest() {
func (suite *LabelsSuite) SetupTest() {
suite.foo = map[string]string{
"owner": "foo-owner",
"resource": "foo-resource",
@@ -57,37 +56,37 @@ func (suite *ParserSuite) SetupTest() {
suite.multipleHeritageText = "heritage=mate,heritage=external-dns,external-dns/owner=random-owner"
}

func (suite *ParserSuite) TestSerialize() {
suite.Equal(suite.fooAsText, serializeLabel(suite.foo, false), "should serializeLabel")
suite.Equal(suite.fooAsTextWithQuotes, serializeLabel(suite.foo, true), "should serializeLabel")
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 *ParserSuite) TestDeserialize() {
foo, err := deserializeLabel(suite.fooAsText)
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 = deserializeLabel(suite.fooAsTextWithQuotes)
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 := deserializeLabel(suite.barText)
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 := deserializeLabel(suite.noHeritageText)
suite.Equal(errInvalidHeritage, err, "should fail if no heritage is found")
noHeritage, err := NewLabelsFromString(suite.noHeritageText)
suite.Equal(ErrInvalidHeritage, err, "should fail if no heritage is found")
suite.Nil(noHeritage, "should return nil")

wrongHeritage, err := deserializeLabel(suite.wrongHeritageText)
suite.Equal(errInvalidHeritage, err, "should fail if wrong heritage is found")
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 := deserializeLabel(suite.multipleHeritageText)
suite.Equal(errInvalidHeritage, err, "should fail if multiple heritage is found")
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 TestParser(t *testing.T) {
suite.Run(t, new(ParserSuite))
func TestLabels(t *testing.T) {
suite.Run(t, new(LabelsSuite))
}
33 changes: 11 additions & 22 deletions registry/txt.go
Original file line number Diff line number Diff line change
@@ -57,17 +57,17 @@ func (im *TXTRegistry) Records() ([]*endpoint.Endpoint, error) {
return nil, err
}

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

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

for _, record := range records {
if record.RecordType != endpoint.RecordTypeTXT {
endpoints = append(endpoints, record)
continue
}
labels, err := deserializeLabel(record.Target)
if err == errInvalidHeritage {
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
@@ -85,11 +85,8 @@ func (im *TXTRegistry) Records() ([]*endpoint.Endpoint, error) {
if labels, ok := labelMap[ep.DNSName]; ok {
ep.Labels = labels
} else {
//this indicates that owner could not be identified, set empty string
// so that record will not be modified by external-dns
ep.Labels = map[string]string{
endpoint.OwnerLabelKey: "",
}
//this indicates that owner could not be identified, as there is no corresponding TXT record
ep.Labels = endpoint.NewLabels()
}
}

@@ -106,12 +103,13 @@ func (im *TXTRegistry) ApplyChanges(changes *plan.Changes) error {
Delete: filterOwnedRecords(im.ownerID, changes.Delete),
}
for _, r := range filteredChanges.Create {
txt := endpoint.NewEndpoint(im.mapper.toTXTName(r.DNSName), im.createTXTLabel(r), 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.createTXTLabel(r), 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

@@ -120,12 +118,12 @@ func (im *TXTRegistry) ApplyChanges(changes *plan.Changes) error {

// 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), im.createTXTLabel(r), endpoint.RecordTypeTXT)
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), im.createTXTLabel(r), endpoint.RecordTypeTXT)
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)
@@ -138,15 +136,6 @@ func (im *TXTRegistry) ApplyChanges(changes *plan.Changes) error {
TXT registry specific private methods
*/

func (im *TXTRegistry) createTXTLabel(e *endpoint.Endpoint) string {
labels := map[string]string{}
for k, v := range e.Labels {
labels[k] = v
}
labels[endpoint.OwnerLabelKey] = im.ownerID
return serializeLabel(labels, true)
}

/**
nameMapper defines interface which maps the dns name defined for the source
to the dns name which TXT record will be created with
4 changes: 2 additions & 2 deletions registry/txt_test.go
Original file line number Diff line number Diff line change
@@ -249,7 +249,7 @@ func testTXTRegistryApplyChangesWithPrefix(t *testing.T) {
}
expected := &plan.Changes{
Create: []*endpoint.Endpoint{
newEndpointWithOwnerResource("new-record-1.test-zone.example.org", "new-loadbalancer-1.lb.com", "", "", "ingress/default/my-ingress"),
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{
@@ -318,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", endpoint.RecordTypeCNAME, ""),
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{