Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pbrd: add vty support for vlan filtering and send to zebra #13514

Closed
wants to merge 5 commits into from
Closed

pbrd: add vty support for vlan filtering and send to zebra #13514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b146fd
pbrd: resolve rebasing conflict
joshua-werner Jun 15, 2023
e270175
pbrd: address PR comments
joshua-werner May 24, 2023
32cf0c3
.gitignore: undo changes to address PR comments
joshua-werner May 24, 2023
f20b116
pbrd: adopt style suggestions
joshua-werner Jun 2, 2023
b21c325
pbrd: add logic to distinguish between ignoring pcp and matching pcp 0
joshua-werner Jun 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,5 @@ refix
.kitchen
.emacs.desktop*

/test-suite.log
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a duplicated line

pceplib/test/*.log
pceplib/test/*.trs
40 changes: 32 additions & 8 deletions doc/user/pbr.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ PBR
***

:abbr:`PBR` is Policy Based Routing. This implementation supports a very simple
interface to allow admins to influence routing on their router. At this time
you can only match on destination and source prefixes for an incoming interface.
At this point in time, this implementation will only work on Linux.
interface to allow admins to influence routing on their router. At this point in
time, this implementation will only work on Linux. Note that some
functionality (VLAN matching, packet mangling) is not supported by the default
Linux kernel provider.

.. _starting-pbr:

Expand Down Expand Up @@ -109,10 +110,12 @@ end destination.
When a incoming packet matches the destination port specified, take the
packet and forward according to the nexthops specified.

.. clicmd:: match ip-protocol [tcp|udp]
.. clicmd:: match ip-protocol PROTOCOL

When a incoming packet matches the specified ip protocol, take the
packet and forward according to the nexthops specified.
packet and forward according to the nexthops specified. Protocols are
queried from the protocols database (``/etc/protocols``; see ``man 5
protocols``).

.. clicmd:: match mark (1-4294967295)

Expand All @@ -136,7 +139,6 @@ end destination.
(ECN) field in the IP header; if this value matches then forward the packet
according to the nexthop(s) specified.


.. clicmd:: set queue-id (1-65535)

Set the egress port queue identifier for matched packets. The Linux Kernel
Expand All @@ -146,7 +148,7 @@ end destination.
.. clicmd:: set pcp (0-7)

Set the 802.1Q priority code point (PCP) for matched packets. A PCP of zero
is the defaul (nominally, "best effort"). The Linux Kernel provider does not
is the default (nominally, "best effort"). The Linux Kernel provider does not
currently support packet mangling, so this field will be ignored unless
another provider is used.

Expand All @@ -161,6 +163,28 @@ end destination.
Strip inner vlan tags from matched packets. The Linux Kernel provider does not currently support packet mangling, so this field will be ignored unless another provider is used. It is invalid to specify both a `strip` and `set
vlan` action.

.. clicmd:: match pcp (0-7)

Match packets according to the 802.1Q Priority Code Point. Zero is the
default (nominally, "best effort"). Note that the Linux kernel provider
does not support matching PCPs, so this field will be ignored unless other
providers are used.

.. clicmd:: match vlan (1-4094)

Match packets according to their VLAN (802.1Q) identifier. Note that VLAN
IDs 0 and 4095 are reserved. The Linux kernel provider does not provide
VLAN-matching facilities, so this field will be ignored unless other
providers are used.

.. clicmd:: match vlan (tagged|untagged|untagged-or-zero)

Match packets according to whether or not they have a VLAN tag. Use
`untagged-or-zero` to also match packets with the reserved VLAN ID of 0
(indicating an untagged frame which includes other 802.1Q fields). The
Linux kernel provider does not provide VLAN-matching facilities, so this
field will be ignored unless other providers are used.

.. clicmd:: set nexthop-group NAME

Use the nexthop-group NAME as the place to forward packets when the match
Expand Down Expand Up @@ -227,7 +251,7 @@ end destination.
| nexthopGroup | This policy's nexthop group (if relevant) | Object |
+-----------------+-------------------------------------------+---------+

Finally, the ``nexthopGroup`` object above cotains information we know
Finally, the ``nexthopGroup`` object above contains information we know
about the configured nexthop for this policy:

+---------------------+--------------------------------------+---------+
Expand Down
38 changes: 24 additions & 14 deletions lib/pbr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: GPL-2.0-or-later
/* Policy Based Routing (PBR) main header
* Copyright (C) 2018 6WIND
* Portions:
* Copyright (c) 2021 The MITRE Corporation.
*/

