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:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:  major  |  Sprint-20111108
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:3 jinmei]:
 >
 > 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.
 >

 This has been dormant in my queue for quite some time now, but this is
 as good a time to handle it as any, I guess.

 > Now the review comments:
 >
 > - I guess we need a changelog entry for this change.

 Hmm, right, bit late though :/

 Proposal:
 [func?] Check master address upon NOTIFY
 b10-xfrin now checks the address a NOTIFY packet came from, and only
 performs xfrin when the address and port match those from the master
 address in the xfrin configuration.

 But perhaps I need to discuss with jeremy on how to phrase this, as the
 functionality really was already in the previous release

 > - 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.

 Ack, added a format_zone_str() module-level convenience function. Also
 did the same for addresses.

 I think we should make these general utility functions, but esp. for
 the second we need to look at what input data we have available in
 other modules first, so i'll defer this to another ticket (for which
 we can use this code as a base, if this is acked and merged)

 > - 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.
 >

 one thing that certainly didn't make sense is comparing the ports as
 well, but only reporting the address on error, but the above change
 with the format strings should at least fix the disconnect.

 > - 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);
 >       }
 > }}}
 >

 ok, changed to INFO

 > - 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.

 Yes. Removed them.

 Regarding multiple addresses, ack, we will need that at some point.
 I think there are more configuration layout changes we need or want,
 perhaps we should discuss and/or propose those in a separate (set of)
 ticket(s).

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