wmnsk / go-pfcp

PFCP (Packet Forwarding Control Protocol) in pure Go.
MIT License
125 stars 56 forks source link

Check for inconsistencies in Apply Action IE flags #113

Closed louisroyer closed 1 year ago

louisroyer commented 1 year ago

Check for inconsistencies in Apply Action IE flags

In section 8.2.26 of TS 29.244, some precisions are indicated about combined uses of Apply Action flags. A CP entity should not send inconsistent flags, and a UP entity should reject messages containing Apply Actions with inconsistent flags because such messages are considered faulty.

This new helper ValidateApplyAction() can be used to help the debugging, but it remain optional since validation could also be done on upper layers. Also removes unnecessary length check in helpers since already checked in the constructor.

tim-ywliu commented 1 year ago

@louisroyer

Implementation note: In flags-check helpers, to avoid infinite recursive call of ApplyAction(), I had to check the IE type and perform the check directly on the Payload. To keep backward compatibility, some (controlled) recursion may still happend if IE Type is not directly Apply Action.

ApplyAction() already checks the IE type, so helper functions call ApplyAction() will not be infinite recursive. If the IE type is ApplyAction, it will stop and return.

louisroyer commented 1 year ago

ApplyAction() already checks the IE type, so helper functions call ApplyAction() will not be infinite recursive. If the IE type is ApplyAction, it will stop and return.

Let's say I don't add Type check directly in HasFORW():

  1. ApplyAction() is called somewhere
  2. ApplyAction() calls inconsistentApplyActionFlags() at line 21
  3. inconsistentApplyActionFlags() calls HasFORW() at line 56
  4. HasFORW() calls ApplyAction(): go back to step 2. I initially spotted this by running go test and having a stack overflow because of the recursion.
louisroyer commented 1 year ago

Ok, these are good points. I will rework this PR.

EDIT: I believe this validation function is in fact more helpful for prototyping/testing than in production, because it facilitates debugging, while in production validation is done on upper layer anyway. The goal is not to be very strict about what is allowed or not, but more to have hints that can help answer the question "Is it my UP or my CP PFCP component who is doing something strange?" without having to re-read the whole specification in details. The spec may evolve of course, but most of this is good sense: for example, a PDU cannot be dropped and forwarded at the same time.