From a0a05a3547c5c90962d2e2713553ffab25eeb816 Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Thu, 4 Sep 2025 11:59:02 +0100 Subject: [PATCH 1/7] fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk --- source/wrappers/dedupsource.go | 17 +++++++++-- source/wrappers/dedupsource_test.go | 46 +++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/source/wrappers/dedupsource.go b/source/wrappers/dedupsource.go index 961ef45cfc..32aa4e848d 100644 --- a/source/wrappers/dedupsource.go +++ b/source/wrappers/dedupsource.go @@ -20,6 +20,8 @@ import ( "context" "strings" + "k8s.io/utils/set" + log "github.com/sirupsen/logrus" "sigs.k8s.io/external-dns/endpoint" @@ -39,7 +41,7 @@ func NewDedupSource(source source.Source) source.Source { // Endpoints collects endpoints from its wrapped source and returns them without duplicates. func (ms *dedupSource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, error) { log.Debug("dedupSource: collecting endpoints and removing duplicates") - result := []*endpoint.Endpoint{} + result := make([]*endpoint.Endpoint, 0) collected := map[string]bool{} endpoints, err := ms.source.Endpoints(ctx) @@ -52,7 +54,13 @@ func (ms *dedupSource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, err continue } - identifier := strings.Join([]string{ep.RecordType, ep.DNSName, ep.SetIdentifier, ep.Targets.String()}, "/") + targets := ep.Targets + if len(targets) > 1 { + targets = removeDuplicates(targets) + ep.Targets = targets + } + + identifier := strings.Join([]string{ep.RecordType, ep.DNSName, ep.SetIdentifier, targets.String()}, "/") if _, ok := collected[identifier]; ok { log.Debugf("Removing duplicate endpoint %s", ep) @@ -66,6 +74,11 @@ func (ms *dedupSource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, err return result, nil } +func removeDuplicates(targets []string) []string { + s := set.New(targets...) + return s.SortedList() +} + func (ms *dedupSource) AddEventHandler(ctx context.Context, handler func()) { log.Debug("dedupSource: adding event handler") ms.source.AddEventHandler(ctx, handler) diff --git a/source/wrappers/dedupsource_test.go b/source/wrappers/dedupsource_test.go index 3f9ac58bad..5f89e90bb6 100644 --- a/source/wrappers/dedupsource_test.go +++ b/source/wrappers/dedupsource_test.go @@ -20,6 +20,7 @@ import ( "context" "testing" + "github.com/stretchr/testify/assert" "sigs.k8s.io/external-dns/endpoint" "sigs.k8s.io/external-dns/internal/testutils" "sigs.k8s.io/external-dns/source" @@ -123,6 +124,20 @@ func testDedupEndpoints(t *testing.T) { {DNSName: "foo.example.org", Targets: endpoint.Targets{"1.2.3.4"}}, }, }, + { + "no endpoints returns empty endpoints", + []*endpoint.Endpoint{}, + []*endpoint.Endpoint{}, + }, + { + "one endpoint with multiple targets returns one endpoint and targets without duplicates", + []*endpoint.Endpoint{ + {DNSName: "foo.example.org", RecordType: "A", Targets: endpoint.Targets{"1.2.3.4", "34.66.66.77", "34.66.66.77"}}, + }, + []*endpoint.Endpoint{ + {DNSName: "foo.example.org", RecordType: "A", Targets: endpoint.Targets{"1.2.3.4", "34.66.66.77"}}, + }, + }, } { t.Run(tc.title, func(t *testing.T) { mockSource := new(testutils.MockSource) @@ -168,3 +183,34 @@ func TestDedupSource_AddEventHandler(t *testing.T) { }) } } + +func TestRemoveDuplicates(t *testing.T) { + tests := []struct { + name string + input []string + expected []string + }{ + { + name: "removes duplicates and sorts", + input: []string{"b", "a", "a", "c"}, + expected: []string{"a", "b", "c"}, + }, + { + name: "no duplicates", + input: []string{"x", "y", "z"}, + expected: []string{"x", "y", "z"}, + }, + { + name: "empty slice", + input: []string{}, + expected: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := removeDuplicates(tt.input) + assert.Equal(t, tt.expected, got) + }) + } +} From ae85e16ec288c01a3fee94e08ded799fee46b703 Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Thu, 4 Sep 2025 12:03:07 +0100 Subject: [PATCH 2/7] fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk --- source/wrappers/dedupsource.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/source/wrappers/dedupsource.go b/source/wrappers/dedupsource.go index 32aa4e848d..59d3e214a6 100644 --- a/source/wrappers/dedupsource.go +++ b/source/wrappers/dedupsource.go @@ -54,13 +54,11 @@ func (ms *dedupSource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, err continue } - targets := ep.Targets - if len(targets) > 1 { - targets = removeDuplicates(targets) - ep.Targets = targets + if len(ep.Targets) > 1 { + ep.Targets = removeDuplicates(ep.Targets) } - identifier := strings.Join([]string{ep.RecordType, ep.DNSName, ep.SetIdentifier, targets.String()}, "/") + identifier := strings.Join([]string{ep.RecordType, ep.DNSName, ep.SetIdentifier, ep.Targets.String()}, "/") if _, ok := collected[identifier]; ok { log.Debugf("Removing duplicate endpoint %s", ep) @@ -75,8 +73,7 @@ func (ms *dedupSource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, err } func removeDuplicates(targets []string) []string { - s := set.New(targets...) - return s.SortedList() + return set.New(targets...).SortedList() } func (ms *dedupSource) AddEventHandler(ctx context.Context, handler func()) { From dca82e9219476c8a06c8d2a035467673c79b3c76 Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Sat, 6 Sep 2025 15:38:33 +0100 Subject: [PATCH 3/7] fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk --- endpoint/endpoint.go | 20 ++--- endpoint/endpoint_test.go | 49 +++++++++++- source/endpoints.go | 2 +- source/endpoints_test.go | 20 ++++- source/istio_gateway_test.go | 151 +++++++++++++++++++++++++++++++++++ source/service_test.go | 115 ++++++++++++++++++++++++++ 6 files changed, 338 insertions(+), 19 deletions(-) diff --git a/endpoint/endpoint.go b/endpoint/endpoint.go index dbeabcef88..3b01b65072 100644 --- a/endpoint/endpoint.go +++ b/endpoint/endpoint.go @@ -19,12 +19,12 @@ package endpoint import ( "fmt" "net/netip" - "slices" "sort" "strconv" "strings" log "github.com/sirupsen/logrus" + "k8s.io/utils/set" "sigs.k8s.io/external-dns/pkg/events" ) @@ -80,11 +80,10 @@ type MXTarget struct { host string } -// NewTargets is a convenience method to create a new Targets object from a vararg of strings +// NewTargets is a convenience method to create a new Targets object from a vararg of strings. +// Returns a new Targets slice with duplicates removed and elements sorted in order. func NewTargets(target ...string) Targets { - t := make(Targets, 0, len(target)) - t = append(t, target...) - return t + return set.New(target...).SortedList() } func (t Targets) String() string { @@ -376,16 +375,7 @@ func (e *Endpoint) Describe() string { // UniqueOrderedTargets removes duplicate targets from the Endpoint and sorts them in lexicographical order. func (e *Endpoint) UniqueOrderedTargets() { - result := make([]string, 0, len(e.Targets)) - existing := make(map[string]bool) - for _, target := range e.Targets { - if _, ok := existing[target]; !ok { - result = append(result, target) - existing[target] = true - } - } - slices.Sort(result) - e.Targets = result + e.Targets = NewTargets(e.Targets...) } // FilterEndpointsByOwnerID Apply filter to slice of endpoints and return new filtered slice that includes diff --git a/endpoint/endpoint_test.go b/endpoint/endpoint_test.go index 31b26a159b..cf877d97cc 100644 --- a/endpoint/endpoint_test.go +++ b/endpoint/endpoint_test.go @@ -59,7 +59,7 @@ func TestNewTargets(t *testing.T) { { name: "multiple targets", input: []string{"example.com", "8.8.8.8", "::0001"}, - expected: Targets{"example.com", "8.8.8.8", "::0001"}, + expected: Targets{"8.8.8.8", "::0001", "example.com"}, }, } @@ -68,7 +68,6 @@ func TestNewTargets(t *testing.T) { Targets := NewTargets(c.input...) changedTarget := Targets.String() assert.Equal(t, c.expected.String(), changedTarget) - }) } } @@ -982,3 +981,49 @@ func TestEndpoint_WithRefObject(t *testing.T) { assert.Equal(t, ref, ep.RefObject(), "refObject should be set") assert.Equal(t, ep, result, "should return the same Endpoint pointer") } + +func TestTargets_UniqueOrdered(t *testing.T) { + tests := []struct { + name string + input Targets + expected Targets + }{ + { + name: "no duplicates", + input: Targets{"a.example.com", "b.example.com"}, + expected: Targets{"a.example.com", "b.example.com"}, + }, + { + name: "with duplicates", + input: Targets{"a.example.com", "b.example.com", "a.example.com"}, + expected: Targets{"a.example.com", "b.example.com"}, + }, + { + name: "already sorted", + input: Targets{"a.example.com", "c.example.com", "d.example.com"}, + expected: Targets{"a.example.com", "c.example.com", "d.example.com"}, + }, + { + name: "unsorted input", + input: Targets{"z.example.com", "a.example.com", "m.example.com"}, + expected: Targets{"a.example.com", "m.example.com", "z.example.com"}, + }, + { + name: "empty input", + input: Targets{}, + expected: Targets{}, + }, + { + name: "single element", + input: Targets{"only.example.com"}, + expected: Targets{"only.example.com"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := NewTargets(tt.input...) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/source/endpoints.go b/source/endpoints.go index b3667052d6..99c74fd07c 100644 --- a/source/endpoints.go +++ b/source/endpoints.go @@ -108,5 +108,5 @@ func EndpointTargetsFromServices(svcInformer coreinformers.ServiceInformer, name } } } - return targets, nil + return endpoint.NewTargets(targets...), nil } diff --git a/source/endpoints_test.go b/source/endpoints_test.go index 0182646ea2..e64f903e18 100644 --- a/source/endpoints_test.go +++ b/source/endpoints_test.go @@ -155,7 +155,25 @@ func TestEndpointTargetsFromServices(t *testing.T) { }, namespace: "default", selector: map[string]string{"app": "nginx"}, - expected: endpoint.Targets{"192.0.2.1", "158.123.32.23"}, + expected: endpoint.Targets{"158.123.32.23", "192.0.2.1"}, + }, + { + name: "matching service with duplicate external IPs", + services: []*corev1.Service{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "svc1", + Namespace: "default", + }, + Spec: corev1.ServiceSpec{ + Selector: map[string]string{"app": "nginx"}, + ExternalIPs: []string{"192.0.2.1", "192.0.2.1", "158.123.32.23"}, + }, + }, + }, + namespace: "default", + selector: map[string]string{"app": "nginx"}, + expected: endpoint.Targets{"158.123.32.23", "192.0.2.1"}, }, { name: "no matching service as service without selector", diff --git a/source/istio_gateway_test.go b/source/istio_gateway_test.go index 50c02e1851..26ac2e0954 100644 --- a/source/istio_gateway_test.go +++ b/source/istio_gateway_test.go @@ -34,6 +34,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/kubernetes/fake" + "sigs.k8s.io/external-dns/internal/testutils" "sigs.k8s.io/external-dns/endpoint" ) @@ -1732,6 +1733,156 @@ func TestTransformerInIstioGatewaySource(t *testing.T) { }, rService.Spec.Selector) } +func TestSingleGatewayMultipleServicesPointingToSameLoadBalancer(t *testing.T) { + fakeKubeClient := fake.NewClientset() + fakeIstioClient := istiofake.NewSimpleClientset() + + gw := &networkingv1beta1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "argocd", + Namespace: "argocd", + }, + Spec: istionetworking.Gateway{ + Servers: []*istionetworking.Server{ + { + Hosts: []string{"example.org"}, + Tls: &istionetworking.ServerTLSSettings{ + HttpsRedirect: true, + }, + }, + { + Hosts: []string{"example.org"}, + Tls: &istionetworking.ServerTLSSettings{ + ServerCertificate: IstioGatewayIngressSource, + Mode: istionetworking.ServerTLSSettings_SIMPLE, + }, + }, + }, + Selector: map[string]string{ + "istio": "ingressgateway", + }, + }, + } + + services := []*v1.Service{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "istio-ingressgateway", + Namespace: "default", + Labels: map[string]string{ + "app": "istio-ingressgateway", + "istio": "ingressgateway", + }, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + ClusterIP: "10.118.223.3", + ClusterIPs: []string{"10.118.223.3"}, + ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyCluster, + IPFamilies: []v1.IPFamily{v1.IPv4Protocol}, + IPFamilyPolicy: testutils.ToPtr(v1.IPFamilyPolicySingleStack), + Ports: []v1.ServicePort{ + { + Name: "http2", + Port: 80, + Protocol: v1.ProtocolTCP, + TargetPort: intstr.FromInt32(8080), + NodePort: 30127, + }, + }, + Selector: map[string]string{ + "app": "istio-ingressgateway", + "istio": "ingressgateway", + }, + SessionAffinity: v1.ServiceAffinityNone, + }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + { + IP: "34.66.66.77", + IPMode: testutils.ToPtr(v1.LoadBalancerIPModeVIP), + }, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "istio-ingressgatewayudp", + Namespace: "default", + Labels: map[string]string{ + "app": "istio-ingressgatewayudp", + "istio": "ingressgateway", + }, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + ClusterIP: "10.118.220.130", + ClusterIPs: []string{"10.118.220.130"}, + ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyCluster, + IPFamilies: []v1.IPFamily{v1.IPv4Protocol}, + IPFamilyPolicy: testutils.ToPtr(v1.IPFamilyPolicySingleStack), + Ports: []v1.ServicePort{ + { + Name: "upd-dns", + Port: 53, + Protocol: v1.ProtocolUDP, + TargetPort: intstr.FromInt32(5353), + NodePort: 30873, + }, + }, + Selector: map[string]string{ + "app": "istio-ingressgatewayudp", + "istio": "ingressgateway", + }, + SessionAffinity: v1.ServiceAffinityNone, + }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + { + IP: "34.66.66.77", + IPMode: testutils.ToPtr(v1.LoadBalancerIPModeVIP), + }, + }, + }, + }, + }, + } + + assert.NotNil(t, services) + + for _, svc := range services { + _, err := fakeKubeClient.CoreV1().Services(svc.Namespace).Create(t.Context(), svc, metav1.CreateOptions{}) + require.NoError(t, err) + } + + _, err := fakeIstioClient.NetworkingV1beta1().Gateways(gw.Namespace).Create(t.Context(), gw, metav1.CreateOptions{}) + require.NoError(t, err) + + src, err := NewIstioGatewaySource( + t.Context(), + fakeKubeClient, + fakeIstioClient, + "", + "", + "", + false, + false, + ) + require.NoError(t, err) + require.NotNil(t, src) + + res, err := src.Endpoints(t.Context()) + require.NoError(t, err) + + validateEndpoints(t, res, []*endpoint.Endpoint{ + endpoint.NewEndpoint("example.org", endpoint.RecordTypeA, "34.66.66.77").WithLabel("resource", "gateway/argocd/argocd"), + endpoint.NewEndpoint("example.org", endpoint.RecordTypeA, "34.66.66.77").WithLabel("resource", "gateway/argocd/argocd"), + }) +} + // gateway specific helper functions func newTestGatewaySource(loadBalancerList []fakeIngressGatewayService, ingressList []fakeIngress) (*gatewaySource, error) { fakeKubernetesClient := fake.NewClientset() diff --git a/source/service_test.go b/source/service_test.go index 26e4320f6c..5b7590de15 100644 --- a/source/service_test.go +++ b/source/service_test.go @@ -3284,6 +3284,121 @@ func TestHeadlessServices(t *testing.T) { } } +func TestMultipleServicesPointingToSameLoadBalancer(t *testing.T) { + kubernetes := fake.NewClientset() + + services := []*v1.Service{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "ingressgateway", + Namespace: "default", + Labels: map[string]string{ + "app": "istio-ingressgateway", + "istio": "ingressgateway", + }, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + ClusterIP: "10.118.223.3", + ClusterIPs: []string{"10.118.223.3"}, + ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyCluster, + IPFamilies: []v1.IPFamily{v1.IPv4Protocol}, + IPFamilyPolicy: testutils.ToPtr(v1.IPFamilyPolicySingleStack), + Ports: []v1.ServicePort{ + { + Name: "web", + Port: 80, + Protocol: v1.ProtocolTCP, + TargetPort: intstr.FromInt32(80), + }, + }, + Selector: map[string]string{ + "app": "istio-ingressgateway", + "istio": "ingressgateway", + }, + SessionAffinity: v1.ServiceAffinityNone, + }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + { + IP: "34.66.66.77", + IPMode: testutils.ToPtr(v1.LoadBalancerIPModeVIP), + }, + }, + }, + }, + }, + // { + // ObjectMeta: metav1.ObjectMeta{ + // Name: "kafka-2", + // Namespace: "default", + // Labels: map[string]string{ + // "app": "kafka", + // }, + // Annotations: map[string]string{ + // annotations.HostnameKey: "example.org", + // }, + // }, + // Spec: v1.ServiceSpec{ + // Type: v1.ServiceTypeClusterIP, + // ClusterIP: v1.ClusterIPNone, + // ClusterIPs: []string{v1.ClusterIPNone}, + // InternalTrafficPolicy: testutils.ToPtr(v1.ServiceInternalTrafficPolicyCluster), + // IPFamilies: []v1.IPFamily{v1.IPv4Protocol}, + // IPFamilyPolicy: testutils.ToPtr(v1.IPFamilyPolicySingleStack), + // Ports: []v1.ServicePort{ + // { + // Name: "web", + // Port: 80, + // Protocol: v1.ProtocolTCP, + // TargetPort: intstr.FromInt32(80), + // }, + // }, + // Selector: map[string]string{ + // "app": "kafka", + // }, + // SessionAffinity: v1.ServiceAffinityNone, + // }, + // Status: v1.ServiceStatus{ + // LoadBalancer: v1.LoadBalancerStatus{}, + // }, + // }, + } + + assert.NotNil(t, services) + + for _, svc := range services { + _, err := kubernetes.CoreV1().Services(svc.Namespace).Create(t.Context(), svc, metav1.CreateOptions{}) + require.NoError(t, err) + } + + src, err := NewServiceSource( + t.Context(), + kubernetes, + v1.NamespaceAll, + "", + "", + false, + "", + false, + false, + false, + []string{}, + false, + labels.Everything(), + false, + false, + false, + ) + require.NoError(t, err) + assert.NotNil(t, src) + + got, err := src.Endpoints(context.Background()) + require.NoError(t, err) + fmt.Println(got) +} + func TestMultipleHeadlessServicesPointingToPodsOnTheSameNode(t *testing.T) { kubernetes := fake.NewClientset() From 1affbd0c0f654705499ce2e947edf2d46fc7be9e Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Sat, 6 Sep 2025 16:33:32 +0100 Subject: [PATCH 4/7] fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk --- source/wrappers/dedupsource.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/wrappers/dedupsource.go b/source/wrappers/dedupsource.go index 59d3e214a6..1d4abe4f52 100644 --- a/source/wrappers/dedupsource.go +++ b/source/wrappers/dedupsource.go @@ -55,7 +55,7 @@ func (ms *dedupSource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, err } if len(ep.Targets) > 1 { - ep.Targets = removeDuplicates(ep.Targets) + ep.Targets = endpoint.NewTargets(ep.Targets...) } identifier := strings.Join([]string{ep.RecordType, ep.DNSName, ep.SetIdentifier, ep.Targets.String()}, "/") From 78a0a4352b01eac3fd301b2de9066eb1c59624f4 Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Sat, 6 Sep 2025 16:34:18 +0100 Subject: [PATCH 5/7] fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk --- source/wrappers/dedupsource.go | 6 ------ source/wrappers/dedupsource_test.go | 32 ----------------------------- 2 files changed, 38 deletions(-) diff --git a/source/wrappers/dedupsource.go b/source/wrappers/dedupsource.go index 1d4abe4f52..2d9fc6c367 100644 --- a/source/wrappers/dedupsource.go +++ b/source/wrappers/dedupsource.go @@ -20,8 +20,6 @@ import ( "context" "strings" - "k8s.io/utils/set" - log "github.com/sirupsen/logrus" "sigs.k8s.io/external-dns/endpoint" @@ -72,10 +70,6 @@ func (ms *dedupSource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, err return result, nil } -func removeDuplicates(targets []string) []string { - return set.New(targets...).SortedList() -} - func (ms *dedupSource) AddEventHandler(ctx context.Context, handler func()) { log.Debug("dedupSource: adding event handler") ms.source.AddEventHandler(ctx, handler) diff --git a/source/wrappers/dedupsource_test.go b/source/wrappers/dedupsource_test.go index 5f89e90bb6..5af481398a 100644 --- a/source/wrappers/dedupsource_test.go +++ b/source/wrappers/dedupsource_test.go @@ -20,7 +20,6 @@ import ( "context" "testing" - "github.com/stretchr/testify/assert" "sigs.k8s.io/external-dns/endpoint" "sigs.k8s.io/external-dns/internal/testutils" "sigs.k8s.io/external-dns/source" @@ -183,34 +182,3 @@ func TestDedupSource_AddEventHandler(t *testing.T) { }) } } - -func TestRemoveDuplicates(t *testing.T) { - tests := []struct { - name string - input []string - expected []string - }{ - { - name: "removes duplicates and sorts", - input: []string{"b", "a", "a", "c"}, - expected: []string{"a", "b", "c"}, - }, - { - name: "no duplicates", - input: []string{"x", "y", "z"}, - expected: []string{"x", "y", "z"}, - }, - { - name: "empty slice", - input: []string{}, - expected: []string{}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := removeDuplicates(tt.input) - assert.Equal(t, tt.expected, got) - }) - } -} From 5ece076ea194b411a90b3fe2635262226c1e6bb3 Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Sun, 7 Sep 2025 14:15:19 +0100 Subject: [PATCH 6/7] fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk --- source/istio_gateway_test.go | 8 +-- source/service_test.go | 97 +++++++++++++++++++++--------------- 2 files changed, 61 insertions(+), 44 deletions(-) diff --git a/source/istio_gateway_test.go b/source/istio_gateway_test.go index 26ac2e0954..7e4e162e7b 100644 --- a/source/istio_gateway_test.go +++ b/source/istio_gateway_test.go @@ -1874,12 +1874,12 @@ func TestSingleGatewayMultipleServicesPointingToSameLoadBalancer(t *testing.T) { require.NoError(t, err) require.NotNil(t, src) - res, err := src.Endpoints(t.Context()) + got, err := src.Endpoints(t.Context()) require.NoError(t, err) - validateEndpoints(t, res, []*endpoint.Endpoint{ - endpoint.NewEndpoint("example.org", endpoint.RecordTypeA, "34.66.66.77").WithLabel("resource", "gateway/argocd/argocd"), - endpoint.NewEndpoint("example.org", endpoint.RecordTypeA, "34.66.66.77").WithLabel("resource", "gateway/argocd/argocd"), + validateEndpoints(t, got, []*endpoint.Endpoint{ + endpoint.NewEndpoint("example.org", endpoint.RecordTypeA, "34.66.66.77").WithLabel(endpoint.ResourceLabelKey, "gateway/argocd/argocd"), + endpoint.NewEndpoint("example.org", endpoint.RecordTypeA, "34.66.66.77").WithLabel(endpoint.ResourceLabelKey, "gateway/argocd/argocd"), }) } diff --git a/source/service_test.go b/source/service_test.go index 5b7590de15..79d8e37e6b 100644 --- a/source/service_test.go +++ b/source/service_test.go @@ -3290,12 +3290,15 @@ func TestMultipleServicesPointingToSameLoadBalancer(t *testing.T) { services := []*v1.Service{ { ObjectMeta: metav1.ObjectMeta{ - Name: "ingressgateway", + Name: "istio-ingressgateway", Namespace: "default", Labels: map[string]string{ "app": "istio-ingressgateway", "istio": "ingressgateway", }, + Annotations: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "example.org", + }, }, Spec: v1.ServiceSpec{ Type: v1.ServiceTypeLoadBalancer, @@ -3306,10 +3309,11 @@ func TestMultipleServicesPointingToSameLoadBalancer(t *testing.T) { IPFamilyPolicy: testutils.ToPtr(v1.IPFamilyPolicySingleStack), Ports: []v1.ServicePort{ { - Name: "web", + Name: "http2", Port: 80, Protocol: v1.ProtocolTCP, - TargetPort: intstr.FromInt32(80), + TargetPort: intstr.FromInt32(8080), + NodePort: 30127, }, }, Selector: map[string]string{ @@ -3329,41 +3333,51 @@ func TestMultipleServicesPointingToSameLoadBalancer(t *testing.T) { }, }, }, - // { - // ObjectMeta: metav1.ObjectMeta{ - // Name: "kafka-2", - // Namespace: "default", - // Labels: map[string]string{ - // "app": "kafka", - // }, - // Annotations: map[string]string{ - // annotations.HostnameKey: "example.org", - // }, - // }, - // Spec: v1.ServiceSpec{ - // Type: v1.ServiceTypeClusterIP, - // ClusterIP: v1.ClusterIPNone, - // ClusterIPs: []string{v1.ClusterIPNone}, - // InternalTrafficPolicy: testutils.ToPtr(v1.ServiceInternalTrafficPolicyCluster), - // IPFamilies: []v1.IPFamily{v1.IPv4Protocol}, - // IPFamilyPolicy: testutils.ToPtr(v1.IPFamilyPolicySingleStack), - // Ports: []v1.ServicePort{ - // { - // Name: "web", - // Port: 80, - // Protocol: v1.ProtocolTCP, - // TargetPort: intstr.FromInt32(80), - // }, - // }, - // Selector: map[string]string{ - // "app": "kafka", - // }, - // SessionAffinity: v1.ServiceAffinityNone, - // }, - // Status: v1.ServiceStatus{ - // LoadBalancer: v1.LoadBalancerStatus{}, - // }, - // }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "istio-ingressgatewayudp", + Namespace: "default", + Labels: map[string]string{ + "app": "istio-ingressgatewayudp", + "istio": "ingressgateway", + }, + Annotations: map[string]string{ + "external-dns.alpha.kubernetes.io/hostname": "example.org", + }, + }, + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + ClusterIP: "10.118.220.130", + ClusterIPs: []string{"10.118.220.130"}, + ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyCluster, + IPFamilies: []v1.IPFamily{v1.IPv4Protocol}, + IPFamilyPolicy: testutils.ToPtr(v1.IPFamilyPolicySingleStack), + Ports: []v1.ServicePort{ + { + Name: "upd-dns", + Port: 53, + Protocol: v1.ProtocolUDP, + TargetPort: intstr.FromInt32(5353), + NodePort: 30873, + }, + }, + Selector: map[string]string{ + "app": "istio-ingressgatewayudp", + "istio": "ingressgateway", + }, + SessionAffinity: v1.ServiceAffinityNone, + }, + Status: v1.ServiceStatus{ + LoadBalancer: v1.LoadBalancerStatus{ + Ingress: []v1.LoadBalancerIngress{ + { + IP: "34.66.66.77", + IPMode: testutils.ToPtr(v1.LoadBalancerIPModeVIP), + }, + }, + }, + }, + }, } assert.NotNil(t, services) @@ -3394,9 +3408,12 @@ func TestMultipleServicesPointingToSameLoadBalancer(t *testing.T) { require.NoError(t, err) assert.NotNil(t, src) - got, err := src.Endpoints(context.Background()) + got, err := src.Endpoints(t.Context()) require.NoError(t, err) - fmt.Println(got) + + validateEndpoints(t, got, []*endpoint.Endpoint{ + endpoint.NewEndpoint("example.org", endpoint.RecordTypeA, "34.66.66.77").WithLabel(endpoint.ResourceLabelKey, "service/default/istio-ingressgateway"), + }) } func TestMultipleHeadlessServicesPointingToPodsOnTheSameNode(t *testing.T) { From c3be0c13a7a916fa87107b22e99065eaad102bb2 Mon Sep 17 00:00:00 2001 From: ivan katliarchuk Date: Mon, 8 Sep 2025 07:57:00 +0100 Subject: [PATCH 7/7] fix(deduplicate): deduplicate targets Signed-off-by: ivan katliarchuk --- endpoint/endpoint.go | 5 ---- endpoint/endpoint_test.go | 48 ++++----------------------------------- source/service.go | 4 ++-- 3 files changed, 7 insertions(+), 50 deletions(-) diff --git a/endpoint/endpoint.go b/endpoint/endpoint.go index 3b01b65072..67bdfcde42 100644 --- a/endpoint/endpoint.go +++ b/endpoint/endpoint.go @@ -373,11 +373,6 @@ func (e *Endpoint) Describe() string { return fmt.Sprintf("record:%s, owner:%s, type:%s, targets:%s", e.DNSName, e.SetIdentifier, e.RecordType, strings.Join(e.Targets, ", ")) } -// UniqueOrderedTargets removes duplicate targets from the Endpoint and sorts them in lexicographical order. -func (e *Endpoint) UniqueOrderedTargets() { - e.Targets = NewTargets(e.Targets...) -} - // FilterEndpointsByOwnerID Apply filter to slice of endpoints and return new filtered slice that includes // only endpoints that match. func FilterEndpointsByOwnerID(ownerID string, eps []*Endpoint) []*Endpoint { diff --git a/endpoint/endpoint_test.go b/endpoint/endpoint_test.go index cf877d97cc..d899b348d0 100644 --- a/endpoint/endpoint_test.go +++ b/endpoint/endpoint_test.go @@ -926,49 +926,6 @@ func TestCheckEndpoint(t *testing.T) { } } -func TestEndpoint_UniqueOrderedTargets(t *testing.T) { - tests := []struct { - name string - targets []string - expected Targets - want bool - }{ - { - name: "no duplicates", - targets: []string{"b.example.com", "a.example.com"}, - expected: Targets{"a.example.com", "b.example.com"}, - }, - { - name: "with duplicates", - targets: []string{"a.example.com", "b.example.com", "a.example.com"}, - expected: Targets{"a.example.com", "b.example.com"}, - }, - { - name: "already sorted", - targets: []string{"a.example.com", "b.example.com"}, - expected: Targets{"a.example.com", "b.example.com"}, - }, - { - name: "all duplicates", - targets: []string{"a.example.com", "a.example.com", "a.example.com"}, - expected: Targets{"a.example.com"}, - }, - { - name: "empty", - targets: []string{}, - expected: Targets{}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ep := &Endpoint{Targets: tt.targets} - ep.UniqueOrderedTargets() - assert.Equal(t, tt.expected, ep.Targets) - }) - } -} - func TestEndpoint_WithRefObject(t *testing.T) { ep := &Endpoint{} ref := &events.ObjectReference{ @@ -998,6 +955,11 @@ func TestTargets_UniqueOrdered(t *testing.T) { input: Targets{"a.example.com", "b.example.com", "a.example.com"}, expected: Targets{"a.example.com", "b.example.com"}, }, + { + name: "all duplicates", + input: []string{"a.example.com", "a.example.com", "a.example.com"}, + expected: Targets{"a.example.com"}, + }, { name: "already sorted", input: Targets{"a.example.com", "c.example.com", "d.example.com"}, diff --git a/source/service.go b/source/service.go index 29447e382b..2e73defcf2 100644 --- a/source/service.go +++ b/source/service.go @@ -294,10 +294,10 @@ func (sc *serviceSource) Endpoints(_ context.Context) ([]*endpoint.Endpoint, err continue } existing[0].Targets = append(existing[0].Targets, ep.Targets...) - existing[0].UniqueOrderedTargets() + existing[0].Targets = endpoint.NewTargets(existing[0].Targets...) mergedEndpoints[key] = existing } else { - ep.UniqueOrderedTargets() + ep.Targets = endpoint.NewTargets(ep.Targets...) mergedEndpoints[key] = []*endpoint.Endpoint{ep} } }