[ovs-dev] [PATCH net-next v4] net: openvswitch: Check vport netdev name

2024-04-17 Thread jun.gu 
Ensure that the provided netdev name is not one of its aliases to
prevent unnecessary creation and destruction of the vport by
ovs-vswitchd.

Signed-off-by: Jun Gu 
Acked-by: Eelco Chaudron 
---
 net/openvswitch/vport-netdev.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
index 903537a5da22..7003e76b8172 100644
--- a/net/openvswitch/vport-netdev.c
+++ b/net/openvswitch/vport-netdev.c
@@ -78,7 +78,10 @@ struct vport *ovs_netdev_link(struct vport *vport, const 
char *name)
int err;
 
vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name);
-   if (!vport->dev) {
+   /* Ensure that the device exists and that the provided
+* name is not one of its aliases.
+*/
+   if ((!vport->dev) || strcmp(name, ovs_vport_name(vport))) {
err = -ENODEV;
goto error_free_vport;
}
-- 
2.25.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH net-next] selftests: openvswitch: Fix escape chars in regexp.

2024-04-17 Thread patchwork-bot+netdevbpf 
Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski :

On Tue, 16 Apr 2024 11:09:13 +0200 you wrote:
> Character sequences starting with `\` are interpreted by python as
> escaped Unicode characters. However, they have other meaning in
> regular expressions (e.g: "\d").
> 
> It seems Python >= 3.12 starts emitting a SyntaxWarning when these
> escaped sequences are not recognized as valid Unicode characters.
> 
> [...]

Here is the summary with links:
  - [net-next] selftests: openvswitch: Fix escape chars in regexp.
https://git.kernel.org/netdev/net-next/c/3fde60afe1f8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2] ovsdb: raft: Fix probe intervals after install snapshot request.

2024-04-17 Thread Mike Pattrick 
On Thu, Apr 11, 2024 at 7:45 PM Ilya Maximets  wrote:
>
> If the new snapshot received with INSTALL_SNAPSHOT request contains
> a different election timer value, the timer is updated, but the
> probe intervals for RAFT connections are not.
>
> Fix that by updating probe intervals whenever we get election timer
> from the log.
>
> Fixes: 14b2b0aad7ae ("raft: Reintroduce jsonrpc inactivity probes.")
> Signed-off-by: Ilya Maximets 

Acked-by: Mike Pattrick 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/2] ovsdb: raft: Fix inability to join a cluster with a large database.

2024-04-17 Thread Mike Pattrick 
On Thu, Apr 11, 2024 at 7:44 PM Ilya Maximets  wrote:
>
> Inactivity probe interval on RAFT connections depend on a value of the
> election timer.  However, the actual value is not known until the
> database snapshot with the RAFT information is received by a joining
> server.  New joining server is using a default 1 second until then.
>
> In case a new joining server is trying to join an existing cluster
> with a large database, it may take more than a second to generate and
> send an initial database snapshot.  This is causing an inability to
> actually join this cluster.  Joining server sends ADD_SERVER request,
> waits 1 second, sends a probe, doesn't get a reply within another
> second, because the leader is busy preparing and sending an initial
> snapshot to it, disconnects, repeat.
>
> This is not an issue for the servers that did already join, since
> their probe intervals are larger than election timeout.
> Cooperative multitasking also doesn't fully solve this issue, since
> it depends on election timer, which is likely higher in the existing
> cluster with a very big database.
>
> Fix that by using the maximum election timer value for inactivity
> probes until the actual value is known.  We still shouldn't completely
> disable the probes, because in the rare event the connection is
> established but the other side silently goes away, we still want to
> disconnect and try to re-establish the connection eventually.
>
> Since probe intervals also depend on the joining state now, update
> them when the server joins the cluster.
>
> Fixes: 14b2b0aad7ae ("raft: Reintroduce jsonrpc inactivity probes.")
> Reported-by: Terry Wilson 
> Reported-at: https://issues.redhat.com/browse/FDP-144
> Signed-off-by: Ilya Maximets 
> ---

Acked-by: Mike Pattrick 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH net-next] selftests: openvswitch: Fix escape chars in regexp.

2024-04-17 Thread Aaron Conole 
Adrian Moreno  writes:

> Character sequences starting with `\` are interpreted by python as
> escaped Unicode characters. However, they have other meaning in
> regular expressions (e.g: "\d").
>
> It seems Python >= 3.12 starts emitting a SyntaxWarning when these
> escaped sequences are not recognized as valid Unicode characters.
>
> An example of these warnings:
>
> tools/testing/selftests/net/openvswitch/ovs-dpctl.py:505:
> SyntaxWarning: invalid escape sequence '\d'
>
> Fix all the warnings by flagging literals as raw strings.
>
> Signed-off-by: Adrian Moreno 
> ---

Thanks, Adrian.

Reviewed-by: Aaron Conole 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 ovn] northd: Fix pmtud for non routed traffic.

2024-04-17 Thread Lorenzo Bianconi 
Similar to what is already implemented for routed e/w traffic,
introduce pmtud support for e/w traffic between two logical switch ports
connected to the same logical switch, but running on two different
hypervisors.

Acked-by: Mark Michelson 
Reported-at: https://issues.redhat.com/browse/FDP-362
Signed-off-by: Lorenzo Bianconi 
---
Changes since v2:
- minor changes
Changes since v1:
- move logic in consider_port_binding
- add more self-test
- fix typos
---
 controller/lflow.h|   1 +
 controller/physical.c |  30 +++-
 northd/northd.c   |  33 +++--
 northd/ovn-northd.8.xml   |  16 +++-
 tests/multinode-macros.at |   4 +
 tests/multinode.at| 151 ++
 tests/ovn-controller.at   |  63 
 tests/ovn-macros.at   |   1 +
 tests/ovn-northd.at   |  24 --
 tests/ovn.at  |   5 +-
 10 files changed, 307 insertions(+), 21 deletions(-)

diff --git a/controller/lflow.h b/controller/lflow.h
index 9b7ffa19c..906a26280 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -94,6 +94,7 @@ struct uuid;
 #define OFTABLE_ECMP_NH  77
 #define OFTABLE_CHK_LB_AFFINITY  78
 #define OFTABLE_MAC_CACHE_USE79
+#define OFTABLE_CT_ZONE_LOOKUP   80
 
 struct lflow_ctx_in {
 struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath;
diff --git a/controller/physical.c b/controller/physical.c
index 7ee308694..25da789f0 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -1498,6 +1498,26 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
 return;
 }
 
+if (get_lport_type(binding) == LP_VIF) {
+/* Table 80, priority 100.
+ * ===
+ *
+ * Process ICMP{4,6} error packets too big locally generated from the
+ * kernel in order to lookup proper ct_zone. */
+struct match match = MATCH_CATCHALL_INITIALIZER;
+match_set_metadata(, htonll(dp_key));
+match_set_reg(, MFF_LOG_INPORT - MFF_REG0, port_key);
+
+struct zone_ids icmp_zone_ids = get_zone_ids(binding, ct_zones);
+ofpbuf_clear(ofpacts_p);
+put_zones_ofpacts(_zone_ids, ofpacts_p);
+put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
+ofctrl_add_flow(flow_table, OFTABLE_CT_ZONE_LOOKUP, 100,
+binding->header_.uuid.parts[0], ,
+ofpacts_p, >header_.uuid);
+ofpbuf_clear(ofpacts_p);
+}
+
 struct match match;
 if (!strcmp(binding->type, "patch")
 || (!strcmp(binding->type, "l3gateway")
@@ -2464,6 +2484,14 @@ physical_run(struct physical_ctx *p_ctx,
   flow_table, );
 }
 
+/* Default flow for CT_ZONE_LOOKUP Table. */
+struct match ct_look_def_match;
+match_init_catchall(_look_def_match);
+ofpbuf_clear();
+put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, );
+ofctrl_add_flow(flow_table, OFTABLE_CT_ZONE_LOOKUP, 0, 0,
+_look_def_match, , hc_uuid);
+
 /* Handle output to multicast groups, in tables 40 and 41. */
 const struct sbrec_multicast_group *mc;
 SBREC_MULTICAST_GROUP_TABLE_FOR_EACH (mc, p_ctx->mc_group_table) {
@@ -2522,7 +2550,7 @@ physical_run(struct physical_ctx *p_ctx,
 /* Add specif flows for E/W ICMPv{4,6} packets if tunnelled packets
  * do not fit path MTU.
  */
-put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, );
+put_resubmit(OFTABLE_CT_ZONE_LOOKUP, );
 
 /* IPv4 */
 match_init_catchall();
diff --git a/northd/northd.c b/northd/northd.c
index 37f443e70..82fdfbd5d 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -8662,7 +8662,7 @@ build_lswitch_lflows_admission_control(struct 
ovn_datapath *od,
 ovs_assert(od->nbs);
 
 /* Default action for recirculated ICMP error 'packet too big'. */
-ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 110,
+ovn_lflow_add(lflows, od, S_SWITCH_IN_CHECK_PORT_SEC, 105,
   "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
   " (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&"
   " flags.tunnel_rx == 1", debug_drop_action(), lflow_ref);
@@ -11858,7 +11858,22 @@ build_lswitch_icmp_packet_toobig_admin_flows(
 {
 ovs_assert(op->nbsp);
 
+ds_clear(match);
 if (!lsp_is_router(op->nbsp)) {
+if (!op->n_lsp_addrs) {
+return;
+}
+
+ds_put_format(match,
+  "((ip4 && icmp4.type == 3 && icmp4.code == 4) ||"
+  " (ip6 && icmp6.type == 2 && icmp6.code == 0)) &&"
+  " eth.src == "ETH_ADDR_FMT" && outport == %s &&"
+  " !is_chassis_resident(%s) && flags.tunnel_rx == 1",
+  ETH_ADDR_ARGS(op->lsp_addrs[0].ea), op->json_key,
+  op->json_key);
+ovn_lflow_add(lflows, op->od,

Re: [ovs-dev] [Patch ovn v3 1/2] actions: New action ct_commit_to_zone.

2024-04-17 Thread Ales Musil 
On Wed, Apr 17, 2024 at 1:05 PM Dumitru Ceara  wrote:

> On 4/17/24 09:04, Ales Musil wrote:
> > On Mon, Apr 15, 2024 at 2:15 PM Martin Kalcok <
> martin.kal...@canonical.com>
> > wrote:
> >
> >> This change adds a new action 'ct_commit_to_zone' that enables users to
> >> commit
> >> the flow into a specific zone in the connection tracker.
> >>
> >> A new feature flag, OVN_FEATURE_CT_COMMIT_TO_ZONE, is also included to
> >> avoid
> >> issues during upgrade in case the northd is upgraded to a version that
> >> supports
> >> this action before the controller is upgraded.
> >>
> >> Note that this action is meaningful only in the context of Logical
> Router
> >> datapath. Logical Switch datapath does not use multiple zones and this
> >> action
> >> falls back to committing the connection into the default zone for the
> >> Logical
> >> Switch.
> >>
> >> Signed-off-by: Martin Kalcok 
> >> ---
> >>
> >
> > Hi Martin,
> >
>
> Hi Martin, Ales,
>
> > thank you for the followup. I have a couple of comments down below.
> >
>
> I have a few of my own. :)
>
> >
> >>  controller/chassis.c  |  8 ++
> >>  include/ovn/actions.h |  1 +
> >>  include/ovn/features.h|  1 +
> >>  lib/actions.c | 60 +++
> >>  northd/en-global-config.c | 11 +++
> >>  northd/en-global-config.h |  1 +
> >>  ovn-sb.xml| 24 
> >>  tests/ovn.at  | 16 +++
> >>  utilities/ovn-trace.c |  1 +
> >>  9 files changed, 123 insertions(+)
> >>
> >> diff --git a/controller/chassis.c b/controller/chassis.c
> >> index ad75df288..9bb2eba95 100644
> >> --- a/controller/chassis.c
> >> +++ b/controller/chassis.c
> >> @@ -371,6 +371,7 @@ chassis_build_other_config(const struct
> >> ovs_chassis_cfg *ovs_cfg,
> >>  smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true");
> >>  smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true");
> >>  smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true");
> >> +smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true");
> >>  }
> >>
> >>  /*
> >> @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct
> >> ovs_chassis_cfg *ovs_cfg,
> >>  return true;
> >>  }
> >>
> >> +if (!smap_get_bool(_rec->other_config,
> >> +   OVN_FEATURE_CT_COMMIT_TO_ZONE,
> >> +   false)) {
> >> +return true;
> >> +}
> >> +
> >>  return false;
> >>  }
> >>
> >> @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported)
> >>  sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP);
> >>  sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN);
> >>  sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2);
> >> +sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE);
> >>  }
> >>
> >>  static void
> >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> >> index 8e794450c..4bcd1f58c 100644
> >> --- a/include/ovn/actions.h
> >> +++ b/include/ovn/actions.h
> >> @@ -68,6 +68,7 @@ struct collector_set_ids;
> >>  OVNACT(CT_NEXT,   ovnact_ct_next) \
> >>  /* CT_COMMIT_V1 is not supported anymore. */  \
> >>  OVNACT(CT_COMMIT_V2,  ovnact_nest)\
> >> +OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat)   \
> >>  OVNACT(CT_DNAT,   ovnact_ct_nat)  \
> >>  OVNACT(CT_SNAT,   ovnact_ct_nat)  \
> >>  OVNACT(CT_DNAT_IN_CZONE,  ovnact_ct_nat)  \
> >> diff --git a/include/ovn/features.h b/include/ovn/features.h
> >> index 08f1d8288..35a5d8ba0 100644
> >> --- a/include/ovn/features.h
> >> +++ b/include/ovn/features.h
> >> @@ -28,6 +28,7 @@
> >>  #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp"
> >>  #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column"
> >>  #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2"
> >> +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone"
> >>
> >>  /* OVS datapath supported features.  Based on availability OVN might
> >> generate
> >>   * different types of openflows.
> >> diff --git a/lib/actions.c b/lib/actions.c
> >> index 39bb5bc8a..f26817018 100644
> >> --- a/lib/actions.c
> >> +++ b/lib/actions.c
> >> @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
> >>  ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
> >>  ct = ofpacts->header;
> >>  ofpact_finish(ofpacts, >ofpact);
> >> +}
> >> +
> >> +static void
> >> +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx)
> >> +{
> >> +add_prerequisite(ctx, "ip");
> >> +
> >> +struct ovnact_ct_commit_nat *ct_commit =
> >> +ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts);
> >>
> >
> > The reuse "ovnact_ct_commit_nat" doesn't feel right, it should be renamed
> > if anything with a flag indicating whether there should be nat or not. By
> > doing that the parse + encoding for ct_commit_nat and ct_commit_to_zone
> can
> > be the same differentiated by the flag.
> >
>
> Just to be clear, your suggestion is to change the

Re: [ovs-dev] [Patch ovn v3 2/2] northd: Fix direct access to SNAT network.

2024-04-17 Thread Dumitru Ceara 
On 4/17/24 09:08, Ales Musil wrote:
> On Mon, Apr 15, 2024 at 2:15 PM Martin Kalcok 
> wrote:
> 
>> This change fixes bug that breaks ability of machines from external
>> networks, to communicate with machines in SNATed networks (specifically
>> when using a Distributed router).
>>
>> Currently when a machine (S1) on an external network tries to talk
>> over TCP with a machine (A1) in a network that has enabled SNAT, the
>> connection is established successfully. However after the three-way
>> handshake, any packets that come from the A1 machine will have their
>> source address translated by the Distributed router, breaking the
>> communication.
>>
>> Existing rule in `build_lrouter_out_snat_flow` that decides which
>> packets should be SNATed already tries to avoid SNATing packets in
>> reply direction with `(!ct.trk || !ct.rpl)`. However, previous stages
>> in the distributed LR egress pipeline do not initiate the CT state.
>>
>> Additionally we need to commit new connections that originate from
>> external networks into CT, so that the packets in the reply direction
>> (back to the external network) can be properly identified.
>>
>> Rationale:
>>
>> In my original RFC [0], there were questions about the motivation for
>> fixing this issue. I'll try to summarize why I think this is a bug
>> that should be fixed.
>>
>> 1. Current implementation for Distributed router already tries to
>>avoid SNATing packets in the reply direction, it's just missing
>>initialized CT states to make proper decisions.
>>
>> 2. This same scenario works with Gateway Router. I tested with
>>following setup:
>>
>> foo -- R1 -- join - R3 -- alice
>>   |
>> bar --R2
>>
>> R1 is a Distributed router with SNAT for foo. R2 is a Gateway
>> router with SNAT for bar. R3 is a Gateway router with no SNAT.
>> Using 'alice1' as a client I was able to talk over TCP with
>> 'bar1' but connection with 'foo1' failed.
>>
>> 3. Regarding security and "leaking" of internal IPs. Reading through
>>RFC 4787 [1], 5382 [2] and their update in 7857 [3], the
>>specifications do not seem to mandate that SNAT implementations
>>must filter incoming traffic destined directly to the internal
>>network. Sections like "5. Filtering Behavior" in 4787 and
>>"4.3 Externally Initiated Connections" in 5382 describe only
>>behavior for traffic destined to external IP/Port associated
>>with NAT on the device that performs NAT.
>>
>>Besides, with the current implementation, it's already possible
>>to scan the internal network with pings and TCP syn scanning.
>>
>> 4. We do have customers/clouds that depend on this functionality.
>>This is a scenario that used to work in Openstack with ML2/OVS
>>and migrating those clouds to ML2/OVN would break it.
>>
>> [0]
>> https://mail.openvswitch.org/pipermail/ovs-dev/2024-February/411670.html
>> [1]https://datatracker.ietf.org/doc/html/rfc4787
>> [2]https://datatracker.ietf.org/doc/html/rfc5382
>> [3]https://datatracker.ietf.org/doc/html/rfc7857
>>
>> Signed-off-by: Martin Kalcok 
>> ---
>>
> 
> Hi Martin,
> 
> I have just some nits down below.
> 
> 
>>  northd/northd.c | 66 ---
>>  northd/ovn-northd.8.xml | 29 
>>  tests/ovn-northd.at | 76 +
>>  tests/system-ovn.at | 68 
>>  4 files changed, 220 insertions(+), 19 deletions(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 02cf5b234..0726c8429 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -14413,20 +14413,27 @@ build_lrouter_out_is_dnat_local(struct
>> lflow_table *lflows,
>>
>>  static void
>>  build_lrouter_out_snat_match(struct lflow_table *lflows,
>> - const struct ovn_datapath *od,
>> - const struct nbrec_nat *nat, struct ds
>> *match,
>> - bool distributed_nat, int cidr_bits, bool
>> is_v6,
>> - struct ovn_port *l3dgw_port,
>> - struct lflow_ref *lflow_ref)
>> + const struct ovn_datapath *od,
>> + const struct nbrec_nat *nat,
>> + struct ds *match,
>> + bool distributed_nat, int
>> cidr_bits,
>> + bool is_v6,
>> + struct ovn_port *l3dgw_port,
>> + struct lflow_ref *lflow_ref,
>> + bool is_reverse)
>>  {
>>  ds_clear(match);
>>
>> -ds_put_format(match, "ip && ip%c.src == %s", is_v6 ? '6' : '4',
>> +ds_put_format(match, "ip && ip%c.%s == %s",
>> +  is_v6 ? '6' : '4',
>> +  is_reverse ? "dst" : "src",
>>

Re: [ovs-dev] [Patch ovn v3 1/2] actions: New action ct_commit_to_zone.

2024-04-17 Thread Dumitru Ceara 
On 4/17/24 09:04, Ales Musil wrote:
> On Mon, Apr 15, 2024 at 2:15 PM Martin Kalcok 
> wrote:
> 
>> This change adds a new action 'ct_commit_to_zone' that enables users to
>> commit
>> the flow into a specific zone in the connection tracker.
>>
>> A new feature flag, OVN_FEATURE_CT_COMMIT_TO_ZONE, is also included to
>> avoid
>> issues during upgrade in case the northd is upgraded to a version that
>> supports
>> this action before the controller is upgraded.
>>
>> Note that this action is meaningful only in the context of Logical Router
>> datapath. Logical Switch datapath does not use multiple zones and this
>> action
>> falls back to committing the connection into the default zone for the
>> Logical
>> Switch.
>>
>> Signed-off-by: Martin Kalcok 
>> ---
>>
> 
> Hi Martin,
> 

Hi Martin, Ales,

> thank you for the followup. I have a couple of comments down below.
> 

I have a few of my own. :)

> 
>>  controller/chassis.c  |  8 ++
>>  include/ovn/actions.h |  1 +
>>  include/ovn/features.h|  1 +
>>  lib/actions.c | 60 +++
>>  northd/en-global-config.c | 11 +++
>>  northd/en-global-config.h |  1 +
>>  ovn-sb.xml| 24 
>>  tests/ovn.at  | 16 +++
>>  utilities/ovn-trace.c |  1 +
>>  9 files changed, 123 insertions(+)
>>
>> diff --git a/controller/chassis.c b/controller/chassis.c
>> index ad75df288..9bb2eba95 100644
>> --- a/controller/chassis.c
>> +++ b/controller/chassis.c
>> @@ -371,6 +371,7 @@ chassis_build_other_config(const struct
>> ovs_chassis_cfg *ovs_cfg,
>>  smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true");
>>  smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true");
>>  smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true");
>> +smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true");
>>  }
>>
>>  /*
>> @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct
>> ovs_chassis_cfg *ovs_cfg,
>>  return true;
>>  }
>>
>> +if (!smap_get_bool(_rec->other_config,
>> +   OVN_FEATURE_CT_COMMIT_TO_ZONE,
>> +   false)) {
>> +return true;
>> +}
>> +
>>  return false;
>>  }
>>
>> @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported)
>>  sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP);
>>  sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN);
>>  sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2);
>> +sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE);
>>  }
>>
>>  static void
>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>> index 8e794450c..4bcd1f58c 100644
>> --- a/include/ovn/actions.h
>> +++ b/include/ovn/actions.h
>> @@ -68,6 +68,7 @@ struct collector_set_ids;
>>  OVNACT(CT_NEXT,   ovnact_ct_next) \
>>  /* CT_COMMIT_V1 is not supported anymore. */  \
>>  OVNACT(CT_COMMIT_V2,  ovnact_nest)\
>> +OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat)   \
>>  OVNACT(CT_DNAT,   ovnact_ct_nat)  \
>>  OVNACT(CT_SNAT,   ovnact_ct_nat)  \
>>  OVNACT(CT_DNAT_IN_CZONE,  ovnact_ct_nat)  \
>> diff --git a/include/ovn/features.h b/include/ovn/features.h
>> index 08f1d8288..35a5d8ba0 100644
>> --- a/include/ovn/features.h
>> +++ b/include/ovn/features.h
>> @@ -28,6 +28,7 @@
>>  #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp"
>>  #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column"
>>  #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2"
>> +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone"
>>
>>  /* OVS datapath supported features.  Based on availability OVN might
>> generate
>>   * different types of openflows.
>> diff --git a/lib/actions.c b/lib/actions.c
>> index 39bb5bc8a..f26817018 100644
>> --- a/lib/actions.c
>> +++ b/lib/actions.c
>> @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>>  ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
>>  ct = ofpacts->header;
>>  ofpact_finish(ofpacts, >ofpact);
>> +}
>> +
>> +static void
>> +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx)
>> +{
>> +add_prerequisite(ctx, "ip");
>> +
>> +struct ovnact_ct_commit_nat *ct_commit =
>> +ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts);
>>
> 
> The reuse "ovnact_ct_commit_nat" doesn't feel right, it should be renamed
> if anything with a flag indicating whether there should be nat or not. By
> doing that the parse + encoding for ct_commit_nat and ct_commit_to_zone can
> be the same differentiated by the flag.
> 

Just to be clear, your suggestion is to change the ct_commit_to_zone()
action so that it accepts an optional "nat" argument, right?  That would
allow actions like:

ct_commit_to_zone(snat, nat)

Can we do this as a follow up and also update northd to generate flows
with ct_commit_to_zone() instead of ct_commit_nat()?

> 
>> +
>> +lexer_force_match(ctx->lexer, LEX_T_LPAREN);
>> +
>> +if

Re: [ovs-dev] [PATCH net-next v3] net: openvswitch: Check vport net device name

2024-04-17 Thread Eelco Chaudron 
On 17 Apr 2024, at 6:20, jun.gu wrote:

> Check vport net device name to ensure the provided name is not one of
> its aliases. This can avoid that ovs userspace create and destroy vport
> repeatedly.

How about changing the text to the following?

  Ensure that the provided netdev name is not one of its aliases to prevent 
unnecessary creation and destruction of the vport by ovs-vswitchd.

Maybe the maintainers are fine with the current text, in that case:

Acked-by: Eelco Chaudron 

>
> Signed-off-by: Jun Gu 
> ---
>  net/openvswitch/vport-netdev.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
> index 903537a5da22..7003e76b8172 100644
> --- a/net/openvswitch/vport-netdev.c
> +++ b/net/openvswitch/vport-netdev.c
> @@ -78,7 +78,10 @@ struct vport *ovs_netdev_link(struct vport *vport, const 
> char *name)
>   int err;
>
>   vport->dev = dev_get_by_name(ovs_dp_get_net(vport->dp), name);
> - if (!vport->dev) {
> + /* Ensure that the device exists and that the provided
> +  * name is not one of its aliases.
> +  */
> + if ((!vport->dev) || strcmp(name, ovs_vport_name(vport))) {
>   err = -ENODEV;
>   goto error_free_vport;
>   }
> -- 
> 2.25.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v4 2/2] northd, controller: Use paused controller action for packet buffering.

2024-04-17 Thread Ales Musil 
On Wed, Apr 17, 2024 at 9:09 AM Ales Musil  wrote:

> The current packet injection loses ct_state in the process. When
> the ct_state is lost we might commit to DNAT zone and perform
> zero SNAT after the packet injection. This causes the first session
> to be wrong as the reply packets are not unDNATted.
>
> Instead of re-injecting the packet back into the pipeline when
> we get the MAC binding, use paused controller action. The paused
> controller action stores ct_state, which is important for the behavior
> of the resumed packet.
>
> At the same time bump the OvS submodule latest branch-3.3. This is
> mainly for [0], which fixes metering for paused controller actions.
>
> In order to make sure that the paused action works during upgrade add
> the output implicitly. Once the upgrade is done northd will create option
> to inform controllers that the implicit action is no longer needed.
>
> [0] c560f6ca3257 ("ofproto-dpif-xlate: Fix continuations with associated
> metering.")
>
> Reported-at: https://issues.redhat.com/browse/FDP-439
> Signed-off-by: Ales Musil 
> ---
> v4: Fix copy paste error in the global_handler.
> v3: Rebase on top of main.
> Add flag to ensure that the paused action works during upgrade.
> v2: Fix the Jira link and add ack from Mark.
> ---
>  controller/lflow.c  |  1 +
>  controller/lflow.h  |  1 +
>  controller/mac-learn.c  | 30 
>  controller/mac-learn.h  |  9 ++--
>  controller/ovn-controller.c | 21 
>  controller/pinctrl.c| 64 +
>  include/ovn/actions.h   |  3 ++
>  lib/actions.c   | 47 +-
>  northd/en-global-config.c   |  4 ++
>  northd/northd.c |  6 +--
>  ovs |  2 +-
>  tests/multinode.at  |  8 
>  tests/ovn-northd.at |  3 ++
>  tests/ovn.at|  8 ++--
>  tests/system-ovn.at | 95 +
>  tests/test-ovn.c|  1 +
>  16 files changed, 240 insertions(+), 63 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 895d17d19..760ec0b41 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -874,6 +874,7 @@ add_matches_to_flow_table(const struct
> sbrec_logical_flow *lflow,
>  .collector_ids = l_ctx_in->collector_ids,
>  .lflow_uuid = lflow->header_.uuid,
>  .dp_key = ldp->datapath->tunnel_key,
> +.explicit_arp_ns_output = l_ctx_in->explicit_arp_ns_output,
>
>  .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
>  .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
> diff --git a/controller/lflow.h b/controller/lflow.h
> index 9b7ffa19c..295d004f4 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -130,6 +130,7 @@ struct lflow_ctx_in {
>  bool lb_hairpin_use_ct_mark;
>  bool localnet_learn_fdb;
>  bool localnet_learn_fdb_changed;
> +bool explicit_arp_ns_output;
>  };
>
>  struct lflow_ctx_out {
> diff --git a/controller/mac-learn.c b/controller/mac-learn.c
> index 071f01b4f..0c3b60c23 100644
> --- a/controller/mac-learn.c
> +++ b/controller/mac-learn.c
> @@ -199,15 +199,24 @@ ovn_fdb_add(struct hmap *fdbs, uint32_t dp_key,
> struct eth_addr mac,
>  /* packet buffering functions */
>
>  struct packet_data *
> -ovn_packet_data_create(struct ofpbuf ofpacts,
> -   const struct dp_packet *original_packet)
> +ovn_packet_data_create(const struct ofputil_packet_in *pin,
> +   const struct ofpbuf *continuation)
>  {
>  struct packet_data *pd = xmalloc(sizeof *pd);
>
> -pd->ofpacts = ofpacts;
> -/* clone the packet to send it later with correct L2 address */
> -pd->p = dp_packet_clone_data(dp_packet_data(original_packet),
> - dp_packet_size(original_packet));
> +pd->pin = (struct ofputil_packet_in) {
> +.packet = xmemdup(pin->packet, pin->packet_len),
> +.packet_len = pin->packet_len,
> +.flow_metadata = pin->flow_metadata,
> +.reason = pin->reason,
> +.table_id = pin->table_id,
> +.cookie = pin->cookie,
> +/* Userdata are empty on purpose,
> + * it is not needed for the continuation. */
> +.userdata = NULL,
> +.userdata_len = 0,
> +};
> +pd->continuation = ofpbuf_clone(continuation);
>
>  return pd;
>  }
> @@ -216,8 +225,8 @@ ovn_packet_data_create(struct ofpbuf ofpacts,
>  void
>  ovn_packet_data_destroy(struct packet_data *pd)
>  {
> -dp_packet_delete(pd->p);
> -ofpbuf_uninit(>ofpacts);
> +free(pd->pin.packet);
> +ofpbuf_delete(pd->continuation);
>  free(pd);
>  }
>
> @@ -307,7 +316,10 @@ ovn_buffered_packets_ctx_run(struct
> buffered_packets_ctx *ctx,
>
>  struct packet_data *pd;
>  LIST_FOR_EACH_POP (pd, node, >queue) {
> -struct eth_header *eth = dp_packet_data(pd->p);
> +struct dp_packet packet;
> +

Re: [ovs-dev] [PATCH ovn v4 1/2] ci: Make sure that multinode test runs on correct branch.

2024-04-17 Thread Ales Musil 
On Wed, Apr 17, 2024 at 9:09 AM Ales Musil  wrote:

> The ovn-fake-multinode workflow can be triggered manually,
> however the definition didn't respect the branch for the manual
> run and always used main branch. Make sure that the correct
> branch is used for the ovn-fake-multinode workflow.
>
> Fixes: 033f5bebf94d ("CI: Add a couple of periodic jobs using
> ovn-fake-multinode.")
> Signed-off-by: Ales Musil 
> ---
> v4: Return job permutation that was accidentally removed.
> ---
>  .github/workflows/ovn-fake-multinode-tests.yml | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/.github/workflows/ovn-fake-multinode-tests.yml
> b/.github/workflows/ovn-fake-multinode-tests.yml
> index ed4d6c4ef..5c9030a49 100644
> --- a/.github/workflows/ovn-fake-multinode-tests.yml
> +++ b/.github/workflows/ovn-fake-multinode-tests.yml
> @@ -17,7 +17,7 @@ jobs:
>  strategy:
>matrix:
>  cfg:
> -- { branch: "main" }
> +- { branch: "${{ github.ref_name }}" }
>  - { branch: "branch-22.03" }
>  env:
>RUNC_CMD: podman
> @@ -76,16 +76,16 @@ jobs:
>fail-fast: false
>matrix:
>  cfg:
> -- { branch: "main", testsuiteflags: ""}
> +- { branch: "${{ github.ref_name }}", testsuiteflags: ""}
>  - { branch: "branch-22.03", testsuiteflags: "-k 'basic test'" }
>  name: multinode tests ${{ join(matrix.cfg.*, ' ') }}
>  env:
>RUNC_CMD: podman
>OS_IMAGE: "fedora:37"
>CENTRAL_IMAGE: "ovn/ovn-multi-node:${{ matrix.cfg.branch }}"
> -  CHASSIS_IMAGE: "ovn/ovn-multi-node:main"
> -  RELAY_IMAGE: "ovn/ovn-multi-node:main"
> -  GW_IMAGE: "ovn/ovn-multi-node:main"
> +  CHASSIS_IMAGE: "ovn/ovn-multi-node:${{ github.ref_name }}"
> +  RELAY_IMAGE: "ovn/ovn-multi-node:${{ github.ref_name }}"
> +  GW_IMAGE: "ovn/ovn-multi-node:${{ github.ref_name }}"
># Disable SSL for now. Revisit this if required.
>ENABLE_SSL: no
>CC: gcc
> @@ -114,7 +114,7 @@ jobs:
>
>  - uses: actions/download-artifact@v4
>with:
> -name: test-main-image
> +name: test-${{ github.ref_name }}-image
>
>  - uses: actions/download-artifact@v4
>with:
> @@ -122,7 +122,7 @@ jobs:
>
>  - name: Load podman image
>run: |
> -sudo podman load --input ovn_main_image.tar
> +sudo podman load --input ovn_${{ github.ref_name }}_image.tar
>  sudo podman load --input ovn_branch-22.03_image.tar
>
>  - name: Check out ovn-fake-multi-node
> --
> 2.44.0
>
>
Recheck-request: github-robot-_ovn-kubernetes
-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 1/1] netdev-dpdk: Improve error print to the user for flow control error.

2024-04-17 Thread Roi Dayan via dev 
When failing to get flow control parameters use VLOG_WARN_BUF()
to expose the error string in ovs-vsctl show.

Signed-off-by: Roi Dayan 
Suggested-by: Simon Horman 
Acked-by: Eli Britstein 
---
 lib/netdev-dpdk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 2111f776810b..32d4193d24af 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2426,8 +2426,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
struct smap *args,
 }
 err = 0; /* Not fatal. */
 } else {
-VLOG_WARN("%s: Cannot get flow control parameters: %s",
-  netdev_get_name(netdev), rte_strerror(err));
+VLOG_WARN_BUF(errp, "%s: Cannot get flow control parameters: %s",
+  netdev_get_name(netdev), rte_strerror(err));
 }
 goto out;
 }
-- 
2.21.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/1] netdev-dpdk: Fix possible memory leak configuring VF MAC address.

2024-04-17 Thread Roi Dayan via dev 



On 16/04/2024 18:48, Simon Horman wrote:
> On Tue, Apr 16, 2024 at 04:21:48PM +0300, Roi Dayan via dev wrote:
>> VLOG_WARN_BUF() is allocating memory for the error string and should
>> e used if the configuration cannot continue and error is being returned
>> so the caller has indication of releasing the pointer.
>> Change to VLOG_WARN() to keep the logic that error is not being
>> returned.
>>
>> Fixes: f4336f504b17 ("netdev-dpdk: Add option to configure VF MAC address.")
>> Signed-off-by: Roi Dayan 
>> Acked-by: Gaetan Rivet 
>> Acked-by: Eli Britstein 
>> ---
>>  lib/netdev-dpdk.c | 17 -
>>  1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 2111f776810b..9249b9e9c646 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -2379,17 +2379,16 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
>> struct smap *args,
>>  struct eth_addr mac;
>>  
>>  if (!dpdk_port_is_representor(dev)) {
>> -VLOG_WARN_BUF(errp, "'%s' is trying to set the VF MAC '%s' "
>> -  "but 'options:dpdk-vf-mac' is only supported for "
>> -  "VF representors.",
>> -  netdev_get_name(netdev), vf_mac);
>> +VLOG_WARN("'%s' is trying to set the VF MAC '%s' "
>> +  "but 'options:dpdk-vf-mac' is only supported for "
>> +  "VF representors.",
>> +  netdev_get_name(netdev), vf_mac);
>>  } else if (!eth_addr_from_string(vf_mac, )) {
>> -VLOG_WARN_BUF(errp, "interface '%s': cannot parse VF MAC '%s'.",
>> -  netdev_get_name(netdev), vf_mac);
>> +VLOG_WARN("interface '%s': cannot parse VF MAC '%s'.",
>> +  netdev_get_name(netdev), vf_mac);
>>  } else if (eth_addr_is_multicast(mac)) {
>> -VLOG_WARN_BUF(errp,
>> -  "interface '%s': cannot set VF MAC to multicast "
>> -  "address '%s'.", netdev_get_name(netdev), vf_mac);
>> +VLOG_WARN("interface '%s': cannot set VF MAC to multicast "
>> +  "address '%s'.", netdev_get_name(netdev), vf_mac);
>>  } else if (!eth_addr_equals(dev->requested_hwaddr, mac)) {
>>  dev->requested_hwaddr = mac;
>>  netdev_request_reconfigure(netdev);
> 
> Thanks Roi,
> 
> I agree that this change makes sense as the allocated
> value will typically be discarded. And I think if
> VLOG_WARN_BUF() is called again in the same invocation of
> netdev_dpdk_set_config() a memory leak occurs.
> 
> Acked-by: Simon Horman 
> 
> After reviewing your patch I am now wondering if, conversely,
> the following change _also_ makes sense:
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 2111f776810b..32d4193d24af 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2426,8 +2426,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
> struct smap *args,
>  }
>  err = 0; /* Not fatal. */
>  } else {
> -VLOG_WARN("%s: Cannot get flow control parameters: %s",
> -  netdev_get_name(netdev), rte_strerror(err));
> +VLOG_WARN_BUF(errp, "%s: Cannot get flow control parameters: %s",
> +  netdev_get_name(netdev), rte_strerror(err));
>  }
>  goto out;
>  }

Hi,

thanks for the review. yes it can improve the error to the user in ovs-vsctl 
show.

error: "enp8s0f1: could not set configuration (Invalid 
argument)"
vs

error: "enp8s0f1: Cannot get flow control parameters: Invalid 
argument"


I'll do it in a different commit as its not related to the memleak fix.

Thanks,
Roi

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v4 1/2] ci: Make sure that multinode test runs on correct branch.

2024-04-17 Thread Ales Musil 
The ovn-fake-multinode workflow can be triggered manually,
however the definition didn't respect the branch for the manual
run and always used main branch. Make sure that the correct
branch is used for the ovn-fake-multinode workflow.

Fixes: 033f5bebf94d ("CI: Add a couple of periodic jobs using 
ovn-fake-multinode.")
Signed-off-by: Ales Musil 
---
v4: Return job permutation that was accidentally removed.
---
 .github/workflows/ovn-fake-multinode-tests.yml | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/.github/workflows/ovn-fake-multinode-tests.yml 
b/.github/workflows/ovn-fake-multinode-tests.yml
index ed4d6c4ef..5c9030a49 100644
--- a/.github/workflows/ovn-fake-multinode-tests.yml
+++ b/.github/workflows/ovn-fake-multinode-tests.yml
@@ -17,7 +17,7 @@ jobs:
 strategy:
   matrix:
 cfg:
-- { branch: "main" }
+- { branch: "${{ github.ref_name }}" }
 - { branch: "branch-22.03" }
 env:
   RUNC_CMD: podman
@@ -76,16 +76,16 @@ jobs:
   fail-fast: false
   matrix:
 cfg:
-- { branch: "main", testsuiteflags: ""}
+- { branch: "${{ github.ref_name }}", testsuiteflags: ""}
 - { branch: "branch-22.03", testsuiteflags: "-k 'basic test'" }
 name: multinode tests ${{ join(matrix.cfg.*, ' ') }}
 env:
   RUNC_CMD: podman
   OS_IMAGE: "fedora:37"
   CENTRAL_IMAGE: "ovn/ovn-multi-node:${{ matrix.cfg.branch }}"
-  CHASSIS_IMAGE: "ovn/ovn-multi-node:main"
-  RELAY_IMAGE: "ovn/ovn-multi-node:main"
-  GW_IMAGE: "ovn/ovn-multi-node:main"
+  CHASSIS_IMAGE: "ovn/ovn-multi-node:${{ github.ref_name }}"
+  RELAY_IMAGE: "ovn/ovn-multi-node:${{ github.ref_name }}"
+  GW_IMAGE: "ovn/ovn-multi-node:${{ github.ref_name }}"
   # Disable SSL for now. Revisit this if required.
   ENABLE_SSL: no
   CC: gcc
@@ -114,7 +114,7 @@ jobs:
 
 - uses: actions/download-artifact@v4
   with:
-name: test-main-image
+name: test-${{ github.ref_name }}-image
 
 - uses: actions/download-artifact@v4
   with:
@@ -122,7 +122,7 @@ jobs:
 
 - name: Load podman image
   run: |
-sudo podman load --input ovn_main_image.tar
+sudo podman load --input ovn_${{ github.ref_name }}_image.tar
 sudo podman load --input ovn_branch-22.03_image.tar
 
 - name: Check out ovn-fake-multi-node
-- 
2.44.0

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v4 2/2] northd, controller: Use paused controller action for packet buffering.

2024-04-17 Thread Ales Musil 
The current packet injection loses ct_state in the process. When
the ct_state is lost we might commit to DNAT zone and perform
zero SNAT after the packet injection. This causes the first session
to be wrong as the reply packets are not unDNATted.

Instead of re-injecting the packet back into the pipeline when
we get the MAC binding, use paused controller action. The paused
controller action stores ct_state, which is important for the behavior
of the resumed packet.

At the same time bump the OvS submodule latest branch-3.3. This is
mainly for [0], which fixes metering for paused controller actions.

In order to make sure that the paused action works during upgrade add
the output implicitly. Once the upgrade is done northd will create option
to inform controllers that the implicit action is no longer needed.

[0] c560f6ca3257 ("ofproto-dpif-xlate: Fix continuations with associated 
metering.")

Reported-at: https://issues.redhat.com/browse/FDP-439
Signed-off-by: Ales Musil 
---
v4: Fix copy paste error in the global_handler.
v3: Rebase on top of main.
Add flag to ensure that the paused action works during upgrade.
v2: Fix the Jira link and add ack from Mark.
---
 controller/lflow.c  |  1 +
 controller/lflow.h  |  1 +
 controller/mac-learn.c  | 30 
 controller/mac-learn.h  |  9 ++--
 controller/ovn-controller.c | 21 
 controller/pinctrl.c| 64 +
 include/ovn/actions.h   |  3 ++
 lib/actions.c   | 47 +-
 northd/en-global-config.c   |  4 ++
 northd/northd.c |  6 +--
 ovs |  2 +-
 tests/multinode.at  |  8 
 tests/ovn-northd.at |  3 ++
 tests/ovn.at|  8 ++--
 tests/system-ovn.at | 95 +
 tests/test-ovn.c|  1 +
 16 files changed, 240 insertions(+), 63 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index 895d17d19..760ec0b41 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -874,6 +874,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow 
*lflow,
 .collector_ids = l_ctx_in->collector_ids,
 .lflow_uuid = lflow->header_.uuid,
 .dp_key = ldp->datapath->tunnel_key,
+.explicit_arp_ns_output = l_ctx_in->explicit_arp_ns_output,
 
 .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
 .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
diff --git a/controller/lflow.h b/controller/lflow.h
index 9b7ffa19c..295d004f4 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -130,6 +130,7 @@ struct lflow_ctx_in {
 bool lb_hairpin_use_ct_mark;
 bool localnet_learn_fdb;
 bool localnet_learn_fdb_changed;
+bool explicit_arp_ns_output;
 };
 
 struct lflow_ctx_out {
diff --git a/controller/mac-learn.c b/controller/mac-learn.c
index 071f01b4f..0c3b60c23 100644
--- a/controller/mac-learn.c
+++ b/controller/mac-learn.c
@@ -199,15 +199,24 @@ ovn_fdb_add(struct hmap *fdbs, uint32_t dp_key, struct 
eth_addr mac,
 /* packet buffering functions */
 
 struct packet_data *
-ovn_packet_data_create(struct ofpbuf ofpacts,
-   const struct dp_packet *original_packet)
+ovn_packet_data_create(const struct ofputil_packet_in *pin,
+   const struct ofpbuf *continuation)
 {
 struct packet_data *pd = xmalloc(sizeof *pd);
 
-pd->ofpacts = ofpacts;
-/* clone the packet to send it later with correct L2 address */
-pd->p = dp_packet_clone_data(dp_packet_data(original_packet),
- dp_packet_size(original_packet));
+pd->pin = (struct ofputil_packet_in) {
+.packet = xmemdup(pin->packet, pin->packet_len),
+.packet_len = pin->packet_len,
+.flow_metadata = pin->flow_metadata,
+.reason = pin->reason,
+.table_id = pin->table_id,
+.cookie = pin->cookie,
+/* Userdata are empty on purpose,
+ * it is not needed for the continuation. */
+.userdata = NULL,
+.userdata_len = 0,
+};
+pd->continuation = ofpbuf_clone(continuation);
 
 return pd;
 }
@@ -216,8 +225,8 @@ ovn_packet_data_create(struct ofpbuf ofpacts,
 void
 ovn_packet_data_destroy(struct packet_data *pd)
 {
-dp_packet_delete(pd->p);
-ofpbuf_uninit(>ofpacts);
+free(pd->pin.packet);
+ofpbuf_delete(pd->continuation);
 free(pd);
 }
 
@@ -307,7 +316,10 @@ ovn_buffered_packets_ctx_run(struct buffered_packets_ctx 
*ctx,
 
 struct packet_data *pd;
 LIST_FOR_EACH_POP (pd, node, >queue) {
-struct eth_header *eth = dp_packet_data(pd->p);
+struct dp_packet packet;
+dp_packet_use_const(, pd->pin.packet, pd->pin.packet_len);
+
+struct eth_header *eth = dp_packet_data();
 eth->eth_dst = mac;
 
 ovs_list_push_back(>ready_packets_data, >node);
diff --git a/controller/mac-learn.h b/controller/mac-learn.h

Re: [ovs-dev] [Patch ovn v3 2/2] northd: Fix direct access to SNAT network.

2024-04-17 Thread Ales Musil 
On Mon, Apr 15, 2024 at 2:15 PM Martin Kalcok 
wrote:

> This change fixes bug that breaks ability of machines from external
> networks, to communicate with machines in SNATed networks (specifically
> when using a Distributed router).
>
> Currently when a machine (S1) on an external network tries to talk
> over TCP with a machine (A1) in a network that has enabled SNAT, the
> connection is established successfully. However after the three-way
> handshake, any packets that come from the A1 machine will have their
> source address translated by the Distributed router, breaking the
> communication.
>
> Existing rule in `build_lrouter_out_snat_flow` that decides which
> packets should be SNATed already tries to avoid SNATing packets in
> reply direction with `(!ct.trk || !ct.rpl)`. However, previous stages
> in the distributed LR egress pipeline do not initiate the CT state.
>
> Additionally we need to commit new connections that originate from
> external networks into CT, so that the packets in the reply direction
> (back to the external network) can be properly identified.
>
> Rationale:
>
> In my original RFC [0], there were questions about the motivation for
> fixing this issue. I'll try to summarize why I think this is a bug
> that should be fixed.
>
> 1. Current implementation for Distributed router already tries to
>avoid SNATing packets in the reply direction, it's just missing
>initialized CT states to make proper decisions.
>
> 2. This same scenario works with Gateway Router. I tested with
>following setup:
>
> foo -- R1 -- join - R3 -- alice
>   |
> bar --R2
>
> R1 is a Distributed router with SNAT for foo. R2 is a Gateway
> router with SNAT for bar. R3 is a Gateway router with no SNAT.
> Using 'alice1' as a client I was able to talk over TCP with
> 'bar1' but connection with 'foo1' failed.
>
> 3. Regarding security and "leaking" of internal IPs. Reading through
>RFC 4787 [1], 5382 [2] and their update in 7857 [3], the
>specifications do not seem to mandate that SNAT implementations
>must filter incoming traffic destined directly to the internal
>network. Sections like "5. Filtering Behavior" in 4787 and
>"4.3 Externally Initiated Connections" in 5382 describe only
>behavior for traffic destined to external IP/Port associated
>with NAT on the device that performs NAT.
>
>Besides, with the current implementation, it's already possible
>to scan the internal network with pings and TCP syn scanning.
>
> 4. We do have customers/clouds that depend on this functionality.
>This is a scenario that used to work in Openstack with ML2/OVS
>and migrating those clouds to ML2/OVN would break it.
>
> [0]
> https://mail.openvswitch.org/pipermail/ovs-dev/2024-February/411670.html
> [1]https://datatracker.ietf.org/doc/html/rfc4787
> [2]https://datatracker.ietf.org/doc/html/rfc5382
> [3]https://datatracker.ietf.org/doc/html/rfc7857
>
> Signed-off-by: Martin Kalcok 
> ---
>

Hi Martin,

I have just some nits down below.


>  northd/northd.c | 66 ---
>  northd/ovn-northd.8.xml | 29 
>  tests/ovn-northd.at | 76 +
>  tests/system-ovn.at | 68 
>  4 files changed, 220 insertions(+), 19 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 02cf5b234..0726c8429 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -14413,20 +14413,27 @@ build_lrouter_out_is_dnat_local(struct
> lflow_table *lflows,
>
>  static void
>  build_lrouter_out_snat_match(struct lflow_table *lflows,
> - const struct ovn_datapath *od,
> - const struct nbrec_nat *nat, struct ds
> *match,
> - bool distributed_nat, int cidr_bits, bool
> is_v6,
> - struct ovn_port *l3dgw_port,
> - struct lflow_ref *lflow_ref)
> + const struct ovn_datapath *od,
> + const struct nbrec_nat *nat,
> + struct ds *match,
> + bool distributed_nat, int
> cidr_bits,
> + bool is_v6,
> + struct ovn_port *l3dgw_port,
> + struct lflow_ref *lflow_ref,
> + bool is_reverse)
>  {
>  ds_clear(match);
>
> -ds_put_format(match, "ip && ip%c.src == %s", is_v6 ? '6' : '4',
> +ds_put_format(match, "ip && ip%c.%s == %s",
> +  is_v6 ? '6' : '4',
> +  is_reverse ? "dst" : "src",
>nat->logical_ip);
>
>  if (!od->is_gw_router) {
>  /* Distributed router. */
> -ds_put_format(match, " && outport == %s",

Re: [ovs-dev] [Patch ovn v3 1/2] actions: New action ct_commit_to_zone.

2024-04-17 Thread Ales Musil 
On Mon, Apr 15, 2024 at 2:15 PM Martin Kalcok 
wrote:

> This change adds a new action 'ct_commit_to_zone' that enables users to
> commit
> the flow into a specific zone in the connection tracker.
>
> A new feature flag, OVN_FEATURE_CT_COMMIT_TO_ZONE, is also included to
> avoid
> issues during upgrade in case the northd is upgraded to a version that
> supports
> this action before the controller is upgraded.
>
> Note that this action is meaningful only in the context of Logical Router
> datapath. Logical Switch datapath does not use multiple zones and this
> action
> falls back to committing the connection into the default zone for the
> Logical
> Switch.
>
> Signed-off-by: Martin Kalcok 
> ---
>

Hi Martin,

thank you for the followup. I have a couple of comments down below.


>  controller/chassis.c  |  8 ++
>  include/ovn/actions.h |  1 +
>  include/ovn/features.h|  1 +
>  lib/actions.c | 60 +++
>  northd/en-global-config.c | 11 +++
>  northd/en-global-config.h |  1 +
>  ovn-sb.xml| 24 
>  tests/ovn.at  | 16 +++
>  utilities/ovn-trace.c |  1 +
>  9 files changed, 123 insertions(+)
>
> diff --git a/controller/chassis.c b/controller/chassis.c
> index ad75df288..9bb2eba95 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -371,6 +371,7 @@ chassis_build_other_config(const struct
> ovs_chassis_cfg *ovs_cfg,
>  smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true");
>  smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true");
>  smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true");
> +smap_replace(config, OVN_FEATURE_CT_COMMIT_TO_ZONE, "true");
>  }
>
>  /*
> @@ -516,6 +517,12 @@ chassis_other_config_changed(const struct
> ovs_chassis_cfg *ovs_cfg,
>  return true;
>  }
>
> +if (!smap_get_bool(_rec->other_config,
> +   OVN_FEATURE_CT_COMMIT_TO_ZONE,
> +   false)) {
> +return true;
> +}
> +
>  return false;
>  }
>
> @@ -648,6 +655,7 @@ update_supported_sset(struct sset *supported)
>  sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP);
>  sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN);
>  sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2);
> +sset_add(supported, OVN_FEATURE_CT_COMMIT_TO_ZONE);
>  }
>
>  static void
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 8e794450c..4bcd1f58c 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -68,6 +68,7 @@ struct collector_set_ids;
>  OVNACT(CT_NEXT,   ovnact_ct_next) \
>  /* CT_COMMIT_V1 is not supported anymore. */  \
>  OVNACT(CT_COMMIT_V2,  ovnact_nest)\
> +OVNACT(CT_COMMIT_TO_ZONE, ovnact_ct_commit_nat)   \
>  OVNACT(CT_DNAT,   ovnact_ct_nat)  \
>  OVNACT(CT_SNAT,   ovnact_ct_nat)  \
>  OVNACT(CT_DNAT_IN_CZONE,  ovnact_ct_nat)  \
> diff --git a/include/ovn/features.h b/include/ovn/features.h
> index 08f1d8288..35a5d8ba0 100644
> --- a/include/ovn/features.h
> +++ b/include/ovn/features.h
> @@ -28,6 +28,7 @@
>  #define OVN_FEATURE_FDB_TIMESTAMP "fdb-timestamp"
>  #define OVN_FEATURE_LS_DPG_COLUMN "ls-dpg-column"
>  #define OVN_FEATURE_CT_COMMIT_NAT_V2 "ct-commit-nat-v2"
> +#define OVN_FEATURE_CT_COMMIT_TO_ZONE "ct-commit-to-zone"
>
>  /* OVS datapath supported features.  Based on availability OVN might
> generate
>   * different types of openflows.
> diff --git a/lib/actions.c b/lib/actions.c
> index 39bb5bc8a..f26817018 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -791,6 +791,64 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>  ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
>  ct = ofpacts->header;
>  ofpact_finish(ofpacts, >ofpact);
> +}
> +
> +static void
> +parse_CT_COMMIT_TO_ZONE(struct action_context *ctx)
> +{
> +add_prerequisite(ctx, "ip");
> +
> +struct ovnact_ct_commit_nat *ct_commit =
> +ovnact_put_CT_COMMIT_TO_ZONE(ctx->ovnacts);
>

The reuse "ovnact_ct_commit_nat" doesn't feel right, it should be renamed
if anything with a flag indicating whether there should be nat or not. By
doing that the parse + encoding for ct_commit_nat and ct_commit_to_zone can
be the same differentiated by the flag.


> +
> +lexer_force_match(ctx->lexer, LEX_T_LPAREN);
> +
> +if (lexer_match_id(ctx->lexer, "dnat")) {
> +ct_commit->dnat_zone = true;
> +} else if (lexer_match_id(ctx->lexer, "snat")) {
> +ct_commit->dnat_zone = false;
> +} else {
> +lexer_syntax_error(ctx->lexer, "expecting parameter 'dnat' or
> 'snat'");
> +}
> +
> +lexer_force_match(ctx->lexer, LEX_T_RPAREN);
> +}
> +
> +static void
> +format_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit,
> + struct ds *s)
> +{
> +ds_put_format(s, "ct_commit_to_zone(%s);",
> +

Re: [ovs-dev] [PATCH ovn] ovn-nbctl: Document "--portrange" in the manpage.

2024-04-17 Thread Ales Musil 
On Mon, Apr 15, 2024 at 8:21 PM Mark Michelson  wrote:

> The --portrange option was added in commit [0]. The option appears in
> the help text for ovn-nbctl, but it is not documented in the manpage at
> all.
>
> This commit adds documentation to the manpage for the --portrange
> option.
>
> Reported-at: https://issues.redhat.com/browse/FDP-537
> Signed-off-by: Mark Michelson 
> ---
>  utilities/ovn-nbctl.8.xml | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> index 39c352212..ea2b201a5 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -1175,7 +1175,7 @@
>  NAT Commands
>
>  
> -  [--may-exist] [--stateless]
> [--gateway-port=GATEWAY_PORT]
> lr-nat-add router type
> external_ip logical_ip [logical_port
> external_mac]
> +  [--may-exist] [--stateless]
> [--gateway-port=GATEWAY_PORT] [--portrange]
> lr-nat-add router type
> external_ip logical_ip [logical_port
> external_mac] [external_port_range]
>
>  
>Adds the specified NAT to router.
> @@ -1212,6 +1212,18 @@
>specify the GATEWAY_PORT.
>  
>
> +
> +  If the --portrange option is specified, then a
> range of
> +  ports may be specified in the external_port_range
> part
> +  of the lr-nat-add command. If this option is
> omitted,
> +  then an external port range may not be specified. The format of
> the
> +  port range is port_low-port_high, where
> +  port_low is a lower number than
> port_high. When
> +  the packet is NATted, a random port from the range will be
> selected
> +  as the source port. The range for the
> +  external_port_range is 1-65535.
> +
> +
>  
>When type is dnat, the externally
>visible IP address external_ip is DNATted to the
> --
> 2.44.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.

Acked-by: Ales Musil 

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev