On Mon, Apr 10, 2023 at 7:26 PM Mark Michelson <[email protected]> wrote:
> This allows for evaluating ACLs at the current tier to stop, and to > start evaluating ACLs at the next tier. If not using tiers, or if we > match on the final ACL tier, then a "pass" verdict results in the > default ACL action being applied. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2134138 > > Signed-off-by: Mark Michelson <[email protected]> > --- > northd/northd.c | 9 ++++++++- > ovn-nb.ovsschema | 4 ++-- > ovn-nb.xml | 10 ++++++++++ > tests/ovn-northd.at | 46 +++++++++++++++++++++++++++++++++++++++++++ > tests/system-ovn.at | 41 ++++++++++++++++++++++++++++++++++---- > utilities/ovn-nbctl.c | 2 +- > 6 files changed, 104 insertions(+), 8 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 36ca94ebb..da93bfe62 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -6411,6 +6411,8 @@ build_acl_log(struct ds *actions, const struct > nbrec_acl *acl, > ds_put_cstr(actions, "verdict=drop, "); > } else if (!strcmp(acl->action, "reject")) { > ds_put_cstr(actions, "verdict=reject, "); > + } else if (!strcmp(acl->action, "pass")) { > + ds_put_cstr(actions, "verdict=pass, "); > } else if (!strcmp(acl->action, "allow") > || !strcmp(acl->action, "allow-related") > || !strcmp(acl->action, "allow-stateless")) { > @@ -6447,12 +6449,15 @@ consider_acl(struct hmap *lflows, struct > ovn_datapath *od, > static const char *allow = REGBIT_ACL_VERDICT_ALLOW " = 1; "; > static const char *drop = REGBIT_ACL_VERDICT_DROP " = 1; "; > static const char *reject = REGBIT_ACL_VERDICT_REJECT " = 1; "; > + static const char *pass = ""; > > const char *verdict; > if (!strcmp(acl->action, "drop")) { > verdict = drop; > } else if (!strcmp(acl->action, "reject")) { > verdict = reject; > + } else if (!strcmp(acl->action, "pass")) { > + verdict = pass; > } else { > verdict = allow; > } > @@ -6472,7 +6477,9 @@ consider_acl(struct hmap *lflows, struct > ovn_datapath *od, > match_tier_len = match->length; > } > > - if (!has_stateful || !strcmp(acl->action, "allow-stateless")) { > + if (!has_stateful > + || !strcmp(acl->action, "pass") > + || !strcmp(acl->action, "allow-stateless")) { > ds_put_cstr(actions, "next;"); > ds_put_format(match, "(%s)", acl->match); > ovn_lflow_add_with_hint(lflows, od, stage, priority, > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema > index f12d39542..e713cce46 100644 > --- a/ovn-nb.ovsschema > +++ b/ovn-nb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Northbound", > "version": "7.0.0", > - "cksum": "3195094080 33650", > + "cksum": "2504399077 33658", > "tables": { > "NB_Global": { > "columns": { > @@ -260,7 +260,7 @@ > "enum": ["set", > ["allow", "allow-related", > "allow-stateless", "drop", > - "reject"]]}}}, > + "reject", "pass"]]}}}, > "log": {"type": "boolean"}, > "severity": {"type": {"key": {"type": "string", > "enum": ["set", > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 19cbd191d..3670cad03 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -2254,6 +2254,16 @@ or > ICMPv4/ICMPv6 unreachable message for other IPv4/IPv6-based > protocols. > </li> > + > + <li> > + <code>pass</code>: Pass to the next ACL tier. If using multiple > ACL > + tiers, a match on this ACL will stop evaluating ACLs at the > current > + tier and move to the next one. If not using ACL tiers or if a > + <code>pass</code> ACL is matched at the final tier, then the > + <ref column="options" key="default_acl_drop" table="NB_Global" > /> > + option from the <ref table="NB_Global" /> table is used to > + determine how to proceed. > + </li> > </ul> > </column> > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 507db7e7a..19385fe06 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -9126,3 +9126,49 @@ acl_test to-lport "" pg ls_out_acl_eval > ls_out_acl_action 4 > > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([ACL "pass" logical flows]) > +AT_KEYWORDS([acl]) > + > +ovn_start > +check ovn-nbctl ls-add ls > +check ovn-nbctl lsp-add ls lsp > +check ovn-nbctl pg-add pg lsp > + > +m4_define([ACL_FLOWS], [grep -w $1 lflows | grep "$2" | sed > 's/table=../table=??/' | sed "s/\($1[[^)]]*\)/$1/" | sort]) > + > +acl_test() { > + direction=$1 > + options=$2 > + thing=$3 > + eval_stage=$4 > + > + # Baseline. Ensure no ACL eval flows are present. > + ovn-sbctl lflow-list ls > lflows > + AT_CHECK([ACL_FLOWS([$eval_stage], [priority=2000])], [0], []) > + > + # Add an ACL with the "pass" verdict. Ensure that it is in the > logical flow > + # table and that it simply moves to the next table without setting a > specific > + # verdict bit. > + check ovn-nbctl --wait=sb $options acl-add $thing $direction 1000 > "ip4.addr == 80.111.111.112" pass > + ovn-sbctl lflow-list ls > lflows > + AT_CHECK_UNQUOTED([ACL_FLOWS([$eval_stage], [priority=2000])], [0], > [dnl > + table=??($eval_stage), priority=2000 , match=((ip4.addr == > 80.111.111.112)), action=(next;) > +]) > + > + # Remove the ACL with the "pass" verdict. Ensure that no eval flows > are present. > + check ovn-nbctl acl-del $thing > + ovn-sbctl lflow-list ls > lflows > + AT_CHECK([ACL_FLOWS([$eval_stage], [priority=2000])], [0], []) > +} > + > +acl_test from-lport "" ls ls_in_acl_eval > +acl_test from-lport "--apply-after-lb" ls ls_in_acl_after_lb_eval > +acl_test to-lport "" ls ls_out_acl_eval > +acl_test from-lport "" pg ls_in_acl_eval > +acl_test from-lport "--apply-after-lb" pg ls_in_acl_after_lb_eval > +acl_test to-lport "" pg ls_out_acl_eval > + > +AT_CLEANUP > +]) > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 620e29cf6..50d2d14bd 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -10831,7 +10831,6 @@ acl_test() { > # Add an untiered drop ACL. This should cause pings to fail. > check ovn-nbctl --wait=sb $options acl-add $thing $direction 1000 > "ip4.dst == 10.0.0.2" drop > acl1_uuid=$(ovn-nbctl --bare --columns _uuid find ACL priority=1000) > - > NS_CHECK_EXEC([lsp1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.2 | PING_PCT], > \ > [0], [dnl > 100% packet loss > @@ -10847,20 +10846,54 @@ acl_test() { > > # Add a tier-0 ACL that allows the traffic. The priority is only 4, > but > # since it is a higher tier, the traffic should be allowed. > - check ovn-nbctl --wait=sb $options acl-add $thing $direction 4 > "ip4.dst == 10.0.0.2" allow > + check ovn-nbctl --wait=hv $options acl-add $thing $direction 4 > "ip4.dst == 10.0.0.2" allow > + NS_CHECK_EXEC([lsp1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.2 | PING_PCT], > \ > +[0], [dnl > +0% packet loss > +]) > + > + # Add a higher-priority tier-0 ACL that passes. This should cause the > traffic > + # to pass over the lower-priority tier-0 "allow" ACL, and move to the > tier-3 > + # ACL that drops the traffic. > + check ovn-nbctl --wait=sb $options acl-add $thing $direction 1000 > "ip4.dst == 10.0.0.2" pass > + NS_CHECK_EXEC([lsp1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.2 | PING_PCT], > \ > +[0], [dnl > +100% packet loss > +]) > + > + # Remove the "pass" ACL, and the "allow" rule should kick back in. > + check ovn-nbctl --wait=sb --tier=0 acl-del $thing $direction 1000 > "ip4.dst == 10.0.0.2" > NS_CHECK_EXEC([lsp1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.2 | PING_PCT], > \ > [0], [dnl > 0% packet loss > ]) > > - # Removing the 0-tier ACL should make traffic go back to being > dropped. > + # Removing the remaining 0-tier ACL should make traffic go back to > being dropped. > check ovn-nbctl --wait=sb acl-del $thing $direction 4 "ip4.dst == > 10.0.0.2" > NS_CHECK_EXEC([lsp1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.2 | PING_PCT], > \ > [0], [dnl > 100% packet loss > ]) > > - # Removing all ACLs should make traffic go back to passing. > + # Adding a higher-priority "pass" ACL at tier 3 should result in > using the > + # default ACL action. Currently, the default is to allow traffic, so > the > + # traffic should be allowed. > + check ovn-nbctl --wait=sb --tier=3 $options acl-add $thing $direction > 2000 "ip4.dst == 10.0.0.2" pass > + NS_CHECK_EXEC([lsp1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.2 | PING_PCT], > \ > +[0], [dnl > +0% packet loss > +]) > + > + # Change the default ACL action to drop, and now the traffic should > be dropped. > + check ovn-nbctl set NB_Global . options:default_acl_drop=true > + NS_CHECK_EXEC([lsp1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.2 | PING_PCT], > \ > +[0], [dnl > +100% packet loss > +]) > + > + # Removing all ACLs (and setting the default acl drop back to false) > should > + # make traffic go back to passing. > + check ovn-nbctl clear NB_Global . options > check ovn-nbctl --wait=sb acl-del $thing > NS_CHECK_EXEC([lsp1], [ping -q -c 3 -i 0.3 -w 2 10.0.0.2 | PING_PCT], > \ > [0], [dnl > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index 7910b9d3f..2f2be0bbb 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -2332,7 +2332,7 @@ nbctl_acl_add(struct ctl_context *ctx) > /* Validate action. */ > if (strcmp(action, "allow") && strcmp(action, "allow-related") > && strcmp(action, "allow-stateless") && strcmp(action, "drop") > - && strcmp(action, "reject")) { > + && strcmp(action, "reject") && strcmp(action, "pass")) { > ctl_error(ctx, "%s: action must be one of \"allow\", " > "\"allow-related\", \"allow-stateless\", \"drop\", " > "and \"reject\"", action); > -- > 2.38.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Looks good to me, thanks. Reviewed-by: Ales Musil <[email protected]> -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
