Initial IPFIX implementation #219

ivan4th · 2022-02-22T05:34:19Z

TODO in a follow-up:

other missing fields (VRF IDs)
make reporting interval configurable
fix format_tbcd (non-numeric chars)

IMSI in the IPFIX packets is presently encoded using the format described in TS 29.274 Section 8.3.

go-ipfix is being used for E2E testing, fixes:
vmware/go-ipfix#276
vmware/go-ipfix#277

miguelbf-alb · 2022-03-07T18:09:11Z

upf/upf_ipfix.c

+  /* the value can't be 255 or more due to limitations of pfcp.c, */
+  /* thus we use simpler encoding */
+  to_b->data[offset] = sx->imsi_len;
+  clib_memcpy_fast (to_b->data + offset + 1, sx->imsi, sx->imsi_len);


I gave this a whirl out of curiosity for the functionality and while pretty much everything works out of the box the imsi ipfix field is not decoded by wireshark or any ipfix collector I've tried. As far as I could see the mobileImsi is sent in the ipfix payload as a string so, considering the imsi has a length of 15 codepoints the provided buffer should be 16 byte buffer (imsi + null terminator). sx->imsi is currently stored as 8 bytes though.
I guess here you need to format the mobile identify and allocate a 16 buffer for this to work:

diff --git a/upf/upf_ipfix.c b/upf/upf_ipfix.c index e493271..4756036 100644 --- a/upf/upf_ipfix.c +++ b/upf/upf_ipfix.c @@ -470,9 +470,15 @@ upf_ipfix_common_add (vlib_buffer_t * to_b, flow_entry_t * f, flow_direction_t d /* mobileIMSI */ /* the value can't be 255 or more due to limitations of pfcp.c, */ /* thus we use simpler encoding */ - to_b->data[offset] = sx->imsi_len; - clib_memcpy_fast (to_b->data + offset + 1, sx->imsi, sx->imsi_len); - offset += sx->imsi_len + 1; + + // note: imsi string is 15 code points and is currently stored as an 8 bit array (u8* -> sizeof(sx->imsi)=8). + // the ipfix rfc state that the imsi is encoded as a string (unicode uses 1 byte per codepoint for the first 128 codepoints). Hence when transfering the imsi in the ipfix payload + // we need to pre-allocate a buffer of size 16 before calling format_mobile_identity (15 codepoints + null terminator). This is equivalent to 2 * sx->imsi_len + 1. + + to_b->data[offset] = sx->imsi_len * 2; + u8 *imsi = format (NULL, "%U", format_mobile_identity, sx->imsi, sx->imsi_len); + clib_memcpy_fast (to_b->data + offset + 1, imsi, 2 * sx->imsi_len); + offset += 2 * sx->imsi_len + 1;

Snippet quality might not be the best one ever but works as a PoC for me :)

Btw I also got a segmentation fault on the vpp startup, gdb pointed out at the inability of allocating memory for the flow table. I had to reduce its size to runtime test this:

diff --git a/upf/flowtable.h b/upf/flowtable.h index e535d87..467242d 100644 --- a/upf/flowtable.h +++ b/upf/flowtable.h @@ -207,7 +207,7 @@ typedef struct * hashtable is configured to alloc (NUM_BUCKETS * CLIB_CACHE_LINE_BYTES) Bytes * with (flow_count / (BIHASH_KVP_PER_PAGE / 2)) Buckets */ -#define FM_POOL_COUNT_LOG2 22 +#define FM_POOL_COUNT_LOG2 15 #define FM_POOL_COUNT (1 << FM_POOL_COUNT_LOG2) #define FM_NUM_BUCKETS (1 << (FM_POOL_COUNT_LOG2 - (BIHASH_KVP_PER_PAGE / 2))) #define FM_MEMORY_SIZE (FM_NUM_BUCKETS * CLIB_CACHE_LINE_BYTES * 6)

Doesn't seem to happen on master (nor with the base branch - 22.02) so likely has to do with the ipfix addition - I haven't really tracked the root cause but can provide the stack trace later.

Sorry for the late reply.
Concerning mobileIMSI field: I had an impression that it must be stored using TBCD encoding, but will recheck
Concerning the segfault (actually SIGABRT I think due to heap exhaustion): you may try increasing the heap size, e.g. by setting heapsize 3G in startup.conf

upf/pfcp.c

RoadRunnr · 2022-03-14T11:45:33Z

upf/pfcp.c

+  for (i = 0; i < n_bytes; i++)
+    {
+      if (bytes[i] & 0xf0 == 0xf0 && i == n_bytes - 1)
+	s = format(s, "%d", bytes[i] & 0xf);


A TCBD string can also contain the special characters * (10), # (11), 'a' (12), 'b' (13), and 'c' (14).

RoadRunnr · 2022-03-14T11:48:12Z

upf/pfcp.c

+
+  for (i = 0; i < n_bytes; i++)
+    {
+      if (bytes[i] & 0xf0 == 0xf0 && i == n_bytes - 1)


In GTPv1 the IMSI is a fixed length field where unused digits are filled with 2#1111. I might therefore be safer to support a tail of arbitrary many fillers.
Terminating the loop when the lower nibble is 2#1111 is what gtplib is doing. That should work here as well.

upf/pfcp.h

RoadRunnr · 2022-03-14T11:55:50Z

upf/upf_flow_node.c

-	  /* timer management */
-	  flow_update_active (flow0, current_time);
-	  flow_update_active (flow1, current_time);
-
-	  /*
-	   * Should update lifetime after updating flow activity to
-	   * avoid scheduling flows "in the past"
-	   */
-	  flow_update_lifetime (flow0, p0, is_ip4);
-	  flow_update_lifetime (flow1, p1, is_ip4);
-
-	  /* flow statistics */
-	  flow0->stats[is_reverse0].pkts++;
-	  flow0->stats[is_reverse0].bytes += b0->current_length;
-	  flow1->stats[is_reverse1].pkts++;
-	  flow1->stats[is_reverse1].bytes += b1->current_length;


Duplicating code like this is often use in VPP to reap performance benefits from instruction pipelining.
We should probably micro-bench this and similar places to find out if make sense to keep them like this or move them to helpers.

Well, I'm not sure inline funcs will be much worse than repeated code / macros (that is: perhaps the same code will be generated in the end), although perhaps I don't know enough of clang internals to be sure about that. If further testing will reveal performance degradation, I'll take a closer look at this place; although so far the main bottleneck in UPG appears to be insufficient cache locality

RoadRunnr · 2022-03-14T12:26:43Z

upf/flowtable.c

+/* *INDENT-OFF* */
+VLIB_REGISTER_NODE (flowtable_process_node) = {
+  .function = flowtable_process,
+  .type = VLIB_NODE_TYPE_PROCESS,


If you made this a VLIB_NODE_TYPE_PRE_INPUT instead, it would run on every worker. The function could then work on the per_cpu flowtable only and that would mean the barrier_sync in there is not needed.

Just replacing VLIB_NODE_TYPE_PROCESS with VLIB_NODE_TYPE_PRE_INPUT causes the code to crash, so it needs a bit more adjustments to do it that way. Perhaps could do it in a follow-up.
This node is disabled by default and is only used in testing situations with low traffic (causing some remaining flows not to expire) and interrupt mode in use.

In IPFIX tests, there happen to be too many VPP instances active at the same time, leading to memory exhaustion.

Also, make sure that `postNAPTSourceTransportPort` is always correct when NAT is being used. It's necessary to postpone NAT processing upon the first packet to avoid having 0 in this field.

sergeymatov

Overal - LGTM

sergeymatov · 2022-05-11T08:23:44Z

upf/upf_cli.c

@@ -444,7 +454,7 @@ VLIB_CLI_COMMAND (upf_nwi_add_del_command, static) =
 {
  .path = "upf nwi",
  .short_help =
-  "upf nwi name <name> [table <table-id>] [del]",
+  "upf nwi name <name> [table <table-id>] [vrf <vrf-id] [ipfix-policy <name>] [del]",


missed > in vrf help, but we can keep it and fix some day

RoadRunnr

The PR is too large for an in depth review. I have a few comments, but considering the size of this beast, those could be addressed in a follow up PR.

RoadRunnr · 2022-05-12T09:11:29Z

upf/flowtable.h

+  u32 pkts_unreported;
  u64 bytes;
+  u64 bytes_unreported;
+  u64 l4_bytes;
+  u64 l4_bytes_unreported;


Sure, that applies to l4_bytes_unreported and pkts_unreported. There is no user for l4_bytes and bytes_unreported. I wonder if that is an oversight?

RoadRunnr · 2022-05-12T09:15:45Z

upf/upf_pfcp_api.c

@@ -2570,6 +2608,9 @@ handle_session_establishment_request (pfcp_msg_t * msg,
      pending->inactivity_timer.handle = ~0;
    }

+  if (ISSET_BIT (req->grp.fields, SESSION_ESTABLISHMENT_REQUEST_USER_ID))
+    memcpy (&sess->user_id, &req->user_id, sizeof (pfcp_user_id_t));


That doesn't work for the nai field. We don't use it at the moment, but to avoid problems in the future it would be better initialize it to zero in the copy.
If you wanted to copy it as well, then it needs to freed together with the session

RoadRunnr · 2022-05-12T09:18:53Z

upf/upf_ipfix_templates.c

+  IPFIX_FIELD_PROTOCOL_IDENTIFIER(F)
+
+#define IPFIX_TEMPLATE_DEFAULT_COMMON(F)	\
+  IPFIX_FIELD_MOBILE_IMSI(F)			\


what about IMSI and IMEI ?

ivan4th force-pushed the feature/ipfix branch 3 times, most recently from c6601d0 to c9bfd43 Compare February 23, 2022 14:56

miguelbf-alb reviewed Mar 7, 2022

View reviewed changes

ivan4th force-pushed the feature/ipfix branch 7 times, most recently from a73affd to a4a4a6d Compare March 12, 2022 19:41

RoadRunnr reviewed Mar 14, 2022

View reviewed changes

ivan4th force-pushed the feature/ipfix branch 5 times, most recently from 6e52b12 to 3309fa2 Compare March 15, 2022 17:16

ivan4th force-pushed the feature/vpp-2202 branch 2 times, most recently from 4068a0a to c4ed5bf Compare March 17, 2022 11:48

Base automatically changed from feature/vpp-2202 to master March 17, 2022 16:33

ivan4th force-pushed the feature/ipfix branch from 3309fa2 to 27b608e Compare March 17, 2022 21:52

ivan4th force-pushed the feature/ipfix branch from f696311 to c411793 Compare March 28, 2022 21:09

korroot added the enhancement New feature or request label Mar 29, 2022

ivan4th force-pushed the feature/ipfix branch 5 times, most recently from be03c81 to e1477dd Compare April 14, 2022 16:21

ivan4th force-pushed the feature/ipfix branch from a9e4bb5 to e707997 Compare April 30, 2022 00:39

ivan4th changed the title ~~[WiP] Initial IPFIX implementation~~ Initial IPFIX implementation May 9, 2022

ivan4th added 10 commits May 9, 2022 20:18

ipfix: add NAT fields

504eb35

format_tbcd: stop upon the 0xf nibble

638f497

Use table IDs instead of FIB indices in binapi dump calls

cdde0cd

e2e: verify table IDs in binapi calls

c20ec7a

Add 'make genbinapi' for e2e tests

8cc1dff

Make the devenv usable again for remote docker + NFS

cf228b9

cli: display table IDs instead of FIB ids in show upf nwi

398dbd5

ipfix: fix flowtable entry init

67168af

ci: reduce parallelism

96d4a3a

In IPFIX tests, there happen to be too many VPP instances active at the same time, leading to memory exhaustion.

e2e/ipfix: only test NAT fields in the TDF mode

e65d106

ivan4th force-pushed the feature/ipfix branch from 84ccde8 to e65d106 Compare May 9, 2022 16:18

ivan4th added 13 commits May 9, 2022 21:37

ipfix: fix thread safety of IPFIX contexts

1db2b00

Store full User ID instead of just IMSI in the session

05b00cc

Remove unused macro

6f6e7d1

binapi: extend IPFIX policy name field to 64 bytes

89b4735

Remove unneeded debug print

69e6b0f

e2e: make sure IPFIX delta fields work across multiple reports

b470bdb

ipfix: make it possible to disable IPFIX via FAR

32b9fff

e2e: ipfix: verify disabling IPFIX via FAR

7437e93

e2e: fix copyright notice

2fb72c0

e2e: ipfix: add proxy test

ca17a61

test: remove outdated comment

8218048

ipfix: make sure the first report isn't sent out immediately

3b00176

e2e: check that IPFIX reporting is delayed for each flow

43f0f56

Also, make sure that `postNAPTSourceTransportPort` is always correct when NAT is being used. It's necessary to postpone NAT processing upon the first packet to avoid having 0 in this field.

ivan4th force-pushed the feature/ipfix branch from e28979d to 43f0f56 Compare May 10, 2022 16:23

sergeymatov requested a review from RoadRunnr May 10, 2022 16:43

sergeymatov approved these changes May 11, 2022

View reviewed changes

RoadRunnr approved these changes May 12, 2022

View reviewed changes

korroot merged commit 21c3fe8 into master May 12, 2022

korroot deleted the feature/ipfix branch May 12, 2022 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial IPFIX implementation #219

Initial IPFIX implementation #219

ivan4th commented Feb 22, 2022 •

edited

Loading

miguelbf-alb Mar 7, 2022 •

edited

Loading

miguelbf-alb Mar 7, 2022 •

edited

Loading

ivan4th Apr 15, 2022

RoadRunnr Mar 14, 2022

RoadRunnr Mar 14, 2022

ivan4th Apr 15, 2022

RoadRunnr Mar 14, 2022

ivan4th Apr 15, 2022

RoadRunnr Mar 14, 2022

ivan4th Apr 15, 2022

sergeymatov left a comment

sergeymatov May 11, 2022

RoadRunnr left a comment

RoadRunnr May 12, 2022

RoadRunnr May 12, 2022

RoadRunnr May 12, 2022

Initial IPFIX implementation #219

Initial IPFIX implementation #219

Conversation

ivan4th commented Feb 22, 2022 • edited Loading

miguelbf-alb Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

miguelbf-alb Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeymatov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RoadRunnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan4th commented Feb 22, 2022 •

edited

Loading

miguelbf-alb Mar 7, 2022 •

edited

Loading

miguelbf-alb Mar 7, 2022 •

edited

Loading