Merge pull request #4293 from yurrriq/fix-aws-provider-nil-endpoint

fix(service): omit nil endpoints and prefer endpointsForHostname()
kubernetes-sigs · Mar 4, 2024 · d2890b0 · d2890b0
2 parents 6cf4783 + 3fac88b
commit d2890b0
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 93 deletions.
diff --git a/source/ingress_test.go b/source/ingress_test.go
@@ -264,6 +264,13 @@ func testEndpointsFromIngress(t *testing.T) {
 			expected:               []*endpoint.Endpoint{},
 			ignoreIngressRulesSpec: true,
 		},
+		{
+			title: "invalid hostname does not generate endpoints",
+			ingress: fakeIngress{
+				dnsnames:  []string{"this-is-an-exceedingly-long-label-that-external-dns-should-reject.example.org"},
+			},
+			expected: []*endpoint.Endpoint{},
+		},
 	} {
 		t.Run(ti.title, func(t *testing.T) {
 			realIngress := ti.ingress.Ingress()

diff --git a/source/service.go b/source/service.go
@@ -36,10 +36,6 @@ import (
 	"sigs.k8s.io/external-dns/endpoint"
 )
 
-const (
-	defaultTargetsCapacity = 10
-)
-
 // serviceSource is an implementation of Source for Kubernetes service objects.
 // It will find all services that are under our jurisdiction, i.e. annotated
 // desired hostname and matching or no controller annotation. For each of the
@@ -360,10 +356,15 @@ func (sc *serviceSource) extractHeadlessEndpoints(svc *v1.Service, hostname stri
 			targets = append(targets, target)
 		}
 
+		var ep *endpoint.Endpoint
 		if ttl.IsConfigured() {
-			endpoints = append(endpoints, endpoint.NewEndpointWithTTL(headlessKey.DNSName, headlessKey.RecordType, ttl, targets...))
+			ep = endpoint.NewEndpointWithTTL(headlessKey.DNSName, headlessKey.RecordType, ttl, targets...)
 		} else {
-			endpoints = append(endpoints, endpoint.NewEndpoint(headlessKey.DNSName, headlessKey.RecordType, targets...))
+			ep = endpoint.NewEndpoint(headlessKey.DNSName, headlessKey.RecordType, targets...)
+		}
+
+		if ep != nil {
+			endpoints = append(endpoints, ep)
 		}
 	}
 
@@ -458,38 +459,14 @@ func (sc *serviceSource) setResourceLabel(service *v1.Service, endpoints []*endp
 	}
 }
 
-func (sc *serviceSource) generateEndpoints(svc *v1.Service, hostname string, providerSpecific endpoint.ProviderSpecific, setIdentifier string, useClusterIP bool) []*endpoint.Endpoint {
+func (sc *serviceSource) generateEndpoints(svc *v1.Service, hostname string, providerSpecific endpoint.ProviderSpecific, setIdentifier string, useClusterIP bool) (endpoints []*endpoint.Endpoint) {
 	hostname = strings.TrimSuffix(hostname, ".")
-	ttl := getTTLFromAnnotations(svc.Annotations, fmt.Sprintf("service/%s/%s", svc.Namespace, svc.Name))
 
-	epA := &endpoint.Endpoint{
-		RecordTTL:  ttl,
-		RecordType: endpoint.RecordTypeA,
-		Labels:     endpoint.NewLabels(),
-		Targets:    make(endpoint.Targets, 0, defaultTargetsCapacity),
-		DNSName:    hostname,
-	}
-
-	epAAAA := &endpoint.Endpoint{
-		RecordTTL:  ttl,
-		RecordType: endpoint.RecordTypeAAAA,
-		Labels:     endpoint.NewLabels(),
-		Targets:    make(endpoint.Targets, 0, defaultTargetsCapacity),
-		DNSName:    hostname,
-	}
-
-	epCNAME := &endpoint.Endpoint{
-		RecordTTL:  ttl,
-		RecordType: endpoint.RecordTypeCNAME,
-		Labels:     endpoint.NewLabels(),
-		Targets:    make(endpoint.Targets, 0, defaultTargetsCapacity),
-		DNSName:    hostname,
-	}
+	resource := fmt.Sprintf("service/%s/%s", svc.Namespace, svc.Name)
 
-	var endpoints []*endpoint.Endpoint
-	var targets endpoint.Targets
+	ttl := getTTLFromAnnotations(svc.Annotations, resource)
 
-	targets = getTargetsFromTargetAnnotation(svc.Annotations)
+	targets := getTargetsFromTargetAnnotation(svc.Annotations)
 
 	if len(targets) == 0 {
 		switch svc.Spec.Type {
@@ -517,32 +494,15 @@ func (sc *serviceSource) generateEndpoints(svc *v1.Service, hostname string, pro
 		case v1.ServiceTypeExternalName:
 			targets = extractServiceExternalName(svc)
 		}
-	}
 
-	for _, t := range targets {
-		switch suitableType(t) {
-		case endpoint.RecordTypeA:
-			epA.Targets = append(epA.Targets, t)
-		case endpoint.RecordTypeAAAA:
-			epAAAA.Targets = append(epAAAA.Targets, t)
-		case endpoint.RecordTypeCNAME:
-			epCNAME.Targets = append(epCNAME.Targets, t)
+		for _, endpoint := range endpoints {
+			endpoint.ProviderSpecific = providerSpecific
+			endpoint.SetIdentifier = setIdentifier
 		}
 	}
 
-	if len(epA.Targets) > 0 {
-		endpoints = append(endpoints, epA)
-	}
-	if len(epAAAA.Targets) > 0 {
-		endpoints = append(endpoints, epAAAA)
-	}
-	if len(epCNAME.Targets) > 0 {
-		endpoints = append(endpoints, epCNAME)
-	}
-	for _, endpoint := range endpoints {
-		endpoint.ProviderSpecific = providerSpecific
-		endpoint.SetIdentifier = setIdentifier
-	}
+	endpoints = append(endpoints, endpointsForHostname(hostname, targets, ttl, providerSpecific, setIdentifier, resource)...)
+
 	return endpoints
 }
 
@@ -742,7 +702,9 @@ func (sc *serviceSource) extractNodePortEndpoints(svc *v1.Service, hostname stri
 				ep = endpoint.NewEndpoint(recordName, endpoint.RecordTypeSRV, target)
 			}
 
-			endpoints = append(endpoints, ep)
+			if ep != nil {
+				endpoints = append(endpoints, ep)
+			}
 		}
 	}
 

diff --git a/source/service_test.go b/source/service_test.go
@@ -1560,6 +1560,17 @@ func TestClusterIpServices(t *testing.T) {
 			expected:      []*endpoint.Endpoint{},
 			labelSelector: "app=web-external",
 		},
+		{
+			title:        "invalid hostname does not generate endpoints",
+			svcNamespace: "testing",
+			svcName:      "foo",
+			svcType:      v1.ServiceTypeClusterIP,
+			annotations: map[string]string{
+				hostnameAnnotationKey: "this-is-an-exceedingly-long-label-that-external-dns-should-reject.example.org.",
+			},
+			clusterIP: "1.2.3.4",
+			expected:  []*endpoint.Endpoint{},
+		},
 	} {
 		tc := tc
 		t.Run(tc.title, func(t *testing.T) {

diff --git a/source/source.go b/source/source.go
@@ -284,52 +284,41 @@ func endpointsForHostname(hostname string, targets endpoint.Targets, ttl endpoin
 	}
 
 	if len(aTargets) > 0 {
-		epA := &endpoint.Endpoint{
-			DNSName:          strings.TrimSuffix(hostname, "."),
-			Targets:          aTargets,
-			RecordTTL:        ttl,
-			RecordType:       endpoint.RecordTypeA,
-			Labels:           endpoint.NewLabels(),
-			ProviderSpecific: providerSpecific,
-			SetIdentifier:    setIdentifier,
-		}
-		if resource != "" {
-			epA.Labels[endpoint.ResourceLabelKey] = resource
+		epA := endpoint.NewEndpointWithTTL(hostname, endpoint.RecordTypeA, ttl, aTargets...)
+		if epA != nil {
+			epA.ProviderSpecific = providerSpecific
+			epA.SetIdentifier = setIdentifier
+			if resource != "" {
+				epA.Labels[endpoint.ResourceLabelKey] = resource
+			}
+			endpoints = append(endpoints, epA)
 		}
-		endpoints = append(endpoints, epA)
 	}
 
 	if len(aaaaTargets) > 0 {
-		epAAAA := &endpoint.Endpoint{
-			DNSName:          strings.TrimSuffix(hostname, "."),
-			Targets:          aaaaTargets,
-			RecordTTL:        ttl,
-			RecordType:       endpoint.RecordTypeAAAA,
-			Labels:           endpoint.NewLabels(),
-			ProviderSpecific: providerSpecific,
-			SetIdentifier:    setIdentifier,
-		}
-		if resource != "" {
-			epAAAA.Labels[endpoint.ResourceLabelKey] = resource
+		epAAAA := endpoint.NewEndpointWithTTL(hostname, endpoint.RecordTypeAAAA, ttl, aaaaTargets...)
+		if epAAAA != nil {
+			epAAAA.ProviderSpecific = providerSpecific
+			epAAAA.SetIdentifier = setIdentifier
+			if resource != "" {
+				epAAAA.Labels[endpoint.ResourceLabelKey] = resource
+			}
+			endpoints = append(endpoints, epAAAA)
 		}
-		endpoints = append(endpoints, epAAAA)
 	}
 
 	if len(cnameTargets) > 0 {
-		epCNAME := &endpoint.Endpoint{
-			DNSName:          strings.TrimSuffix(hostname, "."),
-			Targets:          cnameTargets,
-			RecordTTL:        ttl,
-			RecordType:       endpoint.RecordTypeCNAME,
-			Labels:           endpoint.NewLabels(),
-			ProviderSpecific: providerSpecific,
-			SetIdentifier:    setIdentifier,
-		}
-		if resource != "" {
-			epCNAME.Labels[endpoint.ResourceLabelKey] = resource
+		epCNAME := endpoint.NewEndpointWithTTL(hostname, endpoint.RecordTypeCNAME, ttl, cnameTargets...)
+		if epCNAME != nil {
+			epCNAME.ProviderSpecific = providerSpecific
+			epCNAME.SetIdentifier = setIdentifier
+			if resource != "" {
+				epCNAME.Labels[endpoint.ResourceLabelKey] = resource
+			}
+			endpoints = append(endpoints, epCNAME)
 		}
-		endpoints = append(endpoints, epCNAME)
 	}
+
 	return endpoints
 }