Re: [ovs-dev] [Patch ovn v2] actions: Explicitly finish CT actions.

[martin . kalcok]

Regarding the failed CI run, I suspect that there might be some test
instability. All tests passed successfully in my branch [0] for this
patch.

Martin.

[0] https://github.com/mkalcok/ovn/actions/runs/10009002777

On Fri, 2024-07-19 at 15:37 +0200, Martin Kalcok wrote:
> As discussed here [0], a couple of functions that encode
> CT-related actions were using older, manual, way of finishing
> the action.
> 
> As amusil mentioned here [1], there's a shorter and more explicit
> way of doing it. This change replaces manual way with the more
> explicit aproach.
> 
> [0]
> https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/414667.html
> [1]
> https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html
> 
> Signed-off-by: Martin Kalcok 
> ---
> Thank you for the review, Ales. I added places that I missed
> originally.
> For the unit tests, I copied your suggestion for 'ct_lb' action, and 
> for the rest of the actions, I just pre-pended them with `ct_clear`.
> I hope
> that's enough.
> 
> Martin.
> 
>  controller/lflow.c | 11 +-
>  lib/actions.c  | 52 +++-
> --
>  tests/ovn.at   | 22 
>  3 files changed, 44 insertions(+), 41 deletions(-)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index b4c379044..aa77ed631 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -1795,6 +1795,7 @@ add_lb_ct_snat_hairpin_vip_flow(const struct
> ovn_controller_lb *lb,
>  {
>  uint64_t stub[1024 / 8];
>  struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);
> +    const size_t ct_offset = ofpacts.size;
>  
>  uint8_t address_family;
>  if (IN6_IS_ADDR_V4MAPPED(_vip->vip)) {
> @@ -1811,10 +1812,6 @@ add_lb_ct_snat_hairpin_vip_flow(const struct
> ovn_controller_lb *lb,
>  ct->flags = NX_CT_F_COMMIT;
>  ct->alg = 0;
>  
> -    size_t nat_offset;
> -    nat_offset = ofpacts.size;
> -    ofpbuf_pull(, nat_offset);
> -
>  struct ofpact_nat *nat = ofpact_put_NAT();
>  nat->flags = NX_NAT_F_SRC;
>  nat->range_af = address_family;
> @@ -1828,8 +1825,10 @@ add_lb_ct_snat_hairpin_vip_flow(const struct
> ovn_controller_lb *lb,
>     ? lb-
> >hairpin_snat_ips.ipv6_addrs[0].addr
>     : lb_vip->vip;
>  }
> -    ofpacts.header = ofpbuf_push_uninit(, nat_offset);
> -    ofpact_finish(, >ofpact);
> +
> +    ct = ofpbuf_at_assert(, ct_offset, sizeof *ct);
> +    ofpacts.header = ct;
> +    ofpact_finish_CT(, );
>  
>  struct match match = MATCH_CATCHALL_INITIALIZER;
>  
> diff --git a/lib/actions.c b/lib/actions.c
> index e8cc0994d..9d19dd2dc 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -715,13 +715,18 @@ encode_CT_NEXT(const struct ovnact_ct_next
> *ct_next,
>  const struct ovnact_encode_params *ep,
>  struct ofpbuf *ofpacts)
>  {
> +    size_t ct_offset = ofpacts->size;
> +
>  struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
>  ct->recirc_table = first_ptable(ep, ep->pipeline) + ct_next-
> >ltable;
>  ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE)
>  : mf_from_id(MFF_LOG_DNAT_ZONE);
>  ct->zone_src.ofs = 0;
>  ct->zone_src.n_bits = 16;
> -    ofpact_finish(ofpacts, >ofpact);
> +
> +    ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
> +    ofpacts->header = ct;
> +    ofpact_finish_CT(ofpacts, );
>  }
>  
>  static void
> @@ -761,7 +766,6 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>  struct ofpbuf *ofpacts)
>  {
>  size_t ct_offset = ofpacts->size;
> -    ofpbuf_pull(ofpacts, ct_offset);
>  
>  struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
>  ct->flags = NX_CT_F_COMMIT;
> @@ -776,25 +780,17 @@ encode_CT_COMMIT_V2(const struct ovnact_nest
> *on,
>   * collisions at commit time between NATed and firewalled-only
> sessions.
>   */
>  if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) {
> -    size_t nat_offset = ofpacts->size;
> -    ofpbuf_pull(ofpacts, nat_offset);
> -
>  struct ofpact_nat *nat = ofpact_put_NAT(ofpacts);
>  nat->flags = 0;
>  nat->range_af = AF_UNSPEC;
>  nat->flags |= NX_NAT_F_SRC;
> -    ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
> -    ct = ofpacts->header;
>  }
>  
> -    size_t set_field_offset = ofpacts->size;
> -    ofpbuf_pull(ofpacts, set_field_offset);
> -
>  ovnacts_encode(

[ovs-dev] [Patch ovn v2] actions: Explicitly finish CT actions.

[Martin Kalcok]

As discussed here [0], a couple of functions that encode
CT-related actions were using older, manual, way of finishing
the action.

As amusil mentioned here [1], there's a shorter and more explicit
way of doing it. This change replaces manual way with the more
explicit aproach.

[0] https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/414667.html
[1] https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html

Signed-off-by: Martin Kalcok 
---
Thank you for the review, Ales. I added places that I missed originally.
For the unit tests, I copied your suggestion for 'ct_lb' action, and 
for the rest of the actions, I just pre-pended them with `ct_clear`. I hope
that's enough.

Martin.

 controller/lflow.c | 11 +-
 lib/actions.c  | 52 +++---
 tests/ovn.at   | 22 
 3 files changed, 44 insertions(+), 41 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index b4c379044..aa77ed631 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -1795,6 +1795,7 @@ add_lb_ct_snat_hairpin_vip_flow(const struct 
ovn_controller_lb *lb,
 {
 uint64_t stub[1024 / 8];
 struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);
+const size_t ct_offset = ofpacts.size;
 
 uint8_t address_family;
 if (IN6_IS_ADDR_V4MAPPED(_vip->vip)) {
@@ -1811,10 +1812,6 @@ add_lb_ct_snat_hairpin_vip_flow(const struct 
ovn_controller_lb *lb,
 ct->flags = NX_CT_F_COMMIT;
 ct->alg = 0;
 
-size_t nat_offset;
-nat_offset = ofpacts.size;
-ofpbuf_pull(, nat_offset);
-
 struct ofpact_nat *nat = ofpact_put_NAT();
 nat->flags = NX_NAT_F_SRC;
 nat->range_af = address_family;
@@ -1828,8 +1825,10 @@ add_lb_ct_snat_hairpin_vip_flow(const struct 
ovn_controller_lb *lb,
? lb->hairpin_snat_ips.ipv6_addrs[0].addr
: lb_vip->vip;
 }
-ofpacts.header = ofpbuf_push_uninit(, nat_offset);
-ofpact_finish(, >ofpact);
+
+ct = ofpbuf_at_assert(, ct_offset, sizeof *ct);
+ofpacts.header = ct;
+ofpact_finish_CT(, );
 
 struct match match = MATCH_CATCHALL_INITIALIZER;
 
diff --git a/lib/actions.c b/lib/actions.c
index e8cc0994d..9d19dd2dc 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -715,13 +715,18 @@ encode_CT_NEXT(const struct ovnact_ct_next *ct_next,
 const struct ovnact_encode_params *ep,
 struct ofpbuf *ofpacts)
 {
+size_t ct_offset = ofpacts->size;
+
 struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
 ct->recirc_table = first_ptable(ep, ep->pipeline) + ct_next->ltable;
 ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE)
 : mf_from_id(MFF_LOG_DNAT_ZONE);
 ct->zone_src.ofs = 0;
 ct->zone_src.n_bits = 16;
-ofpact_finish(ofpacts, >ofpact);
+
+ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
+ofpacts->header = ct;
+ofpact_finish_CT(ofpacts, );
 }
 
 static void
@@ -761,7 +766,6 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
 struct ofpbuf *ofpacts)
 {
 size_t ct_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, ct_offset);
 
 struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
 ct->flags = NX_CT_F_COMMIT;
@@ -776,25 +780,17 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
  * collisions at commit time between NATed and firewalled-only sessions.
  */
 if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) {
-size_t nat_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, nat_offset);
-
 struct ofpact_nat *nat = ofpact_put_NAT(ofpacts);
 nat->flags = 0;
 nat->range_af = AF_UNSPEC;
 nat->flags |= NX_NAT_F_SRC;
-ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
-ct = ofpacts->header;
 }
 
-size_t set_field_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, set_field_offset);
-
 ovnacts_encode(on->nested, on->nested_len, ep, ofpacts);
-ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
-ct = ofpacts->header;
-ofpact_finish(ofpacts, >ofpact);
-ofpbuf_push_uninit(ofpacts, ct_offset);
+
+ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
+ofpacts->header = ct;
+ofpact_finish_CT(ofpacts, );
 }
 
 static void
@@ -1027,20 +1023,16 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
   enum mf_field_id zone_src, struct ofpbuf *ofpacts)
 {
 const size_t ct_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, ct_offset);
 
 struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
 ct->recirc_table = cn->ltable + first_ptable(ep, ep->pipeline);
 ct->zone_src.field = mf_from_id(zone_src);
 ct->zone_src.ofs = 0;
 ct->zone_src.n_bits = 16;
-ct->flags = 0;
+ct->flags = cn->comm

[ovs-dev] [Patch ovn v2 1/1] northd: BGP port mirroring.

[Martin Kalcok]

This change adds a 'bgp-mirror' option to LRP that allows
mirroring of BGP control plane traffic to an arbitrary LSP
in its peer LS.

The option expects a string with a LSP name. When set,
any traffic entering LS that's destined for any of the
LRP's IP addresses (including IPv6 LLA) is redirected
to the LSP specified in the option's value.

This enables external BGP daemons to listen on an interface
bound to a LSP and effectively act as if they were listening
on (and speaking from) LRP's IP address.

Signed-off-by: Martin Kalcok 
---
 northd/northd.c | 87 +
 northd/ovn-northd.8.xml | 23 +++
 ovn-nb.xml  | 14 +++
 tests/ovn-northd.at | 45 +
 tests/system-ovn.at | 86 
 5 files changed, 255 insertions(+)

diff --git a/northd/northd.c b/northd/northd.c
index 4353df07d..e07bf68cc 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -13048,6 +13048,92 @@ build_arp_resolve_flows_for_lrp(struct ovn_port *op,
 }
 }
 
+static void
+build_bgp_redirect_rule__(
+const char *s_addr, const char *bgp_port_name, bool is_ipv6,
+struct ovn_port *ls_peer, struct lflow_table *lflows,
+struct ds *match, struct ds *actions)
+{
+int ip_ver = is_ipv6 ? 6 : 4;
+/* Redirect packets in the input pipeline destined for LR's IP to
+ * the port specified in 'bgp-mirror' option.
+ */
+ds_clear(match);
+ds_clear(actions);
+ds_put_format(match, "ip%d.dst == %s && tcp.dst == 179", ip_ver, s_addr);
+ds_put_format(actions, "outport = \"%s\"; output;", bgp_port_name);
+ovn_lflow_add(lflows, ls_peer->od, S_SWITCH_IN_L2_LKUP, 100,
+  ds_cstr(match),
+  ds_cstr(actions),
+  ls_peer->lflow_ref);
+
+
+/* Drop any traffic originating from 'bgp-mirror' port that does
+ * not originate from BGP daemon port. This blocks unnecessary
+ * traffic like ARP broadcasts or IPv6 router solicitation packets
+ * from the dummy 'bgp-mirror' port.
+ */
+ds_clear(match);
+ds_put_format(match, "inport == \"%s\"", bgp_port_name);
+ovn_lflow_add(lflows, ls_peer->od, S_SWITCH_IN_CHECK_PORT_SEC, 80,
+  ds_cstr(match),
+  REGBIT_PORT_SEC_DROP " = 1; next;",
+  ls_peer->lflow_ref);
+
+ds_put_format(match,
+  " && ip%d.src == %s && tcp.src == 179",
+  ip_ver,
+  s_addr);
+ovn_lflow_add(lflows, ls_peer->od, S_SWITCH_IN_CHECK_PORT_SEC, 81,
+  ds_cstr(match),
+  REGBIT_PORT_SEC_DROP " = check_in_port_sec(); next;",
+  ls_peer->lflow_ref);
+}
+
+static void
+build_lrouter_bgp_redirect(
+struct ovn_port *op, struct lflow_table *lflows,
+struct ds *match, struct ds *actions)
+{
+/* LRP has to have a peer.*/
+if (op->peer == NULL) {
+return;
+}
+/* LRP has to have NB record.*/
+if (op->nbrp == NULL) {
+return;
+}
+
+/* Proceed only for LRPs that have 'bgp-mirror' option set. Value of this
+ * option is the name of LSP to which a BGP traffic will be mirrored.
+ */
+const char *bgp_port = smap_get(>nbrp->options, "bgp-mirror");
+if (bgp_port == NULL) {
+return;
+}
+
+if (op->cr_port != NULL) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(, "Option 'bgp-mirror' is not supported on"
+  " Distributed Gateway Port '%s'", op->key);
+return;
+}
+
+/* Mirror traffic destined for LRP's IPs and default BGP port
+ * to the port defined in 'bgp-mirror' option.
+ */
+for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
+const char *ip_s = op->lrp_networks.ipv4_addrs[i].addr_s;
+build_bgp_redirect_rule__(ip_s, bgp_port, false,  op->peer, lflows,
+  match, actions);
+}
+for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
+const char *ip_s = op->lrp_networks.ipv6_addrs[i].addr_s;
+build_bgp_redirect_rule__(ip_s, bgp_port, true, op->peer, lflows,
+  match, actions);
+}
+}
+
 /* This function adds ARP resolve flows related to a LSP. */
 static void
 build_arp_resolve_flows_for_lsp(
@@ -16003,6 +16089,7 @@ build_lswitch_and_lrouter_iterate_by_lrp(struct 
ovn_port *op,
 lsi->meter_groups, op->lflow_ref);
 build_lrouter_icmp_packet_toobig_admin_flows(op, lsi->lflows, >match,
  >actions, op->lflow_ref);
+build_lrouter_bgp_redirect(op, lsi->lflows

[ovs-dev] [Patch ovn v2 0/1] Add LRP option for mirroring of BGP traffic.

[Martin Kalcok]

This patch introduces a new Logical Router Port option
called 'bgp-mirror' that enables mirroring of BGP control plane
traffic to an arbitrary port. More details directly in the commit message.

Although not directly dependent, this change is intended to go
hand-in-hand with a series posted by fnordhal[0].

[0] https://mail.openvswitch.org/pipermail/ovs-dev/2024-July/415795.html

Martin Kalcok (1):
  northd: BGP port mirroring.

 northd/northd.c | 87 +
 northd/ovn-northd.8.xml | 23 +++
 ovn-nb.xml  | 14 +++
 tests/ovn-northd.at | 45 +
 tests/system-ovn.at | 86 
 5 files changed, 255 insertions(+)

-- 
2.40.1

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

Re: [ovs-dev] [Patch ovn 1/2] test macros: Ensure ADD_VETH macro sets IPv6 LLA.

[martin . kalcok]

This change actually broke a bunch of other tests, which I did not
expect. I'll resubmit v2 without it.

Martin.

On Mon, 2024-07-15 at 19:19 +0200, Martin Kalcok wrote:
> Test macro ADD_VET takes an optional argument to specify
> MAC address for the interface. However this address is
> set only after the interface is brought UP, causing it
> to keep IPv6 Link Local Address based on its old MAC.
> 
> Toggling the interface down/up after a new MAC address
> is set fixes this issue.
> 
> Signed-off-by: Martin Kalcok 
> ---
>  tests/system-common-macros.at | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/system-common-macros.at b/tests/system-common-
> macros.at
> index 691c271a3..e7dee9d28 100644
> --- a/tests/system-common-macros.at
> +++ b/tests/system-common-macros.at
> @@ -69,6 +69,9 @@ m4_define([ADD_VETH],
>    NS_CHECK_EXEC([$2], [ip link set dev $1 up])
>    if test -n "$5"; then
>  NS_CHECK_EXEC([$2], [ip link set dev $1 address $5])
> +    # Toggle the interface down/up to get an IPv6 LLA that
> matches its MAC addr.
> +    NS_CHECK_EXEC([$2], [ip link set down $1])
> +    NS_CHECK_EXEC([$2], [ip link set up $1])
>    fi
>    if test -n "$6"; then
>  NS_CHECK_EXEC([$2], [ip route add $6 dev $1])

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

[ovs-dev] [Patch ovn 2/2] northd: BGP port mirroring.

[Martin Kalcok]

This change adds a 'bgp-mirror' option to LRP that allows
mirroring of BGP control plane traffic to an arbitrary LSP
in its peer LS.

The option expects a string with a LSP name. When set,
any traffic entering LS that's destined for any of the
LRP's IP addresses (including IPv6 LLA) is redirected
to the LSP specified in the option's value.

This enables external BGP daemons to listen on an interface
bound to a LSP and effectively act as if they were listening
on (and speaking from) LRP's IP address.

Signed-off-by: Martin Kalcok 
---
 northd/northd.c | 87 +
 northd/ovn-northd.8.xml | 23 +++
 ovn-nb.xml  | 14 +++
 tests/ovn-northd.at | 45 +
 tests/system-ovn.at | 82 ++
 5 files changed, 251 insertions(+)

diff --git a/northd/northd.c b/northd/northd.c
index 4353df07d..e07bf68cc 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -13048,6 +13048,92 @@ build_arp_resolve_flows_for_lrp(struct ovn_port *op,
 }
 }
 
+static void
+build_bgp_redirect_rule__(
+const char *s_addr, const char *bgp_port_name, bool is_ipv6,
+struct ovn_port *ls_peer, struct lflow_table *lflows,
+struct ds *match, struct ds *actions)
+{
+int ip_ver = is_ipv6 ? 6 : 4;
+/* Redirect packets in the input pipeline destined for LR's IP to
+ * the port specified in 'bgp-mirror' option.
+ */
+ds_clear(match);
+ds_clear(actions);
+ds_put_format(match, "ip%d.dst == %s && tcp.dst == 179", ip_ver, s_addr);
+ds_put_format(actions, "outport = \"%s\"; output;", bgp_port_name);
+ovn_lflow_add(lflows, ls_peer->od, S_SWITCH_IN_L2_LKUP, 100,
+  ds_cstr(match),
+  ds_cstr(actions),
+  ls_peer->lflow_ref);
+
+
+/* Drop any traffic originating from 'bgp-mirror' port that does
+ * not originate from BGP daemon port. This blocks unnecessary
+ * traffic like ARP broadcasts or IPv6 router solicitation packets
+ * from the dummy 'bgp-mirror' port.
+ */
+ds_clear(match);
+ds_put_format(match, "inport == \"%s\"", bgp_port_name);
+ovn_lflow_add(lflows, ls_peer->od, S_SWITCH_IN_CHECK_PORT_SEC, 80,
+  ds_cstr(match),
+  REGBIT_PORT_SEC_DROP " = 1; next;",
+  ls_peer->lflow_ref);
+
+ds_put_format(match,
+  " && ip%d.src == %s && tcp.src == 179",
+  ip_ver,
+  s_addr);
+ovn_lflow_add(lflows, ls_peer->od, S_SWITCH_IN_CHECK_PORT_SEC, 81,
+  ds_cstr(match),
+  REGBIT_PORT_SEC_DROP " = check_in_port_sec(); next;",
+  ls_peer->lflow_ref);
+}
+
+static void
+build_lrouter_bgp_redirect(
+struct ovn_port *op, struct lflow_table *lflows,
+struct ds *match, struct ds *actions)
+{
+/* LRP has to have a peer.*/
+if (op->peer == NULL) {
+return;
+}
+/* LRP has to have NB record.*/
+if (op->nbrp == NULL) {
+return;
+}
+
+/* Proceed only for LRPs that have 'bgp-mirror' option set. Value of this
+ * option is the name of LSP to which a BGP traffic will be mirrored.
+ */
+const char *bgp_port = smap_get(>nbrp->options, "bgp-mirror");
+if (bgp_port == NULL) {
+return;
+}
+
+if (op->cr_port != NULL) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(, "Option 'bgp-mirror' is not supported on"
+  " Distributed Gateway Port '%s'", op->key);
+return;
+}
+
+/* Mirror traffic destined for LRP's IPs and default BGP port
+ * to the port defined in 'bgp-mirror' option.
+ */
+for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
+const char *ip_s = op->lrp_networks.ipv4_addrs[i].addr_s;
+build_bgp_redirect_rule__(ip_s, bgp_port, false,  op->peer, lflows,
+  match, actions);
+}
+for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
+const char *ip_s = op->lrp_networks.ipv6_addrs[i].addr_s;
+build_bgp_redirect_rule__(ip_s, bgp_port, true, op->peer, lflows,
+  match, actions);
+}
+}
+
 /* This function adds ARP resolve flows related to a LSP. */
 static void
 build_arp_resolve_flows_for_lsp(
@@ -16003,6 +16089,7 @@ build_lswitch_and_lrouter_iterate_by_lrp(struct 
ovn_port *op,
 lsi->meter_groups, op->lflow_ref);
 build_lrouter_icmp_packet_toobig_admin_flows(op, lsi->lflows, >match,
  >actions, op->lflow_ref);
+build_lrouter_bgp_redirect(op, lsi->lflows, >match, >act

[ovs-dev] [Patch ovn 0/2] Add LRP option for mirroring of BGP traffic.

[Martin Kalcok]

This series of patches introduces new Logical Router Port option
called 'bgp-mirror' that enables mirroring of BGP control plane
traffic to an arbitrary port. More info in commit message 2/2.

Although not directly dependant, this change is intended go
hand-in-hand with a series posted by fnordhal[0].

[0] https://mail.openvswitch.org/pipermail/ovs-dev/2024-July/415795.html

Martin Kalcok (2):
  test macros: Ensure ADD_VETH macro sets IPv6 LLA.
  northd: BGP port mirroring.

 northd/northd.c   | 87 +++
 northd/ovn-northd.8.xml   | 23 +
 ovn-nb.xml| 14 ++
 tests/ovn-northd.at   | 45 ++
 tests/system-common-macros.at |  3 ++
 tests/system-ovn.at   | 82 +
 6 files changed, 254 insertions(+)

-- 
2.40.1

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

[ovs-dev] [Patch ovn 1/2] test macros: Ensure ADD_VETH macro sets IPv6 LLA.

[Martin Kalcok]

Test macro ADD_VET takes an optional argument to specify
MAC address for the interface. However this address is
set only after the interface is brought UP, causing it
to keep IPv6 Link Local Address based on its old MAC.

Toggling the interface down/up after a new MAC address
is set fixes this issue.

Signed-off-by: Martin Kalcok 
---
 tests/system-common-macros.at | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 691c271a3..e7dee9d28 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -69,6 +69,9 @@ m4_define([ADD_VETH],
   NS_CHECK_EXEC([$2], [ip link set dev $1 up])
   if test -n "$5"; then
 NS_CHECK_EXEC([$2], [ip link set dev $1 address $5])
+# Toggle the interface down/up to get an IPv6 LLA that matches its MAC 
addr.
+NS_CHECK_EXEC([$2], [ip link set down $1])
+NS_CHECK_EXEC([$2], [ip link set up $1])
   fi
   if test -n "$6"; then
 NS_CHECK_EXEC([$2], [ip route add $6 dev $1])
-- 
2.40.1

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

[ovs-dev] [Patch ovn] actions: Explicitly finish CT actions.

[Martin Kalcok]

As discussed here [0], a couple of functions that encode
CT-related actions were using older, manual, way of finishing
the action.

As amusil mentioned here [1], there's a shorter and more explicit
way of doing it. This change replaces manual way with the more
explicit aproach.

[0] https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/414667.html
[1] https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html

Signed-off-by: Martin Kalcok 
---
 lib/actions.c | 36 +---
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/lib/actions.c b/lib/actions.c
index e8cc0994d..51da6210f 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -761,7 +761,6 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
 struct ofpbuf *ofpacts)
 {
 size_t ct_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, ct_offset);
 
 struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
 ct->flags = NX_CT_F_COMMIT;
@@ -777,24 +776,19 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
  */
 if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) {
 size_t nat_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, nat_offset);
 
 struct ofpact_nat *nat = ofpact_put_NAT(ofpacts);
 nat->flags = 0;
 nat->range_af = AF_UNSPEC;
 nat->flags |= NX_NAT_F_SRC;
-ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
-ct = ofpacts->header;
+ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct);
 }
 
