Skip to content

Commit 4de8bd6

Browse files
ivankatliarchukivankatliarchuk
authored
fix(endpoint): deduplicate targets (#5805)
* fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk <[email protected]> * fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk <[email protected]> * fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk <[email protected]> * fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk <[email protected]> * fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk <[email protected]> * fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk <[email protected]> * fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk <[email protected]> --------- Signed-off-by: ivan katliarchuk <[email protected]>
1 parent 89191f1 commit 4de8bd6

File tree

9 files changed

+365
-54
lines changed

9 files changed

+365
-54
lines changed

‎endpoint/endpoint.go‎

Lines changed: 4 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,12 @@ package endpoint
1919
import (
2020
"fmt"
2121
"net/netip"
22-
"slices"
2322
"sort"
2423
"strconv"
2524
"strings"
2625

2726
log "github.com/sirupsen/logrus"
27+
"k8s.io/utils/set"
2828

2929
"sigs.k8s.io/external-dns/pkg/events"
3030
)
@@ -80,11 +80,10 @@ type MXTarget struct {
8080
host string
8181
}
8282

83-
// NewTargets is a convenience method to create a new Targets object from a vararg of strings
83+
// NewTargets is a convenience method to create a new Targets object from a vararg of strings.
84+
// Returns a new Targets slice with duplicates removed and elements sorted in order.
8485
func NewTargets(target ...string) Targets {
85-
t := make(Targets, 0, len(target))
86-
t = append(t, target...)
87-
return t
86+
return set.New(target...).SortedList()
8887
}
8988

9089
func (t Targets) String() string {
@@ -374,20 +373,6 @@ func (e *Endpoint) Describe() string {
374373
return fmt.Sprintf("record:%s, owner:%s, type:%s, targets:%s", e.DNSName, e.SetIdentifier, e.RecordType, strings.Join(e.Targets, ", "))
375374
}
376375

377-
// UniqueOrderedTargets removes duplicate targets from the Endpoint and sorts them in lexicographical order.
378-
func (e *Endpoint) UniqueOrderedTargets() {
379-
result := make([]string, 0, len(e.Targets))
380-
existing := make(map[string]bool)
381-
for _, target := range e.Targets {
382-
if _, ok := existing[target]; !ok {
383-
result = append(result, target)
384-
existing[target] = true
385-
}
386-
}
387-
slices.Sort(result)
388-
e.Targets = result
389-
}
390-
391376
// FilterEndpointsByOwnerID Apply filter to slice of endpoints and return new filtered slice that includes
392377
// only endpoints that match.
393378
func FilterEndpointsByOwnerID(ownerID string, eps []*Endpoint) []*Endpoint {

‎endpoint/endpoint_test.go‎

Lines changed: 37 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ func TestNewTargets(t *testing.T) {
5959
{
6060
name: "multiple targets",
6161
input: []string{"example.com", "8.8.8.8", "::0001"},
62-
expected: Targets{"example.com", "8.8.8.8", "::0001"},
62+
expected: Targets{"8.8.8.8", "::0001", "example.com"},
6363
},
6464
}
6565

@@ -68,7 +68,6 @@ func TestNewTargets(t *testing.T) {
6868
Targets := NewTargets(c.input...)
6969
changedTarget := Targets.String()
7070
assert.Equal(t, c.expected.String(), changedTarget)
71-
7271
})
7372
}
7473
}
@@ -927,58 +926,66 @@ func TestCheckEndpoint(t *testing.T) {
927926
}
928927
}
929928

