Opened 5 years ago

Closed 5 years ago

#3689 closed enhancement (fixed)

Hostname in v6 reservations should be used

Reported by: tomek Owned by: tmark
Priority: high Milestone: Kea0.9.1
Component: dhcp6 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: 16
Total Hours: 0 Internal?: no

Description

It is possible to define a hostname in the host reservations for DHCPv6. It will be accepted by the parser, but it will not be used. It should be.

This is a follow-up to the HR v6 tickets.

Subtickets

Change History (13)

comment:1 Changed 5 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea0.9.1

comment:2 Changed 5 years ago by marcin

  • Priority changed from medium to high

comment:3 Changed 5 years ago by tomek

  • Owner set to tomek
  • Status changed from new to assigned

comment:4 Changed 5 years ago by tomek

The code on trac3689 obeys hostnames specified in host reservation. There's even a single unit-test to verify that.

Sadly, I ran out of time to finish it up. Feel free to test it, but I expect many things may not work.

Grey areas:

  • reservation specifies partial hostname
  • reservation specifies hostname, but client didn't sent FQDN

comment:5 Changed 5 years ago by tmark

  • Owner changed from tomek to tmark

comment:6 Changed 5 years ago by tmark

  • Dhcpv6Srv::processClientFqdn() now serves as the central point for determining both the lease hostname and the FQDN. Since the two values are tightly coupled and influenced by host reservations when they exist this seem to be the best place to manage both. The only place beyond this method where these values are changed is when the FQDN (and thus lease hostname) are replaced by a generated name. The rules the function follows are:
        /// - If there is no Client FQDN and no reserved hostname then there
        /// will no be DNS updates and the lease hostname will be empty.
        ///
        /// - If there is no Client FQDN but there is a reserved hostname then
        /// there will be no DNS updates and the lease hostname will be equal
        /// to reserved hostname.
        ///
        /// - If there is a Client FQDN and a reserved hostname, then both the
        /// FQDN and lease hostname will be equal to reserved hostname with
        /// the qualifying suffix appended.
        ///
        /// - If there is a Client FQDN but no reserved hostname then both the
        /// FQDN and lease hostname will be equal to the name provided in the
        /// client FQDN adjusted according the the DhcpDdns configuration
        /// parameters (e.g.replace-client-name, qualifying-suffix...).
    

This should address the issues of concern Tomek expressed above. This allowed similiar
logic to removed from both assignIA_NA and extendIA_NA.

  • Additional changes to Dhcpv6Srv consisted of replacing the duid and subnet parameters

with a context parameter on several methods and adding the context parmater to others.

  • Note that contrary to the ClientContext6 commentary which states that there is only

one context per Client request, Dhcp6Srv actually instantiates a local context within
each of these methods:

assignIA_NA()
assignIA_PD()
extendIA_NA()
extendIA_PD()

All of these methods are now passed a context created within the higher level process<Type> method (e.g. processSolicit, processRequest...) These local contexts
are now seeded with data from the original context. This is a bit clunky but it trying to safely reuse the original client seemed risky without deeper analysis which is beyond the time available.

  • Redundant calls to selectSubnet and findReservation have been eliminated.
  • Removed logic which called findReservation from AllocateEngine::allocateLeases6(). The only code using this "convenience" was unit testing. Rather than leave it as potential hazard, I removed it and altered the test code accordingly. This also keeps it symmetrical with DHCPv4 implementation.
  • Unit tests were added but more are likely needed, and undoubtedly the new tests could be more efficiently done.
  • There is an inconsistency between assignLeases() and extendLeases() in their handling

of a missing duid. I have marked this with a todo as not only do the two methods differ, the code cannot be reached assuming sanityCheck() is called first and this is case.

  • Marcin asked that I consider refactoring the Dhcpv6Srv class to use an exchange class

similiar to what he has done with Dhcpv4Exchange and Dhcpv4Srv as part of #3688. While I do not disagree the proximity to code freeze dictates otherwise as this would entail thorough analysis to be done properly.

Ticket is ready for review.

Last edited 5 years ago by tmark (previous) (diff)

comment:7 Changed 5 years ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from assigned to reviewing

