-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Changes from 15 commits
baaf72a
bc81730
41a3119
1ff3d43
b247a75
d1979ce
ada43e4
c4e6188
d51b1b0
a4fb4bc
88ef640
36ce919
6e93afe
b543159
dbafc58
93f40c6
aaf5c9c
a0c3911
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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" | ||
) | ||
|
||
// 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] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will try it out :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, ",") | ||
} |
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)) | ||
} |
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I would still prefer them to be separated:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed!