Hi Vladislav,
On Mon, 2024-08-12 at 20:57 +0700, Vladislav Odintsov wrote:
>
>
>
>
> On 12.08.2024 20:20, [email protected] wrote:
>
> >
> > Sorry, one more follow-up question.
> >
> > On Mon, 2024-08-12 at 15:09 +0200, [email protected] wrote:
> >
> > >
> > > Hi Dumitru,
> > > thanks for the fast review.
> > >
> > > On Mon, 2024-08-12 at 14:41 +0200, Dumitru Ceara wrote:
> > >
> > > >
> > > > On 8/12/24 13:44, 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,
> > > >
> > > >
> > > > >
> > > > > v8 patch fixes intermittent segfault issue present in previous
> > > > > versions.
> > > > > It was caused by mistakingly using peer port's 'lflow_ref'
> > > > > (op->peer->lflow_ref) when inserting rules into the peer's
> > > > > datapath. I
> > > > > assumed that when rules are injected into the peer's datapath,
> > > > > it
> > > > > should
> > > > > use peer port's 'lflow_ref'. However that is not the case[0] and
> > > > > the
> > > > > thread-unsafe nature of 'lflow_ref'[1] caused crashes when
> > > > > another
> > > > > thread
> > > > > tried to use peer port's lflow_ref at the same time.
> > > > >
> > > > >
> > > >
> > > > Ah, good catch! Yes, lflow_ref is not straightforward.
> > > >
> > > >
> > > > >
> > > > > I'm running tests for this feature in loop and I'm at 500+
> > > > > successful
> > > > > executions, whereas before I would see crashes in about 10
> > > > > attempts.
> > > > >
> > > > > While I was looking at this patch with fresh eyes, I also
> > > > > included
> > > > > few
> > > > > nits:
> > > > > * rename 'redirect_port' var. to more descriptive
> > > > > 'redirect_port_name'
> > > > > * rename potentially confusing iterator variable 'lsp_peer' to
> > > > > more
> > > > > descriptive 'lsp_in_peer'
> > > > > * relaxed overly cautious string comparison
> > > > > 'if (s1_len == s2_len && !strncmp(s1, s2, s1_len))' to more
> > > > > simple
> > > > > 'if (!strcmp(s1, s2))' as I believe that we can safely assume
> > > > > that both
> > > > > s1 and s2 are null-terminated strings
> > > > >
> > > > > [0]
> > > > > https://github.com/ovn-org/ovn/blob/f0a368143e492c798d3233439f9f097f1c9690cd/northd/northd.c#L13953-L13957
> > > > > [1]
> > > > > https://github.com/ovn-org/ovn/blob/f0a368143e492c798d3233439f9f097f1c9690cd/northd/northd.h#L684
> > > > >
> > > > >
> > > >
> > > > I have a couple of small comments below.
> > > >
> > > > Otherwise, the code looks good to me!
> > > >
> > > >
> > > > >
> > > > > northd/northd.c | 226
> > > > > ++++++++++++++++++++++++++++++++++++++++
> > > > > northd/northd.h | 7 ++
> > > > > northd/ovn-northd.8.xml | 54 ++++++++++
> > > > > ovn-nb.xml | 42 ++++++++
> > > > > tests/ovn-northd.at | 93 +++++++++++++++++
> > > > > tests/system-ovn.at | 120 +++++++++++++++++++++
> > > > > 6 files changed, 542 insertions(+)
> > > > >
> > > > > diff --git a/northd/northd.c b/northd/northd.c
> > > > > index 5ad30d854..53012de89 100644
> > > > > --- a/northd/northd.c
> > > > > +++ b/northd/northd.c
> > > > > @@ -14002,6 +14002,230 @@ build_arp_resolve_flows_for_lrp(struct
> > > > > ovn_port *op,
> > > > > }
> > > > > }
> > > > >
> > > > > +static void
> > > > > +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,
> > > > > + struct lflow_ref *lflow_ref)
> > > > > +{
> > > > > + 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),
> > > > > + 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),
> > > > > + lflow_ref);
> > > > > +}
> > > > > +
> > > > > +static void
> > > > > +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, struct lflow_ref
> > > > > *lflow_ref)
> > > > > +{
> > > > > + if (protocol_flags & REDIRECT_BGP) {
> > > > > + build_routing_protocols_redirect_rule__(s_addr,
> > > > > redirect_port_name,
> > > > > + 179, "tcp",
> > > > > is_ipv6, ls_peer,
> > > > > + lflows, match,
> > > > > actions,
> > > > > + lflow_ref);
> > > > > + }
> > > > > +
> > > > > + if (protocol_flags & REDIRECT_BFD) {
> > > > > + build_routing_protocols_redirect_rule__(s_addr,
> > > > > redirect_port_name,
> > > > > + 3784, "udp",
> > > > > is_ipv6, ls_peer,
> > > > > + lflows, match,
> > > > > actions,
> > > > > + lflow_ref);
> > > > > + }
> > > > > +
> > > > > + /* Because the redirected port shares IP and MAC addresses
> > > > > with the LRP,
> > > > > + * special consideration needs to be given to the signaling
> > > > > protocols. */
> > > > > + ds_clear(actions);
> > > > > + ds_put_format(actions,
> > > > > + "clone { outport = \"%s\"; output; }; "
> > > > > + "outport = %s; output;",
> > > > > + redirect_port_name, ls_peer->json_key);
> > > > > + if (is_ipv6) {
> > > > > + /* Ensure that redirect port receives copy of NA
> > > > > messages
> > > > > destined to
> > > > > + * its IP.*/
> > > > > + ds_clear(match);
> > > > > + 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),
> > > > > + lflow_ref);
> > > > > + } else {
> > > > > + /* Ensure that redirect port receives copy of ARP
> > > > > replies
> > > > > destined to
> > > > > + * its IP */
> > > > > + ds_clear(match);
> > > > > + 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),
> > > > > + lflow_ref);
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +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;
> > > > > + }
> > > > > +
> > > > > + 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, struct lflow_ref
> > > > > *lflow_ref)
> > > > > +{
> > > > > + /* LRP has to have a peer.*/
> > > > > + if (op->peer == NULL) {
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + /* 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_name = smap_get(&op->nbrp-
> > > > >
> > > > > >
> > > > > > options,
> > > > > >
> > > > >
> > > > > + "routing-protocol-
> > > > > redirect");
> > > > > + if (redirect_port_name == 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;
> > > > > + }
> > > > >
> > > >
> > >
> >
>
> Hi Martin,
>
> while testing this patch I've built a simple topology (this was done
> by our automation, I've just set new LRP options):
>
> ovn-nbctl lr-add LR
> ovn-nbctl lrp-add public 00:00:00:00:01 169.254.0.1/30
> ovn-nbctl ha-chassis-group-add gr1
> ovn-nbctl ha-chassis-group-add-chassis gr1 chassis1
> ovn-nbctl lrp-set-options public routing-protocol-redirect=frr-port
> routing-protocols="BGP,BFD"
>
> After this I see in ovn-northd.log next warning:
>
> 2024-08-12T13:52:21.196Z|00002|northd(>rker pool hel3)|WARN|Option
> 'routing-protocol-redirect' is not supported on Distributed Gateway
> Port 'public'
>
> I'm wondering why ha-chassis-group is not supported while centralized
> is (via options:chassis=hv1 in LR)?
We went with restricting this feature to gateway routers to simplify
initial development. If this was enabled for distributed routers, we'd
have to answer question like:
* How do you keep BGP daemons running on different chassis in sync wrt
learning/leaking routes, if only one of them has active connection with
peer
* How do you automate re-mapping of LSP to interface in case the
active gateway chassis changes.
Given that you have ha-chassis-group with only single chassis, would it
not work for you to just set "chassis:chassis1" option on LR?
Martin.
>
> >
> > >
> > > >
> > > > >
> > > > > +
> > > > > + /* Ensure that LSP, to which the routing protocol traffic is
> > > > > redirected,
> > > > > + * exists. */
> > > > > + struct ovn_port *lsp_in_peer;
> > > > > + bool redirect_port_exists = false;
> > > > > + HMAP_FOR_EACH (lsp_in_peer, dp_node, &op->peer->od->ports) {
> > > > > + if (!strcmp(lsp_in_peer->key, redirect_port_name)) {
> > > > > + redirect_port_exists = true;
> > > > > + break;
> > > > > + }
> > > > > + }
> > > > >
> > > >
> > > > I'd avoid this and use ovn_port_find(), e.g.:
> > > >
> > > > /* Ensure that LSP, to which the routing protocol traffic is
> > > > redirected,
> > > > * exists. */
> > > > struct ovn_port *lsp_in_peer = ovn_port_find(ls_ports,
> > > > redirect_port_name);
> > > > if (!lsp_in_peer || lsp_in_peer->od != op->peer->od) {
> > > > 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 '%s;. Routing protocol
> > > > redirecting "
> > > > "won't be configured.",
> > > > op->key, redirect_port_name);
> > > > return;
> > > > }
> > > >
> > > > We can pass "lsi->ls_ports" to
> > > > build_lrouter_routing_protocol_redirect().
> > > >
> > >
> > > ack. This will indeed simplify the lookup code.
> > >
> >
> > With this approach, we'd be iterating over every LSP known to OVN,
> > right? With the original approach we are iterating only over LSPs in
> > the datapath of the peer port. Given the amount of ports that can be
> > present in large deployments, does it makes sense to consider cost of
> > this iteration or would it be negligible?
> >
> >
> > >
> > > >
> > > > >
> > > > > +
> > > > > + 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;
> > > > > + }
> > > > > +
> > > > > + 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;
> > > > > + }
> > > > > +
> > > > > + /* Redirect traffic destined for LRP's IPs and the specified
> > > > > routing
> > > > > + * 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_routing_protocols_redirect__(ip_s,
> > > > > redirect_port_name,
> > > > > + redirected_protocols,
> > > > > false,
> > > > > + op->peer, lflows,
> > > > > match, actions,
> > > > > + lflow_ref);
> > > > > + }
> > > > > + 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_routing_protocols_redirect__(ip_s,
> > > > > redirect_port_name,
> > > > > + redirected_protocols,
> > > > > true,
> > > > > + op->peer, lflows,
> > > > > match, actions,
> > > > > + lflow_ref);
> > > > > + }
> > > > > +
> > > > > + /* 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_name);
> > > > > + ovn_lflow_add(lflows, op->peer->od,
> > > > > S_SWITCH_IN_CHECK_PORT_SEC, 80,
> > > > > + ds_cstr(match),
> > > > > + REGBIT_PORT_SEC_DROP " = 1; next;",
> > > > > + lflow_ref);
> > > > > +
> > > > > + ds_clear(match);
> > > > > + ds_put_format(match, "inport == \"%s\" && nd_na",
> > > > > + redirect_port_name);
> > > > > + ovn_lflow_add(lflows, op->peer->od,
> > > > > S_SWITCH_IN_CHECK_PORT_SEC, 80,
> > > > > + ds_cstr(match),
> > > > > + REGBIT_PORT_SEC_DROP " = 1; next;",
> > > > > + lflow_ref);
> > > > > +
> > > > > + ds_clear(match);
> > > > > + ds_put_format(match, "inport == \"%s\" && nd_ra",
> > > > > + redirect_port_name);
> > > > > + ovn_lflow_add(lflows, op->peer->od,
> > > > > S_SWITCH_IN_CHECK_PORT_SEC, 80,
> > > > > + ds_cstr(match),
> > > > > + REGBIT_PORT_SEC_DROP " = 1; next;",
> > > > > + lflow_ref);
> > > > > +}
> > > > > +
> > > > > /* This function adds ARP resolve flows related to a LSP. */
> > > > > static void
> > > > > build_arp_resolve_flows_for_lsp(
> > > > > @@ -16969,6 +17193,8 @@
> > > > > build_lswitch_and_lrouter_iterate_by_lrp(struct ovn_port *op,
> > > > > 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, op-
> > > > >
> > > > > >
> > > > > > lflow_ref);
> > > > > >
> > > > >
> > > > > }
> > > > >
> > > > > static void *
> > > > > diff --git a/northd/northd.h b/northd/northd.h
> > > > > index 6e0258ff4..c7f0b829b 100644
> > > > > --- a/northd/northd.h
> > > > > +++ b/northd/northd.h
> > > > > @@ -93,6 +93,13 @@ ovn_datapath_find_by_key(struct hmap
> > > > > *datapaths,
> > > > > uint32_t dp_key);
> > > > >
> > > > > 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 3abd5f75b..ede38882a 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
> > > > > @@ -2002,6 +2028,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>
> > > > > + <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 bbda423a5..2836f58f5 100644
> > > > > --- a/ovn-nb.xml
> > > > > +++ b/ovn-nb.xml
> > > > > @@ -3575,6 +3575,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 3784)
> > > > > + </li>
> > > > > + </ul>
> > > > > + </column>
> > > > > +
> > > > > <column name="options" key="gateway_mtu_bypass">
> > > > > <p>
> > > > > When configured, represents a match expression, in the
> > > > > same
> > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > > > > index e4c882265..b32639a81 100644
> > > > > --- a/tests/ovn-northd.at
> > > > > +++ b/tests/ovn-northd.at
> > > > > @@ -13761,3 +13761,96 @@ AT_CHECK([grep -e "172.168.0.110" -e
> > > > > "172.168.0.120" -e "10.0.0.3" -e "20.0.0.3"
> > > > >
> > > > > AT_CLEANUP
> > > > > ])
> > > > > +
> > > > > +OVN_FOR_EACH_NORTHD_NO_HV([
> > > > > +AT_SETUP([Routing protocol control plane redirect])
> > > > > +ovn_start
> > > > > +
> > > > > +check ovn-sbctl chassis-add hv1 geneve 127.0.0.1
> > > > > +
> > > > > +check ovn-nbctl lr-add lr -- \
> > > > > + lrp-add lr lr-ls 02:ac:10:01:00:01 172.16.1.1/24
> > > > > +check ovn-nbctl --wait=sb set logical_router lr
> > > > > options:chassis=hv1
> > > > > +
> > > > > +check ovn-nbctl ls-add ls -- \
> > > > > + lsp-add ls ls-lr -- \
> > > > > + lsp-set-type ls-lr router -- \
> > > > > + lsp-set-addresses ls-lr router -- \
> > > > > + lsp-set-options ls-lr router-port=lr-ls
> > > > > +
> > > > > +check ovn-nbctl lsp-add ls lsp-bgp -- \
> > > > > + lsp-set-addresses lsp-bgp unknown
> > > > > +
> > > > > +# Function that ensures that no redirect rules are installed.
> > > > > +check_no_redirect() {
> > > > > + AT_CHECK([ovn-sbctl dump-flows ls | grep ls_in_l2_lkup |
> > > > > grep
> > > > > -E "tcp.dst == 179|tcp.src == 179" | wc -l], [0], [0
> > > > > +])
> > > > > +
> > > > > + AT_CHECK([ovn-sbctl dump-flows ls | grep
> > > > > ls_in_check_port_sec
> > > > >
> > > > > >
> > > > > > grep -E "priority=80" | wc -l], [0], [0
> > > > > >
> > > > >
> > > > > +])
> > > > > + check_no_bfd_redirect
> > > > > +}
> > > > > +
> > > > > +# Function that ensures that no BFD redirect rules are
> > > > > installed.
> > > > > +check_no_bfd_redirect() {
> > > > > + AT_CHECK([ovn-sbctl dump-flows ls | grep ls_in_l2_lkup |
> > > > > grep
> > > > > -E "udp.dst == 3784|udp.src == 3784" | wc -l], [0], [0
> > > > > +])
> > > > > +}
> > > > > +
> > > > > +# By default, no rules related to routing protocol redirect are
> > > > > present
> > > > > +check_no_redirect
> > > > > +
> > > > > +# Set "lsp-bgp" port as target of BGP control plane redirected
> > > > > traffic
> > > > > +check ovn-nbctl --wait=sb set logical_router_port lr-ls
> > > > > options:routing-protocol-redirect=lsp-bgp
> > > > > +check ovn-nbctl --wait=sb set logical_router_port lr-ls
> > > > > options:routing-protocols=BGP
> > > > > +
> > > > > +# Check that BGP control plane traffic is redirected "lsp-bgp"
> > > > > +AT_CHECK([ovn-sbctl dump-flows ls | grep ls_in_l2_lkup | grep -E
> > > > > "tcp.dst == 179|tcp.src == 179" | ovn_strip_lflows], [0], [dnl
> > > > > + table=??(ls_in_l2_lkup ), priority=100 , match=(ip4.dst
> > > > > ==
> > > > > 172.16.1.1 && tcp.dst == 179), action=(outport = "lsp-bgp";
> > > > > output;)
> > > > > + table=??(ls_in_l2_lkup ), priority=100 , match=(ip4.dst
> > > > > ==
> > > > > 172.16.1.1 && tcp.src == 179), action=(outport = "lsp-bgp";
> > > > > output;)
> > > > > + table=??(ls_in_l2_lkup ), priority=100 , match=(ip6.dst
> > > > > ==
> > > > > fe80::ac:10ff:fe01:1 && tcp.dst == 179), action=(outport = "lsp-
> > > > > bgp"; output;)
> > > > > + table=??(ls_in_l2_lkup ), priority=100 , match=(ip6.dst
> > > > > ==
> > > > > fe80::ac:10ff:fe01:1 && tcp.src == 179), action=(outport = "lsp-
> > > > > bgp"; output;)
> > > > > +])
> > > > > +
> > > > > +# Check that ARP/ND traffic is cloned to the "lsp-bgp"
> > > > > +AT_CHECK([ovn-sbctl dump-flows ls | grep ls_in_l2_lkup | grep
> > > > > "arp.op == 2 && arp.tpa == 172.16.1.1" | ovn_strip_lflows], [0],
> > > > > [dnl
> > > > > + table=??(ls_in_l2_lkup ), priority=100 , match=(arp.op
> > > > > ==
> > > > > 2 && arp.tpa == 172.16.1.1), action=(clone { outport = "lsp-bgp";
> > > > > output; }; outport = "ls-lr"; output;)
> > > > > +])
> > > > > +AT_CHECK([ovn-sbctl dump-flows ls | grep ls_in_l2_lkup | grep
> > > > > "&&
> > > > > nd_na" | ovn_strip_lflows], [0], [dnl
> > > > > + table=??(ls_in_l2_lkup ), priority=100 , match=(ip6.dst
> > > > > ==
> > > > > fe80::ac:10ff:fe01:1 && nd_na), action=(clone { outport = "lsp-
> > > > > bgp"; output; }; outport = "ls-lr"; output;)
> > > > > +])
> > > > > +
> > > > > +# Check that at this point no BFD redirecting is present
> > > > > +check_no_bfd_redirect
> > > > > +
> > > > > +# Add BFD traffic redirect
> > > > > +check ovn-nbctl --wait=sb set logical_router_port lr-ls
> > > > > options:routing-protocols=BGP,BFD
> > > > > +
> > > > > +# Check that BFD traffic is redirected to "lsp-bgp"
> > > > > +AT_CHECK([ovn-sbctl dump-flows ls | grep ls_in_l2_lkup | grep -E
> > > > > "udp.dst == 3784|udp.src == 3784" | ovn_strip_lflows], [0], [dnl
> > > > > + table=??(ls_in_l2_lkup ), priority=100 , match=(ip4.dst
> > > > > ==
> > > > > 172.16.1.1 && udp.dst == 3784), action=(outport = "lsp-bgp";
> > > > > output;)
> > > > > + table=??(ls_in_l2_lkup ), priority=100 , match=(ip4.dst
> > > > > ==
> > > > > 172.16.1.1 && udp.src == 3784), action=(outport = "lsp-bgp";
> > > > > output;)
> > > > > + table=??(ls_in_l2_lkup ), priority=100 , match=(ip6.dst
> > > > > ==
> > > > > fe80::ac:10ff:fe01:1 && udp.dst == 3784), action=(outport = "lsp-
> > > > > bgp"; output;)
> > > > > + table=??(ls_in_l2_lkup ), priority=100 , match=(ip6.dst
> > > > > ==
> > > > > fe80::ac:10ff:fe01:1 && udp.src == 3784), action=(outport = "lsp-
> > > > > bgp"; output;)
> > > > > +])
> > > > > +
> > > > > +
> > > > > +# Check that ARP replies and ND advertisements are blocked from
> > > > > exiting "lsp-bgp"
> > > > > +AT_CHECK([ovn-sbctl dump-flows ls | grep ls_in_check_port_sec |
> > > > > grep "priority=80" | ovn_strip_lflows], [0], [dnl
> > > > > + table=??(ls_in_check_port_sec), priority=80 , match=(inport
> > > > > ==
> > > > > "lsp-bgp" && arp.op == 2), action=(reg0[[15]] = 1; next;)
> > > > > + table=??(ls_in_check_port_sec), priority=80 , match=(inport
> > > > > ==
> > > > > "lsp-bgp" && nd_na), action=(reg0[[15]] = 1; next;)
> > > > > + table=??(ls_in_check_port_sec), priority=80 , match=(inport
> > > > > ==
> > > > > "lsp-bgp" && nd_ra), action=(reg0[[15]] = 1; next;)
> > > > > +])
> > > > > +
> > > > > +# Remove 'bgp-redirect' option from LRP and check that rules are
> > > > > removed
> > > > > +check ovn-nbctl --wait=sb remove logical_router_port lr-ls
> > > > > options
> > > > > routing-protocol-redirect
> > > > > +check ovn-nbctl --wait=sb remove logical_router_port lr-ls
> > > > > options
> > > > > routing-protocols
> > > > > +check_no_redirect
> > > > > +
> > > > > +# Set non-existent LSP as target of 'bgp-redirect' and check
> > > > > that
> > > > > no rules are added
> > > > > +check ovn-nbctl --wait=sb set logical_router_port lr-ls
> > > > > options:routing-protocol-redirect=lsp-foo
> > > > > +check ovn-nbctl --wait=sb set logical_router_port lr-ls
> > > > > options:routing-protocols=BGP,BFD
> > > > > +check_no_redirect
> > > > > +
> > > > > +AT_CLEANUP
> > > > > +])
> > > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > > > > index c54b0f3a5..b092111fb 100644
> > > > > --- a/tests/system-ovn.at
> > > > > +++ b/tests/system-ovn.at
> > > > > @@ -13750,3 +13750,123 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error
> > > > > receiving.*/d
> > > > > /.*terminating with signal 15.*/d"])
> > > > > AT_CLEANUP
> > > > > ])
> > > > > +
> > > > > +OVN_FOR_EACH_NORTHD([
> > > > > +AT_SETUP([Routing protocol redirect])
> > > > > +AT_SKIP_IF([test $HAVE_NC = no])
> > > > > +
> > > > > +ovn_start
> > > > > +OVS_TRAFFIC_VSWITCHD_START()
> > > > > +
> > > > > +ADD_BR([br-int])
> > > > > +ADD_BR([br-ext])
> > > > > +
> > > > > +check ovs-ofctl add-flow br-ext action=normal
> > > > > +# Set external-ids in br-int needed for ovn-controller
> > > > > +check ovs-vsctl \
> > > > > + -- set Open_vSwitch . external-ids:system-id=hv1 \
> > > > > + -- set Open_vSwitch . external-ids:ovn-
> > > > > remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> > > > > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve
> > > > > \
> > > > > + -- set Open_vSwitch . external-ids:ovn-encap-
> > > > > ip=169.0.0.1
> > > > > \
> > > > > + -- set bridge br-int fail-mode=secure other-
> > > > > config:disable-in-band=true
> > > > > +
> > > > > +# Start ovn-controller
> > > > > +start_daemon ovn-controller
> > > > > +
> > > > > +check ovn-nbctl lr-add R1 \
> > > > > + -- set Logical_Router R1 options:chassis=hv1
> > > > > +
> > > > > +check ovn-nbctl ls-add public
> > > > > +check ovn-nbctl ls-add bar
> > > > > +
> > > > > +check ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03
> > > > > 172.16.1.1/24
> > > > > +check ovn-nbctl lrp-add R1 rp-bar 00:00:ff:00:00:01
> > > > > 192.168.10.1/24
> > > > > +
> > > > > +check ovn-nbctl lsp-add public public-rp -- set
> > > > > Logical_Switch_Port public-rp \
> > > > > + type=router options:router-port=rp-public \
> > > > > + -- lsp-set-addresses public-rp router
> > > > > +
> > > > > +check ovn-nbctl lsp-add bar bar-rp -- set Logical_Switch_Port
> > > > > bar-
> > > > > rp \
> > > > > + type=router options:router-port=rp-bar \
> > > > > + -- lsp-set-addresses bar-rp router
> > > > > +
> > > > > +check ovn-nbctl lsp-add public bgp-daemon \
> > > > > + -- lsp-set-addresses bgp-daemon unknown
> > > > > +
> > > > > +# Setup container "bar1" representing host on an internal
> > > > > network
> > > > > +ADD_NAMESPACES(bar1)
> > > > > +ADD_VETH(bar1, bar1, br-int, "192.168.10.2/24",
> > > > > "00:00:ff:ff:ff:01", \
> > > > > + "192.168.10.1")
> > > > > +check ovn-nbctl lsp-add bar bar1 \
> > > > > + -- lsp-set-addresses bar1 "00:00:ff:ff:ff:01 192.168.10.2"
> > > > > +
> > > > > +# Setup SNAT for the internal host
> > > > > +AT_CHECK([ovn-nbctl lr-nat-add R1 snat 172.16.1.1 192.168.10.2])
> > > > >
> > > >
> > > > Nit: could be "check ovn-nbctl ..".
> > > >
> > >
> > > Do you mean instead of AT_CHECK macro? I don't mind changing it, but
> > > I'm curious if there are any functional differences. From the
> > > description, they seem to be doing about the same thing. Is it just
> > > unnecessary overhead to use AT_CHECK when we don't validate the
> > > stdout/err output?
> > >
> > >
> > > >
> > > > >
> > > > > +
> > > > > +# Configure external connectivity
> > > > > +AT_CHECK([ovs-vsctl set Open_vSwitch . external-ids:ovn-bridge-
> > > > > mappings=phynet:br-ext])
> > > > >
> > > >
> > > > Nit: could be "check ovs-vsctl .."
> > > >
> > > >
> > > > >
> > > > > +check ovn-nbctl lsp-add public public1 \
> > > > > + -- lsp-set-addresses public1 unknown \
> > > > > + -- lsp-set-type public1 localnet \
> > > > > + -- lsp-set-options public1 network_name=phynet
> > > > > +
> > > > > +check ovn-nbctl --wait=hv sync
> > > > > +
> > > > > +# Set option that redirects BGP traffic to a LSP "bgp-daemon"
> > > > > +check ovn-nbctl --wait=sb set logical_router_port rp-public
> > > > > options:routing-protocol-redirect=bgp-daemon
> > > > > +check ovn-nbctl --wait=sb set logical_router_port rp-public
> > > > > options:routing-protocols=BGP
> > > > > +
> > > > > +# Create "bgp-daemon" interface in a namespace with IP and MAC
> > > > > matching LRP "rp-public"
> > > > > +ADD_NAMESPACES(bgp-daemon)
> > > > > +ADD_VETH(bgp-daemon, bgp-daemon, br-int, "172.16.1.1/24",
> > > > > "00:00:02:01:02:03")
> > > > > +
> > > > > +ADD_NAMESPACES(ext-foo)
> > > > > +ADD_VETH(ext-foo, ext-foo, br-ext, "172.16.1.100/24",
> > > > > "00:10:10:01:02:13", \
> > > > > + "172.16.1.1")
> > > > > +
> > > > > +# Flip the interface down/up to get proper IPv6 LLA
> > > > > +NS_EXEC([bgp-daemon], [ip link set down bgp-daemon])
> > > > > +NS_EXEC([bgp-daemon], [ip link set up bgp-daemon])
> > > > > +NS_EXEC([ext-foo], [ip link set down ext-foo])
> > > > > +NS_EXEC([ext-foo], [ip link set up ext-foo])
> > > > > +
> > > > > +# Wait until IPv6 LLA loses the "tentative" flag otherwise it
> > > > > can't be bound to.
> > > > > +OVS_WAIT_UNTIL([NS_EXEC([bgp-daemon], [ip a show dev bgp-daemon
> > > > > |
> > > > > grep "fe80::" | grep -v tentative])])
> > > > > +OVS_WAIT_UNTIL([NS_EXEC([ext-foo], [ip a show dev ext-foo | grep
> > > > > "fe80::" | grep -v tentative])])
> > > > > +
> > > > > +# Verify that BGP control plane traffic is delivered to the
> > > > > "bgp-
> > > > > daemon"
> > > > > +# interface on both IPv4 and IPv6 LLA addresses
> > > > > +NETNS_DAEMONIZE([bgp-daemon], [nc -l -k 172.16.1.1 179],
> > > > > [bgp_v4.pid])
> > > > > +NS_CHECK_EXEC([ext-foo], [echo "TCP test" | nc --send-only
> > > > > 172.16.1.1 179])
> > > > > +
> > > > > +NETNS_DAEMONIZE([bgp-daemon], [nc -l -6 -k
> > > > > fe80::200:2ff:fe01:203%bgp-daemon 179], [bgp_v6.pid])
> > > > > +NS_CHECK_EXEC([ext-foo], [echo "TCP test" | nc --send-only -6
> > > > > fe80::200:2ff:fe01:203%ext-foo 179])
> > > > > +
> > > > > +# Verify connection in other direction. i.e when daemon running
> > > > > on
> > > > > "bgp-daemon" port
> > > > > +# makes a client connection to its peer
> > > > > +NETNS_DAEMONIZE([ext-foo], [nc -l -k 172.16.1.100 179],
> > > > > [reply_bgp_v4.pid])
> > > > > +NS_CHECK_EXEC([bgp-daemon], [echo "TCP test" | nc --send-only
> > > > > 172.16.1.100 179])
> > > > > +
> > > > > +NETNS_DAEMONIZE([ext-foo], [nc -l -6 -k
> > > > > fe80::210:10ff:fe01:213%ext-foo 179], [reply_bgp_v6.pid])
> > > > > +NS_CHECK_EXEC([bgp-daemon], [echo "TCP test" | nc --send-only -6
> > > > > fe80::210:10ff:fe01:213%bgp-daemon 179])
> > > > > +
> > > > >
> > > >
> > > > Maybe it's worth adding a simple UDP echo test (for the BFD port)
> > > > too?
> > > >
> > >
> > > Yup, makes sense.
> > >
> > > >
> > > > >
> > > > > +# Verify that hosts on the internal network can reach external
> > > > > networks
> > > > > +NETNS_DAEMONIZE([ext-foo], [nc -l -k 172.16.1.100 2222],
> > > > > [nc_external.pid])
> > > > > +NS_CHECK_EXEC([bar1], [echo "TCP test" | nc -w 1 --send-only
> > > > > 172.16.1.100 2222])
> > > > > +
> > > > > +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> > > > > +
> > > > > +as ovn-sb
> > > > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > > > > +
> > > > > +as ovn-nb
> > > > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > > > > +
> > > > > +as northd
> > > > > +OVS_APP_EXIT_AND_WAIT([ovn-northd])
> > > > > +
> > > > > +as
> > > > > +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
> > > > > +/.*terminating with signal 15.*/d"])
> > > > > +AT_CLEANUP
> > > > > +])
> > > > >
> > > >
> > > > Thanks a lot for the hard work on this feature!
> > > >
> > > > Regards,
> > > > Dumitru
> > > >
> > > >
> > >
> > > Thanks,
> > > Martin.
> > >
> > >
> >
> > _______________________________________________
> > dev mailing list
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