930-
func TestEndpoint_UniqueOrderedTargets(t *testing.T) {
929+
func TestEndpoint_WithRefObject(t *testing.T) {
930+
ep := &Endpoint{}
931+
ref := &events.ObjectReference{
932+
Kind: "Service",
933+
Namespace: "default",
934+
Name: "my-service",
935+
}
936+
result := ep.WithRefObject(ref)
937+
938+
assert.Equal(t, ref, ep.RefObject(), "refObject should be set")
939+
assert.Equal(t, ep, result, "should return the same Endpoint pointer")
940+
}
941+
942+
func TestTargets_UniqueOrdered(t *testing.T) {
931943
tests := []struct {
932944
name string
933-
targets []string
945+
input Targets
934946
expected Targets
935-
want bool
936947
}{
937948
{
938949
name: "no duplicates",
939-
targets: []string{"b.example.com", "a.example.com"},
950+
input: Targets{"a.example.com", "b.example.com"},
940951
expected: Targets{"a.example.com", "b.example.com"},
941952
},
942953
{
943954
name: "with duplicates",
944-
targets: []string{"a.example.com", "b.example.com", "a.example.com"},
955+
input: Targets{"a.example.com", "b.example.com", "a.example.com"},
945956
expected: Targets{"a.example.com", "b.example.com"},
946957
},
958+
{
959+
name: "all duplicates",
960+
input: []string{"a.example.com", "a.example.com", "a.example.com"},
961+
expected: Targets{"a.example.com"},
962+
},
947963
{
948964
name: "already sorted",
949-
targets: []string{"a.example.com", "b.example.com"},
950-
expected: Targets{"a.example.com", "b.example.com"},
965+
input: Targets{"a.example.com", "c.example.com", "d.example.com"},
966+
expected: Targets{"a.example.com", "c.example.com", "d.example.com"},
951967
},
952968
{
953-
name: "all duplicates",
954-
targets: []string{"a.example.com", "a.example.com", "a.example.com"},
955-
expected: Targets{"a.example.com"},
969+
name: "unsorted input",
970+
input: Targets{"z.example.com", "a.example.com", "m.example.com"},
971+
expected: Targets{"a.example.com", "m.example.com", "z.example.com"},
956972
},
957973
{
958-
name: "empty",
959-
targets: []string{},
974+
name: "empty input",
975+
input: Targets{},
960976
expected: Targets{},
961977
},
978+
{
979+
name: "single element",
980+
input: Targets{"only.example.com"},
981+
expected: Targets{"only.example.com"},
982+
},
962983
}
963984

964985
for _, tt := range tests {
965986
t.Run(tt.name, func(t *testing.T) {
966-
ep := &Endpoint{Targets: tt.targets}
967-
ep.UniqueOrderedTargets()
968-
assert.Equal(t, tt.expected, ep.Targets)
987+
result := NewTargets(tt.input...)
988+
assert.Equal(t, tt.expected, result)
969989
})
970990
}
971991
}
972-
973-
func TestEndpoint_WithRefObject(t *testing.T) {
974-
ep := &Endpoint{}
975-
ref := &events.ObjectReference{
976-
Kind: "Service",
977-
Namespace: "default",
978-
Name: "my-service",
979-
}
980-
result := ep.WithRefObject(ref)
981-
982-
assert.Equal(t, ref, ep.RefObject(), "refObject should be set")
983-
assert.Equal(t, ep, result, "should return the same Endpoint pointer")
984-
}

‎source/endpoints.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,5 +108,5 @@ func EndpointTargetsFromServices(svcInformer coreinformers.ServiceInformer, name
108108
}
109109
}
110110
}
111-
return targets, nil
111+
return endpoint.NewTargets(targets...), nil
112112
}

