-
Notifications
You must be signed in to change notification settings - Fork 2.3k
slick53 methods grafted onto to boto route53 connection #478
Conversation
I'd prefer if we put slick53 into a boto.contrib.slick53 module to keep the connection modules as close to the API as possible. This would follow what the Django project does - some of the most useful Django modules such as the Admin interface is included not in core but in contrib. |
Fine by me. I wasn't aware boto had a contrib directory. |
Might want to make sure this is what @garnaat had in mind (putting this in contrib). The route53 API we currently expose is very, very quirky and has some usability issues. I tinkered with @rubic's original slick53, and was pretty impressed with the improvement seen over the standard boto.route53 module. Also, we pretty commonly inter-mix Amazon API methods and convenience methods in our other sub-modules. This is effectively what a lot of slick53 is/could be. Convenience methods that fill in the gaps that the standard API leaves as an exercise for the user. @garnaat asked for @rubic to bring this over, maybe he could pipe in? |
My original post was similar to gtaylor's thoughts, but I didn't want to intrude. Generally I agree most enhancements should be handled in a contrib directory. However in the case of route53, the boto code -- while following the semantics of AWS -- is almost unusable, IMO. I'm still happy to go either direction. Also, Brad Carleton is the author of slick53. I'm just interested in getting it into boto. |
I agree that this should be a core part of boto rather than in contrib. I have been experimenting with a layered approach for another module and I think that works pretty well. I can take a crack at doing something similar here. Mitch On Jan 17, 2012, at 10:29 AM, Jeff Bauerreply@reply.github.com wrote:
|
Hi all, I just wanted to say I am excited that this is getting moved into boto! And thank you Jeff for merging the code. |
I agree with using a layered approach. My concern with the commit in this pull request is it doesn't appear to handle Amazon's aliasing or WRRs. While the convenience methods are probably better for the majority of the users of boto, in their current form I'm guessing (haven't tested) that they will break in unexpected ways when they encounter an alias RR and know they will break when they encounter a WRR. I've been trying to convince an unnamed author for two months to submit a pull request for his WRR patch. Sometimes I feel that I may be the only person using boto's route53 code with ELB. :( I'm happy to put code where my mouth is to ensure the final form properly handles alias RRs and WRRs. |
@jimbrowne: Would your patches for RR, WRR affect the proposed method signatures for the proposed code? In my mind, that would be the main issue. Not having looked into the RR/WRR situation I may be talking out of my ass, but could we just make the functions fail if those situations are encountered (until patched), which would admittably affect a minority of users? |
I think the fundamental problem is most people see DNS RRs as being a tuple of (type, value, ttl). In Route53, they are actually (type, value, ttl, wrr set). The implicit goal of the slick53 API, as I see it, is to simplify the interface. One could reasonably argue that most will never use Route 53 ALIAS records nor WRR so the simplified API should ignore them. However, there are tools out there that generate WRRs and ALIAS records, the most likely to be encountered are the elb-(dis)associate CLI commands. For some reason they create the ALIASes as WRRs. That's how I stumbled upon this problem in the first place; I was moving a system formerly controlled by the ELB CLI to boto-based code that manipulated route53 and elb. Another possible source of records with WRRs is the AWS Console as it now supports direct manipulation of Route53 RRs. If one ignores WRRs it breaks being liberal in what you accept and this problem will trip up anyone who encounters pre-existing WRRs. The functions will fail as the changes will fail as Route53 will not be able to match the (type, value, ttl) tuple to the existing (type, value, ttl, wrr set) tuple. At the minimum I want to see WRR support incorporated into the lower level. Perhaps the right approach is to just put a prominent warning in the documentation on the slick53 derived methods noting that they do not support WRRs and encountering WRRs will cause errors. Those who need to manipulate records with WRRs can use the lower level API. I just wanted to raise this aspect for discussion. Disclaimer: I was last knee deep in Route53 about two months ago, so my description might be slightly inaccurate. |
I think we can safely say that the lower level API should support WRRs, as the lower level API should closely mirror the AWS API. Work on that could start now without question, this is the way we've been handling a few of the more recently developed sub-modules. After that, perhaps we can see how the chips fall with the higher level API? |
@jimbrowne Taking into account what you say "most people see DNS RR's as being a tuple of (type, value, ttl)", I don't think that the WRR's really break that concept. It just changes the meaning of value. In this case value could be a regular python dictionary specifying the weights, that way the same function could support both. And this would be super easy for the end user. I also don't see how (type, value, ttl) does not apply for the AWS Alias records as well, although I am not really familiar with them. In short, I think (type, value, ttl) is the right abstraction for DNS records. |
I second gtaylor's conclusion and bluepines' suggestion regarding dictionary values. |
I had some time finally to look at this today. The issue is the tuple is really (name, type, wrr_id, wrr_weight) with many subtleties. The value doesn't matter as you can not have more than one (name, type) without using WRRs. Also, the ALIAS type doesn't require a TTL as it inherits the TTL from the backing ELB. Confused yet? :) Here's a tricky example:
Would the idea be that the value dictionary is constructed like: {'1.2.3.4': 20, '5.6.7.8': 40} and boto would build the two WRRs needed if the weights differ or would build a single RR with a multiple values when the weights are equal? SetIdentifiers would have to be generated, but I believe they are scoped to (name, type) so that shouldn't be a problem (e.g. use "id1", "id2", ...). If that is what you mean I like the idea as it hides WRRs completely while enabling their use and has boto deal with all of the ugliness behind the scenes. I'm happy to code this up if it sounds correct, as well as adding the ALIAS case. (I just submitted a pull request for WRR support overall: #529) |
+1 and thanks for your effort Jim. You seem to be the boto user most current with the WRR issues and your pull request is backward compatible with the current api. |
An oldie but a goodie. It seems a shame to waste all of this good discussion so let's revive this and try to get it merged. I know @rubic is still interested. How about @jimbrowne ? |
I'll take a look at it tonight. |
While re-familiarizing myself with this found and opened #1011. First thoughts tonight after a quick reading are to split the zone changes from the record changes and accept the former immediately while reviewing the latter; though I should be able to finish up something for all of this request this week. |
Questions (mainly for @garnaat and @bluepines): First: The API is inconsistent w.r.t. MX records; the MX related calls only manipulate records attached to the zone top. IOW one can't create MX records for anything but the zone. I think the MX calls should accept a name parameter. Second: Since the Zone object already contains the DNS zone name, would it be better if the methods operating on zones automatically post-pend the zone name to the name parameter as a convenience rather than just ensuring the name parameter is fully qualified (i.e. ends in ".")? I note that the repr for records returned by get_records will be fully qualified (i.e. include the zone name). For example this:
would instead be this:
but that raises the issue of how to handle the zone top. Pass '' as the name parameter? I just find including the zone name on every name attribute to be repeating oneself and would probably result in code like:
w.r.t. WRR and LBR I'll try out some changes tonight. |
Incorporate more verbose __repr__ functions from Slick53, but use to_print on Record to properly represent the more complicated records (e.g. ALIAS, WRR, LBR)
I've merged the proposed patch against the current develop branch and split out the import fix-ups and repr changes that were formally monkey-patched. Still looking at the rest of the code and its interaction with WRR and LBR. |
I've re-factored the original code quite a bit and have added support for WRR and LBR; I'm still running through some tests before pushing. I think I might have found a bug in the Route53 API as well. Should have something to push tomorrow night. |
Awesome news Jim! Thanks for making the effort. |
Code is finished, additional test cases have been written, and all tests pass. I just want to add some documentation and will push. Oh, and I didn't find a bug in the Route53 API; I just had forgotten that passing a name/type to get_all_records just sets a starting point... it's not a true filter. Not sure why they built it that way, but the service works per the documentation. |
Starting with a base from slick53: add support for LBR and WRR records; add documentation; e-factor to pass record objects opaquely and reduce code duplication; rename tests file to test_layer2.py and add testing of LBR and WRR records.
The above commit completes the work I wanted to propose for a new Layer 2. Would appreciate review by those on this issue to guide the hand of @garnaat |
Jim: It looks good to me. @garnaat: Can this be put on track for 2.4? |
Sorry for dragging this out even longer. The code looks good. Could use more docstrings. In looking through this, it seems like the best thing to do would be to leave the current connection.py unchanged and to add a layer2.py file that contains all of the slick53 code that was added to connection.py. We could then leave the default behavior (i.e. connect_route53()) to still use the original but then people could still create a Layer2 connection. Does that make sense? |
@garnaat: Mitch, the resulting merge will not require any additional imports, correct? How will the connection differ from connect_route53() if the user wants to use the slick53 methods? |
@garnaat did you look at the commits I suggested, or just the original pull request? I ask because I added docstrings to every function in my version. Would it make more sense for me to just open a separate pull request for jimbrowne/boto/slick53? For my commit I could move the Zone and Status objects to separate files if you'd like. |
The code looks great. Thanks. My only question was on organization of the code. Some modules like dynamodb have a layer1.py and layer2.py where layer1 is low-level, close to the protocol and layer2 provides a higher-level, more pythonic interface. We could package this in a similar way, but it's not a requirement from my point of view. If you feel this organization makes the most sense, I'm fine with it. |
I was just looking for a pythonic wrapper around boto route53 and this looks like exactly it. Would love to see this merged (or those changes backported into slick53 as a standalone module). |
Per comments split Zone and Status objects into separate files; rename test to reduce confusion as this isn't a true "Layer 2" interface in the boto sense.
@garnaat I changed the code layout, renamed the test to not mention layer2, and fixed an issue noted by @gingerlime. I think my branch is finally ready for merging. |
As mentioned on the boto email list, I'd like to see slick53's more intuitive methods available in the boto package. Aside from minor changes to make the code more idiomatic, I've basically left the code alone, though I may revisit it pending review of this request.
There are some basic unit tests.
If accepted, I'll submit some changes to the docs.