northd: Add option to enable conntrack for the LSP whose peer is l3dgw_port #226

shylou · 2023-11-28T10:54:22Z

By default, OVN skips the conntrack process for router type LSP within a LS. It seems unnecessary for the LSP whose peer is l3dgw_port. Therefore, we introduce a new option named 'enable_conntrack', which defaults to false and can be set true to enable conntrack for the LSP whose peer is l3dgw_port.

And then we can implement l3 gateway stateful firewall, for example:

prelude: R1-S1 is a l3dgw_port
ovn-nbctl pg-add pg_dgw
ovn-nbctl pg-set-ports pg_dgw S1-R1
ovn-nbctl acl-add pg_dgw from-lport 1002 "inport == @pg_dgw && ip4" allow-related
ovn-nbctl acl-add pg_dgw to-lport 1003 "outport == @pg_dgw && ip4" allow-related
ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 enable_conntrack=true

NOTE: this option only works for the LSP whose peer is l3dgw_port.

#225

shylou · 2023-11-28T12:40:59Z

It looks the CI environment has problems @dceara

dceara · 2023-11-28T14:01:49Z

It looks the CI environment has problems @dceara

This is the error the CI is hitting: https://github.com/ovn-org/ovn/actions/runs/7017869125/job/19092070388?pr=226#step:8:2991

That's because of your change here:
https://github.com/ovn-org/ovn/pull/226/files#diff-9929cba282db9475dd8343b95e3b6a564d80dcef2f7a4594774fdc9ddd955df2R1135-R1139

That's an invalid xml there. I suspect that once that's fixed the builds will succeed too.

Regards,
Dumitru

shylou · 2023-12-01T02:53:34Z

@dceara hi, I do not know why Cirrus CI failed. Could you give me any suggestions?

make[3]: Leaving directory '/tmp/cirrus-ci-build/ovn-23.09.90/_build/sub'
make  check-local
make[3]: Entering directory '/tmp/cirrus-ci-build/ovn-23.09.90/_build/sub'
set /bin/sh '../../tests/testsuite' -C tests AUTOTEST_PATH=/tmp/cirrus-ci-build/ovs/utilities:/tmp/cirrus-ci-build/ovs/vswitchd:/tmp/cirrus-ci-build/ovs/ovsdb:/tmp/cirrus-ci-build/ovs/vtep:tests:::controller-vtep:northd:utilities:controller:ic; \
"$@" -j4 1001- || \
(test -z "$(find /tmp/cirrus-ci-build/ovn-23.09.90/_build/sub/tests/testsuite.dir -name 'sanitizers.*')" && \
 test X'no' = Xyes && "$@" --recheck)
invalid test group: 1001
find: ‘/tmp/cirrus-ci-build/ovn-23.09.90/_build/sub/tests/testsuite.dir’: No such file or directory
make[3]: *** [Makefile:3769: check-local] Error 1
make[3]: Leaving directory '/tmp/cirrus-ci-build/ovn-23.09.90/_build/sub'
make[2]: *** [Makefile:3000: check-am] Error 2
make[2]: Leaving directory '/tmp/cirrus-ci-build/ovn-23.09.90/_build/sub'
make[1]: *** [Makefile:3002: check] Error 2
make[1]: Leaving directory '/tmp/cirrus-ci-build/ovn-23.09.90/_build/sub'
make: *** [Makefile:2920: distcheck] Error 1
+ cat '*/_build/sub/tests/testsuite.log'
cat: '*/_build/sub/tests/testsuite.log': No such file or directory
+ return 1
+ stable_rc=1
+ '[' '' ']'
+ [[ 1 -ne 0 ]]
+ exit 1
����

dceara · 2023-12-01T15:17:29Z

@shylou you need to rebase, that's fixed by 5ef2a08

shylou · 2023-12-02T01:42:37Z

@shylou you need to rebase, that's fixed by 5ef2a08

ok! thx @dceara

dceara

@shylou, thanks for the PR, the approach looks OK overall, I did leave some comments though.

On the functionality itself, @numansiddique had added the code to skip router ports from conntrack to avoid extra recirculations with the idea that firewall rules are normally applied on VIF ports only.

When your option is configured we now support a different type of behavior. That's fine, i think, but I wanted to double check with Numan to see what he thinks too.

tests/ovn-northd.at

northd/northd.c

tests/ovn-northd.at

numansiddique · 2023-12-05T00:15:18Z

@shylou, thanks for the PR, the approach looks OK overall, I did leave some comments though.