comment:8 Changed 5 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:9 Changed 5 years ago by marcin

  • Owner changed from marcin to tmark

Reviewed the commit 6fc6f278bea3f1c46e21e32f0ced107d63f719fa

I appreciate that you have taken care of this ticket. I know it is hard to jump on someone's work and defend it in the review. :-) Overall, it is good that you improved efficiency of the code by removing the redundant calls. I also think that there is a lot of refactoring that we should do to make this code maintainable. But, for the time being it looks ok.

This change should include the user guide update, similar to what has been done for #3688. Maybe I missed it somewhere?

Also, please propose a ChangeLog entry.

src/bin/dhcp6/dhcp6_srv.cc
assignLeases: It seems I have missed it for the v4, but the error message DHCP4_SUBNET_SELECTION_FAILED may need to be renamed to something like DHCP4_ASSIGN_LEASE_NO_SUBNET or something similar, given that we don't really do selectSubnet at this point. If you concur, perhaps we should submit the ticket for 0.9.2 release to improve the messages in both v4 and v6?

It seems to me that the check for the NULL duid should rather be performed in the createContext but given that there are todos for this all over the place, we can probably leave it as a future improvement.

processClientFqdn: If there is no FQDN you assign the hostname from the reservation (if any) to the context. There are two questions I have:
1) Should we do the same for the v4? I don't think we do it currently. I may have misunderstood your question on jabber when you asked if we always put the hostname to the lease database. I meant that we do when the client sent FQDN, even if the forward and reverse updates are disabled.
2) Should the hostname be qualified with the qualifying-suffix when there is no FQDN and we use the reserved hostname?

assignIA_NA: instead of this

if (!orig_ctx.subnet_) {
...
}

you maybe intended to do this:

if (!subnet) {
...
}

Also, there are some typos in the scope after this condition.

When creating a new context could you just use the copy constructor and then explicitly set the values that differ between the orig_ctx and the ctx? I think it would be clearer as to what has changed between the two or what new fields are being set.

Could callout_handle be set for the original context in the createContext method?

I really dislike the mixed use of ClientContext6. Part of it is initialized in the createContext. Then, the whole thing is copied to the new context to add new parameters specific to the lease assignment to the particular IAs. Maybe we should have a base context that holds the specific information about the client, subnet etc. There could be a per-IA context which derives from the base context and captures all additional information. Do you think this makes sense? Should we open a ticket for this?

extendIA_NA: The same comment as above about use of copy constructor for creating a new context.

I suggest to add the comment prior to calling renewLeases6(ctx, false) what the "false" is for?

src/bin/dhcp6/dhcp6_srv.h
I wonder if the extractClientId method shouldn't rather belong to the Pkt6 class. I don't insist on it though.

createContext: It would be better if the documentation for this method specified which parameters of the context it does set, vs the rest of the parameters which it doesn't. The "possibly other parameters" doesn't sound like the documentation for the function. :-)

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
All new tests should be implemented using the Dhcp6Client, which avoids the direct calls to processRequest and similar functions. It also does things like append client id, server id and so on.

src/bin/dhcp6/tests/fqdn_unittest.cc
The hybrid configuration of the server using the "configure" method and later "createHost6" method doesn't seem to be doing what expected. Each of those methods performs a commit. The second commit replaces the previously committed configuration, meaning that the whole configuration for the DDNS and the subnets is discarded. It seems to work only because the createHost6 also provides the subnet configuration. I suggest that the host configuration is included in the configuration string, rather than using the createHost6. I'd even suggest that the createHost6 is tossed as it is going to cause mistakes like this in the future.

Shouldn't you call verifyNameChangeRequest after checking the d2 manager queue size at the end of the hostnameReservationSuffix, hostnameReservationNoSuffix and hostnameReservationDisabled?

Also, do you need so lengthy configurations for the DDNS? In particular, when the D2 is disabled, wouldn't it be sufficient to just specify the qualifying-suffix and the enable-updates?

There should be some tests for the Renew and Rebind messages when there is a reservation for the hostname. In particular, if I change the reservation and the client has a lease with the particular hostname, the newly reserved hostname should be handed out during the Renew.

