Opened 7 years ago

Closed 7 years ago

#3036 closed enhancement (complete)

Modify DHCP6 to generate NameChangeRequest

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Sprint-DHCP-20130821
Component: ~dhcp-ddns(obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by marcin)

DHCP6 server should be act upon FQDNs sent by clients. If requested, it should generate NameChangeRequests that would in turn result in sending DNS Updates.

Subtickets

Change History (16)

comment:1 Changed 7 years ago by marcin

  • Description modified (diff)

comment:2 Changed 7 years ago by tmark

This ticket corresponds to Task 2.4 in the D2 Development list.

comment:3 Changed 7 years ago by marcin

  • Owner set to UnAssigned
  • Status changed from new to reviewing

Implemented the new Option6ClientFqdn class which encapsulates the DHCPv6 Client FQDN Option. Server now processes this option sent by client and generates the response. The server behaviour with respect to the FQDN Option is not configurable at the moment. This will be implemented in the next turn. When client sends the FQDN option server will generate NameChangeRequests which are added to the FIFO queue. This queue is not processed at the moment and all requests stored there are dropped. In the future, it is planned to implement the logic which will passed requests from this queue to the NameChangeSender to send them to bind10-d2 module for processing.

Also, added a section in the developer's guide that describes how the FQDN option is processed.

Proposed ChangeLog entry:

6XX.	[func]		marcin
	bind10-dhcp6: Server processes the DHCPv6 Client FQDN Option
	sent by a client and generates the response. The DHCPv6 Client
	FQDN Option is represented by the new class in the libdhcp++.
	As a result of FQDN Option processing, the server generates
	NameChangeRequests which represent changes to DNS mappings for
	a particular lease (addition or removal of DNS mappings).
	Currently all generated NameChangeRequests are dropped. Sending
	them to bind10-d2 will be implemented with the future tickets.
	(Trac #3036, git abc)

comment:4 Changed 7 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:5 follow-up: Changed 7 years ago by tomek

General comments:

Please rename option6_client_fqdn.cc|h to option6_fqdn.cc|h. We try
to keep the names as short as possible. Otherwise we will have a
really long names like option6_authentication.cc or option6_client_identifier.cc

This work really should be split into 2 tickets: option class and then
follow-up ticket that uses that class. Keep that in mind when doing
its v4 counter-part.

option6_client_fqdn.h
FLAG_S, FLAG_O FLAG_N are defined as enums. They should be const
uint8_t as flags field can have more than one of those set. Enums are
used when only one of the possible values is valid.

I don't like the Option6ClientFqdnImpl class much. I understand the
reasons, but similar result could be achieved with having just one
class and domain_name_ defined as private (or protected for testing
purposes). It is true that there is no need to include path to
dns/labelsequence.h when including option6_client_fqdn.h header, but
on the other hand you still need to link with dns library. So the
bottom line is that the Option6ClientFqdn comment in last paragraph
about changes being transparent is not really valid.

Some of the methods in Option6ClientFqdn can be private (or at least
protected), e.g. packDomainName().

option6_client_fqdn.cc
Option6CXlientFqdnImpl and its methods should be documented.

This class allows construction of partial or even empty domain names.
That is not really needed on the server side (Last paragraph in
section 4.2 of RFC4704: "Servers SHOULD send the complete fully
qualified domain name in Client FQDN options."). But I suppose it
is ok, as we will hopefully soon use those options in perfdhcp and
other projects.

Why domain_name_ is a pointer to dns::Name, rather than object itself?
Can dns::Name store empty domain-name? If it can, then I suggest
modify the code to use dns::Name directly, rather than via pointer.

checkFlags() You can't throw if MBZ field is non-zero. See last
paragraph in section 4.1 of RFC4704. Server MUST ignore these
bits. (just bits, not the whole option). If you really want to do
something, printing a debug message is probably ok, though. But it
could be an attack vector to clog server logs.

Assignment operator, Option6ClientFqdnImpl() constructor: What will
happen if you try to assign/copy an option without domain name? :)

parseWireData(): exception comment should say what's the option
size (0) and what is the expected minimum (1).

Option6ClientFqdn::Option6ClientFqdn(first, last):
the data is kept twice in the object (once in Option.data_ and
second time in Option6ClientFqdnImpl). Is there a need for such
redundancy?

getFlag(): That check is weird. Flag enum has only 3 values defined:
1, 2 and 4. Any values in between are not correct. Will the compiler
even allow casting uint8_t into Flag enum?

setFlag(): the same as getFlag(). You are using enum as uint8_t here.
I think the parameters should be uint8_t.

Is there even a guarantee that enum values are coded on 8 bits? I
don't think so. What about big endians? The S flag could be stored in
memory as 0x01000000. Please get rid of the enum Flag and substitute
it with uint8_t. You may add a comment that some combination of the
flags are allowed, so enum is not appropriate here.

What is wrong will clearing all bits (flag=0)? Why do you throw an
exception? My understanding is that 000 sent by client is a valid
combination (client requests server to perform PTR only).

toText(): please reformat toText() to return a single line response.
We have a debug code somewhere that prints out whole packets with
all options. It will be confusing if some options take up more than
one line (multi-line strings should be reserved for encapsulated
options)

alloc_engine.h
When updating copyright year, please use 2012-2013, not 2013.

alloc_engine.cc
Please implement Lease::setHostname(const bool fwd, const bool rev,

const string& name). That will help avoiding invalid

combinations (e.g. fwd=true, rev=true, name="") are not allowed.
Since the fields are public we can't really make sure of it, but
having such a simple method with set up best practice here.

option_definition.cc
I see many cosmetic changes related to maximum line length.
We have and agreemeing in bind10 chatroom that we typically try
to keep the code under 80 columns, but up to 100 columns is considered
ok. If you still use terminal that can't handle more than 80
columns, please at least do not add extra << to the code as
it (marginally) bloats the output. Exploit the preprocessor to
do the work for you:

cout << "text1"
        "text2" << end;

dhcp6_srv.h
I don't like the inclusion of d2/ncr_msg.h. Can you move it to
dhcp6_srv.cc? After that change suddenly the number of dependencies
for dhcp6 became much more complex. Keep in mind that while
ddns will be used in typical environment, we will eventually
need to address embedded/CPE market. It is realistic to expect
that the server will handle FQDN option, but will not do any
DDNS stuff. In fact, we should have something like
--disable-dhcp-ddns parameter in configure that would disable
all DDNS stuff (except handling client FQDN option).

General question for dhcp6_srv modifications: what will the server
do if there is a client that sends 5 IA_NA options?

appendClientFqdn(): please make it clear whether it is fqdn
send by the client or the server's response

dhcp6_srv.cc
appendClientFqdn(): The first if seems wrong. Something
is also wrong with the tests that didn't pick it up.

createNameChangeRequests(): We will eventually support
a case when there is more than one address in a given
IA_NA. That is an uncommon, but valid case (e.g. during
renumbering events). The code will eventually have to do

  for (each IA_NA/IA_TA in the response) {
      for each (address in this IA) {
          generateNCR()
      }
  }

Please add an appropriate @todo in the code.

The method must get an extra parameter: add, update (and possibly
delete or skip). Right now you're generating ncr ADD for RENEWs and
you don't generate anything for RELEASEs.

createRemovalNameChangeRequest(): Third comment it wrong.
You can't create DHCID out of hostname. What you trying to
generate here will be used as AAAA, not DHCID.

run(): Please update the comment near call to
sendNameChangeRequests() to say that we call that method to
send NCRs. You mentioned that it's a stub in at least 3 places.
It will be tricky to update all those comments once we
implement actual NCR transmissions.

processClientFQDN(): The first if is technically correct,
but looks odd.

Please add a @todo somewhere in the processClientFqdn(). Ideally
all uses of constants should be replaced by methods (e.g.
FQDN_GENERATED_PARTIAL_NAME with generateFqdnPartialName(question)),
but I don't insist on it. It can be done as part of the next ticket.


Rest of the review will continue tomorrow.

comment:6 follow-up: Changed 7 years ago by tomek

  • Owner changed from tomek to marcin

Many of the hacks done in Makefiles (like including d2/ncr_msg.cc in
dhcp6) are no longer necessary, because libdhcp_ddns is already merged
to master. Please merge master into trac3036 and clean up the
makefiles.

ncr_msg.cc|h

fromDUID(): Changes look ok, but there's one thing that requires a bit
though (or at least extra comment in the text). RFC4701, Section 3.5
says that FQDN should be in canonical format, as defined in RFC4034,
Section 6.2. That section talks about converting FQDN to lowercase. Do
we do that? If we do, we should make a brief comment about it and
point to specific place where this conversion happens. If we don't, we
should extend the code. I'm not saying that this is the best place. It
isn't. Probably somewhere in dns/labelsequence would be better (or in
places that use that class).

