Hi Dumitru, thank you for the fast review. I'll post v6 momentarily, I just want to do quick fresh end-to-end setup/test. I also want to add that you were correct about the ARP/ND. It didn't work properly and it was living off the entries populated by unicast traffic from the peer, however the entries were flapping wildly and it was discovered when I threw the BFD into the mix (real blessing in disguise). The slower BFD protocol was able to deal with it without disconnects, but BFD was disconnecting every once in a while. Hence the `clone`rules for the ARP replies and IPv6 NAs.
On Fri, 2024-08-09 at 08:19 +0200, Dumitru Ceara wrote: > On 8/9/24 04:45, Martin Kalcok wrote: > > This change adds two new LRP options: > > * routing-protocol-redirect > > * routing-protocols > > > > These allow redirection of a routing protocol traffic to > > an Logical Switch Port. This enables external routing daemons > > to listen on an interface bound to an LSP and effectively act > > as if they were listening on (and speaking from) LRP's IP address. > > > > Option 'routing-protocols' takes a comma-separated list of routing > > protocols whose traffic should be redirected. Currently supported > > are BGP (tcp 179) and BFD (udp 3784). > > > > Option 'routing-protocol-redirect' expects a string with an LSP > > name. > > > > When both of these options are set, any traffic entering LS > > that's destined for LRP's IP addresses (including IPv6 LLA) and > > routing protocol's port number, is redirected to the LSP specified > > in the 'routing-protocol-redirect' value. > > > > NOTE: this feature is experimental and may be subject to > > removal/change in the future. > > > > Signed-off-by: Martin Kalcok <[email protected]> > > --- > > Hi Martin, > > Thanks for v5! Unfortunately it needs a rebase. > > Also there were some minor things that need to be fixed in the man > pages. While at it I have a few other small comments. > > > > > v5 includes: > > * change from pure BGP redirect to a configurable protocol > > redirection > > * fixes issue with local routing daemon not being able to receive > > reply > > traffic when contacting its peer. > > * ARP and ND traffic is cloned to the redirected port, to > > properly populate > > information about its neighbors. > > > > > > northd/northd.c | 217 > > ++++++++++++++++++++++++++++++++++++++++ > > northd/northd.h | 7 ++ > > northd/ovn-northd.8.xml | 54 ++++++++++ > > ovn-nb.xml | 42 ++++++++ > > tests/ovn-northd.at | 93 +++++++++++++++++ > > tests/system-ovn.at | 100 ++++++++++++++++++ > > 6 files changed, 513 insertions(+) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 4353df07d..39b1998fd 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -13048,6 +13048,221 @@ build_arp_resolve_flows_for_lrp(struct > > ovn_port *op, > > } > > } > > > > +static void > > +build_r_p_redirect_rule__( > > I think I prefer a more explicit name, e.g., > build_routing_protocols_redirect_rule__(). > > > + const char *s_addr, const char *redirect_port_name, int > > protocol_port, > > + const char *proto, bool is_ipv6, struct ovn_port *ls_peer, > > + struct lflow_table *lflows, struct ds *match, struct ds > > *actions) > > +{ > > + int ip_ver = is_ipv6 ? 6 : 4; > > + ds_clear(actions); > > + ds_put_format(actions, "outport = \"%s\"; output;", > > redirect_port_name); > > + > > + /* Redirect packets in the input pipeline destined for LR's IP > > + * and the routing protocol's port to the LSP specified in > > + * 'routing-protocol-redirect' option.*/ > > + ds_clear(match); > > + ds_put_format(match, "ip%d.dst == %s && %s.dst == %d", ip_ver, > > s_addr, > > + proto, protocol_port); > > + ovn_lflow_add(lflows, ls_peer->od, S_SWITCH_IN_L2_LKUP, 100, > > + ds_cstr(match), > > + ds_cstr(actions), > > + ls_peer->lflow_ref); > > + > > + /* To accomodate "peer" nature of the routing daemons, > > redirect also > > + * replies to the daemons' client requests. */ > > + ds_clear(match); > > + ds_put_format(match, "ip%d.dst == %s && %s.src == %d", ip_ver, > > s_addr, > > + proto, protocol_port); > > + ovn_lflow_add(lflows, ls_peer->od, S_SWITCH_IN_L2_LKUP, 100, > > + ds_cstr(match), > > + ds_cstr(actions), > > + ls_peer->lflow_ref); > > +} > > + > > +static void > > +apply_r_p_redirect__( > > Same here, I think I prefer apply_routing_protocols_redirect__(). > > > + const char *s_addr, const char *redirect_port_name, int > > protocol_flags, > > + bool is_ipv6, struct ovn_port *ls_peer, struct lflow_table > > *lflows, > > + struct ds *match, struct ds *actions) > > +{ > > + if (protocol_flags & REDIRECT_BGP) { > > + build_r_p_redirect_rule__(s_addr, redirect_port_name, 179, > > "tcp", > > + is_ipv6, ls_peer, lflows, match, > > actions); > > + } > > + > > + if (protocol_flags & REDIRECT_BFD) { > > + build_r_p_redirect_rule__(s_addr, redirect_port_name, > > 3784, "udp", > > + is_ipv6, ls_peer, lflows, match, > > actions); > > + } > > + > > + /* Because the redirected port shares IP and MAC addresses > > with the LRP, > > + * special consideration needs to be given to the signaling > > protocols. */ > > + if (is_ipv6) { > > + /* Ensure that redirect port receives copy of NA messages > > destined to > > + * its IP.*/ > > + ds_clear(match); > > + ds_clear(actions); > > + ds_put_format(actions, "clone { outport = \"%s\"; output; > > };", > > + redirect_port_name); > > + ds_put_format(match, "ip6.dst == %s && nd_na", s_addr); > > + ovn_lflow_add(lflows, ls_peer->od, S_SWITCH_IN_L2_LKUP, > > 100, > > + ds_cstr(match), > > + ds_cstr(actions), > > + ls_peer->lflow_ref); > > + } else { > > + /* Ensure that redirect port receives copy of ARP replies > > destined to > > + * its IP */ > > + ds_clear(match); > > + ds_clear(actions); > > + ds_put_format(actions, "clone { outport = \"%s\"; output; > > };", > > + redirect_port_name); > > + ds_put_format(match, "arp.op == 2 && arp.tpa == %s", > > s_addr); > > + ovn_lflow_add(lflows, ls_peer->od, S_SWITCH_IN_L2_LKUP, > > 100, > > + ds_cstr(match), > > + ds_cstr(actions), > > + ls_peer->lflow_ref); > > + > > Nit: no need for newline. > > > + } > > +} > > + > > +static int > > +parse_redirected_routing_protocols(struct ovn_port *lrp) { > > + int redirected_protocol_flags = 0; > > + const char *redirect_protocols = smap_get(&lrp->nbrp->options, > > + "routing- > > protocols"); > > + if (redirect_protocols == NULL) { > > + return redirected_protocol_flags; > > + } > > + > > + char *proto; > > + char *save_ptr = NULL; > > + char *tokstr = xstrdup(redirect_protocols); > > + for (proto = strtok_r(tokstr, ",", &save_ptr); proto != NULL; > > + proto = strtok_r(NULL, ",", &save_ptr)) { > > + if (!strcmp(proto, "BGP")) { > > + redirected_protocol_flags |= REDIRECT_BGP; > > + continue; > > + } > > + > > + if (!strcmp(proto, "BFD")) { > > + redirected_protocol_flags |= REDIRECT_BFD; > > + continue; > > + } > > Nit: missing newlines. > > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > > 5); > > + VLOG_WARN_RL(&rl, "Option 'routing-protocols' encountered > > unknown " > > + "value %s", > > + proto); > > + } > > + free(tokstr); > > + return redirected_protocol_flags; > > +} > > + > > +static void > > +build_lrouter_routing_protocol_redirect( > > + struct ovn_port *op, struct lflow_table *lflows, > > + struct ds *match, struct ds *actions) > > +{ > > + /* LRP has to have a peer.*/ > > + if (op->peer == NULL) { > > + return; > > + } > > Nit: missing newline. > > > + /* LRP has to have NB record.*/ > > + if (op->nbrp == NULL) { > > + return; > > + } > > + > > + /* Proceed only for LRPs that have 'routing-protocol-redirect' > > option set. > > + * Value of this option is the name of LSP to which the > > routing protocol > > + * traffic will be redirected. */ > > + const char *redirect_port = smap_get(&op->nbrp->options, > > + "routing-protocol- > > redirect"); > > + if (redirect_port == NULL) { > > + return; > > + } > > + > > + if (op->cr_port != NULL) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > > 5); > > + VLOG_WARN_RL(&rl, "Option 'routing-protocol-redirect' is > > not " > > + "supported on Distributed Gateway Port > > '%s'", > > + op->key); > > + return; > > + } > > + > > + /* Ensure that LSP, to which the routing protocol traffic is > > redirected, > > + * exists.*/ > > Nit: missing space after '.' > > > + struct ovn_port *peer_lsp; > > + bool redirect_port_exists = false; > > + HMAP_FOR_EACH (peer_lsp, dp_node, &op->peer->od->ports) { > > + size_t peer_lsp_s = strlen(peer_lsp->key); > > + if (peer_lsp_s == strlen(redirect_port) > > + && !strncmp(peer_lsp->key, redirect_port, > > peer_lsp_s)){ > > + redirect_port_exists = true; > > + break; > > + } > > + } > > + > > + if (!redirect_port_exists) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > > 5); > > + VLOG_WARN_RL(&rl, "Option 'routing-protocol-redirect' set > > on Logical " > > + "Router Port '%s' refers to non-existent > > Logical " > > + "Switch Port. Routing protocol > > redirecting won't be " > > + "configured.", > > + op->key); > > + return; > > + } > > + > > + > > Nit: too many new lines. > > > + int redirected_protocols = > > parse_redirected_routing_protocols(op); > > + if (!redirected_protocols) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > > 5); > > + VLOG_WARN_RL(&rl, "Option 'routing-protocol-redirect' is > > set on " > > + "Logical Router Port '%s' but no known > > protocols " > > + "were set via 'routing-protocols' > > options. This " > > + "configuration has no effect.", > > + op->key); > > + return; > > + } > > + > > + /* Redirecte traffic destined for LRP's IPs and the specified > > routing > > Typo: redirecte > > > + * protocol ports to the port defined in 'routing-protocol- > > redirect' > > + * option.*/ > > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > > + const char *ip_s = op->lrp_networks.ipv4_addrs[i].addr_s; > > + apply_r_p_redirect__(ip_s, redirect_port, > > redirected_protocols, false, > > + op->peer, lflows,match, actions); > > + } > > + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > > + const char *ip_s = op->lrp_networks.ipv6_addrs[i].addr_s; > > + apply_r_p_redirect__(ip_s, redirect_port, > > redirected_protocols, true, > > + op->peer, lflows,match, > > actions); > > + } > > + > > + /* Drop ARP replies and IPv6 RA/NA packets originating from > > + * 'routing-protocol-redirect' LSP. As this port shares IP and > > MAC > > + * addresses with LRP, we don't want to create duplicates.*/ > > + ds_clear(match); > > + ds_put_format(match, "inport == \"%s\" && arp.op == 2", > > redirect_port); > > + ovn_lflow_add(lflows, op->peer->od, > > S_SWITCH_IN_CHECK_PORT_SEC, 80, > > + ds_cstr(match), > > + REGBIT_PORT_SEC_DROP " = 1; next;", > > + op->peer->lflow_ref); > > + > > + ds_clear(match); > > + ds_put_format(match, "inport == \"%s\" && nd_na", > > redirect_port); > > + ovn_lflow_add(lflows, op->peer->od, > > S_SWITCH_IN_CHECK_PORT_SEC, 80, > > + ds_cstr(match), > > + REGBIT_PORT_SEC_DROP " = 1; next;", > > + op->peer->lflow_ref); > > + > > + ds_clear(match); > > + ds_put_format(match, "inport == \"%s\" && nd_ra", > > redirect_port); > > + ovn_lflow_add(lflows, op->peer->od, > > S_SWITCH_IN_CHECK_PORT_SEC, 80, > > + ds_cstr(match), > > + REGBIT_PORT_SEC_DROP " = 1; next;", > > + op->peer->lflow_ref); > > +} > > + > > /* This function adds ARP resolve flows related to a LSP. */ > > static void > > build_arp_resolve_flows_for_lsp( > > @@ -16003,6 +16218,8 @@ > > build_lswitch_and_lrouter_iterate_by_lrp(struct ovn_port *op, > > lsi->meter_groups, op->lflow_ref); > > build_lrouter_icmp_packet_toobig_admin_flows(op, lsi->lflows, > > &lsi->match, > > &lsi->actions, > > op->lflow_ref); > > + build_lrouter_routing_protocol_redirect(op, lsi->lflows, > > + &lsi->match, &lsi- > > >actions); > > } > > > > static void * > > diff --git a/northd/northd.h b/northd/northd.h > > index d4a8d75ab..691c23f01 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -89,6 +89,13 @@ ods_size(const struct ovn_datapaths *datapaths) > > > > bool od_has_lb_vip(const struct ovn_datapath *od); > > > > +/* List of routing and routing-related protocols which > > + * OVN is capable of redirecting from LRP to specific LSP. */ > > +enum redirected_routing_protcol_flag_type { > > + REDIRECT_BGP = (1 << 0), > > + REDIRECT_BFD = (1 << 1), > > +}; > > + > > struct tracked_ovn_ports { > > /* tracked created ports. > > * hmapx node data is 'struct ovn_port *' */ > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index b06b09ac5..250c602f2 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -284,6 +284,32 @@ > > dropped in the next stage. > > </li> > > > > + <li> > > + <p> > > + For each logical port that's defined as a target of > > routing protocol > > + redirecting (via <code>routing-protocol-redirect</code> > > option set on > > + Logical Router Port), a filter is set in place that > > disallows > > + following traffic exiting this port: > > + </p> > > + <ul> > > + <li> > > + ARP replies > > + </li> > > + <li> > > + IPv6 Neighbor Discovery - Router Advertisements > > + </li> > > + <li> > > + IPv6 Neighbor Discovery - Neighbor Advertisements > > + </li> > > + </ul> > > + <p> > > + Since this port shares IP and MAC addresses with the > > Logical Router > > + Port, we wan't to prevent duplicate replies and > > advertisements. This > > + is achieved by a rule with priority 80 that sets > > + <code>REGBIT_PORT_SEC_DROP" = 1; next;"</code>. > > + </p> > > + </li> > > + > > <li> > > For each (enabled) vtep logical port, a priority 70 flow > > is added which > > matches on all packets and applies the action > > @@ -1949,6 +1975,34 @@ output; > > on the logical switch. > > </li> > > > > + <li> > > + <p> > > + For any logical port that's defined as a target of > > routing protocol > > + redirecting (via <code>routing-protocol-redirect</code> > > option set on > > + Logical Router Port), we redirect the traffic related to > > protocols > > + specified in <code>routing-protocols</code> option. It's > > acoomplished > > + with following priority-100 flows: > > + <p> > > This should be </p>. > > > + <ul> > > + <li> > > + Flows that match Logical Router Port's IPs and > > destination port of > > + the routing daemon are redirected to this port to > > allow external > > + peers' connection to the daemon listening on this > > port. > > + </li> > > + <li> > > + Flows that match Logical Router Port's IPs and source > > port of > > + the routing daemon are redirected to this port to > > allow replies > > + from the peers. > > + </li> > > + </ul> > > + <p> > > + In addition to this, we add priority-100 rules that > > + <code>clone</code> ARP replies and IPv6 Neighbor > > Advertisements to > > + this port as well. These allow to build proper ARP/IPv6 > > neighbor > > + list on this port. > > + </p> > > + </li> > > + > > <li> > > Priority-90 flows for transit switches that forward > > registered > > IP multicast traffic to their corresponding multicast > > group , which > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index 9552534f6..092195473 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -3440,6 +3440,48 @@ or > > </p> > > </column> > > > > + <column name="options" key="routing-protocol-redirect" > > + type='{"type": "string"}'> > > + <p> > > + NOTE: this feature is experimental and may be subject to > > + removal/change in the future. > > + </p> > > + <p> > > + This option expects a name of a Logical Switch Port > > that's present > > + in the peer's Logical Switch. If set, it causes any > > traffic > > + that's destined for Logical Router Port's IP addresses > > (including > > + its IPv6 LLA) and the ports associated with routing > > protocols defined > > + ip <code>routing-protocols</code> option, to be > > redirected > > + to the specified Logical Switch Port. > > + > > + This allows external routing daemons to be bound to a > > port in OVN's > > + Logical Switch and act as if they were listening on > > Logical Router > > + Port's IP addresses. > > + </p> > > + </column> > > + > > + <column name="options" key="routing-protocols" > > type='{"type": "string"}'> > > + <p> > > + NOTE: this feature is experimental and may be subject to > > + removal/change in the future. > > + </p> > > + <p> > > + This option expects a comma-separated list of routing, > > and > > + routing-related protocols, whose control plane traffic > > will be > > + redirected to a port specified in > > + <code>routing-protocol-redirect</code> option. Currently > > supported > > + options are: > > + </p> > > + <ul> > > + <li> > > + <code>BGP</code> (forwards TCP port 179) > > + </li> > > + <li> > > + <code>BFD</code> (forwards UDP port 3748) > > + </li> > > + <ul> > > This should be </ul>. > > The rest looks OK to me. You can find the rebased version, including > these minor nits fixed here: > > https://github.com/dceara/ovn/commits/review-pws418655-bgp-redirect-v5/ > > If the small changes I made look OK to you feel free to squash them > in > and post v6 with my: > > Acked-by: Dumitru Ceara <[email protected]> > > Thanks, > Dumitru > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