src/lib/dhcpsrv/alloc_engine.h
It is strange that there is a new boolean parameter for renewLeases6, which seems to be always set to false by the server code. It is even more interesting that this parameter defaults to true. :-) Shouldn't this just be removed and assumed that the reservation is always initialized in the dhcp6_srv?

src/lib/dhcpsrv/tests/alloc_engine_utils.h
Typo in the findReservation documentation: "by the engine" instead of "bg the engine".

comment:10 Changed 5 years ago by marcin

Oh, and I fixed some nits and pushed to the repo. Please pull.

comment:11 Changed 5 years ago by tmark

  • Add Hours to Ticket changed from 0 to 16
  • Owner changed from tmark to marcin

Replying to marcin:

Reviewed the commit 6fc6f278bea3f1c46e21e32f0ced107d63f719fa

I appreciate that you have taken care of this ticket. I know it is hard to jump on someone's work and defend it in the review. :-) Overall, it is good that you improved efficiency of the code by removing the redundant calls. I also think that there is a lot of refactoring that we should do to make this code maintainable. But, for the time being it looks ok.

This change should include the user guide update, similar to what has been done for #3688. Maybe I missed it somewhere?

Sigh. Done. I will kill Tomek.

Also, please propose a ChangeLog entry.

xxx.    [func]      tomek,tmark
    DHCPv6 server supports static reservations of the hostnames
    for the clients.
    (Trac #3689 git$ TDB)

src/bin/dhcp6/dhcp6_srv.cc
assignLeases: It seems I have missed it for the v4, but the error message DHCP4_SUBNET_SELECTION_FAILED may need to be renamed to something like DHCP4_ASSIGN_LEASE_NO_SUBNET or something similar, given that we don't really do selectSubnet at this point. If you concur, perhaps we should submit the ticket for 0.9.2 release to improve the messages in both v4 and v6?

Sure a small ticket for this is fine.

It seems to me that the check for the NULL duid should rather be performed in the createContext but given that there are todos for this all over the place, we can probably leave it as a future improvement.

This one should get a ticket for future work.

processClientFqdn: If there is no FQDN you assign the hostname from the reservation (if any) to the context. There are two questions I have:
1) Should we do the same for the v4? I don't think we do it currently. I may have misunderstood your question on jabber when you asked if we always put the hostname to the lease database. I meant that we do when the client sent FQDN, even if the forward and reverse updates are disabled.

The way tomek had structured the code, the intent did seem to be to set it to the reservation hostname, in the absence of an FQDN. I think this makes sense and for consistencyDHCPv4 ought to do the same thing. For reasons stated before, code structure and tight coupling of FQDN and lease hostname made processClientFqdn a pretty logical place to do it.

2) Should the hostname be qualified with the qualifying-suffix when there is no FQDN and we use the reserved hostname?

No, I don't think we should. In this case we aren't using it as an FQDN so we are apparently not interested in DNS. I don't have a strong opinion on this just a gut feel.

assignIA_NA: instead of this

if (!orig_ctx.subnet_) {
...
}

you maybe intended to do this:

if (!subnet) {
...
}

Yes, thanks. I started to remove the convenience values but in the end left them. Looks like I missed this spot.

Also, there are some typos in the scope after this condition.

"Creatasse" isn't Polish? Got it.

When creating a new context could you just use the copy constructor and then explicitly set the values that differ between the orig_ctx and the ctx? I think it would be clearer as to what has changed between the two or what new fields are being set.

Could callout_handle be set for the original context in the createContext method?

I really dislike the mixed use of ClientContext6. Part of it is initialized in the createContext. Then, the whole thing is copied to the new context to add new parameters specific to the lease assignment to the particular IAs. Maybe we should have a base context that holds the specific information about the client, subnet etc. There could be a per-IA context which derives from the base context and captures all additional information. Do you think this makes sense? Should we open a ticket for this?

extendIA_NA: The same comment as above about use of copy constructor for creating a new context.

Regarding all of the above, I agree but I think we should create a specific ticket to clean it up. To do it properly will take some careful thought (i.e. design).

