Opened 3 years ago

Closed 14 months ago

Last modified 14 months ago

#3357 closed enhancement (duplicate)

Merge DHCPv4-over-DHCPv6 into Kea repo

Reported by: tomek Owned by: contributor
Priority: low Milestone: Kea1.1
Component: dhcp Version: git
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: 7
Total Hours: 7 Internal?: no

Description (last modified by fdupont)

Prof. Yong Cui's team (Qi Sun, Cong Liu) developed DHCPv4-over-DHCPv6 solution that is based on Kea code that was released 9 months ago (bind10-1.1.0). The goal of this ticket is to improve the code, fill in missing gaps and then merge it into Kea master.

Marcin and Tomek will do a detailed review and will put it here.

The code is available here: https://github.com/gnocuil/DHCPv4oDHCPv6

ISC comment: assigning this ticket to 0.9-alpha with medium quality, even though it is not needed for 0.9-alpha release, currently planned for end of March (not needed in a sense that we can still go with the alpha release even if this ticket is still open). It is not possible right now to determine how long it will take to merge the code. If we somehow manage to do it before 0.9-alpha, great. If not, we'll merge it after and include the code in 0.9-beta.

Subtickets

Change History (27)

comment:1 Changed 3 years ago by tomek

Here are initial the steps that need to be completed. Marcin and I will do detailed code review in the next couple days:

