-
-
Notifications
You must be signed in to change notification settings - Fork 143
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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!)
@@ -20,13 +20,15 @@ var ( | |||
ErrHostNotSet = errors.New("host is not set") | |||
ErrHostOnlySubdomain = errors.New("host can only be a subdomain") | |||
ErrHostWildcard = errors.New(`host cannot be a "*"`) | |||
ErrHostedZoneIDNotSet = errors.New("hosted zone id is not set") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use ErrZoneIdentifierNotSet
instead? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated as requested. Please, take a look
ErrIPv4KeyNotSet = errors.New("IPv4 key is not set") | ||
ErrIPv6KeyNotSet = errors.New("IPv6 key is not set") | ||
ErrKeyNotSet = errors.New("key is not set") | ||
ErrKeyNotValid = errors.New("key is not valid") | ||
ErrNameNotSet = errors.New("name is not set") | ||
ErrPasswordNotSet = errors.New("password is not set") | ||
ErrPasswordNotValid = errors.New("password is not valid") | ||
ErrSecretAccessKeyNotSet = errors.New("secret access key is not set") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would ErrSecretNotSet
or ErrKeyNotSet
fit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use existing errors, please take another look.
if err != nil { | ||
return nil, fmt.Errorf("validating provider specific settings: %w", err) | ||
} | ||
ttl := int64(300) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super-nit tll for DNS are int32 as far as I know 😉
region := "us-east-1" | ||
if providerSpecificSettings.Region != "" { | ||
region = providerSpecificSettings.Region | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we enforce the region to be set perhaps? What's the reason to default to us-east-1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No specific reason for that. I just got watherver region UI was showing at the moment for the 'Global Access', I will make it mandatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found out that signing requests for Global Resources must be with Region us-east-1
. As the goal is to UPSERT
, the ddns-updater should make requests only to us-east-1
. I will remove the Region from options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the region to be us-east-1
due to the limitations. Please, take another look
docs/aws.md
Outdated
- `"host"` is your host and can be a subdomain or `"@"` or the wildcard `"*"` | ||
- `"aws_access_key_id"` | ||
- `"aws_secret_access_key"` | ||
- `"hosted_zone_id"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be replaced with zone_id
perhaps? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried using AWS specific names, I can replace this by zone_id
and update the doc to reference the HostedZone ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to zone_id
. Please, take another look.
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
5d52cf2
to
bf99329
Compare
* Implements the request signing version 4 * Print formatted error messages * Remove optional region to global resource * Rename config settings
3c0cf50
to
03ee321
Compare
* revert hardcoded ip for testing
03ee321
to
c388405
Compare
Changes
This includes the support to Simple Route policy from Route53.
Details:
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
Note: this is a partial implementation of #375 because AWS route53 supports many more DNS features.