#5443 closed enhancement (complete)

implements more NEXT_STEP_DROP

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea1.4
Component: hooks 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

For instance to simulate ISC DHCP not authoritative it should be fine that subnet selection hook can simply return NEXT_STEP_DROP.
Trivial to do so keep it medium.

Subtickets

Change History (10)

comment:1 Changed 21 months ago by fdupont

  • Milestone changed from Kea-proposed to Kea1.4

comment:2 Changed 21 months ago by fdupont

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

comment:3 Changed 21 months ago by fdupont

Proposed code for DHCPv4. The main change is when the select subnet hook returns DROP the method itself clears the query shared pointer. As it requires to remove const qualifier at many place if you prefer it can be done by adding another parameter to selectSubnet: it is a matter of taste so if you prefer a solution just speak ASAP!

comment:4 Changed 21 months ago by fdupont

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

Moves to extra argument style. Done for DHCPv4 and DHCPv6 servers (not the alloc engine).
Ready for review.

comment:5 Changed 20 months ago by tomek

  • Owner changed from UnAssigned to tomek

comment:6 Changed 20 months ago by fdupont

There are 2 points to consider after the formal review and before merging:

  • behavior is not always consistent and/or conform to the documentation
  • what to do about statistics?

comment:7 follow-up: Changed 19 months ago by tomek

  • Owner changed from tomek to fdupont

Your changes are fine. I would implement this using exception that is
caught at the very top level as it seems simpler and the code would
be simpler. But it's a matter of persnal preference. The code can stay as it is now.

I made several changes to the docs. Please pull and review.

dhcp{4,6}_hooks.dox
This is just a minor nit, but it should be SKIP or DROP, not SKIP (or
DROP). Those two values are equal in behavior, so there's no need to
have one of them in parentheses.

dhcp{4,6}_message.mes
Your new text mentions "skip flag", which is a copy/paste error from
older text. There used to be a boolean flag, now we have the next
step code. This new text as well as old text you copied it from
should be updated. It should say "next step status set to skip" rather
than "skip flag". I did that. Please pull and review.


There are several hook points in src/lib/dhcpsrv/alloc_engine.cc
that don't have DROP implemented. If you think it's not necessary
to modify them, please remove the @todos in that file.

Statistics are out of scope for this ticket.

comment:8 Changed 19 months ago by tomek

Also, this change requires a Changelog entry. If you don't have any better text, here's my proposal:

13xx.	[func]		fdupont
	Several hook points now support next step status DROP. This allows
	more flexibility with dropping packets from within hooks.
	(Trac #5443, git tbd)

comment:9 in reply to: ↑ 7 Changed 19 months ago by fdupont

Replying to tomek:

Your changes are fine. I would implement this using exception that is
caught at the very top level as it seems simpler and the code would
be simpler. But it's a matter of personal preference. The code can stay as it is now.

=> it is not strictly equivalent and the exception style does not match
the current style, so it does not match the KISS principle I am attached
to for obvious reasons...

I made several changes to the docs. Please pull and review.

dhcp{4,6}_hooks.dox
This is just a minor nit, but it should be SKIP or DROP, not SKIP (or
DROP). Those two values are equal in behavior, so there's no need to
have one of them in parentheses.

=> there is a preferred value but I agree the order is enough to mark this very
secondary point.

dhcp{4,6}_message.mes
Your new text mentions "skip flag", which is a copy/paste error from
older text. There used to be a boolean flag, now we have the next
step code. This new text as well as old text you copied it from
should be updated. It should say "next step status set to skip" rather
than "skip flag". I did that. Please pull and review.

=> yes, I abused of cut & paste.

There are several hook points in src/lib/dhcpsrv/alloc_engine.cc
that don't have DROP implemented. If you think it's not necessary
to modify them, please remove the @todos in that file.

=> IMHO a drop in lease processing does not make sense as there is
nothing to drop in a lease. I am removing to @todo's.

Statistics are out of scope for this ticket.

=> they should be addressed one day but I agree it should be done
in its own ticket.

comment:10 Changed 19 months ago by fdupont

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

Merged. Closing.

Note: See TracTickets for help on using tickets.