On the functionality itself, @numansiddique had added the code to skip router ports from conntrack to avoid extra recirculations with the idea that firewall rules are normally applied on VIF ports only.

When your option is configured we now support a different type of behavior. That's fine, i think, but I wanted to double check with Numan to see what he thinks too.

Its fine with me too to add this option.
I don't like the option name - "enable_conntrack". Using conntrack for ACLs is internal to OVN.
However I'm OK if you can't find a better name.

Thanks

shylou · 2023-12-05T09:28:53Z

@shylou, thanks for the PR, the approach looks OK overall, I did leave some comments though.
On the functionality itself, @numansiddique had added the code to skip router ports from conntrack to avoid extra recirculations with the idea that firewall rules are normally applied on VIF ports only.
When your option is configured we now support a different type of behavior. That's fine, i think, but I wanted to double check with Numan to see what he thinks too.

Its fine with me too to add this option. I don't like the option name - "enable_conntrack". Using conntrack for ACLs is internal to OVN. However I'm OK if you can't find a better name.

Thanks

@numansiddique thanks for your review!

shylou · 2023-12-09T11:27:53Z

@dceara @numansiddique hi，please review this new patch.

dceara · 2023-12-20T11:59:35Z

@shylou This needs a very small rebase (minor conflict in the NEWS file). Also, please address @numansiddique's comment about changing the option name from "enable_conntrack" to something else; maybe we can call it enable-router-port-acl or something like that?

shylou · 2023-12-20T12:06:16Z

@shylou This needs a very small rebase (minor conflict in the NEWS file). Also, please address @numansiddique's comment about changing the option name from "enable_conntrack" to something else; maybe we can call it enable-router-port-acl or something like that?

@dceara OK! I will upgrade it as soon as possible.

shylou · 2023-12-21T08:18:09Z

@dceara @numansiddique please feel free to review the new patch.

dceara

Overall it looks OK to me, I left a few more minor comments. I'll wait for @numansiddique to comment too.

tests/ovn-northd.at

northd/northd.c

By default, OVN skips the conntrack process for router type LSP within a LS. It seems unnecessary for the LSP whose peer is l3dgw_port. Therefore, we introduce an option named 'enable_router_port_acl', which defaults to false and can be set to true to enable conntrack for the LSP whose peer is l3dgw_port. And then we can implement a gateway stateful firewall by dgw with stateful ACL. For example: prelude: R1-S1 is a l3dgw_port ovn-nbctl pg-add pg_dgw ovn-nbctl pg-set-ports pg_dgw S1-R1 ovn-nbctl acl-add pg_dgw from-lport 1002 "inport == @pg_dgw && ip4" allow-related ovn-nbctl acl-add pg_dgw to-lport 1003 "outport == @pg_dgw && ip4" allow-related ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 enable_router_port_acl=true NOTE: this option only works for the LSP whose peer is l3dgw_port. Signed-off-by: Xie Liu <[email protected]>

dceara

I didn't test it but this looks OK to me.

dceara · 2024-01-09T15:34:15Z

In case this goes in:

Acked-by: Dumitru Ceara <[email protected]>

northd/northd.c

numansiddique · 2024-01-09T22:52:07Z

northd/northd.c

@@ -2843,6 +2860,13 @@ join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table,
                                 "port %s is a switch port", op->key);
                }
            }
+        } else if (op->nbsp && !lsp_is_router(op->nbsp)) {


Same here. I don't think we can just ignore and remove this "else if".

@dceara Wdyt ?

@numansiddique I guess you meant "I think we can just ignore and remove.." right?

Yes I meant the same. Sorry for the typo

tests/ovn-northd.at

By default, OVN skips the conntrack process for router type LSP within a LS. It seems unnecessary for the LSP whose peer is l3dgw_port. Therefore, we introduce an option named 'enable_router_port_acl', which defaults to false and can be set to true to enable conntrack for the LSP whose peer is l3dgw_port. And then we can implement a gateway stateful firewall by dgw with stateful ACL. For example: prelude: R1-S1 is a l3dgw_port ovn-nbctl pg-add pg_dgw ovn-nbctl pg-set-ports pg_dgw S1-R1 ovn-nbctl acl-add pg_dgw from-lport 1002 "inport == @pg_dgw && ip4" allow-related ovn-nbctl acl-add pg_dgw to-lport 1003 "outport == @pg_dgw && ip4" allow-related ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 enable_router_port_acl=true NOTE: this option only works for the LSP whose peer is l3dgw_port. Submitted-at: ovn-org#226 Signed-off-by: Xie Liu <[email protected]> Acked-by: Dumitru Ceara <[email protected]> Signed-off-by: Numan Siddique <[email protected]>

numansiddique · 2024-01-10T16:42:14Z

Submitted the patch with the below changes to the ovs-dev mailing list. Once the CI passes I'll apply this patch.

https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/

diff --git a/northd/northd.c b/northd/northd.c
index 34d13a900c..952f8200d4 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -2831,17 +2831,9 @@ join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table,
             }
 
             /* Only used for the router type LSP whose peer is l3dgw_port */
