[UPG-NAT] Controlled CG-NAT #122

sergeymatov · 2021-05-19T11:49:47Z

In order to allow UPG perform CG-NAT function following changes has been introduced:
To UPG:

Ability to allocate and manage NAT Pools
UE IP Address pool information IE to communicate NAT Pools to SMC
BBF PFCP IEs to manage NAT Pool lookup and applying NAT to given UE IP
UPG can manage NAT bindings configured for VPP NAT plugin

To NAT plugin:

NAT bindings allocation/deletion mechanics exposed to UPG
Binding lookup via bihash
Override ED-NAT slow-path address and port allocation in respect to NAT binding created for given user address

RoadRunnr · 2021-05-19T12:02:19Z

upf/pfcp.c

+  length -= 2;
+
+  if (length < 2)
+    return PFCP_CAUSE_INVALID_LENGTH;
+


teh length check is already covered by line 4766

RoadRunnr · 2021-05-19T12:03:03Z

upf/pfcp.c

+  pfcp_bbf_up_function_features_t *n =
+    va_arg (*args, pfcp_bbf_up_function_features_t *);
+
+  s = format (s, "NAT:%u", (*n & BBF_UP_NAT));


Suggested change

s = format (s, "NAT:%u", (*n & BBF_UP_NAT));

s = format (s, "NAT:%u", !!(*n & BBF_UP_NAT));

RoadRunnr · 2021-05-19T12:06:10Z

upf/pfcp.c

+{
+  u64 *v = p;
+
+  if (length % 2 != 0 || length < 2)


minimum length of this IE is 4 bytes

check line updated

RoadRunnr · 2021-05-19T12:07:59Z

upf/pfcp.c

+  put_u16_little (*vec, *v);
+  put_u16_little (*vec, *v >> 16);


Suggested change

put_u16_little (*vec, *v);

put_u16_little (*vec, *v >> 16);

put_u32_little (*vec, *v);

oh, missed this. thanks

RoadRunnr · 2021-05-19T12:10:10Z

upf/pfcp.c

+  if (ISSET_BIT(*v, IP_VERSION_4))
+    s = format (s, "IPv4");
+  else
+    s = format (s, "IPv6");


both flags can be set (v4 and v6)

upf/upf_pfcp.c

upf/upf_pfcp_api.c

RoadRunnr · 2021-05-19T12:42:54Z

upf/upf_pfcp_api.c

+    {
+      if (!(vec_is_equal (np->network_instance, ue_p->nwi_name)))
+	continue;
+      pfcp_bbf_nat_port_block_t *block;


local variable definition should not follow code

this has not been addressed

Sorry, missed this here but fixed for stable branch.

upf/upf_pfcp_api.c

RoadRunnr · 2021-05-19T12:46:49Z

upf/upf_pfcp_api.c

+  do
+    {
+      err =
+	upf_nat_create_binding (user_ip, addr->ext_addr, port_start, port_end,
+				np->vrf_id);
+      if (!err)
+	{
+	  addr->used_blocks += 1;
+	  sx->nat_addr = addr;
+	  created_binding->block = vec_dup (np->name);
+	  created_binding->outside_addr.as_u32 = addr->ext_addr.as_u32;
+	  created_binding->port_range.start_port = port_start;
+	  created_binding->port_range.end_port = port_end;
+	  SET_BIT (created_binding->grp.fields,
+		   TP_CREATED_BINDING_NAT_PORT_BLOCK);
+	  SET_BIT (created_binding->grp.fields,
+		   TP_CREATED_BINDING_NAT_OUTSIDE_ADDRESS);
+	  SET_BIT (created_binding->grp.fields,
+		   TP_CREATED_BINDING_NAT_EXTERNAL_PORT_RANGE);
+	  return 0;
+	}
+      port_start += np->port_block_size;
+      port_end = port_start + np->port_block_size;
+    }
+  while (port_end < np->max_port);


I hate this trial and error approach to finding a suitable block

I've removed this try-err thing out from UPG code and make logic for finding port block inside NAT code.
NAT plugin in full of such constructions in terms of port selection. Now its a part of binding validation procedure.

ivan4th

The major problem I see is the lack of any tests (integration or e2e), but sadly this will have to wait for now.
Other than that, one of the problems I found looks a potential bug, others being some nitpicking

upf/upf_pfcp_api.c

ivan4th · 2021-05-26T15:55:37Z

upf/upf_pfcp.c

+
+  ASSERT (ap->used_blocks > 0);
+
+  ap->used_blocks -= 1;


upf/upf_pfcp.c

ivan4th · 2021-05-26T16:00:20Z

upf/test/test_upf.py

@@ -359,7 +367,6 @@ def associate(self):
            IE_NodeId(id_type="FQDN", id="ergw")
        ]), PFCPAssociationSetupResponse)
        self.assertEqual(CauseValues[resp[IE_Cause].cause], "Request accepted")
-        self.assertIn(b"vpp", resp[IE_EnterpriseSpecific].data)


Is this change somehow related to this PR? This is actually a problem (#104), as the build id should contain upg not vpp, but I thought of fixing it separately and not by removing this check, but rather by fixing the id itself and replacing vpp with upg here

In general, association setup response checks should be enhanced with:

proper build ID check

BBF UserPlane Features checks
I think it's worth a separate review

ivan4th · 2021-05-28T12:27:26Z

LGTM

RoadRunnr · 2021-06-02T10:13:01Z

upf/upf.c

+  np = pool_elt_at_index (gtm->nat_pools, p[0]);
+
+  return np;


Suggested change

np = pool_elt_at_index (gtm->nat_pools, p[0]);

return np;

return pool_elt_at_index (gtm->nat_pools, p[0]);

RoadRunnr · 2021-06-02T10:15:31Z

upf/upf.c

+{
+  u32 i = 0;
+
+  vec_reset_length (np->addresses);


to avoid unnecessary fragmentation, you could used vec_validate to make sure the vector is large enough to hold all entries before going into the loop.

RoadRunnr · 2021-06-02T10:19:30Z

upf/upf_cli.c

+{
+  .path = "upf nat pool",
+  .short_help =
+  "upf nat pool nwi <nwi-name> start <ip4-addr> end <ip4-addr> min_port <min-port> max_port <max-port> block_size <port-block-size> vrf <vrf-id> name <name> [del]",


A NWI currently uniquely identifies a VRF, it the extra VRF really needed or different from the NWI VRF?

We are operating nwi name w/o nwi lookup itself. I will add TODO in a code, but removing vrf out will create a cascade of changes to charts.

"because something somewhere else would need to be redone" is not an argument for leaving this code in a inconsistent state. A TODO for this is only acceptable if it is followed up with some urgency.

VRF removed as redundant CLI argument

RoadRunnr · 2021-06-02T10:21:52Z

upf/upf_pfcp_api.c

+    {
+      if (!(vec_is_equal (np->network_instance, ue_p->nwi_name)))
+	continue;
+      pfcp_bbf_nat_port_block_t *block;


this has not been addressed

NAT plugin dedicated changes. ED NAT address and port selection done based on bindings created by UPG plugin.

Fixed PFCP IE ordering for BBF/TP messages. Fixed minor issues in UPG NAT processing.

Also populate Reencode PFCP test .txt files with missing TP IEs.

ivan4th · 2021-06-02T14:51:46Z

LGTM from my side

RoadRunnr

I think the CLI (nwi vs. vrf) should be sorted out before this is merged to master

RoadRunnr · 2021-06-02T15:12:07Z

upf/upf.c

+	return VNET_API_ERROR_NO_SUCH_ENTRY;
+
+      nat_pool = pool_elt_at_index (gtm->nat_pools, p[0]);
+      //TBD: nat pool cleanup upf_delete_nat_addresses(nat_pool);


removing a NAT pool that used entries would crash at some, right?

RoadRunnr · 2021-06-02T15:16:51Z

upf/upf_cli.c

+{
+  .path = "upf nat pool",
+  .short_help =
+  "upf nat pool nwi <nwi-name> start <ip4-addr> end <ip4-addr> min_port <min-port> max_port <max-port> block_size <port-block-size> vrf <vrf-id> name <name> [del]",


"because something somewhere else would need to be redone" is not an argument for leaving this code in a inconsistent state. A TODO for this is only acceptable if it is followed up with some urgency.

RoadRunnr · 2021-06-07T07:32:08Z

upf/upf_cli.c

+      // something like this). To avoid this, two strings could be picked
+      // in single unformat call. Probably Address Sanitaizer should be
+      // used to identify problems with unformatting input line
+      else if (unformat (line_input, "nwi %_%v%_ name %_%v%_", &nwi_s, &name))


it seems that "%s" does not suffer from this problem. It would therefore be possible to scan the names as string and then convert them to vectors.
That would IMHO be a better workaround.

Looking at the full function also makes me wonder if the fact that name is not initialized to zero might play a role in the problem.

Initiation of name vectors to 0 resolves the issue, so I've removed W/A.
Thanks for good notice

Changes: - Remove redundant vrf_id from "upf nat pool" cli from code and upf scapy tests. - Free nat addresses vector on nat pool deletion. - Code optimization and cleanup.

Sergey Matov added 2 commits May 18, 2021 16:06

[UPG NAT] UPG NAT function initial commit

1266c4e

[UPG NAT] UPG-CG-NAT: add bihash lookup for bindings

cd605c5

sergeymatov requested review from ivan4th and RoadRunnr May 19, 2021 11:49

RoadRunnr requested changes May 19, 2021

View reviewed changes

sergeymatov mentioned this pull request May 19, 2021

[stable backport] [UPG-NAT] Controlled NAT-Function #123

Merged

sergeymatov force-pushed the exp/cg-nat branch from 39926cf to ee6eeb9 Compare May 24, 2021 14:23

sergeymatov requested a review from RoadRunnr May 24, 2021 14:46

ivan4th suggested changes May 26, 2021

View reviewed changes

sergeymatov force-pushed the exp/cg-nat branch from ee6eeb9 to 3dbc14f Compare May 27, 2021 07:00

sergeymatov requested a review from ivan4th May 27, 2021 15:12

sergeymatov force-pushed the exp/cg-nat branch from 3dbc14f to 3bc51a2 Compare May 28, 2021 06:23

ivan4th approved these changes May 28, 2021

View reviewed changes

sergeymatov force-pushed the exp/cg-nat branch from 93fbcc1 to 39b8549 Compare June 2, 2021 06:40

RoadRunnr requested changes Jun 2, 2021

View reviewed changes

Sergey Matov added 4 commits June 2, 2021 14:27

BBF PFCP NAT functionality

df03b2b

[UPG NAT] Controlled NAT function

7274c27

NAT plugin dedicated changes. ED NAT address and port selection done based on bindings created by UPG plugin.

[UPG-NAT] Improve PFCP handling

5cbd250

Fixed PFCP IE ordering for BBF/TP messages. Fixed minor issues in UPG NAT processing.

Fix PFCP TP End time name

356c734

Also populate Reencode PFCP test .txt files with missing TP IEs.

sergeymatov force-pushed the exp/cg-nat branch from 4172037 to 356c734 Compare June 2, 2021 11:05

sergeymatov requested a review from RoadRunnr June 2, 2021 11:32

RoadRunnr requested changes Jun 2, 2021

View reviewed changes

sergeymatov force-pushed the exp/cg-nat branch 4 times, most recently from d81f68c to 169da0c Compare June 6, 2021 03:48

sergeymatov requested a review from RoadRunnr June 6, 2021 03:50

RoadRunnr reviewed Jun 7, 2021

View reviewed changes

[UPG-NAT] Minor code refactor

006f706

Changes: - Remove redundant vrf_id from "upf nat pool" cli from code and upf scapy tests. - Free nat addresses vector on nat pool deletion. - Code optimization and cleanup.

sergeymatov force-pushed the exp/cg-nat branch from 169da0c to 006f706 Compare June 7, 2021 08:59

sergeymatov requested a review from RoadRunnr June 7, 2021 09:00

ivan4th added the enhancement New feature or request label Jun 8, 2021

RoadRunnr approved these changes Jun 8, 2021

View reviewed changes

sergeymatov merged commit 7ac1fcb into master Jun 8, 2021

sergeymatov deleted the exp/cg-nat branch June 8, 2021 11:29

	s = format (s, "NAT:%u", (*n & BBF_UP_NAT));
	s = format (s, "NAT:%u", !!(*n & BBF_UP_NAT));

	put_u16_little (vec, v);
	put_u16_little (vec, v >> 16);
	put_u32_little (vec, v);

[UPG-NAT] Controlled CG-NAT #122

[UPG-NAT] Controlled CG-NAT #122

Conversation

sergeymatov commented May 19, 2021

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

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

ivan4th 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

Choose a reason for hiding this comment

ivan4th commented May 28, 2021

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

ivan4th commented Jun 2, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment