Route53 HostedZone metrics

[julienduchesne]

Summary

This adds support to HostedZone limits. The ones in this request: https://docs.aws.amazon.com/Route53/latest/APIReference/API_GetHostedZoneLimit.html

Pull Request Checklist

• All pull requests should be against the develop branch, not master.
• All pull requests must include the Contributor License Agreement (see below).
• Whether or not your PR includes unit tests:
  • Please make sure you have run the exact code contained in the PR locally, and it functions as desired.
  • Please make sure the TravisCI build passes or, if not, you've corrected any obvious problems identified by the tests.
• Code should conform to the Development Guidelines:
  • pep8 compliant with some exceptions (see pytest.ini)
  • 100% test coverage with pytest (with valid tests). If you have difficulty
    writing tests for the code, that's fine, just mention that in the summary and either
    ask for assistance, or clarify that you'd like someone else to handle the tests. PRs that
    include complete test coverage will usually be merged faster.
  • Complete, correctly-formatted documentation for all classes, functions and methods.
  • documentation has been rebuilt with tox -e docs
  • Connections to the AWS services should only be made by the class's
    connect() and connect_resource() methods, inherited from
    awslimitchecker.connectable.Connectable
  • All modules should have (and use) module-level loggers.
  • Commit messages should be meaningful, and reference the Issue number
    if you're working on a GitHub issue (i.e. "issue #x - "). Please
    refrain from using the "fixes #x" notation unless you are sure that the
    the issue is fixed in that commit.
  • Git history is fully intact; please do not squash or rewrite history.

Contributor License Agreement

By submitting this work for inclusion in awslimitchecker, I agree to the following terms:

• The contribution included in this request (and any subsequent revisions or versions of it)
  is being made under the same license as the awslimitchecker project (the Affero GPL v3,
  or any subsequent version of that license if adopted by awslimitchecker).
• My contribution may perpetually be included in and distributed with awslimitchecker; submitting
  this pull request grants a perpetual, global, unlimited license for it to be used and distributed
  under the terms of awslimitchecker's license.
• I have the legal power and rights to agree to these terms.

[jantman]

@julienduchesne This looks like a great starting point, thanks so much! I'm incredibly busy at work this week and going to be away this weekend, so I most likely won't get a chance to look this over in detail until the weekend or early next week.

I did notice that the TravisCI tests are failing for the Python 2.6 environment only. That can be ignored; per #317 I announced EOL of py26 and py33 support in 3.0.0 in December, so I think I'll drop tests and official support for those in the next release.

I did notice one other thing though... the code needs to be broken up a bit differently. Right now get_limits() is returning an empty dict, which is a problem. The code that gets limits needs to be separated out from the code that finds usage, so it's possible to get the current limits without calculating the actual usage.

Again, thanks so so much for the contribution!

[julienduchesne]

The reason get_limits returns nothing is that, for the HostedZone metrics, the limit is actually for each HostedZone individually (like EC2 instance types). However, we cannot get the limits without fetching all the zones since they do not have fixed ids like the EC2 instance types.

Also, the limit is returned in the same API call as the current value. I thought it a waste, for these metrics, to fetch all the limits from the API and then do another call to fetch the value when both of these are fetched in the same call.

Therefore, I see get_limits as more of a placeholder for further metrics such as the Maximum number of hostedzones.

Does this make sense? I did not find any other limits in the app where the limit is actually fetched at the same time as the value so I did not have any examples from which to derive my code.

[jantman]

@julienduchesne I still haven't been able to review this in depth, but I wanted to respond to your comment:

Ok, that all makes sense, but then we need to switch where we do the actual API calls from find_usage to get_limits.

The problem here is that there is both an API call and a command-line option to print the current limits without calculating usage. When this is done, find_usage() will never be called. However, every code path that calls find_usage() will also implicitly call get_limits().

Overall, I'm not entirely sure about the current implementation of having an AwsLimit for each hosted zone - I'm not sure that really fits with the API and usual use case of awslimitchecker. I'll pull your branch down locally and play around with it a bit within the next week, but I'm thinking that I may opt to make some changes to the internals of AwsLimit to allow it to work better with limits where each AwsLimitUsage has its own threshold, if that makes sense...

[codecov-io]

Codecov Report

Merging #345 into develop will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           develop   #345   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files           27     28    +1     
  Lines         2057   2107   +50     
  Branches       309    318    +9     
======================================
+ Hits          2057   2107   +50

Impacted Files	Coverage Δ
awslimitchecker/limit.py	100% <100%> (ø)	⬆️
awslimitchecker/services/route53.py	100% <100%> (ø)
awslimitchecker/runner.py	100% <100%> (ø)	⬆️

---

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 56ba316...c8dd075. Read the comment docs.

