feat(aws): add support for geoproximity routing #5347

prasadkatti · 2025-05-01T06:17:01Z

Description

This PR adds support for Geoproximity routing in route53 to the AWS provider.

It adds four new annotations to enable this support.

Testing done

I created a hosted zone called example.test
Applied the manifests below to my cluster
Ran a single control loop of external-dns locally
Verified on the AWS console that new records for the services were created
Deleted the services
Ran another loop of external-dns
Verified the records were deleted from Route53.

Also did some more testing along these lines to ensure there are no unnecessary Upserts when the service isn't changed.

I used these service manifests for testing -

apiVersion: v1
kind: Service
metadata:
  name: nginx-test-region
  annotations:
    external-dns.alpha.kubernetes.io/hostname: nginx-test.example.test
    external-dns.alpha.kubernetes.io/target: 8.8.8.8
    external-dns.alpha.kubernetes.io/set-identifier: region
    external-dns.alpha.kubernetes.io/aws-geoproximitylocation-aws-region: us-west-2
    external-dns.alpha.kubernetes.io/aws-geoproximitylocation-bias: "10"
spec:
  ports:
  - port: 80
    targetPort: 80
  selector:
    app: nginx

---

apiVersion: v1
kind: Service
metadata:
  name: nginx-test-coordinates
  annotations:
    external-dns.alpha.kubernetes.io/hostname: nginx-test.example.test
    external-dns.alpha.kubernetes.io/target: 8.8.8.8
    external-dns.alpha.kubernetes.io/set-identifier: coordinates
    external-dns.alpha.kubernetes.io/aws-geoproximitylocation-coordinates: 90,90
spec:
  ports:
  - port: 80
    targetPort: 80
  selector:
    app: nginx

---

# I didn't test this one, but it should work

# apiVersion: v1
# kind: Service
# metadata:
#   name: nginx-test-local-zone
#   annotations:
#     external-dns.alpha.kubernetes.io/hostname: nginx-test.example.test
#     external-dns.alpha.kubernetes.io/target: 8.8.8.8
#     external-dns.alpha.kubernetes.io/set-identifier: localzone
#     external-dns.alpha.kubernetes.io/aws-geoproximitylocation-local-zone-group: "usw2-pdx1-az1"
# spec:
#   ports:
#   - port: 80
#     targetPort: 80
#   selector:
#     app: nginx

Fixes #4927

Checklist

Unit tests updated
End user documentation updated. Haven't yet updated the docs. Will get to it once my others questions get resolved.

k8s-ci-robot · 2025-05-01T06:17:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign mloiseleur for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-05-01T06:17:11Z

Hi @prasadkatti. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

provider/aws/aws.go

provider/aws/aws_test.go

ivankatliarchuk · 2025-05-01T08:07:20Z

/ok-to-test

ivankatliarchuk · 2025-05-01T08:08:50Z

For this to review

shared how it was tested with results
share kubernetes manifests so that we could validate that as well

ivankatliarchuk

pls share code coverage for new code as well

ivankatliarchuk · 2025-05-01T08:09:13Z

provider/aws/aws.go

+	providerSpecificGeolocationContinentCode           = "aws/geolocation-continent-code"
+	providerSpecificGeolocationCountryCode             = "aws/geolocation-country-code"
+	providerSpecificGeolocationSubdivisionCode         = "aws/geolocation-subdivision-code"
+	providerSpecificGeoProximityLocationAWSRegion      = "aws/geoproximitylocation-aws-region"


aws-region not required, just region

I have tried to match it with the field names in the GeoProximityLocation type

aws/geoproximitylocation-aws-region

does not looks right

provider/aws/aws.go

ivankatliarchuk · 2025-05-01T08:11:30Z

provider/aws/aws.go

