BIND 10 #1298: don't do xfrin upon notify when address is unknown

[BIND 10 Development]

#1298: don't do xfrin upon notify when address is unknown
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  UnAssigned
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:  New Tasks
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:1 jelte]:
 > the full master address (including address, inet family, port) is kind
 of an ad-hoc structure, which is completely checked, but the log message
 only mentions the address itself. So this code isn't perfect, but
 depending on whether we want to sneak this into the release we can make it
 better or make that a todo

 I've reviewed the code, and while I personally didn't think it
 sufficiently
 urgent for a last minute push I merged the current snapshot of the branch
 (with some editorial fixes) to master and pushed it as Jeremy suggested
 so.

 To fully complete this task I'd like to discuss some details, so I'll
 keep this ticket open.  I've not deleted the branch yet.  I also guess
 we should defer this task to the next sprint unless we consider it an
 urgent fix.

 Now the review comments:

 - I guess we need a changelog entry for this change.
 - We generally use the format of <zone_name>/<zone_rr_class> for logging
   in xfrin.  I suggest revising the format for XFRIN_NOTIFY_UNKNOWN_MASTER
   in this sense.  We also might want to extract XfrinConnection.zone_str()
   so that it can be used module-wise.
 - As you also seem to have noticed, I'm not sure if it makes sense to
   compare two tuples of address information:
 {{{
                     if notify_addr == master_addr:
 }}}
   Note also that we should probably consider the case where the
   address matches but the ports don't.

 - I agree with Jeremy about the log level for
   XFRIN_NOTIFY_UNKNOWN_MASTER.  I'd even use info in such cases.  FWIW
   BIND 9 uses the info level in this situation:
 {{{
         } else if (i >= zone->masterscnt) {
                 UNLOCK_ZONE(zone);
                 dns_zone_log(zone, ISC_LOG_INFO,
                              "refused notify from non-master: %s",
 fromtext);
                 inc_stats(zone, dns_zonestatscounter_notifyrej);
                 return (DNS_R_REFUSED);
         }
 }}}

 - With this change, the last two assertEqual in
   test_command_handler_notify_known_zone don't make sense any more
   (so I commented them out)
 {{{
 #         self.assertEqual(TEST_MASTER_IPV4_ADDRESS,
 #                          self.xfr.xfrin_started_master_addr)
 #         self.assertEqual(int(TEST_MASTER_PORT),
 #                          self.xfr.xfrin_started_master_port)
 }}}
   For a longer term we'll allow to have a list of master addresses,
   and select one of the listed addresses for the source of xfr,
   preferring the notify source.  At that point we'll probably need
   a completely different types of tests, so, as a result, I suspect
   we should simply remove these commented-out tests right now.

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