[julienduchesne]

I tried switching the API calls from find_usage to get_limits. However, since services call get_limits() on init, it makes a bunch of tests fail in the test_checker.py suite since the AWSLimitChecker inits all services which in turn will cause the route53 service to try to make api on a mock and fail.

So I put the code in _update_limits_from_api instead. Let me know what you think.

[jantman]

@julienduchesne Apologies for letting this sit so long. My personal life has been really busy lately, and I haven't had much time to dedicate to working on projects.

You're right, _update_limits_from_api is probably the more logical place. I'm sorry I didn't think of that myself.

I think I'm still a bit torn on the implementation of this. Right now, the actual set of limits that awslimitchecker returns in its output is a fixed, known set. With the current implementation, the number of limits we return will be increased by the number of hosted zones in an account. This also makes it significantly more difficult to suppress some checks (i.e. all hosted zone checks) or to programmatically work with check results across multiple accounts. This is a problem that awslimitchecker hasn't had to deal with before, because this is the first limit we monitor that doesn't have a fixed set of limits (i.e. even for the instance type limits, we know in the source code what instance types there are and what the default limits for them are).

I'm sort of thinking of perhaps implementing a subclass of AwsLimit (or AwsLimitUsage) for this, which would allow having one hosted zone VPCs limit and one rrsets limit, and then storing usage much like we do for things like subnets per vpc. i.e. I'm thinking of something that would store usage/threshold pairs for each hosted zone. But that would require some changes to the public API of how limits and usage work.

Alternatively we could just subclass AwsLimit without any actual changes, i.e. just an empty wrapper class, called something like HostedZoneLimit... and then tell people who are using the Python API that if they want to handle these limits specially, they can do so based on that class.

[jantman]

@nadlerjessie @spulec @hltbra As far as I know, yipit/awslimits is the only public non-trivial consumer of the awslimitchecker Python API. This PR adds support for the Route53 Hosted Zone limits, which differ from all other limits we handle to date because the actual limit/threshold is specific to each Hosted Zone. The current implementation here means that if an account had 30 hosted zones, we'd be adding potentially 60 limits: "foo.com Record Sets" for each zone and and "foo.com VPC Associations" for each Private zone.

Any thoughts from your perspective on:

1. Just add the limits.
2. Add them but with a specific subclass of AwsLimit, so they can be filtered/handled differently by the API consumer if desired.
3. Rework the internals of AwsLimit to handle limits like these (where the actual numeric limit can differ between each use of it), which would likely end up in some public API changes (or at least changes in output formatting)

[spulec]

For our use case, I think #2 would be sufficient.

#3 sounds like it might be nice at some point if there are going to be more of these (I think there are).

[julienduchesne]

Hey @jantman. Have you started working on something? If not, I can take a stab at it this week or next one when I get the time. Let me know. Thanks.

[jantman]

I have not, sorry. I really feel bad for letting something sit this long, but work has been really busy lately (this project is personal-time-only for me) and I'm also moving at the end of the month, so most of my personal time has been taken up with organizing that...

If you have time, that would be wonderful! If not, the most realistic timeframe for me would be some time after the first or second week of June.

[julienduchesne]

Hey, @jantman. Finally took some time to work on this. I took a very simple approach which is to add a maximum value in the AwsLimitUsage class. If this value exists, AwsLimit will use it for its threshold check, otherwise it will use its own limit. That way, the Route53 service only creates one AwsLimit and adds AwsLimitUsage's with a maximum. I had to slightly modify the list_limits to display these limits.

Let me know what you think.

[jantman]

@julienduchesne I'm really sorry, but I'm probably not going to get a chance to seriously look this over for a few weeks. I'm moving on June 1st and still have a lot to get straightened out before and the week after.

Glancing over the code it looks good to me, but I really don't have more time than that for now. I'm sorry.

[jantman]

PS - If all else fails, I added an item to my calendar on June 9th to look at this. I should be done with the move, settled in, and ready to get back to my F/OSS projects by then.

[jeffwecan]

@jantman: Any movement on this front since your last comment? (I'd also very much like to see support for route53 limits in this module 🙏.)

[jantman]

@jeffwecan and @julienduchesne Apologies. I've had a really busy few months and took a bit of a sabbatical from my F/OSS work to focus on some personal stuff.

Most of that has wound down, so I'll take another look at this sometime this week and get it dealt with.

[jantman]

I'm so sorry that it took so long to get this merged. I'm doing so now.

The build failure was because of py26 and py33 failures, but support for both of those is being dropped anyway.

I'll update again when this is released.

Thanks so much to everyone who contributed to this PR.

[julienduchesne]

No problem @jantman. Thanks for your help with this PR!

[jantman]

This was released in 5.0.0 just now, and should be live on PyPI.