| Message ID | 20210802083242.75982-1-priyankar.jain@nutanix.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v4] northd: Add ACL label | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
References: <20210802083242.75982-1-priyankar.jain@nutanix.com>
Bleep bloop. Greetings Priyankar Jain, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 412 characters long (recommended limit is 79)
#900 FILE: utilities/ovn-nbctl.8.xml:402:
<dt>[<code>--type=</code>{<code>switch</code> | <code>port-group</code>}] [<code>--log</code>] [<code>--meter=</code><var>meter</var>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--label=</code><var>label</var>] [<code>--may-exist</code>] <code>acl-add</code> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt>
Lines checked: 983, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
Hi Numan, In case the patch looks good to you, can you please merge it in master? Thanks and Regards, Priyankar Jain On 02/08/21 2:02 pm, Priyankar Jain wrote: > Allow adding label to an ACL to identify which ACL allowed a particular > flow in the connection tracking table. > > The ACL label covers 32 bits at the end of ct_label. Since only allowed > connections are committed, only "allow" and "allow-related" ACLs can > have the label. > > If the ACL allowing the connection changes, the label associated with the > new ACL gets updated in the ct_label field. This is achieved by committing > every packet that hits the ACL with the label to the connection tracking > table. > In case the new ACL doesn't have a label, the ct_label field is not > cleared. This is done to prevent any performance change with ACLs that > don't have label set. > For the packets which hits an ACL without label, the behaviour remains the > same as before with respect to the conntrack commit. > > Performance: > We used ftrace to measure the time taken by an extra conntrack commit for > the packets hitting the ACL with label. We measured the time taken to > execute the ovs_ct_execute call inside the sock_sendmsg call. > > ACL used :- > from-lport 2000 (tcp && ip4.src == 10.0.0.11 && ip4.dst == 10.0.0.12) allow-related --label=1234 > > It was observed that the extra ovs_ct_execute call accounted for 1-2% > of the round trip time (sock_sendmsg duration). The actual percentage > is expected to be lesser since it doesn't take into account the tracing > overhead which is substantial for smaller functions. > > Signed-off-by: Priyankar Jain <priyankar.jain@nutanix.com> > Acked-by: Numan Siddique <numans@ovn.org> > --- > > v3 -> v4 > * Resolved merge conflicts with master. > > v2 -> v3 > * Added NEWS entry. > > v1 -> v2 > * Changed type of label from string (hex) to unsigned integer. > * Added two system tests for checking the correct ct_label programming. > * Updated ovn-northd.8.xml with the new logical flows added as part of > this patch. > * Rebased with latest master. > > NEWS | 3 + > lib/logical-fields.c | 2 + > northd/ovn-northd.8.xml | 26 +++- > northd/ovn-northd.c | 53 +++++++-- > northd/ovn_northd.dl | 76 +++++++++--- > ovn-nb.ovsschema | 7 +- > ovn-nb.xml | 12 ++ > tests/ovn-nbctl.at | 19 +++ > tests/ovn-northd.at | 107 ++++++++++++++++- > tests/ovn.at | 1 + > tests/system-ovn.at | 244 ++++++++++++++++++++++++++++++++++++++ > utilities/ovn-nbctl.8.xml | 2 +- > utilities/ovn-nbctl.c | 38 +++++- > 13 files changed, 548 insertions(+), 42 deletions(-) > > diff --git a/NEWS b/NEWS > index f328666da..45dc45353 100644 > --- a/NEWS > +++ b/NEWS > @@ -3,6 +3,9 @@ Post-v21.06.0 > - Added Control Plane Protection support (control plane traffic metering). > - Added path MTU discovery for ingress traffic originated outside of the > cluster. > + - Introduced a new "label" field for "allow" and "allow-related" ACLs > + which helps in debugging/backtracking the ACL which allowed a particular > + connection. > > OVN v21.06.0 - 18 Jun 2021 > ------------------------- > diff --git a/lib/logical-fields.c b/lib/logical-fields.c > index 72853013e..7b3d431e0 100644 > --- a/lib/logical-fields.c > +++ b/lib/logical-fields.c > @@ -146,6 +146,8 @@ ovn_init_symtab(struct shash *symtab) > "ct_label[32..79]", WR_CT_COMMIT); > expr_symtab_add_subfield_scoped(symtab, "ct_label.ecmp_reply_port", NULL, > "ct_label[80..95]", WR_CT_COMMIT); > + expr_symtab_add_subfield_scoped(symtab, "ct_label.label", NULL, > + "ct_label[96..127]", WR_CT_COMMIT); > > expr_symtab_add_field(symtab, "ct_state", MFF_CT_STATE, NULL, false); > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 08d484760..c5ea15774 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -670,13 +670,19 @@ > the <code>next;</code> action. If there are any stateful ACLs > on this datapath, then <code>allow</code> ACLs translate to > <code>ct_commit; next;</code> (which acts as a hint for the next tables > - to commit the connection to conntrack), > + to commit the connection to conntrack). In case the <code>ACL</code> > + has a label then <code>reg3</code> is loaded with the label value and > + <code>reg0[13]</code> bit is set to 1 (which acts as a hint for the > + next tables to commit the label to conntrack). > </li> > <li> > <code>allow-related</code> ACLs translate into logical > flows with the <code>ct_commit(ct_label=0/1); next;</code> actions > for new connections and <code>reg0[1] = 1; next;</code> for existing > - connections. > + connections. In case the <code>ACL</code> has a label then > + <code>reg3</code> is loaded with the label value and > + <code>reg0[13]</code> bit is set to 1 (which acts as a hint for the > + next tables to commit the label to conntrack). > </li> > <li> > <code>allow-stateless</code> ACLs translate into logical > @@ -876,9 +882,19 @@ > </li> > > <li> > - A priority-100 flow commits packets to connection tracker using > - <code>ct_commit; next;</code> action based on a hint provided by > - the previous tables (with a match for <code>reg0[1] == 1</code>). > + A priority 100 flow is added which commits the packet to the conntrack > + and sets the most significant 32-bits of <code>ct_label</code> with the > + <code>reg3</code> value based on the hint provided by previous tables > + (with a match for <code>reg0[1] == 1 && reg0[13] == 1</code>). > + This is used by the <code>ACLs</code> with label to commit the label > + value to conntrack. > + </li> > + > + <li> > + For <code>ACLs</code> without label, a second priority-100 flow commits > + packets to connection tracker using <code>ct_commit; next;</code> > + action based on a hint provided by the previous tables (with a match > + for <code>reg0[1] == 1 && reg0[13] == 0</code>). > </li> > <li> > A priority-0 flow that simply moves traffic to the next table. > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index a0eaa1247..1ad15b978 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -241,6 +241,7 @@ enum ovn_stage { > #define REGBIT_ACL_HINT_BLOCK "reg0[10]" > #define REGBIT_LKUP_FDB "reg0[11]" > #define REGBIT_HAIRPIN_REPLY "reg0[12]" > +#define REGBIT_ACL_LABEL "reg0[13]" > > #define REG_ORIG_DIP_IPV4 "reg1" > #define REG_ORIG_DIP_IPV6 "xxreg1" > @@ -273,6 +274,9 @@ enum ovn_stage { > #define REG_SRC_IPV4 "reg1" > #define REG_SRC_IPV6 "xxreg1" > > +/* Register used for setting a label for ACLs in a Logical Switch. */ > +#define REG_LABEL "reg3" > + > #define FLAGBIT_NOT_VXLAN "flags[1] == 0" > > /* > @@ -281,14 +285,15 @@ enum ovn_stage { > * Logical Switch pipeline: > * +----+----------------------------------------------+---+------------------+ > * | R0 | REGBIT_{CONNTRACK/DHCP/DNS} | | | > - * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | X | | > - * | | REGBIT_ACL_HINT_{ALLOW_NEW/ALLOW/DROP/BLOCK} | X | | > + * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | | | > + * | | REGBIT_ACL_HINT_{ALLOW_NEW/ALLOW/DROP/BLOCK} | | | > + * | | REGBIT_ACL_LABEL | X | | > * +----+----------------------------------------------+ X | | > * | R1 | ORIG_DIP_IPV4 (>= IN_STATEFUL) | R | | > * +----+----------------------------------------------+ E | | > * | R2 | ORIG_TP_DPORT (>= IN_STATEFUL) | G | | > * +----+----------------------------------------------+ 0 | | > - * | R3 | UNUSED | | | > + * | R3 | ACL LABEL | | | > * +----+----------------------------------------------+---+------------------+ > * | R4 | UNUSED | | | > * +----+----------------------------------------------+ X | ORIG_DIP_IPV6 | > @@ -5774,7 +5779,12 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > ds_clear(actions); > ds_put_format(match, REGBIT_ACL_HINT_ALLOW_NEW " == 1 && (%s)", > acl->match); > + > ds_put_cstr(actions, REGBIT_CONNTRACK_COMMIT" = 1; "); > + if (acl->label) { > + ds_put_format(actions, REGBIT_ACL_LABEL" = 1; " > + REG_LABEL" = %"PRId64"; ", acl->label); > + } > build_acl_log(actions, acl, meter_groups); > ds_put_cstr(actions, "next;"); > ovn_lflow_add_with_hint(lflows, od, stage, > @@ -5785,15 +5795,21 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > > /* Match on traffic in the request direction for an established > * connection tracking entry that has not been marked for > - * deletion. There is no need to commit here, so we can just > - * proceed to the next table. We use this to ensure that this > + * deletion. We use this to ensure that this > * connection is still allowed by the currently defined > - * policy. Match untracked packets too. */ > + * policy. Match untracked packets too. > + * Commit the connection only if the ACL has a label. This is done > + * to update the connection tracking entry label in case the ACL > + * allowing the connection changes. */ > ds_clear(match); > ds_clear(actions); > ds_put_format(match, REGBIT_ACL_HINT_ALLOW " == 1 && (%s)", > acl->match); > - > + if (acl->label) { > + ds_put_cstr(actions, REGBIT_CONNTRACK_COMMIT" = 1; "); > + ds_put_format(actions, REGBIT_ACL_LABEL" = 1; " > + REG_LABEL" = %"PRId64"; ", acl->label); > + } > build_acl_log(actions, acl, meter_groups); > ds_put_cstr(actions, "next;"); > ovn_lflow_add_with_hint(lflows, od, stage, > @@ -6292,15 +6308,34 @@ build_stateful(struct ovn_datapath *od, struct hmap *lflows) > ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 0, "1", "next;"); > ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 0, "1", "next;"); > > + /* If REGBIT_CONNTRACK_COMMIT is set as 1 and > + * REGBIT_CONNTRACK_SET_LABEL is set to 1, then the packets should be > + * committed to conntrack. > + * We always set ct_mark.blocked to 0 here as > + * any packet that makes it this far is part of a connection we > + * want to allow to continue. */ > + ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, > + REGBIT_CONNTRACK_COMMIT" == 1 && " > + REGBIT_ACL_LABEL" == 1", > + "ct_commit { ct_label.blocked = 0; " > + "ct_label.label = " REG_LABEL "; }; next;"); > + ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100, > + REGBIT_CONNTRACK_COMMIT" == 1 && " > + REGBIT_ACL_LABEL" == 1", > + "ct_commit { ct_label.blocked = 0; " > + "ct_label.label = " REG_LABEL "; }; next;"); > + > /* If REGBIT_CONNTRACK_COMMIT is set as 1, then the packets should be > * committed to conntrack. We always set ct_label.blocked to 0 here as > * any packet that makes it this far is part of a connection we > * want to allow to continue. */ > ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, > - REGBIT_CONNTRACK_COMMIT" == 1", > + REGBIT_CONNTRACK_COMMIT" == 1 && " > + REGBIT_ACL_LABEL" == 0", > "ct_commit { ct_label.blocked = 0; }; next;"); > ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100, > - REGBIT_CONNTRACK_COMMIT" == 1", > + REGBIT_CONNTRACK_COMMIT" == 1 && " > + REGBIT_ACL_LABEL" == 0", > "ct_commit { ct_label.blocked = 0; }; next;"); > } > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > index 091fe10b3..b3d1a9657 100644 > --- a/northd/ovn_northd.dl > +++ b/northd/ovn_northd.dl > @@ -1505,13 +1505,14 @@ function s_ROUTER_OUT_DELIVERY(): Stage { Stage{ Egress, 4, "lr_out_deliv > * Logical Switch pipeline: > * +----+----------------------------------------------+---+------------------+ > * | R0 | REGBIT_{CONNTRACK/DHCP/DNS} | | | > - * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | X | | > + * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | | | > + * | | REGBIT_ACL_LABEL | X | | > * +----+----------------------------------------------+ X | | > * | R1 | ORIG_DIP_IPV4 (>= IN_STATEFUL) | R | | > * +----+----------------------------------------------+ E | | > * | R2 | ORIG_TP_DPORT (>= IN_STATEFUL) | G | | > * +----+----------------------------------------------+ 0 | | > - * | R3 | UNUSED | | | > + * | R3 | ACL_LABEL | | | > * +----+----------------------------------------------+---+------------------+ > * | R4 | UNUSED | | | > * +----+----------------------------------------------+ X | ORIG_DIP_IPV6 | > @@ -1584,6 +1585,7 @@ function rEGBIT_ACL_HINT_DROP() : string = "reg0[9]" > function rEGBIT_ACL_HINT_BLOCK() : string = "reg0[10]" > function rEGBIT_LKUP_FDB() : string = "reg0[11]" > function rEGBIT_HAIRPIN_REPLY() : string = "reg0[12]" > +function rEGBIT_ACL_LABEL() : string = "reg0[13]" > > function rEG_ORIG_DIP_IPV4() : string = "reg1" > function rEG_ORIG_DIP_IPV6() : string = "xxreg1" > @@ -1610,6 +1612,9 @@ function rEG_INPORT_ETH_ADDR() : string = "xreg0[0..47]" > function rEG_ECMP_GROUP_ID() : string = "reg8[0..15]" > function rEG_ECMP_MEMBER_ID() : string = "reg8[16..31]" > > +/* Register used for setting a label for ACLs in a Logical Switch. */ > +function rEG_LABEL() : string = "reg3" > + > function fLAGBIT_NOT_VXLAN() : string = "flags[1] == 0" > > function mFF_N_LOG_REGS() : bit<32> = 10 > @@ -2698,26 +2703,41 @@ for (&SwitchACL(.sw = sw, .acl = acl, .has_fair_meter = fair_meter)) { > * that case here and un-set ct_label.blocked (which will be done > * by ct_commit in the "stateful" stage) to indicate that the > * connection should be allowed to resume. > + * If the ACL has a label, then load REG_LABEL with the label and > + * set the REGBIT_ACL_LABEL field. > */ > - Flow(.logical_datapath = sw._uuid, > - .stage = stage, > - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > - .__match = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})", > - .actions = "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${acl_log}next;", > - .external_ids = stage_hint); > + var __action = if (acl.label != 0) { > + "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${rEGBIT_ACL_LABEL()} = 1;\ > + \ ${rEG_LABEL()} = ${acl.label}; ${acl_log}next;" > + } else { > + "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${acl_log}next;" > + } in Flow(.logical_datapath = sw._uuid, > + .stage = stage, > + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > + .__match = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})", > + .actions = __action, > + .external_ids = stage_hint); > > /* Match on traffic in the request direction for an established > * connection tracking entry that has not been marked for > - * deletion. There is no need to commit here, so we can just > - * proceed to the next table. We use this to ensure that this > + * deletion. We use this to ensure that this > * connection is still allowed by the currently defined > - * policy. Match untracked packets too. */ > - Flow(.logical_datapath = sw._uuid, > - .stage = stage, > - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > - .__match = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})", > - .actions = "${acl_log}next;", > - .external_ids = stage_hint) > + * policy. Match untracked packets too. > + * Commit the connection only if the ACL has a label. This is done to > + * update the connection tracking entry label in case the ACL > + * allowing the connection changes. > + */ > + var __action = if (acl.label != 0) { > + "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${rEGBIT_ACL_LABEL()} = 1;\ > + \ ${rEG_LABEL()} = ${acl.label}; ${acl_log}next;" > + } else { > + "${acl_log}next;" > + } in Flow(.logical_datapath = sw._uuid, > + .stage = stage, > + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > + .__match = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})", > + .actions = __action, > + .external_ids = stage_hint) > } > } else if (acl.action == "allow-stateless") { > Flow(.logical_datapath = sw._uuid, > @@ -2934,6 +2954,24 @@ for (&Switch(._uuid = ls_uuid)) { > .actions = "next;", > .external_ids = map_empty()); > > + /* If REGBIT_CONNTRACK_COMMIT is set as 1 and REGBIT_CONNTRACK_SET_LABEL > + * is set to 1, then the packets should be > + * committed to conntrack. We always set ct_label.blocked to 0 here as > + * any packet that makes it this far is part of a connection we > + * want to allow to continue. */ > + Flow(.logical_datapath = ls_uuid, > + .stage = s_SWITCH_IN_STATEFUL(), > + .priority = 100, > + .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 1", > + .actions = "ct_commit { ct_label.blocked = 0; ct_label.label = ${rEG_LABEL()}; }; next;", > + .external_ids = map_empty()); > + Flow(.logical_datapath = ls_uuid, > + .stage = s_SWITCH_OUT_STATEFUL(), > + .priority = 100, > + .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 1", > + .actions = "ct_commit { ct_label.blocked = 0; ct_label.label = ${rEG_LABEL()}; }; next;", > + .external_ids = map_empty()); > + > /* If REGBIT_CONNTRACK_COMMIT is set as 1, then the packets should be > * committed to conntrack. We always set ct_label.blocked to 0 here as > * any packet that makes it this far is part of a connection we > @@ -2941,13 +2979,13 @@ for (&Switch(._uuid = ls_uuid)) { > Flow(.logical_datapath = ls_uuid, > .stage = s_SWITCH_IN_STATEFUL(), > .priority = 100, > - .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1", > + .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 0", > .actions = "ct_commit { ct_label.blocked = 0; }; next;", > .external_ids = map_empty()); > Flow(.logical_datapath = ls_uuid, > .stage = s_SWITCH_OUT_STATEFUL(), > .priority = 100, > - .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1", > + .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 0", > .actions = "ct_commit { ct_label.blocked = 0; }; next;", > .external_ids = map_empty()) > } > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema > index a53d5e851..2ac8ef3ea 100644 > --- a/ovn-nb.ovsschema > +++ b/ovn-nb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Northbound", > - "version": "5.32.0", > - "cksum": "2501921026 29540", > + "version": "5.32.1", > + "cksum": "2805328215 29734", > "tables": { > "NB_Global": { > "columns": { > @@ -244,6 +244,9 @@ > "debug"]]}, > "min": 0, "max": 1}}, > "meter": {"type": {"key": "string", "min": 0, "max": 1}}, > + "label": {"type": {"key": {"type": "integer", > + "minInteger": 0, > + "maxInteger": 4294967295}}}, > "external_ids": { > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}}, > diff --git a/ovn-nb.xml b/ovn-nb.xml > index c1176e81f..ebafa3e5e 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -1853,6 +1853,18 @@ > and <code>deny</code> as <ref column="action"/>.) > </p> > > + <column name="label"> > + <p> > + Associates an identifier with the ACL. > + The same value will be written to corresponding connection > + tracker entry. The value should be a valid 32-bit unsigned integer. > + This value can help in debugging from connection tracker side. > + For example, through this "label" we can backtrack to the ACL rule > + which is causing a "leaked" connection. Connection tracker entries are > + created only for allowed connections so the label is valid only > + for allow and allow-related actions. > + </p> > + </column> > <column name="priority"> > <p> > The ACL rule's priority. Rules with numerically higher priority > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index 828777b82..4445ab7c0 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -211,18 +211,36 @@ ovn_nbctl_test_acl() { > AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 300 tcp drop]) > AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 200 ip drop]) > AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop]) > + AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 from-lport 70 icmp allow-related]) > + AT_CHECK([ovn-nbctl $2 --label=1235 acl-add $1 to-lport 70 icmp allow-related]) > + > dnl Add duplicated ACL > AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop], [1], [], [stderr]) > AT_CHECK([grep 'already existed' stderr], [0], [ignore]) > AT_CHECK([ovn-nbctl $2 --may-exist acl-add $1 to-lport 100 ip drop]) > > + dnl Add invalid ACL label > + AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 to-lport 50 ip drop], [1], [], [stderr]) > + AT_CHECK([grep 'can only be set with actions' stderr], [0], [ignore]) > + > + AT_CHECK([ovn-nbctl $2 --label=abcd acl-add $1 to-lport 50 ip allow-related], [1], [], [stderr]) > + AT_CHECK([grep 'label must in range 0...4294967295' stderr], [0], [ignore]) > + > + AT_CHECK([ovn-nbctl $2 --label=-1 acl-add $1 to-lport 50 ip allow-related], [1], [], [stderr]) > + AT_CHECK([grep 'label must in range 0...4294967295' stderr], [0], [ignore]) > + > + AT_CHECK([ovn-nbctl $2 --label=4294967296 acl-add $1 to-lport 50 ip allow-related], [1], [], [stderr]) > + AT_CHECK([grep 'label must in range 0...4294967295' stderr], [0], [ignore]) > + > AT_CHECK([ovn-nbctl $2 acl-list $1], [0], [dnl > from-lport 600 (udp) drop log() > from-lport 400 (tcp) drop > from-lport 200 (ip) drop > +from-lport 70 (icmp) allow-related label=1234 > to-lport 500 (udp) drop log(name=test,severity=info) > to-lport 300 (tcp) drop > to-lport 100 (ip) drop > + to-lport 70 (icmp) allow-related label=1235 > ]) > > dnl Delete in one direction. > @@ -231,6 +249,7 @@ from-lport 200 (ip) drop > from-lport 600 (udp) drop log() > from-lport 400 (tcp) drop > from-lport 200 (ip) drop > +from-lport 70 (icmp) allow-related label=1234 > ]) > > dnl Delete all ACLs. > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 474e88021..ba79106a1 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -3608,7 +3608,8 @@ check_stateful_flows() { > > AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl > table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) > - table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > table=12(ls_in_stateful ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg1 = 10.0.0.10; reg2[[0..15]] = 80; ct_lb(backends=10.0.0.4:8080);) > ]) > > @@ -3630,7 +3631,8 @@ check_stateful_flows() { > > AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl > table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) > - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > ]) > } > > @@ -3669,7 +3671,8 @@ AT_CHECK([grep "ls_in_pre_stateful" sw0flows | sort], [0], [dnl > > AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl > table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) > - table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > ]) > > AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl > @@ -3687,14 +3690,108 @@ AT_CHECK([grep "ls_out_pre_stateful" sw0flows | sort], [0], [dnl > > AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl > table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) > - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > ]) > > AT_CLEANUP > ]) > > OVN_FOR_EACH_NORTHD([ > -AT_SETUP([ct.inv usage]) > +AT_SETUP([ovn -- ACL label usage]) > +ovn_start > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl lsp-add sw0 sw0p1 > + > +check ovn-nbctl --wait=sb --label=1234 acl-add sw0 to-lport 1002 tcp allow-related > +check ovn-nbctl --wait=sb --label=1234 acl-add sw0 from-lport 1002 tcp allow-related > + > +ovn-sbctl dump-flows sw0 > sw0flows > +AT_CAPTURE_FILE([sw0flows]) > + > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort], [0], [dnl > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > +]) > +AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl > + table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > +]) > + > +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > +]) > +AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl > + table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > +]) > + > +# Add new ACL without label > +check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 udp allow-related > +check ovn-nbctl --wait=sb acl-add sw0 from-lport 1002 udp allow-related > + > +ovn-sbctl dump-flows sw0 > sw0flows > +AT_CAPTURE_FILE([sw0flows]) > + > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort], [0], [dnl > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) > +]) > +AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl > + table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > +]) > + > +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) > +]) > +AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl > + table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > +]) > + > +# Delete new ACL with label > +check ovn-nbctl --wait=sb acl-del sw0 to-lport 1002 tcp > +check ovn-nbctl --wait=sb acl-del sw0 from-lport 1002 tcp > + > +ovn-sbctl dump-flows sw0 > sw0flows > +AT_CAPTURE_FILE([sw0flows]) > + > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort], [0], [dnl > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) > +]) > +AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl > + table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > +]) > + > +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) > +]) > +AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl > + table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > +]) > +AT_CLEANUP > +]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn -- ct.inv usage]) > ovn_start > > check ovn-nbctl ls-add sw0 > diff --git a/tests/ovn.at b/tests/ovn.at > index a207f5e12..e51a94ad8 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -197,6 +197,7 @@ ct_label = NXM_NX_CT_LABEL > ct_label.blocked = ct_label[0] > ct_label.ecmp_reply_eth = ct_label[32..79] > ct_label.ecmp_reply_port = ct_label[80..95] > +ct_label.label = ct_label[96..127] > ct_label.natted = ct_label[1] > ct_mark = NXM_NX_CT_MARK > ct_state = NXM_NX_CT_STATE > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 4288d80e5..7d03bf3f0 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -6745,5 +6745,249 @@ OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > as > OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > /.*terminating with signal 15.*/d"]) > + > +AT_CLEANUP > +]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ACL label - conntrack ct_label]) > +AT_KEYWORDS([acl label ct_commit]) > + > +CHECK_CONNTRACK() > +ovn_start > + > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_BR([br-int]) > + > +# Set external-ids in br-int needed for ovn-controller > +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 > + > +# Start ovn-controller > +start_daemon ovn-controller > + > +check ovn-nbctl ls-add sw0 > + > +check ovn-nbctl lsp-add sw0 sw0-p1 > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:02 10.0.0.2" > +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:02 10.0.0.2" > + > +check ovn-nbctl lsp-add sw0 sw0-p2 > +check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:03 10.0.0.3" > +check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:03 10.0.0.3" > + > +check ovn-nbctl lsp-add sw0 sw0-p3 > +check ovn-nbctl lsp-set-addresses sw0-p3 "50:54:00:00:00:04 10.0.0.4" > +check ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:04 10.0.0.4" > + > +# ACLs > +# Case 1: sw0-p1 ---> sw0-p3 allowed, label=1234 > +# Case 2: sw0-p3 ---> sw0-p1 allowed, label=1235 > +# Case 3: sw0-p1 ---> sw0-p2 allowed, no label > +# Case 4: sw0-p2 ---> sw0-p1 allowed, no label > + > +check ovn-nbctl --label=1234 acl-add sw0 from-lport 1002 'ip4 && inport == "sw0-p1" && ip4.dst == 10.0.0.4' allow-related > +check ovn-nbctl --label=1235 acl-add sw0 to-lport 1002 'ip4 && outport == "sw0-p1" && ip4.src == 10.0.0.4' allow-related > +check ovn-nbctl acl-add sw0 from-lport 1001 "ip" allow-related > +check ovn-nbctl acl-add sw0 to-lport 1001 "ip" allow-related > + > + > +ADD_NAMESPACES(sw0-p1) > +ADD_VETH(sw0-p1, sw0-p1, br-int, "10.0.0.2/24", "50:54:00:00:00:02", \ > + "10.0.0.1") > +ADD_NAMESPACES(sw0-p2) > +ADD_VETH(sw0-p2, sw0-p2, br-int, "10.0.0.3/24", "50:54:00:00:00:03", \ > + "10.0.0.1") > +ADD_NAMESPACES(sw0-p3) > +ADD_VETH(sw0-p3, sw0-p3, br-int, "10.0.0.4/24", "50:54:00:00:00:04", \ > + "10.0.0.1") > + > +# Ensure ovn-controller is caught up > +ovn-nbctl --wait=hv sync > + > +on_exit 'ovn-nbctl acl-list sw0' > +on_exit 'ovn-sbctl lflow-list' > +on_exit 'ovs-ofctl dump-flows br-int' > + > +wait_for_ports_up > + > +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) > +# 'sw0-p1' should be able to ping 'sw0-p3'. > +NS_CHECK_EXEC([sw0-p1], [ping -q -c 10 -i 0.3 -w 15 10.0.0.4 | FORMAT_PING], \ > +[0], [dnl > +10 packets transmitted, 10 received, 0% packet loss, time 0ms > +]) > + > +# Ensure conntrack entry is present and ct_label is set. > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.4) | \ > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ > +sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2000000000000000000000000/'], [0], [dnl > +icmp,orig=(src=10.0.0.2,dst=10.0.0.4,id=<cleared>,type=8,code=0),reply=(src=10.0.0.4,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared>,labels=0x4d2000000000000000000000000 > +icmp,orig=(src=10.0.0.2,dst=10.0.0.4,id=<cleared>,type=8,code=0),reply=(src=10.0.0.4,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> > +]) > + > +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) > +# 'sw0-p3' should be able to ping 'sw0-p1'. > +NS_CHECK_EXEC([sw0-p3], [ping -q -c 10 -i 0.3 -w 15 10.0.0.2 | FORMAT_PING], \ > +[0], [dnl > +10 packets transmitted, 10 received, 0% packet loss, time 0ms > +]) > + > +# Ensure conntrack entry is present and ct_label is set. > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.2) | \ > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ > +sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3000000000000000000000000/'], [0], [dnl > +icmp,orig=(src=10.0.0.4,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.4,id=<cleared>,type=0,code=0),zone=<cleared>,labels=0x4d3000000000000000000000000 > +icmp,orig=(src=10.0.0.4,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.4,id=<cleared>,type=0,code=0),zone=<cleared> > +]) > + > +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) > +# 'sw0-p1' should be able to ping 'sw0-p2'. > +NS_CHECK_EXEC([sw0-p1], [ping -q -c 10 -i 0.3 -w 15 10.0.0.3 | FORMAT_PING], \ > +[0], [dnl > +10 packets transmitted, 10 received, 0% packet loss, time 0ms > +]) > + > +# Ensure conntrack entry is present and ct_label is not set. > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl > +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> > +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> > +]) > + > +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) > +# 'sw0-p2' should be able to ping 'sw0-p1'. > +NS_CHECK_EXEC([sw0-p2], [ping -q -c 10 -i 0.3 -w 15 10.0.0.2 | FORMAT_PING], \ > +[0], [dnl > +10 packets transmitted, 10 received, 0% packet loss, time 0ms > +]) > + > +# Ensure conntrack entry is present and ct_label is not set. > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.2) | \ > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl > +icmp,orig=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=0,code=0),zone=<cleared> > +icmp,orig=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=0,code=0),zone=<cleared> > +]) > + > +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([NORTHD_TYPE]) > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > +/connection dropped.*/d"]) > + > +AT_CLEANUP > +]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ACL label - conntrack label change]) > +AT_KEYWORDS([acl label ct_commit label change]) > + > +CHECK_CONNTRACK() > +ovn_start > + > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_BR([br-int]) > + > +# Set external-ids in br-int needed for ovn-controller > +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 > + > +# Start ovn-controller > +start_daemon ovn-controller > + > +check ovn-nbctl ls-add sw0 > + > +check ovn-nbctl lsp-add sw0 sw0-p1 > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:02 10.0.0.2" > +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:02 10.0.0.2" > + > +check ovn-nbctl lsp-add sw0 sw0-p2 > +check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:03 10.0.0.3" > +check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:03 10.0.0.3" > + > +# ACLs > +# sw0-p1 ---> sw0-p2 allowed, label=1234 > + > +check ovn-nbctl --label=1234 acl-add sw0 from-lport 1002 'ip4 && inport == "sw0-p1" && ip4.dst == 10.0.0.3' allow-related > + > +ADD_NAMESPACES(sw0-p1) > +ADD_VETH(sw0-p1, sw0-p1, br-int, "10.0.0.2/24", "50:54:00:00:00:02", \ > + "10.0.0.1") > +ADD_NAMESPACES(sw0-p2) > +ADD_VETH(sw0-p2, sw0-p2, br-int, "10.0.0.3/24", "50:54:00:00:00:03", \ > + "10.0.0.1") > + > +# Ensure ovn-controller is caught up > +ovn-nbctl --wait=hv sync > + > +on_exit 'ovn-nbctl acl-list sw0' > +on_exit 'ovn-sbctl lflow-list' > +on_exit 'ovs-ofctl dump-flows br-int' > + > +wait_for_ports_up > + > +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) > + > +# start a background ping for ~30 secs. > +NETNS_DAEMONIZE([sw0-p1], [[ping -q -c 100 -i 0.3 -w 15 10.0.0.3]], [ns-sw0-p1.pid]) > + > +sleep 3s > + > +# Ensure conntrack entry is present and ct_label is set. > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ > +sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2000000000000000000000000/'], [0], [dnl > +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared>,labels=0x4d2000000000000000000000000 > +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> > +]) > + > +# Add a higher priority ACL with different label. > +# This ACL also allows the ping running in background. > + > +check ovn-nbctl --label=1235 acl-add sw0 from-lport 1003 'ip4 && inport == "sw0-p1" && ip4.dst == 10.0.0.3' allow-related > +ovn-nbctl --wait=hv sync > + > +sleep 3s > + > +# Ensure conntrack entry is updated with new ct_label is set. > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ > +sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3000000000000000000000000/'], [0], [dnl > +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared>,labels=0x4d3000000000000000000000000 > +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> > +]) > + > +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([NORTHD_TYPE]) > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > +/connection dropped.*/d"]) > + > AT_CLEANUP > ]) > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml > index 987797860..80a564660 100644 > --- a/utilities/ovn-nbctl.8.xml > +++ b/utilities/ovn-nbctl.8.xml > @@ -399,7 +399,7 @@ > must be either <code>switch</code> or <code>port-group</code>. > </p> > <dl> > - <dt>[<code>--type=</code>{<code>switch</code> | <code>port-group</code>}] [<code>--log</code>] [<code>--meter=</code><var>meter</var>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--may-exist</code>] <code>acl-add</code> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt> > + <dt>[<code>--type=</code>{<code>switch</code> | <code>port-group</code>}] [<code>--log</code>] [<code>--meter=</code><var>meter</var>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--label=</code><var>label</var>] [<code>--may-exist</code>] <code>acl-add</code> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt> > <dd> > <p> > Adds the specified ACL to <var>entity</var>. <var>direction</var> > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index f41238990..ada53b662 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -2011,6 +2011,9 @@ nbctl_acl_list(struct ctl_context *ctx) > ds_chomp(&ctx->output, ','); > ds_put_cstr(&ctx->output, ")"); > } > + if (acl->label) { > + ds_put_format(&ctx->output, " label=%"PRId64, acl->label); > + } > ds_put_cstr(&ctx->output, "\n"); > } > > @@ -2069,6 +2072,19 @@ parse_priority(const char *arg, int64_t *priority_p) > return NULL; > } > > +static char * OVS_WARN_UNUSED_RESULT > +parse_acl_label(const char *arg, int64_t *label_p) > +{ > + /* Validate label. */ > + int64_t label; > + if (!ovs_scan(arg, "%"SCNd64, &label) > + || label < 0 || label > UINT32_MAX) { > + return xasprintf("%s: label must in range 0...4294967295", arg); > + } > + *label_p = label; > + return NULL; > +} > + > static void > nbctl_pre_acl(struct ctl_context *ctx) > { > @@ -2093,6 +2109,7 @@ nbctl_pre_acl_list(struct ctl_context *ctx) > ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_name); > ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_severity); > ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_meter); > + ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_label); > } > > static void > @@ -2160,6 +2177,25 @@ nbctl_acl_add(struct ctl_context *ctx) > nbrec_acl_set_meter(acl, meter); > } > > + /* Set the ACL label */ > + const char *label = shash_find_data(&ctx->options, "--label"); > + if (label) { > + /* Ensure that the action is either allow or allow-related */ > + if (strcmp(action, "allow") && strcmp(action, "allow-related")) { > + ctl_error(ctx, "label can only be set with actions \"allow\" or " > + "\"allow-related\""); > + return; > + } > + > + int64_t label_value = 0; > + error = parse_acl_label(label, &label_value); > + if (error) { > + ctx->error = error; > + return; > + } > + nbrec_acl_set_label(acl, label_value); > + } > + > /* Check if same acl already exists for the ls/portgroup */ > size_t n_acls = pg ? pg->n_acls : ls->n_acls; > struct nbrec_acl **acls = pg ? pg->acls : ls->acls; > @@ -6757,7 +6793,7 @@ static const struct ctl_command_syntax nbctl_commands[] = { > /* acl commands. */ > { "acl-add", 5, 6, "{SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION", > nbctl_pre_acl, nbctl_acl_add, NULL, > - "--log,--may-exist,--type=,--name=,--severity=,--meter=", RW }, > + "--log,--may-exist,--type=,--name=,--severity=,--meter=,--label=", RW }, > { "acl-del", 1, 4, "{SWITCH | PORTGROUP} [DIRECTION [PRIORITY MATCH]]", > nbctl_pre_acl, nbctl_acl_del, NULL, "--type=", RW }, > { "acl-list", 1, 1, "{SWITCH | PORTGROUP}", >
On Wed, Aug 11, 2021 at 11:34 AM Priyankar Jain <priyankar.jain@nutanix.com> wrote: > > Hi Numan, > > In case the patch looks good to you, can you please merge it in master? Done. Applied to the main branch. Thanks Numan > > Thanks and Regards, > Priyankar Jain > > On 02/08/21 2:02 pm, Priyankar Jain wrote: > > Allow adding label to an ACL to identify which ACL allowed a particular > > flow in the connection tracking table. > > > > The ACL label covers 32 bits at the end of ct_label. Since only allowed > > connections are committed, only "allow" and "allow-related" ACLs can > > have the label. > > > > If the ACL allowing the connection changes, the label associated with the > > new ACL gets updated in the ct_label field. This is achieved by committing > > every packet that hits the ACL with the label to the connection tracking > > table. > > In case the new ACL doesn't have a label, the ct_label field is not > > cleared. This is done to prevent any performance change with ACLs that > > don't have label set. > > For the packets which hits an ACL without label, the behaviour remains the > > same as before with respect to the conntrack commit. > > > > Performance: > > We used ftrace to measure the time taken by an extra conntrack commit for > > the packets hitting the ACL with label. We measured the time taken to > > execute the ovs_ct_execute call inside the sock_sendmsg call. > > > > ACL used :- > > from-lport 2000 (tcp && ip4.src == 10.0.0.11 && ip4.dst == 10.0.0.12) allow-related --label=1234 > > > > It was observed that the extra ovs_ct_execute call accounted for 1-2% > > of the round trip time (sock_sendmsg duration). The actual percentage > > is expected to be lesser since it doesn't take into account the tracing > > overhead which is substantial for smaller functions. > > > > Signed-off-by: Priyankar Jain <priyankar.jain@nutanix.com> > > Acked-by: Numan Siddique <numans@ovn.org> > > --- > > > > v3 -> v4 > > * Resolved merge conflicts with master. > > > > v2 -> v3 > > * Added NEWS entry. > > > > v1 -> v2 > > * Changed type of label from string (hex) to unsigned integer. > > * Added two system tests for checking the correct ct_label programming. > > * Updated ovn-northd.8.xml with the new logical flows added as part of > > this patch. > > * Rebased with latest master. > > > > NEWS | 3 + > > lib/logical-fields.c | 2 + > > northd/ovn-northd.8.xml | 26 +++- > > northd/ovn-northd.c | 53 +++++++-- > > northd/ovn_northd.dl | 76 +++++++++--- > > ovn-nb.ovsschema | 7 +- > > ovn-nb.xml | 12 ++ > > tests/ovn-nbctl.at | 19 +++ > > tests/ovn-northd.at | 107 ++++++++++++++++- > > tests/ovn.at | 1 + > > tests/system-ovn.at | 244 ++++++++++++++++++++++++++++++++++++++ > > utilities/ovn-nbctl.8.xml | 2 +- > > utilities/ovn-nbctl.c | 38 +++++- > > 13 files changed, 548 insertions(+), 42 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index f328666da..45dc45353 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -3,6 +3,9 @@ Post-v21.06.0 > > - Added Control Plane Protection support (control plane traffic metering). > > - Added path MTU discovery for ingress traffic originated outside of the > > cluster. > > + - Introduced a new "label" field for "allow" and "allow-related" ACLs > > + which helps in debugging/backtracking the ACL which allowed a particular > > + connection. > > > > OVN v21.06.0 - 18 Jun 2021 > > ------------------------- > > diff --git a/lib/logical-fields.c b/lib/logical-fields.c > > index 72853013e..7b3d431e0 100644 > > --- a/lib/logical-fields.c > > +++ b/lib/logical-fields.c > > @@ -146,6 +146,8 @@ ovn_init_symtab(struct shash *symtab) > > "ct_label[32..79]", WR_CT_COMMIT); > > expr_symtab_add_subfield_scoped(symtab, "ct_label.ecmp_reply_port", NULL, > > "ct_label[80..95]", WR_CT_COMMIT); > > + expr_symtab_add_subfield_scoped(symtab, "ct_label.label", NULL, > > + "ct_label[96..127]", WR_CT_COMMIT); > > > > expr_symtab_add_field(symtab, "ct_state", MFF_CT_STATE, NULL, false); > > > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index 08d484760..c5ea15774 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -670,13 +670,19 @@ > > the <code>next;</code> action. If there are any stateful ACLs > > on this datapath, then <code>allow</code> ACLs translate to > > <code>ct_commit; next;</code> (which acts as a hint for the next tables > > - to commit the connection to conntrack), > > + to commit the connection to conntrack). In case the <code>ACL</code> > > + has a label then <code>reg3</code> is loaded with the label value and > > + <code>reg0[13]</code> bit is set to 1 (which acts as a hint for the > > + next tables to commit the label to conntrack). > > </li> > > <li> > > <code>allow-related</code> ACLs translate into logical > > flows with the <code>ct_commit(ct_label=0/1); next;</code> actions > > for new connections and <code>reg0[1] = 1; next;</code> for existing > > - connections. > > + connections. In case the <code>ACL</code> has a label then > > + <code>reg3</code> is loaded with the label value and > > + <code>reg0[13]</code> bit is set to 1 (which acts as a hint for the > > + next tables to commit the label to conntrack). > > </li> > > <li> > > <code>allow-stateless</code> ACLs translate into logical > > @@ -876,9 +882,19 @@ > > </li> > > > > <li> > > - A priority-100 flow commits packets to connection tracker using > > - <code>ct_commit; next;</code> action based on a hint provided by > > - the previous tables (with a match for <code>reg0[1] == 1</code>). > > + A priority 100 flow is added which commits the packet to the conntrack > > + and sets the most significant 32-bits of <code>ct_label</code> with the > > + <code>reg3</code> value based on the hint provided by previous tables > > + (with a match for <code>reg0[1] == 1 && reg0[13] == 1</code>). > > + This is used by the <code>ACLs</code> with label to commit the label > > + value to conntrack. > > + </li> > > + > > + <li> > > + For <code>ACLs</code> without label, a second priority-100 flow commits > > + packets to connection tracker using <code>ct_commit; next;</code> > > + action based on a hint provided by the previous tables (with a match > > + for <code>reg0[1] == 1 && reg0[13] == 0</code>). > > </li> > > <li> > > A priority-0 flow that simply moves traffic to the next table. > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index a0eaa1247..1ad15b978 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -241,6 +241,7 @@ enum ovn_stage { > > #define REGBIT_ACL_HINT_BLOCK "reg0[10]" > > #define REGBIT_LKUP_FDB "reg0[11]" > > #define REGBIT_HAIRPIN_REPLY "reg0[12]" > > +#define REGBIT_ACL_LABEL "reg0[13]" > > > > #define REG_ORIG_DIP_IPV4 "reg1" > > #define REG_ORIG_DIP_IPV6 "xxreg1" > > @@ -273,6 +274,9 @@ enum ovn_stage { > > #define REG_SRC_IPV4 "reg1" > > #define REG_SRC_IPV6 "xxreg1" > > > > +/* Register used for setting a label for ACLs in a Logical Switch. */ > > +#define REG_LABEL "reg3" > > + > > #define FLAGBIT_NOT_VXLAN "flags[1] == 0" > > > > /* > > @@ -281,14 +285,15 @@ enum ovn_stage { > > * Logical Switch pipeline: > > * +----+----------------------------------------------+---+------------------+ > > * | R0 | REGBIT_{CONNTRACK/DHCP/DNS} | | | > > - * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | X | | > > - * | | REGBIT_ACL_HINT_{ALLOW_NEW/ALLOW/DROP/BLOCK} | X | | > > + * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | | | > > + * | | REGBIT_ACL_HINT_{ALLOW_NEW/ALLOW/DROP/BLOCK} | | | > > + * | | REGBIT_ACL_LABEL | X | | > > * +----+----------------------------------------------+ X | | > > * | R1 | ORIG_DIP_IPV4 (>= IN_STATEFUL) | R | | > > * +----+----------------------------------------------+ E | | > > * | R2 | ORIG_TP_DPORT (>= IN_STATEFUL) | G | | > > * +----+----------------------------------------------+ 0 | | > > - * | R3 | UNUSED | | | > > + * | R3 | ACL LABEL | | | > > * +----+----------------------------------------------+---+------------------+ > > * | R4 | UNUSED | | | > > * +----+----------------------------------------------+ X | ORIG_DIP_IPV6 | > > @@ -5774,7 +5779,12 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > > ds_clear(actions); > > ds_put_format(match, REGBIT_ACL_HINT_ALLOW_NEW " == 1 && (%s)", > > acl->match); > > + > > ds_put_cstr(actions, REGBIT_CONNTRACK_COMMIT" = 1; "); > > + if (acl->label) { > > + ds_put_format(actions, REGBIT_ACL_LABEL" = 1; " > > + REG_LABEL" = %"PRId64"; ", acl->label); > > + } > > build_acl_log(actions, acl, meter_groups); > > ds_put_cstr(actions, "next;"); > > ovn_lflow_add_with_hint(lflows, od, stage, > > @@ -5785,15 +5795,21 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > > > > /* Match on traffic in the request direction for an established > > * connection tracking entry that has not been marked for > > - * deletion. There is no need to commit here, so we can just > > - * proceed to the next table. We use this to ensure that this > > + * deletion. We use this to ensure that this > > * connection is still allowed by the currently defined > > - * policy. Match untracked packets too. */ > > + * policy. Match untracked packets too. > > + * Commit the connection only if the ACL has a label. This is done > > + * to update the connection tracking entry label in case the ACL > > + * allowing the connection changes. */ > > ds_clear(match); > > ds_clear(actions); > > ds_put_format(match, REGBIT_ACL_HINT_ALLOW " == 1 && (%s)", > > acl->match); > > - > > + if (acl->label) { > > + ds_put_cstr(actions, REGBIT_CONNTRACK_COMMIT" = 1; "); > > + ds_put_format(actions, REGBIT_ACL_LABEL" = 1; " > > + REG_LABEL" = %"PRId64"; ", acl->label); > > + } > > build_acl_log(actions, acl, meter_groups); > > ds_put_cstr(actions, "next;"); > > ovn_lflow_add_with_hint(lflows, od, stage, > > @@ -6292,15 +6308,34 @@ build_stateful(struct ovn_datapath *od, struct hmap *lflows) > > ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 0, "1", "next;"); > > ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 0, "1", "next;"); > > > > + /* If REGBIT_CONNTRACK_COMMIT is set as 1 and > > + * REGBIT_CONNTRACK_SET_LABEL is set to 1, then the packets should be > > + * committed to conntrack. > > + * We always set ct_mark.blocked to 0 here as > > + * any packet that makes it this far is part of a connection we > > + * want to allow to continue. */ > > + ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, > > + REGBIT_CONNTRACK_COMMIT" == 1 && " > > + REGBIT_ACL_LABEL" == 1", > > + "ct_commit { ct_label.blocked = 0; " > > + "ct_label.label = " REG_LABEL "; }; next;"); > > + ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100, > > + REGBIT_CONNTRACK_COMMIT" == 1 && " > > + REGBIT_ACL_LABEL" == 1", > > + "ct_commit { ct_label.blocked = 0; " > > + "ct_label.label = " REG_LABEL "; }; next;"); > > + > > /* If REGBIT_CONNTRACK_COMMIT is set as 1, then the packets should be > > * committed to conntrack. We always set ct_label.blocked to 0 here as > > * any packet that makes it this far is part of a connection we > > * want to allow to continue. */ > > ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, > > - REGBIT_CONNTRACK_COMMIT" == 1", > > + REGBIT_CONNTRACK_COMMIT" == 1 && " > > + REGBIT_ACL_LABEL" == 0", > > "ct_commit { ct_label.blocked = 0; }; next;"); > > ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100, > > - REGBIT_CONNTRACK_COMMIT" == 1", > > + REGBIT_CONNTRACK_COMMIT" == 1 && " > > + REGBIT_ACL_LABEL" == 0", > > "ct_commit { ct_label.blocked = 0; }; next;"); > > } > > > > diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl > > index 091fe10b3..b3d1a9657 100644 > > --- a/northd/ovn_northd.dl > > +++ b/northd/ovn_northd.dl > > @@ -1505,13 +1505,14 @@ function s_ROUTER_OUT_DELIVERY(): Stage { Stage{ Egress, 4, "lr_out_deliv > > * Logical Switch pipeline: > > * +----+----------------------------------------------+---+------------------+ > > * | R0 | REGBIT_{CONNTRACK/DHCP/DNS} | | | > > - * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | X | | > > + * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | | | > > + * | | REGBIT_ACL_LABEL | X | | > > * +----+----------------------------------------------+ X | | > > * | R1 | ORIG_DIP_IPV4 (>= IN_STATEFUL) | R | | > > * +----+----------------------------------------------+ E | | > > * | R2 | ORIG_TP_DPORT (>= IN_STATEFUL) | G | | > > * +----+----------------------------------------------+ 0 | | > > - * | R3 | UNUSED | | | > > + * | R3 | ACL_LABEL | | | > > * +----+----------------------------------------------+---+------------------+ > > * | R4 | UNUSED | | | > > * +----+----------------------------------------------+ X | ORIG_DIP_IPV6 | > > @@ -1584,6 +1585,7 @@ function rEGBIT_ACL_HINT_DROP() : string = "reg0[9]" > > function rEGBIT_ACL_HINT_BLOCK() : string = "reg0[10]" > > function rEGBIT_LKUP_FDB() : string = "reg0[11]" > > function rEGBIT_HAIRPIN_REPLY() : string = "reg0[12]" > > +function rEGBIT_ACL_LABEL() : string = "reg0[13]" > > > > function rEG_ORIG_DIP_IPV4() : string = "reg1" > > function rEG_ORIG_DIP_IPV6() : string = "xxreg1" > > @@ -1610,6 +1612,9 @@ function rEG_INPORT_ETH_ADDR() : string = "xreg0[0..47]" > > function rEG_ECMP_GROUP_ID() : string = "reg8[0..15]" > > function rEG_ECMP_MEMBER_ID() : string = "reg8[16..31]" > > > > +/* Register used for setting a label for ACLs in a Logical Switch. */ > > +function rEG_LABEL() : string = "reg3" > > + > > function fLAGBIT_NOT_VXLAN() : string = "flags[1] == 0" > > > > function mFF_N_LOG_REGS() : bit<32> = 10 > > @@ -2698,26 +2703,41 @@ for (&SwitchACL(.sw = sw, .acl = acl, .has_fair_meter = fair_meter)) { > > * that case here and un-set ct_label.blocked (which will be done > > * by ct_commit in the "stateful" stage) to indicate that the > > * connection should be allowed to resume. > > + * If the ACL has a label, then load REG_LABEL with the label and > > + * set the REGBIT_ACL_LABEL field. > > */ > > - Flow(.logical_datapath = sw._uuid, > > - .stage = stage, > > - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > > - .__match = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})", > > - .actions = "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${acl_log}next;", > > - .external_ids = stage_hint); > > + var __action = if (acl.label != 0) { > > + "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${rEGBIT_ACL_LABEL()} = 1;\ > > + \ ${rEG_LABEL()} = ${acl.label}; ${acl_log}next;" > > + } else { > > + "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${acl_log}next;" > > + } in Flow(.logical_datapath = sw._uuid, > > + .stage = stage, > > + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > > + .__match = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})", > > + .actions = __action, > > + .external_ids = stage_hint); > > > > /* Match on traffic in the request direction for an established > > * connection tracking entry that has not been marked for > > - * deletion. There is no need to commit here, so we can just > > - * proceed to the next table. We use this to ensure that this > > + * deletion. We use this to ensure that this > > * connection is still allowed by the currently defined > > - * policy. Match untracked packets too. */ > > - Flow(.logical_datapath = sw._uuid, > > - .stage = stage, > > - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > > - .__match = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})", > > - .actions = "${acl_log}next;", > > - .external_ids = stage_hint) > > + * policy. Match untracked packets too. > > + * Commit the connection only if the ACL has a label. This is done to > > + * update the connection tracking entry label in case the ACL > > + * allowing the connection changes. > > + */ > > + var __action = if (acl.label != 0) { > > + "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${rEGBIT_ACL_LABEL()} = 1;\ > > + \ ${rEG_LABEL()} = ${acl.label}; ${acl_log}next;" > > + } else { > > + "${acl_log}next;" > > + } in Flow(.logical_datapath = sw._uuid, > > + .stage = stage, > > + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), > > + .__match = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})", > > + .actions = __action, > > + .external_ids = stage_hint) > > } > > } else if (acl.action == "allow-stateless") { > > Flow(.logical_datapath = sw._uuid, > > @@ -2934,6 +2954,24 @@ for (&Switch(._uuid = ls_uuid)) { > > .actions = "next;", > > .external_ids = map_empty()); > > > > + /* If REGBIT_CONNTRACK_COMMIT is set as 1 and REGBIT_CONNTRACK_SET_LABEL > > + * is set to 1, then the packets should be > > + * committed to conntrack. We always set ct_label.blocked to 0 here as > > + * any packet that makes it this far is part of a connection we > > + * want to allow to continue. */ > > + Flow(.logical_datapath = ls_uuid, > > + .stage = s_SWITCH_IN_STATEFUL(), > > + .priority = 100, > > + .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 1", > > + .actions = "ct_commit { ct_label.blocked = 0; ct_label.label = ${rEG_LABEL()}; }; next;", > > + .external_ids = map_empty()); > > + Flow(.logical_datapath = ls_uuid, > > + .stage = s_SWITCH_OUT_STATEFUL(), > > + .priority = 100, > > + .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 1", > > + .actions = "ct_commit { ct_label.blocked = 0; ct_label.label = ${rEG_LABEL()}; }; next;", > > + .external_ids = map_empty()); > > + > > /* If REGBIT_CONNTRACK_COMMIT is set as 1, then the packets should be > > * committed to conntrack. We always set ct_label.blocked to 0 here as > > * any packet that makes it this far is part of a connection we > > @@ -2941,13 +2979,13 @@ for (&Switch(._uuid = ls_uuid)) { > > Flow(.logical_datapath = ls_uuid, > > .stage = s_SWITCH_IN_STATEFUL(), > > .priority = 100, > > - .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1", > > + .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 0", > > .actions = "ct_commit { ct_label.blocked = 0; }; next;", > > .external_ids = map_empty()); > > Flow(.logical_datapath = ls_uuid, > > .stage = s_SWITCH_OUT_STATEFUL(), > > .priority = 100, > > - .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1", > > + .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 0", > > .actions = "ct_commit { ct_label.blocked = 0; }; next;", > > .external_ids = map_empty()) > > } > > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema > > index a53d5e851..2ac8ef3ea 100644 > > --- a/ovn-nb.ovsschema > > +++ b/ovn-nb.ovsschema > > @@ -1,7 +1,7 @@ > > { > > "name": "OVN_Northbound", > > - "version": "5.32.0", > > - "cksum": "2501921026 29540", > > + "version": "5.32.1", > > + "cksum": "2805328215 29734", > > "tables": { > > "NB_Global": { > > "columns": { > > @@ -244,6 +244,9 @@ > > "debug"]]}, > > "min": 0, "max": 1}}, > > "meter": {"type": {"key": "string", "min": 0, "max": 1}}, > > + "label": {"type": {"key": {"type": "integer", > > + "minInteger": 0, > > + "maxInteger": 4294967295}}}, > > "external_ids": { > > "type": {"key": "string", "value": "string", > > "min": 0, "max": "unlimited"}}}, > > diff --git a/ovn-nb.xml b/ovn-nb.xml > > index c1176e81f..ebafa3e5e 100644 > > --- a/ovn-nb.xml > > +++ b/ovn-nb.xml > > @@ -1853,6 +1853,18 @@ > > and <code>deny</code> as <ref column="action"/>.) > > </p> > > > > + <column name="label"> > > + <p> > > + Associates an identifier with the ACL. > > + The same value will be written to corresponding connection > > + tracker entry. The value should be a valid 32-bit unsigned integer. > > + This value can help in debugging from connection tracker side. > > + For example, through this "label" we can backtrack to the ACL rule > > + which is causing a "leaked" connection. Connection tracker entries are > > + created only for allowed connections so the label is valid only > > + for allow and allow-related actions. > > + </p> > > + </column> > > <column name="priority"> > > <p> > > The ACL rule's priority. Rules with numerically higher priority > > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > > index 828777b82..4445ab7c0 100644 > > --- a/tests/ovn-nbctl.at > > +++ b/tests/ovn-nbctl.at > > @@ -211,18 +211,36 @@ ovn_nbctl_test_acl() { > > AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 300 tcp drop]) > > AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 200 ip drop]) > > AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop]) > > + AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 from-lport 70 icmp allow-related]) > > + AT_CHECK([ovn-nbctl $2 --label=1235 acl-add $1 to-lport 70 icmp allow-related]) > > + > > dnl Add duplicated ACL > > AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop], [1], [], [stderr]) > > AT_CHECK([grep 'already existed' stderr], [0], [ignore]) > > AT_CHECK([ovn-nbctl $2 --may-exist acl-add $1 to-lport 100 ip drop]) > > > > + dnl Add invalid ACL label > > + AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 to-lport 50 ip drop], [1], [], [stderr]) > > + AT_CHECK([grep 'can only be set with actions' stderr], [0], [ignore]) > > + > > + AT_CHECK([ovn-nbctl $2 --label=abcd acl-add $1 to-lport 50 ip allow-related], [1], [], [stderr]) > > + AT_CHECK([grep 'label must in range 0...4294967295' stderr], [0], [ignore]) > > + > > + AT_CHECK([ovn-nbctl $2 --label=-1 acl-add $1 to-lport 50 ip allow-related], [1], [], [stderr]) > > + AT_CHECK([grep 'label must in range 0...4294967295' stderr], [0], [ignore]) > > + > > + AT_CHECK([ovn-nbctl $2 --label=4294967296 acl-add $1 to-lport 50 ip allow-related], [1], [], [stderr]) > > + AT_CHECK([grep 'label must in range 0...4294967295' stderr], [0], [ignore]) > > + > > AT_CHECK([ovn-nbctl $2 acl-list $1], [0], [dnl > > from-lport 600 (udp) drop log() > > from-lport 400 (tcp) drop > > from-lport 200 (ip) drop > > +from-lport 70 (icmp) allow-related label=1234 > > to-lport 500 (udp) drop log(name=test,severity=info) > > to-lport 300 (tcp) drop > > to-lport 100 (ip) drop > > + to-lport 70 (icmp) allow-related label=1235 > > ]) > > > > dnl Delete in one direction. > > @@ -231,6 +249,7 @@ from-lport 200 (ip) drop > > from-lport 600 (udp) drop log() > > from-lport 400 (tcp) drop > > from-lport 200 (ip) drop > > +from-lport 70 (icmp) allow-related label=1234 > > ]) > > > > dnl Delete all ACLs. > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 474e88021..ba79106a1 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -3608,7 +3608,8 @@ check_stateful_flows() { > > > > AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl > > table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) > > - table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > > table=12(ls_in_stateful ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg1 = 10.0.0.10; reg2[[0..15]] = 80; ct_lb(backends=10.0.0.4:8080);) > > ]) > > > > @@ -3630,7 +3631,8 @@ check_stateful_flows() { > > > > AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl > > table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) > > - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > > ]) > > } > > > > @@ -3669,7 +3671,8 @@ AT_CHECK([grep "ls_in_pre_stateful" sw0flows | sort], [0], [dnl > > > > AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl > > table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) > > - table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > > ]) > > > > AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl > > @@ -3687,14 +3690,108 @@ AT_CHECK([grep "ls_out_pre_stateful" sw0flows | sort], [0], [dnl > > > > AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl > > table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) > > - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > > ]) > > > > AT_CLEANUP > > ]) > > > > OVN_FOR_EACH_NORTHD([ > > -AT_SETUP([ct.inv usage]) > > +AT_SETUP([ovn -- ACL label usage]) > > +ovn_start > > + > > +check ovn-nbctl ls-add sw0 > > +check ovn-nbctl lsp-add sw0 sw0p1 > > + > > +check ovn-nbctl --wait=sb --label=1234 acl-add sw0 to-lport 1002 tcp allow-related > > +check ovn-nbctl --wait=sb --label=1234 acl-add sw0 from-lport 1002 tcp allow-related > > + > > +ovn-sbctl dump-flows sw0 > sw0flows > > +AT_CAPTURE_FILE([sw0flows]) > > + > > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort], [0], [dnl > > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > > +]) > > +AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl > > + table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) > > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > > +]) > > + > > +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl > > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > > +]) > > +AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl > > + table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) > > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > > +]) > > + > > +# Add new ACL without label > > +check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 udp allow-related > > +check ovn-nbctl --wait=sb acl-add sw0 from-lport 1002 udp allow-related > > + > > +ovn-sbctl dump-flows sw0 > sw0flows > > +AT_CAPTURE_FILE([sw0flows]) > > + > > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort], [0], [dnl > > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) > > +]) > > +AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl > > + table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) > > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > > +]) > > + > > +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl > > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) > > +]) > > +AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl > > + table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) > > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > > +]) > > + > > +# Delete new ACL with label > > +check ovn-nbctl --wait=sb acl-del sw0 to-lport 1002 tcp > > +check ovn-nbctl --wait=sb acl-del sw0 from-lport 1002 tcp > > + > > +ovn-sbctl dump-flows sw0 > sw0flows > > +AT_CAPTURE_FILE([sw0flows]) > > + > > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort], [0], [dnl > > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > > + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) > > +]) > > +AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl > > + table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) > > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > > +]) > > + > > +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl > > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > > + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) > > +]) > > +AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl > > + table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) > > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) > > + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) > > +]) > > +AT_CLEANUP > > +]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn -- ct.inv usage]) > > ovn_start > > > > check ovn-nbctl ls-add sw0 > > diff --git a/tests/ovn.at b/tests/ovn.at > > index a207f5e12..e51a94ad8 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -197,6 +197,7 @@ ct_label = NXM_NX_CT_LABEL > > ct_label.blocked = ct_label[0] > > ct_label.ecmp_reply_eth = ct_label[32..79] > > ct_label.ecmp_reply_port = ct_label[80..95] > > +ct_label.label = ct_label[96..127] > > ct_label.natted = ct_label[1] > > ct_mark = NXM_NX_CT_MARK > > ct_state = NXM_NX_CT_STATE > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index 4288d80e5..7d03bf3f0 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -6745,5 +6745,249 @@ OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > > as > > OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > > /.*terminating with signal 15.*/d"]) > > + > > +AT_CLEANUP > > +]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ACL label - conntrack ct_label]) > > +AT_KEYWORDS([acl label ct_commit]) > > + > > +CHECK_CONNTRACK() > > +ovn_start > > + > > +OVS_TRAFFIC_VSWITCHD_START() > > +ADD_BR([br-int]) > > + > > +# Set external-ids in br-int needed for ovn-controller > > +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 > > + > > +# Start ovn-controller > > +start_daemon ovn-controller > > + > > +check ovn-nbctl ls-add sw0 > > + > > +check ovn-nbctl lsp-add sw0 sw0-p1 > > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:02 10.0.0.2" > > +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:02 10.0.0.2" > > + > > +check ovn-nbctl lsp-add sw0 sw0-p2 > > +check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:03 10.0.0.3" > > +check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:03 10.0.0.3" > > + > > +check ovn-nbctl lsp-add sw0 sw0-p3 > > +check ovn-nbctl lsp-set-addresses sw0-p3 "50:54:00:00:00:04 10.0.0.4" > > +check ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:04 10.0.0.4" > > + > > +# ACLs > > +# Case 1: sw0-p1 ---> sw0-p3 allowed, label=1234 > > +# Case 2: sw0-p3 ---> sw0-p1 allowed, label=1235 > > +# Case 3: sw0-p1 ---> sw0-p2 allowed, no label > > +# Case 4: sw0-p2 ---> sw0-p1 allowed, no label > > + > > +check ovn-nbctl --label=1234 acl-add sw0 from-lport 1002 'ip4 && inport == "sw0-p1" && ip4.dst == 10.0.0.4' allow-related > > +check ovn-nbctl --label=1235 acl-add sw0 to-lport 1002 'ip4 && outport == "sw0-p1" && ip4.src == 10.0.0.4' allow-related > > +check ovn-nbctl acl-add sw0 from-lport 1001 "ip" allow-related > > +check ovn-nbctl acl-add sw0 to-lport 1001 "ip" allow-related > > + > > + > > +ADD_NAMESPACES(sw0-p1) > > +ADD_VETH(sw0-p1, sw0-p1, br-int, "10.0.0.2/24", "50:54:00:00:00:02", \ > > + "10.0.0.1") > > +ADD_NAMESPACES(sw0-p2) > > +ADD_VETH(sw0-p2, sw0-p2, br-int, "10.0.0.3/24", "50:54:00:00:00:03", \ > > + "10.0.0.1") > > +ADD_NAMESPACES(sw0-p3) > > +ADD_VETH(sw0-p3, sw0-p3, br-int, "10.0.0.4/24", "50:54:00:00:00:04", \ > > + "10.0.0.1") > > + > > +# Ensure ovn-controller is caught up > > +ovn-nbctl --wait=hv sync > > + > > +on_exit 'ovn-nbctl acl-list sw0' > > +on_exit 'ovn-sbctl lflow-list' > > +on_exit 'ovs-ofctl dump-flows br-int' > > + > > +wait_for_ports_up > > + > > +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) > > +# 'sw0-p1' should be able to ping 'sw0-p3'. > > +NS_CHECK_EXEC([sw0-p1], [ping -q -c 10 -i 0.3 -w 15 10.0.0.4 | FORMAT_PING], \ > > +[0], [dnl > > +10 packets transmitted, 10 received, 0% packet loss, time 0ms > > +]) > > + > > +# Ensure conntrack entry is present and ct_label is set. > > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.4) | \ > > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ > > +sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2000000000000000000000000/'], [0], [dnl > > +icmp,orig=(src=10.0.0.2,dst=10.0.0.4,id=<cleared>,type=8,code=0),reply=(src=10.0.0.4,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared>,labels=0x4d2000000000000000000000000 > > +icmp,orig=(src=10.0.0.2,dst=10.0.0.4,id=<cleared>,type=8,code=0),reply=(src=10.0.0.4,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> > > +]) > > + > > +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) > > +# 'sw0-p3' should be able to ping 'sw0-p1'. > > +NS_CHECK_EXEC([sw0-p3], [ping -q -c 10 -i 0.3 -w 15 10.0.0.2 | FORMAT_PING], \ > > +[0], [dnl > > +10 packets transmitted, 10 received, 0% packet loss, time 0ms > > +]) > > + > > +# Ensure conntrack entry is present and ct_label is set. > > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.2) | \ > > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ > > +sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3000000000000000000000000/'], [0], [dnl > > +icmp,orig=(src=10.0.0.4,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.4,id=<cleared>,type=0,code=0),zone=<cleared>,labels=0x4d3000000000000000000000000 > > +icmp,orig=(src=10.0.0.4,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.4,id=<cleared>,type=0,code=0),zone=<cleared> > > +]) > > + > > +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) > > +# 'sw0-p1' should be able to ping 'sw0-p2'. > > +NS_CHECK_EXEC([sw0-p1], [ping -q -c 10 -i 0.3 -w 15 10.0.0.3 | FORMAT_PING], \ > > +[0], [dnl > > +10 packets transmitted, 10 received, 0% packet loss, time 0ms > > +]) > > + > > +# Ensure conntrack entry is present and ct_label is not set. > > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ > > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl > > +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> > > +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> > > +]) > > + > > +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) > > +# 'sw0-p2' should be able to ping 'sw0-p1'. > > +NS_CHECK_EXEC([sw0-p2], [ping -q -c 10 -i 0.3 -w 15 10.0.0.2 | FORMAT_PING], \ > > +[0], [dnl > > +10 packets transmitted, 10 received, 0% packet loss, time 0ms > > +]) > > + > > +# Ensure conntrack entry is present and ct_label is not set. > > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.2) | \ > > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl > > +icmp,orig=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=0,code=0),zone=<cleared> > > +icmp,orig=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=0,code=0),zone=<cleared> > > +]) > > + > > +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([NORTHD_TYPE]) > > + > > +as > > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > > +/connection dropped.*/d"]) > > + > > +AT_CLEANUP > > +]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ACL label - conntrack label change]) > > +AT_KEYWORDS([acl label ct_commit label change]) > > + > > +CHECK_CONNTRACK() > > +ovn_start > > + > > +OVS_TRAFFIC_VSWITCHD_START() > > +ADD_BR([br-int]) > > + > > +# Set external-ids in br-int needed for ovn-controller > > +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 > > + > > +# Start ovn-controller > > +start_daemon ovn-controller > > + > > +check ovn-nbctl ls-add sw0 > > + > > +check ovn-nbctl lsp-add sw0 sw0-p1 > > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:02 10.0.0.2" > > +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:02 10.0.0.2" > > + > > +check ovn-nbctl lsp-add sw0 sw0-p2 > > +check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:03 10.0.0.3" > > +check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:03 10.0.0.3" > > + > > +# ACLs > > +# sw0-p1 ---> sw0-p2 allowed, label=1234 > > + > > +check ovn-nbctl --label=1234 acl-add sw0 from-lport 1002 'ip4 && inport == "sw0-p1" && ip4.dst == 10.0.0.3' allow-related > > + > > +ADD_NAMESPACES(sw0-p1) > > +ADD_VETH(sw0-p1, sw0-p1, br-int, "10.0.0.2/24", "50:54:00:00:00:02", \ > > + "10.0.0.1") > > +ADD_NAMESPACES(sw0-p2) > > +ADD_VETH(sw0-p2, sw0-p2, br-int, "10.0.0.3/24", "50:54:00:00:00:03", \ > > + "10.0.0.1") > > + > > +# Ensure ovn-controller is caught up > > +ovn-nbctl --wait=hv sync > > + > > +on_exit 'ovn-nbctl acl-list sw0' > > +on_exit 'ovn-sbctl lflow-list' > > +on_exit 'ovs-ofctl dump-flows br-int' > > + > > +wait_for_ports_up > > + > > +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) > > + > > +# start a background ping for ~30 secs. > > +NETNS_DAEMONIZE([sw0-p1], [[ping -q -c 100 -i 0.3 -w 15 10.0.0.3]], [ns-sw0-p1.pid]) > > + > > +sleep 3s > > + > > +# Ensure conntrack entry is present and ct_label is set. > > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ > > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ > > +sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2000000000000000000000000/'], [0], [dnl > > +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared>,labels=0x4d2000000000000000000000000 > > +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> > > +]) > > + > > +# Add a higher priority ACL with different label. > > +# This ACL also allows the ping running in background. > > + > > +check ovn-nbctl --label=1235 acl-add sw0 from-lport 1003 'ip4 && inport == "sw0-p1" && ip4.dst == 10.0.0.3' allow-related > > +ovn-nbctl --wait=hv sync > > + > > +sleep 3s > > + > > +# Ensure conntrack entry is updated with new ct_label is set. > > +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ > > +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ > > +sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3000000000000000000000000/'], [0], [dnl > > +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared>,labels=0x4d3000000000000000000000000 > > +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> > > +]) > > + > > +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([NORTHD_TYPE]) > > + > > +as > > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > > +/connection dropped.*/d"]) > > + > > AT_CLEANUP > > ]) > > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml > > index 987797860..80a564660 100644 > > --- a/utilities/ovn-nbctl.8.xml > > +++ b/utilities/ovn-nbctl.8.xml > > @@ -399,7 +399,7 @@ > > must be either <code>switch</code> or <code>port-group</code>. > > </p> > > <dl> > > - <dt>[<code>--type=</code>{<code>switch</code> | <code>port-group</code>}] [<code>--log</code>] [<code>--meter=</code><var>meter</var>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--may-exist</code>] <code>acl-add</code> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt> > > + <dt>[<code>--type=</code>{<code>switch</code> | <code>port-group</code>}] [<code>--log</code>] [<code>--meter=</code><var>meter</var>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--label=</code><var>label</var>] [<code>--may-exist</code>] <code>acl-add</code> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt> > > <dd> > > <p> > > Adds the specified ACL to <var>entity</var>. <var>direction</var> > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > > index f41238990..ada53b662 100644 > > --- a/utilities/ovn-nbctl.c > > +++ b/utilities/ovn-nbctl.c > > @@ -2011,6 +2011,9 @@ nbctl_acl_list(struct ctl_context *ctx) > > ds_chomp(&ctx->output, ','); > > ds_put_cstr(&ctx->output, ")"); > > } > > + if (acl->label) { > > + ds_put_format(&ctx->output, " label=%"PRId64, acl->label); > > + } > > ds_put_cstr(&ctx->output, "\n"); > > } > > > > @@ -2069,6 +2072,19 @@ parse_priority(const char *arg, int64_t *priority_p) > > return NULL; > > } > > > > +static char * OVS_WARN_UNUSED_RESULT > > +parse_acl_label(const char *arg, int64_t *label_p) > > +{ > > + /* Validate label. */ > > + int64_t label; > > + if (!ovs_scan(arg, "%"SCNd64, &label) > > + || label < 0 || label > UINT32_MAX) { > > + return xasprintf("%s: label must in range 0...4294967295", arg); > > + } > > + *label_p = label; > > + return NULL; > > +} > > + > > static void > > nbctl_pre_acl(struct ctl_context *ctx) > > { > > @@ -2093,6 +2109,7 @@ nbctl_pre_acl_list(struct ctl_context *ctx) > > ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_name); > > ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_severity); > > ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_meter); > > + ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_label); > > } > > > > static void > > @@ -2160,6 +2177,25 @@ nbctl_acl_add(struct ctl_context *ctx) > > nbrec_acl_set_meter(acl, meter); > > } > > > > + /* Set the ACL label */ > > + const char *label = shash_find_data(&ctx->options, "--label"); > > + if (label) { > > + /* Ensure that the action is either allow or allow-related */ > > + if (strcmp(action, "allow") && strcmp(action, "allow-related")) { > > + ctl_error(ctx, "label can only be set with actions \"allow\" or " > > + "\"allow-related\""); > > + return; > > + } > > + > > + int64_t label_value = 0; > > + error = parse_acl_label(label, &label_value); > > + if (error) { > > + ctx->error = error; > > + return; > > + } > > + nbrec_acl_set_label(acl, label_value); > > + } > > + > > /* Check if same acl already exists for the ls/portgroup */ > > size_t n_acls = pg ? pg->n_acls : ls->n_acls; > > struct nbrec_acl **acls = pg ? pg->acls : ls->acls; > > @@ -6757,7 +6793,7 @@ static const struct ctl_command_syntax nbctl_commands[] = { > > /* acl commands. */ > > { "acl-add", 5, 6, "{SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION", > > nbctl_pre_acl, nbctl_acl_add, NULL, > > - "--log,--may-exist,--type=,--name=,--severity=,--meter=", RW }, > > + "--log,--may-exist,--type=,--name=,--severity=,--meter=,--label=", RW }, > > { "acl-del", 1, 4, "{SWITCH | PORTGROUP} [DIRECTION [PRIORITY MATCH]]", > > nbctl_pre_acl, nbctl_acl_del, NULL, "--type=", RW }, > > { "acl-list", 1, 1, "{SWITCH | PORTGROUP}", > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/NEWS b/NEWS index f328666da..45dc45353 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,9 @@ Post-v21.06.0 - Added Control Plane Protection support (control plane traffic metering). - Added path MTU discovery for ingress traffic originated outside of the cluster. + - Introduced a new "label" field for "allow" and "allow-related" ACLs + which helps in debugging/backtracking the ACL which allowed a particular + connection. OVN v21.06.0 - 18 Jun 2021 ------------------------- diff --git a/lib/logical-fields.c b/lib/logical-fields.c index 72853013e..7b3d431e0 100644 --- a/lib/logical-fields.c +++ b/lib/logical-fields.c @@ -146,6 +146,8 @@ ovn_init_symtab(struct shash *symtab) "ct_label[32..79]", WR_CT_COMMIT); expr_symtab_add_subfield_scoped(symtab, "ct_label.ecmp_reply_port", NULL, "ct_label[80..95]", WR_CT_COMMIT); + expr_symtab_add_subfield_scoped(symtab, "ct_label.label", NULL, + "ct_label[96..127]", WR_CT_COMMIT); expr_symtab_add_field(symtab, "ct_state", MFF_CT_STATE, NULL, false); diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 08d484760..c5ea15774 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -670,13 +670,19 @@ the <code>next;</code> action. If there are any stateful ACLs on this datapath, then <code>allow</code> ACLs translate to <code>ct_commit; next;</code> (which acts as a hint for the next tables - to commit the connection to conntrack), + to commit the connection to conntrack). In case the <code>ACL</code> + has a label then <code>reg3</code> is loaded with the label value and + <code>reg0[13]</code> bit is set to 1 (which acts as a hint for the + next tables to commit the label to conntrack). </li> <li> <code>allow-related</code> ACLs translate into logical flows with the <code>ct_commit(ct_label=0/1); next;</code> actions for new connections and <code>reg0[1] = 1; next;</code> for existing - connections. + connections. In case the <code>ACL</code> has a label then + <code>reg3</code> is loaded with the label value and + <code>reg0[13]</code> bit is set to 1 (which acts as a hint for the + next tables to commit the label to conntrack). </li> <li> <code>allow-stateless</code> ACLs translate into logical @@ -876,9 +882,19 @@ </li> <li> - A priority-100 flow commits packets to connection tracker using - <code>ct_commit; next;</code> action based on a hint provided by - the previous tables (with a match for <code>reg0[1] == 1</code>). + A priority 100 flow is added which commits the packet to the conntrack + and sets the most significant 32-bits of <code>ct_label</code> with the + <code>reg3</code> value based on the hint provided by previous tables + (with a match for <code>reg0[1] == 1 && reg0[13] == 1</code>). + This is used by the <code>ACLs</code> with label to commit the label + value to conntrack. + </li> + + <li> + For <code>ACLs</code> without label, a second priority-100 flow commits + packets to connection tracker using <code>ct_commit; next;</code> + action based on a hint provided by the previous tables (with a match + for <code>reg0[1] == 1 && reg0[13] == 0</code>). </li> <li> A priority-0 flow that simply moves traffic to the next table. diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index a0eaa1247..1ad15b978 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -241,6 +241,7 @@ enum ovn_stage { #define REGBIT_ACL_HINT_BLOCK "reg0[10]" #define REGBIT_LKUP_FDB "reg0[11]" #define REGBIT_HAIRPIN_REPLY "reg0[12]" +#define REGBIT_ACL_LABEL "reg0[13]" #define REG_ORIG_DIP_IPV4 "reg1" #define REG_ORIG_DIP_IPV6 "xxreg1" @@ -273,6 +274,9 @@ enum ovn_stage { #define REG_SRC_IPV4 "reg1" #define REG_SRC_IPV6 "xxreg1" +/* Register used for setting a label for ACLs in a Logical Switch. */ +#define REG_LABEL "reg3" + #define FLAGBIT_NOT_VXLAN "flags[1] == 0" /* @@ -281,14 +285,15 @@ enum ovn_stage { * Logical Switch pipeline: * +----+----------------------------------------------+---+------------------+ * | R0 | REGBIT_{CONNTRACK/DHCP/DNS} | | | - * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | X | | - * | | REGBIT_ACL_HINT_{ALLOW_NEW/ALLOW/DROP/BLOCK} | X | | + * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | | | + * | | REGBIT_ACL_HINT_{ALLOW_NEW/ALLOW/DROP/BLOCK} | | | + * | | REGBIT_ACL_LABEL | X | | * +----+----------------------------------------------+ X | | * | R1 | ORIG_DIP_IPV4 (>= IN_STATEFUL) | R | | * +----+----------------------------------------------+ E | | * | R2 | ORIG_TP_DPORT (>= IN_STATEFUL) | G | | * +----+----------------------------------------------+ 0 | | - * | R3 | UNUSED | | | + * | R3 | ACL LABEL | | | * +----+----------------------------------------------+---+------------------+ * | R4 | UNUSED | | | * +----+----------------------------------------------+ X | ORIG_DIP_IPV6 | @@ -5774,7 +5779,12 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_clear(actions); ds_put_format(match, REGBIT_ACL_HINT_ALLOW_NEW " == 1 && (%s)", acl->match); + ds_put_cstr(actions, REGBIT_CONNTRACK_COMMIT" = 1; "); + if (acl->label) { + ds_put_format(actions, REGBIT_ACL_LABEL" = 1; " + REG_LABEL" = %"PRId64"; ", acl->label); + } build_acl_log(actions, acl, meter_groups); ds_put_cstr(actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, @@ -5785,15 +5795,21 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, /* Match on traffic in the request direction for an established * connection tracking entry that has not been marked for - * deletion. There is no need to commit here, so we can just - * proceed to the next table. We use this to ensure that this + * deletion. We use this to ensure that this * connection is still allowed by the currently defined - * policy. Match untracked packets too. */ + * policy. Match untracked packets too. + * Commit the connection only if the ACL has a label. This is done + * to update the connection tracking entry label in case the ACL + * allowing the connection changes. */ ds_clear(match); ds_clear(actions); ds_put_format(match, REGBIT_ACL_HINT_ALLOW " == 1 && (%s)", acl->match); - + if (acl->label) { + ds_put_cstr(actions, REGBIT_CONNTRACK_COMMIT" = 1; "); + ds_put_format(actions, REGBIT_ACL_LABEL" = 1; " + REG_LABEL" = %"PRId64"; ", acl->label); + } build_acl_log(actions, acl, meter_groups); ds_put_cstr(actions, "next;"); ovn_lflow_add_with_hint(lflows, od, stage, @@ -6292,15 +6308,34 @@ build_stateful(struct ovn_datapath *od, struct hmap *lflows) ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 0, "1", "next;"); ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 0, "1", "next;"); + /* If REGBIT_CONNTRACK_COMMIT is set as 1 and + * REGBIT_CONNTRACK_SET_LABEL is set to 1, then the packets should be + * committed to conntrack. + * We always set ct_mark.blocked to 0 here as + * any packet that makes it this far is part of a connection we + * want to allow to continue. */ + ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, + REGBIT_CONNTRACK_COMMIT" == 1 && " + REGBIT_ACL_LABEL" == 1", + "ct_commit { ct_label.blocked = 0; " + "ct_label.label = " REG_LABEL "; }; next;"); + ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100, + REGBIT_CONNTRACK_COMMIT" == 1 && " + REGBIT_ACL_LABEL" == 1", + "ct_commit { ct_label.blocked = 0; " + "ct_label.label = " REG_LABEL "; }; next;"); + /* If REGBIT_CONNTRACK_COMMIT is set as 1, then the packets should be * committed to conntrack. We always set ct_label.blocked to 0 here as * any packet that makes it this far is part of a connection we * want to allow to continue. */ ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, - REGBIT_CONNTRACK_COMMIT" == 1", + REGBIT_CONNTRACK_COMMIT" == 1 && " + REGBIT_ACL_LABEL" == 0", "ct_commit { ct_label.blocked = 0; }; next;"); ovn_lflow_add(lflows, od, S_SWITCH_OUT_STATEFUL, 100, - REGBIT_CONNTRACK_COMMIT" == 1", + REGBIT_CONNTRACK_COMMIT" == 1 && " + REGBIT_ACL_LABEL" == 0", "ct_commit { ct_label.blocked = 0; }; next;"); } diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl index 091fe10b3..b3d1a9657 100644 --- a/northd/ovn_northd.dl +++ b/northd/ovn_northd.dl @@ -1505,13 +1505,14 @@ function s_ROUTER_OUT_DELIVERY(): Stage { Stage{ Egress, 4, "lr_out_deliv * Logical Switch pipeline: * +----+----------------------------------------------+---+------------------+ * | R0 | REGBIT_{CONNTRACK/DHCP/DNS} | | | - * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | X | | + * | | REGBIT_{HAIRPIN/HAIRPIN_REPLY} | | | + * | | REGBIT_ACL_LABEL | X | | * +----+----------------------------------------------+ X | | * | R1 | ORIG_DIP_IPV4 (>= IN_STATEFUL) | R | | * +----+----------------------------------------------+ E | | * | R2 | ORIG_TP_DPORT (>= IN_STATEFUL) | G | | * +----+----------------------------------------------+ 0 | | - * | R3 | UNUSED | | | + * | R3 | ACL_LABEL | | | * +----+----------------------------------------------+---+------------------+ * | R4 | UNUSED | | | * +----+----------------------------------------------+ X | ORIG_DIP_IPV6 | @@ -1584,6 +1585,7 @@ function rEGBIT_ACL_HINT_DROP() : string = "reg0[9]" function rEGBIT_ACL_HINT_BLOCK() : string = "reg0[10]" function rEGBIT_LKUP_FDB() : string = "reg0[11]" function rEGBIT_HAIRPIN_REPLY() : string = "reg0[12]" +function rEGBIT_ACL_LABEL() : string = "reg0[13]" function rEG_ORIG_DIP_IPV4() : string = "reg1" function rEG_ORIG_DIP_IPV6() : string = "xxreg1" @@ -1610,6 +1612,9 @@ function rEG_INPORT_ETH_ADDR() : string = "xreg0[0..47]" function rEG_ECMP_GROUP_ID() : string = "reg8[0..15]" function rEG_ECMP_MEMBER_ID() : string = "reg8[16..31]" +/* Register used for setting a label for ACLs in a Logical Switch. */ +function rEG_LABEL() : string = "reg3" + function fLAGBIT_NOT_VXLAN() : string = "flags[1] == 0" function mFF_N_LOG_REGS() : bit<32> = 10 @@ -2698,26 +2703,41 @@ for (&SwitchACL(.sw = sw, .acl = acl, .has_fair_meter = fair_meter)) { * that case here and un-set ct_label.blocked (which will be done * by ct_commit in the "stateful" stage) to indicate that the * connection should be allowed to resume. + * If the ACL has a label, then load REG_LABEL with the label and + * set the REGBIT_ACL_LABEL field. */ - Flow(.logical_datapath = sw._uuid, - .stage = stage, - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), - .__match = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})", - .actions = "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${acl_log}next;", - .external_ids = stage_hint); + var __action = if (acl.label != 0) { + "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${rEGBIT_ACL_LABEL()} = 1;\ + \ ${rEG_LABEL()} = ${acl.label}; ${acl_log}next;" + } else { + "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${acl_log}next;" + } in Flow(.logical_datapath = sw._uuid, + .stage = stage, + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), + .__match = "${rEGBIT_ACL_HINT_ALLOW_NEW()} == 1 && (${acl.__match})", + .actions = __action, + .external_ids = stage_hint); /* Match on traffic in the request direction for an established * connection tracking entry that has not been marked for - * deletion. There is no need to commit here, so we can just - * proceed to the next table. We use this to ensure that this + * deletion. We use this to ensure that this * connection is still allowed by the currently defined - * policy. Match untracked packets too. */ - Flow(.logical_datapath = sw._uuid, - .stage = stage, - .priority = acl.priority + oVN_ACL_PRI_OFFSET(), - .__match = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})", - .actions = "${acl_log}next;", - .external_ids = stage_hint) + * policy. Match untracked packets too. + * Commit the connection only if the ACL has a label. This is done to + * update the connection tracking entry label in case the ACL + * allowing the connection changes. + */ + var __action = if (acl.label != 0) { + "${rEGBIT_CONNTRACK_COMMIT()} = 1; ${rEGBIT_ACL_LABEL()} = 1;\ + \ ${rEG_LABEL()} = ${acl.label}; ${acl_log}next;" + } else { + "${acl_log}next;" + } in Flow(.logical_datapath = sw._uuid, + .stage = stage, + .priority = acl.priority + oVN_ACL_PRI_OFFSET(), + .__match = "${rEGBIT_ACL_HINT_ALLOW()} == 1 && (${acl.__match})", + .actions = __action, + .external_ids = stage_hint) } } else if (acl.action == "allow-stateless") { Flow(.logical_datapath = sw._uuid, @@ -2934,6 +2954,24 @@ for (&Switch(._uuid = ls_uuid)) { .actions = "next;", .external_ids = map_empty()); + /* If REGBIT_CONNTRACK_COMMIT is set as 1 and REGBIT_CONNTRACK_SET_LABEL + * is set to 1, then the packets should be + * committed to conntrack. We always set ct_label.blocked to 0 here as + * any packet that makes it this far is part of a connection we + * want to allow to continue. */ + Flow(.logical_datapath = ls_uuid, + .stage = s_SWITCH_IN_STATEFUL(), + .priority = 100, + .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 1", + .actions = "ct_commit { ct_label.blocked = 0; ct_label.label = ${rEG_LABEL()}; }; next;", + .external_ids = map_empty()); + Flow(.logical_datapath = ls_uuid, + .stage = s_SWITCH_OUT_STATEFUL(), + .priority = 100, + .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 1", + .actions = "ct_commit { ct_label.blocked = 0; ct_label.label = ${rEG_LABEL()}; }; next;", + .external_ids = map_empty()); + /* If REGBIT_CONNTRACK_COMMIT is set as 1, then the packets should be * committed to conntrack. We always set ct_label.blocked to 0 here as * any packet that makes it this far is part of a connection we @@ -2941,13 +2979,13 @@ for (&Switch(._uuid = ls_uuid)) { Flow(.logical_datapath = ls_uuid, .stage = s_SWITCH_IN_STATEFUL(), .priority = 100, - .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1", + .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 0", .actions = "ct_commit { ct_label.blocked = 0; }; next;", .external_ids = map_empty()); Flow(.logical_datapath = ls_uuid, .stage = s_SWITCH_OUT_STATEFUL(), .priority = 100, - .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1", + .__match = "${rEGBIT_CONNTRACK_COMMIT()} == 1 && ${rEGBIT_ACL_LABEL()} == 0", .actions = "ct_commit { ct_label.blocked = 0; }; next;", .external_ids = map_empty()) } diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema index a53d5e851..2ac8ef3ea 100644 --- a/ovn-nb.ovsschema +++ b/ovn-nb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Northbound", - "version": "5.32.0", - "cksum": "2501921026 29540", + "version": "5.32.1", + "cksum": "2805328215 29734", "tables": { "NB_Global": { "columns": { @@ -244,6 +244,9 @@ "debug"]]}, "min": 0, "max": 1}}, "meter": {"type": {"key": "string", "min": 0, "max": 1}}, + "label": {"type": {"key": {"type": "integer", + "minInteger": 0, + "maxInteger": 4294967295}}}, "external_ids": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}, diff --git a/ovn-nb.xml b/ovn-nb.xml index c1176e81f..ebafa3e5e 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -1853,6 +1853,18 @@ and <code>deny</code> as <ref column="action"/>.) </p> + <column name="label"> + <p> + Associates an identifier with the ACL. + The same value will be written to corresponding connection + tracker entry. The value should be a valid 32-bit unsigned integer. + This value can help in debugging from connection tracker side. + For example, through this "label" we can backtrack to the ACL rule + which is causing a "leaked" connection. Connection tracker entries are + created only for allowed connections so the label is valid only + for allow and allow-related actions. + </p> + </column> <column name="priority"> <p> The ACL rule's priority. Rules with numerically higher priority diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at index 828777b82..4445ab7c0 100644 --- a/tests/ovn-nbctl.at +++ b/tests/ovn-nbctl.at @@ -211,18 +211,36 @@ ovn_nbctl_test_acl() { AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 300 tcp drop]) AT_CHECK([ovn-nbctl $2 acl-add $1 from-lport 200 ip drop]) AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop]) + AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 from-lport 70 icmp allow-related]) + AT_CHECK([ovn-nbctl $2 --label=1235 acl-add $1 to-lport 70 icmp allow-related]) + dnl Add duplicated ACL AT_CHECK([ovn-nbctl $2 acl-add $1 to-lport 100 ip drop], [1], [], [stderr]) AT_CHECK([grep 'already existed' stderr], [0], [ignore]) AT_CHECK([ovn-nbctl $2 --may-exist acl-add $1 to-lport 100 ip drop]) + dnl Add invalid ACL label + AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 to-lport 50 ip drop], [1], [], [stderr]) + AT_CHECK([grep 'can only be set with actions' stderr], [0], [ignore]) + + AT_CHECK([ovn-nbctl $2 --label=abcd acl-add $1 to-lport 50 ip allow-related], [1], [], [stderr]) + AT_CHECK([grep 'label must in range 0...4294967295' stderr], [0], [ignore]) + + AT_CHECK([ovn-nbctl $2 --label=-1 acl-add $1 to-lport 50 ip allow-related], [1], [], [stderr]) + AT_CHECK([grep 'label must in range 0...4294967295' stderr], [0], [ignore]) + + AT_CHECK([ovn-nbctl $2 --label=4294967296 acl-add $1 to-lport 50 ip allow-related], [1], [], [stderr]) + AT_CHECK([grep 'label must in range 0...4294967295' stderr], [0], [ignore]) + AT_CHECK([ovn-nbctl $2 acl-list $1], [0], [dnl from-lport 600 (udp) drop log() from-lport 400 (tcp) drop from-lport 200 (ip) drop +from-lport 70 (icmp) allow-related label=1234 to-lport 500 (udp) drop log(name=test,severity=info) to-lport 300 (tcp) drop to-lport 100 (ip) drop + to-lport 70 (icmp) allow-related label=1235 ]) dnl Delete in one direction. @@ -231,6 +249,7 @@ from-lport 200 (ip) drop from-lport 600 (udp) drop log() from-lport 400 (tcp) drop from-lport 200 (ip) drop +from-lport 70 (icmp) allow-related label=1234 ]) dnl Delete all ACLs. diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 474e88021..ba79106a1 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -3608,7 +3608,8 @@ check_stateful_flows() { AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) - table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) table=12(ls_in_stateful ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg1 = 10.0.0.10; reg2[[0..15]] = 80; ct_lb(backends=10.0.0.4:8080);) ]) @@ -3630,7 +3631,8 @@ check_stateful_flows() { AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) ]) } @@ -3669,7 +3671,8 @@ AT_CHECK([grep "ls_in_pre_stateful" sw0flows | sort], [0], [dnl AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) - table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) ]) AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl @@ -3687,14 +3690,108 @@ AT_CHECK([grep "ls_out_pre_stateful" sw0flows | sort], [0], [dnl AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) ]) AT_CLEANUP ]) OVN_FOR_EACH_NORTHD([ -AT_SETUP([ct.inv usage]) +AT_SETUP([ovn -- ACL label usage]) +ovn_start + +check ovn-nbctl ls-add sw0 +check ovn-nbctl lsp-add sw0 sw0p1 + +check ovn-nbctl --wait=sb --label=1234 acl-add sw0 to-lport 1002 tcp allow-related +check ovn-nbctl --wait=sb --label=1234 acl-add sw0 from-lport 1002 tcp allow-related + +ovn-sbctl dump-flows sw0 > sw0flows +AT_CAPTURE_FILE([sw0flows]) + +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort], [0], [dnl + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) +]) +AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl + table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) +]) + +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) +]) +AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl + table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) +]) + +# Add new ACL without label +check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 udp allow-related +check ovn-nbctl --wait=sb acl-add sw0 from-lport 1002 udp allow-related + +ovn-sbctl dump-flows sw0 > sw0flows +AT_CAPTURE_FILE([sw0flows]) + +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort], [0], [dnl + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) +]) +AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl + table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) +]) + +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) +]) +AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl + table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) +]) + +# Delete new ACL with label +check ovn-nbctl --wait=sb acl-del sw0 to-lport 1002 tcp +check ovn-nbctl --wait=sb acl-del sw0 from-lport 1002 tcp + +ovn-sbctl dump-flows sw0 > sw0flows +AT_CAPTURE_FILE([sw0flows]) + +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort], [0], [dnl + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) +]) +AT_CHECK([grep "ls_in_stateful" sw0flows | sort], [0], [dnl + table=12(ls_in_stateful ), priority=0 , match=(1), action=(next;) + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=12(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) +]) + +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) +]) +AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl + table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_label.blocked = 0; }; next;) + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_label.blocked = 0; ct_label.label = reg3; }; next;) +]) +AT_CLEANUP +]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn -- ct.inv usage]) ovn_start check ovn-nbctl ls-add sw0 diff --git a/tests/ovn.at b/tests/ovn.at index a207f5e12..e51a94ad8 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -197,6 +197,7 @@ ct_label = NXM_NX_CT_LABEL ct_label.blocked = ct_label[0] ct_label.ecmp_reply_eth = ct_label[32..79] ct_label.ecmp_reply_port = ct_label[80..95] +ct_label.label = ct_label[96..127] ct_label.natted = ct_label[1] ct_mark = NXM_NX_CT_MARK ct_state = NXM_NX_CT_STATE diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 4288d80e5..7d03bf3f0 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -6745,5 +6745,249 @@ OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) as OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d /.*terminating with signal 15.*/d"]) + +AT_CLEANUP +]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ACL label - conntrack ct_label]) +AT_KEYWORDS([acl label ct_commit]) + +CHECK_CONNTRACK() +ovn_start + +OVS_TRAFFIC_VSWITCHD_START() +ADD_BR([br-int]) + +# Set external-ids in br-int needed for ovn-controller +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 + +# Start ovn-controller +start_daemon ovn-controller + +check ovn-nbctl ls-add sw0 + +check ovn-nbctl lsp-add sw0 sw0-p1 +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:02 10.0.0.2" +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:02 10.0.0.2" + +check ovn-nbctl lsp-add sw0 sw0-p2 +check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:03 10.0.0.3" +check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:03 10.0.0.3" + +check ovn-nbctl lsp-add sw0 sw0-p3 +check ovn-nbctl lsp-set-addresses sw0-p3 "50:54:00:00:00:04 10.0.0.4" +check ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:04 10.0.0.4" + +# ACLs +# Case 1: sw0-p1 ---> sw0-p3 allowed, label=1234 +# Case 2: sw0-p3 ---> sw0-p1 allowed, label=1235 +# Case 3: sw0-p1 ---> sw0-p2 allowed, no label +# Case 4: sw0-p2 ---> sw0-p1 allowed, no label + +check ovn-nbctl --label=1234 acl-add sw0 from-lport 1002 'ip4 && inport == "sw0-p1" && ip4.dst == 10.0.0.4' allow-related +check ovn-nbctl --label=1235 acl-add sw0 to-lport 1002 'ip4 && outport == "sw0-p1" && ip4.src == 10.0.0.4' allow-related +check ovn-nbctl acl-add sw0 from-lport 1001 "ip" allow-related +check ovn-nbctl acl-add sw0 to-lport 1001 "ip" allow-related + + +ADD_NAMESPACES(sw0-p1) +ADD_VETH(sw0-p1, sw0-p1, br-int, "10.0.0.2/24", "50:54:00:00:00:02", \ + "10.0.0.1") +ADD_NAMESPACES(sw0-p2) +ADD_VETH(sw0-p2, sw0-p2, br-int, "10.0.0.3/24", "50:54:00:00:00:03", \ + "10.0.0.1") +ADD_NAMESPACES(sw0-p3) +ADD_VETH(sw0-p3, sw0-p3, br-int, "10.0.0.4/24", "50:54:00:00:00:04", \ + "10.0.0.1") + +# Ensure ovn-controller is caught up +ovn-nbctl --wait=hv sync + +on_exit 'ovn-nbctl acl-list sw0' +on_exit 'ovn-sbctl lflow-list' +on_exit 'ovs-ofctl dump-flows br-int' + +wait_for_ports_up + +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) +# 'sw0-p1' should be able to ping 'sw0-p3'. +NS_CHECK_EXEC([sw0-p1], [ping -q -c 10 -i 0.3 -w 15 10.0.0.4 | FORMAT_PING], \ +[0], [dnl +10 packets transmitted, 10 received, 0% packet loss, time 0ms +]) + +# Ensure conntrack entry is present and ct_label is set. +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.4) | \ +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ +sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2000000000000000000000000/'], [0], [dnl +icmp,orig=(src=10.0.0.2,dst=10.0.0.4,id=<cleared>,type=8,code=0),reply=(src=10.0.0.4,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared>,labels=0x4d2000000000000000000000000 +icmp,orig=(src=10.0.0.2,dst=10.0.0.4,id=<cleared>,type=8,code=0),reply=(src=10.0.0.4,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> +]) + +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) +# 'sw0-p3' should be able to ping 'sw0-p1'. +NS_CHECK_EXEC([sw0-p3], [ping -q -c 10 -i 0.3 -w 15 10.0.0.2 | FORMAT_PING], \ +[0], [dnl +10 packets transmitted, 10 received, 0% packet loss, time 0ms +]) + +# Ensure conntrack entry is present and ct_label is set. +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.2) | \ +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ +sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3000000000000000000000000/'], [0], [dnl +icmp,orig=(src=10.0.0.4,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.4,id=<cleared>,type=0,code=0),zone=<cleared>,labels=0x4d3000000000000000000000000 +icmp,orig=(src=10.0.0.4,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.4,id=<cleared>,type=0,code=0),zone=<cleared> +]) + +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) +# 'sw0-p1' should be able to ping 'sw0-p2'. +NS_CHECK_EXEC([sw0-p1], [ping -q -c 10 -i 0.3 -w 15 10.0.0.3 | FORMAT_PING], \ +[0], [dnl +10 packets transmitted, 10 received, 0% packet loss, time 0ms +]) + +# Ensure conntrack entry is present and ct_label is not set. +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> +]) + +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) +# 'sw0-p2' should be able to ping 'sw0-p1'. +NS_CHECK_EXEC([sw0-p2], [ping -q -c 10 -i 0.3 -w 15 10.0.0.2 | FORMAT_PING], \ +[0], [dnl +10 packets transmitted, 10 received, 0% packet loss, time 0ms +]) + +# Ensure conntrack entry is present and ct_label is not set. +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.2) | \ +sed -e 's/zone=[[0-9]]*/zone=<cleared>/'], [0], [dnl +icmp,orig=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=0,code=0),zone=<cleared> +icmp,orig=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=8,code=0),reply=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=0,code=0),zone=<cleared> +]) + +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([NORTHD_TYPE]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) + +AT_CLEANUP +]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ACL label - conntrack label change]) +AT_KEYWORDS([acl label ct_commit label change]) + +CHECK_CONNTRACK() +ovn_start + +OVS_TRAFFIC_VSWITCHD_START() +ADD_BR([br-int]) + +# Set external-ids in br-int needed for ovn-controller +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 + +# Start ovn-controller +start_daemon ovn-controller + +check ovn-nbctl ls-add sw0 + +check ovn-nbctl lsp-add sw0 sw0-p1 +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:02 10.0.0.2" +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:02 10.0.0.2" + +check ovn-nbctl lsp-add sw0 sw0-p2 +check ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:03 10.0.0.3" +check ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:03 10.0.0.3" + +# ACLs +# sw0-p1 ---> sw0-p2 allowed, label=1234 + +check ovn-nbctl --label=1234 acl-add sw0 from-lport 1002 'ip4 && inport == "sw0-p1" && ip4.dst == 10.0.0.3' allow-related + +ADD_NAMESPACES(sw0-p1) +ADD_VETH(sw0-p1, sw0-p1, br-int, "10.0.0.2/24", "50:54:00:00:00:02", \ + "10.0.0.1") +ADD_NAMESPACES(sw0-p2) +ADD_VETH(sw0-p2, sw0-p2, br-int, "10.0.0.3/24", "50:54:00:00:00:03", \ + "10.0.0.1") + +# Ensure ovn-controller is caught up +ovn-nbctl --wait=hv sync + +on_exit 'ovn-nbctl acl-list sw0' +on_exit 'ovn-sbctl lflow-list' +on_exit 'ovs-ofctl dump-flows br-int' + +wait_for_ports_up + +AT_CHECK([ovs-appctl dpctl/flush-conntrack]) + +# start a background ping for ~30 secs. +NETNS_DAEMONIZE([sw0-p1], [[ping -q -c 100 -i 0.3 -w 15 10.0.0.3]], [ns-sw0-p1.pid]) + +sleep 3s + +# Ensure conntrack entry is present and ct_label is set. +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ +sed -e 's/labels=0x4d2[[0-9a-f]]*/labels=0x4d2000000000000000000000000/'], [0], [dnl +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared>,labels=0x4d2000000000000000000000000 +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> +]) + +# Add a higher priority ACL with different label. +# This ACL also allows the ping running in background. + +check ovn-nbctl --label=1235 acl-add sw0 from-lport 1003 'ip4 && inport == "sw0-p1" && ip4.dst == 10.0.0.3' allow-related +ovn-nbctl --wait=hv sync + +sleep 3s + +# Ensure conntrack entry is updated with new ct_label is set. +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.3) | \ +sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ +sed -e 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3000000000000000000000000/'], [0], [dnl +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared>,labels=0x4d3000000000000000000000000 +icmp,orig=(src=10.0.0.2,dst=10.0.0.3,id=<cleared>,type=8,code=0),reply=(src=10.0.0.3,dst=10.0.0.2,id=<cleared>,type=0,code=0),zone=<cleared> +]) + +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([NORTHD_TYPE]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) + AT_CLEANUP ]) diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml index 987797860..80a564660 100644 --- a/utilities/ovn-nbctl.8.xml +++ b/utilities/ovn-nbctl.8.xml @@ -399,7 +399,7 @@ must be either <code>switch</code> or <code>port-group</code>. </p> <dl> - <dt>[<code>--type=</code>{<code>switch</code> | <code>port-group</code>}] [<code>--log</code>] [<code>--meter=</code><var>meter</var>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--may-exist</code>] <code>acl-add</code> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt> + <dt>[<code>--type=</code>{<code>switch</code> | <code>port-group</code>}] [<code>--log</code>] [<code>--meter=</code><var>meter</var>] [<code>--severity=</code><var>severity</var>] [<code>--name=</code><var>name</var>] [<code>--label=</code><var>label</var>] [<code>--may-exist</code>] <code>acl-add</code> <var>entity</var> <var>direction</var> <var>priority</var> <var>match</var> <var>verdict</var></dt> <dd> <p> Adds the specified ACL to <var>entity</var>. <var>direction</var> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index f41238990..ada53b662 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -2011,6 +2011,9 @@ nbctl_acl_list(struct ctl_context *ctx) ds_chomp(&ctx->output, ','); ds_put_cstr(&ctx->output, ")"); } + if (acl->label) { + ds_put_format(&ctx->output, " label=%"PRId64, acl->label); + } ds_put_cstr(&ctx->output, "\n"); } @@ -2069,6 +2072,19 @@ parse_priority(const char *arg, int64_t *priority_p) return NULL; } +static char * OVS_WARN_UNUSED_RESULT +parse_acl_label(const char *arg, int64_t *label_p) +{ + /* Validate label. */ + int64_t label; + if (!ovs_scan(arg, "%"SCNd64, &label) + || label < 0 || label > UINT32_MAX) { + return xasprintf("%s: label must in range 0...4294967295", arg); + } + *label_p = label; + return NULL; +} + static void nbctl_pre_acl(struct ctl_context *ctx) { @@ -2093,6 +2109,7 @@ nbctl_pre_acl_list(struct ctl_context *ctx) ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_name); ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_severity); ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_meter); + ovsdb_idl_add_column(ctx->idl, &nbrec_acl_col_label); } static void @@ -2160,6 +2177,25 @@ nbctl_acl_add(struct ctl_context *ctx) nbrec_acl_set_meter(acl, meter); } + /* Set the ACL label */ + const char *label = shash_find_data(&ctx->options, "--label"); + if (label) { + /* Ensure that the action is either allow or allow-related */ + if (strcmp(action, "allow") && strcmp(action, "allow-related")) { + ctl_error(ctx, "label can only be set with actions \"allow\" or " + "\"allow-related\""); + return; + } + + int64_t label_value = 0; + error = parse_acl_label(label, &label_value); + if (error) { + ctx->error = error; + return; + } + nbrec_acl_set_label(acl, label_value); + } + /* Check if same acl already exists for the ls/portgroup */ size_t n_acls = pg ? pg->n_acls : ls->n_acls; struct nbrec_acl **acls = pg ? pg->acls : ls->acls; @@ -6757,7 +6793,7 @@ static const struct ctl_command_syntax nbctl_commands[] = { /* acl commands. */ { "acl-add", 5, 6, "{SWITCH | PORTGROUP} DIRECTION PRIORITY MATCH ACTION", nbctl_pre_acl, nbctl_acl_add, NULL, - "--log,--may-exist,--type=,--name=,--severity=,--meter=", RW }, + "--log,--may-exist,--type=,--name=,--severity=,--meter=,--label=", RW }, { "acl-del", 1, 4, "{SWITCH | PORTGROUP} [DIRECTION [PRIORITY MATCH]]", nbctl_pre_acl, nbctl_acl_del, NULL, "--type=", RW }, { "acl-list", 1, 1, "{SWITCH | PORTGROUP}",