BIND 10 #1460: Define system-level tests for DDNS

[BIND 10 Development]

#1460: Define system-level tests for DDNS
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jinmei
                       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:  5
        Add Hours to Ticket:  0      |           Total Hours:  6
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 (assigning this back to you, i think we can about close the ticket and add
 things later, but there are a few open questions below)

 Replying to [comment:7 jinmei]:
 > Replying to [comment:6 jelte]:
 >
 > > I am willing to do the adds/deletes and all their special cases at
 some point if we think it is worth it, but these things are also tested in
 the unit tests. Because of that, perhaps, if we are short on time, we may
 only want to test the inter-process parts (i.e. the bind10-specific
 tests), and the TCP test and the IXFR test (which perhaps should be
 considered b10-specific too). But for now, I've left them out before I'm
 spending more than a full day on just this document :)
 >
 > I think it makes sense to focus more on tests as the entire system
 > (e.g. behavior involving multiple components) and let unit tests be
 > responsible for corner case checks.  Technically, unit tests and
 > system (integration) tests are conceptually different and it's not a
 > good practice if we omit cases just because they are covered in a
 > different type of tests, but realistically we are not a very large
 > organization with dedicated QA stuff, so that would be a reasonable
 > compromise.
 >

 Ack. I think we should probably even make two types of what we now call
 system tests; integration tests and conformance tests. But as usual,
 probably something for Later(tm).

 > Comments on the wiki description follow.  This kind of task can never
 > be perfect, so I suggest just fixing some trivial errors and then
 > closing this ticket.
 >

 OK, for some i'll probably include a ([improvement needed here]) or
 something, so we can at least come back to it at some point, hopefully
 without a comprehensive new review

 > '''General'''
 >
 > I've noticed some earlier test descriptions miss corresponding RFC
 > sections.
 >

 Oh right, added

 For the first section (the bind 10 specific tests), I left them out since
 they are supposed to test our internals, not necessarily any RFC
 compliance. Subtests there that happen to conform to RFC sections are
 mostly a coincedence.


 > BIND 9's system tests have their own update cases.  From a quick look,
 > I've not found so many we need to incorporate from them, but you may
 > also want to take a look at it.  It's
 > bind9/bin/tests/system/nsupdate/tests.sh.  One thing that drew my
 > attention is this:
 >
 > {{{
 > # inserting an NS record without a corresponding A or AAAA record should
 fail
 > $NSUPDATE -l -p 5300 -k ns1/session.key > nsupdate.out 2>&1 << END &&
 status=1
 > update add other.nil. 600 in ns ns3.other.nil.
 > send
 > END
 > grep REFUSED nsupdate.out > /dev/null 2>&1 || status=1
 > }}}
 >
 > although the underlying implementation uses the generic zone
 > management module, so in our case it's probably more reasonable to
 > hold off until we do generic zone loading with post-load checks.
 >

 So what this does is refuse the addition of NS records that need glue but
 don't have it? Is this specified somewhere or just a useful feature that
 bind implements?

 I did not write it up yet, but I've added this in a new section 'possible
 additional tests' with a bit of explanation.

 > '''Module tests'''
 >
 > - Is there any reason why we use in-memory over DB (sqlite3 in
 practice)?
 > - note: Tests 1 will need #1986.  With #1986 completed, I think the
 >   RCODE can be of implementator's choice.  I'd slightly prefer NOTIMP,
 >   but REFUSED would also be okay considering it's one form of 'reject
 >   all' ACL.

 Most of the test cases assume direct sqlite3 usage, and I thought this
 would be a good place to test that the 'update inmem after update' also
 works. Added a note that we may want to split these up.

 > - We may want to test what should happen if a running b10-ddns is
 >   stopped.
 >

 Ack, added to the same test set, one to one-shot stop b10-ddns, and one to
 stop running it completely.

 Ideally this test would also try to see what happens if it is stopped
 during the handling of an actual transfer but I'm not sure we have the
 instrumentation to reliably test such a case, so I left it out.

 > '''SOA serial tests'''
 >
 > - test 4: "e.g." should be add_1_4?

 ack

 > - test 6: I think it can be implementation dependent, but BIND 9 seems
 >   to avoid SOA increment if the diff ends up being empty.
 > {{{#!c
 >       /*
 >        * If any changes were made, increment the SOA serial number,
 >        * update RRSIGs and NSECs (if zone is secure), and write the
 update
 >        * to the journal.
 >        */
 > }}}

 Yes, this is option 2 from section 3.6. Currently we are implementing
 option 1, but I agree that 2 would be better. This was already included in
 the tests (5, 6, and 7 of the erial tests), I updated the note with some
 more information about that.

 > - We may want to test this, although this level of thing should also
 >   be covered in unit tests:
 > {{{
 >    7.11. A zone's SOA SERIAL should never be set to zero (0) due to
 >    interoperability problems with some older but widely installed
 >    implementations of DNS.  When incrementing an SOA SERIAL, if the
 >    result of the increment is zero (0) (as will be true when wrapping
 >    around 2**32), it is necessary to increment it again or set it to one
 >    (1).  See [RFC1982] for more detail on this subject.
 > }}}

 Added to serial tests.

 The RFC does not seem to be very clear on what to do if the update itself
 tries to set the serial to 0. It says it is not allowed, but should we
 then change it to 1 or refuse the update?

 > - There can be a case where the zone doesn't even have an SOA for
 >   DB-based data sources (in-memory should ensure that it never
 >   happens).
 >

 Eek. While of course true, I don't think there is anything that one could
 do to make a zone more broken than that :) While we should of course not
 crash on it I have no idea on how it is supposed to behave in that
 scenario. Generate one with defaults (if not provided in update)?

 Also added to possible additional tests.

 > '''Zone section tests'''
 > - There's another case: the specified zone is a secondary.
 >

 ack, added

 > '''Prerequisite section tests'''
 >
 > From a quick look there doesn't seem to be anything obviously wrong,
 > although I have to admit I didn't check the tests are comprehensive
 > (and, in practice, we may not necessarily need to be 100%
 > comprehensive in system tests as long as they are covered in unit
 > tests) or they don't have any minor errors.
 >

 ok. Well we'll find out the second case when we implement them :p
 I am almost sure the list of tests is not exhaustive; there is a great
 deal of combinations to make here (and even more so for the updates)...

 > '''Update section tests'''
 >
 > - it doesn't seem to contain post-prescan things.  is the intent that
 >   they should be covered in other cases?

 No I got tired and did not specify these (yet), at this point I thought it
 was better to focus on the actual system tests and not so much on the
 compliance tests; basic addition and deletion should be covered by other
 tests here. See also previous point on completeness :)

 Added note

 > - what about special cases such as deleting apex NS?
 >

 ditto

 > '''Prescan failures'''
 >
 > - Shouldn't the "additional section"s be "update section"? (the update
 >   section is ddns-equivalent of the answer section, btw)

 of course, dunno how that got there.

 > - (maybe it's more about unit-level tests) what about MAILA/MAILB?
 >

 ack, left to note once again

 > '''DDNS over TCP tests'''
 >
 > - note: we may need to develop our own tool for this test (update:
 >   probably not; nsupdate has the -v option to force TCP)

 ok cool. When we have some form of nsupdate ourselves, we can use that
 (I'd *really* like to create a better nsupdate tool, if only to have
 readline support :p). Although then we would be testing our own code
 against our own code, which reduces dependencies, but could in theory
 cause bugs to go unnoticed, even on system tests.

 > - and, if possible, we might want to check what happens if the test
 >   client tries to keep the TCP connection for multiple DDNS
 >   transactions (I suspect b10-ddns won't support such an operation
 >   even after we complete #2003)

 Added to possible additional tests.

 Replying to [comment:9 jinmei]:
 > A couple of more BIND 10 specific things:
 >
 > - It's worth testing what happens b10-ddns once started but stops
 >   running, then restarts again.  update requests will have to be
 >   forwarded to and processed by b10-ddns after restart.  Note that the
 >   first request after restart will probably fail with SERVFAIL because
 >   b10-auth still keeps the socket with the now-stale connection, and
 >   it can only detect the failure in the next push() operation (#1986
 >   could solve this)

 I was kind of thinking that if the system notices ddns is down it would
 always return SERVFAIL until it has been restarted and is answering again.
 Are you suggesting it should buffer the updates?


 > - Likewise, we should check the behavior when b10-auth dies (and
 >   restarts) after establishing a connection with b10-ddns.  That's
 >   apparently the scenario that makes xfrout fall into a busy loop.
 >   The smarter socket session library and the way b10-ddns uses it
 >   should avoid the bug, but we need to check.

 OK, added to first test set.

 Replying to [comment:10 jinmei]:
 > Yet another BIND 10 specific thing:
 >
 > - Check if notify is sent out from xfrout after update.  Maybe by
 >   configuring a separate secondary and seeing if the secondary zone is
 >   immediately updated.

 added.

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