From 580a98b798fe14ce7a9013df2d242afcb66f93a1 Mon Sep 17 00:00:00 2001 From: "G. Paul Ziemba" Date: Mon, 17 Jul 2023 09:31:06 -0700 Subject: lib: zapi PBR common encode/decode bgpd, pbrd: use common pbr encoder zebra: use common pbr decoder tests: pbr_topo1: check more filter fields Purpose: 1. Reduce likelihood of zapi format mismatches when adding PBR fields due to multiple parallel encoder implementations 2. Encourage common PBR structure usage among various daemons 3. Reduce coding errors via explicit per-field enable flags Signed-off-by: G. Paul Ziemba --- pbrd/pbr_zebra.c | 159 ++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 100 insertions(+), 59 deletions(-) (limited to 'pbrd') diff --git a/pbrd/pbr_zebra.c b/pbrd/pbr_zebra.c index 28d89b0b5..f4bf287c5 100644 --- a/pbrd/pbr_zebra.c +++ b/pbrd/pbr_zebra.c @@ -483,27 +483,9 @@ void pbr_send_rnh(struct nexthop *nhop, bool reg) } } -static void pbr_encode_pbr_map_sequence_prefix(struct stream *s, - struct prefix *p, - unsigned char family) -{ - struct prefix any; - - if (!p) { - memset(&any, 0, sizeof(any)); - any.family = family; - p = &any; - } - - stream_putc(s, p->family); - stream_putc(s, p->prefixlen); - stream_put(s, &p->u.prefix, prefix_blen(p)); -} -static void -pbr_encode_pbr_map_sequence_vrf(struct stream *s, - const struct pbr_map_sequence *pbrms, - const struct interface *ifp) +static uint32_t pbr_map_sequence_vrf(const struct pbr_map_sequence *pbrms, + const struct interface *ifp) { struct pbr_vrf *pbr_vrf; @@ -514,66 +496,130 @@ pbr_encode_pbr_map_sequence_vrf(struct stream *s, if (!pbr_vrf) { DEBUGD(&pbr_dbg_zebra, "%s: VRF not found", __func__); - return; + return 0; } - stream_putl(s, pbr_vrf->vrf->data.l.table_id); + return pbr_vrf->vrf->data.l.table_id; + } +/* + * 230716 gpz note: it would be worthwhile for pbrd to represent + * its rules internally using the lib/pbr.h structures to help + * move toward a more common structure across pbrd, bgpd, and zebra. + */ static bool pbr_encode_pbr_map_sequence(struct stream *s, struct pbr_map_sequence *pbrms, struct interface *ifp) { - unsigned char family; + struct pbr_rule r; + uint8_t family; + + /* + * There seems to be some effort in pbr_vty.c to keep the three + * copies of "family" equal. Not sure if the reason goes beyond + * ensuring consistency in ZAPI encoding. In any case, it might + * be handled better as an internal matter for the encoder (TBD). + */ family = AF_INET; if (pbrms->family) family = pbrms->family; - stream_putl(s, pbrms->seqno); - stream_putl(s, pbrms->ruleno); - stream_putl(s, pbrms->unique); - - stream_putl(s, pbrms->filter_bm); - - stream_putc(s, pbrms->ip_proto); /* The ip_proto */ - pbr_encode_pbr_map_sequence_prefix(s, pbrms->src, family); - stream_putw(s, pbrms->src_prt); - pbr_encode_pbr_map_sequence_prefix(s, pbrms->dst, family); - stream_putw(s, pbrms->dst_prt); - stream_putc(s, pbrms->dsfield); - stream_putl(s, pbrms->mark); - /* PCP */ - if (CHECK_FLAG(pbrms->filter_bm, PBR_FILTER_PCP)) - stream_putc(s, pbrms->match_pcp); + if (pbrms->src) + assert(family == pbrms->src->family); + if (pbrms->dst) + assert(family == pbrms->dst->family); + + /* + * Convert struct pbr_map_sequence to canonical form + */ + memset(&r, 0, sizeof(r)); + r.seq = pbrms->seqno; + r.priority = pbrms->ruleno; + r.unique = pbrms->unique; + + /* filter */ + r.filter.filter_bm = pbrms->filter_bm; + if (pbrms->src) + r.filter.src_ip = *pbrms->src; + else + r.filter.src_ip.family = family; + if (pbrms->dst) + r.filter.dst_ip = *pbrms->dst; else - stream_putc(s, 0); - stream_putw(s, pbrms->action_pcp); - /* VLAN */ - stream_putw(s, pbrms->match_vlan_id); - stream_putw(s, pbrms->match_vlan_flags); + r.filter.dst_ip.family = family; + r.filter.src_port = pbrms->src_prt; + r.filter.dst_port = pbrms->dst_prt; + r.filter.pcp = pbrms->match_pcp; + r.filter.vlan_id = pbrms->match_vlan_id; + r.filter.vlan_flags = pbrms->match_vlan_flags; + r.filter.dsfield = pbrms->dsfield; + r.filter.fwmark = pbrms->mark; + r.filter.ip_proto = pbrms->ip_proto; - stream_putw(s, pbrms->action_vlan_id); - stream_putw(s, pbrms->action_vlan_flags); - stream_putl(s, pbrms->action_queue_id); + /* + * Fix up filter flags for now, since PBRD doesn't maintain + * them yet (aside from PBR_FILTER_PCP) + */ + if (!is_default_prefix(&r.filter.src_ip)) + SET_FLAG(r.filter.filter_bm, PBR_FILTER_SRC_IP); + if (!is_default_prefix(&r.filter.dst_ip)) + SET_FLAG(r.filter.filter_bm, PBR_FILTER_DST_IP); + if (r.filter.src_port) + SET_FLAG(r.filter.filter_bm, PBR_FILTER_SRC_PORT); + if (r.filter.dst_port) + SET_FLAG(r.filter.filter_bm, PBR_FILTER_DST_PORT); + if (r.filter.vlan_id) + SET_FLAG(r.filter.filter_bm, PBR_FILTER_VLAN_ID); + if (r.filter.vlan_flags) + SET_FLAG(r.filter.filter_bm, PBR_FILTER_VLAN_FLAGS); + if (r.filter.dsfield) + SET_FLAG(r.filter.filter_bm, PBR_FILTER_DSFIELD); + if (r.filter.fwmark) + SET_FLAG(r.filter.filter_bm, PBR_FILTER_FWMARK); + if (r.filter.ip_proto) + SET_FLAG(r.filter.filter_bm, PBR_FILTER_IP_PROTOCOL); + + /* actions */ - /* if the user does not use the command "set vrf name |unchanged" - * then pbr_encode_pbr_map_sequence_vrf will not be called + /* + * PBR should maintain its own set of action flags that we + * can copy here instead of trying to infer from magic values. */ + SET_FLAG(r.action.flags, PBR_ACTION_TABLE); /* always valid */ + if (PBR_MAP_UNDEFINED_QUEUE_ID != pbrms->action_queue_id) + SET_FLAG(r.action.flags, PBR_ACTION_QUEUE_ID); + if (0 != pbrms->action_pcp) + SET_FLAG(r.action.flags, PBR_ACTION_PCP); + if (0 != pbrms->action_vlan_id) + SET_FLAG(r.action.flags, PBR_ACTION_VLAN_ID); + if (0 != pbrms->action_vlan_flags) + SET_FLAG(r.action.flags, PBR_ACTION_VLAN_FLAGS); - /* these statement get a table id */ + /* + * if the user does not use the command "set vrf name unchanged" + * then pbr_encode_pbr_map_sequence_vrf will not be called + */ if (pbrms->vrf_unchanged || pbrms->vrf_lookup) - pbr_encode_pbr_map_sequence_vrf(s, pbrms, ifp); + r.action.table = pbr_map_sequence_vrf(pbrms, ifp); else if (pbrms->nhgrp_name) - stream_putl(s, pbr_nht_get_table(pbrms->nhgrp_name)); + r.action.table = pbr_nht_get_table(pbrms->nhgrp_name); else if (pbrms->nhg) - stream_putl(s, pbr_nht_get_table(pbrms->internal_nhg_name)); + r.action.table = pbr_nht_get_table(pbrms->internal_nhg_name); else { /* Not valid for install without table */ return false; } - stream_put(s, ifp->name, INTERFACE_NAMSIZ); + r.action.queue_id = pbrms->action_queue_id; + r.action.pcp = pbrms->action_pcp; + r.action.vlan_id = pbrms->action_vlan_id; + r.action.vlan_flags = pbrms->action_vlan_flags; + + strlcpy(r.ifname, ifp->name, sizeof(r.ifname)); + + zapi_pbr_rule_encode(s, &r); return true; } @@ -607,11 +653,6 @@ bool pbr_send_pbr_map(struct pbr_map_sequence *pbrms, install ? ZEBRA_RULE_ADD : ZEBRA_RULE_DELETE, VRF_DEFAULT); - /* - * We are sending one item at a time at the moment - */ - stream_putl(s, 1); - DEBUGD(&pbr_dbg_zebra, "%s: %s %s seq %u %d %s %u", __func__, install ? "Installing" : "Deleting", pbrm->name, pbrms->seqno, install, pmi->ifp->name, pmi->delete); -- cgit v1.2.3