On 8/12/24 15:20, [email protected] wrote: > Sorry, one more follow-up question. >
No problem! :) > 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; >>>> + } >>>> + >>>> + /* 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? > ovn_port_find() is a hmap lookup, that's O(1) (amortized). Should be OK, IMO. >> >>> >>>> + >>>> + 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? >> Yes, I meant instead of AT_CHECK. No functional differences, just less characters when we don't validate output (well, when we expect no 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. >> > Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
