diff --git a/endpoint/endpoint.go b/endpoint/endpoint.go index dbeabcef88..67bdfcde42 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 { @@ -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 { diff --git a/endpoint/endpoint_test.go b/endpoint/endpoint_test.go index 31b26a159b..d899b348d0 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) - }) } } @@ -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") -} 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..7e4e162e7b 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) + + 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() 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} } } diff --git a/source/service_test.go b/source/service_test.go index 26e4320f6c..79d8e37e6b 100644 --- a/source/service_test.go +++ b/source/service_test.go @@ -3284,6 +3284,138 @@ func TestHeadlessServices(t *testing.T) { } } +func TestMultipleServicesPointingToSameLoadBalancer(t *testing.T) { + kubernetes := fake.NewClientset() + + services := []*v1.Service{ + { + ObjectMeta: metav1.ObjectMeta{ + 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, + 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", + }, + 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) + + 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(t.Context()) + require.NoError(t, err) + + 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) { kubernetes := fake.NewClientset() diff --git a/source/wrappers/dedupsource.go b/source/wrappers/dedupsource.go index 961ef45cfc..2d9fc6c367 100644 --- a/source/wrappers/dedupsource.go +++ b/source/wrappers/dedupsource.go @@ -39,7 +39,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,6 +52,10 @@ func (ms *dedupSource) Endpoints(ctx context.Context) ([]*endpoint.Endpoint, err continue } + if len(ep.Targets) > 1 { + ep.Targets = endpoint.NewTargets(ep.Targets...) + } + identifier := strings.Join([]string{ep.RecordType, ep.DNSName, ep.SetIdentifier, ep.Targets.String()}, "/") if _, ok := collected[identifier]; ok { diff --git a/source/wrappers/dedupsource_test.go b/source/wrappers/dedupsource_test.go index 3f9ac58bad..5af481398a 100644 --- a/source/wrappers/dedupsource_test.go +++ b/source/wrappers/dedupsource_test.go @@ -123,6 +123,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)