| Message ID | 20210608203317.1032014-1-ihrachys@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] Document priority behavior for allow-stateless ACLs | expand |
On Tue, Jun 8, 2021 at 1:33 PM Ihar Hrachyshka <ihrachys@redhat.com> wrote: > > It's complex and probably impossible to split returning traffic for > allow-related ACLs from stateless traffic, we don't fully implement > ACL priority for allow-stateless rules. Meaning, allow-stateless rules > always take precedence over stateful rules regardless of their > relative priority order. > > This patch documents this behavior and covers it with explicit test > cases. > > Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> > --- > northd/ovn-northd.c | 1 + > northd/ovn_northd.dl | 1 + > ovn-nb.xml | 7 +++++ > tests/ovn-northd.at | 66 +++++++++++++++++++++++++++++++++++++++++--- > 4 files changed, 71 insertions(+), 4 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 9652ce252..d872f6a3c 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -5063,6 +5063,7 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *port_groups, > 110, lflows); > } > > + /* stateless filters always take precedence over stateful ACLs. */ > build_stateless_filters(od, port_groups, lflows); > > /* Ingress and Egress Pre-ACL Table (Priority 110). > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > index cb8418540..3afa80a3b 100644 > --- a/northd/ovn_northd.dl > +++ b/northd/ovn_northd.dl > @@ -1841,6 +1841,7 @@ for (&Switch(._uuid =ls_uuid)) { > .external_ids = map_empty()) > } > > +/* stateless filters always take precedence over stateful ACLs. */ > for (&SwitchACL(.sw = sw@&Switch{._uuid = ls_uuid}, .acl = &acl, .has_fair_meter = fair_meter)) { > if (sw.has_stateful_acl) { > if (acl.action == "allow-stateless") { > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 47f25eac1..91b5e303a 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -1761,6 +1761,13 @@ > Return traffic from an <code>allow-related</code> flow is always > allowed and cannot be changed through an ACL. > </p> > + > + <p> > + <code>allow-stateless</code> flows always take precedence before > + stateful ACLs, regardless of their priority. (Both > + <code>allow</code> and <code>allow-related</code> ACLs can be > + stateful.) > + </p> > </column> > > <column name="direction"> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 818ff7a20..4692775ad 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2722,7 +2722,7 @@ ct_next(ct_state=new|trk) { > > # Allow stateless for TCP. > for direction in from to; do > - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless > + ovn-nbctl acl-add ls ${direction}-lport 4 tcp allow-stateless > done > ovn-nbctl --wait=sb sync > > @@ -2778,7 +2778,7 @@ ct_lb { > > # Allow stateless for TCP. > for direction in from to; do > - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless > + ovn-nbctl acl-add ls ${direction}-lport 4 tcp allow-stateless > done > ovn-nbctl --wait=sb sync > > @@ -2858,7 +2858,7 @@ ct_next(ct_state=new|trk) { > > # Allow stateless for TCP. > for direction in from to; do > - ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless > + ovn-nbctl acl-add pg ${direction}-lport 4 tcp allow-stateless > done > ovn-nbctl --wait=sb sync > > @@ -2914,7 +2914,7 @@ ct_lb { > > # Allow stateless for TCP. > for direction in from to; do > - ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless > + ovn-nbctl acl-add pg ${direction}-lport 4 tcp allow-stateless > done > ovn-nbctl --wait=sb sync > > @@ -2943,6 +2943,64 @@ ct_lb { > AT_CLEANUP > ]) > > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn -- ACL allow-stateless overrides stateful rules with higher priority - Logical_Switch]) > +ovn_start > + > +ovn-nbctl ls-add ls > +ovn-nbctl lsp-add ls lsp1 > +ovn-nbctl lsp-set-addresses lsp1 00:00:00:00:00:01 > +ovn-nbctl lsp-add ls lsp2 > +ovn-nbctl lsp-set-addresses lsp2 00:00:00:00:00:02 > + > +for direction in from to; do > + ovn-nbctl acl-add ls ${direction}-lport 3 "tcp" allow-related > + ovn-nbctl acl-add ls ${direction}-lport 3 "udp" allow > +done > +ovn-nbctl --wait=sb sync > + > +flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02' > +flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66' > +flow_tcp='tcp && tcp.dst == 80' > +flow_udp='udp && udp.dst == 80' > + > +lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) > + > +# TCP packets should go to conntrack. > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > +ct_next(ct_state=new|trk) { > + ct_next(ct_state=new|trk) { > + output("lsp2"); > + }; > +}; > +]) > + > +# Allow stateless with *lower* priority. It always beats stateful rules. > +for direction in from to; do > + ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless > + ovn-nbctl acl-add ls ${direction}-lport 1 udp allow-stateless > +done > +ovn-nbctl --wait=sb sync > + > +# TCP packets should not go to conntrack anymore. > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" > +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl > +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 > +output("lsp2"); > +]) > + > +# UDP packets should not go to conntrack anymore. > +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}" > +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl > +# udp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80 > +output("lsp2"); > +]) > + > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD([ > AT_SETUP([ovn -- check BFD config propagation to SBDB]) > AT_KEYWORDS([northd-bfd]) > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Thanks Ihar. I applied this to master and also backported to branch-21.06 since the document is important to avoid confusion.
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 9652ce252..d872f6a3c 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -5063,6 +5063,7 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *port_groups, 110, lflows); } + /* stateless filters always take precedence over stateful ACLs. */ build_stateless_filters(od, port_groups, lflows); /* Ingress and Egress Pre-ACL Table (Priority 110). diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl index cb8418540..3afa80a3b 100644 --- a/northd/ovn_northd.dl +++ b/northd/ovn_northd.dl @@ -1841,6 +1841,7 @@ for (&Switch(._uuid =ls_uuid)) { .external_ids = map_empty()) } +/* stateless filters always take precedence over stateful ACLs. */ for (&SwitchACL(.sw = sw@&Switch{._uuid = ls_uuid}, .acl = &acl, .has_fair_meter = fair_meter)) { if (sw.has_stateful_acl) { if (acl.action == "allow-stateless") { diff --git a/ovn-nb.xml b/ovn-nb.xml index 47f25eac1..91b5e303a 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -1761,6 +1761,13 @@ Return traffic from an <code>allow-related</code> flow is always allowed and cannot be changed through an ACL. </p> + + <p> + <code>allow-stateless</code> flows always take precedence before + stateful ACLs, regardless of their priority. (Both + <code>allow</code> and <code>allow-related</code> ACLs can be + stateful.) + </p> </column> <column name="direction"> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 818ff7a20..4692775ad 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -2722,7 +2722,7 @@ ct_next(ct_state=new|trk) { # Allow stateless for TCP. for direction in from to; do - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless + ovn-nbctl acl-add ls ${direction}-lport 4 tcp allow-stateless done ovn-nbctl --wait=sb sync @@ -2778,7 +2778,7 @@ ct_lb { # Allow stateless for TCP. for direction in from to; do - ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless + ovn-nbctl acl-add ls ${direction}-lport 4 tcp allow-stateless done ovn-nbctl --wait=sb sync @@ -2858,7 +2858,7 @@ ct_next(ct_state=new|trk) { # Allow stateless for TCP. for direction in from to; do - ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless + ovn-nbctl acl-add pg ${direction}-lport 4 tcp allow-stateless done ovn-nbctl --wait=sb sync @@ -2914,7 +2914,7 @@ ct_lb { # Allow stateless for TCP. for direction in from to; do - ovn-nbctl acl-add pg ${direction}-lport 1 tcp allow-stateless + ovn-nbctl acl-add pg ${direction}-lport 4 tcp allow-stateless done ovn-nbctl --wait=sb sync @@ -2943,6 +2943,64 @@ ct_lb { AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn -- ACL allow-stateless overrides stateful rules with higher priority - Logical_Switch]) +ovn_start + +ovn-nbctl ls-add ls +ovn-nbctl lsp-add ls lsp1 +ovn-nbctl lsp-set-addresses lsp1 00:00:00:00:00:01 +ovn-nbctl lsp-add ls lsp2 +ovn-nbctl lsp-set-addresses lsp2 00:00:00:00:00:02 + +for direction in from to; do + ovn-nbctl acl-add ls ${direction}-lport 3 "tcp" allow-related + ovn-nbctl acl-add ls ${direction}-lport 3 "udp" allow +done +ovn-nbctl --wait=sb sync + +flow_eth='eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02' +flow_ip='ip.ttl==64 && ip4.src == 42.42.42.1 && ip4.dst == 66.66.66.66' +flow_tcp='tcp && tcp.dst == 80' +flow_udp='udp && udp.dst == 80' + +lsp1_inport=$(fetch_column Port_Binding tunnel_key logical_port=lsp1) + +# TCP packets should go to conntrack. +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 +ct_next(ct_state=new|trk) { + ct_next(ct_state=new|trk) { + output("lsp2"); + }; +}; +]) + +# Allow stateless with *lower* priority. It always beats stateful rules. +for direction in from to; do + ovn-nbctl acl-add ls ${direction}-lport 1 tcp allow-stateless + ovn-nbctl acl-add ls ${direction}-lport 1 udp allow-stateless +done +ovn-nbctl --wait=sb sync + +# TCP packets should not go to conntrack anymore. +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}" +AT_CHECK_UNQUOTED([ovn-trace --minimal ls "${flow}"], [0], [dnl +# tcp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80,tcp_flags=0 +output("lsp2"); +]) + +# UDP packets should not go to conntrack anymore. +flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}" +AT_CHECK_UNQUOTED([ovn-trace --ct new --ct new --minimal ls "${flow}"], [0], [dnl +# udp,reg14=0x${lsp1_inport},vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00:00:02,nw_src=42.42.42.1,nw_dst=66.66.66.66,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=0,tp_dst=80 +output("lsp2"); +]) + +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD([ AT_SETUP([ovn -- check BFD config propagation to SBDB]) AT_KEYWORDS([northd-bfd])
It's complex and probably impossible to split returning traffic for allow-related ACLs from stateless traffic, we don't fully implement ACL priority for allow-stateless rules. Meaning, allow-stateless rules always take precedence over stateful rules regardless of their relative priority order. This patch documents this behavior and covers it with explicit test cases. Signed-off-by: Ihar Hrachyshka <ihrachys@redhat.com> --- northd/ovn-northd.c | 1 + northd/ovn_northd.dl | 1 + ovn-nb.xml | 7 +++++ tests/ovn-northd.at | 66 +++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 71 insertions(+), 4 deletions(-)