ncr_unittests.cc
Please also test min (1), max (128) lengths of DUID when calculating
DHCID.

dhcp6/dhcp6.dox
Please udpate the copyright year to 2012-2013.

Please also update doc/devel/mainpage.dox with a reference to
dhcpv6DDNSIntegration.

Please update section dhcp6-std in doc/guide/bind10-guide.xml.

Second paragraph gives wrong RFC. It should be 4703, not 4307 (which
is about IKEv2).

Second paragraph in dhcpv6DDNSIntegration should be moved to
doc/guide/bind10-guide.xml to section dhcp6-limit. Also, the comment
about DNS Update not being supported needs to be removed there.

bind10-dhcp6 => b10-dhcp6

"Depending on the message type, a DHCPv6 server may generate 0, 1 or 2
NameChangeRequests? during single message processing." Is this true?
How many NCRs will be generated if client sends REQUEST with 10
IA_NAs and server assigns them?

The list "(no update, reverse only update, both reverse and
forward update)" is not complete. Also forward only is
possible. Although client may request both, server can be configured
to do only forward updates.

The @todo starting with "The decision about not..." is confusing. I
don't understand what is left to do there. Do you mean writing the
house keeper code? There's no need to mention it here. I think it is
ok to remove the whole paragraph. If you want to keep it, please
rephrase it to make its point clearer.

"The DHCPv6 Client FQDN Option is comprises "NOS" flags" =>
"The DHCPv6 Client FQDN Option contains "NOS" flags

dhcp6_srv_unittest.cc
This code added around 700 lines of new code to the file. For hooks, I
did split them into separate file (see hooks_unittest.cc). You may
consider doing the same. It is ok if you prefer to do it as a separate
ticket, though.

Why do you need N,O,S flags redefined? See also previous comment about
migrating Option6ClientFqdn::Flags enum to uint8_t.

FqdnDhcpv6SrvTest class and its members should be documented.

testFqdn(): != 0 is not required in the flag_{n,s,o} initialization.

serverAAAAUpdatePartialName(), serverAAAAUpdateNoName() and similar
should probably use the same constants that are used in the
Dhcpv6Srv::processClientFqdn(). It is ok to leave the code as it is
now, but you'll likely forget to remove/update it when
Dhcpv6Srv::processClientFqdn() is implemented using actual values.

createNameChangeRequests(): I like this test a lot and the fact that
it checks mulitple IA_NAs in a single message.

FqdnDhcpv6SrvTest.processSolicit(). Interesting test. What about a
case when during ADVERTISE generation server reuses old expired lease?
I think it should then delete old entries, but I haven't given it
enough thought, so I'm not 100% convinced. If we don't do it, client
may get ADVERTISE and then try to resolve it and see who (what domain
name) was using that address before.

option6_client_fqdn_unittest.cc
N,O,S flags are defined here third time.

Option6ClientFqdnTest.assignment, .copyConstruct: Please extend the
tests to copy empty name.

Please add a test that will check that the code throws an exception
if:

  • any label is longer than 63 octets
  • the whole domain name is longer than 255 bytes

See RFC4704, section 4.2, first paragraph.

Also see my comment about ignoring MBZ field. The code should not
throw. RFC4704 says that the server must ignore those bits, not the
whole option.

Finally, the code doesn't build on Ubuntu 13.04 x64.

You include d2/ncr_msg.cc when building dhcp6, but you do not include
$(BOTAN_INCLUDE) in AM_CPPFLAGS in src/bin/dhcp6/Makefile.am

The following fix makes the compilation possible, but it later fails linking.
I have not pursued it further as the libdhcp-ddns code on master makes
any fixes to the current linking issues obsolete.

