fix(endpoint): deduplicate targets

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 {
@@ -374,20 +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() {
-	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
-}
-
 // 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 {

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)
-
 		})
 	}
 }
@@ -927,58 +926,66 @@ func TestCheckEndpoint(t *testing.T) {
 	}
 }
 
-func TestEndpoint_UniqueOrderedTargets(t *testing.T) {
+func TestEndpoint_WithRefObject(t *testing.T) {
+	ep := &Endpoint{}
+	ref := &events.ObjectReference{
+		Kind:      "Service",
+		Namespace: "default",
+		Name:      "my-service",
+	}
+	result := ep.WithRefObject(ref)
+
+	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
-		targets  []string
+		input    Targets
 		expected Targets
-		want     bool
 	}{
 		{
 			name:     "no duplicates",
-			targets:  []string{"b.example.com", "a.example.com"},
+			input:    Targets{"a.example.com", "b.example.com"},
 			expected: Targets{"a.example.com", "b.example.com"},
 		},
 		{
 			name:     "with duplicates",
-			targets:  []string{"a.example.com", "b.example.com", "a.example.com"},
+			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",
-			targets:  []string{"a.example.com", "b.example.com"},
-			expected: Targets{"a.example.com", "b.example.com"},
+			input:    Targets{"a.example.com", "c.example.com", "d.example.com"},
+			expected: Targets{"a.example.com", "c.example.com", "d.example.com"},
 		},
 		{
-			name:     "all duplicates",
-			targets:  []string{"a.example.com", "a.example.com", "a.example.com"},
-			expected: Targets{"a.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",
-			targets:  []string{},
+			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) {
-			ep := &Endpoint{Targets: tt.targets}
-			ep.UniqueOrderedTargets()
-			assert.Equal(t, tt.expected, ep.Targets)
+			result := NewTargets(tt.input...)
+			assert.Equal(t, tt.expected, result)
 		})
 	}
 }
-
-func TestEndpoint_WithRefObject(t *testing.T) {
-	ep := &Endpoint{}
-	ref := &events.ObjectReference{
-		Kind:      "Service",
-		Namespace: "default",
-		Name:      "my-service",
-	}
-	result := ep.WithRefObject(ref)
-
-	assert.Equal(t, ref, ep.RefObject(), "refObject should be set")
-	assert.Equal(t, ep, result, "should return the same Endpoint pointer")
-}

source/endpoints.go

@@ -108,5 +108,5 @@ func EndpointTargetsFromServices(svcInformer coreinformers.ServiceInformer, name
 			}
 		}
 	}
-	return targets, nil
+	return endpoint.NewTargets(targets...), nil
 }

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",

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)
+
+	got, err := src.Endpoints(t.Context())
+	require.NoError(t, err)
+
+	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"),
+	})
+}
+
 // gateway specific helper functions
 func newTestGatewaySource(loadBalancerList []fakeIngressGatewayService, ingressList []fakeIngress) (*gatewaySource, error) {
 	fakeKubernetesClient := fake.NewClientset()

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}
 			}
 		}