@@ -926,6 +961,39 @@ func (p *AWSProvider) newChange(action route53types.ChangeAction, ep *endpoint.E
 		if useGeolocation {
 			change.ResourceRecordSet.GeoLocation = geolocation
 		}
+
+		geoproximity := &route53types.GeoProximityLocation{}


what if geoproximity not enabled? why we even have it here?

Honestly, I looked at how Geolocation works and tried to follow suit.

If there are no annotations corresponding to GeoProximity, then the GeoProximityLocation field of the ResourceRecordSet just stays nil. The field is populated iff any of the geoproximity annotations are seen.

Also, I wish there was a separate annotation for specifying routing policy. But since that isn't present, I have to check for the presence of all possible geoproximity annotations before I can definitively say that the record is not using geoproximity.

This way it could be a breaking change, so unsure

provider/aws/aws.go

prasadkatti · 2025-05-02T07:13:55Z

@ivankatliarchuk - First of all, thanks for the taking the time to review. I have pushed another commit to address some of your review comments. I have also updated the PR description to include the manifests used for testing this. Let me know if you need anything else.

I am still trying to figure out how to get code coverage for just the files I touched. I ran make cover-html and that shows me that I have added good coverage for the lines I added. I don't know how to share that with you.

ivankatliarchuk

would be nice to add docs with example setup and links to AWS docs.

https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials/aws.md

ivankatliarchuk · 2025-05-04T13:48:49Z

provider/aws/aws.go

@@ -939,6 +982,40 @@ func (p *AWSProvider) newChange(action route53types.ChangeAction, ep *endpoint.E
 	return change
 }

+func buildChangeForGeoProximityEndpoint(change *Route53Change, ep *endpoint.Endpoint) {


all lines need to be covered in tests

ivankatliarchuk · 2025-05-04T14:02:09Z

provider/aws/aws.go

@@ -560,6 +566,22 @@ func (p *AWSProvider) records(ctx context.Context, zones map[string]*profiledZon
 	return endpoints, nil
 }

+func handleGeoProximityLocationRecord(r *route53types.ResourceRecordSet, ep *endpoint.Endpoint) {


Here is suggested improvements

Use Early Returns: If the GeoProximityLocation is nil, return early to avoid unnecessary checks.

Slightly rewrite the logic, to simplify readability.

if r.GeoProximityLocation == nil { return } if region := aws.ToString(r.GeoProximityLocation.AWSRegion); region != "" { ep.WithProviderSpecific(providerSpecificGeoProximityLocationAWSRegion, region) } if bias := r.GeoProximityLocation.Bias; bias != nil { ep.WithProviderSpecific(providerSpecificGeoProximityLocationBias, fmt.Sprintf("%d", aws.ToInt32(bias))) } if coords := r.GeoProximityLocation.Coordinates; coords != nil { coordinates := fmt.Sprintf("%s,%s", aws.ToString(coords.Latitude), aws.ToString(coords.Longitude)) ep.WithProviderSpecific(providerSpecificGeoProximityLocationCoordinates, coordinates) } if localZoneGroup := aws.ToString(r.GeoProximityLocation.LocalZoneGroup); localZoneGroup != "" { ep.WithProviderSpecific(providerSpecificGeoProximityLocationLocalZoneGroup, localZoneGroup) }

ivankatliarchuk · 2025-05-04T14:08:05Z

provider/aws/aws.go

@@ -939,6 +982,40 @@ func (p *AWSProvider) newChange(action route53types.ChangeAction, ep *endpoint.E
 	return change
 }

+func buildChangeForGeoProximityEndpoint(change *Route53Change, ep *endpoint.Endpoint) {


Suggested change

func buildChangeForGeoProximityEndpoint(change *Route53Change, ep *endpoint.Endpoint) {

func witchChangeForGeoProximityEndpoint(change *Route53Change, ep *endpoint.Endpoint) {

ivankatliarchuk · 2025-05-04T14:10:26Z

provider/aws/aws.go

@@ -939,6 +982,40 @@ func (p *AWSProvider) newChange(action route53types.ChangeAction, ep *endpoint.E
 	return change
 }

+func buildChangeForGeoProximityEndpoint(change *Route53Change, ep *endpoint.Endpoint) {
+	geoproximity := &route53types.GeoProximityLocation{}


This method complexity is quite high, we need to split it most likely, example

func buildChangeForGeoProximityEndpoin(......) geoproximity := &route53types.GeoProximityLocation{} isGeoproximity := false isGeoproximity = withGeoProximityAWSRegion(ep, geoproximity) || useGeoproximity isGeoproximity = withGeoProximityBias(ep, geoproximity) || useGeoproximity isGeoproximity = withGeoProximityCoordinates(ep, geoproximity) || useGeoproximity isGeoproximity = withGeoProximityLocalZoneGroup(ep, geoproximity) || useGeoproximity if isGeoproximity { change.ResourceRecordSet.GeoProximityLocation = geoproximity } } where func withGeoProximityLocalZoneGroup(ep *endpoint.Endpoint, geoproximity *route53types.GeoProximityLocation) bool { if prop, ok := ep.GetProviderSpecificProperty(providerSpecificGeoProximityLocationLocalZoneGroup); ok { geoproximity.LocalZoneGroup = aws.String(prop) return true } return false }

ivankatliarchuk · 2025-05-04T14:18:12Z

provider/aws/aws.go

+			latitude := coordinates[0]
+			longitude := coordinates[1]
+			geoproximity.Coordinates = &route53types.Coordinates{
+				Latitude:  aws.String(latitude),
+				Longitude: aws.String(longitude),
+			}


Would be nice to add some validation here as well

type Coordinates struct { // Specifies a coordinate of the north–south position of a geographic point on // the surface of the Earth (-90 - 90). // // This member is required. Latitude *string // Specifies a coordinate of the east–west position of a geographic point on the // surface of the Earth (-180 - 180). // // This member is required. Longitude *string }

Probably something like

// validateCoordinates checks if the given latitude and longitude are valid. func (c *Coordinates) validate(lat, lon string) error { latitude, err := strconv.ParseFloat(lat, 64) if err != nil || latitude < -90 || latitude > 90 { return errors.New("invalid latitude: must be a number between -90 and 90") } longitude, err := strconv.ParseFloat(lon, 64) if err != nil || longitude < -180 || longitude > 180 { return errors.New("invalid longitude: must be a number between -180 and 180") } return nil } // Example usage func (c *Coordinates) validateAndSet(coordinates string) (*route53types.Coordinates, error) { parts := strings.Split(coordinates, ",") if len(parts) != 2 { return nil, errors.New("invalid coordinates format: expected 'latitude,longitude'") } lat := strings.TrimSpace(parts[0]) lon := strings.TrimSpace(parts[1]) if err := validateCoordinates(lat, lon); err != nil { return nil, err } return &route53types.Coordinates{ Latitude: aws.String(lat), Longitude: aws.String(lon), }, nil }

ivankatliarchuk · 2025-05-04T14:19:25Z

provider/aws/aws.go

+			}
+			useGeoproximity = true
+		} else {
+			log.Errorf("Invalid coordinates format for %s: %s; expected format 'latitude,longitude'", providerSpecificGeoProximityLocationCoordinates, prop)


This is debatable, but most likely should be log.Warn, to warn user so they need to change configuraiton

feat(aws): add support for geoproximity routing

377f2c8

k8s-ci-robot requested a review from ivankatliarchuk May 1, 2025 06:17

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 1, 2025

k8s-ci-robot requested a review from mloiseleur May 1, 2025 06:17

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 1, 2025

prasadkatti marked this pull request as draft May 1, 2025 06:17

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 1, 2025

prasadkatti commented May 1, 2025

View reviewed changes

provider/aws/aws.go Show resolved Hide resolved

prasadkatti commented May 1, 2025

View reviewed changes

provider/aws/aws.go Outdated Show resolved Hide resolved

prasadkatti commented May 1, 2025

View reviewed changes

provider/aws/aws_test.go Show resolved Hide resolved

remove the invalid test

1a5b4c9

prasadkatti marked this pull request as ready for review May 1, 2025 06:51

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 1, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 1, 2025

ivankatliarchuk suggested changes May 1, 2025

View reviewed changes

k8s-ci-robot assigned ivankatliarchuk May 1, 2025

make some changes based on review comments

901e340

fix linting errors

7b7dac7

ivankatliarchuk suggested changes May 4, 2025

View reviewed changes

	func buildChangeForGeoProximityEndpoint(change Route53Change, ep endpoint.Endpoint) {
	func witchChangeForGeoProximityEndpoint(change Route53Change, ep endpoint.Endpoint) {

feat(aws): add support for geoproximity routing #5347

Are you sure you want to change the base?

feat(aws): add support for geoproximity routing #5347

Uh oh!

Conversation

prasadkatti commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented May 1, 2025

Uh oh!

k8s-ci-robot commented May 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivankatliarchuk commented May 1, 2025

Uh oh!

ivankatliarchuk commented May 1, 2025

Uh oh!

ivankatliarchuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

prasadkatti commented May 2, 2025

Uh oh!

ivankatliarchuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivankatliarchuk May 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

prasadkatti commented May 1, 2025 •

edited

Loading

ivankatliarchuk May 4, 2025 •

edited

Loading