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

fix: duplicated endpoint per hosted zone #4296

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
b8053fe
Group By Endpoint names per Hosted Zone on AWS
leonardocaylent Mar 3, 2024
e55200a
Group By Endpoint names per Hosted Zone on AWS
leonardocaylent Mar 3, 2024
7c7a1c3
Group By Endpoint names per Hosted Zone on AWS
leonardocaylent Mar 3, 2024
9307374
Group By Endpoint names per Hosted Zone on AWS
leonardocaylent Mar 3, 2024
846f4b4
Going back to endpoint.go
leonardocaylent Mar 5, 2024
294b4c6
Fix format on aws and endpoint files
leonardocaylent Mar 5, 2024
0308cc2
Fix Lint
leonardocaylent Mar 5, 2024
3395eba
Add const to avoid Lint
leonardocaylent Mar 5, 2024
cea0496
Revert "Fix Lint"
leonardocaylent Mar 5, 2024
a3826d5
Update endpoint.go with Key function
leonardocaylent Mar 16, 2024
30c9e77
Fix lint
leonardocaylent Mar 16, 2024
f4f39d8
Merge branch 'kubernetes-sigs:master' into bugfix/group-endpoints-per…
leonardocaylent Mar 16, 2024
0ca2796
Fix Lint
leonardocaylent Mar 16, 2024
82046cc
Removing comments and clean up
leonardocaylent Mar 16, 2024
d3c2f47
Merge remote-tracking branch 'origin/master' into bugfix/group-endpoi…
leonardocaylent Mar 29, 2024
7fe2d3f
Fix for duplicated endpoints and unit tests
leonardocaylent Mar 29, 2024
2b3da1b
Fix for duplicated endpoints and unit tests
leonardocaylent Mar 30, 2024
6066b70
Fix for duplicated endpoints
leonardocaylent Apr 4, 2024
3ca4d02
Update endpoint/endpoint.go
leonardocaylent Apr 4, 2024
17ce6b4
Merge remote-tracking branch 'origin/master' into bugfix/group-endpoi…
leonardocaylent Apr 5, 2024
deba1ea
Fix suggestions
leonardocaylent Apr 5, 2024
4fb2f2e
Specific bugfix near the root cause for duplicated deletes on plan.go
leonardocaylent Apr 5, 2024
ba56b7a
Merge remote-tracking branch 'origin/master' into bugfix/group-endpoi…
leonardocaylent Apr 6, 2024
d9b7439
Specify and clarify root cause of issue 4241
leonardocaylent Apr 6, 2024
05ca35e
Merge remote-tracking branch 'origin/master' into bugfix/group-endpoi…
leonardocaylent Apr 22, 2024
64d0833
Final fix for error on delete
leonardocaylent Apr 22, 2024
5190777
Fix unrelated change on service_test.go address
leonardocaylent Apr 22, 2024
56024fd
Merge remote-tracking branch 'origin/master' into bugfix/group-endpoi…
leonardocaylent Apr 23, 2024
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
21 changes: 21 additions & 0 deletions endpoint/endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -374,3 +374,24 @@ type DNSEndpointList struct {
metav1.ListMeta `json:"metadata,omitempty"`
Items []DNSEndpoint `json:"items"`
}

// RemoveDuplicates returns a slice holding the unique endpoints.
// This function doesn't contemplate the Targets of an Endpoint
// as part of the primary Key
func RemoveDuplicates(endpoints []*Endpoint) []*Endpoint {
visited := make(map[EndpointKey]struct{})
result := []*Endpoint{}

for _, ep := range endpoints {
key := ep.Key()

if _, found := visited[key]; !found {
result = append(result, ep)
visited[key] = struct{}{}
} else {
log.Debugf(`Skipping duplicated endpoint: %v`, ep)
}
}

return result
}
133 changes: 133 additions & 0 deletions endpoint/endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -252,3 +252,136 @@ func TestIsOwnedBy(t *testing.T) {
})
}
}

