external-dns icon indicating copy to clipboard operation
external-dns copied to clipboard

Migrate to aws-sdk-go-v2

Open mjlshen opened this issue 1 year ago • 6 comments

Description This PR migrates the codebase to aws-sdk-go-v2 by migrating these three main components:

  • The DynamoDB registry functionality
  • The AWS-SD (Cloud Map) provider
  • The AWS (Route53) provider

Which then allowed me to:

  • Remove the aws-sdk-go session-based logic and replace it with the aws-sdk-go-v2 config-based https://aws.github.io/aws-sdk-go-v2/docs/migrating/#configuration-loading

My commits are all self-contained to align with those 4 steps, so I'd be happy to split this PR up if that would be preferred.

Fixes #4637

Checklist

  • [x] Unit tests updated
  • [ ] End user documentation updated

mjlshen avatar Jul 29 '24 05:07 mjlshen

Thanks for this @mjlshen. It's amazing. I've taken a look at the test, the code and you added error check when possible. Considering the major impact of this PR, it would be nice if some AWS user can test it for real. /lgtm

mloiseleur avatar Aug 09 '24 08:08 mloiseleur

Thanks @mloiseleur! I needed to rebase to resolve conflicts in go.mod - I'm happy to help test this on AWS. I'll plan on going through https://kubernetes-sigs.github.io/external-dns/v0.14.2/tutorials/aws-sd/ and https://kubernetes-sigs.github.io/external-dns/v0.14.2/tutorials/aws/ to make sure these still work and will report back with how it went. If there's anything else I can help with please feel free to let me know as well.

  • [x] Validate AWS Cloud Map API Docs
  • [x] Validate AWS (Route53) Docs
  • [x] Validate DynamoDB Registry

mjlshen avatar Aug 09 '24 21:08 mjlshen

@mjlshen I am not very confident with this change because it has too many changes in one. This one https://github.com/kubernetes-sigs/external-dns/pull/4640/commits/ea921690d22fa20329365dd3f645935104343954 is the commit that worries me, rest seems fine. The logic in the commit changes way too much for a refactoring that should focus on limiting the logic change if having even one.

szuecs avatar Aug 12 '24 15:08 szuecs

I'm happy to help test this on AWS. I'll plan on going through https://kubernetes-sigs.github.io/external-dns/v0.14.2/tutorials/aws-sd/ and https://kubernetes-sigs.github.io/external-dns/v0.14.2/tutorials/aws/ to make sure these still work and will report back with how it went. If there's anything else I can help with please feel free to let me know as well.

@szuecs / @mloiseleur I have (finally :D) validated my code in AWS by:

  • Running through the AWS Cloud Map Docs (Ensure ability to create/delete Route53 Records via Cloud Map with the TXT registry)
  • Running through the AWS (Route53) Docs (Ensure ability do create/delete Route53 Records via a public hosted zone with the TXT registry)
  • Setting up a DynamoDB registry and running through the AWS (Route53) Docs (Ensure ability to create/delete Route53 Records via a public hosted zone with the DynamoDB registry)

So I'm confident that it works! Is there anything else I can help test or demonstrate?

mjlshen avatar Aug 17 '24 02:08 mjlshen

/approve

szuecs avatar Sep 06 '24 13:09 szuecs

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: szuecs

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

The pull request process is described here

Needs approval from an approver in each of these files:

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 avatar Sep 06 '24 13:09 k8s-ci-robot

@mloiseleur when you have time, it would be great to have a re-review on this one. Thanks!

mjlshen avatar Sep 06 '24 18:09 mjlshen

/lgtm

mloiseleur avatar Sep 07 '24 07:09 mloiseleur