diff --git a/src/bin/dhcp6/Makefile.am b/src/bin/dhcp6/Makefile.am
index 6f2c694..1f00c66 100644
--- a/src/bin/dhcp6/Makefile.am
+++ b/src/bin/dhcp6/Makefile.am
@@ -3,7 +3,7 @@ SUBDIRS = . tests
 AM_CPPFLAGS = -I$(top_srcdir)/src/lib -I$(top_builddir)/src/lib
 AM_CPPFLAGS += -I$(top_srcdir)/src/bin -I$(top_builddir)/src/bin
 AM_CPPFLAGS += -I$(top_srcdir)/src/lib/cc -I$(top_builddir)/src/lib/cc
-AM_CPPFLAGS += $(BOOST_INCLUDES)
+AM_CPPFLAGS += $(BOOST_INCLUDES) $(BOTAN_INCLUDES)
 
 AM_CXXFLAGS = $(B10_CXXFLAGS)
 if USE_CLANGPP

We had a long discussion on bind10 chatroom today about using enums for
flags in FQDN option. The source of confusion comes from the fact that
the 'flag' word it is used in two contexts. The first one refers to the
whole bitfield called 'flags', which flag refers to only a single bit.
Both Tom and I would prefer to use uint8_t for both as more then one
flag is allowed to be set. And it seems wrong to do logical operations
on enums. However, if our arguments didn't convince you, please at
least rename the FLAG_N to BIT_N as this information is referred to as
'N bit' in RFC4704. Also, please use plural form (flags) when you refer
to the whole bitfield. The distinction would then be easier to understand.

Your ChangeLog? proposal is ok, but a bit long. Also, D2 is now officially
called b10-dhcp-ddns, so please use that name.

I have no more comments at this time. I would definitely like to see this
ticket again.

comment:7 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by marcin

Replying to tomek:

General comments:

Please rename option6_client_fqdn.cc|h to option6_fqdn.cc|h. We try
to keep the names as short as possible. Otherwise we will have a
really long names like option6_authentication.cc or option6_client_identifier.cc

I would avoid making option names too short. The mnemonic for option 39 is OPTION_CLIENT_FQDN and the full option name is ''DHCPv6 Option Client FQDN''. I wanted the class to reflect the actual option name. It precisely identifies the purpose of the class.

This work really should be split into 2 tickets: option class and then
follow-up ticket that uses that class. Keep that in mind when doing
its v4 counter-part.

In fact, it should be 3 tickets: implementation of the option, changes to allocation engine to support FQDNs, changes to the DHCPv6 server to process the option. Optionally, I could add another ticket to add support to store FQDN information in MySQL. But, this will be implemented together with the relevant changes for DHCPv4.

option6_client_fqdn.h
FLAG_S, FLAG_O FLAG_N are defined as enums. They should be const
uint8_t as flags field can have more than one of those set. Enums are
used when only one of the possible values is valid.

I told you on the chatroom, that use of enums does certain things more clear from the API point of you. Functions like getFlag() and setFlag() require that there is a single ''bit'' specified (because you want to get a value of the bit N, or bit S or bit O etc.) Enums make it more clear to the caller that it is expected that only one of them is specified. Caller would be able to pass different values (e.g. 0x3) but it is not that easy because he would have to cast the value of 0x3 from int to enumeration type. This discourages invalid use of these two methods. This doesn't work like this when use uint8_t.

Technically, both solutions (using enums and not using enums) work so it is just a discussion what is more/less clear from the interface point of you and we seem to have different opinions. I decided to do the change to avoid confusion in the use of constructor which takes uint8_t value to accept the combinations of flags. I also extended the description of setFlag and getFlag functions to clarify that only specific uint8_t values should be used.

I don't like the Option6ClientFqdnImpl class much. I understand the
reasons, but similar result could be achieved with having just one
class and domain_name_ defined as private (or protected for testing
purposes). It is true that there is no need to include path to
dns/labelsequence.h when including option6_client_fqdn.h header, but
on the other hand you still need to link with dns library. So the
bottom line is that the Option6ClientFqdn comment in last paragraph
about changes being transparent is not really valid.

I disagree. The pimpl idiom is a common technique to separate the interface from the implementation. Not only it is common C++ programming technique, but also it is used in bind10 already in many places. I guess we both aren't happy with the fact that we need to link with libdns. If I do what you suggest, not only do I have to link with libdns but also I need to include a libdns header. Suppose, there are people who want to use libdhcp and make use of the !Option6ClientFqdn class. If they do, together with the option6_client_fqdn.h header they include a bunch of stuff from libdns. Who knows, maybe they even use functions from there to do some specific things. When we write the common libraries we should really avoid misuse of our classes.

Some of the methods in Option6ClientFqdn can be private (or at least
protected), e.g. packDomainName().

No. This function is used in dhcp6_srv.cc and thus must be public.

option6_client_fqdn.cc
Option6CXlientFqdnImpl and its methods should be documented.

I added documentation.

This class allows construction of partial or even empty domain names.
That is not really needed on the server side (Last paragraph in
section 4.2 of RFC4704: "Servers SHOULD send the complete fully
qualified domain name in Client FQDN options."). But I suppose it
is ok, as we will hopefully soon use those options in perfdhcp and
other projects.

I agree it is not needed on the server side. But, the libdhcp is not a server-only library. So, I don't really understand the comment.

Why domain_name_ is a pointer to dns::Name, rather than object itself?
Can dns::Name store empty domain-name? If it can, then I suggest
modify the code to use dns::Name directly, rather than via pointer.

It is a pointer because when I set it to NULL it marks that the domain name is empty. Please read through the class functions. This should clarify how this pointer is used.

checkFlags() You can't throw if MBZ field is non-zero. See last
paragraph in section 4.1 of RFC4704. Server MUST ignore these
bits. (just bits, not the whole option). If you really want to do
something, printing a debug message is probably ok, though. But it
could be an attack vector to clog server logs.

Ok. I shouldn't be throwing when I received an option with MBZ bits set, but I should throw if I am attempting to send option with MBZ bits set. I made a relevant change. Depending on the constructor used the exception is thrown or not.

Assignment operator, Option6ClientFqdnImpl() constructor: What will
happen if you try to assign/copy an option without domain name? :)

Good catch. I added a check for this case. I also realized that the operator is not really used anywhere in the current code. So, technically I could remove the operator, but I left it, if it occurs to be needed in the future.

