On Fri, Jul 23, 2021 at 11:54 AM Priyankar Jain <[email protected]> wrote: > > Sure. Thanks Numan.
Hi Priyankar, Please see below for a few comments. This is not a full review. But over all the patch LGTM. > > Best Regards, > Priyankar Jain > > On 23/07/21 7:57 pm, Numan Siddique wrote: > > On Tue, Jul 20, 2021 at 12:17 PM Priyankar Jain > > <[email protected]> wrote: > >> > >> ping for review. > > > > Hi Priyankar, > > > > I'll take a look at the patch and come back to you with comments. > > > > Thanks > > Numan > > > >> > >> On 12/07/21 1:04 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=0x1234 > >>> > >>> 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 <[email protected]> > >>> --- > >>> lib/logical-fields.c | 2 + > >>> northd/ovn-northd.c | 53 +++++++++++++++---- > >>> northd/ovn_northd.dl | 75 +++++++++++++++++++------- > >>> ovn-nb.ovsschema | 3 +- > >>> ovn-nb.xml | 12 +++++ > >>> tests/ovn-nbctl.at | 22 ++++++++ > >>> tests/ovn-northd.at | 107 ++++++++++++++++++++++++++++++++++++-- > >>> tests/ovn.at | 1 + > >>> utilities/ovn-nbctl.8.xml | 2 +- > >>> utilities/ovn-nbctl.c | 32 +++++++++++- > >>> 10 files changed, 273 insertions(+), 36 deletions(-) > >>> > >>> 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.c b/northd/ovn-northd.c > >>> index 562dc62b2..92590a49d 100644 > >>> --- a/northd/ovn-northd.c > >>> +++ b/northd/ovn-northd.c > >>> @@ -238,6 +238,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" > >>> @@ -270,6 +271,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" > >>> > >>> /* > >>> @@ -278,14 +282,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 | > >>> @@ -5732,7 +5737,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" = %s; ", acl->label); > >>> + } > >>> build_acl_log(actions, acl, meter_groups); > >>> ds_put_cstr(actions, "next;"); > >>> ovn_lflow_add_with_hint(lflows, od, stage, > >>> @@ -5743,15 +5753,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" = %s; ", acl->label); > >>> + } > >>> build_acl_log(actions, acl, meter_groups); > >>> ds_put_cstr(actions, "next;"); > >>> ovn_lflow_add_with_hint(lflows, od, stage, > >>> @@ -6240,15 +6256,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 499130d7e..175642dad 100644 > >>> --- a/northd/ovn_northd.dl > >>> +++ b/northd/ovn_northd.dl > >>> @@ -1502,13 +1502,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 | > >>> @@ -1581,6 +1582,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" > >>> @@ -1607,6 +1609,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 > >>> @@ -2639,26 +2644,40 @@ 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 = match (acl.label) { > >>> + None -> "${rEGBIT_CONNTRACK_COMMIT()} = 1; > >>> ${acl_log}next;", > >>> + Some{label} -> "${rEGBIT_CONNTRACK_COMMIT()} = 1; > >>> ${rEGBIT_ACL_LABEL()} = 1;\ > >>> + \ ${rEG_LABEL()} = ${label}; > >>> ${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 = match (acl.label) { > >>> + None -> "${acl_log}next;", > >>> + Some{label} -> "${rEGBIT_CONNTRACK_COMMIT()} = 1; > >>> ${rEGBIT_ACL_LABEL()} = 1;\ > >>> + \ ${rEG_LABEL()} = ${label}; > >>> ${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, > >>> @@ -2870,6 +2889,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 > >>> @@ -2877,13 +2914,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 faf619a1c..ddd4637cd 100644 > >>> --- a/ovn-nb.ovsschema > >>> +++ b/ovn-nb.ovsschema > >>> @@ -1,7 +1,7 @@ > >>> { > >>> "name": "OVN_Northbound", > >>> "version": "5.32.0", > >>> - "cksum": "204590300 28863", > >>> + "cksum": "4049685538 28937", Since you're adding a column please bump up the version to 5.32.1. > >>> "tables": { > >>> "NB_Global": { > >>> "columns": { > >>> @@ -233,6 +233,7 @@ > >>> "debug"]]}, > >>> "min": 0, "max": 1}}, > >>> "meter": {"type": {"key": "string", "min": 0, "max": > >>> 1}}, > >>> + "label": {"type": {"key": "string", "min": 0, "max": 1}}, Since the label is an integer, I'd suggest changing the type to integer. eg: "label": {"type": {"key": {"type": "integer", "minInteger": 0, "maxInteger": 32767}}}, If 0 is not allowed, then change the min to 1. I think it should be fine for the user to set the label as an integer value rather than a string of hex value. Please update the documentation in ovn-northd.8.xml with the new logical flows added. Can you please add a system test (or enhance an existing one) which checks that the ct_label is set as expected. Thanks Numan > >>> "external_ids": { > >>> "type": {"key": "string", "value": "string", > >>> "min": 0, "max": "unlimited"}}}, > >>> diff --git a/ovn-nb.xml b/ovn-nb.xml > >>> index b6a0d1f43..cc79b30ac 100644 > >>> --- a/ovn-nb.xml > >>> +++ b/ovn-nb.xml > >>> @@ -1779,6 +1779,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 in hex, for example: 0x1234. > >>> + 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..53502196d 100644 > >>> --- a/tests/ovn-nbctl.at > >>> +++ b/tests/ovn-nbctl.at > >>> @@ -211,18 +211,39 @@ 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=0x1234 acl-add $1 from-lport 70 icmp > >>> allow-related]) > >>> + AT_CHECK([ovn-nbctl $2 --label=0x1235 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=0x1234 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=1234 acl-add $1 to-lport 50 ip > >>> allow-related], [1], [], [stderr]) > >>> + AT_CHECK([grep 'should start with "0x"' stderr], [0], [ignore]) > >>> + > >>> + AT_CHECK([ovn-nbctl $2 --label=0xagh acl-add $1 to-lport 50 ip > >>> allow-related], [1], [], [stderr]) > >>> + AT_CHECK([grep 'should be in hex format' stderr], [0], [ignore]) > >>> + > >>> + AT_CHECK([ovn-nbctl $2 --label=0x acl-add $1 to-lport 50 ip > >>> allow-related], [1], [], [stderr]) > >>> + AT_CHECK([grep 'should be between 0x0 and 0xffffffff' stderr], [0], > >>> [ignore]) > >>> + > >>> + AT_CHECK([ovn-nbctl $2 --label=0xfffffffff acl-add $1 to-lport 50 ip > >>> allow-related], [1], [], [stderr]) > >>> + AT_CHECK([grep 'label should be between 0x0 and 0xffffffff' 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=0x1234 > >>> 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=0x1235 > >>> ]) > >>> > >>> dnl Delete in one direction. > >>> @@ -231,6 +252,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=0x1234 > >>> ]) > >>> > >>> dnl Delete all ACLs. > >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > >>> index 11461d3f4..123d7362a 100644 > >>> --- a/tests/ovn-northd.at > >>> +++ b/tests/ovn-northd.at > >>> @@ -3515,7 +3515,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);) > >>> ]) > >>> > >>> @@ -3537,7 +3538,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;) > >>> ]) > >>> } > >>> > >>> @@ -3576,7 +3578,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 > >>> @@ -3594,14 +3597,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=0x1234 acl-add sw0 to-lport 1002 tcp > >>> allow-related > >>> +check ovn-nbctl --wait=sb --label=0x1234 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 = 0x1234; next;) > >>> + table=9 (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 > >>> && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 0x1234; 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 = 0x1234; next;) > >>> + table=4 (ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 > >>> && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 0x1234; 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 = 0x1234; 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 = 0x1234; 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 = 0x1234; 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 = 0x1234; 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 e5d8869a8..29d0f7f1b 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/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml > >>> index c3ff87e74..d1d8c24d6 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 3144fba49..a51a6ea8e 100644 > >>> --- a/utilities/ovn-nbctl.c > >>> +++ b/utilities/ovn-nbctl.c > >>> @@ -1988,6 +1988,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=%s", acl->label); > >>> + } > >>> ds_put_cstr(&ctx->output, "\n"); > >>> } > >>> > >>> @@ -2070,6 +2073,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 > >>> @@ -2137,6 +2141,32 @@ 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; > >>> + } > >>> + /* Validate that label is in the hex format (for example: 0x1234) > >>> */ > >>> + size_t label_len = strlen(label); > >>> + if (strncmp(label, "0x", 2)) { > >>> + ctl_error(ctx, "label: %s, should start with \"0x\"", label); > >>> + return; > >>> + } > >>> + if (label_len < 3 || label_len > 10) { > >>> + ctl_error(ctx, "label should be between 0x0 and 0xffffffff"); > >>> + return; > >>> + } > >>> + if (strspn(label + 2, "0123456789abcdefABCDEF") + 2 != label_len) { > >>> + ctl_error(ctx, "label: %s, should be in hex format", label); > >>> + return; > >>> + } > >>> + nbrec_acl_set_label(acl, label); > >>> + } > >>> + > >>> /* 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; > >>> @@ -6593,7 +6623,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 > >> [email protected] > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIFaQ&c=s883GpUCOChKOHiocYtGcg&r=BWfApgwqQ_BTXd_yEq_kwWLM-KTqkdPvolDMCJmUagg&m=EkeF6Zfi-qvQGRbUCCa_HtXipSgBXC55B9BBdYtcgeE&s=uceROnAaA02iOdUainqevCHeSjYPQI5dig0vdeVwgfQ&e= > >> > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
