Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#5104 closed defect (fixed)

kea saves multiple instances of the prefix lease during RENEW/REBIND

Reported by: wlodekwencel Owned by: fdupont
Priority: very high Milestone: Kea1.3 beta
Component: database-all 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

While client RENEW/REBIND lease with prefix Kea saves 3 leases to the file instead of one.

One lease with lifetime 0
Two leases with lifetimes >0

Scenario:
Client solicit for prefix.
Client request prefix.
Kea saves:

3001::,00:01:00:01:00:c1:11:a4:a3:60:9c:84:69:7d,4000,1483633090,1,3000,2,95903,120,0,0,,a3:60:9c:84:69:7d,0

Client Renew/Rebind? prefix.
Kea saves:

3001::,00:01:00:01:00:c1:11:a4:a3:60:9c:84:69:7d,0,1483629090,1,0,2,95903,120,0,0,,a3:60:9c:84:69:7d,0
3001::,00:01:00:01:00:c1:11:a4:a3:60:9c:84:69:7d,4000,1483633090,1,3000,2,95903,120,0,0,,a3:60:9c:84:69:7d,0
3001::,00:01:00:01:00:c1:11:a4:a3:60:9c:84:69:7d,4000,1483633090,1,3000,2,95903,120,0,0,,a3:60:9c:84:69:7d,0

When Kea save lease with address (not prefix) during Rebind/Renew? it save just one lease with lifetime >0

Subtickets

Attachments (1)

results.tar.gz (15.6 KB) - added by wlodekwencel 3 years ago.
kea logs, and trafic capture

Download all attachments as: .zip

Change History (12)

Changed 3 years ago by wlodekwencel

kea logs, and trafic capture

comment:1 Changed 3 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.3

Per Kea team meeting 5 Jan, move to 1.3. Leave as Medium.

comment:2 Changed 2 years ago by fdupont

  • Owner set to fdupont
  • Priority changed from medium to very high
  • Status changed from new to accepted

I found the source of the problem: the inPool method is wrong for prefixes because it begins by checking if its arguments is in the subnet range. Unfortunately this means that prefix delegation was broken, so the fix can reveal other bugs and should be back ported to previous releases (for this second point I bump its priority).

comment:3 Changed 2 years ago by fdupont

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

Ready for review. It definitely requires a ~ChangeLog? entry.

comment:4 Changed 2 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:5 follow-up: Changed 2 years ago by tmark

  • Owner changed from tmark to fdupont

The changes seem to work fine. I tested them using configurations with prefix pools that were both inside and outside the selected subnet. I do have the following questions:

  1. The comment for commit e01f1d6f7ff941446a05022ebb751658b636a7a2 says "Fix core". Was there a scenario that was causing a core dump? If so, it should be probably have been described in the ticket and/or would need to be mentioned in a ChangeLog? entry. If not "Fix core" doesn't explain much.
  1. You recommend we back port this because PD delegation is "broken". Can you expand on that? Your change certainly corrects the code flow and we no longer write extra entries to lease storage but ultimately the prefix does get renewed. Are you suggesting we do maintenance releases? To my knowledge we have no plans to do so.


comment:6 in reply to: ↑ 5 Changed 2 years ago by fdupont

  • Owner changed from fdupont to tmark

Replying to tmark:

The changes seem to work fine. I tested them using configurations with prefix pools that were both inside and outside the selected subnet. I do have the following questions:

  1. The comment for commit e01f1d6f7ff941446a05022ebb751658b636a7a2 says "Fix core". Was there a scenario that was causing a core dump?

=> no, I split the fix into a core which only add the missing check before rejecting out-of-range assignments, and the remaining part with a comment update and a new unit test. The idea is to backport only the code (and the ChangeLog entry).

  1. You recommend we back port this because PD delegation is "broken". Can you expand on that?

=> any previous prefix assignment can be considered as out of the pool because it is out of the subnet range. This is why the ticket says there are 3 lines: the first to delete the previous assignment (removeNonmatchingReservedLeases6), the second allocates a prefix (allocateUnreservedLeases6) and the third because it is considered as not good but reusable (extendLease6). BTW fortunately the same prefix is assigned but the fact PD assignments do not pass sanity checks is IMHO enough to say it is broken.

Your change certainly corrects the code flow and we no longer write extra entries to lease storage but ultimately the prefix does get renewed.

=> it is renewed only because the default assignment algorithm is stable. If for instance you use an algorithm which has a hold down phase not only you'll get different prefixes but you'll consume 2 prefixes at each iteration.

Are you suggesting we do maintenance releases? To my knowledge we have no plans to do so.

=> yes I am suggesting we do this. I propose we discuss about this tomorrow or Friday.

To merge the fix I need a ChangeLog entry, and if we backport the fix it should be usable for maintenance release too. Do you have a proposal?

comment:7 Changed 2 years ago by tomek

  • Component changed from Unclassified to database-backend

comment:8 Changed 2 years ago by tmark

  • Owner changed from tmark to fdupont

Changes are fine to merge to master.

This is probably sufficient for ChangeLog?:

"Corrected logic in prefix delegation that was causing multiple entries to be written to the lease file when renewing or rebinding a prefix."

As to maintenance releases, I'll leave that to Tomek as to how to proceed.

comment:9 Changed 2 years ago by fdupont

Merged. Closing (the maintenance release question is not closed but this is not an accepted candidate).

comment:10 Changed 2 years ago by fdupont

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

comment:11 Changed 2 years ago by vicky

  • Milestone changed from Kea1.3 to Kea1.3 beta

Milestone renamed

Note: See TracTickets for help on using tickets.