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

provider/aws/aws.go

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

I feel an Errorf here is okay, because the user is going to expect e-dns to create those records but it won't. That object with the invalid coordinates is just going to fail to reconcile.
We are also using an Errorf here even when the record will get created.

Warning please, we will correct this in follow up if needed

I'm not sure if I agree with the coding style, where methods with same prefix with* have different return types bool,none and have side effects with logging errors.

If there is an error, so we most likely do need to return an error, as we have concepts of soft erorrs as well, and we capture them in metrics

k8s-ci-robot · 2025-06-02T20:10:01Z

[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 ask for approval from ivankatliarchuk. 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

ivankatliarchuk · 2025-06-03T05:34:22Z

Any chance you share here a result of a smoke test? With geoproximity and without geoproximity settings, share relevant kube manifests as well pls. Does it require AWS IAM specific actions?

Docs needs updating as well https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials/aws.md

ivankatliarchuk · 2025-06-03T05:34:48Z

/label tide/merge-method-squash

prasadkatti · 2025-06-09T08:26:59Z

New changes -

updated the docs by adding the new annotations for geoproximity
added some more tests. The code coverage should be pretty good for the code that was updated.

Does it require AWS IAM specific actions?

No. It doesn't. It's just the same actions except the routing policy will be different.

Any chance you share here a result of a smoke test? With geoproximity and without geoproximity settings, share relevant kube manifests as well pls.

I have shared a manifest that I have been using for testing in the PR description. Let me know if that doesn't work for some reason. Here is a screenshot of the records that get added for those services in that manifest.

Let me know if you need anything else from me at this point. Thanks so much for all your help so far @ivankatliarchuk !

ivankatliarchuk · 2025-06-09T08:28:56Z

Nice one. Any chance you fix linter before we do a review?

docs/tutorials/aws.md

ivankatliarchuk

Found few areas that require code coverage as well

provider/aws/aws.go

ivankatliarchuk · 2025-06-09T09:22:10Z

provider/aws/aws.go

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


Warning please, we will correct this in follow up if needed

ivankatliarchuk · 2025-06-09T09:24:28Z

provider/aws/aws.go

+	return nil
+}
+
+func withGeoProximityCoordinates(ep *endpoint.Endpoint, geoproximity *route53types.GeoProximityLocation) bool {


This and similar methods contains too much of the side effects, return boolean, update endpoint and etc, etc.

Can we do it in more object oriented way, something like

type geoProximity { loc *route53types.GeoProximityLocation ep. *endpoint.Endpoint isSet bool } func (gp *geoproximity) withX() func (gp *geoproximity) withY() func (gp *geoproximity) withZ()

and in the method

func withGeoProximityCoordinates ... .... change.ResourceRecordSet.GeoProximityLocation = geoProximity.build()

And add tests per each method for the private type geoProximity

This should simply operational support for us

ivankatliarchuk · 2025-06-09T09:34:28Z

provider/aws/aws.go

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


I'm not sure if I agree with the coding style, where methods with same prefix with* have different return types bool,none and have side effects with logging errors.

If there is an error, so we most likely do need to return an error, as we have concepts of soft erorrs as well, and we capture them in metrics

This commit addresses the review feedback by making the following changes: - use a more object-oriented approach for geoProximity handling - change log levels to warnings instead of errors - add more test cases for geoProximity

prasadkatti · 2025-06-27T06:05:23Z

New changes since last review:

used a more object oriented approach for handling geoproximity. Created a new private type for it.
used early return to reduce nesting
removed usage of magic numbers
updated log levels to warnings as suggested
added more tests to ensure good code coverage

This is ready for another review. Thanks!

ivankatliarchuk

/lgtm

cc: @mloiseleur

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

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

ivankatliarchuk suggested changes May 4, 2025

View reviewed changes

k8s-ci-robot added the provider Issues or PRs related to a provider label Jun 2, 2025

prasadkatti force-pushed the geoproximity branch from c76adc7 to c134a01 Compare June 2, 2025 20:12

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jun 3, 2025

prasadkatti force-pushed the geoproximity branch from c134a01 to 87aeab6 Compare June 9, 2025 08:12

k8s-ci-robot added the docs label Jun 9, 2025

prasadkatti commented Jun 9, 2025

View reviewed changes

docs/tutorials/aws.md Show resolved Hide resolved

prasadkatti requested a review from ivankatliarchuk June 9, 2025 09:26

ivankatliarchuk suggested changes Jun 9, 2025

View reviewed changes

prasadkatti added 9 commits June 26, 2025 22:58

feat(aws): add support for geoproximity routing

266a566

remove the invalid test

633a9ca

make some changes based on review comments

3692a3b

fix linting errors

79a7318

make changes based on review feedback

a4d1329

add more tests to get better coverage

99cb5aa

update docs

674269c

make the linter happy

eb200f8

address review feedback

1519d8c

This commit addresses the review feedback by making the following changes: - use a more object-oriented approach for geoProximity handling - change log levels to warnings instead of errors - add more test cases for geoProximity

prasadkatti force-pushed the geoproximity branch from 126148b to 1519d8c Compare June 27, 2025 06:00

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 27, 2025

fix linting error

4a44a13

ivankatliarchuk reviewed Jun 27, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 27, 2025

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

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!

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!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jun 2, 2025

Uh oh!

ivankatliarchuk commented Jun 3, 2025

Uh oh!

ivankatliarchuk commented Jun 3, 2025

Uh oh!

prasadkatti commented Jun 9, 2025

Uh oh!

ivankatliarchuk commented Jun 9, 2025

Uh oh!

Uh oh!

ivankatliarchuk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

prasadkatti commented Jun 27, 2025

Uh oh!

ivankatliarchuk left a comment

Choose a reason for hiding this comment

prasadkatti commented May 1, 2025 •

edited

Loading