-size_t set_field_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, set_field_offset);
-
 ovnacts_encode(on->nested, on->nested_len, ep, ofpacts);
-ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
-ct = ofpacts->header;
-ofpact_finish(ofpacts, >ofpact);
-ofpbuf_push_uninit(ofpacts, ct_offset);
+
+ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
+ofpacts->header = ct;
+ofpact_finish_CT(ofpacts, );
 }
 
 static void
@@ -1027,7 +1021,6 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
   enum mf_field_id zone_src, struct ofpbuf *ofpacts)
 {
 const size_t ct_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, ct_offset);
 
 struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
 ct->recirc_table = cn->ltable + first_ptable(ep, ep->pipeline);
@@ -1040,7 +1033,6 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
 struct ofpact_nat *nat;
 size_t nat_offset;
 nat_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, nat_offset);
 
 nat = ofpact_put_NAT(ofpacts);
 nat->range_af = cn->family;
@@ -1081,13 +1073,13 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
 }
 }
 
-ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
-ct = ofpacts->header;
+ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct);
+ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
 if (cn->commit) {
 ct->flags |= NX_CT_F_COMMIT;
 }
-ofpact_finish(ofpacts, >ofpact);
-ofpbuf_push_uninit(ofpacts, ct_offset);
+ofpacts->header = ct;
+ofpact_finish_CT(ofpacts, );
 }
 
 static void
@@ -1383,7 +1375,6 @@ encode_ct_lb(const struct ovnact_ct_lb *cl,
 /* ct_lb without any destinations means that this is an established
  * connection and we just need to do a NAT. */
 const size_t ct_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, ct_offset);
 
 struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
 struct ofpact_nat *nat;
@@ -1397,16 +1388,15 @@ encode_ct_lb(const struct ovnact_ct_lb *cl,
 ct->alg = 0;
 
 nat_offset = ofpacts->size;
-ofpbuf_pull(ofpacts, nat_offset);
 
 nat = ofpact_put_NAT(ofpacts);
 nat->flags = 0;
 nat->range_af = AF_UNSPEC;
 
-ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
-ct = ofpacts->header;
-ofpact_finish(ofpacts, >ofpact);
-ofpbuf_push_uninit(ofpacts, ct_offset);
+ct = ofpbuf_at_assert(ofpacts, nat_offset, sizeof *ct);
+ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct);
+ofpacts->header = ct;
+ofpact_finish_CT(ofpacts, );
 return;
 }
 
-- 
2.40.1

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

Re: [ovs-dev] [PATCH ovn v1] controller: Fix issue with ct_commit encode.

[Martin Kalcok]

> On 11 Jun 2024, at 00:07, Dumitru Ceara  wrote:
> 
> On 6/10/24 18:05, martin.kal...@canonical.com 
>  wrote:
>> On Mon, 2024-06-10 at 12:54 +, Naveen Yerramneni wrote:
>>> Action length is getting set incorrectly during ct_commit encode
>>> due to which ct action is getting skipped  during phsycial flows
>>> creation. This issue is noticed only if ct_commit is prefixed
>>> with other actions.
>>> 
>>> logical flow: reg8[17] = 1; ct_commit { ct_mark.blocked = 1; };
>>> without fix: encodes as set_field:0x2/0x2-
 xreg4
>>> with fix: encodes as set_field:0x2/0x2-
 xreg4,ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1-
 ct_mark))
>>> 
>>> Signed-off-by: Naveen Yerramneni 
>>> ---
> 
> Thanks Naveen, for the catch!
> 
>>> v1:
> 
> Well, this should actually be v2 but that's a technicality.
> 
>>>   - Addressed review comments from Ales.
>>> ---
>>>  lib/actions.c | 3 +++
>>>  tests/ovn.at  | 3 +++
>>>  2 files changed, 6 insertions(+)
>>> 
>>> diff --git a/lib/actions.c b/lib/actions.c
>>> index 361d55009..794d2e916 100644
>>> --- a/lib/actions.c
>>> +++ b/lib/actions.c
>>> @@ -760,6 +760,8 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>>>  const struct ovnact_encode_params *ep
>>> OVS_UNUSED,
>>>  struct ofpbuf *ofpacts)
>>>  {
>>> +size_t ct_offset = ofpacts->size;
>>> +ofpbuf_pull(ofpacts, ct_offset);
> 
> Nit: missing new line (but I can fix that up at apply time).
> 
>>>  struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
>>>  ct->flags = NX_CT_F_COMMIT;
>>>  ct->recirc_table = NX_CT_RECIRC_NONE;
>>> @@ -791,6 +793,7 @@ 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);
>>> +ofpbuf_push_uninit(ofpacts, ct_offset);
>> 
>> Just a thought here. I was just wondering if this wouldn't be a good
>> opportunity to also replace the "manual" way of finishing the action
>> encoding (as @amusil referred to it in this review [0]) with the more
>> explicit approach (example included in [0]).
>> 
> 
> I agree that we should do that but we should do it in multiple
> functions, e.g.:
> 
> encode_CT_NEXT()
> encode_ct_nat()
> encode_ct_lb()
> 
> I think it's fine to do that in a follow up patch.  Martin, do you have
> time to post one?  Otherwise I'll put it on my list of things to do in
> the future.

Sure, I’ll find some time to make the proposal this week.

> 
> Regarding the current bug fix I'm planning to apply and backport this
> patch, pending ovsrobot re-run results.
> 
> Recheck-request: github-robot
> 
> Regards,
> Dumitru
> 
>> [0]
>> https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html
>> 
>>>  }
>>>  
>>>  static void
>>> diff --git a/tests/ovn.at b/tests/ovn.at
>>> index dc6aafd53..d4f62f487 100644
>>> --- a/tests/ovn.at
>>> +++ b/tests/ovn.at
>>> @@ -1315,6 +1315,9 @@ ct_commit {
>>> ct_label=0x181716151413121110090807060504030201; };
>>>  141-bit constant is not compatible with 128-bit field ct_label.
>>>  ct_commit { ip4.dst = 192.168.0.1; };
>>>  Field ip4.dst is not modifiable.
>>> +reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; };
>>> +encodes as set_field:0x2/0x2-
 xreg4,ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1-
 ct_mark))
>>> +has prereqs ip
>>>  
>>>  # Legact ct_commit_v1 action.
>>>  ct_commit();
>> 
>> Martin.
>> ___
>> dev mailing list
>> d...@openvswitch.org 
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Martin.

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

Re: [ovs-dev] [PATCH ovn v1] controller: Fix issue with ct_commit encode.

[martin . kalcok]

On Mon, 2024-06-10 at 12:54 +, Naveen Yerramneni wrote:
> Action length is getting set incorrectly during ct_commit encode
> due to which ct action is getting skipped  during phsycial flows
> creation. This issue is noticed only if ct_commit is prefixed
> with other actions.
> 
> logical flow: reg8[17] = 1; ct_commit { ct_mark.blocked = 1; };
> without fix: encodes as set_field:0x2/0x2-
> >xreg4
> with fix: encodes as set_field:0x2/0x2-
> >xreg4,ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1-
> >ct_mark))
> 
> Signed-off-by: Naveen Yerramneni 
> ---
> v1:
>   - Addressed review comments from Ales.
> ---
>  lib/actions.c | 3 +++
>  tests/ovn.at  | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/lib/actions.c b/lib/actions.c
> index 361d55009..794d2e916 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -760,6 +760,8 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on,
>  const struct ovnact_encode_params *ep
> OVS_UNUSED,
>  struct ofpbuf *ofpacts)
>  {
> +    size_t ct_offset = ofpacts->size;
> +    ofpbuf_pull(ofpacts, ct_offset);
>  struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
>  ct->flags = NX_CT_F_COMMIT;
>  ct->recirc_table = NX_CT_RECIRC_NONE;
> @@ -791,6 +793,7 @@ 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);
> +    ofpbuf_push_uninit(ofpacts, ct_offset);

Just a thought here. I was just wondering if this wouldn't be a good
opportunity to also replace the "manual" way of finishing the action
encoding (as @amusil referred to it in this review [0]) with the more
explicit approach (example included in [0]).

[0]
https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html

>  }
>  
>  static void
> diff --git a/tests/ovn.at b/tests/ovn.at
> index dc6aafd53..d4f62f487 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1315,6 +1315,9 @@ ct_commit {
> ct_label=0x181716151413121110090807060504030201; };
>  141-bit constant is not compatible with 128-bit field ct_label.
>  ct_commit { ip4.dst = 192.168.0.1; };
>  Field ip4.dst is not modifiable.
> +reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; };
> +    encodes as set_field:0x2/0x2-
> >xreg4,ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1-
> >ct_mark))
> +    has prereqs ip
>  
>  # Legact ct_commit_v1 action.
>  ct_commit();

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

Re: [ovs-dev] [Patch] ovsdb-client: Document "--timeout" option in help.

[martin . kalcok]

I made a silly typo in the commit message s/infomration/information.
Would it be possible to fix it when the commit is applied, or is it
worth posting v2?

Thanks,
Martin.

On Fri, 2024-06-07 at 13:53 +0200, Martin Kalcok wrote:
> Add infomration about "-t" and "--timeout" options for ovsdb-client.
> The option is documented in the "Other options" section, similar
> to how "ovs-appctl" has it.
> 
> Signed-off-by: Martin Kalcok 
> ---
>  ovsdb/ovsdb-client.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
> index cf2ecfd08..80ef90c82 100644
> --- a/ovsdb/ovsdb-client.c
> +++ b/ovsdb/ovsdb-client.c
> @@ -474,6 +474,8 @@ usage(void)
>  vlog_usage();
>  ovs_replay_usage();
>  printf("\nOther options:\n"
> +   "  -t, --timeout=SECS  limits ovsdb-client
> runtime to\n"
> +   "  approximately SECS
> seconds.\n"
>     "  -h, --help  display this help
> message\n"
>     "  -V, --version   display version
> information\n");
>  exit(EXIT_SUCCESS);

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

[ovs-dev] [Patch] ovsdb-client: Document "--timeout" option in help.

[Martin Kalcok]

Add infomration about "-t" and "--timeout" options for ovsdb-client.
The option is documented in the "Other options" section, similar
to how "ovs-appctl" has it.

Signed-off-by: Martin Kalcok 
---
 ovsdb/ovsdb-client.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
index cf2ecfd08..80ef90c82 100644
--- a/ovsdb/ovsdb-client.c
+++ b/ovsdb/ovsdb-client.c
@@ -474,6 +474,8 @@ usage(void)
 vlog_usage();
 ovs_replay_usage();
 printf("\nOther options:\n"
+   "  -t, --timeout=SECS  limits ovsdb-client runtime to\n"
+   "  approximately SECS seconds.\n"
"  -h, --help  display this help message\n"
"  -V, --version   display version information\n");
 exit(EXIT_SUCCESS);
-- 
2.40.1

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

Re: [ovs-dev] [Patch] ovsdb-client: Add "COLUMN" arg to help for 'dump'.

[martin . kalcok]

Thank you for correction Ilya. Sorry I originally missed your review,
but the v2 is up now.

Martin.

On Tue, 2024-05-28 at 19:35 +0200, Ilya Maximets wrote:
> On 5/28/24 19:35, Ilya Maximets wrote:
> > On 5/21/24 10:38, Martin Kalcok wrote:
> > > Help text for 'ovsdb-client dump' does not mention that it's
> > > capable
> > > of dumping a specific column's contents if the user supplies the
> > > column's name as a fourth positional argument.
> > > 
> > > Signed-off-by: Martin Kalcok 
> > > ---
> > >  ovsdb/ovsdb-client.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
> > > index cf2ecfd08..0a3f1d4df 100644
> > > --- a/ovsdb/ovsdb-client.c
> > > +++ b/ovsdb/ovsdb-client.c
> > > @@ -451,9 +451,9 @@ usage(void)
> > >     "    wait until DATABASE reaches STATE "
> > >     "(\"added\" or \"connected\" or \"removed\")\n"
> > >     "    in DATBASE on SERVER.\n"
> > > -   "\n  dump [SERVER] [DATABASE] [TABLE]\n"
> > > -   "    dump contents of TABLE (or all tables) in
> > > DATABASE on SERVER\n"
> > > -   "    to stdout\n"
> > > +   "\n  dump [SERVER] [DATABASE] [TABLE] [COLUMN]\n"
> > > +   "    dump contents of COLUMN, TABLE (or all tables)
> > > in DATABASE\n"
> > > +   "    on SERVER to stdout\n"
> > 
> > I think it was '[TABLE [COLUMN]...]' until commit 85226894ddec
> > ("ovsdb-client: support monitor2") removed that part on accident.
> 
> It is also how it is defined in the man page.
> 
> > 
> > Best regards, Ilya Maximets.
> 

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

[ovs-dev] [Patch v2] ovsdb-client: Add "COLUMN" arg to help for 'dump'.

[Martin Kalcok]

Help text for 'ovsdb-client dump' does not mention that it's capable
of dumping a specific column's contents if the user supplies the
column's name as a fourth positional argument.

Signed-off-by: Martin Kalcok 
---
 ovsdb/ovsdb-client.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
index cf2ecfd08..b7b189c7e 100644
--- a/ovsdb/ovsdb-client.c
+++ b/ovsdb/ovsdb-client.c
@@ -451,9 +451,9 @@ usage(void)
"wait until DATABASE reaches STATE "
"(\"added\" or \"connected\" or \"removed\")\n"
"in DATBASE on SERVER.\n"
-   "\n  dump [SERVER] [DATABASE] [TABLE]\n"
-   "dump contents of TABLE (or all tables) in DATABASE on SERVER\n"
-   "to stdout\n"
+   "\n  dump [SERVER] [DATABASE] [TABLE [COLUMN]...]\n"
+   "dump contents of COLUMNs, TABLE (or all tables) in DATABASE\n"
+   "on SERVER to stdout\n"
"\n  backup [SERVER] [DATABASE] > SNAPSHOT\n"
"dump database contents in the form of a database file\n"
"\n  [--force] restore [SERVER] [DATABASE] < SNAPSHOT\n"
-- 
2.40.1

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

[ovs-dev] [Patch] ovsdb-client: Add "COLUMN" arg to help for 'dump'.

[Martin Kalcok]

Help text for 'ovsdb-client dump' does not mention that it's capable
of dumping a specific column's contents if the user supplies the
column's name as a fourth positional argument.

Signed-off-by: Martin Kalcok 
---
 ovsdb/ovsdb-client.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
index cf2ecfd08..0a3f1d4df 100644
--- a/ovsdb/ovsdb-client.c
+++ b/ovsdb/ovsdb-client.c
@@ -451,9 +451,9 @@ usage(void)
"wait until DATABASE reaches STATE "
"(\"added\" or \"connected\" or \"removed\")\n"
"in DATBASE on SERVER.\n"
-   "\n  dump [SERVER] [DATABASE] [TABLE]\n"
-   "dump contents of TABLE (or all tables) in DATABASE on SERVER\n"
-   "to stdout\n"
+   "\n  dump [SERVER] [DATABASE] [TABLE] [COLUMN]\n"
+   "dump contents of COLUMN, TABLE (or all tables) in DATABASE\n"
+   "on SERVER to stdout\n"
"\n  backup [SERVER] [DATABASE] > SNAPSHOT\n"
"dump database contents in the form of a database file\n"
"\n  [--force] restore [SERVER] [DATABASE] < SNAPSHOT\n"
-- 
2.40.1

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

Re: [ovs-dev] [Patch] ovsdb-client: Add missing arg to help for 'dump'.

[martin . kalcok]

Thanks for the merge and backports Simon.

On Tue, 2024-05-07 at 17:26 +0100, Simon Horman wrote:
> On Tue, May 07, 2024 at 03:16:25PM +0100, Simon Horman wrote:
> > On Fri, May 03, 2024 at 02:22:27PM +0200,
> > martin.kal...@canonical.com wrote:
> 
> ...
> 
> > Thanks,
> > 
> > Sorry for the confusion on my part.
> > Now that I can see it again I have applied it to main.
> > 
> > - ovsdb-client: Add missing arg to help for 'dump'.
> >   https://github.com/openvswitch/ovs/commit/0940a51b1f5a
> > 
> > As a follow-up I'll look at backporting this.
> 
> Hi Martin,
> 
> In preparing the backports I noticed that dump also supports optional
> TABLES arguments. Could you consider a follow-up patch to add
> that to the help text too?

You mean COLUMNS argument, right? Yeah, I didn't noticed this earlier.
I'll post follow-up patch, thanks.

> 
> In any case, I have prepared and pushed the following backports.
> 
> branch-3.3:
> - ovsdb-client: Add missing arg to help for 'dump'.
>   https://github.com/openvswitch/ovs/commit/20ed5491c53f
> 
> branch-3.2:
> - ovsdb-client: Add missing arg to help for 'dump'.
>   https://github.com/openvswitch/ovs/commit/a3f9c5ae1b1e
> 
> branch-3.1:
> - ovsdb-client: Add missing arg to help for 'dump'.
>   https://github.com/openvswitch/ovs/commit/8b029bd258d5
> 
> branch-3.0:
> - ovsdb-client: Add missing arg to help for 'dump'.
>   https://github.com/openvswitch/ovs/commit/65dcd5f27da0
> 
> branch-2.17:
> - ovsdb-client: Add missing arg to help for 'dump'.
>   https://github.com/openvswitch/ovs/commit/d1a2af7c33fa
> 

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

Re: [ovs-dev] [Patch] ovsdb-client: Add missing arg to help for 'dump'.

[martin . kalcok]

Hi Simon,

It seems that it[0] got archived, not sure how. I unticked the
'archived' box and it should again appear in the the list in OVS
patchwork.

Thanks for the review.
Martin.

[0]
https://patchwork.ozlabs.org/project/openvswitch/patch/2024050043.357669-1-martin.kal...@canonical.com/

On Fri, 2024-05-03 at 13:16 +0100, Simon Horman wrote:
> On Wed, May 01, 2024 at 01:50:36PM +0100, Simon Horman wrote:
> > On Wed, May 01, 2024 at 01:10:43PM +0200, Martin Kalcok wrote:
> > > Help text for 'ovsdb-client dump' does not mention that it's
> > > capable
> > > of dumping specific table's contents if user supplies table's
> > > name
> > > as a third positional argument.
> > > 
> > > Signed-off-by: Martin Kalcok 
> > 
> > Acked-by: Simon Horman 
> 
> Hi Martin,
> 
> I am unsure what has happened, perhaps there is some silly error on
> my
> side, but I am unable to see this patch in OVS patchwork [1].
> 
> If it is not there, would it be possible to resubmit?
> 
> [1] https://patchwork.ozlabs.org/project/openvswitch/list/

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

[ovs-dev] [Patch] ovsdb-client: Add missing arg to help for 'dump'.

[Martin Kalcok]

Help text for 'ovsdb-client dump' does not mention that it's capable
of dumping specific table's contents if user supplies table's name
as a third positional argument.

Signed-off-by: Martin Kalcok 
---
 ovsdb/ovsdb-client.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
index 7249805ba..cf2ecfd08 100644
--- a/ovsdb/ovsdb-client.c
+++ b/ovsdb/ovsdb-client.c
@@ -451,8 +451,9 @@ usage(void)
"wait until DATABASE reaches STATE "
"(\"added\" or \"connected\" or \"removed\")\n"
"in DATBASE on SERVER.\n"
-   "\n  dump [SERVER] [DATABASE]\n"
-   "dump contents of DATABASE on SERVER to stdout\n"
+   "\n  dump [SERVER] [DATABASE] [TABLE]\n"
+   "dump contents of TABLE (or all tables) in DATABASE on SERVER\n"
+   "to stdout\n"
"\n  backup [SERVER] [DATABASE] > SNAPSHOT\n"
"dump database contents in the form of a database file\n"
"\n  [--force] restore [SERVER] [DATABASE] < SNAPSHOT\n"
-- 
2.40.1

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

[ovs-dev] [Patch ovn] docs: Typo. Remove duplicated "to" in ovn-sb.xml.

[Martin Kalcok]

Signed-off-by: Martin Kalcok 
---
 ovn-sb.xml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ovn-sb.xml b/ovn-sb.xml
index f9fb6c304..bf4689f12 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -1456,7 +1456,7 @@
   
 ct_dnat sends the packet through the DNAT zone in
 connection tracking table to unDNAT any packet that was DNATed in
-the opposite direction.  The packet is then automatically sent to
+the opposite direction.  The packet is then automatically sent
 to the next tables as if followed by next; action.
 The next tables will see the changes in the packet caused by
 the connection tracker.
@@ -1498,7 +1498,7 @@
 ct_dnat_in_czone sends the packet through the common
 NAT zone (used for both DNAT and SNAT) in connection tracking table
 to unDNAT any packet that was DNATed in the opposite direction.
-The packet is then automatically sent to to the next tables as if
+The packet is then automatically sent to the next tables as if
 followed by next; action.  The next tables will see
 the changes in the packet caused by the connection tracker.
   
-- 
2.40.1

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

Re: [ovs-dev] [PATCH ovn] tests: Fix netcat 7.94 issues.

[martin . kalcok]

Thanks Dumitru,
I didn't noticed that the patch was applied while I was typing my
message :D

Overall I think it's fine if it stays as it was proposed by Ales. I
just wanted  to raise a very fringe concern that perhaps using two
separate UDP servers could mask some underlying issue and if there's a
way to consistently reproduce failures of this test, I'd be happy to
take a look at it.


On Tue, 2024-04-23 at 11:15 +0200, Dumitru Ceara wrote:
> On 4/23/24 11:12, martin.kal...@canonical.com wrote:
> > Hi Ales,
> > Sorry that these new tests are causing problems. Just out of
> > curiosity,
> > do you have link to some failing test runs? I'll add few thoughts
> > below.
> > 
> > On Tue, 2024-04-23 at 09:41 +0200, Ales Musil wrote:
> > > The netcat 7.94 allows multiple connections over udp (-k/--keep-
> > > open)
> > > [0],
> > > without this option the connection can be closed "unexpctedly".
> > > This
> > > to keep the test backward compatible make new servers for every
> > > UDP
> > > connection.
> > > 
> > > The second issue is that netcat is attempting to listen on IPv4
> > > when
> > > the there isn't any server address specified and fails to do so.
> > > Add
> > > -6 flag to indicate that this is pure IPv6 connection.
> > > 
> > > [0]
> > > https://github.com/nmap/nmap/commit/4e6c8feb153c0c9ff8a68cd841669d650319ab45
> > > Fixes: 40136a2f2c84 ("northd: Fix direct access to SNAT
> > > network.")
> > > Signed-off-by: Ales Musil 
> > > ---
> > >  tests/system-ovn.at | 14 +++---
> > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > > index 41c051c1e..6dcdb45d1 100644
> > > --- a/tests/system-ovn.at
> > > +++ b/tests/system-ovn.at
> > > @@ -3582,7 +3582,6 @@ test_connectivity_from_ext() {
> > >  local ip=$1; shift
> > >  
> > >  # Start listening daemons for UDP and TCP connections
> > 
> > nit: Comment above should be adjusted to reflect that UDP server is
> > no
> > longer started here.
> >  
> 
> I forgot to mention in my previous email that I had fixed this up. 
> Same
> for the other comment.
> 
> > > -    NETNS_DAEMONIZE($vm, [nc -l -u 1234], [nc-$vm-$ip-udp.pid])
> > >  NETNS_DAEMONIZE($vm, [nc -l -k 1235], [nc-$vm-$ip-tcp.pid])
> > >  
> > >  # Ensure that vm can be pinged on the specified IP
> > > @@ -3592,8 +3591,13 @@ test_connectivity_from_ext() {
> > >  ])
> > >  
> > >  # Perform two consecutive UDP connections to the specified
> > > IP
> > > +    NETNS_DAEMONIZE($vm, [nc -l -u 1234], [nc-$vm-$ip-udp.pid])
> > >  NS_CHECK_EXEC([alice1], [nc -u $ip 1234 -p 2000 -z])
> > > +    kill $(cat nc-$vm-$ip-udp.pid)
> > > +
> > > +    NETNS_DAEMONIZE($vm, [nc -l -u 1234], [nc-$vm-$ip-udp.pid])
> > >  NS_CHECK_EXEC([alice1], [nc -u $ip 1234 -p 2000 -z])
> > > +    kill $(cat nc-$vm-$ip-udp.pid)
> > 
> > In the original tests, the two separate, consecutive, client
> > connections used same source port, which should ensure that the
> > test
> > passes even without the '-k' option. This should work because a
> > socket
> > is opened (and kept alive) between source IP:PORT and destination
> > IP:PORT on the server side. So if two client processes use the same
> > source port, to the server it just looks like a single client
> > sending
> > two datagrams.
> > 
> > The reason why I decided to go with two consecutive client
> > connections
> > is that inlining script for a single 'nc' process to send two
> > messages
> > was very cumbersome
> > 
> > I'm just wondering that if the connection is "unexpectedly closed"
> > between the two client messages, whether it could signal some kind
> > of
> > connection problem that should be investigated.
> > 
> > >  
> > >  # Send data over TCP connection to the specified IP
> > >  NS_CHECK_EXEC([alice1], [echo "TCP test" | nc --send-only
> > > $ip
> > > 1235])
> > > @@ -3781,8 +3785,7 @@ test_connectivity_from_ext() {
> > >  local ip=$1; shift
> > >  
> > >  # Start listening daemons for UDP and TCP connections
> > 
> > nit: Same nitpick about the comment as above
> > 
> > > -    NETNS_DAEMONIZE($vm, [nc -l -u 1234], [nc-$vm-$ip-udp.pid])
> > > -    NETNS_DAEMONIZE($vm, [nc -l -k 1235], [nc-$vm-$ip-tcp.pid])
> > > +    NETNS_DAEMONIZE($vm, [nc -6 -l -k 1235], [nc-$vm-$ip-
> > > tcp.pid])
> > >  
> > >  # Ensure that vm can be pinged on the specified IP
> > >  NS_CHECK_EXEC([alice1], [ping -q -c 3 -i 0.3 -w 2 $ip |
> > > FORMAT_PING], \
> > > @@ -3791,8 +3794,13 @@ test_connectivity_from_ext() {
> > >  ])
> > >  
> > >  # Perform two consecutive UDP connections to the specified
> > > IP
> > > +    NETNS_DAEMONIZE($vm, [nc -6 -l -u 1234], [nc-$vm-$ip-
> > > udp.pid])
> > >  NS_CHECK_EXEC([alice1], [nc -u $ip 1234 -p 2000 -z])
> > > +    kill $(cat nc-$vm-$ip-udp.pid)
> > > +
> > > +    NETNS_DAEMONIZE($vm, [nc -6 -l -u 1234], [nc-$vm-$ip-
> > > udp.pid])
> > >  NS_CHECK_EXEC([alice1], [nc -u $ip 1234 -p 2000 -z])
> > > +    kill 

Re: [ovs-dev] [PATCH ovn] tests: Fix netcat 7.94 issues.

[martin . kalcok]

Hi Ales,
Sorry that these new tests are causing problems. Just out of curiosity,
do you have link to some failing test runs? I'll add few thoughts
below.

On Tue, 2024-04-23 at 09:41 +0200, Ales Musil wrote:
> The netcat 7.94 allows multiple connections over udp (-k/--keep-open)
> [0],
> without this option the connection can be closed "unexpctedly". This
> to keep the test backward compatible make new servers for every UDP
> connection.
> 
> The second issue is that netcat is attempting to listen on IPv4 when
> the there isn't any server address specified and fails to do so. Add
> -6 flag to indicate that this is pure IPv6 connection.
> 
> [0]
> https://github.com/nmap/nmap/commit/4e6c8feb153c0c9ff8a68cd841669d650319ab45
> Fixes: 40136a2f2c84 ("northd: Fix direct access to SNAT network.")
> Signed-off-by: Ales Musil 
> ---
>  tests/system-ovn.at | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 41c051c1e..6dcdb45d1 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -3582,7 +3582,6 @@ test_connectivity_from_ext() {
>  local ip=$1; shift
>  
>  # Start listening daemons for UDP and TCP connections

nit: Comment above should be adjusted to reflect that UDP server is no
longer started here.
 
> -    NETNS_DAEMONIZE($vm, [nc -l -u 1234], [nc-$vm-$ip-udp.pid])
>  NETNS_DAEMONIZE($vm, [nc -l -k 1235], [nc-$vm-$ip-tcp.pid])
>  
>  # Ensure that vm can be pinged on the specified IP
> @@ -3592,8 +3591,13 @@ test_connectivity_from_ext() {
>  ])
>  
>  # Perform two consecutive UDP connections to the specified IP
> +    NETNS_DAEMONIZE($vm, [nc -l -u 1234], [nc-$vm-$ip-udp.pid])
>  NS_CHECK_EXEC([alice1], [nc -u $ip 1234 -p 2000 -z])
> +    kill $(cat nc-$vm-$ip-udp.pid)
> +
> +    NETNS_DAEMONIZE($vm, [nc -l -u 1234], [nc-$vm-$ip-udp.pid])
>  NS_CHECK_EXEC([alice1], [nc -u $ip 1234 -p 2000 -z])
> +    kill $(cat nc-$vm-$ip-udp.pid)

In the original tests, the two separate, consecutive, client
connections used same source port, which should ensure that the test
passes even without the '-k' option. This should work because a socket
is opened (and kept alive) between source IP:PORT and destination
IP:PORT on the server side. So if two client processes use the same
source port, to the server it just looks like a single client sending
two datagrams.

The reason why I decided to go with two consecutive client connections
is that inlining script for a single 'nc' process to send two messages
was very cumbersome

I'm just wondering that if the connection is "unexpectedly closed"
between the two client messages, whether it could signal some kind of
connection problem that should be investigated.

>  
>  # Send data over TCP connection to the specified IP
>  NS_CHECK_EXEC([alice1], [echo "TCP test" | nc --send-only $ip
> 1235])
> @@ -3781,8 +3785,7 @@ test_connectivity_from_ext() {
>  local ip=$1; shift
>  
>  # Start listening daemons for UDP and TCP connections

nit: Same nitpick about the comment as above

> -    NETNS_DAEMONIZE($vm, [nc -l -u 1234], [nc-$vm-$ip-udp.pid])
> -    NETNS_DAEMONIZE($vm, [nc -l -k 1235], [nc-$vm-$ip-tcp.pid])
> +    NETNS_DAEMONIZE($vm, [nc -6 -l -k 1235], [nc-$vm-$ip-tcp.pid])
>  
>  # Ensure that vm can be pinged on the specified IP
>  NS_CHECK_EXEC([alice1], [ping -q -c 3 -i 0.3 -w 2 $ip |
> FORMAT_PING], \
> @@ -3791,8 +3794,13 @@ test_connectivity_from_ext() {
>  ])
>  
>  # Perform two consecutive UDP connections to the specified IP
> +    NETNS_DAEMONIZE($vm, [nc -6 -l -u 1234], [nc-$vm-$ip-udp.pid])
>  NS_CHECK_EXEC([alice1], [nc -u $ip 1234 -p 2000 -z])
> +    kill $(cat nc-$vm-$ip-udp.pid)
> +
> +    NETNS_DAEMONIZE($vm, [nc -6 -l -u 1234], [nc-$vm-$ip-udp.pid])
>  NS_CHECK_EXEC([alice1], [nc -u $ip 1234 -p 2000 -z])
> +    kill $(cat nc-$vm-$ip-udp.pid)
>  
>  # Send data over TCP connection to the specified IP
>  NS_CHECK_EXEC([alice1], [echo "TCP test" | nc --send-only $ip
> 1235])

Thanks,
Martin.

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

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

[Martin Kalcok]

Thanks for the review Ales and sorry about those unrelated changes. I just
noticed those two typos and thought I'll sneak in the fix.

On Mon, Apr 22, 2024 at 10:49 AM Ales Musil  wrote:

>
>
> On Fri, Apr 19, 2024 at 2:33 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,
>
> there are only two things that can be addressed during merge.
>
>
>>  controller/chassis.c  |   8 ++
>>  include/ovn/actions.h |   8 +-
>>  include/ovn/features.h|   1 +
>>  lib/actions.c | 155 +-
>>  northd/en-global-config.c |  10 +++
>>  northd/en-global-config.h |   1 +
>>  ovn-sb.xml|  22 +-
>>  tests/ovn.at  |  16 
>>  utilities/ovn-trace.c |  45 ---
>>  9 files changed, 199 insertions(+), 67 deletions(-)
>>
>> 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..aa5e4ab89 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_to_zone) \
>>  OVNACT(CT_DNAT,   ovnact_ct_nat)  \
>>  OVNACT(CT_SNAT,   ovnact_ct_nat)  \
>>  OVNACT(CT_DNAT_IN_CZONE,  ovnact_ct_nat)  \
>> @@ -76,7 +77,7 @@ struct collector_set_ids;
>>  OVNACT(CT_LB_MARK,ovnact_ct_lb)   \
>>  OVNACT(SELECT,ovnact_select)  \
>>  OVNACT(CT_CLEAR,  ovnact_null)\
>> -OVNACT(CT_COMMIT_NAT, ovnact_ct_commit_nat)   \
>> +OVNACT(CT_COMMIT_NAT, ovnact_ct_commit_to_zone) \
>>  OVNACT(CLONE, ovnact_nest)\
>>  OVNACT(ARP,   ovnact_nest)\
>>  OVNACT(ICMP4, ovnact_nest)\
>> @@ -290,11 +291,12 @@ struct ovnact_ct_nat {
>>  uint8_t ltable; /* Logical table ID of next table. */
>>  };
>>
>> -/* OVNACT_CT_COMMIT_NAT. */
>> -struct ovnact_ct_commit_nat {
>> +/* OVNACT_CT_COMMIT_TO_ZONE, OVNACT_CT_COMMIT_NAT. */
>> +struct ovnact_ct_commit_to_zone {
>>  struct ovnact ovnact;
>>
>>  bool dnat_zone;
>> +bool do_nat;
>>  uint8_t ltable;
>>  };
>>
>> diff --git a/include/ovn/features.h b/include/ovn/feature

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

[Martin Kalcok]

wrt the failed ovn-kubernetes tests, they seemed to have passed
successfully in my branch [0]. Is it possible that the tests are unstable?

[0] https://github.com/mkalcok/ovn/actions/runs/8752915096

On Fri, Apr 19, 2024 at 2:33 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 
> ---
>  northd/northd.c | 66 ---
>  northd/ovn-northd.8.xml | 28 +++
>  tests/ovn-northd.at | 76 +
>  tests/system-ovn.at | 68 
>  4 files changed, 219 insertions(+), 19 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 37f443e70..2c2eee445 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -14425,20 +14425,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 lflo

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

[Martin Kalcok]

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 
---
 northd/northd.c | 66 ---
 northd/ovn-northd.8.xml | 28 +++
 tests/ovn-northd.at | 76 +
 tests/system-ovn.at | 68 
 4 files changed, 219 insertions(+), 19 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 37f443e70..2c2eee445 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -14425,20 +14425,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", l3dgw_port->json_key);
+ds_put_format(match, " && %s == %s",
+  is_reverse ? "inport" : "outport

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

[Martin Kalcok]

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 
---
 controller/chassis.c  |   8 ++
 include/ovn/actions.h |   8 +-
 include/ovn/features.h|   1 +
 lib/actions.c | 155 +-
 northd/en-global-config.c |  10 +++
 northd/en-global-config.h |   1 +
 ovn-sb.xml|  22 +-
 tests/ovn.at  |  16 
 utilities/ovn-trace.c |  45 ---
 9 files changed, 199 insertions(+), 67 deletions(-)

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..aa5e4ab89 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_to_zone) \
 OVNACT(CT_DNAT,   ovnact_ct_nat)  \
 OVNACT(CT_SNAT,   ovnact_ct_nat)  \
 OVNACT(CT_DNAT_IN_CZONE,  ovnact_ct_nat)  \
@@ -76,7 +77,7 @@ struct collector_set_ids;
 OVNACT(CT_LB_MARK,ovnact_ct_lb)   \
 OVNACT(SELECT,ovnact_select)  \
 OVNACT(CT_CLEAR,  ovnact_null)\
-OVNACT(CT_COMMIT_NAT, ovnact_ct_commit_nat)   \
+OVNACT(CT_COMMIT_NAT, ovnact_ct_commit_to_zone) \
 OVNACT(CLONE, ovnact_nest)\
 OVNACT(ARP,   ovnact_nest)\
 OVNACT(ICMP4, ovnact_nest)\
@@ -290,11 +291,12 @@ struct ovnact_ct_nat {
 uint8_t ltable; /* Logical table ID of next table. */
 };
 
-/* OVNACT_CT_COMMIT_NAT. */
-struct ovnact_ct_commit_nat {
+/* OVNACT_CT_COMMIT_TO_ZONE, OVNACT_CT_COMMIT_NAT. */
+struct ovnact_ct_commit_to_zone {
 struct ovnact ovnact;
 
 bool dnat_zone;
+bool do_nat;
 uint8_t ltable;
 };
 
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..b9bdcd48d 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -870,6 +870,45 @@ parse_ct_nat(struct action_context *ctx, const char *name,
 }
 }
 
+static void
+parse_ct_commit_to_zone(struct action_context *ctx,  const char *name,
+bool do_nat, bool require_param,
+struct ovnact_ct_commit_to_zone *cn)
+{
+add_prerequisite(ctx, "ip");
+
+if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
+lexer_error(ctx->lexer,
+"\"%s\" action not allowed in last table.", name);
+return;
+}
+
+cn->ltable = ctx->pp->cur_ltable + 1;
+cn->do_nat = do_nat;
+cn->dnat_zone = true;

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

[Martin Kalcok]

Thanks for another round of review.

On Wed, Apr 17, 2024 at 1:45 PM Ales Musil  wrote:

>
>
> 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/

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

[Martin Kalcok]

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 
---
 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);
+
+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);",
+  ct_commit->dnat_zone ? "dnat" : "snat");
+
+}
+
+static void
+encode_CT_COMMIT_TO_ZONE(const struct ovnact_ct_commit_nat *ct_commit,
+ const struct ovnact_encode_params *ep,
+ struct ofpbuf *ofpacts)
+{
+struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
+ct->flags = NX_CT_F_COMMIT;
+ct->recirc_table = NX_CT_RECIRC_NONE;
+ct->zone_src.ofs 

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

[Martin Kalcok]

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 
---
 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", l3dgw_port->json_key);
+ds_put_format(match, " && %s == %s",
+  is_reverse ? "inport" : "outport

Re: [ovs-dev] [Patch ovn v2 1/2] actions: Enable specifying zone for ct_commit.

[Martin Kalcok]

Thanks for the feedback Ales, Dumitru. I'm back from vacation so I'm again
focusing on this change.

On Tue, Apr 2, 2024 at 5:33 PM Dumitru Ceara  wrote:

> On 3/20/24 12:20, Ales Musil wrote:
> > On Tue, Mar 12, 2024 at 9:18 PM Martin Kalcok <
> martin.kal...@canonical.com>
> > wrote:
> >
> > Hi Martin,
> >
>
> Hi Ales, Martin, Numan,
>
> > sorry for the late reply.
> >
>
> Sorry for jumping in late in the discussion.
>
> > Following up on the comments from v1.
> >>
> >> @amusil You were right that the struct in actions.h was not necessary
> >> then. However I also noticed that I forgot to modify
> `format_CT_COMMIT_V1`
> >> function and for that I think the struct is necessary. I need to
> >> distinguish whether the `ct_commit` action was called with dnat, snat,
> or
> >> without any argument to format it properly. If you still don't like it,
> I
> >> can try to figure out how to do it without the struct, but I couldn't
> >> figure out an obvious solution, so I left it in there in this version.
> >>
> >
> > I still think we should basically remove any other functionality from
> > ct_commit_v1 and leave just those two options.
> >
> >
> >>
> >> Regarding the action formatting unit tests, I have two
> >> assumptions/questions:
> >> 1. There's no way to distinguish router/switch datapaths in these
> tests. I
> >> saw that both `ct_commit_nat(dnat)` and `ct_commit_nat(snat)` [0]
> expect to
> >> encode into the same zone, although they'd output different zones if
> they
> >> were used in LR datapath.
> >>
> >
> > Yeah that's correct, this is limitation/intention of the way we encode
> the
> > actions in the test-ovn.c.
> >
> >
> >> 2. When action formats into identical string as was its input (e.g.
> >> "ct_commit(snat)" -> "ct_commit(snat)"), the test should not use "format
> >> as" check, otherwise it fails. (This one took me a while to figure out,
> as
> >> it was not obvious from the testlog why it was failing)
> >>
> >
> > That is correct and a little confusing, if the formatting is the same as
> > the original input the test utility doesn't produce the output again.
> >
> >>
> >> Are these correct?
> >>
> >> @numans I though a lot about your suggestions for different action
> names,
> >> but I think that current "ct_commit(snat/dnat)" fits semantically the
> most.
> >> Brand new actions would share pretty much all of the code with current
> >> "ct_commit_v1" handling. So to address your comments regarding the
> backward
> >> compatibility, I added new feature flag, following Ales' approach in
> [1]. I
> >> believe that this should avoid problems of backward incompatibility in
> >> cases when northd is upgraded but controller is not.
> >>
> >
> > I don't think the plan is to backport those or is it? In case of this
> being
> > considered as a feature we don't need the feature flag, but that depends
> on
> > decision from maintainers.
> >
>
> Right, in theory this shouldn't be backported so unless there's a strong
> case for it to be backported we don't really need the feature flags.
>

My main motivation for using the feature flag was to avoid problems in a
scenario where northd gets upgraded to a new version before controller
does. Without the feature flag, in that scenario, northd would start using
new action that controller would not understand. With the feature flag,
northd would use this new action only if the controller supports it, and
fall back to the old behavior if it does not. Is there a way to achieve
this without feature flag? Or is it a non-issue?


>
> >
> >>
> >> @0-day Robot: I forgot to run checkpatch.py, my bad. However the only
> >> problem is 81 char line in ovn-sb.xml and there are already many lines
> that
> >> go over this limit. Should I create v3 if this turns out to be the only
> >> modification needed?
> >>
> >
> > That's fine, as you said there are instances where it is over the 79
> limit.
> >
> >
> >>
> >> [0]
> >>
> https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/tests/ovn.at#L1500-L1511
> >> [1]
> >>
> https://github.com/ovn-org/ovn/commit/43f741c2f029a68a11436e5b14c5bbda6e207dd3#diff-ca917e7415d06776f8ee2baf6102a866c5c31f998e4df93ff8eaa246b65e1da2
> >>
> >> On Tue, Mar 12, 2024 at 8:45

Re: [ovs-dev] [PATCH ovn] actions: Remove ct_commit_v1.

[Martin Kalcok]

mit,zone=NXM_NX_REG13[[0..15]])
>> -has prereqs ip
>> +Syntax error at `(' expecting `;'.
>>  ct_commit(ct_mark=1);
>> -formats as ct_commit(ct_mark=0x1);
>> -encodes as
>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1->ct_mark))
>> -has prereqs ip
>> +Syntax error at `(' expecting `;'.
>>  ct_commit(ct_mark=1/1);
>> -formats as ct_commit(ct_mark=0x1/0x1);
>> -encodes as
>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1->ct_mark))
>> -has prereqs ip
>> +Syntax error at `(' expecting `;'.
>>  ct_commit(ct_label=1);
>> -formats as ct_commit(ct_label=0x1);
>> -encodes as
>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1->ct_label))
>> -has prereqs ip
>> +Syntax error at `(' expecting `;'.
>>  ct_commit(ct_label=1/1);
>> -formats as ct_commit(ct_label=0x1/0x1);
>> -encodes as
>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1->ct_label))
>> -has prereqs ip
>> +Syntax error at `(' expecting `;'.
>>  ct_commit(ct_mark=1, ct_label=2);
>> -formats as ct_commit(ct_mark=0x1, ct_label=0x2);
>> -encodes as
>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label))
>> -has prereqs ip
>> +Syntax error at `(' expecting `;'.
>>
>>  ct_commit(ct_label=0x01020304050607080910111213141516);
>> -formats as ct_commit(ct_label=0x1020304050607080910111213141516);
>> -encodes as
>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1020304050607080910111213141516->ct_label))
>> -has prereqs ip
>> +Syntax error at `(' expecting `;'.
>>  ct_commit(ct_label=0x181716151413121110090807060504030201);
>> -formats as ct_commit(ct_label=0x16151413121110090807060504030201);
>> -encodes as
>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x16151413121110090807060504030201->ct_label))
>> -has prereqs ip
>> +Syntax error at `(' expecting `;'.
>>
>>  
>> ct_commit(ct_label=0x100/0x100);
>> -encodes as
>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x100/0x100->ct_label))
>> -has prereqs ip
>> +Syntax error at `(' expecting `;'.
>>  ct_commit(ct_label=18446744073709551615);
>> -formats as ct_commit(ct_label=0x);
>> -encodes as
>> ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x->ct_label))
>> -has prereqs ip
>> +Syntax error at `(' expecting `;'.
>>  ct_commit(ct_label=18446744073709551616);
>> -Decimal constants must be less than 2**64.
>> +Syntax error at `(' expecting `;'.
>>
>>  ct_mark = 12345
>>  Field ct_mark is not modifiable.
>> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
>> index e0f1c3ec9a..5e55fbbcc0 100644
>> --- a/utilities/ovn-trace.c
>> +++ b/utilities/ovn-trace.c
>> @@ -3107,7 +3107,6 @@ trace_actions(const struct ovnact *ovnacts, size_t
>> ovnacts_len,
>>  execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline,
>> super);
>>  break;
>>
>> -case OVNACT_CT_COMMIT_V1:
>>  case OVNACT_CT_COMMIT_V2:
>>  /* Nothing to do. */
>>  break;
>> --
>> 2.44.0
>>
>>
> Makes sense, thanks!
>
> Acked-by: Ales Musil 
>
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> amu...@redhat.com
> <https://red.ht/sig>
>


-- 
Best Regards,
Martin Kalcok.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Martin Kalcok]

Hi Ales, thanks for another round of review.

On Wed, Mar 20, 2024 at 12:34 PM Ales Musil  wrote:

>
>
> On Thu, Mar 14, 2024 at 2:13 PM Martin Kalcok 
> wrote:
>
>> Hello all,
>> I have one more follow-up regarding the comments in v1. @amusil, you were
>> concerned about the impact this change would have on the performance so I
>> ran some tests to try to gauge it. I used following setup with two physical
>> hosts connected over switch:
>>
>>|Physical ext. | Hypervisor1
>>|   network|
>> alice1-|- switch -|-- DLR --- foo1
>>|  |
>>
>> * alice1 acts as external device.
>>   * It doesn't run OVN
>>   * It's connected to regular physical network.
>> * Hypervisor1 runs OVN chassis
>> * foo1 is a container behind Distributed LR with SNAT enabled.
>>
>> Goal of this test was to measure whether the proposed patch affects
>> throughput of the traffic that flows from "foo1" to external network. I
>> used iperf3 and ran 3x 5 minute measurements with 10 streams ("iperf3 -i 0
>> -t 300 -P 10 -c alice1"). I used 5 minute for each measurement to smooth
>> out possible jitter and I ran each measurement 3 times to get feel for the
>> variance in the network throughput over longer period. I also did some
>> trial and error testing that showed that 10 parallel streams reached
>> highest throughput.
>>
>> Following are the three (summary) results with this patch applied:
>>
>> # run 1 (with patch)
>> [SUM]   0.00-300.00 sec   296 GBytes  8.48 Gbits/sec  sender
>> [SUM]   0.00-300.00 sec   296 GBytes  8.48 Gbits/sec  receiver
>>
>> # run 2 (with patch)
>> [SUM]   0.00-300.00 sec   288 GBytes  8.24 Gbits/sec sender
>> [SUM]   0.00-300.00 sec   288 GBytes  8.23 Gbits/sec receiver
>>
>> # run 3 (with patch)
>> [SUM]   0.00-300.00 sec   299 GBytes  8.55 Gbits/sec sender
>> [SUM]   0.00-300.00 sec   299 GBytes  8.55 Gbits/sec receiver
>>
>> Next thing I did was to rebuild ovn-controller without these patches,
>> replaced the ovn-controller binary on the Hypervisor1 and restarted
>> ovn-controller daemon. I verified that the feature flag "ct-commit-to-zone"
>> was removed from the chassis (in the SB database), which forced the
>> recompute, and then I reran the tests:
>>
>> # run 1 (clean)
>> [SUM]   0.00-300.00 sec   300 GBytes  8.59 Gbits/sec sender
>> [SUM]   0.00-300.00 sec   300 GBytes  8.59 Gbits/sec receiver
>>
>> # run 2 (clean)
>> [SUM]   0.00-300.00 sec   285 GBytes  8.15 Gbits/sec sender
>> [SUM]   0.00-300.00 sec   285 GBytes  8.15 Gbits/sec receiver
>>
>> # run 3 (clean)
>> [SUM]   0.00-300.00 sec   295 GBytes  8.46 Gbits/sec sender
>> [SUM]   0.00-300.00 sec   295 GBytes  8.46 Gbits/sec receiver
>>
>> These tests show that while there was some fluctuation between each
>> individual test, when comparing patched and clean version, they come out
>> about the same. I'm happy to run more tests/scenarios if you have something
>> else in mind that should be tested.
>>
>>
>>
> Hi Martin,
>
> thank you for the performance numbers, it looks fine to me. I have some
> comments, see down below.
>
>
>> On Wed, Mar 13, 2024 at 10:17 AM Martin Kalcok <
>> martin.kal...@canonical.com> wrote:
>>
>>> Regarding the failed unstable test in the CI, I suspect that this is not
>>> related to the code change, I've had couple successful CI runs in my branch
>>> [0].
>>>
>>> Martin.
>>>
>>> [0] https://github.com/mkalcok/ovn/actions/runs/8256539983
>>>
>>> On Tue, Mar 12, 2024 at 8:45 PM Martin Kalcok <
>>> martin.kal...@canonical.com> 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 

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

[Martin Kalcok]

Hello all,
I have one more follow-up regarding the comments in v1. @amusil, you were
concerned about the impact this change would have on the performance so I
ran some tests to try to gauge it. I used following setup with two physical
hosts connected over switch:

   |Physical ext. | Hypervisor1
   |   network|
alice1-|- switch -|-- DLR --- foo1
   |  |

* alice1 acts as external device.
  * It doesn't run OVN
  * It's connected to regular physical network.
* Hypervisor1 runs OVN chassis
* foo1 is a container behind Distributed LR with SNAT enabled.

Goal of this test was to measure whether the proposed patch affects
throughput of the traffic that flows from "foo1" to external network. I
used iperf3 and ran 3x 5 minute measurements with 10 streams ("iperf3 -i 0
-t 300 -P 10 -c alice1"). I used 5 minute for each measurement to smooth
out possible jitter and I ran each measurement 3 times to get feel for the
variance in the network throughput over longer period. I also did some
trial and error testing that showed that 10 parallel streams reached
highest throughput.

Following are the three (summary) results with this patch applied:

# run 1 (with patch)
[SUM]   0.00-300.00 sec   296 GBytes  8.48 Gbits/sec  sender
[SUM]   0.00-300.00 sec   296 GBytes  8.48 Gbits/sec  receiver

# run 2 (with patch)
[SUM]   0.00-300.00 sec   288 GBytes  8.24 Gbits/sec sender
[SUM]   0.00-300.00 sec   288 GBytes  8.23 Gbits/sec receiver

# run 3 (with patch)
[SUM]   0.00-300.00 sec   299 GBytes  8.55 Gbits/sec sender
[SUM]   0.00-300.00 sec   299 GBytes  8.55 Gbits/sec receiver

Next thing I did was to rebuild ovn-controller without these patches,
replaced the ovn-controller binary on the Hypervisor1 and restarted
ovn-controller daemon. I verified that the feature flag "ct-commit-to-zone"
was removed from the chassis (in the SB database), which forced the
recompute, and then I reran the tests:

# run 1 (clean)
[SUM]   0.00-300.00 sec   300 GBytes  8.59 Gbits/sec sender
[SUM]   0.00-300.00 sec   300 GBytes  8.59 Gbits/sec receiver

# run 2 (clean)
[SUM]   0.00-300.00 sec   285 GBytes  8.15 Gbits/sec sender
[SUM]   0.00-300.00 sec   285 GBytes  8.15 Gbits/sec receiver

# run 3 (clean)
[SUM]   0.00-300.00 sec   295 GBytes  8.46 Gbits/sec sender
[SUM]   0.00-300.00 sec   295 GBytes  8.46 Gbits/sec receiver

These tests show that while there was some fluctuation between each
individual test, when comparing patched and clean version, they come out
about the same. I'm happy to run more tests/scenarios if you have something
else in mind that should be tested.


On Wed, Mar 13, 2024 at 10:17 AM Martin Kalcok 
wrote:

> Regarding the failed unstable test in the CI, I suspect that this is not
> related to the code change, I've had couple successful CI runs in my branch
> [0].
>
> Martin.
>
> [0] https://github.com/mkalcok/ovn/actions/runs/8256539983
>
> On Tue, Mar 12, 2024 at 8:45 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
>> 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 wit

[ovs-dev] [Patch ovn] docs: Remove ref. to "ovn-sbctl --no-wait".

[Martin Kalcok]

Couple places in the documentation reference "--wait" and "--no-wait"
options for "ovn-sbctl" but it doesn't support these options [0].

Trying, for example, "ovn-sbctl --no-wait init" exits with:

ovn-sbctl: --no-wait not supported

[0] 
https://github.com/ovn-org/ovn/blob/63b35e2f6789f7843363c8f7a92430402bf989f9/utilities/ovn-sbctl.c#L127

Signed-off-by: Martin Kalcok 
---
 Documentation/intro/install/general.rst | 2 +-
 utilities/ovn-sbctl.8.xml   | 8 
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index ab6209482..6efb3a701 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -428,7 +428,7 @@ the first time after you create the databases with 
ovsdb-tool, though running
 it at any time is harmless::
 
 $ ovn-nbctl --no-wait init
-$ ovn-sbctl --no-wait init
+$ ovn-sbctl init
 
 Start ``ovn-northd``, telling it to connect to the OVN db servers same
 Unix domain socket::
diff --git a/utilities/ovn-sbctl.8.xml b/utilities/ovn-sbctl.8.xml
index 81ab4131d..32035d051 100644
--- a/utilities/ovn-sbctl.8.xml
+++ b/utilities/ovn-sbctl.8.xml
@@ -123,10 +123,10 @@
   
 Instructs the daemon process to run one or more ovn-sbctl
 commands described above and reply with the results of running these
-commands. Accepts the --no-wait, --wait,
---timeout, --dry-run, --oneline,
-and the options described under Table Formatting Options
-in addition to the the command-specific options.
+commands. Accepts the --timeout, --dry-run,
+--oneline, and the options described under
+Table Formatting Options in addition to the the
+command-specific options.
   
 
   exit
-- 
2.40.1

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

Re: [ovs-dev] [Patch ovn v2 1/2] actions: Enable specifying zone for ct_commit.

[Martin Kalcok]

On Tue, Mar 12, 2024 at 9:17 PM Martin Kalcok 
wrote:

> Following up on the comments from v1.
>
> @amusil You were right that the struct in actions.h was not necessary
> then. However I also noticed that I forgot to modify `format_CT_COMMIT_V1`
> function and for that I think the struct is necessary. I need to
> distinguish whether the `ct_commit` action was called with dnat, snat, or
> without any argument to format it properly. If you still don't like it, I
> can try to figure out how to do it without the struct, but I couldn't
> figure out an obvious solution, so I left it in there in this version.
>
> Regarding the action formatting unit tests, I have two
> assumptions/questions:
> 1. There's no way to distinguish router/switch datapaths in these tests. I
> saw that both `ct_commit_nat(dnat)` and `ct_commit_nat(snat)` [0] expect to
> encode into the same zone, although they'd output different zones if they
> were used in LR datapath.
> 2. When action formats into identical string as was its input (e.g.
> "ct_commit(snat)" -> "ct_commit(snat)"), the test should not use "format
> as" check, otherwise it fails. (This one took me a while to figure out, as
> it was not obvious from the testlog why it was failing)
>
> Are these correct?
>
> @numans I though a lot about your suggestions for different action names,
> but I think that current "ct_commit(snat/dnat)" fits semantically the most.
> Brand new actions would share pretty much all of the code with current
> "ct_commit_v1" handling. So to address your comments regarding the backward
> compatibility, I added new feature flag, following Ales' approach in [1]. I
> believe that this should avoid problems of backward incompatibility in
> cases when northd is upgraded but controller is not.
>
>
Sorry to re-iterate on this @numans, I just hope I didn't misunderstood
your original comments on the v1. The way I took it is that you are OK with
using `ct_commit(dnat/snat)` and repurposing the implementation of
`ct_commit_v1`, as long as it doesn't break backwards compatibility. Or do
you think completely new action names with separate implementations are
still needed? I just don't wan't to give impression that I'm ignoring your
suggestions from v1.


> @0-day Robot: I forgot to run checkpatch.py, my bad. However the only
> problem is 81 char line in ovn-sb.xml and there are already many lines that
> go over this limit. Should I create v3 if this turns out to be the only
> modification needed?
>
> [0]
> https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/tests/ovn.at#L1500-L1511
> [1]
> https://github.com/ovn-org/ovn/commit/43f741c2f029a68a11436e5b14c5bbda6e207dd3#diff-ca917e7415d06776f8ee2baf6102a866c5c31f998e4df93ff8eaa246b65e1da2
>
> On Tue, Mar 12, 2024 at 8:45 PM Martin Kalcok 
> wrote:
>
>> Action `ct_commit` currently does not allow specifying zone into
>> which connection is committed. For example, in LR datapath, the
>> `ct_commit`
>> will always use the DNAT zone.
>>
>> This change adds option to use `ct_commit(snat)` or `ct_commit(dnat)` to
>> explicitly specify the zone into which the connection will be committed.
>> It also comes with new feature flag OVN_FEATURE_CT_COMMIT_TO_ZONE to avoid
>> incompatibility between northd and controller in case when controller does
>> not suport these actions.
>>
>> Original behavior of `ct_commit` without the arguments remains unchanged.
>>
>> Signed-off-by: Martin Kalcok 
>> ---
>>  controller/chassis.c  |  8 
>>  include/ovn/actions.h |  9 +
>>  include/ovn/features.h|  1 +
>>  lib/actions.c | 29 -
>>  northd/en-global-config.c | 10 ++
>>  northd/en-global-config.h |  1 +
>>  ovn-sb.xml| 10 ++
>>  tests/ovn.at  |  7 +++
>>  8 files changed, 74 insertions(+), 1 deletion(-)
>>
>> 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,
>>  re

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

[Martin Kalcok]

Regarding the failed unstable test in the CI, I suspect that this is not
related to the code change, I've had couple successful CI runs in my branch
[0].

Martin.

[0] https://github.com/mkalcok/ovn/actions/runs/8256539983

On Tue, Mar 12, 2024 at 8:45 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
> 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 
> ---
>  northd/northd.c | 68 
>  northd/ovn-northd.8.xml | 29 +
>  tests/ovn-northd.at | 33 
>  tests/system-ovn.at | 69 +
>  4 files changed, 180 insertions(+), 19 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 2c3560ce2..25af52d5a 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -14438,20 +14438,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,
> +   

Re: [ovs-dev] [Patch ovn v2 1/2] actions: Enable specifying zone for ct_commit.

[Martin Kalcok]

Following up on the comments from v1.

@amusil You were right that the struct in actions.h was not necessary then.
However I also noticed that I forgot to modify `format_CT_COMMIT_V1`
function and for that I think the struct is necessary. I need to
distinguish whether the `ct_commit` action was called with dnat, snat, or
without any argument to format it properly. If you still don't like it, I
can try to figure out how to do it without the struct, but I couldn't
figure out an obvious solution, so I left it in there in this version.

Regarding the action formatting unit tests, I have two
assumptions/questions:
1. There's no way to distinguish router/switch datapaths in these tests. I
saw that both `ct_commit_nat(dnat)` and `ct_commit_nat(snat)` [0] expect to
encode into the same zone, although they'd output different zones if they
were used in LR datapath.
2. When action formats into identical string as was its input (e.g.
"ct_commit(snat)" -> "ct_commit(snat)"), the test should not use "format
as" check, otherwise it fails. (This one took me a while to figure out, as
it was not obvious from the testlog why it was failing)

