BIND 10 #1457: use the difference normalizer in the DDNS module

[BIND 10 Development]

#1457: use the difference normalizer in the DDNS module
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  vorner
                       Type:  task   |                Status:  reviewing
                   Priority:         |             Milestone:
  medium                             |  Sprint-20120612
                  Component:  DDNS   |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DNS
Feature Depending on Ticket:  DDNS   |  Estimated Difficulty:  3
        Add Hours to Ticket:  0      |           Total Hours:  22
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => vorner


Comment:

 Replying to [comment:14 vorner]:
 > Hello
 >
 > I must have been paying little attention when reading the diff, since my
 comments are a lot shorter than the diff.
 >

 phew! ;)

 > '''Log messages''':
 >
 > The text „In theory, if the datasource is implemented +correctly, no
 changes should have been made to the zone contents.“ provokes to the
 question „In theory ‒ but how about practice?“. In fact, the text is not
 very reassuring everything is OK and implies we are not confident at all
 with the data sources we ship.
 >

 Hmm. True, though the comment was mostly about the datasources we don't
 ship :) But that's really the datasources responsibility, removed that
 entire line.

 > The message with id `LIBDDNS_UPDATE_DELETE_RRSET_NOT_EMPTY:` contains
 wrongly capitalized „Rrset“.
 >

 ack, fixed

 > '''Code comments''':
 >
 > {{{
 > (the rrset in *wargs is replaced by the temporary one)
 > }}}
 >
 > If I leave aside the fact that the function parameter is `*kwargs`, not
 `*wargs`, the sentence completely confused me. I got little bit cleaner
 after reading the code, but it should be clarified, or the function be
 more intuitive ‒ see below.
 >

 see below2

 > {{{
 > If this method raises an exception, these members are not set
 > }}}
 >
 > This is not true. If the `find_zone` throws, there are some options set
 already. It may make sense to place the `find_zone` call first in the
 assignments.
 >

 ah completely true, serves me right for writing the docstring then adding
 code :p Fixed.

 > {{{#!python
 >         # (note; SOA is handled separately by __do_update, so that one
 >         # is not explicitely ignored here)
 >         if rrset.get_type() == RRType.SOA():
 >             return
 > }}}
 >
 > The comment somewhat contradicts the code, it says it is _not_ ignored.
 >

 yup, that not was incorrect, unnotted.

 > {{{
 > the same offort whether
 > }}}
 > typo: offort
 >

 s/offort/effort/

 > '''Code''':
 > The `foreach_rr_in__rrset` is strange. It takes the result of the last
 call. Is it of any practical use? Also, I think it would be more user-
 friendly and intuitive if it returned a context, so you could write it as:
 > {{{#!python
 > foreach_rr(rrset):
 >      something_do_do(rr)
 > }}}
 >
 > This should be possible, since the `TestCase.assertRaises` can be used
 this way, if the function is not passed, it returns some kind of strange
 object that makes the thing work.
 >

 oh yeah, good point. Apparently I too fall into the trap of
 overcomplicating things sometimes :)

 changed it to a generator, and it indeeds makes both the method itself and
 calling it more clear


 > This sounds really fishy. Is such situation allowed at all?:
 > {{{#!python
 >                     # In case there's multiple soa records in the update
 >                     # somehow, just take the last
 >                     foreach_rr_in_rrset(rrset, self.__set_soa_rrset,
 rrset)
 > }}}

 AFAICT, the RFC is a bit unclear on that; it only talks about one RR at a
 time, and if it is a SOA, delete whatever is there, which when
 extrapolated, would result in the current behaviour (though it ignores
 serial checks at this point). We could FORMERR on it, I just didn't think
 it worth it at the time. If you do, I can add it (and an associated log
 message about duplicate SOA records)

 >
 > In the `__do_update_delete_rrset`, what if the `find` does not return
 SUCCESS, but a DELEGATION, for example, and there's a (different) rrset
 presented for the answer?
 >

 hmm, did I misunderstand the behaviour of FIND_GLUE_OK? It would appear to
 return NXDOMAIN if you ask for a name below a zone cut with FIND_GLUE_OK.

 Anyway, still a valid point; this would horribly fail even for an NXDOMAIN
 (or as you said, any non-SUCCESS return codes); I added a check for a
 SUCCESS result before deleting or inspecting the data. And added a couple
 of tests that muck with some potentially below-zone-cut data to make sure.


 > Considering the `__ns_deleter_helper`, what is the proper way to change
 all the NSs? I think removing them all and then adding the new ones will
 not be allowed. Also, adding first and then removing the old ones will not
 work either, as the function will take the original values, ignoring the
 new additions. Which brings me to the question ‒ how should it work if
 there are some things added and removed from the same RRset? What is the
 correct result of „add this RR“ followed by „remove whole RRset“?
 >

 shit. Completely true, but now we run into the problem that we first
 buffer everything in that diff class to possibly support IXFR later. So we
 probably need to be able to search and remove records from the buffers in
 said class to handle cases like these (part of which would probably also
 solve the 'add and delete the same record' issue). However, as I think
 this is a nontrivial task in itself, I propose we defer that to another
 ticket. We can discuss whether or not to put it into this sprint (since we
 did intend to finish ddns), but at least we can get this branch in, and
 have basic support with a few known issues (which can be circumvented by
 doing two updates), and solve potential conflicts with the acl and serial
 update code now).

 > Is this a forgotten TODO, or is there a ticket to handle these?:
 > {{{#!python
 >         if self.__added_soa is not None:
 >             new_soa = self.__added_soa
 >             # serial check goes here
 >         else:
 >             new_soa = old_soa
 >             # increment goes here
 > }}}
 >

 yes it's ticket #1514, those comments are an attempt to help whoever
 implements or merges that branch :)

 > The `add_rdata` is below `create_rrset`, even when `create_rrset` uses
 `add_rdata`. I think it is not a problem syntactically, but I find it
 little bit confusing. Maybe I'm too old-fashioned and used to pascal and C
 though.
 >

 moved add_ up

 > Should the `initalize_update_rrsets` and `check_inzone_data` helper
 functions be private like so many other functions?
 >

 if nothing else, for the sake of consistency, yes. Changed

 > Also, one more general question: What happens if RRs of the same RRset
 are interspersed with another RR? Will it create one RRset or two? Because
 if two, there'll  be problems with updates.

 I think I noted on this somewhere in a comment, but may have done so in
 the prerequisite handler; our Message class already puts them together in
 such a scenario, so this potential problem is already fixed in underlying
 structures. If it didn't i'd say the prescan should probably prepare data
 so that it's ready for actual processing. This behaviour by libdns++ can
 cause even more problems related to both adding and deleting stuff in one
 update, btw...

 Added a simple test to see whether it kinda works as expected with
 interspersed additions, but in this specific case it's hard to tell where
 exactly they are merged; might as well be done in the datasource storage
 as far as this test is concerned.

-- 
Ticket URL: <http://bind10.isc.org/ticket/1457#comment:15>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development