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

Adding support for AuroraDNS #38

Merged
merged 19 commits into from
Aug 1, 2017
Merged

Adding support for AuroraDNS #38

merged 19 commits into from
Aug 1, 2017

Conversation

wilfriedjonker
Copy link
Contributor

Thank you for contributing to sewer.
Every contribution to sewer is important to us.
You may not know it, but you have just contributed to making the world a more safer and secure place.

Contributor offers to license certain software (a “Contribution” or multiple
“Contributions”) to sewer, and sewer agrees to accept said Contributions,
under the terms of the MIT License.
Contributor understands and agrees that sewer shall have the irrevocable and perpetual right to make
and distribute copies of any Contribution, as well as to create and distribute collective works and
derivative works of any Contribution, under the MIT License.

Now,

What(What have you changed?)

Added support for AuroraDns.

Why(Why did you change it?)

Would like to see support for this dns provider in sewer.

@wilfriedjonker
Copy link
Contributor Author

wilfriedjonker commented Jul 29, 2017

Test with

import sewer

dns_class = sewer.AuroraDns(
    AURORA_API_KEY='api_key', AURORA_SECRET_KEY='secret_key')

client = sewer.Client(domain_name='example.com', dns_class=dns_class)
certificate = client.cert()
certificate_key = client.certificate_key
account_key = client.account_key

print "your certicate is:", certificate
print "your certificate's key is:", certificate_key
print "\n\n"
print "you can write them to a file then add that file to your favourite webserver."

with open('certificate.crt', 'w') as certificate_file:
    certificate_file.write(certificate)

with open('certificate.key', 'w') as certificate_key_file:
    certificate_key_file.write(certificate_key)

print "your account key is:", account_key
print "IMPORTANT: keep your account key in a very safe and secure place."

with open('account_key.key', 'w') as account_key_file:
    account_key_file.write(account_key)

@wilfriedjonker
Copy link
Contributor Author

I see that it works good with subdomains, like test.example.com. But not for domains without subdomain. I know what the problems causes. I will look into this and solve this problem.

Fix for "Unused variable 'record' record = zone.create_record(" in codacy.
@komuw
Copy link
Owner

komuw commented Jul 30, 2017

Hi @wilfriedjonker Thanks for doing this.

Would you mind doing a few things:

  1. add your name and an optional link to your website/contact etc to contributors.md[1]
  2. add support for AuroraDns to the sewer cli(command line interface)[2], look at the way the cloudflareDNS was added to the cli[3]
  3. add libcloud pypi package to sewer's setup.py requirements[4]
  4. add tldextract pypi package to sewer's setup.py requirements[4]

References:

  1. https://github.com/komuW/sewer/blob/master/CONTRIBUTORS.md
  2. https://github.com/komuW/sewer/blob/master/sewer/cli.py
  3. https://github.com/komuW/sewer/blob/master/sewer/cli.py#L111
  4. https://github.com/komuW/sewer/blob/master/setup.py#L76

domain_name=domain_name,
base64_of_acme_keyauthorization=base64_of_acme_keyauthorization)

extractedDomain = tldextract.extract(domain_name)
Copy link
Owner

Choose a reason for hiding this comment

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

@wilfriedjonker
Hi, I'm trying to limit the number of dependencies that sewer has.
Since it seems like you are using the pypi package(tldextract) for only extracting the domain name, would you mind extracting the domain name using only libraries that are found in the standard library(urlparse maybe)? Unless it is impossible to do so.

Copy link
Owner

@komuw komuw Jul 31, 2017

Choose a reason for hiding this comment

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

In other words, we should assume that whatever is passed into the method create_dns_record(self, domain_name, base64_of_acme_keyauthorization) as a domain_name will be a valid DNS name.
sewer should not concern itself, at least not at this point, with DNS name validation.
DNS name validation is kind of a hard topic[1][2], and I wouldn't want sewer to concern itself with it.

  1. https://stackoverflow.com/a/26987741/2768067
  2. https://stackoverflow.com/a/26618995/2768067

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komuw. The need for splitting up the domain is as follows.

When having test1.example.com

  • example.com is used to select the right domain
  • test1 is used to select or add the record

It is not possible to select, create or delete a record based on the whole domain name (test.example.com). This is the reason why it is needed tot split up the record.

Splitting up records by hand or regular expression is difficult, because you can have example.com and example.co.uk. So you can't select the part's (tld, domain name, sub domains) based on the . in de fqdn. tldextract uses the Public Suffix List (https://publicsuffix.org/learn/) to solve this problem.

There are some alternatives posible, wich I would no prefer:

  • Asking user to give sub and domain part. This way there is no processing needed.
  • Include a tldextract like function with sewer.

I have looked before at urlparse. Urlparse kan selects parts of a url, but not parts of a domain. So this is not suitable.

Copy link
Owner

Choose a reason for hiding this comment

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

@wilfriedjonker ok, Thanks for the explanation.
The only remaining issue is then; #38 (comment)

@wilfriedjonker
Copy link
Contributor Author

@komuw.

  1. add your name and an optional link to your website/contact etc to contributors.md - Done
  2. add support for AuroraDns to the sewer cli(command line interface), look at the way the cloudflareDNS was added to the cli - Done, haven't been able to test this.
  3. add libcloud pypi package to sewer's setup.py requirements - Done
  4. add tldextract pypi package to sewer's setup.py requirements - Done

sewer/cli.py Outdated
from . import AuroraDns
try:
AURORA_API_KEY = os.environ['AURORA_API_KEY']
AURORA_SECRET_KEY = os.environ['AURORA_API_KEY']
Copy link
Owner

Choose a reason for hiding this comment

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

@wilfriedjonker Hi,
should this line be:
AURORA_SECRET_KEY = os.environ['AURORA_SECRET_KEY'] ?
Or does auroraDNS use the same value for Api-key and Secret-key?

sewer/cli.py Outdated
@@ -125,6 +125,22 @@ def main():
except KeyError as e:
logger.info("ERROR:: Please supply {0} as an environment variable.".
format(str(e)))

if dns_provider == 'aurora':
Copy link
Owner

Choose a reason for hiding this comment

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

@wilfriedjonker This should be an elif clause.
ie:

if dns_provider == 'cloudflare':
... cloudflare code...
elif dns_provider == 'aurora':
... aurora code ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komuw. I changed the problems you mention in cli.py. Thank you!

@komuw
Copy link
Owner

komuw commented Jul 31, 2017

@wilfriedjonker Thanks for all the fixes.
I've highlighted a few places in cli.py that need small fixes. Ping me when you are done so that I can merge.

I can see that ci/circleci is failing, but do not worry about that; since it's not the tests that are failng. Rather it's the test coverage percentage that has gone down. But that will get fixed when we merge.

@komuw
Copy link
Owner

komuw commented Aug 1, 2017

closes: #33

@komuw komuw merged commit 7926c46 into komuw:master Aug 1, 2017
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