Thanks for the update, some minor issues with the documentation
On Fri, Sep 25, 2026 at 8:36 AM Alexandra Rukomoinikova via dev < [email protected]> wrote: > Add per logical router port option for ICMPv4 redirect support, > The option is disabled by default. > > RFC 1812 describes the operation of this functionality for the following > scenario: When a router forwards a packet back out the port it arrived on, > and the next hop is on the sender's own network, the sender could have > reached that next hop directly. RFC 1812 (5.2.7.2) requires the > router to point this out with an ICMPv4 Redirect (RFC 792, type 5, > code 1), naming the better first hop. OVN routers do not send Redirects, > so such traffic keeps taking the extra hop through the router. > > In this commit we assume that ip destination of packet is not in one of > directly connected router's networks, so the Redirect always points to > another router and never to the destination host. > > Of the conditions RFC 1812 5.2.7.2 and RFC 1122 3.2.2 put on sending a > Redirect, this implements: > - the packet leaves through the port it arrived on; > - the sender and the next hop are on the same network of that port; > - the next hop is not the sender itself; > - the packet is not a later fragment; > - the packet is not itself an ICMP Redirect. > > Not implemented: packets with a source route option. > > Routing overwrites eth.src with the egress router port mac, so the > original eth.src has to be saved in REG_ORIG_ETH_SRC beforehand. > > Signed-off-by: Alexandra Rukomoinikova <[email protected]> > --- > Documentation/ref/ovn-logical-flows.7.rst | 92 +++++++++++---- > NEWS | 5 + > lib/ovn-util.c | 4 +- > northd/northd.c | 129 +++++++++++++++++++--- > northd/northd.h | 19 ++-- > ovn-nb.xml | 17 +++ > tests/ovn-northd.at | 68 ++++++++++++ > tests/ovn.at | 36 ++++++ > tests/system-ovn.at | 106 ++++++++++++++++++ > 9 files changed, 432 insertions(+), 44 deletions(-) > > diff --git a/Documentation/ref/ovn-logical-flows.7.rst > b/Documentation/ref/ovn-logical-flows.7.rst > index 44fd560bf..f55567852 100644 > --- a/Documentation/ref/ovn-logical-flows.7.rst > +++ b/Documentation/ref/ovn-logical-flows.7.rst > @@ -3097,6 +3097,12 @@ with priority 100 and action setting > unique-generated per-datapath 32-bit value > If packet didn't match any configured inport (*<main>* route table), > register 7 > value is set to 0. > > +If Logical Router Port *P* has ``options:send_icmp4_redirects`` set to > +``true``, the original Ethernet source address of the packet is also saved > +in ``xreg1[0..47]``, because routing overwrites ``eth.src`` with the > +address of the egress router port. The saved address is used by > +:ref:`ICMP Redirect <lr-in-20>` as the destination of the ICMPv4 Redirect. > + > This table contains the following logical flows: > > - Priority-100 flow with match ``inport == "LRP_NAME"`` value and action, > which > @@ -3115,7 +3121,7 @@ setting ``reg0`` (or ``xxreg0`` for IPv6) to the > next-hop IP address (leaving > ``ip4.dst`` or ``ip6.dst``, the packet's final destination, unchanged) and > advances to the next table for ARP resolution. It also sets ``reg1`` (or > ``xxreg1``) to the IP address owned by the selected router port (ingress > table > -:ref:`ARP Request <lr-in-27>` will generate an ARP request, if needed, > with > +:ref:`ARP Request <lr-in-28>` will generate an ARP request, if needed, > with > ``reg0`` as the target protocol address and ``reg1`` as the source > protocol > address). > > @@ -3251,7 +3257,7 @@ setting ``reg0`` (or ``xxreg0`` for IPv6) to the > next-hop IP address (leaving > ``ip4.dst`` or ``ip6.dst``, the packet's final destination, unchanged) and > advances to the next table for ARP resolution. It also sets ``reg1`` (or > ``xxreg1``) to the IP address owned by the selected router port (ingress > table > -:ref:`ARP Request <lr-in-27>` will generate an ARP request, if needed, > with > +:ref:`ARP Request <lr-in-28>` will generate an ARP request, if needed, > with > ``reg0`` as the target protocol address and ``reg1`` as the source > protocol > address). > > @@ -3348,7 +3354,55 @@ nexthops. > > .. _lr-in-20: > > -Ingress Table 20: DHCP Relay Response Check > +Ingress Table 20: ICMP Redirect > ++~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > extra "+" at the start of the underline > + > +This table sends ICMPv4 Redirect messages (RFC 792, type 5, code 1) as > +described in RFC 1812 section 5.2.7.2. When a packet is forwarded back > out > +of the logical router port it arrived on and the next hop is on the same > +network as the sender, the sender is told to use that next hop directly. > The > +original packet is still forwarded. Flows are added only for logical > router > +ports with ``options:send_icmp4_redirects`` set to ``true``. > + > +- For each logical router port *P* with > + ``options:send_icmp4_redirects=true``, a priority-110 flow with match > + ``inport == P && icmp4.type == {3, 5, 11,}`` and action ``next;``, > there is an extra trailing "," in {3, 5, 11} Also am I reading this correctly? it looks like you are saying that each port gets a flow with the match but in build_icmp_redirect_flows_for_lrouter() it looks like if there is any port on a router with the option enabled a flow gets added for icmp4.type = {3, 5, 11} with action next. Also seen in the tests > + so that no Redirect is sent in reply to icmp4 errors (RFC 1122 3.2.2). > + > +- For each IPv4 network *N* of such a port *P*, a priority-110 flow with > match > + ``ip4 && ip4.dst == N`` and action ``next;``. No Redirect is sent when > the > + destination itself is on the network of the port, so a Redirect always > + points to another router and never to the destination host. > + > +- For each IPv4 network *N* of such a port *P*, whose Ethernet address is > *E* > + and IPv4 address on *N* is *A*, a priority-100 flow with match ``inport > == P > + && outport == P && ip4 && ip4.src == N && reg0 == N && !ip.later_frag`` > and > + the following actions:: > + > + icmp4_redirect { > + eth.dst = xreg1[0..47]; > + eth.src = E; > + ip4.dst = ip4.src; > + ip4.src = A; > + ip.ttl = 254; > + outport = P; > + flags.loopback = 1; > + output; > + }; > + next; > + > + ``reg0`` holds the next hop selected by routing, and ``xreg1[0..47]`` > holds > + the original Ethernet source address saved in :ref:`IP Routing Pre > + <lr-in-15>`. The next hop is sent in the gateway field of the Redirect. > + ``ovn-controller`` does not send the Redirect if the next hop is the > source > + of the packet itself. These packets are rate-limited by the > ``icmp4-error`` > + control plane protection meter. > + > +- A priority-0 flow that matches all packets to advance to the next table. > + > +.. _lr-in-21: > + > +Ingress Table 21: DHCP Relay Response Check > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This stage process the DHCP response packets coming from the DHCP server. > @@ -3366,9 +3420,9 @@ This stage process the DHCP response packets coming > from the DHCP server. > > - A priority-0 flow that matches all packets to advance to the next table. > > -.. _lr-in-21: > +.. _lr-in-22: > > -Ingress Table 21: DHCP Relay Response > +Ingress Table 22: DHCP Relay Response > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This stage process the DHCP response packets on which > ``dhcp_relay_resp_chk`` > @@ -3393,9 +3447,9 @@ action is applied in the previous stage. > > - A priority-0 flow that matches all packets to advance to the next table. > > -.. _lr-in-22: > +.. _lr-in-23: > > -Ingress Table 22: ARP/ND Resolution > +Ingress Table 23: ARP/ND Resolution > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > it isn't touched by this patch so it is a few lines below but there is a stale table reference ":ref:`ARP Request <lr-in-27>`" I think it should be incremeneted to lr-in-28 > > Any packet that reaches this table is an IP packet whose next-hop IPv4 > address > @@ -3509,9 +3563,9 @@ contains the final destination.) This table > resolves the IP address in ``reg0`` > !is_chassis_resident("cr-ROUTER_PORT")`` has actions ``eth.dst = E; > next;``, > where *E* is the ethernet address of the logical router port. > > -.. _lr-in-23: > +.. _lr-in-24: > > -Ingress Table 23: Check packet length > +Ingress Table 24: Check packet length > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > For distributed logical routers or gateway routers with gateway port > configured > @@ -3534,9 +3588,9 @@ flow is added, with priority-55, to bypass the > ``check_pkt_larger`` flow. > This table adds one priority-0 fallback flow that matches all packets and > advances to the next table. > > -.. _lr-in-24: > +.. _lr-in-25: > > -Ingress Table 24: Handle larger packets > +Ingress Table 25: Handle larger packets > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > For distributed logical routers or gateway routers with gateway port > configured > @@ -3583,9 +3637,9 @@ respectively:: > This table adds one priority-0 fallback flow that matches all packets and > advances to the next table. > > -.. _lr-in-25: > +.. _lr-in-26: > > -Ingress Table 25: Gateway Redirect > +Ingress Table 26: Gateway Redirect > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > For distributed logical routers where one or more of the logical router > ports > @@ -3633,9 +3687,9 @@ following flows: > > - A priority-0 logical flow with match ``1`` has actions ``next;``. > > -.. _lr-in-26: > +.. _lr-in-27: > > -Ingress Table 26: Network ID > +Ingress Table 27: Network ID > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This table contains flows that set ``flags.network_id`` for IP packets: > @@ -3661,9 +3715,9 @@ This table contains flows that set > ``flags.network_id`` for IP packets: > > - Catch-all: A priority-0 flow with match ``1`` has actions ``next;``. > > -.. _lr-in-27: > +.. _lr-in-28: > > -Ingress Table 27: ARP Request > +Ingress Table 28: ARP Request > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > In the common case where the Ethernet destination has been resolved, this > table > @@ -3696,9 +3750,9 @@ or IPv6 Neighbor Solicitation request. It holds the > following flows: > > - Known MAC address. A priority-0 flow with match ``1`` has actions > ``next;``. > > -.. _lr-in-28: > +.. _lr-in-29: > > -Ingress Table 28: ECMP symmetric reply processing for egress > +Ingress Table 29: ECMP symmetric reply processing for egress > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > This table contains logical flows that commit IP traffic forwarded by ECMP > diff --git a/NEWS b/NEWS > index f1c56dd14..f75309b9d 100644 > --- a/NEWS > +++ b/NEWS > @@ -129,6 +129,11 @@ OVN v26.09.0 - xxx xx xxxx > represent a logical router port (e.g. ovn-ic transit switch LSPs). > Such ports are treated like type=router, including omission from > _MC_flood_l2. > + - Logical_Router_Port: Added a new "options:send_icmp4_redirects" key. > + If set to true, the router sends an ICMPv4 Redirect (RFC 1812, > 5.2.7.2) > + to the source of a packet that is forwarded back out of the port it > + arrived on, when the source and the next hop are on the same network > + of that port. Disabled by default. > > OVN v26.03.0 - xxx xx xxxx > -------------------------- > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > index f8094afc3..3d3bebf9f 100644 > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -1007,8 +1007,8 @@ ip_address_and_port_from_lb_key(const char *key, > char **ip_address, > * > * NOTE: If OVN_NORTHD_PIPELINE_CSUM is updated make sure to double check > * whether an update of OVN_INTERNAL_MINOR_VER is required. */ > -#define OVN_NORTHD_PIPELINE_CSUM "289408664 11318" > -#define OVN_INTERNAL_MINOR_VER 16 > +#define OVN_NORTHD_PIPELINE_CSUM "4027926261 <(402)%20792-6261> 11398" > +#define OVN_INTERNAL_MINOR_VER 17 > > /* Returns the OVN version. The caller must free the returned value. */ > char * > diff --git a/northd/northd.c b/northd/northd.c > index 4eb2ea44b..3fdcf746d 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -234,6 +234,13 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2)); > #define REG_POLICY_CHAIN_ID "reg9[16..31]" > #define REG_ROUTE_TABLE_ID "reg7" > > +/* Register that holds the Ethernet source address of the packet as > + * received. Must be saved, since routing will overwrite eth.src > + * with the egress router port's address. Read back when building > + * the ICMP redirect packet. > + * Only set for ports with options:send_icmp4_redirects=true. */ > +#define REG_ORIG_ETH_SRC "xreg1[0..47]" > + > /* Registers used for pasing observability information for switches: > * domain and point ID. */ > #define REG_OBS_POINT_ID_NEW "reg3" > @@ -330,18 +337,18 @@ static const char *reg_ct_state[] = { > * | R0 | REGBIT_ND_RA_OPTS_RESULT | | | | > | > * | | (= IN_ND_RA_OPTIONS) | X | | | > | > * | | NEXT_HOP_IPV4 | R | | | > | > - * | | (>= IN_IP_ROUTING) | E | INPORT_ETH_ADDR | X | > | > - * +-----+---------------------------+ G | (< IP_INPUT) | X | > | > - * | R1 | REG_CT_TP_DST (0..15) | 0 | | R | > | > - * | | REG_CT_PROTO (16..23) | | | E | > NEXT_HOP_IPV6 (>= IN_IP_ROUTING) | > - * | | (>= IN_CT_EXTRACT && | | | G | > | > - * | | <= IN_LB_AFF_LEARN) | | | | > | > - * > +-----+---------------------------+---+-----------------+---+------------------------------------+ > + * | | (>= IN_IP_ROUTING) | E | INPORT_ETH_ADDR | | > | > + * +-----+---------------------------+ G | (< IP_INPUT) | | > | > + * | R1 | REG_CT_TP_DST (0..15) | 0 | | X | > | > + * | | REG_CT_PROTO (16..23) | | | X | > NEXT_HOP_IPV6 (>= IN_IP_ROUTING) | > + * | | (>= IN_CT_EXTRACT && | | | R | > | > + * | | <= IN_LB_AFF_LEARN) | | | E | > | > + * +-----+---------------------------+---+-----------------+ G > +------------------------------------+ > * | R2 | REG_DHCP_RELAY_DIP_IPV4 | | | 0 | > | > - * | | REG_LB_PORT | X | | 0 | > | > - * | | (>= IN_LB_AFF_CHECK | R | | | > | > - * | | <= IN_LB_AFF_LEARN) | E | | | > | > - * +-----+---------------------------+ G | UNUSED | | > | > + * | | REG_LB_PORT | X | | | > | > + * | | (>= IN_LB_AFF_CHECK | R | REG_ORIG_ETH_SRC| | > | > + * | | <= IN_LB_AFF_LEARN) | E | (>= IP_ROUTING_ | | > | > + * +-----+---------------------------+ G |PRE <= ICMP_RED) | | > | > * | R3 | UNUSED | 1 | | | > | > * | | | | | | > | > * > +-----+---------------------------+---+-----------------+---+------------------------------------+ > @@ -12503,13 +12510,18 @@ build_route_table_lflow(struct ovn_datapath *od, > struct lflow_table *lflows, > > const char *route_table_name = smap_get(&lrp->options, "route_table"); > uint32_t rtb_id = get_route_table_id(route_tables, route_table_name); > - if (!rtb_id) { > + bool icmp_redirect = > + smap_get_bool(&lrp->options, "send_icmp4_redirects", false); > + if (!rtb_id && !icmp_redirect) { > return; > } > > ds_put_format(&match, "inport == \"%s\"", lrp->name); > - ds_put_format(&actions, "%s = %d; next;", > - REG_ROUTE_TABLE_ID, rtb_id); > + if (icmp_redirect) { > + /* Routing overwrites eth.src, save it for the ICMP redirect. */ > + ds_put_format(&actions, "%s = eth.src; ", REG_ORIG_ETH_SRC); > + } > + ds_put_format(&actions, "%s = %d; next;", REG_ROUTE_TABLE_ID, rtb_id); > > ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING_PRE, 100, > ds_cstr(&match), ds_cstr(&actions), lflow_ref); > @@ -12704,6 +12716,90 @@ parsed_route_lookup_by_source(enum route_source > source, > return NULL; > } > > +/* Flow for building ICMP redirect packet (ICMP error type 5, > + * code 1 - Redirect for Destination Host). > + * > + * RFC 1812 5.2.7.2 allows the Redirect only when: > + * 1) the ingress and egress interfaces are the same. > + * 2) the next hop sits on an ingress port network. > + * 3) the next hop does not match source ip - this is > + * checked by ovn-controller. > + * 4) the packet is not icmp redirect itself. > + * > + * RFC 1812 lists additional conditions for sending a Redirect (e.g. the > + * datagram is not source-routed (LSRR/SSRR options)), but OVN currently > + * has no way to check those conditions, so they are not enforced here. > + * > + * We also rely on the destination not being on the ingress port network > + * itself, so the Redirect always points to another router and never to > the > + * destination host. > + */ > +static void > +build_icmp_redirect_flows_for_lrouter_port( > + struct lflow_table *lflows, const struct ovn_port *op, > + const struct shash *meter_groups, struct lflow_ref *lflow_ref, > + struct ds *match, struct ds *actions) > +{ > + if (!smap_get_bool(&op->nbrp->options, "send_icmp4_redirects", > false)) { > + return; > + } > + > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > + const struct ipv4_netaddr *na = &op->lrp_networks.ipv4_addrs[i]; > + > + ds_clear(match); > + ds_clear(actions); > + ds_put_format(match, "ip4 && ip4.dst == %s/%u", > + na->network_s, na->plen); > + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_ICMP_REDIRECT, 110, > + ds_cstr(match), "next;", lflow_ref); > + ds_clear(match); > + ds_clear(actions); > + > + ds_put_format(match, > + "inport == %s && outport == %s && ip4 && " > + "ip4.src == %s/%u && " > + REG_NEXT_HOP_IPV4" == %s/%u && " > + "!ip.later_frag", > + op->json_key, op->json_key, > + na->network_s, na->plen, na->network_s, na->plen); > + > + ds_put_format(actions, > + "icmp4_redirect {" > + "eth.dst = "REG_ORIG_ETH_SRC"; eth.src = %s; " > + "ip4.dst = ip4.src; ip4.src = %s; ip.ttl = 254; " > + "outport = %s; flags.loopback = 1; output; }; > next;", > + op->lrp_networks.ea_s, na->addr_s, op->json_key); > + > + ovn_lflow_add(lflows, op->od, S_ROUTER_IN_ICMP_REDIRECT, 100, > + ds_cstr(match), ds_cstr(actions), lflow_ref, > + WITH_CTRL_METER(copp_meter_get(COPP_ICMP4_ERR, > + op->od->nbr->copp, > + meter_groups)), > + WITH_HINT(&op->nbrp->header_)); > + } > +} > + > +static void > +build_icmp_redirect_flows_for_lrouter(struct ovn_datapath *od, > + struct lflow_table *lflows) > +{ > + ovn_lflow_add(lflows, od, S_ROUTER_IN_ICMP_REDIRECT, 0, "1", "next;", > + od->datapath_lflows); > + struct ovn_port *op; > + HMAP_FOR_EACH (op, dp_node, &od->ports) { > + if (op->lrp_networks.n_ipv4_addrs > + && smap_get_bool(&op->nbrp->options, > + "send_icmp4_redirects", false)) { > + /* No Redirect in reply to a Redirect (RFC 1122 3.2.2). */ > + ovn_lflow_add(lflows, od, S_ROUTER_IN_ICMP_REDIRECT, 110, > + "icmp4.type == {3, 5, 11}", "next;", > + od->datapath_lflows); > + break; > + } > + } > +} > + > /* This hash needs to be equal to the one used in > * build_route_flows_for_lrouter to iterate over all routes of a datapath. > * This is distinct from route_hash which is stored in > parsed_route->hash. */ > @@ -15641,6 +15737,7 @@ build_route_flows_for_lrouter( > { > ovs_assert(od->nbr); > build_default_route_flows_for_lrouter(od, lflows, route_tables); > + build_icmp_redirect_flows_for_lrouter(od, lflows); > > const struct group_ecmp_datapath *datapath_node = > group_ecmp_datapath_lookup(route_data, od); > @@ -17800,6 +17897,10 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, > match, actions, > meter_groups, lflow_ref); > > + /* ICMP redirect */ > + build_icmp_redirect_flows_for_lrouter_port(lflows, op, meter_groups, > + lflow_ref, match, actions); > + > /* ARP reply. These flows reply to ARP requests for the router's own > * IP address. */ > for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > diff --git a/northd/northd.h b/northd/northd.h > index 9a74a4abc..549907245 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -611,17 +611,18 @@ ls_has_localnet_port(const struct ovn_datapath *od) > PIPELINE_STAGE(ROUTER, IN, IP_ROUTING_ECMP, 17, > "lr_in_ip_routing_ecmp") \ > PIPELINE_STAGE(ROUTER, IN, POLICY, 18, "lr_in_policy") > \ > PIPELINE_STAGE(ROUTER, IN, POLICY_ECMP, 19, > "lr_in_policy_ecmp") \ > - PIPELINE_STAGE(ROUTER, IN, DHCP_RELAY_RESP_CHK, 20, > \ > + PIPELINE_STAGE(ROUTER, IN, ICMP_REDIRECT, 20, > "lr_in_icmp_redirect") \ > + PIPELINE_STAGE(ROUTER, IN, DHCP_RELAY_RESP_CHK, 21, > \ > "lr_in_dhcp_relay_resp_chk") > \ > - PIPELINE_STAGE(ROUTER, IN, DHCP_RELAY_RESP, 21, > \ > + PIPELINE_STAGE(ROUTER, IN, DHCP_RELAY_RESP, 22, > \ > "lr_in_dhcp_relay_resp") > \ > - PIPELINE_STAGE(ROUTER, IN, ARP_RESOLVE, 22, > "lr_in_arp_resolve") \ > - PIPELINE_STAGE(ROUTER, IN, CHK_PKT_LEN, 23, > "lr_in_chk_pkt_len") \ > - PIPELINE_STAGE(ROUTER, IN, LARGER_PKTS, 24, > "lr_in_larger_pkts") \ > - PIPELINE_STAGE(ROUTER, IN, GW_REDIRECT, 25, > "lr_in_gw_redirect") \ > - PIPELINE_STAGE(ROUTER, IN, NETWORK_ID, 26, "lr_in_network_id") > \ > - PIPELINE_STAGE(ROUTER, IN, ARP_REQUEST, 27, > "lr_in_arp_request") \ > - PIPELINE_STAGE(ROUTER, IN, ECMP_STATEFUL_EGR, 28, > \ > + PIPELINE_STAGE(ROUTER, IN, ARP_RESOLVE, 23, > "lr_in_arp_resolve") \ > + PIPELINE_STAGE(ROUTER, IN, CHK_PKT_LEN, 24, > "lr_in_chk_pkt_len") \ > + PIPELINE_STAGE(ROUTER, IN, LARGER_PKTS, 25, > "lr_in_larger_pkts") \ > + PIPELINE_STAGE(ROUTER, IN, GW_REDIRECT, 26, > "lr_in_gw_redirect") \ > + PIPELINE_STAGE(ROUTER, IN, NETWORK_ID, 27, "lr_in_network_id") > \ > + PIPELINE_STAGE(ROUTER, IN, ARP_REQUEST, 28, > "lr_in_arp_request") \ > + PIPELINE_STAGE(ROUTER, IN, ECMP_STATEFUL_EGR, 29, > \ > "lr_in_ecmp_stateful_egr") > > /* Logical router egress stages. */ > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 078bd6068..14fcf49c5 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -5028,6 +5028,23 @@ or > router version of this option. > </p> > </column> > + > + <column name="options" key="send_icmp4_redirects" > + type='{"type": "boolean"}'> > + <p> > + If set to <code>true</code>, the router sends an ICMPv4 > Redirect to > + the source of a packet that arrived on this port and is > forwarded > + back out of it, provided that the source and the next hop are on > + the same network of the port. The Redirect names the next hop as > + the better first hop; the packet itself is still forwarded. > + No Redirect is sent when : > extra space "when :" -> "when:" > + - The next hop is the source of the packet. > + - When the packet is itself an ICMP Error. > + - When the destination itself belongs to one of directly > connected > + router's network: a Redirect always points to another router. > + It is <code>false</code> by default. > + </p> > + </column> > </group> > > <group title="Attachment"> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 6572b1318..cdff4d06c 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -24352,3 +24352,71 @@ OVN_CLEANUP_NORTHD > AT_CLEANUP > ]) > > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([ICMPv4 redirect]) > +ovn_start > + > +check ovn-nbctl lr-add lr1 > +check ovn-nbctl lrp-add lr1 lrp0 00:00:00:00:00:01 192.168.1.1/24 > +check ovn-nbctl lrp-add lr1 lrp1 00:00:00:00:00:02 10.0.0.1/24 > 10.0.1.1/24 > +check ovn-nbctl --wait=sb lrp-add lr1 lrp2 00:00:00:00:00:03 > 2001:db8::1/64 > + > +# Off by default. > +AT_CHECK([ovn-sbctl lflow-list | grep "lr_in_icmp_redirect" | > ovn_strip_lflows], [0], [dnl > + table=??(lr_in_icmp_redirect), priority=0 , match=(1), action=(next;) > +]) > +AT_CHECK([ovn-sbctl lflow-list | grep "lr_in_ip_routing_pre" | > ovn_strip_lflows], [0], [dnl > + table=??(lr_in_ip_routing_pre), priority=0 , match=(1), action=(reg7 > = 0; next;) > +]) > + > +# Enabled per router port. > +check ovn-nbctl --wait=sb set logical_router_port lrp1 > options:send_icmp4_redirects=true > +AT_CHECK([ovn-sbctl lflow-list | grep "lr_in_icmp_redirect" | > ovn_strip_lflows], [0], [dnl > + table=??(lr_in_icmp_redirect), priority=0 , match=(1), action=(next;) > + table=??(lr_in_icmp_redirect), priority=100 , match=(inport == "lrp1" > && outport == "lrp1" && ip4 && ip4.src == 10.0.0.0/24 && reg0 == > 10.0.0.0/24 && !ip.later_frag), action=(icmp4_redirect {eth.dst = > xreg1[[0..47]]; eth.src = 00:00:00:00:00:02; ip4.dst = ip4.src; ip4.src = > 10.0.0.1; ip.ttl = 254; outport = "lrp1"; flags.loopback = 1; output; }; > next;) > + table=??(lr_in_icmp_redirect), priority=100 , match=(inport == "lrp1" > && outport == "lrp1" && ip4 && ip4.src == 10.0.1.0/24 && reg0 == > 10.0.1.0/24 && !ip.later_frag), action=(icmp4_redirect {eth.dst = > xreg1[[0..47]]; eth.src = 00:00:00:00:00:02; ip4.dst = ip4.src; ip4.src = > 10.0.1.1; ip.ttl = 254; outport = "lrp1"; flags.loopback = 1; output; }; > next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(icmp4.type == {3, > 5, 11}), action=(next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(ip4 && ip4.dst == > 10.0.0.0/24), action=(next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(ip4 && ip4.dst == > 10.0.1.0/24), action=(next;) > +]) > + > +# The original eth.src is saved only on ports with the option enabled. > +AT_CHECK([ovn-sbctl lflow-list | grep "lr_in_ip_routing_pre" | > ovn_strip_lflows], [0], [dnl > + table=??(lr_in_ip_routing_pre), priority=0 , match=(1), action=(reg7 > = 0; next;) > + table=??(lr_in_ip_routing_pre), priority=100 , match=(inport == > "lrp1"), action=(xreg1[[0..47]] = eth.src; reg7 = 0; next;) > +]) > + > +# Together with a route table. > +check ovn-nbctl --wait=sb set logical_router_port lrp1 > options:route_table=rtb1 > +AT_CHECK([ovn-sbctl lflow-list | grep "lr_in_ip_routing_pre" | > ovn_strip_lflows], [0], [dnl > + table=??(lr_in_ip_routing_pre), priority=0 , match=(1), action=(reg7 > = 0; next;) > + table=??(lr_in_ip_routing_pre), priority=100 , match=(inport == > "lrp1"), action=(xreg1[[0..47]] = eth.src; reg7 = 1; next;) > +]) > +check ovn-nbctl --wait=sb remove logical_router_port lrp1 options > route_table > + > +check ovn-nbctl --wait=sb set logical_router_port lrp0 > options:send_icmp4_redirects=true > +check ovn-nbctl --wait=sb set logical_router_port lrp2 > options:send_icmp4_redirects=true > +AT_CHECK([ovn-sbctl lflow-list | grep "lr_in_icmp_redirect" | > ovn_strip_lflows], [0], [dnl > + table=??(lr_in_icmp_redirect), priority=0 , match=(1), action=(next;) > + table=??(lr_in_icmp_redirect), priority=100 , match=(inport == "lrp0" > && outport == "lrp0" && ip4 && ip4.src == 192.168.1.0/24 && reg0 == > 192.168.1.0/24 && !ip.later_frag), action=(icmp4_redirect {eth.dst = > xreg1[[0..47]]; eth.src = 00:00:00:00:00:01; ip4.dst = ip4.src; ip4.src = > 192.168.1.1; ip.ttl = 254; outport = "lrp0"; flags.loopback = 1; output; }; > next;) > + table=??(lr_in_icmp_redirect), priority=100 , match=(inport == "lrp1" > && outport == "lrp1" && ip4 && ip4.src == 10.0.0.0/24 && reg0 == > 10.0.0.0/24 && !ip.later_frag), action=(icmp4_redirect {eth.dst = > xreg1[[0..47]]; eth.src = 00:00:00:00:00:02; ip4.dst = ip4.src; ip4.src = > 10.0.0.1; ip.ttl = 254; outport = "lrp1"; flags.loopback = 1; output; }; > next;) > + table=??(lr_in_icmp_redirect), priority=100 , match=(inport == "lrp1" > && outport == "lrp1" && ip4 && ip4.src == 10.0.1.0/24 && reg0 == > 10.0.1.0/24 && !ip.later_frag), action=(icmp4_redirect {eth.dst = > xreg1[[0..47]]; eth.src = 00:00:00:00:00:02; ip4.dst = ip4.src; ip4.src = > 10.0.1.1; ip.ttl = 254; outport = "lrp1"; flags.loopback = 1; output; }; > next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(icmp4.type == {3, > 5, 11}), action=(next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(ip4 && ip4.dst == > 10.0.0.0/24), action=(next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(ip4 && ip4.dst == > 10.0.1.0/24), action=(next;) > + table=??(lr_in_icmp_redirect), priority=110 , match=(ip4 && ip4.dst == > 192.168.1.0/24), action=(next;) > +]) > + > +check ovn-nbctl --wait=sb set logical_router_port lrp0 > options:send_icmp4_redirects=false > +check ovn-nbctl --wait=sb remove logical_router_port lrp1 options > send_icmp4_redirects > +AT_CHECK([ovn-sbctl lflow-list | grep "lr_in_icmp_redirect" | > ovn_strip_lflows], [0], [dnl > + table=??(lr_in_icmp_redirect), priority=0 , match=(1), action=(next;) > +]) > +AT_CHECK([ovn-sbctl lflow-list | grep "lr_in_ip_routing_pre" | > ovn_strip_lflows], [0], [dnl > + table=??(lr_in_ip_routing_pre), priority=0 , match=(1), action=(reg7 > = 0; next;) > + table=??(lr_in_ip_routing_pre), priority=100 , match=(inport == > "lrp2"), action=(xreg1[[0..47]] = eth.src; reg7 = 0; next;) > +]) > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > +]) > diff --git a/tests/ovn.at b/tests/ovn.at > index 13e95f9db..7e6b27bbe 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -47407,3 +47407,39 @@ AT_CHECK([grep "skipping output to input port" \ > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([ICMPv4 redirect - ovn-trace]) > +ovn_start > + > +check ovn-nbctl lr-add lr1 > +check ovn-nbctl lrp-add lr1 lrp0 00:00:00:00:00:01 10.0.0.1/24 > +check ovn-nbctl lrp-set-options lrp0 send_icmp4_redirects=true > +check ovn-nbctl ls-add sw1 > +check ovn-nbctl lsp-add-router-port sw1 sw-lr1 lrp0 > +check ovn-nbctl lsp-add sw1 sw1-lport1 > +check ovn-nbctl lsp-set-addresses sw1-lport1 "00:00:00:00:00:10 10.0.0.10" > + > +# A better first hop for 172.16.0.0/24 sits on the same segment as the > VM. > +check ovn-nbctl --wait=sb lr-route-add lr1 172.16.0.0/24 10.0.0.80 > + > +AT_CHECK([ovn-trace lr1 'inport == "lrp0" && eth.src == 00:00:00:00:00:10 > && eth.dst == 00:00:00:00:00:01 && ip4.src == 10.0.0.10 && ip4.dst == > 172.16.0.5 && ip.ttl == 64 && icmp4.type == 8' | > + grep -c "icmp4_redirect: gw = 10.0.0.80"], [0], [dnl > +1 > +]) > + > +# No Redirect in reply to a Redirect. > +AT_CHECK([ovn-trace lr1 'inport == "lrp0" && eth.src == 00:00:00:00:00:10 > && eth.dst == 00:00:00:00:00:01 && ip4.src == 10.0.0.10 && ip4.dst == > 172.16.0.5 && ip.ttl == 64 && icmp4.type == 5' | \ > + grep -c "icmp4_redirect"], [1], [dnl > +0 > +]) > + > +# No Redirect to the next hop itself. > +AT_CHECK([ovn-trace lr1 'inport == "lrp0" && eth.src == 00:00:00:00:00:80 > && eth.dst == 00:00:00:00:00:01 && ip4.src == 10.0.0.80 && ip4.dst == > 172.16.0.5 && ip.ttl == 64 && icmp4.type == 8' | \ > + grep -c "icmp4_redirect: suppressed, next hop 10.0.0.80 is the > source"], [0], [dnl > +1 > +]) > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > +]) > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 5b6ba3731..f18f0b06b 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -23978,3 +23978,109 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query > port patch-.*/d > > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ICMPv4 redirect]) > +AT_KEYWORDS([icmp redirect]) > + > +ovn_start > + > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_BR([br-int]) > + > +# Set external-ids in br-int needed for ovn-controller > +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 > + > +# One switch, one router. sw0-p1 is the host with the bad routing table: > +# it sends everything to lr0. sw0-p2 is the better first hop: it also owns > +# 10.10.10.1, and lr0 has a static route for 10.10.10.0/24 pointing at > it. > +# So a packet from sw0-p1 to 10.10.10.1 goes lr0 -> sw0-p2, back out of > the > +# port it came in on, and lr0 must tell sw0-p1 to use sw0-p2 directly. > +check ovn-nbctl ls-add sw0 > + > +check ovn-nbctl lsp-add sw0 sw0-p1 > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:04 172.31.0.4" > + > +check ovn-nbctl lsp-add sw0 sw0-p2 > +check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:50 172.31.0.80" > + > +check ovn-nbctl lr-add lr0 > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 172.31.0.1/16 > +check ovn-nbctl lrp-set-options lr0-sw0 send_icmp4_redirects=true > +check ovn-nbctl lsp-add-router-port sw0 sw0-lr0 lr0-sw0 > +check ovn-nbctl lr-route-add lr0 10.10.10.0/24 172.31.0.80 > + > +ADD_NAMESPACES(sw0-p1) > +ADD_VETH(sw0-p1, sw0-p1, br-int, "172.31.0.4/16", "50:54:00:00:00:04", \ > + "172.31.0.1") > +NS_CHECK_EXEC([sw0-p1], [sysctl -q -w > net.ipv4.conf.all.accept_redirects=1]) > +NS_CHECK_EXEC([sw0-p1], [sysctl -q -w > net.ipv4.conf.sw0-p1.accept_redirects=1]) > + > +ADD_NAMESPACES(sw0-p2) > +ADD_VETH(sw0-p2, sw0-p2, br-int, "172.31.0.80/16", "50:54:00:00:00:50") > +NS_CHECK_EXEC([sw0-p2], [ip addr add 10.10.10.1/24 dev sw0-p2]) > + > +OVN_POPULATE_ARP > +check ovn-nbctl --wait=hv sync > + > +NS_CHECK_EXEC([sw0-p1], [ping -q -c 1 -w 2 172.31.0.80 | FORMAT_PING], \ > +[0], [dnl > +1 packets transmitted, 1 received, 0% packet loss, time 0ms > +]) > + > +NETNS_START_TCPDUMP([sw0-p1], [-nn -i sw0-p1 icmp and src 172.31.0.1], > [sw0-p1-redirect]) > +NETNS_START_TCPDUMP([sw0-p2], [-nn -e -i sw0-p2 icmp and dst 10.10.10.1], > [sw0-p2-echo]) > + > +NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.10.10.1 | > FORMAT_PING], \ > +[0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > + > +OVS_WAIT_UNTIL([ > + grep -q "172.31.0.1 > 172.31.0.4: ICMP redirect 10.10.10.1 to host > 172.31.0.80" sw0-p1-redirect.tcpdump > +]) > + > +# The first echo request went through lr0, i.e. left it with lr0's MAC. > +OVS_WAIT_UNTIL([ > + grep "00:00:00:00:ff:01 > 50:54:00:00:00:50" sw0-p2-echo.tcpdump | > grep -q "172.31.0.4 > 10.10.10.1" > +]) > + > +# sw0-p1 applied the Redirect: later ones come straight from it. > +OVS_WAIT_UNTIL([ > + grep "50:54:00:00:00:04 > 50:54:00:00:00:50" sw0-p2-echo.tcpdump | > grep -q "172.31.0.4 > 10.10.10.1" > +]) > +AT_CHECK([ip netns exec sw0-p1 ip route get 10.10.10.1 | grep -q "via > 172.31.0.80"]) > + > +# Turning the option off stops the Redirects but not the traffic. > +check ovn-nbctl --wait=hv set logical_router_port lr0-sw0 > options:send_icmp4_redirects=false > +# Drop the learned exception so that sw0-p1 goes through lr0 again. > +NS_CHECK_EXEC([sw0-p1], [ip route flush cache]) > +kill $(cat sw0-p1-redirect.pid) > +rm -f sw0-p1-redirect.tcpdump > +NETNS_START_TCPDUMP([sw0-p1], [-nn -i sw0-p1 icmp and src 172.31.0.1], > [sw0-p1-redirect]) > + > +NS_CHECK_EXEC([sw0-p1], [ping -q -c 3 -i 0.3 -w 2 10.10.10.1 | > FORMAT_PING], \ > +[0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > +ovn-sbctl lflow-list | grep icmp_redirect > +AT_CHECK([grep -c "ICMP redirect" sw0-p1-redirect.tcpdump], [1], [dnl > +0 > +]) > + > +OVN_CLEANUP_CONTROLLER([hv1]) > +OVN_CLEANUP_NORTHD > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > +/connection dropped.*/d"]) > +AT_CLEANUP > +]) > -- > 2.48.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Jacob _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