-            if (smap_get(&op->nbsp->options, "enable_router_port_acl")) {
-                if (op->peer && is_l3dgw_port(op->peer)) {
-                    op->enable_router_port_acl = smap_get_bool(&op->nbsp->options,
-                                              "enable_router_port_acl", false);
-                } else {
-                    static struct vlog_rate_limit rl =
-                        VLOG_RATE_LIMIT_INIT(5, 1);
-                    VLOG_WARN_RL(&rl, "enable_router_port_acl option is not "
-                                      "supported on logical port %s",
-                                      op->nbsp->name);
-                }
+            if (op->peer && is_l3dgw_port(op->peer)) {
+                op->enable_router_port_acl = smap_get_bool(
+                    &op->nbsp->options, "enable_router_port_acl", false);
             }
         } else if (op->nbrp && op->nbrp->peer && !op->l3dgw_port) {
             struct ovn_port *peer = ovn_port_find(ports, op->nbrp->peer);
@@ -2860,13 +2852,6 @@ join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table,
                                  "port %s is a switch port", op->key);
                 }
             }
-        } else if (op->nbsp && !lsp_is_router(op->nbsp)) {
-            /* Only used for the router type LSP whose peer is l3dgw_port */
-            if (smap_get(&op->nbsp->options, "enable_router_port_acl")) {
-                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
-                VLOG_WARN_RL(&rl, "enable_router_port_acl option is not supported "
-                                    "on logical port %s", op->nbsp->name);
-            }
         }
     }
 
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 98cf7adb43..f1eb9ecb1b 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -438,7 +438,12 @@
       <code>Pre-stateful</code> to send IP packets to the connection tracker
       before eventually advancing to ingress table <code>ACLs</code>. If
       special ports such as route ports or localnet ports can't use ct(), a
-      priority-110 flow is added to skip over stateful ACLs. Multicast, IPv6
+      priority-110 flow is added to skip over stateful ACLs.  This
+      priority-110 flow is not addd for router ports if the option
+      enable_router_port_acl is set to true in
+      <ref column="options:enable_router_port_acl"
+      table="Logical_Switch_Port" db="OVN_Northbound"/> column of
+      <ref table="Logical_Switch_Port" db="OVN_Northbound"/>.  Multicast, IPv6
       Neighbor Discovery and MLD traffic also skips stateful ACLs. For
       "allow-stateless" ACLs, a flow is added to bypass setting the hint for
       connection tracker processing when there are stateful ACLs or LB rules;

By default, OVN skips the conntrack process for router type LSP within a LS. It seems unnecessary for the LSP whose peer is l3dgw_port. Therefore, we introduce an option named 'enable_router_port_acl', which defaults to false and can be set to true to enable conntrack for the LSP whose peer is l3dgw_port. And then we can implement a gateway stateful firewall by dgw with stateful ACL. For example: prelude: R1-S1 is a l3dgw_port ovn-nbctl pg-add pg_dgw ovn-nbctl pg-set-ports pg_dgw S1-R1 ovn-nbctl acl-add pg_dgw from-lport 1002 "inport == @pg_dgw && ip4" allow-related ovn-nbctl acl-add pg_dgw to-lport 1003 "outport == @pg_dgw && ip4" allow-related ovn-nbctl lsp-set-options S1-R1 router-port=R1-S1 enable_router_port_acl=true NOTE: this option only works for the LSP whose peer is l3dgw_port. Submitted-at: ovn-org#226 Signed-off-by: Xie Liu <[email protected]> Acked-by: Dumitru Ceara <[email protected]> Signed-off-by: Numan Siddique <[email protected]> Signed-off-by: 0-day Robot <[email protected]>

numansiddique · 2024-01-10T20:03:48Z

Patch applied - 9a0f307

Closing the PR

shylou force-pushed the stateless-drop branch from ef238fd to 7651944 Compare November 28, 2023 11:08