func TestDuplicatedEndpointsWithSimpleZone(t *testing.T) {
foo1 := &Endpoint{
DNSName: "foo.com",
RecordType: RecordTypeA,
Labels: Labels{
OwnerLabelKey: "foo",
},
}
foo2 := &Endpoint{
DNSName: "foo.com",
RecordType: RecordTypeA,
Labels: Labels{
OwnerLabelKey: "foo",
},
}
bar := &Endpoint{
DNSName: "foo.com",
RecordType: RecordTypeA,
Labels: Labels{
OwnerLabelKey: "bar",
},
}

type args struct {
eps []*Endpoint
}
tests := []struct {
name string
args args
want []*Endpoint
}{
{
name: "filter values",
args: args{
eps: []*Endpoint{
foo1,
foo2,
bar,
},
},
want: []*Endpoint{
foo1,
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := RemoveDuplicates(tt.args.eps); !reflect.DeepEqual(got, tt.want) {
t.Errorf("RemoveDuplicates() = %v, want %v", got, tt.want)
}
})
}
}

func TestDuplicatedEndpointsWithOverlappingZones(t *testing.T) {
foo1 := &Endpoint{
DNSName: "internal.foo.com",
RecordType: RecordTypeA,
Labels: Labels{
OwnerLabelKey: "foo",
},
}
foo2 := &Endpoint{
DNSName: "internal.foo.com",
RecordType: RecordTypeA,
Labels: Labels{
OwnerLabelKey: "foo",
},
}
foo3 := &Endpoint{
DNSName: "foo.com",
RecordType: RecordTypeA,
Labels: Labels{
OwnerLabelKey: "foo",
},
}
foo4 := &Endpoint{
DNSName: "foo.com",
RecordType: RecordTypeA,
Labels: Labels{
OwnerLabelKey: "foo",
},
}
bar := &Endpoint{
DNSName: "internal.foo.com",
RecordType: RecordTypeA,
Labels: Labels{
OwnerLabelKey: "bar",
},
}
bar2 := &Endpoint{
DNSName: "foo.com",
RecordType: RecordTypeA,
Labels: Labels{
OwnerLabelKey: "bar",
},
}

type args struct {
eps []*Endpoint
}
tests := []struct {
name string
args args
want []*Endpoint
}{
{
name: "filter values",
args: args{
eps: []*Endpoint{
foo1,
foo2,
foo3,
foo4,
bar,
bar2,
},
},
want: []*Endpoint{
Copy link
Contributor

@yurrriq yurrriq Apr 4, 2024

Choose a reason for hiding this comment

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

The first instance of an duplicate endpoint is the one that's kept. Is that an agreeable implementation detail? Thinking out loud here... registry.Records() is implemented by each provider, e.g. AWS, which calls (a variant of) ListResourceRecordSets under the hood which "sorts results first by DNS name with the labels reversed... When multiple records have the same DNS name, ListResourceRecordSets sorts results by the record type."

ref: https://pkg.go.dev/github.com/aws/aws-sdk-go/service/route53#hdr-Sort_order

If yes, then this PR seems a good solution to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The concern is that even using the EndpointKey function, global and weighted records don't have enough test coverage and I'm not familiar if they use the same targets, but that's what dedupsource is using to generate the primary key, without the recordtype. The real questions here are "What use cases do we have that needs endpoints with the same "Key" but different targets?" "What should the endpoint's primary key be? Does it need to change across providers?"

foo1,
foo3,
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := RemoveDuplicates(tt.args.eps); !reflect.DeepEqual(got, tt.want) {
t.Errorf("RemoveDuplicates() = %v, want %v", got, tt.want)
}
})
}
}
1 change: 1 addition & 0 deletions plan/plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ func (p *Plan) Calculate() *Plan {
// filter out updates this external dns does not have ownership claim over
if p.OwnerID != "" {
changes.Delete = endpoint.FilterEndpointsByOwnerID(p.OwnerID, changes.Delete)
changes.Delete = endpoint.RemoveDuplicates(changes.Delete)
changes.UpdateOld = endpoint.FilterEndpointsByOwnerID(p.OwnerID, changes.UpdateOld)
changes.UpdateNew = endpoint.FilterEndpointsByOwnerID(p.OwnerID, changes.UpdateNew)
}
Expand Down
Loading