Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(aws): add support to route53 simple records #715

Merged
merged 17 commits into from
Jun 15, 2024
Merged

feat(aws): add support to route53 simple records #715

merged 17 commits into from
Jun 15, 2024

Conversation

marcelohpf
Copy link
Contributor

@marcelohpf marcelohpf commented May 9, 2024

Changes

This includes the support to Simple Route policy from Route53.

Details:

  • Set default TTL to 300s as it is same default interval as ddns-updater
  • It implements only Simple Routing, there are other 7 policies available for route53, but it is not a use case for me. If there are larger interest on them, I can send more PRs.
  • Providing the basic policy from documentation is enough for upserting domains following least privilege accesses
  • Uses only Static Credentials following this project design (/data/config.json for all provider configs).
  • It doesn't wait for propagation (GetChange API) because it updates one domain at a time and runs quite often.

Testing

Build container and ran with the default configs and using policy in the documentation

Here is one event for a domain that didn't exit at route53

{
    "eventVersion": "1.09",
    "userIdentity": {...},
    "eventTime": "2024-05-09T18:10:47Z",
    "eventSource": "route53.amazonaws.com",
    "eventName": "ChangeResourceRecordSets",
    "awsRegion": "us-east-1",
    "sourceIPAddress": "<redacted>",
    "userAgent": "DDNS-Updater quentin.mcgaw@gmail.com",
    "requestParameters": {
        "hostedZoneId": "<redacted>",
        "changeBatch": {
            "comment": "Record updated by ddns-updater",
            "changes": [
                {
                    "action": "UPSERT",
                    "resourceRecordSet": {
                        "name": "test.<my-domain>",
                        "type": "A",
                        "tTL": 300,
                        "resourceRecords": [
                            {
                                "value": "<redacted>"
                            }
                        ]
                    }
                }
            ]
        }
    },
    "responseElements": {
        "changeInfo": {
            "id": "/change/<redacted>",
            "status": "PENDING",
            "submittedAt": "May 9, 2024 6:10:47 PM",
            "comment": "Record updated by ddns-updater"
        }
    },
   ...
}

Note: this is a partial implementation of #375 because AWS route53 supports many more DNS features.

Copy link
Owner

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! It looks quite good, I just wonder if it could be achieved without using their Go SDK to rely on less dependencies (call me crazy, that's fair enough as well!)

internal/provider/errors/validation.go Outdated Show resolved Hide resolved
internal/provider/errors/validation.go Outdated Show resolved Hide resolved
internal/provider/providers/aws/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/aws/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/aws/provider.go Outdated Show resolved Hide resolved
docs/aws.md Outdated Show resolved Hide resolved
docs/aws.md Outdated Show resolved Hide resolved
docs/aws.md Outdated Show resolved Hide resolved
docs/aws.md Outdated Show resolved Hide resolved
docs/aws.md Outdated Show resolved Hide resolved
@qdm12
Copy link
Owner

qdm12 commented Jun 13, 2024

First of all, my apologies for the long delay!

Second, it was very enjoyable to review your quality Go code, congrats on that! A lot of attention to details and good work on porting code from the AWS sdk to this repo 💯

  • I rebased your branch on the master branch
  • I reworked your code to be a bit smaller/simpler. For example the headers sanitization got replaced by just hardcoded constants, which fit fine in this use case AFAIK.
  • I added tests for the signer, although I am not 100% certain of the test expected values since I cannot really test it... And so, can you please test the new code and see if it still works fine (force an update by setting your record to 127.0.0.1 on the AWS UI first for example)?? Thanks!! - you can get the tip of the rebased branch by doing git fetch && git reset --hard origin/feat/aws-simple-record 😉
  • In the end, I decided to keep it as it is regarding the aws_ prefix part. The reason is the environment variables do have the AWS_ prefix to separate them from other programs, but in this case, it's already separated, so it doesn't make much sense. Anyway, that's just left as you left it last (without the prefix) 😉

@marcelohpf
Copy link
Contributor Author

marcelohpf commented Jun 15, 2024

  • which fit fine in this use case AFAIK

Yes, by their documentation, it wouldn't be a problem.

And so, can you please test the new code

DNS was updated correctly. Everything is looking fine.

Some sample from debug logs [redacted some info]

2024-06-15T06:46:10Z DEBUG 200 OK | headers: X-Amzn-Requestid: ffffffff-ffff-ffff-ffff-ffffffffffff; Content-Type: text/xml; Content-Length: 283; Date: Sat, 15 Jun 2024 06:46:10 GMT | body: <?xml version="1.0"?><ChangeResourceRecordSetsResponse xmlns="https://route53.amazonaws.com/doc/2013-04-01/"><ChangeInfo><Id>/change/C0MYCHANGERECORDID34C</Id><Status>PENDING</Status><SubmittedAt>2024-06-15T06:46:10.886Z</SubmittedAt></ChangeInfo></ChangeResourceRecordSetsResponse>
2024-06-15T06:46:10Z DEBUG POST https://route53.amazonaws.com/2013-04-01/hostedzone/ZMYRECORDZONEIDAWSD35/rrset | headers: User-Agent: DDNS-Updater quentin.mcgaw@gmail.com; Content-Type: application/xml; Accept: application/xml; Date: 20240615T064610Z; Host: route53.amazonaws.com; Authorization: AWS4-HMAC-SHA256 Credential=SECRETKEYIDHERE00000/20240615/us-east-1/route53/aws4_request,SignedHeaders=content-type;host,Signature=0ff0123456312aaaaaaaaaaaffffffffffff000000000ccccccccccaaaaaaaff | body: <ChangeResourceRecordSetsRequest xmlns="https://route53.amazonaws.com/doc/2013-04-01/"><ChangeBatch><Changes><Change><Action>UPSERT</Action><ResourceRecordSet><Name>whoami.domain.com</Name><Type>A</Type><TTL>300</TTL><ResourceRecords><ResourceRecord><Value>10.0.0.0</Value></ResourceRecord></ResourceRecords></ResourceRecordSet></Change></Changes></ChangeBatch></ChangeResourceRecordSetsRequest>
2024-06-15T06:46:10Z INFO Updating record [domain: domain.com | host: whoami | provider: route53 | ip: ipv4] to use 10.0.0.0

@qdm12
Copy link
Owner

qdm12 commented Jun 15, 2024

Perfect, thanks for the feedback, merging it 🚀 !

@qdm12 qdm12 merged commit d3b689d into qdm12:master Jun 15, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants