Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#5280 closed enhancement (complete)

Implement lease deletion commands

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea1.3 beta
Component: management API Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Mozilla Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 1
Total Hours: 32 Internal?: no

Description

Once #5279 is done, this ticket will continue the effort. It will cover the removal related commands:

lease4-del, lease6-del, leases4-del-all, leases6-del-all.

All calls should be implemented as part of the lease hook library (#5272).

Subtickets

Change History (12)

comment:1 Changed 3 years ago by tomek

  • Summary changed from Implement leaseX-delete calls to Implement lease deletion calls

comment:2 Changed 3 years ago by tomek

  • Summary changed from Implement lease deletion calls to Implement lease deletion commands

comment:3 Changed 2 years ago by tomek

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

comment:4 Changed 2 years ago by tomek

  • Add Hours to Ticket changed from 0 to 24
  • Owner changed from tomek to Unassigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 24

The code is now ready for review. Changes are on trac5280 branch.

Proposed ChangeLog?:

12XX.	[func]		tomek
	Several new commands implemented in lease_cmds library:
	lease4-del, lease6-del, lease4-update, lease6-update,
	lease4-wipe and lease6-wipe that allow deleting and updating
	leases and also wipe all leases from a specific directory.
	(Trac #5280, #5281, git tbd)

One thing to be kept in mind is that the leaseX-wipe commands are implemented only for memfile.

comment:5 Changed 2 years ago by tmark

  • Owner changed from Unassigned to tmark

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

The code is pretty straight forward so I really didn't find much that needs addressing. I did do some word-smithing on the docs so please pull first:


leaseXUpdateHandler() - both handlers test if the lease exists before we update it, but
so do the Memfile updates methods. We'll look up the lease twice, do we care? The SQL
backends don't check, but the update statement will fail and we catch that.


LeaseCmdsImpl::getParameters(const ConstElementPtr?& params)

Looks to me like this doesn't ensure the value for ip-address and Lease::TYPE_V4
are sane. In other words, I can specify a V6 address and type of TYPE_V4.


Lease4Parser::parse()
Lease6Parser::parse()

Neither of these validate the value of state. So in theory we could attempt to put a
lease into an unknown state.


src/lib/dhcpsrv/tests/lease_mgr_unittest.cc

Commentary for wipeleases4() and wipeleases6() is missing @param and I believe
you meant to say "Pretends to wipe all" not "Pretends to will all".


LeaseCmdsImpl::lease4WipeHandler(const string& /*cmd*/, ConstElementPtr? params)

The Comment below is a bit wrong ;). Same in lease6WipeHandler()

+ try {
+
+ We need the lease to be specified.

I believe we need to specify the subnet-id ;)


Lastly, your ChangeLog? proposal says the wipe commands remove leases "from a specific directory." I believe you mean subnet, unless I really don't understand this stuff at all. ;)

Builds and unit tests pass on OS-X.

comment:7 Changed 2 years ago by tmark

  • Add Hours to Ticket changed from 24 to 4
  • Owner changed from tmark to tomek
  • Total Hours changed from 24 to 28

comment:8 in reply to: ↑ 6 Changed 2 years ago by tomek

  • Add Hours to Ticket changed from 4 to 3
  • Total Hours changed from 28 to 31

Replying to tmark:

The code is pretty straight forward so I really didn't find much that needs addressing. I did do some word-smithing on the docs so please pull first:

Thanks for fixing them.

leaseXUpdateHandler() - both handlers test if the lease exists before we update it, but
so do the Memfile updates methods. We'll look up the lease twice, do we care? The SQL
backends don't check, but the update statement will fail and we catch that.

Good point. Removed that extra check and added unit-tests that verify that the command will indeed fail if there's no lease to be updated.

LeaseCmdsImpl::getParameters(const ConstElementPtr?& params)

Looks to me like this doesn't ensure the value for ip-address and Lease::TYPE_V4
are sane. In other words, I can specify a V6 address and type of TYPE_V4.

Added a boolean parameter that specifies expected family type. Also added bunch of tests.

Lease4Parser::parse()
Lease6Parser::parse()

Neither of these validate the value of state. So in theory we could attempt to put a
lease into an unknown state.

Check and tests added.

src/lib/dhcpsrv/tests/lease_mgr_unittest.cc

Commentary for wipeleases4() and wipeleases6() is missing @param and I believe
you meant to say "Pretends to wipe all" not "Pretends to will all".

Yup. Fixed.

LeaseCmdsImpl::lease4WipeHandler(const string& /*cmd*/, ConstElementPtr? params)

The Comment below is a bit wrong ;). Same in lease6WipeHandler()

+ try {
+
+ We need the lease to be specified.

I believe we need to specify the subnet-id ;)

Uh oh. Copy-paste error. It's amazing what kind of crap they put on the Internet. ;)

Lastly, your ChangeLog? proposal says the wipe commands remove leases "from a specific directory." I believe you mean subnet, unless I really don't understand this stuff at all. ;)

Uh oh. My brain was going offline when I wrote that.

Builds and unit tests pass on OS-X.

Hopefully the changes pushed address all the comments.

comment:9 Changed 2 years ago by tomek

  • Owner changed from tomek to tmark

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

  • Add Hours to Ticket changed from 3 to 1
  • Owner changed from tmark to tomek
  • Total Hours changed from 31 to 32

The changes are good, go ahead and merge.

comment:11 in reply to: ↑ 10 Changed 2 years ago by tomek

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

Replying to tmark:

The changes are good, go ahead and merge.

Thanks. Code merged. Closing ticket.

comment:12 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.