parseWireData(): exception comment should say what's the option
size (0) and what is the expected minimum (1).

Ok.

Option6ClientFqdn::Option6ClientFqdn(first, last):
the data is kept twice in the object (once in Option.data_ and
second time in Option6ClientFqdnImpl). Is there a need for such
redundancy?

Well, isn't it what you have been doing for other options too? Isn't it what we do for all options?

The purpose of the specialized class is to parse the data and store them in the data structure which fits the format of the option. So, what is the actual problem?

getFlag(): That check is weird. Flag enum has only 3 values defined:
1, 2 and 4. Any values in between are not correct. Will the compiler
even allow casting uint8_t into Flag enum?

Yes, I would allow casting from 0x3 to enum. Please refer to the C++ standard.

setFlag(): the same as getFlag(). You are using enum as uint8_t here.
I think the parameters should be uint8_t.

See above.

Is there even a guarantee that enum values are coded on 8 bits? I
don't think so. What about big endians? The S flag could be stored in
memory as 0x01000000. Please get rid of the enum Flag and substitute
it with uint8_t. You may add a comment that some combination of the
flags are allowed, so enum is not appropriate here.

The big endian is not an issue, unless you copy values to the memory. This is not the cases here.

What is wrong will clearing all bits (flag=0)? Why do you throw an
exception? My understanding is that 000 sent by client is a valid
combination (client requests server to perform PTR only).

You may have misunderstood the purpose of this method. There are three bits that you want to set or get: N, O, S. Each is identified by the value (mask) which is used to set this bit in the flags field: 0x1, 0x2, 0x4. Only those values makes sense. Specifying 0x0 doesn't make sense because it doesn't specify which bit you want to get/set.

toText(): please reformat toText() to return a single line response.
We have a debug code somewhere that prints out whole packets with
all options. It will be confusing if some options take up more than
one line (multi-line strings should be reserved for encapsulated
options)

Ok. I made a change to make it consistent. But, I am not very happy about the format of the text being printed. In my opinion it would be much easier to read the log if we made proper indentation. Sub-options could be more indented. We can have some offline discussion about it.

alloc_engine.h
When updating copyright year, please use 2012-2013, not 2013.

Yes. It was accidental.

alloc_engine.cc
Please implement Lease::setHostname(const bool fwd, const bool rev,

const string& name). That will help avoiding invalid

combinations (e.g. fwd=true, rev=true, name="") are not allowed.
Since the fields are public we can't really make sure of it, but
having such a simple method with set up best practice here.

I wish we followed this best practice everywhere in the code. I am not going to do it now, because it would bloat this ticket. This ticket is not really about changing how Allocation Engine manages internal data. New ticket: http://bind10.isc.org/ticket/3096

option_definition.cc
I see many cosmetic changes related to maximum line length.
We have and agreemeing in bind10 chatroom that we typically try
to keep the code under 80 columns, but up to 100 columns is considered
ok. If you still use terminal that can't handle more than 80
columns, please at least do not add extra << to the code as
it (marginally) bloats the output. Exploit the preprocessor to
do the work for you:

cout << "text1"
        "text2" << end;

We should either work with all developers to document this informal agreement and make it formal or we should stick to 80.

My terminal is doing good and it can absorb more than 80 chars but if I open two windows in emacs, each 80 chars wide, the code gets wrapped.

I will not expand it to over 80 lines because other developers don't really agree that it is allowed, and they also review my code.

dhcp6_srv.h
I don't like the inclusion of d2/ncr_msg.h. Can you move it to
dhcp6_srv.cc? After that change suddenly the number of dependencies
for dhcp6 became much more complex. Keep in mind that while
ddns will be used in typical environment, we will eventually
need to address embedded/CPE market. It is realistic to expect
that the server will handle FQDN option, but will not do any
DDNS stuff. In fact, we should have something like
--disable-dhcp-ddns parameter in configure that would disable
all DDNS stuff (except handling client FQDN option).

You tried to make me include the libdns header in the lbdhcp++ but you now suggest that linking dhcp6_srv with libdhcp_ddns is wrong. I understand the motiviation but that would require encapsulating the NameChangeRequests? queue in the other class. If you really think that linking with libdhcp_ddns is a big issue, please confirm, I will file a ticket for it.

General question for dhcp6_srv modifications: what will the server
do if there is a client that sends 5 IA_NA options?

Each IA_NA is processed. Thus, at least 5 NameChangeRequests will be generated.

appendClientFqdn(): please make it clear whether it is fqdn
send by the client or the server's response

It is now clearer.

dhcp6_srv.cc
appendClientFqdn(): The first if seems wrong. Something
is also wrong with the tests that didn't pick it up.

createNameChangeRequests(): We will eventually support
a case when there is more than one address in a given
IA_NA. That is an uncommon, but valid case (e.g. during
renumbering events). The code will eventually have to do

  for (each IA_NA/IA_TA in the response) {
      for each (address in this IA) {
          generateNCR()
      }
  }

Please add an appropriate @todo in the code.

Added.

The method must get an extra parameter: add, update (and possibly
delete or skip). Right now you're generating ncr ADD for RENEWs and
you don't generate anything for RELEASEs.

No. There are two types of NameChangeRequest: for addition and for removal. The ones which remove DNS entries we also use for updating (by doing remove, add). Please have a look into renewIA_NA and releaseIA_NA.

createRemovalNameChangeRequest(): Third comment it wrong.
You can't create DHCID out of hostname. What you trying to
generate here will be used as AAAA, not DHCID.

It is not. I am converting hostname to the canonical wire format so as I can generate DHCID using it (together with FQDN) a few lines down.

run(): Please update the comment near call to
sendNameChangeRequests() to say that we call that method to
send NCRs. You mentioned that it's a stub in at least 3 places.
It will be tricky to update all those comments once we
implement actual NCR transmissions.

Arguably, someone could tell that saying that we send NCRs while we actually don't is also wrong. :-) But, I changed the comment. One or another is fine by me.

processClientFQDN(): The first if is technically correct,
but looks odd.

And, what is the problem? Returning existing empty pointer is faster than creating new empty pointer which will be copy constructed anyway.

Please add a @todo somewhere in the processClientFqdn(). Ideally
all uses of constants should be replaced by methods (e.g.
FQDN_GENERATED_PARTIAL_NAME with generateFqdnPartialName(question)),
but I don't insist on it. It can be done as part of the next ticket.

