route53: fix CAA record ordering for idempotency #46049
Conversation
|
Hi @felixfontein, Thank you for the pullrequest, just so you are aware we have a dedicated Working Group for aws. |
| @@ -593,13 +600,14 @@ def main(): | |||
| identifier_in = str(identifier_in) | |||
|
|
|||
| if rset.type == type_in and decoded_name.lower() == record_in.lower() and rset.identifier == identifier_in: | |||
| if need_to_sort_records: | |||
| # Sort records | |||
| rset.resource_records = sorted(rset.resource_records) | |||
There was a problem hiding this comment.
I don't understand what this changes, as line 621 just sorts the resource records again whether or not need_to_sort_records is true or not.
There was a problem hiding this comment.
This changes the rset object itself; it's needed because the comparison is done in line 621 between rset and wanted_rset. All the other lines (i.e. putting stuff into record) is done only for state == 'get' if I understand it correctly, and not used anywhere else (in particular, not for comparison).
I kept the sorting in line 621 because that used to be done for all record types (but as I said, it is only used for state == 'get'); I could have only sorted again if it wasn't sorted here already, but I thought that would make it unnecessary complex.
There was a problem hiding this comment.
Right, I nearly wrote another comment about how I still don't understand but I got there while writing it :)
|
This change looks good to me from a code review point of view but I haven't tested it and there is no test suite for route53. If someone else can give a second opinion, that would be great. |
|
Hi @felixfontein, I've taken some time to add an integration test that reproduces the CAA issue (but doesn't cover much else) to your fix here. It's a start on the test suite, but for now will not be running in CI. I had a couple other feedback items, but after that I think this is good to go. |
|
@ryansb Thanks a lot for the tests! Out of curiosity: I've seen that the |
|
Mostly the reason is that those are for zones, and the current CI policy doesn't have permissions to update records and such, which would be required for these tests. |
|
Ah, that makes sense. Thanks! |
|
@willthames @ryansb thanks for reviewing, adding the tests and merging! Should I create backports for it (2.7 and 2.6)? |
|
Backports would be great, but when you do, strip out the new integration tests. I don't want to break past integration suites. |
|
Sure, I can do that. (Since the tests are disabled, they shouldn't break anything, though.) |
* Fixing record order for CAA records to properly handle idempotency. * Add integration tests that reproduce CAA failure (cherry picked from commit a727a1e)
* Fixing record order for CAA records to properly handle idempotency. * Add integration tests that reproduce CAA failure (cherry picked from commit a727a1e)
SUMMARY
As reported in #45970, the CAA records returned by boto are sometimes in the order in which they were set, but sometimes not. (This also seems to depend on whether Py2 or Py3 is used. This problem is probably either caused by boto or sax, or by the Route53 API itself.)
This sorts CAA records before comparison, but restores the order in case the record set is finally created or updated via boto.
The PR allows to extend the set of records for which sorting is done by updating the line
need_to_sort_records = (type_in == 'CAA')to something likeneed_to_sort_records = (type_in in ('CAA', 'another type')). I don't know if this is required for other record types as well. Someone more knowledgable of DNS might be able to comment on this.Fixes #45970.
ISSUE TYPE
COMPONENT NAME
route53
ANSIBLE VERSION