Opened 5 years ago

Closed 4 years ago

#3694 closed defect (complete)

conflicts in DHCPv4 reservation malfunction part 2.

Reported by: wlodekwencel Owned by: marcin
Priority: high Milestone: Kea0.9.1
Component: dhcp4 Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Test scenario:

  1. Server is configured with pool 192.168.50.5-192.168.50.10 and without any reserved addresses.
  1. Client X request address and leases with address A is saved.
  1. Server reconfiguration with same pool but with reservation for address A for client Y.
  1. Client Y send DISCOVER as attempt to get address A. But address A is granted to Client X so Client Y should get any other available address from the pool. But Kea remains silent (or send NAK as respond to REQUEST).

(same thing happens when address A was initially reserved to Client X)

attached:

  • leases file
  • log file
  • message capture

Subtickets

Attachments (1)

hrv4.tar.gz (2.8 KB) - added by wlodekwencel 5 years ago.

Download all attachments as: .zip

Change History (11)

Changed 5 years ago by wlodekwencel

comment:1 Changed 5 years ago by tomek

We (Shawn, Marcin, Wlodek, Tomek) had a discussion about this and agree that the test is valid. The Kea code does not support it yet. It's a matter of deciding on priority.

comment:2 Changed 5 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea0.9.1

comment:3 Changed 5 years ago by marcin

  • Priority changed from medium to high

comment:4 Changed 5 years ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:5 Changed 4 years ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing

This change is now ready for the review.

I reorganized the code in the AllocEngine::allocateLease4. Depending on whether it is the fake allocation (DHCPDISCOVER case) or real allocation (DHCPREQUEST case) a different private method is called: discoverLease4 or requestLease4 respectively. These methods now offer/allocate an address from the dynamic pool if the reserved address is in use by another client.

Since the Allocation engine contains the methods for both DHCPv4 and DHCPv6 I grouped these methods in the header file and in the .cc file, so as all the common methods are together, DHCPv6 methods are together and the DHCPv4 methods are together. It improves the readability of the code, which is important given that the allocation engine's logic is quite complex.

I also applied several little changes in the Kea Guide.

I apologize for the size of the diff, but it is large mostly because I moved functions around, usually without modifying them.

Proposed ChangeLog:

XXX.	[func]		marcin
	DHCPv4 server assigns an address from the dynamic address
	pool if the reserved address is in use by another client.
	(Trac #3694, git abcd)

comment:6 Changed 4 years ago by sar

  • Owner changed from UnAssigned to sar

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

  • Owner changed from sar to marcin

dhcp4-srv.xml

We have "its temporary address" as TA is a specific phrase something else is better.

direct_client_unittest.cc

line 310,346 - the comment says subnet is required to create the lease, after the changes it isn't.

dora_unittest.cc

reservationswWithConflicts

The description has the DISCOVERs from Client B being dropped when A has the reserved address.

line 669 - we test that client B didn't get the address for A but we don't check that it got its previous address

line 684 - why do we do a toText() in this address check when we don't in the others?

io_address_unittest.cc

Several extra tests can be added for completeness, comparing the following addresses in the tests
isV4Zero: 0.0.0.x, x.0.0.0, "::"

isV4Bcast: 255.255.255.x, x.255.255.255, "FF::FF"

isV6Zero: "::FF", "FF::"

alloc_engine.cc

Line 857, 921 - I notice that when setting the subnet for v4 in the callout handle we explicitly do a cast (and mention it in the comment) but we don't do that for v6. Do we need to?

Line 1154 - Why does addresReserved() return true if there isn't a hardware address to compare? It would seem we either expect that as an option in which case we should be able to match on something else or we don't need to handle it.

line 1192 - I'm curious why the default constructor for ClientContext4() is in the .cc file but the one for ClientContext6() is in the .h file.

line 1201 - I think we need to talk a bit about myLease() and how the identifiers are being used. In v4 if there is a client_id it should take precedence and completely identify the client. I think this might require moving the clientid checks to be before the hwaddr checks and adding a return of true if both lease and context have a client id and they are equal. The use of hwaddr vs client_id may extend beyond myLease().

line 1336 - It seems to me that for (minor) efficiency it would be better to do the addressReserved() call after the inPool() check.

line 1660 - resueExpiredLease4() checks expired and ctx.subnet_ and then calls updateLease4Information() which does the exact same thing, seems a bit wasteful.

alloc_engine.h

The documentation seems a bit odd. There is a grouping of the allocator classes around line 61 but it doesn't seem to include the Allocator classes. Just the enum and pointers. The v4 and v6 groupings also end up at different header levels. I've tried rebuilding it and it looks the same.

line 205 - should the "public:" tag be aginst the left margin?

line 234 - the previous description of ~AllocEngine?() included a mention of when it would be used. Should that still be included?

line 394 - I'm curious why the default constructor for v4 was moved into alloc_engine.cc while the one for v4 was left in alloc_engine.h.

line 855 - In this paragarph the comment says we don't hand out a lease to a client that has a reservation if the reserved address is in use.

line 939, 978 - Shouldn't the exceptions be listed?

line 1026,1041 - Should the information used from the context be sepecified?

alloc_engine4_unittest.cc

line 549 - should that be renewLease4?

line 551 - There should be some comments describing what requestOtherClientLease is doing.

line 883 - The comment needs to be updated that it gets a differnet lease,

line 902,1020,1280 - It would be useful to complete the request (fake allocate = false) to verify that the lease is properly processed.

line 1076 - the comment should be movidied a bit, it currently reads as if the lease is being removed while the test actually expects it to remain. Adding a request and verifying the old is removed and the new added would be useful.

line 1244 - I believe this test is doing a request, you may as well check that the lease was properly added to the lease manager.

dhcpsrv_messages.mes

The use of the phrase temporary address is a bit unfortunate.

Other:

Some of the comments for getLease4 in lease_mgr.h seem to have cut and paste errors - using hwaddr or hardware address instead of client id

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

  • Owner changed from marcin to sar

Replying to sar:

dhcp4-srv.xml

We have "its temporary address" as TA is a specific phrase something else is better.

Ok. I changed it to "temporarily allocated".

direct_client_unittest.cc

line 310,346 - the comment says subnet is required to create the lease, after the changes it isn't.

Removed those.

dora_unittest.cc

reservationswWithConflicts

The description has the DISCOVERs from Client B being dropped when A has the reserved address.

Fixed.

line 669 - we test that client B didn't get the address for A but we don't check that it got its previous address

Checking it now.

line 684 - why do we do a toText() in this address check when we don't in the others?

Removed the toText().

io_address_unittest.cc

Several extra tests can be added for completeness, comparing the following addresses in the tests
isV4Zero: 0.0.0.x, x.0.0.0, "::"

isV4Bcast: 255.255.255.x, x.255.255.255, "FF::FF"

isV6Zero: "::FF", "FF::"

Added tests as requested.

alloc_engine.cc

Line 857, 921 - I notice that when setting the subnet for v4 in the callout handle we explicitly do a cast (and mention it in the comment) but we don't do that for v6. Do we need to?

I prefer the explicit cast to the implicit cast as it makes clearer what is happening. At the same time I didn't

Line 1154 - Why does addresReserved() return true if there isn't a hardware address to compare? It would seem we either expect that as an option in which case we should be able to match on something else or we don't need to handle it.

You're right. I changed it to false. I think we will cover testing this case when we revisit handling of HW addresses and client ids, including NULL HW address and no client id.

line 1192 - I'm curious why the default constructor for ClientContext4() is in the .cc file but the one for ClientContext6() is in the .h file.

I was working on the v4 part so I just focused on this. Now I moved the implementation for the !ClientContext6 constructor to .cc file.

line 1201 - I think we need to talk a bit about myLease() and how the identifiers are being used. In v4 if there is a client_id it should take precedence and completely identify the client. I think this might require moving the clientid checks to be before the hwaddr checks and adding a return of true if both lease and context have a client id and they are equal. The use of hwaddr vs client_id may extend beyond myLease().

I fully agree. As we discussed over the phone, we need new tickets to revisit the code and properly handle various cases for HW addresses and client ids.

line 1336 - It seems to me that for (minor) efficiency it would be better to do the addressReserved() call after the inPool() check.

The line number you provided was incorrect. But I fixed it a few lines down.

line 1660 - resueExpiredLease4() checks expired and ctx.subnet_ and then calls updateLease4Information() which does the exact same thing, seems a bit wasteful.

True. I removed those checks and added the warning to the documentation to state that this function doesn't check pointers.

alloc_engine.h

The documentation seems a bit odd. There is a grouping of the allocator classes around line 61 but it doesn't seem to include the Allocator classes. Just the enum and pointers. The v4 and v6 groupings also end up at different header levels. I've tried rebuilding it and it looks the same.

Yes, it seems that doxygen doesn't deal well with repeating scopes. I removed the groupings but the generated documentation still doesn't present a good order at times. But, I decided to leave it as is.

line 205 - should the "public:" tag be aginst the left margin?

Yes. Fixed.

line 234 - the previous description of ~AllocEngine?() included a mention of when it would be used. Should that still be included?

No, the previous description was inappropriate. There are other cases when the allocation engine is called than server shut down. And it is not really important given that the destructor is no-op.

line 394 - I'm curious why the default constructor for v4 was moved into alloc_engine.cc while the one for v4 was left in alloc_engine.h.

This is just that I worked and focused on v4. But, I moved it now.

line 855 - In this paragarph the comment says we don't hand out a lease to a client that has a reservation if the reserved address is in use.

Fixed.

line 939, 978 - Shouldn't the exceptions be listed?

We could try, but there are different possible exceptions that can be thrown and I am not sure which exceptions exactly. So, unless we catch all exceptions and re-throw I think it is not worth to try to list them because if we miss any exception the caller may not catch it. By saying it throws various exception I wanted to indicate that the caller should catch std::exception to catch all possible ones.

line 1026,1041 - Should the information used from the context be sepecified?

I think that being too specific has a downside that it will need to be updated when new context fields are added. If someone is interested what fields the context has, he can refer to the !ClientContext4 documentation.

alloc_engine4_unittest.cc

line 549 - should that be renewLease4?

Yes. But removed it. We don't want to test the private method. We test it indirectly.

line 551 - There should be some comments describing what requestOtherClientLease is doing.

Added comments.

line 883 - The comment needs to be updated that it gets a differnet lease,

Updated.

line 902,1020,1280 - It would be useful to complete the request (fake allocate = false) to verify that the lease is properly processed.

I decided to not do this. These tests were for the fake allocation which is only a DHCPDISCOVER case. For DHCPREQUESTs we have other tests.

line 1076 - the comment should be movidied a bit, it currently reads as if the lease is being removed while the test actually expects it to remain. Adding a request and verifying the old is removed and the new added would be useful.

Fixed.

line 1244 - I believe this test is doing a request, you may as well check that the lease was properly added to the lease manager.

Added.

dhcpsrv_messages.mes

The use of the phrase temporary address is a bit unfortunate.

Yes. Changed it.

Other:

Some of the comments for getLease4 in lease_mgr.h seem to have cut and paste errors - using hwaddr or hardware address instead of client id

Can you point out where exactly you found those?

comment:9 in reply to: ↑ 8 Changed 4 years ago by sar

  • Owner changed from sar to marcin

I've fixed several more typos and pushed the results.

There are a couple more items below, after they are addressed the code can be merged - it doesn't require another review.

Replying to marcin:

Replying to sar:

dora_unittest.cc

reservationswWithConflicts

The description has the DISCOVERs from Client B being dropped when A has the reserved address.

Fixed.

Items 14 & 17 in the comment before the test still mention that the DISCOVER for Client B is dropped when Client A holds the reserved lease.

io_address_unittest.cc

Several extra tests can be added for completeness, comparing the following addresses in the tests
isV4Zero: 0.0.0.x, x.0.0.0, "::"

isV4Bcast: 255.255.255.x, x.255.255.255, "FF::FF"

In isV6Zero we check out using a v4 0 address, do we want to do the same for the isV4Zero and isV4Bcast tests with the 0 and all ones V6 addresses?

isV6Zero: "::FF", "FF::"

Added tests as requested.

Two tests were added but both of them are "::ff", I've updated one to be "ff::"

alloc_engine.cc

line 1201 - I think we need to talk a bit about myLease() and how the identifiers are being used. In v4 if there is a client_id it should take precedence and completely identify the client. I think this might require moving the clientid checks to be before the hwaddr checks and adding a return of true if both lease and context have a client id and they are equal. The use of hwaddr vs client_id may extend beyond myLease().

I fully agree. As we discussed over the phone, we need new tickets to revisit the code and properly handle various cases for HW addresses and client ids.

I've created two tickets for the client id vs hardware address issues 3746 & 3747

Other:

Some of the comments for getLease4 in lease_mgr.h seem to have cut and paste errors - using hwaddr or hardware address instead of client id

Can you point out where exactly you found those?

I believe I was referring to line 238 in lease_mgr.h - I've changed the hardware address to client id but am not entirely sure the whole comment is correct - it seems like it should be (only one lease per client id in a single pool).

comment:10 Changed 4 years ago by marcin

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

Fixed issues and merged with commit 95b09ff53b941691cba172c933de0682b05a0d85

Note: See TracTickets for help on using tickets.