Constants are described in the header file.


Rest of the review will continue tomorrow.

The other chunk of comments will be addressed separately.

comment:8 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by marcin

Replying to tomek:

Many of the hacks done in Makefiles (like including d2/ncr_msg.cc in
dhcp6) are no longer necessary, because libdhcp_ddns is already merged
to master. Please merge master into trac3036 and clean up the
makefiles.

I merged master to trac3036 and cleaned up the Makefiles.

ncr_msg.cc|h

fromDUID(): Changes look ok, but there's one thing that requires a bit
though (or at least extra comment in the text). RFC4701, Section 3.5
says that FQDN should be in canonical format, as defined in RFC4034,
Section 6.2. That section talks about converting FQDN to lowercase. Do
we do that? If we do, we should make a brief comment about it and
point to specific place where this conversion happens. If we don't, we
should extend the code. I'm not saying that this is the best place. It
isn't. Probably somewhere in dns/labelsequence would be better (or in
places that use that class).

That is a good point. The isc::dns::Name class is capable of doing it but by default it is disabled. I made a change in the DHCPv6 srv code to force downcase.

ncr_unittests.cc
Please also test min (1), max (128) lengths of DUID when calculating
DHCID.

Added.

dhcp6/dhcp6.dox
Please udpate the copyright year to 2012-2013.

Please also update doc/devel/mainpage.dox with a reference to
dhcpv6DDNSIntegration.

Added.

Please update section dhcp6-std in doc/guide/bind10-guide.xml.

Second paragraph gives wrong RFC. It should be 4703, not 4307 (which
is about IKEv2).

Fixed.

Second paragraph in dhcpv6DDNSIntegration should be moved to
doc/guide/bind10-guide.xml to section dhcp6-limit. Also, the comment
about DNS Update not being supported needs to be removed there.

I suggest that we keep this in mind and do this change when we actually support DNS updates. Currently it is only stub implementation. The doxygen section regarding the DDNS will be evolving with almost every ticket that I am working on right now. I prefer not to spread the information everywhere until we really support DDNS.

bind10-dhcp6 => b10-dhcp6

"Depending on the message type, a DHCPv6 server may generate 0, 1 or 2
NameChangeRequests? during single message processing." Is this true?
How many NCRs will be generated if client sends REQUEST with 10
IA_NAs and server assigns them?

I clarified the text a bit. That was an example picked for simplicity.

The list "(no update, reverse only update, both reverse and
forward update)" is not complete. Also forward only is
possible. Although client may request both, server can be configured
to do only forward updates.

Ok. I was under the impression that in most cases server does reverse. And it can delegate the forward to the client but still does reverse. Isn't it the case?

The @todo starting with "The decision about not..." is confusing. I
don't understand what is left to do there. Do you mean writing the
house keeper code? There's no need to mention it here. I think it is
ok to remove the whole paragraph. If you want to keep it, please
rephrase it to make its point clearer.

ok. Removed.

"The DHCPv6 Client FQDN Option is comprises "NOS" flags" =>
"The DHCPv6 Client FQDN Option contains "NOS" flags

dhcp6_srv_unittest.cc
This code added around 700 lines of new code to the file. For hooks, I
did split them into separate file (see hooks_unittest.cc). You may
consider doing the same. It is ok if you prefer to do it as a separate
ticket, though.

I don't want to bloat this ticket anymore.

Why do you need N,O,S flags redefined? See also previous comment about
migrating Option6ClientFqdn::Flags enum to uint8_t.

I needed them because I had to initialize arrays of uint8_t in many tests. I wanted to use named values to initialize the bytes corresponding to flags field in those arrays. If I used enum some compilers would complain that I am using non-uint8_t value to initialize the pieces of array of uint8_t. That was a hack to avoid it. But, it is not a problem anymore because I changed the enum to uint8_t.

FqdnDhcpv6SrvTest class and its members should be documented.

I added some documentation.

testFqdn(): != 0 is not required in the flag_{n,s,o} initialization.

Actually, it doesn't need ''? true : false''.

serverAAAAUpdatePartialName(), serverAAAAUpdateNoName() and similar
should probably use the same constants that are used in the
Dhcpv6Srv::processClientFqdn(). It is ok to leave the code as it is
now, but you'll likely forget to remove/update it when
Dhcpv6Srv::processClientFqdn() is implemented using actual values.

Fixed.

createNameChangeRequests(): I like this test a lot and the fact that
it checks mulitple IA_NAs in a single message.

FqdnDhcpv6SrvTest.processSolicit(). Interesting test. What about a
case when during ADVERTISE generation server reuses old expired lease?
I think it should then delete old entries, but I haven't given it
enough thought, so I'm not 100% convinced. If we don't do it, client
may get ADVERTISE and then try to resolve it and see who (what domain
name) was using that address before.

It will delete old entries when the client actually gets the lease when sends REQUEST.

option6_client_fqdn_unittest.cc
N,O,S flags are defined here third time.

I explained that already.

Option6ClientFqdnTest.assignment, .copyConstruct: Please extend the
tests to copy empty name.

Added new tests.

Please add a test that will check that the code throws an exception
if:

  • any label is longer than 63 octets
  • the whole domain name is longer than 255 bytes

See RFC4704, section 4.2, first paragraph.

Added.

Also see my comment about ignoring MBZ field. The code should not
throw. RFC4704 says that the server must ignore those bits, not the
whole option.

That has been addressed already.

Finally, the code doesn't build on Ubuntu 13.04 x64.

You include d2/ncr_msg.cc when building dhcp6, but you do not include
$(BOTAN_INCLUDE) in AM_CPPFLAGS in src/bin/dhcp6/Makefile.am

The following fix makes the compilation possible, but it later fails linking.
I have not pursued it further as the libdhcp-ddns code on master makes
any fixes to the current linking issues obsolete.

diff --git a/src/bin/dhcp6/Makefile.am b/src/bin/dhcp6/Makefile.am
index 6f2c694..1f00c66 100644
--- a/src/bin/dhcp6/Makefile.am
+++ b/src/bin/dhcp6/Makefile.am
@@ -3,7 +3,7 @@ SUBDIRS = . tests
 AM_CPPFLAGS = -I$(top_srcdir)/src/lib -I$(top_builddir)/src/lib
 AM_CPPFLAGS += -I$(top_srcdir)/src/bin -I$(top_builddir)/src/bin
 AM_CPPFLAGS += -I$(top_srcdir)/src/lib/cc -I$(top_builddir)/src/lib/cc
