Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#3981 closed enhancement (complete)

Move v4 lease to declined state (DHCPDECLINE support)

Reported by: tomek Owned by: tomek
Priority: high Milestone: Kea1.0-beta
Component: dhcp4 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 DHCPDECLINE message, i.e. put the 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 ready for review. Note that the sanityCheck() is currently disabled and that part of the code awaits rework in #3116 (added extra comment in #3116 about the details).

Proposed ChangeLog? entry:

9XX.	[func]		tomek
	DHCPDECLINE message in DHCPv4 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 #3981, git tbd)

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

dhcp4_messages.mes

I fixed some typos, please do a pull and see if you agree with them.

Personally I would use the same starting string for all three of the messages, something like
Received DHCPDECLINE for addr %1 from client %2 <rest of message>
as I think it's a bit easier to handle the messages in that case, but I'm okay if you leave them the way they are.

dhcp4_srv.h

The description of processDecline() should be updated to reflect the code changes.

dhcp4_srv.cc

Several items in declineLease()
1) Shouldn't the code to disassociate a lease from the client be bundled with the rest of the lease functions? Otherwise won't this code need to be updated when the lease structure is updated to clear any newly added fields?

2) Why is the statistics name (line 1929) being assembled by hand instead of using generateName()?

3) The design document mentions a global stat for declines but I don't see that being incremented.

4) I don't see the stats for declined addresses being removed in CfgSubnets4::removeStatistics() should they be?

5) I don't see the assigned-addresses counter being decremented - does this need to be done explicitly or will it be done as part of an expiration step when the lease moves back to available?

decline_unittest.cc

In RFC2131 on the bottom of page 8 the specification requires that the client be consistent in its use of client id - either it always includes it or it never includes it. So according to a strict reading of it the tests for declineNoClientId() and declineNoclientId2 should both refuse to decline the address as the clientID doesn't match up. I'm not sure if it's better to be strict or not in this case.

The comments for declineNonMathcingIPAddress are a bit misleading. They state "Client sends DHCPDecline with the caddy set to a different address", to me this implies that the message sent to the server has ciaddr set to other than 0 rather than the test modifying the ip address of the client.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 4 years ago by tomek

  • Owner changed from tomek to sar

Replying to sar:

dhcp4_messages.mes

I fixed some typos, please do a pull and see if you agree with them.

Thanks.

Personally I would use the same starting string for all three of the messages, something like
Received DHCPDECLINE for addr %1 from client %2 <rest of message>
as I think it's a bit easier to handle the messages in that case, but I'm okay if you leave them the way they are.

Updated as suggested.

dhcp4_srv.h

The description of processDecline() should be updated to reflect the code changes.

Description updated.

dhcp4_srv.cc

Several items in declineLease()
1) Shouldn't the code to disassociate a lease from the client be bundled with the rest of the lease functions? Otherwise won't this code need to be updated when the lease structure is updated to clear any newly added fields?

In principle yes, but there's a number of arguments in favor of keeping it where it is now. First, there are other methods in Dhcp4Srv that deal with leases, so not everything needs to happen in AllocEngine?. Second, the next ticket will call hooks. One of the parameters passed to hooks will be client's packet. Note that AllocEngine? has no notion of packets and it operates on leases, so it would be tricky to handle packets there. We could do it, but then I'd have to split the method into two: one in Dhcpv4Srv and another one in AllocEngine?. Finally, AllocEngine? is complex as hell already and it's going to get more complicated when we implement other allocators. I'm trying to move things away from it, if possible.

2) Why is the statistics name (line 1929) being assembled by hand instead of using generateName()?

Oops. Corrected.

3) The design document mentions a global stat for declines but I don't see that being incremented.

Good catch. Added global stat and updated tests to check it.

4) I don't see the stats for declined addresses being removed in CfgSubnets4::removeStatistics() should they be?

That's a tricky question. If we remove it and then the lease is being recovered afterwards, declined addresses would go negative. This is general problem for several other statistics, so I created #4043 to address that. We did napkin design in jabber and it seems that the way to go will be to create a separate command for recalculating the stats.

5) I don't see the assigned-addresses counter being decremented - does this need to be done explicitly or will it be done as part of an expiration step when the lease moves back to available?

It's gonna be a part of the expiration/recovery step. I added an explanation in the code, but also in the docs. Note that the doc section is mostly empty, as there's a dedicated ticket for filling it out.

decline_unittest.cc

In RFC2131 on the bottom of page 8 the specification requires that the client be consistent in its use of client id - either it always includes it or it never includes it. So according to a strict reading of it the tests for declineNoClientId() and declineNoclientId2 should both refuse to decline the address as the clientID doesn't match up. I'm not sure if it's better to be strict or not in this case.

I'm using what other message processing methods are using: Lease::belongsToClient(). There's a very lengthy discussion regarding that method in its description. We could consider changing it, but it would have a non-trivial impact on other packets processing. And there's the Postel principle as well. Personally, I would like to keep it the way it is now, at least in the near future. One day we'll get a knob called strict-or-relaxed and then we could tighten our checks. That's far in the future, though.

The comments for declineNonMathcingIPAddress are a bit misleading. They state "Client sends DHCPDecline with the caddy set to a different address", to me this implies that the message sent to the server has ciaddr set to other than 0 rather than the test modifying the ip address of the client.

Yes. Updated the description. It was incorrect. In DHCPDECLINE ciaddr is always zero and the actual address being declined is sent in Requested IP address option.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by sar

  • Owner changed from sar to tomek

I fixed some more typo level items - once again please check them and see if you agree.

Replying to tomek:

Replying to sar:

dhcp4_srv.cc

Several items in declineLease()
1) Shouldn't the code to disassociate a lease from the client be bundled with the rest of the lease functions? Otherwise won't this code need to be updated when the lease structure is updated to clear any newly added fields?

In principle yes, but there's a number of arguments in favor of keeping it where it is now. First, there are other methods in Dhcp4Srv that deal with leases, so not everything needs to happen in AllocEngine?. Second, the next ticket will call hooks. One of the parameters passed to hooks will be client's packet. Note that AllocEngine? has no notion of packets and it operates on leases, so it would be tricky to handle packets there. We could do it, but then I'd have to split the method into two: one in Dhcpv4Srv and another one in AllocEngine?. Finally, AllocEngine? is complex as hell already and it's going to get more complicated when we implement other allocators. I'm trying to move things away from it, if possible.

I think I wasn't clear. I was simply thinking of a trivial function in lib/dhcpsrv/lease.cc that would include the code to reset or clear the fields in the lease structure. So lines 1952 to 1960 would be replaced with a single call to the cleanup function. Possibly valid_lft_ would be set to zero in the function and line 1956 would need to move to be after the call.

Everything else looks fine

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

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

Replying to sar:

I think I wasn't clear. I was simply thinking of a trivial function in lib/dhcpsrv/lease.cc that would include the code to reset or clear the fields in the lease structure. So lines 1952 to 1960 would be replaced with a single call to the cleanup function. Possibly valid_lft_ would be set to zero in the function and line 1956 would need to move to be after the call.

Done as requested. Since the change was trivial, I merged the code without further reviews (as agreed on jabber).

Thanks 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.