BIND 10 #3241: Implement NameAddTransaction's DNS update build methods

[BIND 10 Development]

#3241: Implement NameAddTransaction's DNS update build methods
-------------------------------------+-------------------------------------
            Reporter:  tmark         |                        Owner:
                Type:  task          |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp-ddns     |  reviewing
            Keywords:  Task# 4.2.1   |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea1.0-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => marcin


Comment:

 Replying to [comment:7 marcin]:
 > Reviewed commit 5414d2bdabfc5d440848014901c7744979a92486
 >
 > This work required a bit of reading RFCs and understanding them. Well
 done. A few rather minor comments....
 >
 > '''src/bin/d2/d2_messages.mes'''
 > General comment to all messages: It may be reasonable to add a comma
 before word !''reason!'' to separate the request name from the failure
 reason. So for example:
 > {{{
 > +% DHCP_DDNS_FORWARD_ADD_BUILD_FAILURE A DNS udpate message to add a
 forward DNS entry could not be constructed for this request: %1, reason:
 %2
 > }}}

 Picky, picky.  Actually I did this on similiar messages, guess I'm
 slipping in my old age.
 >
 > DHCP_DDNS_ADD_FAILED DHCP_DDNS: Typo !''There precise...!''. Shouldn't
 it be !''The precise..!''?
 >
 >

 Got it.

 > '''src/bin/d2/d2_update_message.cc'''
 >
 > This line:
 > {{{
 >         message_.setRcode(Rcode(Rcode::NOERROR_CODE));
 > }}}
 > could be replaced with:
 > {{{
 >         message_.setRcode(Rcode::NOERROR());
 > }}}
 >

 Fixed. What was I thinking?

 > '''src/bin/d2/nc_add.cc'''
 > addingFwdAddrsHandler: What is it going to happen if the
 buildAddFwdAddressRequest throws an exception? In such case the error is
 logged, the state model is transitioned to the failed state. But, in line
 190 you call sendUpdate. Should sendUpdate be called if building ADD
 request fails? At least the appropriate commentary should be added to
 describe intended behavior of this code.
 >
 > The same comment applies to replacingFwdAddrsHandler() and
 replacingRevPtrsHandler().
 >

 GOOD CATCH!  There should be break at the end of the catch clause.
 I added unit tests for this as well.

 > '''src/bin/d2/tests/nc_add_unittests.cc'''
 > fakeResponse: Typo !''exercise!'' instead of !''exercised!''.
 >

 Fixed.

 > addingFwrdAddrsHandler_sendUpdateException: Consider ASSERT here:
 > {{{
 >     EXPECT_NO_THROW(name_add->addingFwdAddrsHandler());
 > }}}

 Agreed. Same change in similiar tests.
 >
 > '''src/bin/d2/tests/nc_trans_unittests.cc'''
 >
 > prepNewRequest: typo in todo: !DdsnDomain.
 >

 Boy, nothing gets by you does it?  Fixed.

 > '''src/lib/dhcp_ddns/ncr_msg.cc'''
 >
 > setIpAddress: If you keep the IP address in the IOAddress object called
 ip_io_address_, I don't see a reason why you should duplicate the IP
 address in the string object: ip_address_. You can always return string
 version of the IP address by calling .toText(). I don't think there is any
 serious overhead if you do it.
 >
 > Besides, the fact that you keep the same value in two distinct object is
 already making the code vulnerable to inconsistency. Consider the case
 when I call setIpAddress with an invalid address. The ip_address_ (string)
 object will happily accept the new value. The attempt to assign the bogus
 value to the ip_io_address_ object will result in exception and the new
 value will not be assigned. Your object ends up having two different IP
 address values being held in string and IOAddress objects.
 >
 > BTW, your test could have checked that.
 >

 I reworked it a bit so NCR now only stores an IOAddress.

 > IMHO, the setters should be consistent and you could validate fqdn value
 in setFqdn, as you do it in setIpAddress. The same applies to setDhcid.

 I added validation to setFqdn.  setDhcid doesn't need it because it calls
 Dhcid::fromStr() which validates.

 >
 > '''src/lib/dhcp_ddns/ncr_msg.h'''
 >
 > isV4 and isV6 should be const.

 I was just testing you to see if you would catch this.  Fixed.

 >
 > '''src/lib/dhcp_ddns/tests/ncr_unittests.cc'''
 >
 > Did you intend not to put any unit tests for isV4 and isV6?

 Look again, my friend.  They are tested in here:
 TEST(!NameChangeRequestTest, ipAddresses).


 >
 > ipAddresses:
 > {{{
 >     // Verify that an valid address fails.
 >     ASSERT_THROW(ncr.setIpAddress("x001:1::f3"),NcrMessageError);
 > }}}
 >
 > It should be !''an invalid!''. Also, there should be a space before the
 exception name.

 Fixed.

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