#ifndef _PBR_H
Expand All @@ -25,30 +27,38 @@ extern "C" {
* specified.
*/
struct pbr_filter {
uint32_t filter_bm; /* not encoded by zapi
*/
#define PBR_FILTER_SRC_IP (1 << 0)
#define PBR_FILTER_DST_IP (1 << 1)
#define PBR_FILTER_SRC_PORT (1 << 2)
#define PBR_FILTER_DST_PORT (1 << 3)
#define PBR_FILTER_FWMARK (1 << 4)
#define PBR_FILTER_PROTO (1 << 5)
#define PBR_FILTER_SRC_PORT_RANGE (1 << 6)
#define PBR_FILTER_DST_PORT_RANGE (1 << 7)
#define PBR_FILTER_DSFIELD (1 << 8)
#define PBR_FILTER_IP_PROTOCOL (1 << 9)
uint32_t filter_bm; /* not encoded by zapi */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may need to encode these bits - how can we tell the difference between "not present" and "value is zero" otherwise?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, can you explain a little more in depth what you mean? The comment was only changed in the original MR to be on one line instead of 2. I have reverted it per a ci:cd warning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anything related to the issues that Paul and I asked about earlier - the encoding issues?

#define PBR_FILTER_SRC_IP (1 << 0)
#define PBR_FILTER_DST_IP (1 << 1)
#define PBR_FILTER_SRC_PORT (1 << 2)
#define PBR_FILTER_DST_PORT (1 << 3)
#define PBR_FILTER_FWMARK (1 << 4)
#define PBR_FILTER_PROTO (1 << 5)
#define PBR_FILTER_SRC_PORT_RANGE (1 << 6)
#define PBR_FILTER_DST_PORT_RANGE (1 << 7)
#define PBR_FILTER_DSFIELD (1 << 8)
#define PBR_FILTER_IP_PROTOCOL (1 << 9)
#define PBR_FILTER_PCP (1 << 10)
#define PBR_FILTER_VLAN_FLAGS (1 << 11)
#define PBR_FILTER_VLAN_ID (1 << 12)

#define PBR_DSFIELD_DSCP (0xfc) /* Upper 6 bits of DS field: DSCP */
#define PBR_DSFIELD_ECN (0x03) /* Lower 2 bits of DS field: BCN */
#define PBR_PCP (0x07) /* 3-bit value 0..7 for prioritization*/

/* Source and Destination IP address with masks. */
/* Source and Destination IP address with masks */
struct prefix src_ip;
struct prefix dst_ip;

/* Source and Destination higher-layer (TCP/UDP) port numbers. */
/* Source and Destination higher-layer (TCP/UDP) port numbers */
uint16_t src_port;
uint16_t dst_port;

/* Filter by VLAN and prioritization */
uint8_t pcp;
uint16_t vlan_id;
uint16_t vlan_flags;

/* Filter by Differentiated Services field */
uint8_t dsfield; /* DSCP (6 bits) & ECN (2 bits) */

Expand Down
40 changes: 32 additions & 8 deletions pbrd/pbr_map.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
* PBR-map Code
* Copyright (C) 2018 Cumulus Networks, Inc.
* Donald Sharp
* Portions:
* Copyright (c) 2021 The MITRE Corporation.
*/
#include <zebra.h>

Expand Down Expand Up @@ -101,6 +103,24 @@ static bool pbrms_is_installed(const struct pbr_map_sequence *pbrms,
return false;
}

void pbr_set_match_clause_for_pcp(struct pbr_map_sequence *pbrms, uint8_t pcp)
{
if (pbrms) {
pbrms->match_pcp = pcp;
pbr_map_check(pbrms, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work for matching PCP==0 ? Unless I missed something in the code, there is no use of a separate flag to indicate that PCP should be matched vs. ignored.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you're right and that currently match pcp zero is simply ignored since there is a check in pbr_vty.c to see if it is non-zero. My initial thought is to set the default value to 8, rather than 0, in the pbrms_get and then handle 8 as a special ignore case and allow 0 as a match case. However, this will be difficult to test since, again per Eli, the default Linux kernel provider does not support matching PCPs, so actually testing this case differential may be difficult. Might be good to have a quick meeting to discuss if you have any availability?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your proposal could work. It would mean that the check for "are we matching" is something like (pcp != 8) and then the value to match against is (pcp & 7).

Another way might be to set the "8" bit for every match and unset for no match. Then the "are we matching check" (e.g., in zapi_msg.c) remains (pcp != 0) and the value to match is (pcp & 7).

I don't have a strong opinion either way - please pick whichever seems clearer to you.

}
}

void pbr_set_match_clause_for_vlan(struct pbr_map_sequence *pbrms,
uint16_t vlan_id, uint16_t vlan_flags)
{
if (pbrms) {
pbrms->match_vlan_id = vlan_id;
pbrms->match_vlan_flags = vlan_flags;
pbr_map_check(pbrms, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same concern as above for PCP: How does the code distinguish between "match VLAN_ID==0" and "ignore VLAN_ID" ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, VLAN_ID should only be a value between 1-4094 (see https://datatracker.ietf.org/doc/html/rfc5517). ID 0 is reserved, and handled by the untagged_0 match, so a value of zero is always ignored unless you do match match untagged-or-zero, rather than match 0. Eli documented this in the .rst and it is has special casing already in pbr_vty.c where it specifically excludes the value of 0 and matches instead on the constants defined in the pbr header (PBR_MAP_VLAN_UNTAGGED_0, PBR_MAP_VLAN_UNTAGGED, etc.). So, from my understanding these cases are distinct already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - it's a good explanation and I agree.

}
}

/* If any sequence is installed on the interface, assume installed */
static bool
pbr_map_interface_is_installed(const struct pbr_map *pbrm,
Expand Down Expand Up @@ -486,9 +506,9 @@ uint8_t pbr_map_decode_dscp_enum(const char *name)

struct pbr_map_sequence *pbrms_get(const char *name, uint32_t seqno)
{
struct pbr_map *pbrm;
struct pbr_map_sequence *pbrms;
struct listnode *node;
struct pbr_map *pbrm = NULL;
struct pbr_map_sequence *pbrms = NULL;
struct listnode *node = NULL;

pbrm = pbrm_find(name);
if (!pbrm) {
Expand Down Expand Up @@ -526,6 +546,10 @@ struct pbr_map_sequence *pbrms_get(const char *name, uint32_t seqno)
pbrms->ruleno = pbr_nht_get_next_rule(seqno);
pbrms->parent = pbrm;

pbrms->match_vlan_id = 0;
pbrms->match_vlan_flags = 0;
pbrms->match_pcp = PBR_MAP_IGNORE_PCP;

pbrms->action_vlan_id = 0;
pbrms->action_vlan_flags = 0;
pbrms->action_pcp = 0;
Expand Down Expand Up @@ -594,10 +618,11 @@ pbr_map_sequence_check_nexthops_valid(struct pbr_map_sequence *pbrms)

static void pbr_map_sequence_check_not_empty(struct pbr_map_sequence *pbrms)
{
if (!pbrms->src && !pbrms->dst && !pbrms->mark && !pbrms->dsfield
&& !pbrms->action_vlan_id && !pbrms->action_vlan_flags
&& !pbrms->action_pcp
&& pbrms->action_queue_id == PBR_MAP_UNDEFINED_QUEUE_ID)
if (!pbrms->src && !pbrms->dst && !pbrms->mark && !pbrms->dsfield &&
!pbrms->match_pcp && !pbrms->action_pcp && !pbrms->match_vlan_id &&
!pbrms->match_vlan_flags && !pbrms->action_vlan_id &&
!pbrms->action_vlan_flags &&
pbrms->action_queue_id == PBR_MAP_UNDEFINED_QUEUE_ID)
pbrms->reason |= PBR_MAP_INVALID_EMPTY;
}

Expand Down Expand Up @@ -734,7 +759,6 @@ void pbr_map_policy_delete(struct pbr_map *pbrm, struct pbr_map_interface *pmi)
struct pbr_map_sequence *pbrms;
bool sent = false;


for (ALL_LIST_ELEMENTS_RO(pbrm->seqnumbers, node, pbrms))
if (pbr_send_pbr_map(pbrms, pmi, false, true))
sent = true; /* rule removal sent to zebra */
Expand Down
16 changes: 16 additions & 0 deletions pbrd/pbr_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,17 @@ struct pbr_map_sequence {
*/
unsigned char family;

uint8_t match_pcp;
uint16_t match_vlan_id;

#define PBR_MAP_VLAN_NO_WILD 0
#define PBR_MAP_VLAN_TAGGED (1 << 0)
#define PBR_MAP_VLAN_UNTAGGED (1 << 1)
#define PBR_MAP_VLAN_UNTAGGED_0 (1 << 2)
#define PBR_MAP_IGNORE_PCP 8

uint16_t match_vlan_flags;

/*
* Use interface's vrf.
*/
Expand Down Expand Up @@ -222,4 +233,9 @@ extern void pbr_map_check_vrf_nh_group_change(const char *nh_group,
extern void pbr_map_check_interface_nh_group_change(const char *nh_group,
struct interface *ifp,
ifindex_t oldifindex);
extern void pbr_set_match_clause_for_vlan(struct pbr_map_sequence *pbrms,
uint16_t vlan_id,
uint16_t vlan_flags);
extern void pbr_set_match_clause_for_pcp(struct pbr_map_sequence *pbrms,
uint8_t pcp);
#endif
Loading