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

Add TXAWS-based Route53 backend. #121

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add TXAWS-based Route53 backend. #121

wants to merge 11 commits into from

Conversation

Lukasa
Copy link
Member

@Lukasa Lukasa commented Jul 4, 2017

Resolves #32.

Right now there's no testing for this, I'm pushing this up atm to get feedback on whether this looks about right to most people.

/cc @glyph


This change is Reviewable

@mithrandi
Copy link
Contributor

Review status: 0 of 3 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


a discussion (no related file):
I think this is generally on the right track; I added some commentary inline.


src/txacme/challenges/_route53.py, line 27 at r1 (raw file):

    d = Deferred()
    reactor.callLater(delay, d.callback, rval)
    return d

I think this can be written as return deferLater(reactor, delay, lambda: rval) which has the added bonus of supporting cancellation.


src/txacme/challenges/_route53.py, line 34 at r1 (raw file):

    """
    # TODO: This is just duplicated directly from _libcloud.py. Should we hoist
    # this out to a common utility module?

👍


src/txacme/challenges/_route53.py, line 92 at r1 (raw file):

        rr_set = rr_sets[key]
    except KeyError:
        return

I think this should be return succeed(None); I've found that functions that sometimes return a Deferred and sometimes do not lead to confusion (even though probably nothing is calling this function directly).


src/txacme/challenges/_route53.py, line 100 at r1 (raw file):

    if len(rr_set.records) == 1:
        rr_set_update = delete_rrset(rr_set)

This logic isn't quite right, because the single record left might not be the one we want to remove.


src/txacme/challenges/_route53.py, line 176 at r1 (raw file):

            client=self._client
        )
        d.addCallback(_sleep, reactor=self._reactor, delay=self.settle_delay)

Route 53 has an API for querying the status of a change, so we can poll that instead of arbitrarily waiting some fixed delay: http://docs.aws.amazon.com/Route53/latest/APIReference/API_GetChange.html

Oh, except txaws doesn't seem to bind that API yet. Argh. So I guess we should get that implemented upstream first. This branch probably shouldn't be blocked on that, then, so I guess this is okay for a first pass, but I think we should avoid exposing settle_delay in the public API in anticipation of removing it.


Comments from Reviewable

@Lukasa
Copy link
Member Author

Lukasa commented Jul 4, 2017

src/txacme/challenges/_route53.py, line 100 at r1 (raw file):

Previously, mithrandi (Tristan Seligmann) wrote…

This logic isn't quite right, because the single record left might not be the one we want to remove.

Yeah it is: check the set __contains__ block above.


Comments from Reviewable

@codecov
Copy link

codecov bot commented Jul 4, 2017

Codecov Report

Merging #121 into master will decrease coverage by 3.38%.
The diff coverage is 17.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage     100%   96.61%   -3.39%     
==========================================
  Files          27       29       +2     
  Lines        1987     2066      +79     
  Branches      174      181       +7     
==========================================
+ Hits         1987     1996       +9     
- Misses          0       70      +70
Impacted Files Coverage Δ
src/txacme/challenges/_dnsutil.py 100% <100%> (ø)
src/txacme/challenges/__init__.py 100% <100%> (ø) ⬆️
src/txacme/challenges/_libcloud.py 100% <100%> (ø) ⬆️
src/txacme/challenges/_route53.py 5.4% <5.4%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc015fd...d5aaeed. Read the comment docs.

@Lukasa
Copy link
Member Author

Lukasa commented Jul 4, 2017

src/txacme/challenges/_route53.py, line 176 at r1 (raw file):

Previously, mithrandi (Tristan Seligmann) wrote…

Route 53 has an API for querying the status of a change, so we can poll that instead of arbitrarily waiting some fixed delay: http://docs.aws.amazon.com/Route53/latest/APIReference/API_GetChange.html

Oh, except txaws doesn't seem to bind that API yet. Argh. So I guess we should get that implemented upstream first. This branch probably shouldn't be blocked on that, then, so I guess this is okay for a first pass, but I think we should avoid exposing settle_delay in the public API in anticipation of removing it.

Ok, I've removed settle_delay from the constructor and privatised the ivar.


Comments from Reviewable

@Lukasa
Copy link
Member Author

Lukasa commented Jul 4, 2017

Review status: 0 of 5 files reviewed at latest revision, 6 unresolved discussions.


a discussion (no related file):

Previously, mithrandi (Tristan Seligmann) wrote…

I think this is generally on the right track; I added some commentary inline.

Resolved.


src/txacme/challenges/_route53.py, line 27 at r1 (raw file):

Previously, mithrandi (Tristan Seligmann) wrote…

I think this can be written as return deferLater(reactor, delay, lambda: rval) which has the added bonus of supporting cancellation.

Yup, done.


src/txacme/challenges/_route53.py, line 34 at r1 (raw file):

Previously, mithrandi (Tristan Seligmann) wrote…

👍

Ok, done.


src/txacme/challenges/_route53.py, line 92 at r1 (raw file):

Previously, mithrandi (Tristan Seligmann) wrote…

I think this should be return succeed(None); I've found that functions that sometimes return a Deferred and sometimes do not lead to confusion (even though probably nothing is calling this function directly).

Done.


Comments from Reviewable

@mithrandi
Copy link
Contributor

Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


src/txacme/challenges/_route53.py, line 100 at r1 (raw file):

Previously, Lukasa (Cory Benfield) wrote…

Yeah it is: check the set __contains__ block above.

Oh, doh!


Comments from Reviewable

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