-AM_CPPFLAGS += $(BOOST_INCLUDES)
+AM_CPPFLAGS += $(BOOST_INCLUDES) $(BOTAN_INCLUDES)
 
 AM_CXXFLAGS = $(B10_CXXFLAGS)
 if USE_CLANGPP

Hopefully, it compiles now.

We had a long discussion on bind10 chatroom today about using enums for
flags in FQDN option. The source of confusion comes from the fact that
the 'flag' word it is used in two contexts. The first one refers to the
whole bitfield called 'flags', which flag refers to only a single bit.
Both Tom and I would prefer to use uint8_t for both as more then one
flag is allowed to be set. And it seems wrong to do logical operations
on enums. However, if our arguments didn't convince you, please at
least rename the FLAG_N to BIT_N as this information is referred to as
'N bit' in RFC4704. Also, please use plural form (flags) when you refer
to the whole bitfield. The distinction would then be easier to understand.

Your ChangeLog? proposal is ok, but a bit long. Also, D2 is now officially
called b10-dhcp-ddns, so please use that name.

Yes. That will be changed.

I don't understand the point about too long entry. Anything shorter would be vague and would not reflect what the new code is actually doing. Plus, this is not the final implementation and I don't want to make impression that we are doing any DNS updates at this time. So the last sentence is required.

I have no more comments at this time. I would definitely like to see this
ticket again.

comment:9 Changed 7 years ago by marcin

  • Owner changed from marcin to tomek

I made the changes according to review comments. Please review it again.

comment:10 in reply to: ↑ 7 Changed 7 years ago by tomek

Replying to marcin:

I would avoid making option names too short. The mnemonic for option 39 is OPTION_CLIENT_FQDN and the full option name is ''DHCPv6 Option Client FQDN''. I wanted the class to reflect the actual option name. It precisely identifies the purpose of the class.

There are no other options with similar names. But that was only a suggestion. Let's keep the current name.

This work really should be split into 2 tickets: option class and then
follow-up ticket that uses that class. Keep that in mind when doing
its v4 counter-part.

In fact, it should be 3 tickets: implementation of the option, changes to allocation engine to support FQDNs, changes to the DHCPv6 server to process the option. Optionally, I could add another ticket to add support to store FQDN information in MySQL. But, this will be implemented together with the relevant changes for DHCPv4.

Good to know. I don't like large tickets, because besides doing the review is tedious, there's also another aspect. Technically valid comments are rejected on the grounds of making the ticket work too big or too bloated.
Anyway, it's good that the next work is done in smaller chunks.

option6_client_fqdn.h
FLAG_S, FLAG_O FLAG_N are defined as enums. They should be const
uint8_t as flags field can have more than one of those set. Enums are
used when only one of the possible values is valid.

I told you on the chatroom, that use of enums does certain things more clear from the API point of you. Functions like getFlag() and setFlag() require that there is a single ''bit'' specified (because you want to get a value of the bit N, or bit S or bit O etc.) Enums make it more clear to the caller that it is expected that only one of them is specified. Caller would be able to pass different values (e.g. 0x3) but it is not that easy because he would have to cast the value of 0x3 from int to enumeration type. This discourages invalid use of these two methods. This doesn't work like this when use uint8_t.

Technically, both solutions (using enums and not using enums) work so it is just a discussion what is more/less clear from the interface point of you and we seem to have different opinions. I decided to do the change to avoid confusion in the use of constructor which takes uint8_t value to accept the combinations of flags. I also extended the description of setFlag and getFlag functions to clarify that only specific uint8_t values should be used.

Thank you.

I don't like the Option6ClientFqdnImpl class much. I understand the
reasons, but similar result could be achieved with having just one
class and domain_name_ defined as private (or protected for testing
purposes). It is true that there is no need to include path to
dns/labelsequence.h when including option6_client_fqdn.h header, but
on the other hand you still need to link with dns library. So the
bottom line is that the Option6ClientFqdn comment in last paragraph
about changes being transparent is not really valid.

I disagree. The pimpl idiom is a common technique to separate the interface from the implementation. Not only it is common C++ programming technique, but also it is used in bind10 already in many places. I guess we both aren't happy with the fact that we need to link with libdns. If I do what you suggest, not only do I have to link with libdns but also I need to include a libdns header. Suppose, there are people who want to use libdhcp and make use of the !Option6ClientFqdn class. If they do, together with the option6_client_fqdn.h header they include a bunch of stuff from libdns. Who knows, maybe they even use functions from there to do some specific things. When we write the common libraries we should really avoid misuse of our classes.

Ok, I perhaps didn't express myself clearly. I understand that the dependency is needed (at least for now). I only commented on your statement that the future changes will be transparent. Regardless of the choice, they won't be. I didn't expect you to rewrite it with just one class.

Some of the methods in Option6ClientFqdn can be private (or at least
protected), e.g. packDomainName().

No. This function is used in dhcp6_srv.cc and thus must be public.

Ok, I missed that.

option6_client_fqdn.cc
Option6CXlientFqdnImpl and its methods should be documented.

I added documentation.

Thanks.

This class allows construction of partial or even empty domain names.
That is not really needed on the server side (Last paragraph in
section 4.2 of RFC4704: "Servers SHOULD send the complete fully
qualified domain name in Client FQDN options."). But I suppose it
is ok, as we will hopefully soon use those options in perfdhcp and
other projects.

I agree it is not needed on the server side. But, the libdhcp is not a server-only library. So, I don't really understand the comment.

It was just it - a comment, not a suggestion to do anything.

checkFlags() You can't throw if MBZ field is non-zero. See last
paragraph in section 4.1 of RFC4704. Server MUST ignore these
bits. (just bits, not the whole option). If you really want to do
something, printing a debug message is probably ok, though. But it
could be an attack vector to clog server logs.

Ok. I shouldn't be throwing when I received an option with MBZ bits set, but I should throw if I am attempting to send option with MBZ bits set. I made a relevant change. Depending on the constructor used the exception is thrown or not.

Good approach.

Assignment operator, Option6ClientFqdnImpl() constructor: What will
happen if you try to assign/copy an option without domain name? :)

