Skip to content

NEW PROVIDER: AkamaiEdgeDNS#1174

Merged
tlimoncelli merged 9 commits into
DNSControl:masterfrom
svernick-zz:master
Jun 22, 2021
Merged

NEW PROVIDER: AkamaiEdgeDNS#1174
tlimoncelli merged 9 commits into
DNSControl:masterfrom
svernick-zz:master

Conversation

@svernick-zz
Copy link
Copy Markdown
Contributor

This PR introduces a provider for Akamai EdgeDNS.

This PR also includes a change to downcase TLSA data. TLSA data is a hexadecimal string and is case insensitive. See pull request 1171.

Copy link
Copy Markdown
Contributor

@tlimoncelli tlimoncelli left a comment

Choose a reason for hiding this comment

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

It's awesome to see a new provider, especially coming from the company themselves!

Everything looks good so far. I have one big request about the name. See below.

Comment thread integrationTest/providers.json Outdated
"api_key": "$DNSMADEEASY_API_KEY",
"secret_key": "$DNSMADEEASY_SECRET_KEY"
},
"EDGEDNS": {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would you be open to a name that includes Akamai? Outsiders are more familiar with the name Akamai than EdgeDNS.

We learned the hard way that these names are difficult to change. As a result we have "GCLOUD" and "oracle" and "route53" which aren't optimal names. I'd like to make sure we put a lot of thought into any new names.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Now "akamaiedgedns".

Comment thread pkg/js/parse_tests/015-tlsa/foo.com.zone
Comment thread providers/edgedns/edgeDnsProvider.go Outdated
}

// AuditRecords returns an error if any records are not supportable by this provider.
func AuditRecords(records []*models.RecordConfig) error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please move the function AuditRecords to a file called auditrecords.go`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread providers/edgedns/edgeDnsService.go Outdated
)

// EDInitialize initializes the "Akamai OPEN EdgeGrid" library
func EDInitialize(clientSecret string, host string, accessToken string, clientToken string) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do the the ED* functions need to be exported? They might... I'm just not seeing why right off the bat. You might try changing one and see if go-lint or go-vet complain.

If they don't need to be exported, consider calling the functions initialize, zoneDoesExist, createZone, etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks for catching that!

Comment thread providers/edgedns/edgeDnsService.go Outdated
zone := &dnsv2.ZoneCreate{
Zone: zonename,
Type: "PRIMARY",
Comment: "This zone created by DNSControl (https://stackexchange.github.io/dnscontrol/)",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use the url http://dnscontrol.org instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@svernick-zz svernick-zz changed the title NEW PROVIDER: EdgeDNS NEW PROVIDER: AkamaiEdgeDNS Jun 11, 2021
@svernick-zz
Copy link
Copy Markdown
Contributor Author

I forgot to run 'go generate' before the previous commit. My apologies.

@svernick-zz
Copy link
Copy Markdown
Contributor Author

Hi. Is there anything that I need to do before this PR can be merged? Thanks!

Copy link
Copy Markdown
Contributor

@tlimoncelli tlimoncelli left a comment

Choose a reason for hiding this comment

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

Hi! Sorry for the delay. It's been quite a week!

This is nearly ready. I made a few inline comments. One additional thing:

  • docs/provider-list.md Add AKAMAIEDGEDNS along with your git name.

I think that's it!

layout: default
jsId: AKAMAIEDGEDNS
---
# Akamai Edge DNS Provider
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please leave a blank line before and after any # and before and after any code fence (three ` in a row)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done


## Usage
A new zone created by DNSControl:
```
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add a blank line before this and others like it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@svernick-zz
Copy link
Copy Markdown
Contributor Author

I think that I made all of the required corrections. If not, I'm happy to do so.
Thanks very much!

@tlimoncelli tlimoncelli merged commit be1f03f into DNSControl:master Jun 22, 2021
@tlimoncelli
Copy link
Copy Markdown
Contributor

Thank you for contributing this new provider! I'm excited to have Akamai join the DNSControl family!

@svernick-zz
Copy link
Copy Markdown
Contributor Author

svernick-zz commented Jun 22, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants