Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#3982 closed enhancement (complete)

Move v6 lease to declined state (DECLINE support)

Reported by: tomek Owned by: tomek
Priority: high Milestone: Kea1.0-beta
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: 0
Total Hours: 0 Internal?: no

Description

Once #3965 is done, we need to implement support for processing incoming DECLINE message, i.e. put the v6 lease into Declined state. See DeclineDesign for details.

Subtickets

Change History (8)

comment:1 Changed 4 years ago by tomek

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

comment:2 Changed 4 years ago by tomek

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

The code is now ready for review. Proposed ChangeLog?:

XX.	[func]		tomek
	DECLINE message in DHCPv6 is now supported. The server is able
	to receive it, check its correctness and move the lease to
	DECLINED state. Currently there is no way to recover the lease
	before 'decline-probation-period' time.
	(Trac #3982, git tbd)

Note that the code in principle is able to handle multiple addresses in a single decline. However, there are no tests to check that for couple reasons. First, the RFC does not say what the status code should be if one of them is declined successfully while the other is not. Second, even single address Decline are very uncommon scenarios. Having a duplicate two addresses being detected at the same time would be extremely uncommon. Third, we do not support multiple addresses per IA in Solicit or Request responses, so there's no way a client handled by Kea would get multiple addresses for the same IA.

comment:3 Changed 4 years ago by sar

  • Owner changed from UnAssigned to sar

comment:4 follow-up: Changed 4 years ago by sar

  • Owner changed from sar to tomek

dhcp6_messages.mes

I fixed some typo level items please do a pull and see if they are acceptable.

As with the v4 version I would have all of the messages start with the same substring as I think that makes it easier to find the interesting values, but this is a minor item and you don't need to address it if you don't agree. Something like:
Client %1 tried to decline address %2, <rest of message>

dhcp6_srv.h

The description for processDecline() should be updated to at least not say it is a stub function and describe the return value.

The description for declineIA should include a description of what it returns.

dhcp6_srv.cc

I've made a couple of changes to the comments in declineLeases() to update them from the cut and paste of release.

And similar for declineIA - but you need to check to see if the comments are still correct.

Line 2678: shouldn't this be something like opt->second instead of ia->getOption(D6O_IAADDR)

Line 2739: the IAIDs appear to be swapped between the message (msg IAID, lease IAID) and the code (lease IAID, msg IAID) - or perhaps I misunderstand what the message is trying to say in which case perhaps it should be rewritten to make it clearer which IAID is from the message and which is form the lease.

Line 2758 & 2761 - the extra return is redundant and I would probably only go with one of them, but it's not a big deal either way.

The declineIA function uses both setStatusCode() and ia_rsp->addOption() to include status options. Is this intentional? or should it only be using setStatusCode()?

In declineLease should we use generateName() to construct the statistics name? As with v4 should the stats be cleared on stats removal? and should there be a total vs a subnet level counter?

Shouldn't the code to dis-associate the information from the lease be grouped into a function in the lease files?

decline_unittest.cc

acquire_lease() doesn't appear to be written or used. Is it going to be in the future or should it be removed?

Can we add a test for a decline request with no addresses? or IAs? (To test out the code that we hit if total_adds == 0).

comment:5 in reply to: ↑ 4 Changed 4 years ago by tomek

  • Owner changed from tomek to sar

Replying to sar:

dhcp6_messages.mes

I fixed some typo level items please do a pull and see if they are acceptable.

Thanks.

As with the v4 version I would have all of the messages start with the same substring as I think that makes it easier to find the interesting values, but this is a minor item and you don't need to address it if you don't agree. Something like:
Client %1 tried to decline address %2, <rest of message>

Updated all messages (positive and negative) to:
Client %1 send DECLINE for address %2, <rest of message>

dhcp6_srv.h

The description for processDecline() should be updated to at least not say it is a stub function and describe the return value.

Description updated.

The description for declineIA should include a description of what it returns.

It is documented now.

dhcp6_srv.cc

I've made a couple of changes to the comments in declineLeases() to update them from the cut and paste of release.

Thanks.

And similar for declineIA - but you need to check to see if the comments are still correct.

Line 2678: shouldn't this be something like opt->second instead of ia->getOption(D6O_IAADDR)

Good catch. Although it typically wouldn't matter, this code now in principle is able to decline multiple addresses in a single IA_NA.

Line 2739: the IAIDs appear to be swapped between the message (msg IAID, lease IAID) and the code (lease IAID, msg IAID) - or perhaps I misunderstand what the message is trying to say in which case perhaps it should be rewritten to make it clearer which IAID is from the message and which is form the lease.

Oops. Swapped them to correct order.

Line 2758 & 2761 - the extra return is redundant and I would probably only go with one of them, but it's not a big deal either way.

Removed.

The declineIA function uses both setStatusCode() and ia_rsp->addOption() to include status options. Is this intentional? or should it only be using setStatusCode()?

No. I forgot to update one case after implementing setStatusCode.

In declineLease should we use generateName() to construct the statistics name? As with v4 should the stats be cleared on stats removal? and should there be a total vs a subnet level counter?

Yes. Updated the code.

Shouldn't the code to dis-associate the information from the lease be grouped into a function in the lease files?

Yes. Implemented Lease6::decline() and appropriate unit-test.

decline_unittest.cc

acquire_lease() doesn't appear to be written or used. Is it going to be in the future or should it be removed?

Copy-paste error. Removed.

Can we add a test for a decline request with no addresses? or IAs? (To test out the code that we hit if total_adds == 0).

Sure. Added 2 unit-tests: DeclineTest?.noAddrsSent and DeclineTest?.noIAs.

Thanks for the review.

comment:6 Changed 4 years ago by sar

  • Owner changed from sar to tomek

All the changes look good.

The only item I noticed is that the unit test for Lease6::decline() doesn't verify the DUID.

comment:7 Changed 4 years ago by tomek

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

The code has been merged. Thanks a lot for the review. Closing ticket.

comment:8 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.