Are these correct?

@numans I though a lot about your suggestions for different action names,
but I think that current "ct_commit(snat/dnat)" fits semantically the most.
Brand new actions would share pretty much all of the code with current
"ct_commit_v1" handling. So to address your comments regarding the backward
compatibility, I added new feature flag, following Ales' approach in [1]. I
believe that this should avoid problems of backward incompatibility in
cases when northd is upgraded but controller is not.

@0-day Robot: I forgot to run checkpatch.py, my bad. However the only
problem is 81 char line in ovn-sb.xml and there are already many lines that
go over this limit. Should I create v3 if this turns out to be the only
modification needed?

[0]
https://github.com/ovn-org/ovn/blob/b92ad9e0b408a202273d69ba573f2538e53c6e48/tests/ovn.at#L1500-L1511
[1]
https://github.com/ovn-org/ovn/commit/43f741c2f029a68a11436e5b14c5bbda6e207dd3#diff-ca917e7415d06776f8ee2baf6102a866c5c31f998e4df93ff8eaa246b65e1da2

On Tue, Mar 12, 2024 at 8:45 PM Martin Kalcok 
wrote:

> Action `ct_commit` currently does not allow specifying zone into
> which connection is committed. For example, in LR datapath, the `ct_commit`
> will always use the DNAT zone.
>
> This change adds option to use `ct_commit(snat)` or `ct_commit(dnat)` to
> explicitly specify the zone into which the connection will be committed.
> It also comes with new feature flag OVN_FEATURE_CT_COMMIT_TO_ZONE to avoid
> incompatibility between northd and controller in case when controller does
> not suport these actions.
>
> Original behavior of `ct_commit` without the arguments remains unchanged.
>
> Signed-off-by: Martin Kalcok 
> ---
>  controller/chassis.c  |  8 
>  include/ovn/actions.h |  9 +
>  include/ovn/features.h|  1 +
>  lib/actions.c | 29 -
>  northd/en-global-config.c | 10 ++
>  northd/en-global-config.h |  1 +
>  ovn-sb.xml| 10 ++
>  tests/ovn.at  |  7 +++
>  8 files changed, 74 insertions(+), 1 deletion(-)
>
> 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 49fb96fc6..ce9597cf5 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -259,11 +259,20 @@ struct ovnact_ct_next {
>  uint8_t ltable;/* Logical table ID of next table. */
> 

[ovs-dev] [Patch ovn v2 2/2] northd: Fix direct access to SNAT network on DR.

[Martin Kalcok]

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
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 
---
 northd/northd.c | 68 
 northd/ovn-northd.8.xml | 29 +
 tests/ovn-northd.at | 33 
 tests/system-ovn.at | 69 +
 4 files changed, 180 insertions(+), 19 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 2c3560ce2..25af52d5a 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -14438,20 +14438,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", l3dgw_port->json_key);
+ds_put_format(match, " && %s == %s",
+  is_reverse ? "inport" : "outport",
+  l3dgw_port->json_key);
 if 

[ovs-dev] [Patch ovn v2 1/2] actions: Enable specifying zone for ct_commit.

[Martin Kalcok]

Action `ct_commit` currently does not allow specifying zone into
which connection is committed. For example, in LR datapath, the `ct_commit`
will always use the DNAT zone.

This change adds option to use `ct_commit(snat)` or `ct_commit(dnat)` to
explicitly specify the zone into which the connection will be committed.
It also comes with new feature flag OVN_FEATURE_CT_COMMIT_TO_ZONE to avoid
incompatibility between northd and controller in case when controller does
not suport these actions.

Original behavior of `ct_commit` without the arguments remains unchanged.

Signed-off-by: Martin Kalcok 
---
 controller/chassis.c  |  8 
 include/ovn/actions.h |  9 +
 include/ovn/features.h|  1 +
 lib/actions.c | 29 -
 northd/en-global-config.c | 10 ++
 northd/en-global-config.h |  1 +
 ovn-sb.xml| 10 ++
 tests/ovn.at  |  7 +++
 8 files changed, 74 insertions(+), 1 deletion(-)

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 49fb96fc6..ce9597cf5 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -259,11 +259,20 @@ struct ovnact_ct_next {
 uint8_t ltable;/* Logical table ID of next table. */
 };
 
+/* Conntrack zone to be used for commiting CT entries by the action.
+ * DEFAULT uses default zone for given datapath. */
+enum ovnact_ct_zone {
+OVNACT_CT_ZONE_DEFAULT,
+OVNACT_CT_ZONE_SNAT,
+OVNACT_CT_ZONE_DNAT,
+};
+
 /* OVNACT_CT_COMMIT_V1. */
 struct ovnact_ct_commit_v1 {
 struct ovnact ovnact;
 uint32_t ct_mark, ct_mark_mask;
 ovs_be128 ct_label, ct_label_mask;
+enum ovnact_ct_zone zone;
 };
 
 /* Type of NAT used for the particular action.
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 a45874dfb..9e27a68a5 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -707,6 +707,7 @@ static void
 parse_ct_commit_v1_arg(struct action_context *ctx,
struct ovnact_ct_commit_v1 *cc)
 {
+cc->zone = OVNACT_CT_ZONE_DEFAULT;
 if (lexer_match_id(ctx->lexer, "ct_mark")) {
 if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
 return;
@@ -737,6 +738,10 @@ parse_ct_commit_v1_arg(struct action_context *ctx,
 return;
 }
 lexer_get(ctx->lexer);
+} else if (lexer_match_id(ctx->lexer, "snat")) {
+cc->zone = OVNACT_CT_ZONE_SNAT;
+} else if (lexer_match_id(ctx->lexer, "dnat")) {
+cc->zone = OVNACT_CT_ZONE_DNAT;
 } else {
 lexer_syntax_error(ctx->lexer, NULL);
 }
@@ -800,6 +805,18 @@ format_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc, 
struct ds *s)
 ds_put_hex(s, >ct_label_mask, sizeof cc->ct_label_mask);
 }
 }
+if (cc->zone != OVNACT_CT_ZONE_DEFAULT) {
+if (ds_last(s) != '(') {
+ds_put_cstr(s, ", ");
+}
+
+if (cc->zone == OVNACT_CT_ZONE_SNAT) {
+ds_put_cstr(s, "snat");
+} else if (cc->zone == OVNACT_CT_ZONE_DNAT) {
+ds_put_cstr(s, "dnat");
+}
+}
+
 if (!ds_chomp(s, '(')) {
 ds_put_char(s, ')');
 }
@@ -814,7 +831,17 @@ encode_CT_COMMI

Re: [ovs-dev] [Patch ovn 1/2] actions.c/h: Enable specifying zone for ct_commit.

[Martin Kalcok]

On Thu, Mar 7, 2024 at 1:26 AM Numan Siddique  wrote:

>
>
> On Wed, Mar 6, 2024 at 7:26 AM Martin Kalcok 
> wrote:
>
>> Hi Ales,
>> Thank you for review and helpful comments. I'll update commit subjects
>> for this series in v2.
>>
>> On Wed, Mar 6, 2024 at 7:45 AM Ales Musil  wrote:
>>
>>>
>>>
>>> On Mon, Mar 4, 2024 at 11:56 AM Martin Kalcok <
>>> martin.kal...@canonical.com> wrote:
>>>
>>>> Action `ct_commit` currently does not allow specifying zone into
>>>> which connection is committed. For example, in LR datapath, the
>>>> `ct_commit`
>>>> will always use the DNAT zone.
>>>>
>>>> This change adds option to use `ct_commit(snat)` or `ct_commit(dnat)` to
>>>> explicitly specify the zone into which the connection will be committed.
>>>>
>>>> Original behavior of `ct_commit` without the arguments remains
>>>> unchanged.
>>>>
>>>> Signed-off-by: Martin Kalcok 
>>>>
>>>
>>> Hi Martin,
>>>
>>> thank you for the followup, I have a few comments down below.
>>> One small nit for the commit subject, we usually specify only the module
>>> name without .c/.h e.g. action, ovn-controller, northd etc.
>>> AFAIR ct_commit_v1 is deprecated, but I guess it's fine to repurpose it
>>> for this. Maybe we could remove the old behavior and leave just this?
>>> Before posting v2 we should discuss this with others.
>>> Adding Dumitru and Numan to this discussion.
>>>
>>
>> Ack, I'll defer to your recommendation as to where this should be
>> implemented. If I read it correctly,  `parse_ct_commit_v1_arg` still has
>> code to parse mark/label options but it's never used because if `ct_commit`
>> action is followed by "{", the `parse_CT_COMMIT` will send it to
>> `parse_nested_action` instead. If that's the case and the
>> `parse_CT_COMMIT_V1` is unused, I'd be happy to remove it/repurpose it to
>> something like `parse_CT_COMMIT_ZONE` that handles only
>> `ct_commit({snat,dnat})` actions.
>>
>
>
> Instead of modifying ct_commit,  I'd suggest adding 2 new actions -
> ct_commit_snat and ct_commit_dnat.
>
> This would not have any ambiguity on the backward compatibility.
>
> Thanks
> Numan
>
>
Hi Numan,
Thanks for the feedback, my only concern is that this approach could cause
some confusion wrt the already existing action ct_commit_nat. Aside from
having ct_commit_nat, ct_commit_snat and ct_commit_dnat, there would also
be slight functional differences. According to the ovn-sb docs
"[ct_commit_nat] Applies NAT and commits the connection to the CT", but
ct_commit_snat and ct_commit_dnat would only commit the connection to CT,
without applying any NAT.

Martin.


>
>>
>>>
>>>> ---
>>>>  include/ovn/actions.h |  9 +
>>>>  lib/actions.c | 20 +++-
>>>>  ovn-sb.xml|  9 +
>>>>  3 files changed, 37 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>>>> index 49fb96fc6..ce9597cf5 100644
>>>> --- a/include/ovn/actions.h
>>>> +++ b/include/ovn/actions.h
>>>> @@ -259,11 +259,20 @@ struct ovnact_ct_next {
>>>>  uint8_t ltable;/* Logical table ID of next table.
>>>> */
>>>>  };
>>>>
>>>> +/* Conntrack zone to be used for commiting CT entries by the action.
>>>> + * DEFAULT uses default zone for given datapath. */
>>>> +enum ovnact_ct_zone {
>>>> +OVNACT_CT_ZONE_DEFAULT,
>>>> +OVNACT_CT_ZONE_SNAT,
>>>> +OVNACT_CT_ZONE_DNAT,
>>>> +};
>>>> +
>>>>
>>>
>>> There is no need for this enum, see details below.
>>>
>>>
>>>>  /* OVNACT_CT_COMMIT_V1. */
>>>>  struct ovnact_ct_commit_v1 {
>>>>  struct ovnact ovnact;
>>>>  uint32_t ct_mark, ct_mark_mask;
>>>>  ovs_be128 ct_label, ct_label_mask;
>>>> +enum ovnact_ct_zone zone;
>>>>
>>>  };
>>>>
>>>>  /* Type of NAT used for the particular action.
>>>> diff --git a/lib/actions.c b/lib/actions.c
>>>> index a45874dfb..319e65b6f 100644
>>>> --- a/lib/actions.c
>>>> +++ b/lib/actions.c
>>>> @@ -707,6 +707,7 @@ static void
>>>>  parse_ct_commit_v1_arg(struct action_context *ctx,
>&

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

[Martin Kalcok]

Thanks for reviewing this as well, Ales.

On Wed, Mar 6, 2024 at 8:08 AM Ales Musil  wrote:

>
>
> On Wed, Mar 6, 2024 at 8:07 AM Ales Musil  wrote:
>
>>
>>
>> On Mon, Mar 4, 2024 at 11:56 AM Martin Kalcok <
>> martin.kal...@canonical.com> 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
>>> 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.
>>>
>>>If an additional security is needed, ACLs can be used to filter
>>>out incomming traffic from external networks.
>>>
>>> 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 a few comments down below.
>>
>>
>>>  northd/northd.c | 82 -
>>>  northd/ovn-northd.8.xml | 29 +++
>>>  tests/ovn-northd.at | 15 +++-
>>>  tests/system-ovn.at | 69 ++
>>>  4 files changed, 185 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index 2c3560ce2..4b79b357c 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -14437,21 +14437,28 @@ build_lrouter_out_is_dnat_local(struct
>>> lflow_table *lflows,
>>>  }
>>>
>>>  static void
>>> -build_lrouter_out_snat_match

Re: [ovs-dev] [Patch ovn 1/2] actions.c/h: Enable specifying zone for ct_commit.

[Martin Kalcok]

Hi Ales,
Thank you for review and helpful comments. I'll update commit subjects for
this series in v2.

On Wed, Mar 6, 2024 at 7:45 AM Ales Musil  wrote:

>
>
> On Mon, Mar 4, 2024 at 11:56 AM Martin Kalcok 
> wrote:
>
>> Action `ct_commit` currently does not allow specifying zone into
>> which connection is committed. For example, in LR datapath, the
>> `ct_commit`
>> will always use the DNAT zone.
>>
>> This change adds option to use `ct_commit(snat)` or `ct_commit(dnat)` to
>> explicitly specify the zone into which the connection will be committed.
>>
>> Original behavior of `ct_commit` without the arguments remains unchanged.
>>
>> Signed-off-by: Martin Kalcok 
>>
>
> Hi Martin,
>
> thank you for the followup, I have a few comments down below.
> One small nit for the commit subject, we usually specify only the module
> name without .c/.h e.g. action, ovn-controller, northd etc.
> AFAIR ct_commit_v1 is deprecated, but I guess it's fine to repurpose it
> for this. Maybe we could remove the old behavior and leave just this?
> Before posting v2 we should discuss this with others.
> Adding Dumitru and Numan to this discussion.
>

Ack, I'll defer to your recommendation as to where this should be
implemented. If I read it correctly,  `parse_ct_commit_v1_arg` still has
code to parse mark/label options but it's never used because if `ct_commit`
action is followed by "{", the `parse_CT_COMMIT` will send it to
`parse_nested_action` instead. If that's the case and the
`parse_CT_COMMIT_V1` is unused, I'd be happy to remove it/repurpose it to
something like `parse_CT_COMMIT_ZONE` that handles only
`ct_commit({snat,dnat})` actions.


>
>> ---
>>  include/ovn/actions.h |  9 +
>>  lib/actions.c | 20 +++-
>>  ovn-sb.xml|  9 +
>>  3 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
>> index 49fb96fc6..ce9597cf5 100644
>> --- a/include/ovn/actions.h
>> +++ b/include/ovn/actions.h
>> @@ -259,11 +259,20 @@ struct ovnact_ct_next {
>>  uint8_t ltable;/* Logical table ID of next table. */
>>  };
>>
>> +/* Conntrack zone to be used for commiting CT entries by the action.
>> + * DEFAULT uses default zone for given datapath. */
>> +enum ovnact_ct_zone {
>> +OVNACT_CT_ZONE_DEFAULT,
>> +OVNACT_CT_ZONE_SNAT,
>> +OVNACT_CT_ZONE_DNAT,
>> +};
>> +
>>
>
> There is no need for this enum, see details below.
>
>
>>  /* OVNACT_CT_COMMIT_V1. */
>>  struct ovnact_ct_commit_v1 {
>>  struct ovnact ovnact;
>>  uint32_t ct_mark, ct_mark_mask;
>>  ovs_be128 ct_label, ct_label_mask;
>> +enum ovnact_ct_zone zone;
>>
>  };
>>
>>  /* Type of NAT used for the particular action.
>> diff --git a/lib/actions.c b/lib/actions.c
>> index a45874dfb..319e65b6f 100644
>> --- a/lib/actions.c
>> +++ b/lib/actions.c
>> @@ -707,6 +707,7 @@ static void
>>  parse_ct_commit_v1_arg(struct action_context *ctx,
>> struct ovnact_ct_commit_v1 *cc)
>>  {
>> +cc->zone = OVNACT_CT_ZONE_DEFAULT;
>>  if (lexer_match_id(ctx->lexer, "ct_mark")) {
>>  if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
>>  return;
>> @@ -737,6 +738,10 @@ parse_ct_commit_v1_arg(struct action_context *ctx,
>>  return;
>>  }
>>  lexer_get(ctx->lexer);
>> +} else if (lexer_match_id(ctx->lexer, "snat")) {
>> +cc->zone = OVNACT_CT_ZONE_SNAT;
>> +} else if (lexer_match_id(ctx->lexer, "dnat")) {
>> +cc->zone = OVNACT_CT_ZONE_DNAT;
>>  } else {
>>  lexer_syntax_error(ctx->lexer, NULL);
>>  }
>> @@ -814,7 +819,20 @@ encode_CT_COMMIT_V1(const struct ovnact_ct_commit_v1
>> *cc,
>>  struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
>>  ct->flags = NX_CT_F_COMMIT;
>>  ct->recirc_table = NX_CT_RECIRC_NONE;
>> -ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
>> +switch (cc->zone) {
>> +case OVNACT_CT_ZONE_SNAT:
>> +ct->zone_src.field = mf_from_id(MFF_LOG_SNAT_ZONE);
>> +break;
>> +
>> +case OVNACT_CT_ZONE_DNAT:
>> +ct->zone_src.field = mf_from_id(MFF_LOG_DNAT_ZONE);
>> +break;
>> +
>> +case OVNACT_CT_ZONE_DEFAULT:
>> +default:
>> +ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
>> +break

[ovs-dev] [Patch ovn 2/2] northd.c: Fix direct access to SNAT network on DR.

[Martin Kalcok]

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
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.

   If an additional security is needed, ACLs can be used to filter
   out incomming traffic from external networks.

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 
---
 northd/northd.c | 82 -
 northd/ovn-northd.8.xml | 29 +++
 tests/ovn-northd.at | 15 +++-
 tests/system-ovn.at | 69 ++
 4 files changed, 185 insertions(+), 10 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 2c3560ce2..4b79b357c 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -14437,21 +14437,28 @@ 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)
+build_lrouter_out_snat_direction_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,
+ 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", l3dgw_port->json_key);
+ds_put_format(match, &quo

[ovs-dev] [Patch ovn 1/2] actions.c/h: Enable specifying zone for ct_commit.

[Martin Kalcok]

Action `ct_commit` currently does not allow specifying zone into
which connection is committed. For example, in LR datapath, the `ct_commit`
will always use the DNAT zone.

This change adds option to use `ct_commit(snat)` or `ct_commit(dnat)` to
explicitly specify the zone into which the connection will be committed.

Original behavior of `ct_commit` without the arguments remains unchanged.

Signed-off-by: Martin Kalcok 
---
 include/ovn/actions.h |  9 +
 lib/actions.c | 20 +++-
 ovn-sb.xml|  9 +
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 49fb96fc6..ce9597cf5 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -259,11 +259,20 @@ struct ovnact_ct_next {
 uint8_t ltable;/* Logical table ID of next table. */
 };
 
