On 4/3/25 14:57, Dumitru Ceara wrote:
> Hi Martin,
>
> Thanks for the RFC!
>
> On 4/3/25 2:43 PM, Martin Morgenstern via dev wrote:
>> Add LR option "fallback_to_routing_policy" that, when set to true,
>> changes router behavior to always evaluate routing policies, even if
>> there was no match in the routing table (otherwise it would simply drop
>> the packet).
>>
>> If the option is set, we adjust the lflows in such a way that the
>> decision to drop the packet is delayed until we reach the lr_in_policy
>> pipeline.
>>
>> The change takes advantage of the fact that the IPv4 and IPv6 src ip
>> and nexthop registers share bits. Thus, at least at the moment, we
>> don't need to use another register to save the lookup result (match or
>> no match) from the lr_in_ip_routing pipeline.
>>
>> Having this option is useful for routers that do not have a default
>> route and one still wants to make use of the more advanced matching
>> capabilities of routing policies.
>>
Hi Dumitru,
thanks for your feedback!
> I didn't look at the code yet but I was thinking of the use case you
> mentioned above. Aren't the two parts of the phrase above a bit
> contradicting eachother?
>
> I mean sure, there's no default route but you'd like a default action,
> i.e., apply routing policies. Isn't that essentially achievable by
> adding a default route with any next-hop IP and a default, lowest
> priority, policy that drops everything not matched by policies?
You're right, this can be done with OVN out of the box.
However, when OpenStack Neutron comes into play we are facing some
issues. It takes ownership of the logical router and discards (dummy)
routes it doesn't know. Routing policies, on the other hand, (at least
for now) are not touched by Neutron and this is why we came up with this
LR option.
> Or am I simplifying the use case too much?
We have use cases where we need routing policies with Neutron routers
without an external gateway (projects without internet access). In that
case, unless some custom routes are added via Neutron, the route table
is empty and policies are not evaluated.
To make managing all of this easier (and hide confusing implementation
details from users), we wanted to avoid the overhead of (repeatedly)
adding dummy routes, ports and other objects that are required to
achieve this with Neutron.
> I'm asking because I'd like to avoid new knobs if possible.
I understand and agree that this would come with a cost (I think it
could increase the complexity of the tests quite a lot).
With your feedback in mind, I am currently evaluating other ideas,
including reevaluating the already mentioned approach with the dummy
objects, on the Neutron side. Depending on what I find out, I'll come
back to discuss this on ovs-discuss again.
Thanks,Martin
> Thanks,
> Dumitru
>
>> Signed-off-by: Martin Morgenstern <martin.morgenst...@cloudandheat.com>
>> ---
>
>
>> NEWS | 3 +++
>> northd/northd.c | 51 +++++++++++++++++++++++++++++++++++++++++--------
>> ovn-nb.xml | 10 ++++++++++
>> 3 files changed, 56 insertions(+), 8 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 656176d20..bcbc8fb65 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -4,6 +4,9 @@ Post v25.03.0
>> external-ids, this option allows to specify if ovn-controller should
>> perform cleanup when exiting. The "--restart" exit always has priority
>> to keep the backward compatibility.
>> + - Added a new logical router option "fallback_to_routing_policy" that
>> + changes router behavior to always evaluate routing policies even if
>> + there was no match in the routing table, instead of dropping the
>> packet.
>>
>> OVN v25.03.0 - 07 Mar 2025
>> --------------------------
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 471d9969b..41e3f973f 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -275,7 +275,7 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2));
>> * | | | | | E |
>> NEXT_HOP_IPV6 (>= IN_IP_ROUTING) |
>> * +-----+---------------------------+---+-----------------+ G |
>> |
>> * | R2 | REG_DHCP_RELAY_DIP_IPV4 | | | 0 |
>> |
>> - * | | REG_LB_PORT | X | | 0 |
>> |
>> + * | | REG_LB_PORT | X | | |
>> |
>> * | | (>= IN_LB_AFF_CHECK | R | | |
>> |
>> * | | <= IN_LB_AFF_LEARN) | E | | |
>> |
>> * +-----+---------------------------+ G | UNUSED | |
>> |
>> @@ -283,11 +283,11 @@ BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2));
>> * | | | | | |
>> |
>> *
>> +-----+---------------------------+---+-----------------+---+------------------------------------+
>> * | R4 | REG_LB_IPV4 | X | | |
>> |
>> - * | | (>= IN_LB_AFF_CHECK && | R | | |
>> |
>> - * | | <= IN_LB_AFF_LEARN) | R | | |
>> |
>> - * +-----+---------------------------+ E | UNUSED | X |
>> |
>> - * | R5 | SRC_IPV4 for ARP-REQ | G | | X |
>> REG_LB_IPV6 |
>> - * | | (>= IP_INPUT) | 2 | | R | (>=
>> IN_LB_AFF_CHECK && |
>> + * | | (>= IN_LB_AFF_CHECK && | R | | |
>> REG_SRC_IPV6 |
>> + * | | <= IN_LB_AFF_LEARN) | E | | | (>=
>> IN_IP_ROUTING && |
>> + * +-----+---------------------------+ G | UNUSED | X | <=
>> IN_POLICY_ECMP) / |
>> + * | R5 | SRC_IPV4 for ARP-REQ | 2 | | X |
>> REG_LB_IPV6 |
>> + * | | (>= IP_INPUT) | | | R | (>=
>> IN_LB_AFF_CHECK && |
>> * +-----+---------------------------+---+-----------------+ E | <=
>> IN_LB_AFF_LEARN) |
>> * | R6 | UNUSED | X | | G |
>> |
>> * | | | R | | 1 |
>> |
>> @@ -1432,6 +1432,12 @@ lsp_disable_arp_nd_rsp(const struct
>> nbrec_logical_switch_port *nbsp)
>> return smap_get_bool( ->options, "disable_arp_nd_rsp", false);
>> }
>>
>> +static bool
>> +lr_fallback_to_routing_policy(const struct nbrec_logical_router *nbr)
>> +{
>> + return smap_get_bool(&nbr->options, "fallback_to_routing_policy",
>> false);
>> +}
>> +
>> static bool
>> lsp_is_type_changed(const struct sbrec_port_binding *sb,
>> const struct nbrec_logical_switch_port *nbsp,
>> @@ -13719,8 +13725,24 @@ build_default_route_flows_for_lrouter(
>> {
>> ovn_lflow_add_default_drop(lflows, od, S_ROUTER_IN_IP_ROUTING_ECMP,
>> NULL);
>> - ovn_lflow_add_default_drop(lflows, od, S_ROUTER_IN_IP_ROUTING,
>> - NULL);
>> + if (lr_fallback_to_routing_policy(od->nbr)) {
>> + /* If we have no match in the routing table, delay the decision to
>> drop
>> + * the packet until we reach the policy table.
>> + * We zero the IPv6 src ip and nexthop registers and later use them
>> in
>> + * the lr_in_policy pipeline for the decision whether to proceed or
>> drop
>> + * the packet.
>> + */
>> + ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 0, "1",
>> + "ip.ttl--; flags.loopback = 1; "
>> + REG_ECMP_GROUP_ID" = 0; "
>> + REG_NEXT_HOP_IPV6" = 0; "
>> + REG_SRC_IPV6" = 0; "
>> + "next;",
>> + NULL);
>> + } else {
>> + ovn_lflow_add_default_drop(lflows, od, S_ROUTER_IN_IP_ROUTING,
>> + NULL);
>> + }
>> ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING_ECMP, 150,
>> REG_ECMP_GROUP_ID" == 0", "next;",
>> NULL);
>> @@ -14096,6 +14118,19 @@ build_ingress_policy_flows_for_lrouter(
>> struct lflow_ref *lflow_ref)
>> {
>> ovs_assert(od->nbr);
>> + if (lr_fallback_to_routing_policy(od->nbr)) {
>> + /* Drop the packet if there was neither a matching rule in the
>> routing
>> + * table nor a match in the policy table that set the IPv4/IPv6 src
>> ip
>> + * and nexthop registers to a non-zero value. (This takes
>> advantage of
>> + * the fact that the IPv6 registers are non-zero if IPv4 registers
>> are
>> + * non-zero, i.e., that the IPv4 register bits are a subset of the
>> IPv6
>> + * register bits.)
>> + */
>> + ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY, 1,
>> + REG_NEXT_HOP_IPV6" == 0 && "
>> + REG_SRC_IPV6" == 0",
>> + "drop;", lflow_ref);
>> + }
>> /* This is a catch-all rule. It has the lowest priority (0)
>> * does a match-all("1") and pass-through (next) */
>> ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY, 0, "1",
>> diff --git a/ovn-nb.xml b/ovn-nb.xml
>> index ff5f2f249..ef9f799bc 100644
>> --- a/ovn-nb.xml
>> +++ b/ovn-nb.xml
>> @@ -3151,6 +3151,16 @@ or
>> </p>
>> </column>
>>
>> + <column name="options" key="fallback_to_routing_policy"
>> + type='{"type": "boolean"}'>
>> + <p>
>> + If set to <code>true</code>, the routing behavior of the logical
>> + router will be changed to always evaluate routing policies instead
>> + of dropping the packet, even if there was no routing rule match in
>> + the routing table. Defaults to <code>false</code>.
>> + </p>
>> + </column>
>> +
>> <column name="options" key="ct-commit-all" type='{"type": "boolean"}'>
>> When enabled the LR will commit traffic in a zone that is stateful.
>> The traffic is not commited to both zones but it is selective based
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