‎source/endpoints_test.go‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,25 @@ func TestEndpointTargetsFromServices(t *testing.T) {
155155
},
156156
namespace: "default",
157157
selector: map[string]string{"app": "nginx"},
158-
expected: endpoint.Targets{"192.0.2.1", "158.123.32.23"},
158+
expected: endpoint.Targets{"158.123.32.23", "192.0.2.1"},
159+
},
160+
{
161+
name: "matching service with duplicate external IPs",
162+
services: []*corev1.Service{
163+
{
164+
ObjectMeta: metav1.ObjectMeta{
165+
Name: "svc1",
166+
Namespace: "default",
167+
},
168+
Spec: corev1.ServiceSpec{
169+
Selector: map[string]string{"app": "nginx"},
170+
ExternalIPs: []string{"192.0.2.1", "192.0.2.1", "158.123.32.23"},
171+
},
172+
},
173+
},
174+
namespace: "default",
175+
selector: map[string]string{"app": "nginx"},
176+
expected: endpoint.Targets{"158.123.32.23", "192.0.2.1"},
159177
},
160178
{
161179
name: "no matching service as service without selector",

‎source/istio_gateway_test.go‎

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import (
3434
"k8s.io/apimachinery/pkg/types"
3535
"k8s.io/apimachinery/pkg/util/intstr"
3636
"k8s.io/client-go/kubernetes/fake"
37+
"sigs.k8s.io/external-dns/internal/testutils"
3738

3839
"sigs.k8s.io/external-dns/endpoint"
3940
)
@@ -1732,6 +1733,156 @@ func TestTransformerInIstioGatewaySource(t *testing.T) {
17321733
}, rService.Spec.Selector)
17331734
}
17341735