Good catch. I added a check for this case. I also realized that the operator is not really used anywhere in the current code. So, technically I could remove the operator, but I left it, if it occurs to be needed in the future.

I have mixed feelings about assignment operators. We are mostly accessing objects by smart pointers to them. It is a frequent mistake to assign one shared pointer to another rather then assign objects they point to. Anyway, let's keep the operator.

Option6ClientFqdn::Option6ClientFqdn(first, last):
the data is kept twice in the object (once in Option.data_ and
second time in Option6ClientFqdnImpl). Is there a need for such
redundancy?

Well, isn't it what you have been doing for other options too? Isn't it what we do for all options?

The purpose of the specialized class is to parse the data and store them in the data structure which fits the format of the option. So, what is the actual problem?

Ok, we had a discussion about that in chatroom and the consensus was to keep the code as it is.

getFlag(): That check is weird. Flag enum has only 3 values defined:
1, 2 and 4. Any values in between are not correct. Will the compiler
even allow casting uint8_t into Flag enum?

Yes, I would allow casting from 0x3 to enum. Please refer to the C++ standard.

Ok, I feel now more educated. But that enum behavior is counter-intuitive. However, since we're not in the language standardization business yet, I'll accept your explanation for now ;-)

What is wrong will clearing all bits (flag=0)? Why do you throw an
exception? My understanding is that 000 sent by client is a valid
combination (client requests server to perform PTR only).

You may have misunderstood the purpose of this method. There are three bits that you want to set or get: N, O, S. Each is identified by the value (mask) which is used to set this bit in the flags field: 0x1, 0x2, 0x4. Only those values makes sense. Specifying 0x0 doesn't make sense because it doesn't specify which bit you want to get/set.

Ok.

toText(): please reformat toText() to return a single line response.
We have a debug code somewhere that prints out whole packets with
all options. It will be confusing if some options take up more than
one line (multi-line strings should be reserved for encapsulated
options)

Ok. I made a change to make it consistent. But, I am not very happy about the format of the text being printed. In my opinion it would be much easier to read the log if we made proper indentation. Sub-options could be more indented. We can have some offline discussion about it.

Printing out whole messages will never be pretty. If we ever need detailed information about messages stored, I think it would be useful to consider having an pcap writer.

alloc_engine.cc
Please implement Lease::setHostname(const bool fwd, const bool rev,

const string& name). That will help avoiding invalid

combinations (e.g. fwd=true, rev=true, name="") are not allowed.
Since the fields are public we can't really make sure of it, but
having such a simple method with set up best practice here.

I wish we followed this best practice everywhere in the code. I am not going to do it now, because it would bloat this ticket. This ticket is not really about changing how Allocation Engine manages internal data. New ticket: http://bind10.isc.org/ticket/3096

Another ticket is fine. Thanks.

option_definition.cc
I see many cosmetic changes related to maximum line length.
We have and agreemeing in bind10 chatroom that we typically try
to keep the code under 80 columns, but up to 100 columns is considered
ok. If you still use terminal that can't handle more than 80
columns, please at least do not add extra << to the code as
it (marginally) bloats the output. Exploit the preprocessor to
do the work for you:

cout << "text1"
        "text2" << end;

We should either work with all developers to document this informal agreement and make it formal or we should stick to 80.

My terminal is doing good and it can absorb more than 80 chars but if I open two windows in emacs, each 80 chars wide, the code gets wrapped.

It was intended as a joke. But I get your point. Nevertheless, the rule for having 80 chars was good for C code, when you had relatively short names and no namespaces or classes. With C++ 80 chars is not enough.

I will not expand it to over 80 lines because other developers don't really agree that it is allowed, and they also review my code.

Yes, I'll add it to my TODO list.

dhcp6_srv.h
I don't like the inclusion of d2/ncr_msg.h. Can you move it to
dhcp6_srv.cc? After that change suddenly the number of dependencies
for dhcp6 became much more complex. Keep in mind that while
ddns will be used in typical environment, we will eventually
need to address embedded/CPE market. It is realistic to expect
that the server will handle FQDN option, but will not do any
DDNS stuff. In fact, we should have something like
--disable-dhcp-ddns parameter in configure that would disable
all DDNS stuff (except handling client FQDN option).

You tried to make me include the libdns header in the lbdhcp++ but you now suggest that linking dhcp6_srv with libdhcp_ddns is wrong. I understand the motiviation but that would require encapsulating the NameChangeRequests? queue in the other class. If you really think that linking with libdhcp_ddns is a big issue, please confirm, I will file a ticket for it.

No, it is fine as it is. I wrote my comment before I realized that libdhcp_ddns is already merged to master. I see that the d2/ncr_msg.h header is gone from dhcp6_srv.h. That's even better.

The method must get an extra parameter: add, update (and possibly
delete or skip). Right now you're generating ncr ADD for RENEWs and
you don't generate anything for RELEASEs.

No. There are two types of NameChangeRequest: for addition and for removal. The ones which remove DNS entries we also use for updating (by doing remove, add). Please have a look into renewIA_NA and releaseIA_NA.

So during normal operation when client renews his address (and server is configured to do updates during RENEW) there will be a short periods of time when the name and address won't be resolving correctly?

processClientFQDN(): The first if is technically correct,
but looks odd.

And, what is the problem? Returning existing empty pointer is faster than creating new empty pointer which will be copy constructed anyway.

There's no problem. I sometimes make comments and they are just that - comments.

Please add a @todo somewhere in the processClientFqdn(). Ideally
all uses of constants should be replaced by methods (e.g.
FQDN_GENERATED_PARTIAL_NAME with generateFqdnPartialName(question)),
but I don't insist on it. It can be done as part of the next ticket.

Constants are described in the header file.

FQDN_GENERATE_PARTIAL_NAME is not defined in a header file, it is in dhcp6_srv.cc:112. Anyway, it doesn't make sense to do any clean-ups here as that part of the code will go away once #3034 is merged. I merely added @todo near the temporary constants definitions.

comment:11 in reply to: ↑ 8 ; follow-up: Changed 7 years ago by tomek

  • Owner changed from tomek to marcin

Replying to marcin:

Replying to tomek:

Many of the hacks done in Makefiles (like including d2/ncr_msg.cc in
dhcp6) are no longer necessary, because libdhcp_ddns is already merged
to master. Please merge master into trac3036 and clean up the
makefiles.

I merged master to trac3036 and cleaned up the Makefiles.

Thanks.

ncr_msg.cc|h

fromDUID(): Changes look ok, but there's one thing that requires a bit
though (or at least extra comment in the text). RFC4701, Section 3.5
says that FQDN should be in canonical format, as defined in RFC4034,
Section 6.2. That section talks about converting FQDN to lowercase. Do
we do that? If we do, we should make a brief comment about it and
point to specific place where this conversion happens. If we don't, we
should extend the code. I'm not saying that this is the best place. It
isn't. Probably somewhere in dns/labelsequence would be better (or in
places that use that class).

That is a good point. The isc::dns::Name class is capable of doing it but by default it is disabled. I made a change in the DHCPv6 srv code to force downcase.

Are there any unit-tests that check that?

Second paragraph in dhcpv6DDNSIntegration should be moved to
doc/guide/bind10-guide.xml to section dhcp6-limit. Also, the comment
about DNS Update not being supported needs to be removed there.

I suggest that we keep this in mind and do this change when we actually support DNS updates. Currently it is only stub implementation. The doxygen section regarding the DDNS will be evolving with almost every ticket that I am working on right now. I prefer not to spread the information everywhere until we really support DDNS.

Ok.

The list "(no update, reverse only update, both reverse and
forward update)" is not complete. Also forward only is
possible. Although client may request both, server can be configured
to do only forward updates.

Ok. I was under the impression that in most cases server does reverse. And it can delegate the forward to the client but still does reverse. Isn't it the case?

That's the typical case, I agree. However, in some networks the opposite (server doing only AAAA) is also valid. Sysadmins may configure the server to do only forward updates, e.g. because they don't care about reverse.

"The DHCPv6 Client FQDN Option is comprises "NOS" flags" =>
"The DHCPv6 Client FQDN Option contains "NOS" flags

dhcp6_srv_unittest.cc
This code added around 700 lines of new code to the file. For hooks, I
did split them into separate file (see hooks_unittest.cc). You may
consider doing the same. It is ok if you prefer to do it as a separate
ticket, though.

I don't want to bloat this ticket anymore.

Then please create a separate ticket for that (or at least add a @todo in the test code).

testFqdn(): != 0 is not required in the flag_{n,s,o} initialization.

Actually, it doesn't need ''? true : false''.

True.

Finally, the code doesn't build on Ubuntu 13.04 x64.

Hopefully, it compiles now.

Nope. Still failed compilation. But the fix was easy. I pushed my changes. It compiles now.
Please pull.

I don't understand the point about too long entry. Anything shorter would be vague and would not reflect what the new code is actually doing. Plus, this is not the final implementation and I don't want to make impression that we are doing any DNS updates at this time. So the last sentence is required.

Again, it was only a comment. Let it keep that way.

I have one more minor comment, which probably doesn't require any code changes, just a bit of explanation.
Option6ClientFqdn::packDomainName() creates labels object from dereferenced domain_name_. What if the name is empty?

comment:12 in reply to: ↑ 11 Changed 7 years ago by marcin

  • Owner changed from marcin to tomek

Replying to tomek:

ncr_msg.cc|h

fromDUID(): Changes look ok, but there's one thing that requires a bit
though (or at least extra comment in the text). RFC4701, Section 3.5
says that FQDN should be in canonical format, as defined in RFC4034,
Section 6.2. That section talks about converting FQDN to lowercase. Do
we do that? If we do, we should make a brief comment about it and
point to specific place where this conversion happens. If we don't, we
should extend the code. I'm not saying that this is the best place. It
isn't. Probably somewhere in dns/labelsequence would be better (or in
places that use that class).

That is a good point. The isc::dns::Name class is capable of doing it but by default it is disabled. I made a change in the DHCPv6 srv code to force downcase.

Are there any unit-tests that check that?

I now leverage existing unit tests to cover that.

I don't want to bloat this ticket anymore.

Then please create a separate ticket for that (or at least add a @todo in the test code).

Created #3101.

Finally, the code doesn't build on Ubuntu 13.04 x64.

Hopefully, it compiles now.

Nope. Still failed compilation. But the fix was easy. I pushed my changes. It compiles now.
Please pull.

Thanks for this!

I don't understand the point about too long entry. Anything shorter would be vague and would not reflect what the new code is actually doing. Plus, this is not the final implementation and I don't want to make impression that we are doing any DNS updates at this time. So the last sentence is required.

Again, it was only a comment. Let it keep that way.

I have one more minor comment, which probably doesn't require any code changes, just a bit of explanation.
Option6ClientFqdn::packDomainName() creates labels object from dereferenced domain_name_. What if the name is empty?

That's a great point.

Actually, it requires changes to the code. I now check the domain_name pointer and if it is NULL, I do nothing.

comment:13 Changed 7 years ago by tomek

  • Owner changed from tomek to marcin

Ok, the last changes look good. Please merge.

comment:14 Changed 7 years ago by marcin

  • Owner changed from marcin to tomek

I ran the tests on the build bot and found two issues:

  • valgrind tests failed for dhcp_ddns because Botan library was not initialized. I used the cryptolink to initialize the Botan library.
  • cppcheck reported lack of check for assignment to self. This check was not really required so I suppressed it.

Please review two recent commits.

comment:15 Changed 7 years ago by tomek

  • Owner changed from tomek to marcin

I reviewed the last 2 commits (aad0b789f4f82e50e219debc7d6d52675d7c85e0 and 586809ac7369b85efe135f62b6281e141d88bfdb). The code looks ok, but I have minor comments. They do not require any code changes, though (perhaps adding @todo). As far as I understand cryptolink, it is a generic abstraction layer around a crypto library. We use botan for now, but other lib could be supported later. So we use generic API to initalize the library, but use specific implementation to do SHA calculations.

Also, documentation to cryptolink (src/lib/cryptolink/cryptolink.h:101) mentions that initialized() method can be used. But I think it is ok to keep the code as it is now.

comment:16 Changed 7 years ago by marcin

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged with commit 209f3964b9f12afbf36f3fa6b62964e03049ec6e

Note: See TracTickets for help on using tickets.