Issues:

  1. The code should be based on latest master from Kea repo (see http://bind10.isc.org/wiki/GitGuidelines)
  2. subnet-id needs to be passed between v4 and v6.
  3. open one socket and use it, not open/close one per packet.
  4. need unit-tests.
  5. we need configuration (DHCPv4 only, DHCPv6 only, DHCPv4o6). The default for 4o6 should be disabled, so it will work only if administrator explicitly enables it.
  6. receive6to4/4to6 should be generic v4-v6 communication (Marcin/Tomek? will take care of this)
  7. Update documentation: User's guide and developer's guide. This is really minor thing, so we can deal with it last.

comment:2 Changed 3 years ago by tomek

  • Description modified (diff)

comment:3 Changed 3 years ago by stephen

  • Milestone changed from DHCP-Kea0.9-alpha to DHCP-Kea0.9-beta

comment:4 Changed 3 years ago by marcin

I've sent quite extensive review to the implementers. The review follows.

Overall, I think that the approach is good. I realize that it will require more work. In particular, configuration knobs for DHCPv4 over DHCPv6. The lack of configuration knobs doesn't however preclude the code from being included as "experimental" in the Kea tree. The one configuration knob that MUST be present is the one that enables/disables the DHCPv4 over DHCPv6 function. By default, the server should ignore DHCPv4-query packets so as the inclusion of this experimental implementation doesn't affect the operation of the "normal" DHCPv6 server, unless someone specifically enables it. So this knob should be on our todo list.

The code you provided works but we will need to work on refinement of its structure, given the fact that this implementation will evolve (and will require more data to be passed back and forth), and we would like the communication between processes to be more generic, so as it is easier to implement similar protocols in the future.

A couple of suggestions below.

Socket for Inter process communication (IPC)

I think it is a good idea to use UNIX sockets to pass the packets back and forth between the Kea4 and Kea6 processes. I think we will stick to this. However, the IPC can be possibly used for other types of traffic (not only DHCPv4 over DHCPv6 traffic) so the code should be more generic.

Note that for any type of communication between two processes there will be always a pair of UNIX sockets needed. Therefore, the code that opens sockets will be common for any type of communication.

What I want to propose here is creation of the simple C++ class (let's call it BaseIPC). This class should be used by both servers Kea4 and Kea6 to create a UNIX socket used to communicate with other entity (perhaps it could have an openSocket function). Constructor of this class could take source and destination FILENAMEs or addresses as arguments.

So, instead of duplicating the code that opens socket (in IfaceMgr::openSockets4 and IfaceMgr::openSockets6), there could be something similar to this:

BaseIPC ipc(FILENAME1);
int sock4 = ipc.openSocket();

and in the other server:

BaseIPC ipc(FILENAME2);
int sock6 = ipc.openSocket();

Communication between processes

Two processes communicating over the UNIX socket may typically exchange any type of information. The DHCPv4 over DHCPv6 protocol implementation may certainly need to exchange some more information beyond the DHCPv4 packet decapsulated from the DHCPv4-query. Some examples of extra information that needs to be carried from DHCPv6 server to the DHCPv4 server:

  • flags field from the DHCPv6 message
  • IPv6 enabled interface or IPv6 subnet associated with the DHCPv6 packet received, as the DHCPv4 may need to make allocation decisions based on the v6 subnet or interface data, because the received v4 packet lacks this kind of information (as it has been received over IPv6 network and no IPv4 may be in place).

Conversely, the DHCPv4 server may need to include some additional data beyond DHCPv4 Offer/Reply? too. For example, instead of keeping a state of the DHCPv4-query packet in the map (map4o6) we could choose to send some information (e.g. id of the v6 enabled interface on which we received the DHCPv4-query) to the DHCPv4 server. The DHCPv4 would always send it back to the DHCPv6 server. Perhaps, some other information could be sent by the DHCPv6 server to DHCPv4 server and be reflected back when the DHCPv4 packet processing is finished. This would eliminate the need for keeping DHCPv6 packet state in the map.

Elimination of the map is actually an important point, because there are certain issues that arise when the map is in use. For example, if the packets are not responded by the DHCPv4 server, the map grows as the new packets arrive, effectively leading to the leak of memory, unless purged.

Having said that, if we take into account that the extra data have to be exchanged between the servers it makes me think that we need some expandable API for the communication between processes. Therefore we should probably remove DHCPv4-over-DHCPv6 specific functions like send4to6 from the IfaceMgr? and put them into the new class derived from the BaseIPC (see below).

Specific communication between DHCPv4 and DHCPv6

So far, the proposed BaseIPC class implements the functionality to open a UNIX socket. At this point, it should probably be extended to be able to send and receive raw data. Note, that IfaceMgr::send4to6 and Dhcpv6Srv::processDHCPv4Query in your implementation are rather agnostic to the Pkt4 and Pkt6 objects, except for the initial step to retrieve the raw buffer from these objects. The rest of the code (with some modifications), could be moved from these functions to the new function BaseIPC::send(const std::vector<uint8_t>& buf). This would be a generic function used to opaque data between processes.

Similarly, there could be a BaseIPC::receive function to receive raw data on the socket.

Having a generic class opening sockets and sending/receiving data through a unix socket, it will be possible to create a new classes (derived from BaseIPC) DHCPv6to4IPC and DHCPv4to6IPC which would customize the communication specifically for the sake of DHCPv4 over DHCPv6 protocol implementation. For example the class DHCPv6to4IPC could have a new "send" method:

DHCPv6to4IPC::send(const Pkt4Ptr& pkt4, const Metadata& metadata) {
   create buffer;
   write length of the pkt4 to the buffer;
   write pkt4 as binary;
   write length of the metadata;
   write metadata as binary;

   BaseIPC::send(buffer);
}

The data on the other end would be reconstructed by the DHCPv4to6IPC::receive().

Please note that by abstracting out the BaseIPC and derived classes, if we ever decide to change the format of the serialized data from binary to something else (e.g. text based) the change is limited to the DHCPv6to4IPC and DHCPv4to6IPC.

Taking server specific code out of the libdhcp++

The libdhcp++ is now meant to be a low level library for operation on sockets but it is not really dedicated to solve server-side problems. The inter process communication, inclusion of the metadata like subnet information is more a server side problem.

Therefore, the code that is preparing these data to send to the other process should be taken out of libdhcp++. What I am proposing here is that IPC sockets are not opened in the IfaceMgr?, that IfaceMgr::send and IfaceMgr::receive don't call send4to6, receive6to4 etc.

The servers (either dhcpv6_srv or dhcpv4_srv) should rather call DHCPvXtoYIPC::send and DHCPvXtoYIPC::receive directly (or via something in libdhcpsrv).
Also, please note that there should be a way to disable/enable the DHCPv4 over DHCPv6 function. If this is disabled (and it should actually be the default setting), the server knowing about it, would not open a UNIX socket and would simply ignore the DHCPv4-query packets. Note, that current code doesn't really allow this behavior, because the logic that opens UNIX socket is buried under IfaceMgr::openSockets4 and IfaceMgr::openSocket6 so the UNIX socket is always opened.

Coding style

Each code that is committed to Kea needs to conform to the Kea coding guidelines: http://kea.isc.org/wiki/CodingGuidelines. This is a part of our review process to check that.

Additional comments

The IfaceMgr? already opens unix socket for IPC, there is no need to open a new socket to send the packet (see processDHCPv4Query). Simply use the socket that is bound. That could be solved by the new classes I proposed above.

The processDHCPv4Response calls these functions:

  • copyDefaultOptions
  • appendDefaultOptions
  • appendRequestedOptions

to append a bunch of options to the DHCPv4-response packet. This shouldn't be the case. The client should not get the v6 client id, and other configured options, such as NTP servers etc. Note that the client has got his configuration already in the initial DHCPv6 exchange with the DHCPv6 server.

Every code must be unit tested. This code doesn't include unit tests.

The latest Kea code (precisely IfaceMgr?) allows for registration of the "external" sockets and associated callback function. The callback function is invoked when the external socket has the data. The server code may provide a callback function which will call DHCPvXoYIPC::receive to receive the data over the UNIX socket. Your code should be updated to the latest version of Kea.

comment:5 Changed 3 years ago by tomek

  • Milestone changed from DHCP-Kea0.9 to DHCP-Kea-proposed

comment:6 Changed 3 years ago by tomek

  • Milestone changed from Kea-proposed to Kea0.9
  • Priority changed from medium to low

comment:7 follow-ups: Changed 3 years ago by marcin

  • Add Hours to Ticket changed from 0 to 7
  • Owner set to marcin
  • Status changed from new to reviewing
  • Total Hours changed from 0 to 7

I have completed another round of review of the DHCPv4 over DHCPv6 implementation located here: https://github.com/gnocuil/kea (4o6 branch)

src/bin/dhcp4/dhcp4_srv.cc
Constructor: The same comment as for dhcp6_srv.... The unix socket should not be opened until indicated by the configuration. Why open a socket that will not be used if the 4o6 is disabled?

run(): There is an issue that you override the received DHCP packet (other than from 4o6 server) with the 4o6 packet here:

        try {
            query = receivePacket(timeout);
        } catch (const std::exception& e) {
            LOG_ERROR(dhcp4_logger, DHCP4_PACKET_RECEIVE_FAIL).arg(e.what());
        }

        // 4o6
        if (ipc_ && !ipc_->empty()) {
            query = ipc_->pop()->getPkt4();

            //set Pkt4's localAddr according to U flag in Pkt6's transid field
            ipc_->current()->setPkt4LocalAddr();
            
            if (!CfgMgr::instance().dhcp4o6Enabled()) {
                query = Pkt4Ptr();
            }
        }

See the corresponding comment for dhcp6_srv.

General question: do you already make use of the ancillary data? It seems like the only data you're really using is the U flag from the transaction id field? Correct?

src/bin/dhcp6/dhcp6_srv.cc
Constructor: does it make sense to open IPC socket in the constructor when you don't yet know if the IPC will be used at all? The socket should only be opened if the server configuration indicates that DHCP4o6 function is to be enabled.

run(): This piece of the function will break the server's ability to respond to the regular DHCPv6 traffic when the DHCPv4-over-DHCPv6 function is enabled:

        try {
            query = receivePacket(timeout);
        } catch (const std::exception& e) {
            LOG_ERROR(dhcp6_logger, DHCP6_PACKET_RECEIVE_FAIL).arg(e.what());
        }
        
        // 4o6
        if (ipc_ && !ipc_->empty()) {
            query = ipc_->pop()->getPkt6();
            if (!CfgMgr::instance().dhcp4o6Enabled()) {
                query = Pkt6Ptr();
            }
        }

Let's assume that the server receives a Solicit message from some client. The receivePacket() function will read the packet from the interface. The next thing that the code does is override the Solicit packet (stored in the query) with the DHCPV4_QUERY packet from the tip of the queue. The Solicit packet is then discarded. Further processing is performed on the 4o6 packet. In order to overcome this problem we probably need a balancing logic that would pick packet from receivePacket on the first pass over the loop and then the 4o6 packet on the second pass of the loop and so on. Obviously, if there is nothing in the queue, take what is available.

src/bin/dhcp6/dhcp6_srv.h
processDHCPv4Query: documentation: is this really a stub function? It has a logic to handle packets received from the clients and from the DHCPv4 server. The documentation of this function should be updated to reflect what it is doing.

src/lib/dhcp/dhcp4o6_ipc.cc
The open() function as it is defined here should be moved to BaseIPC class implementation (See my comments below for BaseIPC).

sendPkt4o6: The error string here:

    if (!pkt4o6) {
        isc_throw(DHCP4o6IPCSendError, "Invalid Pkt4o6");
    }

should be improved. It is not invalid Pkt4o6. It is NULL packet. Also, I'd rather avoid awkward naming like ''Pkt4o6'' in error string that the user can see. Perhaps ''4o6 packet'' would be better?

What will happen if the server will receive a lot of packets but none of the will be received on time? In other words, should there be any limitation put on the number of elements in the queue?

processDHCPv4Query: In this check:

    if (!ipc_ || !CfgMgr::instance().dhcp4o6Enabled())
        return Pkt6Ptr();

There should not be a case that the dhcp4o6Enabled() returns true and ipc_ is NULL. If this is the case it is a programmatic error and should be reported as error. The way it is coded now, such an error is silently ignored and the only indication that something is wrong is that there will be no response from the server to the client.

appendRequestedOptions should not be added here because client must not request any options through the DHCPv4_QUERY message.

src/lib/dhcp/dhcp4o6_ipc.h
Since classes defined here strictly serve the purpose of the communication between two DHCP servers the better location for them would be src/lib/dhcpsrv, not srv/lib/dhcp.

A couple of nits:

  • This header file contains some typos and editorial errors in the doxygen documentation. Also, all comments should start with a capital letter.
  • Some functions have undocumented parameters: e.g. isCurrent.
  • returned values in the return statements should be in parens according to Kea coding style.

I don't understand the purpose of the DHCP4o6IPC::instance_. I think that the server should create its own instance of the DHCP4o6IPC and remove it when the server exists or is reconfigured, e.g. when the new configuration disables the use of DHCPv4-over-DHCPv6.

I am not sure if there is any reason for the callback function to be static. Using boost::bind you can make it non-static plus you could even get rid of ir and use the recvPkt4o6 directly. So because callback is static you're introducing the static instance_ member?

I don't understand the description of isCurrent functions.

Instead of making getLocalFilename and getRemoteFilename pure virtual you could just put these functions into the BaseIPC class to return the file names passed to the constructor of the BaseIPC class (see my comment below referring to ipc.h). having said tha, the filenames should be rather stored as constants in the dhcp4_srv and dhcp6_srv .cc files.

enable/disable: I think that the names of these methods are confusing. They in fact don't enable or disable DHCP4o6 function but remove the socket from the IfaceMgr?. By enabling/disabling the DHCP4o6 function I'd understand that the DHCP server's configuration was changed to disable or enable processing of DHCP4-QUERY packets.

src/lib/dhcp/pkt4o6.cc
src/lib/dhcp/pkt4o6.h
IANA has already assigned the appropriate numbers, OPTION_DHCPv4_MSG, DHCPv4_QUERY and DHCPV4_RESPONSE should be updated. Also, they should be moved to the dhcp6.h where we have other codes of this kind defined.

Pkt4o6Ptr should be preceded by a short comment.

There is no doxygen description of Pkt4o6 class.

src/lib/dhcp/tests/Makefile.am
src/lib/dhcp/tests/dhcp4o6_ipc_unittest.cc
Test fixture classes should be documented along with their methods.

Copyright dates should be updated.

src/lib/dhcp/tests/pkt4o6_unittest.cc
Test fixture classes should be documented along with their methods.

Copyright dates should be updated.

src/lib/dhcpsrv/cfgmgr.cc
Changes should be unittested.

src/lib/util/Makefile.am
src/lib/util/ipc.h
The methods in this header file are sufficiently large to have implementation in the .cc file rather than .h file. It is generally ok to put implementation of one-liners, e.g. accesor methods into the header file but otherwise I'd like to see ipc.cc that contains implementation of the other methods.

Documentation: Either a BaseIPC class or the particular methods that belong to this class should have more comprehensive doxygen documentation. The BaseIPC class is documented as follows: ''IPC tool based on UNIX domain socket''. There is instantly a question: what is the ''IPC''. It should rather be ''Inter Process Communication''. Examples of other useful information for such a class are:

  • It may (maybe should) be used as a base class for future classes implementing inter process communication.
  • It should be noted that the instance of this class (or derived) should be opened by each process.
  • What type of errors may occur and when
  • Are there any limitations
  • Is there any particular message format to be used between processes.
  • etc.

I think that the IPC object has to be configured when it is created. In other words, the constructor should have parameters and receive all required parameters. These parameters should remain unchanged throughout the life of the IPC object. So, can we just specify local_name and remote_name as parameters of the constructor? And then it would be used like this:

BaseIPC ipc("some-local-file-name", "some-remote-file-name");
try {
    ipc.open();
} catch (const IPCError& ex) {
    // do something with it.
}

The open() method could do all sorts of things that currently openSocket, bindSocket, setRemote does. These methods could be either be combined in the open() method or they could be pushed to protected scope and called by open().

This change would also imply a reduction of the number of exception types defined.

Similarly, the closeSocket() could be replaced by close().

In such case there will be no need for the open() function in derived classes.

The reason for the changes I am proposing is that there will never be the case that someone will want to open socket socket, but not bind it etc. So, why to split all those?

This class will rather not be used directly but through the derived classes which already have virtual functions. So, the destructor if the BaseIPC should be virtual.

setRemote: The documentation says it sets the remote file name while it really opens a socket, which is not mentioned at all. I guess setRemote should not be exposed publicly and the filenames should be configured via constructor. If it is to be exposed publicly I'd throw an exception if the socket is open.

Why is this:

        strcpy(&remote_addr_.sun_path[1], remote_name.c_str());

instead of

        strcpy(remote_addr_.sun_path, remote_name.c_str());

?

I think I am not clear why the filename is copied to the second byte of sun_path not to the first (at index 0)?

src/lib/util/tests/ipc_unittest.cc
Invalid copyright date.

comment:8 in reply to: ↑ 7 Changed 3 years ago by gnocuil

Thank you very much for the careful review. We are working on the issues and some replies are inline below.

Replying to marcin:

I have completed another round of review of the DHCPv4 over DHCPv6 implementation located here: https://github.com/gnocuil/kea (4o6 branch)

src/bin/dhcp4/dhcp4_srv.cc
Constructor: The same comment as for dhcp6_srv.... The unix socket should not be opened until indicated by the configuration. Why open a socket that will not be used if the 4o6 is disabled?

[Cong] Will try to fix it.

run(): There is an issue that you override the received DHCP packet (other than from 4o6 server) with the 4o6 packet here:

        try {
            query = receivePacket(timeout);
        } catch (const std::exception& e) {
            LOG_ERROR(dhcp4_logger, DHCP4_PACKET_RECEIVE_FAIL).arg(e.what());
        }

        // 4o6
        if (ipc_ && !ipc_->empty()) {
            query = ipc_->pop()->getPkt4();

            //set Pkt4's localAddr according to U flag in Pkt6's transid field
            ipc_->current()->setPkt4LocalAddr();
            
            if (!CfgMgr::instance().dhcp4o6Enabled()) {
                query = Pkt4Ptr();
            }
        }

See the corresponding comment for dhcp6_srv.

General question: do you already make use of the ancillary data? It seems like the only data you're really using is the U flag from the transaction id field? Correct?

[Cong] Yes, currently only U flag is used. Other data from the original DHCPv6 packet may also be used, e.g. Lightweight 4over6 will use source address of the client (proposed in draft-fsc-dhc-dhcp4o6-saddr-opt, in a DHCPv6 option).

src/bin/dhcp6/dhcp6_srv.cc
Constructor: does it make sense to open IPC socket in the constructor when you don't yet know if the IPC will be used at all? The socket should only be opened if the server configuration indicates that DHCP4o6 function is to be enabled.

[Cong] Will fix it.

run(): This piece of the function will break the server's ability to respond to the regular DHCPv6 traffic when the DHCPv4-over-DHCPv6 function is enabled:

        try {
            query = receivePacket(timeout);
        } catch (const std::exception& e) {
            LOG_ERROR(dhcp6_logger, DHCP6_PACKET_RECEIVE_FAIL).arg(e.what());
        }
        
        // 4o6
        if (ipc_ && !ipc_->empty()) {
            query = ipc_->pop()->getPkt6();
            if (!CfgMgr::instance().dhcp4o6Enabled()) {
                query = Pkt6Ptr();
            }
        }

Let's assume that the server receives a Solicit message from some client. The receivePacket() function will read the packet from the interface. The next thing that the code does is override the Solicit packet (stored in the query) with the DHCPV4_QUERY packet from the tip of the queue. The Solicit packet is then discarded. Further processing is performed on the 4o6 packet. In order to overcome this problem we probably need a balancing logic that would pick packet from receivePacket on the first pass over the loop and then the 4o6 packet on the second pass of the loop and so on. Obviously, if there is nothing in the queue, take what is available.

[Cong] For the current code, the queue is added with a 4o6 packet only when receivePacket() -> receive6() calls the callback function, and the queue length is no more than 1. In this case the receivePacket() will return null so query will not be override. When query is a received Solicit packet, the empty() will always be true.
Will make it clearer.

src/bin/dhcp6/dhcp6_srv.h
processDHCPv4Query: documentation: is this really a stub function? It has a logic to handle packets received from the clients and from the DHCPv4 server. The documentation of this function should be updated to reflect what it is doing.

[Cong] Will fix it.

src/lib/dhcp/dhcp4o6_ipc.cc
The open() function as it is defined here should be moved to BaseIPC class implementation (See my comments below for BaseIPC).

sendPkt4o6: The error string here:

    if (!pkt4o6) {
        isc_throw(DHCP4o6IPCSendError, "Invalid Pkt4o6");
    }

should be improved. It is not invalid Pkt4o6. It is NULL packet. Also, I'd rather avoid awkward naming like ''Pkt4o6'' in error string that the user can see. Perhaps ''4o6 packet'' would be better?

[Cong] Sure, will modify it.

What will happen if the server will receive a lot of packets but none of the will be received on time? In other words, should there be any limitation put on the number of elements in the queue?

[Cong] Currently the queue length is no more than 1 (not a really queue), every enqueued packet dequeues immediately. So if the receiver server is busy, the unix socket buffer size will block the sender.

processDHCPv4Query: In this check:

    if (!ipc_ || !CfgMgr::instance().dhcp4o6Enabled())
        return Pkt6Ptr();

There should not be a case that the dhcp4o6Enabled() returns true and ipc_ is NULL. If this is the case it is a programmatic error and should be reported as error. The way it is coded now, such an error is silently ignored and the only indication that something is wrong is that there will be no response from the server to the client.

appendRequestedOptions should not be added here because client must not request any options through the DHCPv4_QUERY message.

[Cong] draft-fsc-dhc-dhcp4o6-saddr-opt proposes to use ORO in DHCP4o6. The requested options are also new defined options just for DHCP4o6 and they may be processed by appendRequestedOptions. draft-ietf-dhc-dhcpv4-over-dhcpv6 doesn't mention ORO in DHCP4o6, so I think it's not a MUST NOT (not quite sure).

src/lib/dhcp/dhcp4o6_ipc.h
Since classes defined here strictly serve the purpose of the communication between two DHCP servers the better location for them would be src/lib/dhcpsrv, not srv/lib/dhcp.

A couple of nits:

  • This header file contains some typos and editorial errors in the doxygen documentation. Also, all comments should start with a capital letter.
  • Some functions have undocumented parameters: e.g. isCurrent.
  • returned values in the return statements should be in parens according to Kea coding style.

I don't understand the purpose of the DHCP4o6IPC::instance_. I think that the server should create its own instance of the DHCP4o6IPC and remove it when the server exists or is reconfigured, e.g. when the new configuration disables the use of DHCPv4-over-DHCPv6.

[Cong] It's called by the callback() and DHCP4o6IPC::enable()/disable(). The callback() will be removed. The enable()/disable() are called by CfgMgr? and I'll try to improve it.

I am not sure if there is any reason for the callback function to be static. Using boost::bind you can make it non-static plus you could even get rid of ir and use the recvPkt4o6 directly. So because callback is static you're introducing the static instance_ member?

[Cong] Will use boost:bind instead.

I don't understand the description of isCurrent functions.

[Cong] When dhcp4_srv receives a 4o6 packet from dhcp6_srv, it process the query as usual and results a Pkt4 reply (rsp). Then isCurrent() is used to check if current query is from a 4o6 packet. If true, a 4o6 reply will be generated and send back to dhcp6_srv.
On the dhcp6_srv side, isCurrent() is use to check whether the query comes from a 4o6 packet (from dhcp4_srv) or not (from client). Will consider rename it.

Instead of making getLocalFilename and getRemoteFilename pure virtual you could just put these functions into the BaseIPC class to return the file names passed to the constructor of the BaseIPC class (see my comment below referring to ipc.h). having said tha, the filenames should be rather stored as constants in the dhcp4_srv and dhcp6_srv .cc files.

enable/disable: I think that the names of these methods are confusing. They in fact don't enable or disable DHCP4o6 function but remove the socket from the IfaceMgr?. By enabling/disabling the DHCP4o6 function I'd understand that the DHCP server's configuration was changed to disable or enable processing of DHCP4-QUERY packets.

[Cong] Will move enable/disable directly to dhcp4/6_srv.cc and trigger add/remove the socket

src/lib/dhcp/pkt4o6.cc
src/lib/dhcp/pkt4o6.h
IANA has already assigned the appropriate numbers, OPTION_DHCPv4_MSG, DHCPv4_QUERY and DHCPV4_RESPONSE should be updated. Also, they should be moved to the dhcp6.h where we have other codes of this kind defined.

Pkt4o6Ptr should be preceded by a short comment.

There is no doxygen description of Pkt4o6 class.

src/lib/dhcp/tests/Makefile.am
src/lib/dhcp/tests/dhcp4o6_ipc_unittest.cc
Test fixture classes should be documented along with their methods.

Copyright dates should be updated.

src/lib/dhcp/tests/pkt4o6_unittest.cc
Test fixture classes should be documented along with their methods.

Copyright dates should be updated.

src/lib/dhcpsrv/cfgmgr.cc
Changes should be unittested.

src/lib/util/Makefile.am
src/lib/util/ipc.h
The methods in this header file are sufficiently large to have implementation in the .cc file rather than .h file. It is generally ok to put implementation of one-liners, e.g. accesor methods into the header file but otherwise I'd like to see ipc.cc that contains implementation of the other methods.

Documentation: Either a BaseIPC class or the particular methods that belong to this class should have more comprehensive doxygen documentation. The BaseIPC class is documented as follows: ''IPC tool based on UNIX domain socket''. There is instantly a question: what is the ''IPC''. It should rather be ''Inter Process Communication''. Examples of other useful information for such a class are:

  • It may (maybe should) be used as a base class for future classes implementing inter process communication.
  • It should be noted that the instance of this class (or derived) should be opened by each process.
  • What type of errors may occur and when
  • Are there any limitations
  • Is there any particular message format to be used between processes.
  • etc.

I think that the IPC object has to be configured when it is created. In other words, the constructor should have parameters and receive all required parameters. These parameters should remain unchanged throughout the life of the IPC object. So, can we just specify local_name and remote_name as parameters of the constructor? And then it would be used like this:

BaseIPC ipc("some-local-file-name", "some-remote-file-name");
try {
    ipc.open();
} catch (const IPCError& ex) {
    // do something with it.
}

The open() method could do all sorts of things that currently openSocket, bindSocket, setRemote does. These methods could be either be combined in the open() method or they could be pushed to protected scope and called by open().

This change would also imply a reduction of the number of exception types defined.

Similarly, the closeSocket() could be replaced by close().

In such case there will be no need for the open() function in derived classes.

The reason for the changes I am proposing is that there will never be the case that someone will want to open socket socket, but not bind it etc. So, why to split all those?

This class will rather not be used directly but through the derived classes which already have virtual functions. So, the destructor if the BaseIPC should be virtual.

[Cong] Will follow your suggestion for IPC.

setRemote: The documentation says it sets the remote file name while it really opens a socket, which is not mentioned at all. I guess setRemote should not be exposed publicly and the filenames should be configured via constructor. If it is to be exposed publicly I'd throw an exception if the socket is open.

[Cong] It just copies the filename, the remote filename is only used in sendto(). Will remove it by the constructor.

Why is this:

        strcpy(&remote_addr_.sun_path[1], remote_name.c_str());

instead of

        strcpy(remote_addr_.sun_path, remote_name.c_str());

?

I think I am not clear why the filename is copied to the second byte of sun_path not to the first (at index 0)?

[Cong] It's the abstract socket address, if starts from index 0 then the filename needs to be a filesystem path name. Reference - http://man7.org/linux/man-pages/man7/unix.7.html

src/lib/util/tests/ipc_unittest.cc
Invalid copyright date.

comment:9 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by gnocuil

Replying to marcin:

We've made some update and trying to resolve issues in comment#7.
The main changes are:

  1. Update the option code types to IANA values, and move them to dhcp6.h.
  2. Update the documented draft name to RFC 7341.
  3. Enable/disable 4o6 function: remove all changes in Cfgmgr, add enable4o6()/disable4o6() functions in dhcp4_srv and dhcp6_srv. These functions are called by config_parser.
  4. BaseIPC and DHCP4o6IPC constructor takes 2 filenames. Change BaseIPC::bindSocket() and BaseIPC::setRemoteFilename() to protected. BaseIPC has a open() calls these 2 functions. DHCP4o6IPC constructor directly calls BaseIPC::open().
  5. Split dhcp6_srv/processDHCPv4Query function into 2 functions, a new processDHCPv4Response() processes DHCPv4 response from dhcp4_srv.
  6. Remove dhcp4o6_ipc/isCurrent() for dhcp6_srv.
  7. Update some documentation nits: copyright date, capital letter, etc.
  8. Splitting ipc.h into ipc.h/ipc.cc will cause a reference error because dhcp4o6_ipc.h references to util/ipc.h, but the defination is in ipc.cc and not included. I'm not sure how to modify Makefile.am to solve this, so ipc.h is unchanged.

make[4]: Entering directory `/home/gnocuil/kea/tests/tools/perfdhcp'
Making all in .
make[5]: Entering directory `/home/gnocuil/kea/tests/tools/perfdhcp'

CXXLD perfdhcp

../../../src/lib/dhcp/.libs/libkea-dhcp++.so: undefined reference to `isc::util::BaseIPC::openSocket()'
../../../src/lib/dhcp/.libs/libkea-dhcp++.so: undefined reference to `isc::util::BaseIPC::bindSocket(std::string const&)'
collect2: error: ld returned 1 exit status
make[5]: * [perfdhcp] Error 1

Also the code is at ​https://github.com/gnocuil/kea (4o6 branch).

comment:10 in reply to: ↑ 9 ; follow-up: Changed 3 years ago by fdupont

Replying to gnocuil:

  1. Update the documented draft name to RFC 7341.

=> I am working on the DHCP4 version (so one day you should get a client :-) so I check: the number of the to be published RFC was assigned to 7341?

comment:11 in reply to: ↑ 10 Changed 3 years ago by marcin

Replying to fdupont:

Replying to gnocuil:

  1. Update the documented draft name to RFC 7341.

=> I am working on the DHCP4 version (so one day you should get a client :-) so I check: the number of the to be published RFC was assigned to 7341?

Correct, the RFC number is 7341.

comment:12 follow-up: Changed 3 years ago by fdupont

  • Version set to git

For Marcin?

I am working on DHCP4 so I'd like to build a compatible (in term of message format and naming) communication channel. Do you have a concern with sending the DHCPV4-QUERY and DHCPV4-RESPONSE not decapsulated (so without specific handling of the multicast flag)? For additional infos, I think the receiving interface name and the first usable (not unspecified or link-local) relay address
(or unspecified if it can't be found or not relayed) must be added. To finish I object against the AF_UNIX socket idea, I really prefer a pair of sockets to ::1 with succeeding ports (no default so it will have to be explicitly configured).

comment:13 in reply to: ↑ 12 ; follow-up: Changed 3 years ago by marcin

Replying to fdupont:

For Marcin?

I am working on DHCP4 so I'd like to build a compatible (in term of message format and naming) communication channel. Do you have a concern with sending the DHCPV4-QUERY and DHCPV4-RESPONSE not decapsulated (so without specific handling of the multicast flag)? For additional infos, I think the receiving interface name and the first usable (not unspecified or link-local) relay address
(or unspecified if it can't be found or not relayed) must be added. To finish I object against the AF_UNIX socket idea, I really prefer a pair of sockets to ::1 with succeeding ports (no default so it will have to be explicitly configured).

Francis,

I am not sure I understand your point about the messages not being decapsulated. For the DHCPV4-QUERY, does it mean that you want to pass the DHCPv6 message to the DHCPv4 server for decapsulation? What does it mean for DHCPV4-RESPONSE?

In Kea, we would like to process the DHCPV4-QUERY on a DHCPv6 server side and pass the extracted DHCPv4 message along with ancillary data to the DHCPv4 server. Note that the DHCPv6 server may need to do some basic filtering of the packets received. For example, do a basic check if the DHCPV4-QUERY message contains a DHCPv4 message at all. Also, we want to do the processing of the flags field in the DHCPV6-QUERY message by the DHCPv6 server because this field may be in the future extended to support other flags and the future flags may be more relevant to the DHCPv6 server, rather than DHCPv4 server.

I am hoping it addresses some of your questions, unless I completely misunderstood.

Regarding the additional data, I agree that the interface name is essential. So as the relay address. But, I think it is equally important to guarantee that the set of additional data is easily extensible as I can imagine that various people will come up with different use cases for this protocol which will require extra information be exchanged between the DHCPv4 and DHCPv6 servers. Also, the server should not fall over when it receives unknown type of information and simply make use of the information it understands.

Regarding the Unix socket issue... I understand this is a limitation on Windows. Was there anything else you were thinking might be a problem here?

comment:14 Changed 3 years ago by marcin

There are still outstanding issues in the code and the coding style as well as documentation could be improved. But, I realize that sending review back and forth doesn't get us closer to the completion of this task.

The code is based on the version of Kea from April and since then Kea has gone through significant changes. Hence, the merge is not straightforward. I will need to go through the diff bit by bit and apply the relevant changes to the new code base.

I will be doing it as a background task and will create a trac3357 branch in the main Kea repo for this work.

When this is done, I will ask implementers to make a clone of our Kea repo hosted on github: https://github.com/isc-projects/kea and future work should be conducted on the cloned repo.

comment:15 in reply to: ↑ 13 Changed 3 years ago by fdupont

Replying to marcin:

Replying to fdupont:

For Marcin?

I am working on DHCP4 so I'd like to build a compatible (in term of message format and naming) communication channel. Do you have a concern with sending the DHCPV4-QUERY and DHCPV4-RESPONSE not decapsulated (so without specific handling of the multicast flag)? For additional infos, I think the receiving interface name and the first usable (not unspecified or link-local) relay address
(or unspecified if it can't be found or not relayed) must be added. To finish I object against the AF_UNIX socket idea, I really prefer a pair of sockets to ::1 with succeeding ports (no default so it will have to be explicitly configured).

Francis,

I am not sure I understand your point about the messages not being decapsulated. For the DHCPV4-QUERY, does it mean that you want to pass the DHCPv6 message to the DHCPv4 server for decapsulation?

=> yes, in fact I believe now the whole DHCPv6 packet should be passed (so the DHCPv6 role is just to recognised DHCPV4-QUERY including when relayed).

What does it mean for DHCPV4-RESPONSE

=> the issue is not the same but if the DHCPv4 server builds the complete DHCPv6 message (DHCPV4-RESPONSE or RELAY_REPL chain encapsulating a DHCPV4-RESPONSE) the DHCPv6 server no longer needs to keep the initial message to build the response.

In Kea, we would like to process the DHCPV4-QUERY on a DHCPv6 server side and pass the extracted DHCPv4 message along with ancillary data to the DHCPv4 server.

=> I am afraid either the ancillary data is big or the initial message is kept.

Note that the DHCPv6 server may need to do some basic filtering of the packets received. For example, do a basic check if the DHCPV4-QUERY message contains a DHCPv4 message at all.

=> it doesn't matter: this must be done by the last entity which has the whole DHCPV4-QUERY.

In fact I can rephrase my question: does the DHCPv4 server have the DHCPv6 message handling code? As you know ISC DHCP is not modular so for it the answer is yes.

Regarding the additional data, I agree that the interface name is essential. So as the relay address. But, I think it is equally important to guarantee that the set of additional data is easily extensible as I can imagine that various people will come up with different use cases for this protocol which will require extra information be exchanged between the DHCPv4 and DHCPv6 servers. Also, the server should not fall over when it receives unknown type of information and simply make use of the information it understands.

=> auto-descriptive formats are too heavy...

Regarding the Unix socket issue... I understand this is a limitation on Windows. Was there anything else you were thinking might be a problem here?

=> where to put the socket name in the file system? Don't forget to remove the in ode. etc... In conclusion if you don't need to transmit file descriptors over it (and even in this case as the WIN32 API provides this function too) it is a bad idea to use named UNIX sockets.

comment:16 Changed 3 years ago by tomek

  • Milestone changed from Kea0.9 to Kea0.9.1

We haven't managed to finish the review before 0.9 release, so carrying this over to 0.9.1.

comment:17 Changed 3 years ago by fdupont

Finished the ISC DHCP work on DHCPv4 over DHCPv6:

  • client (full support with 2 processes, cf the documentation)
  • relay (never used/tested but the basic support is in two added lines to relay (vs. dropped as unknown) DHCPv4-query/response
  • server (full support with2 processes and client localisation based on DHCPv4 and IPv6 elements, cf the doc)

BTW the server communication is between the 2 server processes on ::1 port (v4) et port+1 (v6, the port number is a common command line argument) with interface name (16 octets eventually padded with '\0', client source address (IPv6 -> 16 octets) and the full DHCPv6 message, in both ways (i.e., v6 -> v4 and v4 -> v6).

comment:18 Changed 3 years ago by marcin

I created a new branch in the main Kea repository: trac3357 based on the tip of master. and I merged the code from https://github.com/gnocuil/kea.git (branch 4o6) to this branch.

I am now walking through the specific changes and fixing or cleaning it up where appropriate. So far I made the following changes:

  • Renamed !BaseIPC to UnixSocket because it better describes what the class is doing. Note that IPC may also be implemented in terms of something different than unix socket so calling it a base IPC may be an abuse of this term.
  • Cleaned up the UnixSocket class, fixed descriptions and unit tests. I also don't use the abstract unix sockets because it is not portable.

In the next phase I'll review the IPC classes derived from UnixSocket class.

comment:19 follow-up: Changed 3 years ago by marcin

I have a handful of comments about the !Pkt4o6 class and I am hoping to get some help from the authors of the patch as they don't appear to be cosmetic changes.

But, before I share the comments. I'd like to ask that we follow the new process when working on this feature. That is, ISC has a Kea repository on github: https://github.com/isc-projects/kea. We try to keep it up-to-date with the main repository. The idea is that contributors clone this repository and work on the clone. This cloned repository (apart from the master branch) also contain s the trac3357 branch which I am using for the merge of your DHCPv4 over DHCPv6 implementation. So, please make a clone of the repository on github, checkout branch trac3357. You'll see some changes I applied to the code. On top of these changes, please address the following comments.

I think it is a good idea to encapsulate the packets exchanged between the DHCPv6 and DHCPv4 server in the new class: !Pkt4o6. I also like the idea of having a JSON string passed there containing parameters.

My take on how this class should be used is depicted below:

     --Pkt6---------------                -Pkt4o6-------------
     |                   |                |                  |
     | DHCPv4-query/resp |                |  decapsulated    |
     |                   |                | DHCPv4 message   |
     ---------------------                | + ancillary data |
                                          --------------------
client <-----------------> DHCPv6 server  <-----------------> DHCPv4 server
                              
                              

The role of the DHCPv6 server is to receive the Pkt6 packets, including normal DHCPv6 messages (e.g. Solicit, Request etc.) but also new DHCPv6 messages like DHCPv4-query from the clients.

The DHCPv6 server checks the message type. For all standard messages, the server processes them in a regular way. For DHCPv4-query messages the server decapsulates them and extracts the original DHCPv4 message before sending it to the DHCPv4 server via the unix socket. Since the DHCPv6 server decapsulates the original message, it should not send the DHCPv6 message to the DHCPv4 server. Instead it should provide some additional information as ancillary data and stick it after the DHCPv4 message passed over the unix socket.

So, I propose the following:

  • Make Pkt4o6 class derive from Pkt4 message (class Pkt4o6 : public Pkt4)
  • Create the static Pkt4o6::fromPkt6(Pkt6Ptr) as a factory creating the !Pkt4o6Ptr. [ This would be executed on the DHCPv6 server side ]
  • Extend the !Pkt4o6::pack so as it stores the message in the buffer (using parent's classe's pack()), plus it stores the ancillary data. [ This would be executed on the DHCPv4 server side.
  • Extend the Pkt4o6::unpack so as it can read the ancillary data and parse the DHCPv4 message (using Pkt4::unpack). [ This would be executed on the DHCPv4 server side ]
  • Create a !Pkt4o6::toPkt6 function which would use the DHCPv4 message encapsulated in the !Pkt4o6 and the ancillary data to create a DHCPv6-reply message. [ This would be executed on the DHCPv6 server side ]

So I think that the ancillary data should contain the information required to reconstruct the DHCPv6 message (DHCPv6-response). It should also contain the interface name on which the message was received and the flags field extracted from the DHCPv6 message.

comment:20 in reply to: ↑ 19 Changed 2 years ago by gnocuil

Replying to marcin:
I created a pull request on github. Please find the updates there and see if it's suitable.

As Pkt6 is not passed between DHCP4/6 servers, currently DHCPv6 relay is not supported because it requires adding relay_info_ and all D6O_INTERFACE_ID into ancillary data.

comment:21 Changed 2 years ago by marcin

Thanks for doing it. But, it appears that you haven't updated the unit tests for pkt4o6 and they don't compile. This needs to be updated as we need running unit tests to accept this code.

comment:22 Changed 2 years ago by marcin

  • Owner changed from marcin to contributor

comment:23 Changed 2 years ago by hschempf

  • Milestone changed from Kea0.9.1 to Kea0.9.2

comment:24 Changed 2 years ago by hschempf

  • Milestone changed from Kea0.9.2 to Kea1.1

comment:25 Changed 14 months ago by fdupont

  • Description modified (diff)

IMHO this was replaced by the Hackathon94 work so can be closed.

comment:26 Changed 14 months ago by fdupont

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

Got agreement on jabber: closing.

comment:27 Changed 14 months ago by tomek

For new tickets that replaced this work, see a list at the bottom of Dhcp4o6Design.

Note: See TracTickets for help on using tickets.