1736+
func TestSingleGatewayMultipleServicesPointingToSameLoadBalancer(t *testing.T) {
1737+
fakeKubeClient := fake.NewClientset()
1738+
fakeIstioClient := istiofake.NewSimpleClientset()
1739+
1740+
gw := &networkingv1beta1.Gateway{
1741+
ObjectMeta: metav1.ObjectMeta{
1742+
Name: "argocd",
1743+
Namespace: "argocd",
1744+
},
1745+
Spec: istionetworking.Gateway{
1746+
Servers: []*istionetworking.Server{
1747+
{
1748+
Hosts: []string{"example.org"},
1749+
Tls: &istionetworking.ServerTLSSettings{
1750+
HttpsRedirect: true,
1751+
},
1752+
},
1753+
{
1754+
Hosts: []string{"example.org"},
1755+
Tls: &istionetworking.ServerTLSSettings{
1756+
ServerCertificate: IstioGatewayIngressSource,
1757+
Mode: istionetworking.ServerTLSSettings_SIMPLE,
1758+
},
1759+
},
1760+
},
1761+
Selector: map[string]string{
1762+
"istio": "ingressgateway",
1763+
},
1764+
},
1765+
}
1766+
1767+
services := []*v1.Service{
1768+
{
1769+
ObjectMeta: metav1.ObjectMeta{
1770+
Name: "istio-ingressgateway",
1771+
Namespace: "default",
1772+
Labels: map[string]string{
1773+
"app": "istio-ingressgateway",
1774+
"istio": "ingressgateway",
1775+
},
1776+
},
1777+
Spec: v1.ServiceSpec{
1778+
Type: v1.ServiceTypeLoadBalancer,
1779+
ClusterIP: "10.118.223.3",
1780+
ClusterIPs: []string{"10.118.223.3"},
1781+
ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyCluster,
1782+
IPFamilies: []v1.IPFamily{v1.IPv4Protocol},
1783+
IPFamilyPolicy: testutils.ToPtr(v1.IPFamilyPolicySingleStack),
1784+
Ports: []v1.ServicePort{
1785+
{
1786+
Name: "http2",
1787+
Port: 80,
1788+
Protocol: v1.ProtocolTCP,
1789+
TargetPort: intstr.FromInt32(8080),
1790+
NodePort: 30127,
1791+
},
1792+
},
1793+
Selector: map[string]string{
1794+
"app": "istio-ingressgateway",
1795+
"istio": "ingressgateway",
1796+
},
1797+
SessionAffinity: v1.ServiceAffinityNone,
1798+
},
1799+
Status: v1.ServiceStatus{
1800+
LoadBalancer: v1.LoadBalancerStatus{
1801+
Ingress: []v1.LoadBalancerIngress{
1802+
{
1803+
IP: "34.66.66.77",
1804+
IPMode: testutils.ToPtr(v1.LoadBalancerIPModeVIP),
1805+
},
1806+
},
1807+
},
1808+
},
1809+
},
1810+
{
1811+
ObjectMeta: metav1.ObjectMeta{
1812+
Name: "istio-ingressgatewayudp",
1813+
Namespace: "default",
1814+
Labels: map[string]string{
1815+
"app": "istio-ingressgatewayudp",
1816+
"istio": "ingressgateway",
1817+
},
1818+
},
1819+
Spec: v1.ServiceSpec{
1820+
Type: v1.ServiceTypeLoadBalancer,
1821+
ClusterIP: "10.118.220.130",
1822+
ClusterIPs: []string{"10.118.220.130"},
1823+
ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyCluster,
1824+
IPFamilies: []v1.IPFamily{v1.IPv4Protocol},
1825+
IPFamilyPolicy: testutils.ToPtr(v1.IPFamilyPolicySingleStack),
1826+
Ports: []v1.ServicePort{
1827+
{
1828+
Name: "upd-dns",
1829+
Port: 53,
1830+
Protocol: v1.ProtocolUDP,
1831+
TargetPort: intstr.FromInt32(5353),
1832+
NodePort: 30873,
1833+
},
1834+
},
1835+
Selector: map[string]string{
1836+
"app": "istio-ingressgatewayudp",
1837+
"istio": "ingressgateway",
1838+
},
1839+
SessionAffinity: v1.ServiceAffinityNone,
1840+
},
1841+
Status: v1.ServiceStatus{
1842+
LoadBalancer: v1.LoadBalancerStatus{
1843+
Ingress: []v1.LoadBalancerIngress{
1844+
{
1845+
IP: "34.66.66.77",
1846+
IPMode: testutils.ToPtr(v1.LoadBalancerIPModeVIP),
1847+
},
1848+
},
1849+
},
1850+
},
1851+
},
1852+
}
1853+
1854+
assert.NotNil(t, services)
1855+
1856+
for _, svc := range services {
1857+
_, err := fakeKubeClient.CoreV1().Services(svc.Namespace).Create(t.Context(), svc, metav1.CreateOptions{})
1858+
require.NoError(t, err)
1859+
}
1860+
1861+
_, err := fakeIstioClient.NetworkingV1beta1().Gateways(gw.Namespace).Create(t.Context(), gw, metav1.CreateOptions{})
1862+
require.NoError(t, err)
1863+
1864+
src, err := NewIstioGatewaySource(
1865+
t.Context(),
1866+
fakeKubeClient,
1867+
fakeIstioClient,
1868+
"",
1869+
"",
1870+
"",
1871+
false,
1872+
false,
1873+
)
1874+
require.NoError(t, err)
1875+
require.NotNil(t, src)
1876+
1877+
got, err := src.Endpoints(t.Context())
1878+
require.NoError(t, err)
1879+
1880+
validateEndpoints(t, got, []*endpoint.Endpoint{
1881+
endpoint.NewEndpoint("example.org", endpoint.RecordTypeA, "34.66.66.77").WithLabel(endpoint.ResourceLabelKey, "gateway/argocd/argocd"),
1882+
endpoint.NewEndpoint("example.org", endpoint.RecordTypeA, "34.66.66.77").WithLabel(endpoint.ResourceLabelKey, "gateway/argocd/argocd"),
1883+
})
1884+
}
1885+
17351886
// gateway specific helper functions
17361887
func newTestGatewaySource(loadBalancerList []fakeIngressGatewayService, ingressList []fakeIngress) (*gatewaySource, error) {
17371888
fakeKubernetesClient := fake.NewClientset()

‎source/service.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -294,10 +294,10 @@ func (sc *serviceSource) Endpoints(_ context.Context) ([]*endpoint.Endpoint, err
294294
continue
295295
}
296296
existing[0].Targets = append(existing[0].Targets, ep.Targets...)
297-
existing[0].UniqueOrderedTargets()
297+
existing[0].Targets = endpoint.NewTargets(existing[0].Targets...)
298298
mergedEndpoints[key] = existing
299299
} else {
300-
ep.UniqueOrderedTargets()
300+
ep.Targets = endpoint.NewTargets(ep.Targets...)
301301
mergedEndpoints[key] = []*endpoint.Endpoint{ep}
302302
}
303303
}

0 commit comments

Comments
 (0)