shylou force-pushed the stateless-drop branch 8 times, most recently from e508224 to 474bdfa Compare November 30, 2023 13:54

shylou force-pushed the stateless-drop branch from 474bdfa to 2d1f05e Compare December 2, 2023 01:21

dceara requested changes Dec 4, 2023

View reviewed changes

tests/ovn-northd.at Outdated Show resolved Hide resolved

northd/northd.c Outdated Show resolved Hide resolved

northd/northd.c Outdated Show resolved Hide resolved

tests/ovn-northd.at Outdated Show resolved Hide resolved

shylou force-pushed the stateless-drop branch 4 times, most recently from eac2f44 to 518ec54 Compare December 9, 2023 05:58

shylou closed this Dec 9, 2023

shylou reopened this Dec 9, 2023

shylou force-pushed the stateless-drop branch 2 times, most recently from 5c61d2f to 674f396 Compare December 20, 2023 14:04

shylou closed this Dec 21, 2023

shylou reopened this Dec 21, 2023

shylou closed this Dec 21, 2023

shylou reopened this Dec 21, 2023

shylou force-pushed the stateless-drop branch 2 times, most recently from 993db39 to 4abd3a3 Compare December 21, 2023 03:39

shylou closed this Dec 21, 2023

shylou reopened this Dec 21, 2023

dceara requested changes Jan 3, 2024

View reviewed changes

tests/ovn-northd.at Outdated Show resolved Hide resolved

tests/ovn-northd.at Outdated Show resolved Hide resolved

northd/northd.c Outdated Show resolved Hide resolved

shylou force-pushed the stateless-drop branch from 4abd3a3 to 9b0fa91 Compare January 4, 2024 04:39

shylou force-pushed the stateless-drop branch from 9b0fa91 to 48397c0 Compare January 4, 2024 05:16

dceara approved these changes Jan 9, 2024

View reviewed changes

numansiddique reviewed Jan 9, 2024

View reviewed changes

northd/northd.c Show resolved Hide resolved

numansiddique reviewed Jan 9, 2024

View reviewed changes

tests/ovn-northd.at Show resolved Hide resolved

numansiddique closed this Jan 10, 2024

dceara mentioned this pull request Jan 23, 2024

no need skip conntrack for the lsp which peer is l3dgw_port #225

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

northd: Add option to enable conntrack for the LSP whose peer is l3dgw_port #226

northd: Add option to enable conntrack for the LSP whose peer is l3dgw_port #226

shylou commented Nov 28, 2023 •

edited

Loading

shylou commented Nov 28, 2023

dceara commented Nov 28, 2023

shylou commented Dec 1, 2023

dceara commented Dec 1, 2023

shylou commented Dec 2, 2023

dceara left a comment

numansiddique commented Dec 5, 2023

shylou commented Dec 5, 2023

shylou commented Dec 9, 2023

dceara commented Dec 20, 2023

shylou commented Dec 20, 2023

shylou commented Dec 21, 2023

dceara left a comment

dceara left a comment

dceara commented Jan 9, 2024

numansiddique Jan 9, 2024

dceara Jan 10, 2024

numansiddique Jan 10, 2024

numansiddique commented Jan 10, 2024 •

edited

Loading

numansiddique commented Jan 10, 2024

northd: Add option to enable conntrack for the LSP whose peer is l3dgw_port #226

northd: Add option to enable conntrack for the LSP whose peer is l3dgw_port #226

Conversation

shylou commented Nov 28, 2023 • edited Loading

shylou commented Nov 28, 2023

dceara commented Nov 28, 2023

shylou commented Dec 1, 2023

dceara commented Dec 1, 2023

shylou commented Dec 2, 2023

dceara left a comment

Choose a reason for hiding this comment

numansiddique commented Dec 5, 2023

shylou commented Dec 5, 2023

shylou commented Dec 9, 2023

dceara commented Dec 20, 2023

shylou commented Dec 20, 2023

shylou commented Dec 21, 2023

dceara left a comment

Choose a reason for hiding this comment

dceara left a comment

Choose a reason for hiding this comment

dceara commented Jan 9, 2024

numansiddique Jan 9, 2024

Choose a reason for hiding this comment

dceara Jan 10, 2024

Choose a reason for hiding this comment

numansiddique Jan 10, 2024

Choose a reason for hiding this comment

numansiddique commented Jan 10, 2024 • edited Loading

numansiddique commented Jan 10, 2024

shylou commented Nov 28, 2023 •

edited

Loading

numansiddique commented Jan 10, 2024 •

edited

Loading