feat(aws): add support for geoproximity routing

[prasadkatti]

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

1. I created a hosted zone called example.test
2. Applied the manifests below to my cluster
3. Ran a single control loop of external-dns locally
4. Verified on the AWS console that new records for the services were created
5. Deleted the services
6. Ran another loop of external-dns
7. 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]

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.

Details

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.

[ivankatliarchuk]

/ok-to-test

[ivankatliarchuk]

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]

aws-region not required, just region

[prasadkatti]

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

[ivankatliarchuk]

aws/geoproximitylocation-aws-region

does not looks right

[ivankatliarchuk]

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

[prasadkatti]

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.

[ivankatliarchuk]

This way it could be a breaking change, so unsure

[prasadkatti]

@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]

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

[prasadkatti]

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.

[ivankatliarchuk]

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

[ivankatliarchuk]

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

[ivankatliarchuk]

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]

/label tide/merge-method-squash

[prasadkatti]

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]

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

[ivankatliarchuk]

Found few areas that require code coverage as well

[ivankatliarchuk]

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

[ivankatliarchuk]

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]

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

[prasadkatti]

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

[mloiseleur]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mloiseleur

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [mloiseleur]

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