+/* Conntrack zone to be used for commiting CT entries by the action.
+ * DEFAULT uses default zone for given datapath. */
+enum ovnact_ct_zone {
+OVNACT_CT_ZONE_DEFAULT,
+OVNACT_CT_ZONE_SNAT,
+OVNACT_CT_ZONE_DNAT,
+};
+
 /* OVNACT_CT_COMMIT_V1. */
 struct ovnact_ct_commit_v1 {
 struct ovnact ovnact;
 uint32_t ct_mark, ct_mark_mask;
 ovs_be128 ct_label, ct_label_mask;
+enum ovnact_ct_zone zone;
 };
 
 /* Type of NAT used for the particular action.
diff --git a/lib/actions.c b/lib/actions.c
index a45874dfb..319e65b6f 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -707,6 +707,7 @@ static void
 parse_ct_commit_v1_arg(struct action_context *ctx,
struct ovnact_ct_commit_v1 *cc)
 {
+cc->zone = OVNACT_CT_ZONE_DEFAULT;
 if (lexer_match_id(ctx->lexer, "ct_mark")) {
 if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
 return;
@@ -737,6 +738,10 @@ parse_ct_commit_v1_arg(struct action_context *ctx,
 return;
 }
 lexer_get(ctx->lexer);
+} else if (lexer_match_id(ctx->lexer, "snat")) {
+cc->zone = OVNACT_CT_ZONE_SNAT;
+} else if (lexer_match_id(ctx->lexer, "dnat")) {
+cc->zone = OVNACT_CT_ZONE_DNAT;
 } else {
 lexer_syntax_error(ctx->lexer, NULL);
 }
@@ -814,7 +819,20 @@ encode_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc,
 struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
 ct->flags = NX_CT_F_COMMIT;
 ct->recirc_table = NX_CT_RECIRC_NONE;
-ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
+switch (cc->zone) {
+case OVNACT_CT_ZONE_SNAT:
+ct->zone_src.field = mf_from_id(MFF_LOG_SNAT_ZONE);
+break;
+
+case OVNACT_CT_ZONE_DNAT:
+ct->zone_src.field = mf_from_id(MFF_LOG_DNAT_ZONE);
+break;
+
+case OVNACT_CT_ZONE_DEFAULT:
+default:
+ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
+break;
+}
 ct->zone_src.ofs = 0;
 ct->zone_src.n_bits = 16;
 
diff --git a/ovn-sb.xml b/ovn-sb.xml
index ac4e585f2..66cb9747d 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -1405,6 +1405,8 @@
 ct_commit { ct_mark=value[/mask]; };
 ct_commit { ct_label=value[/mask]; };
 ct_commit { ct_mark=value[/mask]; 
ct_label=value[/mask]; };
+ct_commit(snat);
+ct_commit(dnat);
 
   
 Commit the flow to the connection tracking entry associated with it
@@ -1421,6 +1423,13 @@
 in order to have specific bits set.
   
 
+  
+Parameters ct_commit(snat) or ct_commit(dnat)
+ can be used to explicitly specify into which zone should be
+connection committed. Without this parameter, the connection is
+committed to the default zone for the Datapath.
+  
+
   
 Note that if you want processing to continue in the next table,
 you must execute the next action after
-- 
2.40.1

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

Re: [ovs-dev] [RFC ovn] northd.c: Fix direct access to SNATed networks.

[Martin Kalcok]

> On 8 Feb 2024, at 11:12, martin.kal...@canonical.com wrote:
> 
> On Thu, 2024-02-08 at 09:30 +0100, Ales Musil wrote:
>> 
>> 
>> On Thu, Feb 8, 2024 at 1:22 AM Mark Michelson 
>> wrote:
>>> Hi Martin
>>> 
>>> Thanks a bunch for your first OVN contribution. I have comments
>>> below.
>>> 
>>> On 2/7/24 10:56, Martin Kalcok wrote:
>>>> When a machine from an external network tries to access machine
>>>> in the
>>>> OVN's internal network with SNAT enabled, using protocol like
>>>> TCP, it
>>>> receives connection reset after first packets are successfully
>>>> exchanged.
>>>> 
>>>> This setup may be a bit unusual and requires that whatever is
>>>> responsible
>>>> for routing in the external network, routes packets for OVN's
>>>> internal
>>>> network via LR's external port IP. However it is something that
>>>> used to
>>>> work in ML2/OVS Openstack deployments and got broken after
>>>> upgrade to
>>>> ML2/OVN.
>>>> 
>>>> To demonstrate what the traffic looked like from the point of
>>>> view of the
>>>> external machine, lets say we have:
>>>> * E  (IP of machine in the external network)
>>>> * I  (IP of machine in OVN's internal network)
>>>> * LR (IP of OVN's Logical Router Port connected to external
>>>> network)
>>>> 
>>>> The traffic looks like this:
>>>> * [SYN]  E  -> I
>>>> * [SYN,ACK]  I  -> E
>>>> * [ACK]  E  -> I
>>>> * [PSH,ACK]  E  -> I
>>>> * [ACK]  LR -> E
>>>> 
>>>> Connection gets reseted after it receives ACK from an unexpected
>>>> IP (LR).
>>>> 
>>>> Although I'm not completely sure why the first [SYN,ACK] reply
>>>> got back
>>>> without SNAT, root cause of this issue is that traffic initiated
>>>> from
>>>> the external network is not tracked in CT. This causes even
>>>> traffic that's
>>>> in the "reply" direction (i.e. it was initiated from outside the
>>>> SNAT
>>>> network), to be translated by Logical Router.
>>>> 
>>>> My proposal is to initiate CT and commit "new" connections
>>>> destined
>>>> to the internal network (with enabled SNAT) on Logical Router.
>>>> This
>>>> will enable SNAT rules to translate only traffic that originates
>>>> from
>>>> internal networks.
>>>> 
>>>> Signed-off-by: Martin Kalcok 
>>>> ---
>>>>northd/northd.c | 70
>>>> ++---
>>>>1 file changed, 61 insertions(+), 9 deletions(-)
>>>> 
>>>> As this would be my first "real" contribution to the OVN, I'd
>>>> love to hear
>>>> some feeback on my approach while I work on performance
>>>> measurments and
>>>> tests, which I plan on including in final proposal.
>>>> I also left some inline XXX comments marking parts that I'm not
>>>> sure about
>>>> and need resolving before final proposal.
>>>> I tested this change in my local Openstack deployment with
>>>> distributed gw
>>>> chassis and it seems to solve the issue regardless of the VM's
>>>> placement,
>>>> whether it does or does not have a floating IP. It also doesn't
>>>> seem to
>>>> break outbound connectivity, nor connectivity to VM's floating
>>>> IPs.
>>>> Thank you, for any reviews/sugestions.
>>> 
>>> Without getting into the details of this particular patch, I have
>>> some 
>>> general suggestions for contributing:
>>> 
>>> 1) Before sending a patch, run utilities/checkpatch.py against all 
>>> patches in the series. This can help 0-day Robot not to complain
>>> about 
>>> coding guidelines violations.
>>> 
>>> 2) Run the testsuite locally before posting. If you see test
>>> errors, 
>>> double check if your change is what's causing them.
> 
> Sorry about the failing tests. I ran the unit tests a checked the
> errors (that were caused by me) but given that I intended this as an
> RFC and I wasn't completely sure about my approach I didn't want to
> spend too much effort on fixing/adding uni

Re: [ovs-dev] [RFC ovn] northd.c: Fix direct access to SNATed networks.

[martin . kalcok]

On Thu, 2024-02-08 at 09:30 +0100, Ales Musil wrote:
> 
> 
> On Thu, Feb 8, 2024 at 1:22 AM Mark Michelson 
> wrote:
> > Hi Martin
> > 
> > Thanks a bunch for your first OVN contribution. I have comments
> > below.
> > 
> > On 2/7/24 10:56, Martin Kalcok wrote:
> > > When a machine from an external network tries to access machine
> > > in the
> > > OVN's internal network with SNAT enabled, using protocol like
> > > TCP, it
> > > receives connection reset after first packets are successfully
> > > exchanged.
> > > 
> > > This setup may be a bit unusual and requires that whatever is
> > > responsible
> > > for routing in the external network, routes packets for OVN's
> > > internal
> > > network via LR's external port IP. However it is something that
> > > used to
> > > work in ML2/OVS Openstack deployments and got broken after
> > > upgrade to
> > > ML2/OVN.
> > > 
> > > To demonstrate what the traffic looked like from the point of
> > > view of the
> > > external machine, lets say we have:
> > >     * E  (IP of machine in the external network)
> > >     * I  (IP of machine in OVN's internal network)
> > >     * LR (IP of OVN's Logical Router Port connected to external
> > > network)
> > > 
> > > The traffic looks like this:
> > >     * [SYN]      E  -> I
> > >     * [SYN,ACK]  I  -> E
> > >     * [ACK]      E  -> I
> > >     * [PSH,ACK]  E  -> I
> > >     * [ACK]      LR -> E
> > > 
> > > Connection gets reseted after it receives ACK from an unexpected
> > > IP (LR).
> > > 
> > > Although I'm not completely sure why the first [SYN,ACK] reply
> > > got back
> > > without SNAT, root cause of this issue is that traffic initiated
> > > from
> > > the external network is not tracked in CT. This causes even
> > > traffic that's
> > > in the "reply" direction (i.e. it was initiated from outside the
> > > SNAT
> > > network), to be translated by Logical Router.
> > > 
> > > My proposal is to initiate CT and commit "new" connections
> > > destined
> > > to the internal network (with enabled SNAT) on Logical Router.
> > > This
> > > will enable SNAT rules to translate only traffic that originates
> > > from
> > > internal networks.
> > > 
> > > Signed-off-by: Martin Kalcok 
> > > ---
> > >    northd/northd.c | 70
> > > ++---
> > >    1 file changed, 61 insertions(+), 9 deletions(-)
> > > 
> > > As this would be my first "real" contribution to the OVN, I'd
> > > love to hear
> > > some feeback on my approach while I work on performance
> > > measurments and
> > > tests, which I plan on including in final proposal.
> > > I also left some inline XXX comments marking parts that I'm not
> > > sure about
> > > and need resolving before final proposal.
> > > I tested this change in my local Openstack deployment with
> > > distributed gw
> > > chassis and it seems to solve the issue regardless of the VM's
> > > placement,
> > > whether it does or does not have a floating IP. It also doesn't
> > > seem to
> > > break outbound connectivity, nor connectivity to VM's floating
> > > IPs.
> > > Thank you, for any reviews/sugestions.
> > 
> > Without getting into the details of this particular patch, I have
> > some 
> > general suggestions for contributing:
> > 
> > 1) Before sending a patch, run utilities/checkpatch.py against all 
> > patches in the series. This can help 0-day Robot not to complain
> > about 
> > coding guidelines violations.
> > 
> > 2) Run the testsuite locally before posting. If you see test
> > errors, 
> > double check if your change is what's causing them.

Sorry about the failing tests. I ran the unit tests a checked the
errors (that were caused by me) but given that I intended this as an
RFC and I wasn't completely sure about my approach I didn't want to
spend too much effort on fixing/adding unit tests that might get
scraped. In the hindsight, I should've put more effort in and post a
clean work for the review. 

> > 
> > 3) As a sanity check, consider pushing the code to your github
> > fork, and 
> > ensure that github actions complete without error. If there are any
> > errors, 

[ovs-dev] [RFC ovn] northd.c: Fix direct access to SNATed networks.

[Martin Kalcok]

When a machine from an external network tries to access machine in the
OVN's internal network with SNAT enabled, using protocol like TCP, it
receives connection reset after first packets are successfully exchanged.

This setup may be a bit unusual and requires that whatever is responsible
for routing in the external network, routes packets for OVN's internal
network via LR's external port IP. However it is something that used to
work in ML2/OVS Openstack deployments and got broken after upgrade to
ML2/OVN.

To demonstrate what the traffic looked like from the point of view of the
external machine, lets say we have:
  * E  (IP of machine in the external network)
  * I  (IP of machine in OVN's internal network)
  * LR (IP of OVN's Logical Router Port connected to external network)

The traffic looks like this:
  * [SYN]  E  -> I
  * [SYN,ACK]  I  -> E
  * [ACK]  E  -> I
  * [PSH,ACK]  E  -> I
  * [ACK]  LR -> E

Connection gets reseted after it receives ACK from an unexpected IP (LR).

Although I'm not completely sure why the first [SYN,ACK] reply got back
without SNAT, root cause of this issue is that traffic initiated from
the external network is not tracked in CT. This causes even traffic that's
in the "reply" direction (i.e. it was initiated from outside the SNAT
network), to be translated by Logical Router.

My proposal is to initiate CT and commit "new" connections destined
to the internal network (with enabled SNAT) on Logical Router. This
will enable SNAT rules to translate only traffic that originates from
internal networks.

Signed-off-by: Martin Kalcok 
---
 northd/northd.c | 70 ++---
 1 file changed, 61 insertions(+), 9 deletions(-)

As this would be my first "real" contribution to the OVN, I'd love to hear
some feeback on my approach while I work on performance measurments and
tests, which I plan on including in final proposal.
I also left some inline XXX comments marking parts that I'm not sure about
and need resolving before final proposal.
I tested this change in my local Openstack deployment with distributed gw
chassis and it seems to solve the issue regardless of the VM's placement,
whether it does or does not have a floating IP. It also doesn't seem to
break outbound connectivity, nor connectivity to VM's floating IPs.
Thank you, for any reviews/sugestions.

diff --git a/northd/northd.c b/northd/northd.c
index 01eec64ca..b695932fd 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -10772,6 +10772,12 @@ build_ecmp_route_flow(struct lflow_table *lflows, 
struct ovn_datapath *od,
 ds_destroy();
 }
 
+static inline bool
+lrouter_use_common_zone(const struct ovn_datapath *od)
+{
+return !od->is_gw_router && use_common_zone;
+}
+
 static void
 add_route(struct lflow_table *lflows, struct ovn_datapath *od,
   const struct ovn_port *op, const char *lrp_addr_s,
@@ -10796,6 +10802,9 @@ add_route(struct lflow_table *lflows, struct 
ovn_datapath *od,
 build_route_match(op_inport, rtb_id, network_s, plen, is_src_route,
   is_ipv4, , , ofs);
 
+bool use_ct = false;
+struct ds target_net = DS_EMPTY_INITIALIZER;
+struct ds snat_port_match = DS_EMPTY_INITIALIZER;
 struct ds common_actions = DS_EMPTY_INITIALIZER;
 struct ds actions = DS_EMPTY_INITIALIZER;
 if (is_discard_route) {
@@ -10808,16 +10817,61 @@ add_route(struct lflow_table *lflows, struct 
ovn_datapath *od,
 } else {
 ds_put_format(_actions, "ip%s.dst", is_ipv4 ? "4" : "6");
 }
+
+/* To enable direct connectivity from external networks to Logical IPs
+ * of VMs/Containers in the SNATed networks, we need to track
+ * connections initiated from external networks.  This allows SNAT
+ * rules to avoid SNATing packets in "reply" direction. */
+if (od->nbr && od->nbr->n_nat) {
+ds_put_format(_net, "%s/%d", network_s, plen);
+for (int i = 0; i < od->nbr->n_nat; i++) {
+const struct nbrec_nat *nat = od->nbr->nat[i];
+if (strcmp(nat->type, "snat") ||
+strcmp(nat->logical_ip, ds_cstr(_net)) ||
+lrouter_use_common_zone(od)) {
+continue;
+}
+
+/* Initiate connection tracking for traffic destined to
+ * SNATed network. */
+use_ct = true;
+
+/* XXX: Need to figure out what is appropraite priority for 
these rules.
+All they require is to be after any existing specific 
rules
+and before the default rule.*/
+
+/* Commit new connections initiated from the outside of
+ * SNATed network to CT. */
+ds_put_format(_port_match,
+

[ovs-dev] [PATCH ovn v3] ovn-ctl: Add option to skip schema conversion

[Martin Kalcok]

ovn-ctl script currently automatically attempts to perform clustered
database schema upgrade when starting OVN SB or NB clustered
database. To provide more control over this process a
`--db-cluster-schema-upgrade` option is added.

Default value for this option is `yes`, to preserve current default
behavior.

To start database without performing schema conversion, user can
provide either `--db-cluster-schema-upgrade=no` option or
`--no-db-cluster-schema-upgrade` flag to the ovn-ctl script.

Signed-off-by: Martin Kalcok 
---
 utilities/ovn-ctl   |  7 ++-
 utilities/ovn-ctl.8.xml | 21 +
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
index 876565c80..50d588358 100755
--- a/utilities/ovn-ctl
+++ b/utilities/ovn-ctl
@@ -184,6 +184,7 @@ start_ovsdb__() {
 local ovn_db_ssl_cacert
 local ovn_db_election_timer
 local relay_mode
+local cluster_db_upgrade
 eval db_pid_file=\$DB_${DB}_PIDFILE
 eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
 eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
@@ -212,6 +213,7 @@ start_ovsdb__() {
 eval ovn_db_election_timer=\$DB_${DB}_ELECTION_TIMER
 eval relay_mode=\$RELAY_MODE
 eval relay_remote=\$DB_${DB}_REMOTE
+eval cluster_db_upgrade=\$DB_CLUSTER_SCHEMA_UPGRADE
 
 ovn_install_dir "$OVN_RUNDIR"
 ovn_install_dir "$ovn_logdir"
@@ -347,7 +349,7 @@ $cluster_remote_port
 $(echo ovn-${db}ctl | tr _ -) --no-leader-only --db="unix:$sock" init
 fi
 
-if test $mode = cluster; then
+if test $mode = cluster && test X"$cluster_db_upgrade" = Xyes; then
 upgrade_cluster "$schema" "unix:$sock"
 fi
 
@@ -898,6 +900,8 @@ set_defaults () {
 OVN_SB_RELAY_DB_SSL_CERT=""
 OVN_SB_RELAY_DB_SSL_CA_CERT=""
 DB_SB_RELAY_USE_REMOTE_IN_DB="yes"
+
+DB_CLUSTER_SCHEMA_UPGRADE="yes"
 }
 
 set_option () {
@@ -1148,6 +1152,7 @@ File location options:
   --ovn-sb-relay-db-ssl-key=KEY OVN_Southbound DB relay SSL private key file
   --ovn-sb-relay-db-ssl-cert=CERT OVN_Southbound DB relay SSL certificate file
   --ovn-sb-relay-db-ssl-ca-cert=CERT OVN OVN_Southbound DB relay SSL CA 
certificate file
+  --db-cluster-schema-upgrade=yes|no (default: $DB_CLUSTER_SCHEMA_UPGRADE)
 
 Default directories with "configure" option and environment variable override:
   logs: /usr/local/var/log/ovn (--with-logdir, OVN_LOGDIR)
diff --git a/utilities/ovn-ctl.8.xml b/utilities/ovn-ctl.8.xml
index 01f4aa26b..3bab055e4 100644
--- a/utilities/ovn-ctl.8.xml
+++ b/utilities/ovn-ctl.8.xml
@@ -156,6 +156,7 @@
 --db-ic-sb-cluster-remote-addr=IP ADDRESS
 --db-ic-sb-cluster-remote-port=PORT NUMBER
 --db-ic-sb-cluster-remote-proto=PROTO 
(tcp/ssl)
+--db-cluster-schema-upgrade=yes|no
 
  Probe interval options 
 --db-nb-probe-interval-to-active=Time in 
milliseconds
@@ -324,4 +325,24 @@
start_northd
   
 
+Avoiding automatic clustered OVN database schema upgrade
+
+  If you desire more control over clustered DB schema upgrade, you can
+  opt-out of automatic on-start upgrade attempts with
+  --no-db-cluster-schema-upgrade.
+
+Start OVN NB and SB clustered databases on host with IP x.x.x.x 
without schema upgrade
+
+  
+# ovn-ctl start_nb_ovsdb --db-nb-cluster-local-addr=x.x.x.x 
--no-db-cluster-schema-upgrade
+# ovn-ctl start_sb_ovsdb --db-sb-cluster-local-addr=x.x.x.x 
--no-db-cluster-schema-upgrade
+  
+
+Trigger clustered DB schema upgrade manually
+
+  
+# ovsdb-client convert unix:/var/run/ovn/ovnnb_db.sock 
/usr/local/share/ovn/ovn-nb.ovsschema
+# ovsdb-client convert unix:/var/run/ovn/ovnsb_db.sock 
/usr/local/share/ovn/ovn-sb.ovsschema
+  
+
 
-- 
2.40.1

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

[ovs-dev] [PATCH ovn] ovn-ctl: Add option to skip schema conversion

[Martin Kalcok]

ovn-ctl script currently automatically attempts to perform clustered
database schema upgrade when starting OVN SB or NB clustered
database. To provide more controll over this procees a
`--db-cluster-schema-upgrade` option is added.

Default value for this option is `yes`, to preserve current default
behavior.

To start database without performing schema conversion, user can
provide either `--db-cluster-schema-upgrade=no` option or
`--no-db-cluster-schema-upgrade` flag to the ovn-ctl script.

Signed-off-by: Martin Kalcok 
---
 utilities/ovn-ctl   |  7 ++-
 utilities/ovn-ctl.8.xml | 21 +
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
index 876565c80..50d588358 100755
--- a/utilities/ovn-ctl
+++ b/utilities/ovn-ctl
@@ -184,6 +184,7 @@ start_ovsdb__() {
 local ovn_db_ssl_cacert
 local ovn_db_election_timer
 local relay_mode
+local cluster_db_upgrade
 eval db_pid_file=\$DB_${DB}_PIDFILE
 eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
 eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
@@ -212,6 +213,7 @@ start_ovsdb__() {
 eval ovn_db_election_timer=\$DB_${DB}_ELECTION_TIMER
 eval relay_mode=\$RELAY_MODE
 eval relay_remote=\$DB_${DB}_REMOTE
+eval cluster_db_upgrade=\$DB_CLUSTER_SCHEMA_UPGRADE
 
 ovn_install_dir "$OVN_RUNDIR"
 ovn_install_dir "$ovn_logdir"
@@ -347,7 +349,7 @@ $cluster_remote_port
 $(echo ovn-${db}ctl | tr _ -) --no-leader-only --db="unix:$sock" init
 fi
 
-if test $mode = cluster; then
+if test $mode = cluster && test X"$cluster_db_upgrade" = Xyes; then
 upgrade_cluster "$schema" "unix:$sock"
 fi
 
@@ -898,6 +900,8 @@ set_defaults () {
 OVN_SB_RELAY_DB_SSL_CERT=""
 OVN_SB_RELAY_DB_SSL_CA_CERT=""
 DB_SB_RELAY_USE_REMOTE_IN_DB="yes"
+
+DB_CLUSTER_SCHEMA_UPGRADE="yes"
 }
 
 set_option () {
@@ -1148,6 +1152,7 @@ File location options:
   --ovn-sb-relay-db-ssl-key=KEY OVN_Southbound DB relay SSL private key file
   --ovn-sb-relay-db-ssl-cert=CERT OVN_Southbound DB relay SSL certificate file
   --ovn-sb-relay-db-ssl-ca-cert=CERT OVN OVN_Southbound DB relay SSL CA 
certificate file
+  --db-cluster-schema-upgrade=yes|no (default: $DB_CLUSTER_SCHEMA_UPGRADE)
 
 Default directories with "configure" option and environment variable override:
   logs: /usr/local/var/log/ovn (--with-logdir, OVN_LOGDIR)
diff --git a/utilities/ovn-ctl.8.xml b/utilities/ovn-ctl.8.xml
index 01f4aa26b..4e3d85a52 100644
--- a/utilities/ovn-ctl.8.xml
+++ b/utilities/ovn-ctl.8.xml
@@ -156,6 +156,7 @@
 --db-ic-sb-cluster-remote-addr=IP ADDRESS
 --db-ic-sb-cluster-remote-port=PORT NUMBER
 --db-ic-sb-cluster-remote-proto=PROTO 
(tcp/ssl)
+--db-cluster-skip-upgrade=yes|no
 
  Probe interval options 
 --db-nb-probe-interval-to-active=Time in 
milliseconds
@@ -324,4 +325,24 @@
start_northd
   
 
+Avoiding automatic clustered OVN database schema upgrade
+
+  If you desire more control over clustered DB schema upgrade, you can
+  opt-out of automatic on-start upgrade attempts with
+  --no-db-cluster-schema-upgrade.
+
+Start OVN NB and SB clustered databases on host with IP x.x.x.x 
without schema upgrade
+
+  
+# ovn-ctl start_nb_ovsdb --db-nb-cluster-local-addr=x.x.x.x 
--no-db-cluster-schema-upgrade
+# ovn-ctl start_sb_ovsdb --db-sb-cluster-local-addr=x.x.x.x 
--no-db-cluster-schema-upgrade
+  
+
+Trigger clustered DB schema upgrade manually
+
+  
+# ovsdb-client convert unix:/var/run/ovn/ovnnb_db.sock 
/usr/local/share/ovn/ovn-nb.ovsschema
+# ovsdb-client convert unix:/var/run/ovn/ovnsb_db.sock 
/usr/local/share/ovn/ovn-sb.ovsschema
+  
+
 
-- 
2.40.1

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

Re: [ovs-dev] [PATCH ovn] ovn-ctl: Add option to skip schema conversion

[martin . kalcok]

On Tue, 2024-01-09 at 08:23 +0100, Frode Nordahl wrote:
> Hello, Martin,
> 
> Thank you for working on this!
> 
> While the automatic conversion is nice when it works, it is a source
> of problems when it does not, so I welcome this change.
> 
> An additional argument for this change is that the existing approach
> executes asynchronously, so there is no way for the human or machine
> operator to get the result of the `ovsdb-client` call.
> 
> On Mon, Jan 8, 2024 at 10:15 AM Martin Kalcok
>  wrote:
> > 
> > ovn-ctl script currently automatically attempts to perform
> > clustered
> > database schema upgrade when starting OVN SB or NB clustered
> > database. To provide more controll over this procees a
> 
> nit: a couple of typos above.

Thanks I'll fix these.

> 
> > `--db-cluster-skip-upgrade` option is added that allows skipping
> > the upgrade.
> 
> There is a precedent in the `ovn-ctl` and `ovs-ctl` scripts to use
> options in the form of `--no-something` for optional negation of some
> feature (`--no-leader-only` `--no-record-hostname` etc).
> 
> What do you think about making your new option conforming with this?

Yup, make's more sense. I'll flip the wording to fit '--flag'/'--no-
flag' pattern.
> 
> > Default value for this option is `no`, to preserve current default
> > behavior.
> > 
> > Signed-off-by: Martin Kalcok 
> > ---
> >  utilities/ovn-ctl   |  7 ++-
> >  utilities/ovn-ctl.8.xml | 11 +++
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
> > index 876565c80..011ef9c0c 100755
> > --- a/utilities/ovn-ctl
> > +++ b/utilities/ovn-ctl
> > @@ -184,6 +184,7 @@ start_ovsdb__() {
> >  local ovn_db_ssl_cacert
> >  local ovn_db_election_timer
> >  local relay_mode
> > +    local skip_cluster_db_upgrade
> >  eval db_pid_file=\$DB_${DB}_PIDFILE
> >  eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
> >  eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
> > @@ -212,6 +213,7 @@ start_ovsdb__() {
> >  eval ovn_db_election_timer=\$DB_${DB}_ELECTION_TIMER
> >  eval relay_mode=\$RELAY_MODE
> >  eval relay_remote=\$DB_${DB}_REMOTE
> > +    eval skip_cluster_db_upgrade=\$DB_CLUSTER_SKIP_UPGRADE
> > 
> >  ovn_install_dir "$OVN_RUNDIR"
> >  ovn_install_dir "$ovn_logdir"
> > @@ -347,7 +349,7 @@ $cluster_remote_port
> >  $(echo ovn-${db}ctl | tr _ -) --no-leader-only --
> > db="unix:$sock" init
> >  fi
> > 
> > -    if test $mode = cluster; then
> > +    if test $mode = cluster && test X"$skip_cluster_db_upgrade" =
> > Xno; then
> >  upgrade_cluster "$schema" "unix:$sock"
> >  fi
> > 
> > @@ -898,6 +900,8 @@ set_defaults () {
> >  OVN_SB_RELAY_DB_SSL_CERT=""
> >  OVN_SB_RELAY_DB_SSL_CA_CERT=""
> >  DB_SB_RELAY_USE_REMOTE_IN_DB="yes"
> > +
> > +    DB_CLUSTER_SKIP_UPGRADE="no"
> >  }
> > 
> >  set_option () {
> > @@ -1148,6 +1152,7 @@ File location options:
> >    --ovn-sb-relay-db-ssl-key=KEY OVN_Southbound DB relay SSL
> > private key file
> >    --ovn-sb-relay-db-ssl-cert=CERT OVN_Southbound DB relay SSL
> > certificate file
> >    --ovn-sb-relay-db-ssl-ca-cert=CERT OVN OVN_Southbound DB relay
> > SSL CA certificate file
> > +  --db-cluster-skip-upgrade=yes|no (default:
> > $DB_CLUSTER_SKIP_UPGRADE)
> > 
> >  Default directories with "configure" option and environment
> > variable override:
> >    logs: /usr/local/var/log/ovn (--with-logdir, OVN_LOGDIR)
> > diff --git a/utilities/ovn-ctl.8.xml b/utilities/ovn-ctl.8.xml
> > index 01f4aa26b..5762fd3a4 100644
> > --- a/utilities/ovn-ctl.8.xml
> > +++ b/utilities/ovn-ctl.8.xml
> > @@ -156,6 +156,7 @@
> >  --db-ic-sb-cluster-remote-addr=IP
> > ADDRESS
> >  --db-ic-sb-cluster-remote-port=PORT
> > NUMBER
> >  --db-ic-sb-cluster-remote-proto=PROTO
> > (tcp/ssl)
> > +    --db-cluster-skip-
> > upgrade=yes|no
> > 
> >   Probe interval options 
> >  --db-nb-probe-interval-to-active=Time in
> > milliseconds
> > @@ -324,4 +325,14 @@
> >     start_northd
> >    
> >  
> > +    Avoiding automatic clustered OVN database schema
> > upgrade
> > +    
> > +  If you desire more controll over clustered DB schema
> > upgrade, you can
&

[ovs-dev] [PATCH ovn] ovn-ctl: Add option to skip schema conversion

[Martin Kalcok]

ovn-ctl script currently automatically attempts to perform clustered
database schema upgrade when starting OVN SB or NB clustered
database. To provide more controll over this procees a
`--db-cluster-skip-upgrade` option is added that allows skipping
the upgrade.

Default value for this option is `no`, to preserve current default
behavior.

Signed-off-by: Martin Kalcok 
---
 utilities/ovn-ctl   |  7 ++-
 utilities/ovn-ctl.8.xml | 11 +++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/utilities/ovn-ctl b/utilities/ovn-ctl
index 876565c80..011ef9c0c 100755
--- a/utilities/ovn-ctl
+++ b/utilities/ovn-ctl
@@ -184,6 +184,7 @@ start_ovsdb__() {
 local ovn_db_ssl_cacert
 local ovn_db_election_timer
 local relay_mode
+local skip_cluster_db_upgrade
 eval db_pid_file=\$DB_${DB}_PIDFILE
 eval cluster_local_addr=\$DB_${DB}_CLUSTER_LOCAL_ADDR
 eval cluster_local_port=\$DB_${DB}_CLUSTER_LOCAL_PORT
@@ -212,6 +213,7 @@ start_ovsdb__() {
 eval ovn_db_election_timer=\$DB_${DB}_ELECTION_TIMER
 eval relay_mode=\$RELAY_MODE
 eval relay_remote=\$DB_${DB}_REMOTE
+eval skip_cluster_db_upgrade=\$DB_CLUSTER_SKIP_UPGRADE
 
 ovn_install_dir "$OVN_RUNDIR"
 ovn_install_dir "$ovn_logdir"
@@ -347,7 +349,7 @@ $cluster_remote_port
 $(echo ovn-${db}ctl | tr _ -) --no-leader-only --db="unix:$sock" init
 fi
 
-if test $mode = cluster; then
+if test $mode = cluster && test X"$skip_cluster_db_upgrade" = Xno; then
 upgrade_cluster "$schema" "unix:$sock"
 fi
 
@@ -898,6 +900,8 @@ set_defaults () {
 OVN_SB_RELAY_DB_SSL_CERT=""
 OVN_SB_RELAY_DB_SSL_CA_CERT=""
 DB_SB_RELAY_USE_REMOTE_IN_DB="yes"
+
+DB_CLUSTER_SKIP_UPGRADE="no"
 }
 
 set_option () {
@@ -1148,6 +1152,7 @@ File location options:
   --ovn-sb-relay-db-ssl-key=KEY OVN_Southbound DB relay SSL private key file
   --ovn-sb-relay-db-ssl-cert=CERT OVN_Southbound DB relay SSL certificate file
   --ovn-sb-relay-db-ssl-ca-cert=CERT OVN OVN_Southbound DB relay SSL CA 
certificate file
+  --db-cluster-skip-upgrade=yes|no (default: $DB_CLUSTER_SKIP_UPGRADE)
 
 Default directories with "configure" option and environment variable override:
   logs: /usr/local/var/log/ovn (--with-logdir, OVN_LOGDIR)
diff --git a/utilities/ovn-ctl.8.xml b/utilities/ovn-ctl.8.xml
index 01f4aa26b..5762fd3a4 100644
--- a/utilities/ovn-ctl.8.xml
+++ b/utilities/ovn-ctl.8.xml
@@ -156,6 +156,7 @@
 --db-ic-sb-cluster-remote-addr=IP ADDRESS
 --db-ic-sb-cluster-remote-port=PORT NUMBER
 --db-ic-sb-cluster-remote-proto=PROTO 
(tcp/ssl)
+--db-cluster-skip-upgrade=yes|no
 
  Probe interval options 
 --db-nb-probe-interval-to-active=Time in 
milliseconds
@@ -324,4 +325,14 @@
start_northd
   
 
+Avoiding automatic clustered OVN database schema upgrade
+
+  If you desire more controll over clustered DB schema upgrade, you can
+  opt-out of automatic on-start upgrade attempts with
+  --db-cluster-skip-upgrade.
+
+Start OVN NB (or SB) clustered database on host with IP x.x.x.x 
without schema upgrade
+# ovn-ctl start_nb_ovsdb --db-nb-cluster-local-addr=x.x.x.x 
--db-cluster-skip-upgrade=yes
+Trigger clustered DB schema upgrade manually
+# ovsdb-client -t 30 convert unix:/var/run/ovn/ovnnb_db.sock 
/usr/local/share/ovn/ovn-nb.ovsschema
 
-- 
2.40.1

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

[ovs-dev] [PATCH ovn] ovn-ctl man: Add election timer config to manpage

[martin . kalcok]

Raft election timers for NB and SB DB clusters are configurable via
ovn-ctl but the options are not mention on the man page.

Signed-off-by: Martin Kalcok 
---
 utilities/ovn-ctl.8.xml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/utilities/ovn-ctl.8.xml b/utilities/ovn-ctl.8.xml
index 82804096f..01f4aa26b 100644
--- a/utilities/ovn-ctl.8.xml
+++ b/utilities/ovn-ctl.8.xml
@@ -136,12 +136,14 @@
 --db-nb-cluster-remote-addr=IP ADDRESS
 --db-nb-cluster-remote-port=PORT NUMBER
 --db-nb-cluster-remote-proto=PROTO (tcp/ssl)
+    --db-nb-election-timer=Timeout in 
milliseconds
 --db-sb-cluster-local-addr=IP ADDRESS
 --db-sb-cluster-local-port=PORT NUMBER
 --db-sb-cluster-local-proto=PROTO (tcp/ssl)
 --db-sb-cluster-remote-addr=IP ADDRESS
 --db-sb-cluster-remote-port=PORT NUMBER
 --db-sb-cluster-remote-proto=PROTO (tcp/ssl)
+    --db-sb-election-timer=Timeout in 
milliseconds
 --db-ic-nb-cluster-local-addr=IP ADDRESS
 --db-ic-nb-cluster-local-port=PORT NUMBER
 --db-ic-nb-cluster-local-proto=PROTO 
(tcp/ssl)
-- 
2.39.2


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