Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#3499 closed enhancement (complete)

Hooks: need a way to drop packet

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea1.0-beta
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

During discussion with Shawn Lewis and David Gong, it became apparent
that the hooks need to have the ability to signal that
a given packet should be dropped altogether. I admit that we (isc) were
talking internally about such capability, but decided that there's
no real need for it. However, since you have use cases for it, we will
implement it.

Here's how I propose to make it work. There will be 2 more methods in
CalloutHandle?:

enum CalloutNextStep {
     NEXT_STEP_CONTINUE, // continue normally
     NEXT_STEP_SKIP,  // skip the next processing step
     NEXT_STEP_DROP // drop the packet
};

/// @brief Specifies what should be the next step
///
/// This method is to be used by callouts to signal to the DHCP engine,
/// what should the next step be.
/// NEXT_STEP_CONTINUE means that the server should continue processing
///     as usual. This is the default action.
/// NEXT_STEP_SKIP means that the server should skip the next processing
///     step. This is hook-specific. For example for buffer4_receive returning
///     this status means that the server should not do packet parsing (it would
///     be the next step in the packet processing)
/// NEXT_STEP_DROP this means that the server must immediately drop the
///     packet and do not process it any further.
///
/// Note that setNextStep(NEXT_STEP_SKIP) is a replacement for setSkip(true).
/// setSkip() method is deprecated and should not be used. It will be removed
/// in future versions.
void setNextStep(CalloutNextStep next);

/// @brief Returns the next step that was set by callouts
///
/// This method returns currently set next step.
/// @return the action that will be taken as a next step
CalloutNextStep getNextStep();

Under normal circumstances, this API would replace
getSkip()/setSkip(), but unfortunately there are already field deployments of Kea
that use hooks, so we need to maintain that capability at least for a while and
allow users gentle transition to the new API.

David, Shawn: does this proposal addresses your needs? If that is so,
we can start implementing it.

Subtickets

Change History (6)

comment:1 Changed 4 years ago by tomek

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

Several hook tickets require this change, so getting on to it.

comment:2 Changed 4 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from assigned to reviewing

The code is now ready for review. I decided to replace the old API (getSkip, setSkip) with the new approach and document the change, explain our rationale for it and provide migration guidance.

Proposed ChangeLog? entry:

1XXX.	[func]*		tomek
	Boolean Skip flag in Hooks API has been replaced by enum status.
	This is backward incompatible change if you developed hook
	library that takes advantage of the skip flag. See Hooks
	Developer Guide for easy steps necessary for migration.
	(trac #3499, git tbd)

Note that this ticket does introduce DROP status, but did not update any existing calls. All places where we could potentially add the support were marked with @todo. The primary purpose for adding DROP was to be used in new calls.

The review should be quick - there's over 1000 lines, but most of them are the same type of changes.

comment:3 Changed 4 years ago by fdupont

  • Owner changed from UnAssigned to fdupont

comment:4 follow-up: Changed 4 years ago by fdupont

  • Owner changed from fdupont to tomek

dhcp4_hooks.dox: Nest -> Next (committed in the #3986 branch so please don't fix)

hooks_user.dox: fixed a spelling error so please pull.

hooks_component_developer.dox file must be updated.

Many hook library source files: 2013,2015 -> 2013, 2015

IMHO there should be DROP tests in HandlesTest? , for instance in CheckModifiedArgument? (just to get a better coverage).

Note as #3986 was based on this ticket and Jenkins was happy there is no need for a dedicated test for this ticket.

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

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

Replying to fdupont:

dhcp4_hooks.dox: Nest -> Next (committed in the #3986 branch so please don't fix)

Oops. I fixed it. That's probably ok, as I'll be the one dealing with 3986 merge anyway :)

hooks_user.dox: fixed a spelling error so please pull.

Thanks for fixing this.

hooks_component_developer.dox file must be updated.

Good catch! Updated.

Many hook library source files: 2013,2015 -> 2013, 2015

I couldn't find any problem with the dates. So I left them as they are.

IMHO there should be DROP tests in HandlesTest? , for instance in CheckModifiedArgument? (just to get a better coverage).

Extended CheckModifiedArgument? to cover all three statuses: continue, skip and drop.

Note as #3986 was based on this ticket and Jenkins was happy there is no need for a dedicated test for this ticket.

Thanks.

Code merged, changes pushed, closing ticket.

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