On 11/18/25 4:50 AM, Xie Liu wrote:
> Consider the case of stateful Firewall for N-S traffic:
>
> PUBLIC---S1-(S1-R1)---------(R1-S1)-R1 -------- S2 ---- VM1
>
> Configuration:
>
> ovn-nbctl pg-add pg_dgw
> ovn-nbctl pg-set-ports pg_dgw S1-R1
> ovn-nbctl acl-add pg_dgw from-lport 2000 "inport == @pg_dgw && ip4 && icmp4"
> allow-related
> ovn-nbctl acl-add pg_dgw from-lport 1000 "inport == @pg_dgw && ip4" drop
> ovn-nbctl acl-add pg_dgw to-lport 1000 "outport == @pg_dgw && ip4" drop
> ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 enable_router_port_acl=true
>
> VM1 pings external network.
>
> Through this patch[1], the ovn-controller assigned a CT zone ID
> to the localnet LSP but not the dgw LSP.
>
> This caused ACL failures: ICMP reply packets from external networks
> performed CT lookups in the wrong zone, couldn't match established
> connections, and were incorrectly dropped.
>
> This commit enables CT zone allocation for patch ports that correspond
> to router gateway ports when enable_router_port_acl=true is set.
>
> Changes:
> - northd: Add enable-router-port-acl option to southbound port binding
> - binding: Handle patch port CT zone requirements in local_lports
> - controller: Add/Delete CT zone for patch ports enabled/disabled ACL
>
> [1]https://github.com/ovn-org/ovn/commit/5ae7d2cb60a50541e88e8f5c74a669e2aa7acdda
>
> Reported-at: https://github.com/ovn-org/ovn/issues/264
> Signed-off-by: Xie Liu <[email protected]>
> ---
Hi Xie Liu,
Thanks for this new version and sorry for the delay in reviewing it.
> controller/binding.c | 31 ++++++++
> controller/ovn-controller.c | 23 +++---
> controller/physical.c | 4 +-
> northd/northd.c | 5 ++
> tests/system-ovn.at | 145 ++++++++++++++++++++++++++++++++++++
> 5 files changed, 197 insertions(+), 11 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index b429323a9..6e138f1df 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -791,6 +791,16 @@ update_active_pb_ras_pd(const struct sbrec_port_binding
> *pb,
> }
> }
>
> +/* Check the patch port needs CT zone based on enable-router-port-acl.
> + * Returns true if the patch port should be added to local_lports for
> + * CT zone allocation.
> + */
> +static bool
> +patch_port_needs_ct_zone(const struct sbrec_port_binding *pb)
> +{
> + return smap_get_bool(&pb->options, "enable-router-port-acl", false);
To avoid confusion I'd use the same option name as we have in the
northbound database, enable_router_port_acl.
> +}
> +
> static bool is_ext_id_changed(const struct smap *a, const struct smap *b,
> const char *key);
> static struct local_binding *local_binding_create(
> @@ -2259,6 +2269,12 @@ binding_run(struct binding_ctx_in *b_ctx_in, struct
> binding_ctx_out *b_ctx_out)
> case LP_PATCH:
> update_related_lport(pb, b_ctx_out);
> consider_patch_port_for_local_datapaths(pb, b_ctx_in, b_ctx_out);
> + /* Check if this patch port needs CT zone allocation and add it
> + * to local_lports if required. */
> + if (patch_port_needs_ct_zone(pb)) {
> + update_local_lports(pb->logical_port, b_ctx_out,
> + LPORT_STATUS_BOUND);
> + }
> break;
>
> case LP_VTEP:
> @@ -3158,6 +3174,21 @@ handle_updated_port(struct binding_ctx_in *b_ctx_in,
> update_related_lport(pb, b_ctx_out);
> handled = consider_patch_port_for_local_datapaths(pb, b_ctx_in,
> b_ctx_out);
> + /* Handle dynamic changes to enable-router-port-acl option.
> + * Check CT zone requirement if options updated. */
> + if (sbrec_port_binding_is_updated(pb,
> + SBREC_PORT_BINDING_COL_OPTIONS)) {
> + tracked_datapath_lport_add(pb, TRACKED_RESOURCE_UPDATED,
> + b_ctx_out->tracked_dp_bindings);
> + if (patch_port_needs_ct_zone(pb)) {
> + update_local_lports(pb->logical_port, b_ctx_out,
> + LPORT_STATUS_BOUND);
> + } else {
> + /* Option was removed, remove from local_lports to
> + * release CT zone */
> + remove_local_lports(pb->logical_port, b_ctx_out);
> + }
> + }
> break;
>
> case LP_VTEP:
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 14b9b9e28..b3317eb15 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2486,20 +2486,23 @@ ct_zones_runtime_data_handler(struct engine_node
> *node, void *data)
> if (strcmp(t_lport->pb->type, "")
> && strcmp(t_lport->pb->type, "localport")
> && strcmp(t_lport->pb->type, "l3gateway")
> - && strcmp(t_lport->pb->type, "localnet")) {
> + && strcmp(t_lport->pb->type, "localnet")
> + && strcmp(t_lport->pb->type, "patch")) {
> /* We allocate zone-id's only to VIF, localport, l3gateway,
> - * and localnet lports. */
> - if (sbrec_port_binding_is_updated(t_lport->pb,
> - SBREC_PORT_BINDING_COL_TYPE)) {
> - updated |=
> ct_zone_handle_port_update(&ct_zones_data->ctx,
> - t_lport->pb,
> - false, &scan_start,
> - min_ct_zone, max_ct_zone);
> - }
> -
> + * localnet and patch (enabled ACL) lports. */
> continue;
> }
>
> + /* For patch ports without ACL flag, delete the ct_zone */
> + if (!strcmp(t_lport->pb->type, "patch") &&
> + !smap_get_bool(&t_lport->pb->options,
> + "enable-router-port-acl", false)) {
> + updated |= ct_zone_handle_port_update(&ct_zones_data->ctx,
> + t_lport->pb,
> + false, &scan_start,
> + min_ct_zone, max_ct_zone);
> + continue;
> + }
> bool port_updated =
> t_lport->tracked_type == TRACKED_RESOURCE_NEW ||
> t_lport->tracked_type == TRACKED_RESOURCE_UPDATED;
> diff --git a/controller/physical.c b/controller/physical.c
> index 6ac5dcd3f..a4bb29403 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -2012,8 +2012,10 @@ consider_port_binding(const struct physical_ctx *ctx,
> ofpact_put_CT_CLEAR(ofpacts_p);
> put_load(0, MFF_LOG_DNAT_ZONE, 0, 32, ofpacts_p);
> put_load(0, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p);
> - put_load(0, MFF_LOG_CT_ZONE, 0, 16, ofpacts_p);
> struct zone_ids peer_zones = get_zone_ids(peer, ctx->ct_zones);
> + if (!peer_zones.ct) {
> + put_load(0, MFF_LOG_CT_ZONE, 0, 16, ofpacts_p);
> + }
I don't think we need this change.
> load_logical_ingress_metadata(peer, &peer_zones, ctx->n_encap_ips,
> ctx->encap_ips, ofpacts_p, false);
> put_load(0, MFF_LOG_FLAGS, 0, 32, ofpacts_p);
> diff --git a/northd/northd.c b/northd/northd.c
> index cdf12ec86..0dc631fe4 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -2856,6 +2856,11 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
> } else {
> smap_add(&new, "peer", router_port);
> }
> + /* Add enable-router-port-acl option for CT zone alloc
> + * to patch ports that need ACL processing. */
> + if (op->enable_router_port_acl) {
> + smap_add(&new, "enable-router-port-acl", "true");
> + }
> }
> if (chassis) {
> smap_add(&new, "l3gateway-chassis", chassis);
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 76f73d96e..742d7df5f 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -19283,3 +19283,148 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d
> /.*terminating with signal 15.*/d"])
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([Distributed gw port CT zone allocation])
> +CHECK_CONNTRACK()
> +
> +ovn_start
> +
> +# Topo: vm1 -- ls -- lr -- ext (with ACLs on ext_lr)
> +
> +OVS_TRAFFIC_VSWITCHD_START()
> +ADD_BR([br-int])
> +ADD_BR([br-ext], [set Bridge br-ext fail-mode=standalone])
> +
> +# Set external-ids in br-int needed for ovn-controller
> +check ovs-vsctl \
> + -- set Open_vSwitch . external-ids:system-id=hv1 \
> + -- set Open_vSwitch .
> external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \
> + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \
> + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \
> + -- set bridge br-int fail-mode=secure
> other-config:disable-in-band=true \
> + -- set Open_vSwitch . external-ids:ovn-bridge-mappings=phynet:br-ext
> +
> +start_daemon ovn-controller
> +
> +# Create logical switches: internal and external networks
> +check ovn-nbctl ls-add ls
> +check ovn-nbctl ls-add ext
> +
> +# Add localnet port to external switch for physical connectivity
> +check ovn-nbctl lsp-add-localnet-port ext ln_port phynet
> +
> +# Add VM port to internal switch
> +check ovn-nbctl lsp-add ls vm1 \
> + -- lsp-set-addresses vm1 "00:00:00:00:00:11 192.168.100.11"
> +
> +# Create logical_router_ports: lr_ls and lr_ext
> +check ovn-nbctl lr-add lr
> +check ovn-nbctl lrp-add lr lr_ls 00:00:00:00:01:01 192.168.100.1/24
> +check ovn-nbctl lrp-add lr lr_ext 00:00:00:00:01:02 172.16.0.1/24
> +check ovn-nbctl lrp-set-gateway-chassis lr_ext hv1
> +
> +# Create peer logical_switch_ports: ls_lr and ext_lr
> +check ovn-nbctl lsp-add ls ls_lr \
> + -- lsp-set-type ls_lr router \
> + -- lsp-set-addresses ls_lr router \
> + -- lsp-set-options ls_lr router-port=lr_ls
> +
> +check ovn-nbctl lsp-add ext ext_lr \
> + -- lsp-set-type ext_lr router \
> + -- lsp-set-addresses ext_lr router \
> + -- lsp-set-options ext_lr router-port=lr_ext enable_router_port_acl=true
> +
> +# Create port group for external gateway ACLs
> +check ovn-nbctl pg-add gw_pg ext_lr
> +
> +# Add ACL rule to enable connection tracking for external gateway traffic
> +# Allowing ICMP from vm1 to external
> +check ovn-nbctl acl-add gw_pg to-lport 2001 "inport == @gw_pg && ip4 &&
> icmp4" allow-related
> +check ovn-nbctl acl-add gw_pg to-lport 1001 "inport == @gw_pg && ip4" drop
> +check ovn-nbctl acl-add gw_pg to-lport 1001 "outport == @gw_pg && ip4" drop
> +check ovn-nbctl --wait=sb sync
> +
> +# Create namespaces vm1
> +ADD_NAMESPACES(vm1)
> +ADD_VETH(vm1, vm1, br-int, "192.168.100.11/24", "00:00:00:00:00:11", \
> + "192.168.100.1")
> +
> +# Create namespace external
> +ADD_NAMESPACES(external)
> +ADD_VETH(external, external, br-ext, "172.16.0.100/24", "00:00:00:00:01:10",
> \
> + "172.16.0.1")
> +NS_CHECK_EXEC([external], [ip route add 192.168.100.0/24 via 172.16.0.1])
We don't need this, 172.16.0.1 is already the default gateway in the
external namespace.
> +
> +wait_for_ports_up
I'd add an ovn-nbctl --wait=hv sync here, and remove the --wait=sb above.
> +
> +# ext_lr has enable-router-port-acl option
> +AT_CHECK([ovn-sbctl get Port_Binding ext_lr options:enable-router-port-acl],
> [0], [dnl
> +["true"]
> +])
> +
> +# ls_lr does not have enable-router-port-acl option
> +AT_CHECK([ovn-sbctl --columns=options list Port_Binding ls_lr | grep
> enable-router-port-acl], [1], [])
> +
I'd write these as:
# Check that ext_lr has enable_router_port_acl option.
AT_CHECK([ovn-sbctl get Port_Binding ext_lr options:enable_router_port_acl],
[0], [dnl
"true"
])
# Check that ls_lr does not have enable_router_port_acl option.
AT_CHECK([ovn-sbctl --columns options list Port_Binding ls_lr | grep -q
enable_router_port_acl], [1], [])
> +# Allocated CT zone for ext_lr
> +AT_CHECK([ovn-appctl -t ovn-controller ct-zone-list | sed "s/ [[0-9]]*/ ??/"
> | sort], [0], [stdout])
Nit: no need to sort.
> +AT_CHECK([grep "ext_lr" stdout], [0], [dnl
> +ext_lr ??
> +])
> +
> +# No CT zone for ls_lr
> +AT_CHECK([grep "ls_lr" stdout], [1], [])
> +
> +# Generate ICMP traffic to create conntrack entries
> +NS_CHECK_EXEC([vm1], [ping -q -c 2 -W 1 172.16.0.100 | FORMAT_PING], \
> +[0], [dnl
> +2 packets transmitted, 2 received, 0% packet loss, time 0ms
> +])
> +
> +# Check that ICMP conntrack entries exist
> +zone_id=$(ovn-appctl -t ovn-controller ct-zone-list | grep ext_lr | cut -d '
> ' -f2)
> +AT_CHECK_UNQUOTED([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(172.16.0.100)
> | grep "zone=$zone_id"], [0], [dnl
> +icmp,orig=(src=192.168.100.11,dst=172.16.0.100,id=<cleared>,type=8,code=0),reply=(src=172.16.0.100,dst=192.168.100.11,id=<cleared>,type=0,code=0),zone=$zone_id
> +])
> +
> +# Test TCP traffic - should be blocked by ACLs
> +OVS_START_L7([external], [http])
> +AT_CHECK([ip netns exec vm1 wget 172.16.0.100:80 -t 3 -T 1
> --retry-connrefused], [4], [ignore], [ignore])
On my test machine this fails because wget exits with 0 even if we
failed to get the page.
We can simplify the check and use nc instead:
NETNS_DAEMONIZE([external], [nc -l -k 172.16.0.100 80], [external.pid])
NS_CHECK_EXEC([vm1], [nc -z 172.16.0.100 80], [1])
This seems to be more reliable.
> +
> +# Remove enable_router_port_acl and verify CT zone release
> +check ovn-nbctl lsp-set-options ext_lr router-port=lr_ext
> +check ovn-nbctl --wait=sb sync
> +AT_CHECK([ovn-appctl -t ovn-controller ct-zone-list | grep "ext_lr"], [1],
> [])
> +
> +# Re-enable enable_router_port_acl and verify CT zone reallocation
> +check ovn-nbctl lsp-set-options ext_lr router-port=lr_ext
> enable_router_port_acl=true
> +check ovn-nbctl --wait=sb sync
> +AT_CHECK([ovn-appctl -t ovn-controller ct-zone-list | sed "s/ [[0-9]]*/ ??/"
> | sort], [0], [stdout])
> +AT_CHECK([grep "ext_lr" stdout], [0], [dnl
> +ext_lr ??
> +])
> +
> +# Test ICMP traffic still works after dynamic ACL changes
> +NS_CHECK_EXEC([vm1], [ping -q -c 2 -W 1 172.16.0.100 | FORMAT_PING], \
> +[0], [dnl
> +2 packets transmitted, 2 received, 0% packet loss, time 0ms
> +])
> +
> +OVS_APP_EXIT_AND_WAIT([ovn-controller])
> +
> +as ovn-sb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as ovn-nb
> +OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> +
> +as northd
> +OVS_APP_EXIT_AND_WAIT([ovn-northd])
We should use OVN_CLEANUP_NORTHD instead.
> +
> +as
> +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d
> +/connection dropped.*/d"])
> +
> +AT_CLEANUP
> +])
> +])
I took care of the minor issues above (also changed some other minor style
issues) and applied the patch to main.
I also pushed the patch to 25.09. It didn't apply cleanly on 25.03,
24.09 and 24.03, would you happen to have some time to prepare a custom
backport patch for those branches?
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