I suggest to add the comment prior to calling renewLeases6(ctx, false) what the "false" is for?

Actually this was a slightly bigger issue that I missed. The flag tells the renewLeases6() not to call findRervation(). In keeping with allocateLeases6(), I have removed this logic as well as the parameter. It was only used as true forunit tests and I think it is somewhat cross-purposing to make allocate engine also able to/repsonsible for doing reservation lookup.

src/bin/dhcp6/dhcp6_srv.h
I wonder if the extractClientId method shouldn't rather belong to the Pkt6 class. I don't insist on it though.

Probably, but I don't what to do this now.

createContext: It would be better if the documentation for this method specified which parameters of the context it does set, vs the rest of the parameters which it doesn't. The "possibly other parameters" doesn't sound like the documentation for the function. :-)

Added commentary. Shame on tomek... lol

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
All new tests should be implemented using the Dhcp6Client, which avoids the direct calls to processRequest and similar functions. It also does things like append client id, server id and so on.

The two tests I added to dhcp_srv_unittests.cc have been removed, as well as createHost6(). I have moved the tests into a new file, host_unittest.cc and the tests are now Dhcp6Client based.

src/bin/dhcp6/tests/fqdn_unittest.cc
The hybrid configuration of the server using the "configure" method and later "createHost6" method doesn't seem to be doing what expected. Each of those methods performs a commit. The second commit replaces the previously committed configuration, meaning that the whole configuration for the DDNS and the subnets is discarded. It seems to work only because the createHost6 also provides the subnet configuration. I suggest that the host configuration is included in the configuration string, rather than using the createHost6. I'd even suggest that the createHost6 is tossed as it is going to cause mistakes like this in the future.

I have reworked the tests in fqdn_unittest.c to use only the configure() method. This took quite a bit of doing. I had to add a method, setSubnetAndPool(), in order for lower level test methods, which are based on the subnet_ and pool_ members to succeed. This is a bit of hack, but it will have to do for now. If/when Dhcp6Client is extended to support DDNSing this whole file could be reworked.

Shouldn't you call verifyNameChangeRequest after checking the d2 manager queue size at the end of the hostnameReservationSuffix, hostnameReservationNoSuffix and hostnameReservationDisabled?

I have added calls to verifyNameChangeRequest to the first two tests. The last test will produce no NCRs as the D2 isn't enabled, and thus has no queue size.

Also, do you need so lengthy configurations for the DDNS? In particular, when the D2 is disabled, wouldn't it be sufficient to just specify the qualifying-suffix and the enable-updates?

I trimmed them.

There should be some tests for the Renew and Rebind messages when there is a reservation for the hostname. In particular, if I change the reservation and the client has a lease with the particular hostname, the newly reserved hostname should be handed out during the Renew.

Added to host_unittest.c

src/lib/dhcpsrv/alloc_engine.h
It is strange that there is a new boolean parameter for renewLeases6, which seems to be always set to false by the server code. It is even more interesting that this parameter defaults to true. :-) Shouldn't this just be removed and assumed that the reservation is always initialized in the dhcp6_srv?

Yes, this is covered above.

src/lib/dhcpsrv/tests/alloc_engine_utils.h
Typo in the findReservation documentation: "by the engine" instead of "bg the engine".

Got it.

Ready for review.

comment:12 Changed 5 years ago by marcin

  • Owner changed from marcin to tmark

Reviewed commit 2d290633dbfd4a5c5cf3f7f0e6a914822e6577ea

All changes look good to me. I fixed some typos and pushed to the branch. So, please pull.

Can you please open tickets for the issues discussed above for which we concluded that there should be new tickets?

comment:13 Changed 5 years ago by tmark

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

Changes merged with git c13c824d9948f7e3f71a65ed43798f3b5c14042c
Added ChangeLog entry #918

I have created three follow up tickets:

#3773 new defect Cleanup "missing DUID" logic in Dhcpv6Srv
#3774 new defect Clean up Dhcp6Srv handling/use of context class instances
#3775 new defect FQDN unit test for host reservations should use Dhcp6Client test class

Ticket is closed. Note this also closes #3708

Note: See TracTickets for help on using tickets.