BIND 10 #221: Refactoring auth server and merge the axfr and notify logic into it

[BIND 10 Development]

#221: Refactoring auth server and merge the axfr and notify logic into it
-------------------------+--------------------------------------------------
 Reporter:  hanfeng      |        Owner:  jinmei                                        
     Type:  enhancement  |       Status:  reviewing                                     
 Priority:  major        |    Milestone:  05. 3rd Incremental Release: Serious Secondary
Component:  b10-auth     |   Resolution:                                                
 Keywords:               |    Sensitive:  0                                             
-------------------------+--------------------------------------------------

Comment(by jinmei):

 Replying to [comment:7 jinmei]:

 > Anyway, I'm writing a counter proposal ASIO wrapper layer in the
 trac221, some of which have already been committed to the repository.
 Once it's completed I'll merge other part of your change and let you know.

 Okay, I've completed my counter proposal patch.

 It's committed to branches/trac221.  The diff is:
 svn diff -r 2169 svn+ssh://bind10.isc.org/svn/bind10/branches/trac221

 This is a completely from-the-scratch patch, and should be reviewed by
 someone.  Can you review it?

 FWIW, an HTML version of the doxygen documentation is available at:
 http://bind10.isc.org/~jinmei/bind10/cpp/namespaceasio__link.html

 Next, here are some preliminary comments on the code for the AXFR (+
 notify?) support.  Essentially these are comments only on
 AuthSrvImpl::processAxfrQuery() and AuthSrvImpl::processIxfrQuery(), but I
 have some general comments:

  - general
   - please read the coding guideline carefully.  your original patch
 contains many code fragments that break the guideline.
   - I didn't adopt the "nag()" method.  I agree we could log things better
 than in the current form, but it's expensive to construct log message
 string before checking the verbose level.
   - I also didn't use boost::noncopyable.  My understanding is that we
 agreed we don't want to add dependency on boost for this purpose.  I
 didn't oppose to homemaid noncopyable class, but in any event that should
 go to a different task.

  - processAxfrQuery
   - add detailed tests, please.
   - (as a result of refactoring) it's now possible that we receive an AXFR
 query over UDP.  It should be detected and rejected here.  BIND 9 returns
 FORMERR (although I don't know it's implementation specific or is defined
 in the protocol spec)
   - we shouldn't use catch-all catch.  for example, we don't want to catch
 std::bad_alloc here.  Be as specific as poissible, and the broadest
 allowable exception class is isc::Exception.
   - when exception happens we should return an error, rather than ignore
 the query.
   - is it reasonable to establish a connection with the xfrout process
 every time we receive AXFR request?  It seems to be an unnecessary waste.
 It would be better to open a connection at initialization and keep it for
 subsequent AXFR processing.  (but this should probably go to a separate
 ticket).
   - we should probably do a bit more validation on the request.  for
 example, some evil node may send a very long AXFR request, trying to mount
 a DoS by increasing the overhead between the auth server and the xfrout
 process.
   - a related note: alghouh unlikely, sendXfroutRequestInfo() can block,
 and if that happens will stop responding to other queries.  I don't think
 this is an ugent problem, but we'll probably need to make it asynchronous
 eventually.

  - processIxfrQuery
   - first off, adding this stuff seems to go beyond the scope of
 "refactoring".  maybe we should leave this to a separate subseqent ticket.
   - secondly, detailed tests, please.  whether or not we do this work in
 this branch or in a separate ticket.
   - thirdly, is this for "IXFR"?  The original branch name and some part
 of the code seem to indicate NOTIFY.  On the other hand, the dispatch part
 of the code seems to indicate it's for IXFR.  I guess it's for notify,
 because if it's for IXFR the remaining code doesn't make sense at all.  In
 what follows I assume this is for notify.  note that this kind of
 confusion should have been eliminated if you began with tests.
   - overall, this method ignores many error cases/possible exceptions
 (some specific points are noted as comments).  it should be massively
 corrected.
   - is this a short term workaround, or is this the model you're proposing
 for notify support?  if it's a short term hack, that's okay with me.  But
 if this is intended to be a longer term solution, I must oppose to that
 because what it does is very expensive, involving many string
 manipulations.
   - assuming it's a notify, we need to respond to it from this method
 (currently it's ignored)

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