| Message ID | 20230213163539.4507-1-sangana.abhiram@nutanix.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] northd, controller: Commit flows dropped by ACLs to conntrack | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
References: <20230213163539.4507-1-sangana.abhiram@nutanix.com>
Bleep bloop. Greetings Abhiram Sangana, 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 82 characters long (recommended limit is 79)
#464 FILE: ovn-sb.xml:1412:
<dt><code>ct_commit_drop { ct_mark=<var>value[/mask]</var>; };</code></dt>
WARNING: Line is 83 characters long (recommended limit is 79)
#465 FILE: ovn-sb.xml:1413:
<dt><code>ct_commit_drop { ct_label=<var>value[/mask]</var>; };</code></dt>
WARNING: Line is 116 characters long (recommended limit is 79)
#466 FILE: ovn-sb.xml:1414:
<dt><code>ct_commit_drop { ct_mark=<var>value[/mask]</var>; ct_label=<var>value[/mask]</var>; };</code></dt>
Lines checked: 862, Warnings: 3, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
> On 13 Feb 2023, at 16:35, Abhiram Sangana <sangana.abhiram@nutanix.com> wrote: > > This patch adds support to commit connections dropped/rejected by > ACLs to the connection tracking table. Dropped connections are > committed to conntrack only if NB_Global options:ct_commit_acl_drop > is set to true (false by default) and ACL dropping/rejecting the > connection has label configured. The dropped connections are > committed in a separate conntrack zone so that they can be managed > independently and do not interact with the connection tracking state > of allowed connections. > > This provides a new approach to identify connections dropped by ACLs > besides the existing ACL logging and drop sampling approaches. > > Each logical switch is assigned a new conntrack zone for committing > dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE. > A new lflow action "ct_commit_drop" is introduced that commits flows > to connection tracking table in a zone identified by > MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action > and non-empty label translates to include "ct_commit_drop" in its > actions instead of simply dropping/rejecting the packet. > > Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com> > --- > controller/ovn-controller.c | 14 +++- > controller/physical.c | 32 ++++++++- > include/ovn/actions.h | 1 + > include/ovn/logical-fields.h | 1 + > lib/actions.c | 65 +++++++++++++++++ > lib/ovn-util.c | 4 +- > lib/ovn-util.h | 2 +- > northd/northd.c | 25 ++++++- > northd/ovn-northd.8.xml | 30 +++++++- > ovn-nb.xml | 17 +++-- > ovn-sb.xml | 22 ++++++ > tests/ovn-nbctl.at | 10 ++- > tests/ovn-northd.at | 133 ++++++++++++++++++++++++----------- > tests/ovn.at | 90 +++++++++++++++++++++++- > utilities/ovn-nbctl.c | 7 -- > utilities/ovn-trace.c | 2 + > 16 files changed, 383 insertions(+), 72 deletions(-) > Can someone please review this patch? Thank you, Abhiram Sangana
On 3/3/23 13:20, Abhiram Sangana wrote: > > >> On 13 Feb 2023, at 16:35, Abhiram Sangana <sangana.abhiram@nutanix.com> wrote: >> >> This patch adds support to commit connections dropped/rejected by >> ACLs to the connection tracking table. Dropped connections are >> committed to conntrack only if NB_Global options:ct_commit_acl_drop >> is set to true (false by default) and ACL dropping/rejecting the >> connection has label configured. The dropped connections are >> committed in a separate conntrack zone so that they can be managed >> independently and do not interact with the connection tracking state >> of allowed connections. >> >> This provides a new approach to identify connections dropped by ACLs >> besides the existing ACL logging and drop sampling approaches. >> >> Each logical switch is assigned a new conntrack zone for committing >> dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE. >> A new lflow action "ct_commit_drop" is introduced that commits flows >> to connection tracking table in a zone identified by >> MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action >> and non-empty label translates to include "ct_commit_drop" in its >> actions instead of simply dropping/rejecting the packet. >> >> Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com> >> --- >> controller/ovn-controller.c | 14 +++- >> controller/physical.c | 32 ++++++++- >> include/ovn/actions.h | 1 + >> include/ovn/logical-fields.h | 1 + >> lib/actions.c | 65 +++++++++++++++++ >> lib/ovn-util.c | 4 +- >> lib/ovn-util.h | 2 +- >> northd/northd.c | 25 ++++++- >> northd/ovn-northd.8.xml | 30 +++++++- >> ovn-nb.xml | 17 +++-- >> ovn-sb.xml | 22 ++++++ >> tests/ovn-nbctl.at | 10 ++- >> tests/ovn-northd.at | 133 ++++++++++++++++++++++++----------- >> tests/ovn.at | 90 +++++++++++++++++++++++- >> utilities/ovn-nbctl.c | 7 -- >> utilities/ovn-trace.c | 2 + >> 16 files changed, 383 insertions(+), 72 deletions(-) >> > > Can someone please review this patch? > > Thank you, > Abhiram Sangana Sorry for the delay, Abhiram. I'll try to get to this early next week. Regards, Dumitru
On Mon, Feb 13, 2023 at 11:36 AM Abhiram Sangana <sangana.abhiram@nutanix.com> wrote: > > This patch adds support to commit connections dropped/rejected by > ACLs to the connection tracking table. Dropped connections are > committed to conntrack only if NB_Global options:ct_commit_acl_drop > is set to true (false by default) and ACL dropping/rejecting the > connection has label configured. The dropped connections are > committed in a separate conntrack zone so that they can be managed > independently and do not interact with the connection tracking state > of allowed connections. > > This provides a new approach to identify connections dropped by ACLs > besides the existing ACL logging and drop sampling approaches. > > Each logical switch is assigned a new conntrack zone for committing > dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE. > A new lflow action "ct_commit_drop" is introduced that commits flows > to connection tracking table in a zone identified by > MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action > and non-empty label translates to include "ct_commit_drop" in its > actions instead of simply dropping/rejecting the packet. > > Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com> Hi Abhiram, Sorry for the delays in the reviews. Overall the patch looks good to me. I do have a few comments and suggestions. Instead of adding a global option, how about an option per ACL (in the ACL options column) ? Users can enable or disable conntrack commit per ACL. Are you going to use this feature all the time in your deployment ? Or enable it only when debugging an issue ? If you want to enable it during debugging, is it a bit of a hassle to enable the ct_commit option for the interested ACL ? I'm a little bit concerned with allocating a zone id for each logical switch even if there are no ACLs configured with this option. But I think it's not a big deal. Thanks Numan > --- > controller/ovn-controller.c | 14 +++- > controller/physical.c | 32 ++++++++- > include/ovn/actions.h | 1 + > include/ovn/logical-fields.h | 1 + > lib/actions.c | 65 +++++++++++++++++ > lib/ovn-util.c | 4 +- > lib/ovn-util.h | 2 +- > northd/northd.c | 25 ++++++- > northd/ovn-northd.8.xml | 30 +++++++- > ovn-nb.xml | 17 +++-- > ovn-sb.xml | 22 ++++++ > tests/ovn-nbctl.at | 10 ++- > tests/ovn-northd.at | 133 ++++++++++++++++++++++++----------- > tests/ovn.at | 90 +++++++++++++++++++++++- > utilities/ovn-nbctl.c | 7 -- > utilities/ovn-trace.c | 2 + > 16 files changed, 383 insertions(+), 72 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 265740cab..e8f0b7242 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -734,8 +734,8 @@ update_ct_zones(const struct shash *binding_lports, > HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > /* XXX Add method to limit zone assignment to logical router > * datapaths with NAT */ > - char *dnat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "dnat"); > - char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat"); > + char *dnat = alloc_ct_zone_key(&ld->datapath->header_.uuid, "dnat"); > + char *snat = alloc_ct_zone_key(&ld->datapath->header_.uuid, "snat"); > sset_add(&all_users, dnat); > sset_add(&all_users, snat); > > @@ -745,6 +745,14 @@ update_ct_zones(const struct shash *binding_lports, > } > free(dnat); > free(snat); > + > + /* Zone for committing connections dropped by ACLs with labels. */ > + if (ld->is_switch) { > + char *drop = alloc_ct_zone_key( > + &ld->datapath->header_.uuid, "drop"); > + sset_add(&all_users, drop); > + free(drop); > + } > } > > /* Delete zones that do not exist in above sset. */ > @@ -2420,7 +2428,7 @@ ct_zones_datapath_binding_handler(struct engine_node *node, void *data) > /* Check if the requested snat zone has changed for the datapath > * or not. If so, then fall back to full recompute of > * ct_zone engine. */ > - char *snat_dp_zone_key = alloc_nat_zone_key(&dp->header_.uuid, "snat"); > + char *snat_dp_zone_key = alloc_ct_zone_key(&dp->header_.uuid, "snat"); > struct simap_node *simap_node = simap_find(&ct_zones_data->current, > snat_dp_zone_key); > free(snat_dp_zone_key); > diff --git a/controller/physical.c b/controller/physical.c > index 4dcf44e01..3c573c492 100644 > --- a/controller/physical.c > +++ b/controller/physical.c > @@ -60,6 +60,7 @@ struct zone_ids { > int ct; /* MFF_LOG_CT_ZONE. */ > int dnat; /* MFF_LOG_DNAT_ZONE. */ > int snat; /* MFF_LOG_SNAT_ZONE. */ > + int drop; /* MFF_LOG_ACL_DROP_ZONE. */ > }; > > struct tunnel { > @@ -204,14 +205,18 @@ get_zone_ids(const struct sbrec_port_binding *binding, > > const struct uuid *key = &binding->datapath->header_.uuid; > > - char *dnat = alloc_nat_zone_key(key, "dnat"); > + char *dnat = alloc_ct_zone_key(key, "dnat"); > zone_ids.dnat = simap_get(ct_zones, dnat); > free(dnat); > > - char *snat = alloc_nat_zone_key(key, "snat"); > + char *snat = alloc_ct_zone_key(key, "snat"); > zone_ids.snat = simap_get(ct_zones, snat); > free(snat); > > + char *drop = alloc_ct_zone_key(key, "drop"); > + zone_ids.drop = simap_get(ct_zones, drop); > + free(drop); > + > return zone_ids; > } > > @@ -830,6 +835,9 @@ put_zones_ofpacts(const struct zone_ids *zone_ids, struct ofpbuf *ofpacts_p) > if (zone_ids->snat) { > put_load(zone_ids->snat, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p); > } > + if (zone_ids->drop) { > + put_load(zone_ids->drop, MFF_LOG_ACL_DROP_ZONE, 0, 32, ofpacts_p); > + } > } > } > > @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key, > pb->header_.uuid.parts[0], &match, ofpacts_p, > &pb->header_.uuid); > > + if (zone_ids->drop) { > + /* Table 39, Priority 1. > + * ======================= > + * > + * Clear the logical registers (for consistent behavior with packets > + * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */ > + match_init_catchall(&match); > + ofpbuf_clear(ofpacts_p); > + match_set_metadata(&match, htonll(dp_key)); > + for (int i = 0; i < MFF_N_LOG_REGS; i++) { > + if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) { > + put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p); > + } > + } > + put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, ofpacts_p); > + ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 1, > + pb->datapath->header_.uuid.parts[0], &match, > + ofpacts_p, &pb->datapath->header_.uuid); > + } > + > /* Table 39, Priority 100. > * ======================= > * > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index 6ca08b3c6..c7a6785d3 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -125,6 +125,7 @@ struct ovn_extend_table; > OVNACT(COMMIT_LB_AFF, ovnact_commit_lb_aff) \ > OVNACT(CHK_LB_AFF, ovnact_result) \ > OVNACT(SAMPLE, ovnact_sample) \ > + OVNACT(CT_COMMIT_DROP, ovnact_nest) \ > > /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */ > enum OVS_PACKED_ENUM ovnact_type { > diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h > index a7b64ef67..c7ea9e0ce 100644 > --- a/include/ovn/logical-fields.h > +++ b/include/ovn/logical-fields.h > @@ -47,6 +47,7 @@ enum ovn_controller_event { > #define MFF_LOG_REG0 MFF_REG0 > #define MFF_LOG_LB_ORIG_DIP_IPV4 MFF_REG1 > #define MFF_LOG_LB_ORIG_TP_DPORT MFF_REG2 > +#define MFF_LOG_ACL_DROP_ZONE MFF_REG9 > > #define MFF_LOG_XXREG0 MFF_XXREG0 > #define MFF_LOG_LB_ORIG_DIP_IPV6 MFF_XXREG1 > diff --git a/lib/actions.c b/lib/actions.c > index 2da5a696b..d86b1efbc 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -5210,6 +5210,69 @@ encode_CHK_LB_AFF(const struct ovnact_result *res, > MLF_USE_LB_AFF_SESSION_BIT, ofpacts); > } > > +static void > +parse_ct_commit_drop(struct action_context *ctx) > +{ > + if (ctx->lexer->token.type == LEX_T_LCURLY) { > + parse_nested_action(ctx, OVNACT_CT_COMMIT_DROP, "ip", > + WR_CT_COMMIT); > + } else { > + /* Add an empty nested action to allow for "ct_commit_drop;" syntax */ > + add_prerequisite(ctx, "ip"); > + struct ovnact_nest *on = ovnact_put(ctx->ovnacts, > + OVNACT_CT_COMMIT_DROP, > + OVNACT_ALIGN(sizeof *on)); > + on->nested_len = 0; > + on->nested = NULL; > + } > +} > + > +static void > +format_CT_COMMIT_DROP(const struct ovnact_nest *on, struct ds *s) > +{ > + if (on->nested_len) { > + format_nested_action(on, "ct_commit_drop", s); > + } else { > + ds_put_cstr(s, "ct_commit_drop;"); > + } > +} > + > +static void > +encode_CT_COMMIT_DROP(const struct ovnact_nest *on, > + const struct ovnact_encode_params *ep OVS_UNUSED, > + struct ofpbuf *ofpacts) > +{ > + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > + ct->flags = NX_CT_F_COMMIT; > + ct->recirc_table = NX_CT_RECIRC_NONE; > + ct->zone_src.field = mf_from_id(MFF_LOG_ACL_DROP_ZONE); > + ct->zone_src.ofs = 0; > + ct->zone_src.n_bits = 16; > + > + /* If the datapath supports all-zero SNAT then use it to avoid tuple > + * collisions at commit time between NATed and firewalled-only sessions. > + */ > + if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) { > + size_t nat_offset = ofpacts->size; > + ofpbuf_pull(ofpacts, nat_offset); > + > + struct ofpact_nat *nat = ofpact_put_NAT(ofpacts); > + nat->flags = 0; > + nat->range_af = AF_UNSPEC; > + nat->flags |= NX_NAT_F_SRC; > + ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); > + ct = ofpacts->header; > + } > + > + size_t set_field_offset = ofpacts->size; > + ofpbuf_pull(ofpacts, set_field_offset); > + > + ovnacts_encode(on->nested, on->nested_len, ep, ofpacts); > + ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); > + ct = ofpacts->header; > + ofpact_finish(ofpacts, &ct->ofpact); > +} > + > /* Parses an assignment or exchange or put_dhcp_opts action. */ > static void > parse_set_action(struct action_context *ctx) > @@ -5415,6 +5478,8 @@ parse_action(struct action_context *ctx) > parse_commit_lb_aff(ctx, ovnact_put_COMMIT_LB_AFF(ctx->ovnacts)); > } else if (lexer_match_id(ctx->lexer, "sample")) { > parse_sample(ctx); > + } else if (lexer_match_id(ctx->lexer, "ct_commit_drop")) { > + parse_ct_commit_drop(ctx); > } else { > lexer_syntax_error(ctx->lexer, "expecting action"); > } > diff --git a/lib/ovn-util.c b/lib/ovn-util.c > index f3665b89f..646aff6ef 100644 > --- a/lib/ovn-util.c > +++ b/lib/ovn-util.c > @@ -443,12 +443,12 @@ split_addresses(const char *addresses, struct svec *ipv4_addrs, > destroy_lport_addresses(&laddrs); > } > > -/* Allocates a key for NAT conntrack zone allocation for a provided > +/* Allocates a key for conntrack zone allocation for a provided > * 'key' record and a 'type'. > * > * It is the caller's responsibility to free the allocated memory. */ > char * > -alloc_nat_zone_key(const struct uuid *key, const char *type) > +alloc_ct_zone_key(const struct uuid *key, const char *type) > { > return xasprintf(UUID_FMT"_%s", UUID_ARGS(key), type); > } > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index cd19919cb..170685872 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -92,7 +92,7 @@ const char *find_lport_address(const struct lport_addresses *laddrs, > void split_addresses(const char *addresses, struct svec *ipv4_addrs, > struct svec *ipv6_addrs); > > -char *alloc_nat_zone_key(const struct uuid *key, const char *type); > +char *alloc_ct_zone_key(const struct uuid *key, const char *type); > > const char *default_nb_db(void); > const char *default_sb_db(void); > diff --git a/northd/northd.c b/northd/northd.c > index 77e105b86..bc39e9bc6 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -84,6 +84,10 @@ static bool use_ct_inv_match = true; > */ > static bool default_acl_drop; > > +/* If this option is 'true', drop/reject ACLs with label are translated to > + * commit flows to conntrack besides dropping/rejecting them. */ > +static bool ct_commit_acl_drop; > + > #define MAX_OVN_TAGS 4096 > > /* Pipeline stages. */ > @@ -311,7 +315,7 @@ enum ovn_stage { > * +----+----------------------------------------------+---+-----------------------------------+ > * | R8 | LB_AFF_MATCH_PORT | > * +----+----------------------------------------------+ > - * | R9 | UNUSED | > + * | R9 | ACL DROP CT ZONE | > * +----+----------------------------------------------+ > * > * Logical Router pipeline: > @@ -6493,6 +6497,14 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > ds_clear(match); > ds_clear(actions); > ds_put_cstr(match, REGBIT_ACL_HINT_DROP " == 1"); > + /* If the ACL has a label, we commit the packet in > + * a separate zone for debugging purposes before > + * rejecting/dropping it. */ > + if (ct_commit_acl_drop && acl->label) { > + ds_put_format(actions, "ct_commit_drop { " > + "ct_label.label = %"PRId64"; }; ", > + acl->label); > + } > if (!strcmp(acl->action, "reject")) { > build_reject_acl_rules(od, lflows, stage, acl, match, > actions, &acl->header_, meter_groups); > @@ -6519,8 +6531,15 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > ds_clear(match); > ds_clear(actions); > ds_put_cstr(match, REGBIT_ACL_HINT_BLOCK " == 1"); > - ds_put_format(actions, "ct_commit { %s = 1; }; ", > + ds_put_format(actions, "ct_commit { %s = 1; ", > ct_blocked_match); > + /* Update ct_label.label to reflect the new policy matching > + * the connection. */ > + if (acl->label) { > + ds_put_format(actions, "ct_label.label = %"PRId64"; ", > + acl->label); > + } > + ds_put_cstr(actions, "}; "); > if (!strcmp(acl->action, "reject")) { > build_reject_acl_rules(od, lflows, stage, acl, match, > actions, &acl->header_, meter_groups); > @@ -16236,6 +16255,8 @@ ovnnb_db_run(struct northd_input *input_data, > check_lsp_is_up = !smap_get_bool(&nb->options, > "ignore_lsp_down", true); > default_acl_drop = smap_get_bool(&nb->options, "default_acl_drop", false); > + ct_commit_acl_drop = smap_get_bool(&nb->options, > + "ct_commit_acl_drop", false); > > install_ls_lb_from_router = smap_get_bool(&nb->options, > "install_ls_lb_from_router", > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 3d7a92ea8..74f34cfe8 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -727,13 +727,26 @@ > action for TCP connections,<code>icmp4/icmp6</code> action > for UDP connections, and <code>sctp_abort {output <-%gt; inport; > next(pipeline=egress,table=5);}</code> action for SCTP associations. > + Additionally, if the ACL has a <code>label</code> and > + <ref column="options:ct_commit_acl_drop" table="NB_Global" > + db="OVN_Northbound"/> column of <ref table="NB_Global" > + db="OVN_Northbound"/> table is <code>true</code>, the translated > + action includes <code>ct_commit_drop(ct_label.label=label)</code>. > </li> > <li> > Other ACLs translate to <code>drop;</code> for new or untracked > connections and <code>ct_commit(ct_label=1/1);</code> for known > connections. Setting <code>ct_label</code> marks a connection > as one that was previously allowed, but should no longer be > - allowed due to a policy change. > + allowed due to a policy change. If the ACLs have <code>label</code>, > + then they instead translate to > + <code>ct_commit_drop(ct_label.label=label)</code> for > + new and untracked connections if > + <ref column="options:ct_commit_acl_drop" table="NB_Global" > + db="OVN_Northbound"/> column of <ref table="NB_Global" > + db="OVN_Northbound"/> table is also set to <code>true</code> and > + <code>ct_commit(ct_label.blocked=1; ct_label.label=label);</code> > + for known connections. > </li> > </ul> > > @@ -1165,13 +1178,26 @@ > action for TCP connections,<code>icmp4/icmp6</code> action > for UDP connections, and <code>sctp_abort {output <-%gt; inport; > next(pipeline=egress,table=5);}</code> action for SCTP associations. > + Additionally, if the ACL has a <code>label</code> and > + <ref column="options:ct_commit_acl_drop" table="NB_Global" > + db="OVN_Northbound"/> column of <ref table="NB_Global" > + db="OVN_Northbound"/> table is <code>true</code>, the translated > + action includes <code>ct_commit_drop(ct_label.label=label)</code>. > </li> > <li> > Other apply-after-lb ACLs translate to <code>drop;</code> for new > or untracked connections and <code>ct_commit(ct_label=1/1);</code> for > known connections. Setting <code>ct_label</code> marks a connection > as one that was previously allowed, but should no longer be > - allowed due to a policy change. > + allowed due to a policy change. If the ACLs have <code>label</code>, > + then they instead translate to > + <code>ct_commit_drop(ct_label.label=label)</code> for > + new and untracked connections if > + <ref column="options:ct_commit_acl_drop" table="NB_Global" > + db="OVN_Northbound"/> column of <ref table="NB_Global" > + db="OVN_Northbound"/> table is also set to <code>true</code> and > + <code>ct_commit(ct_label.blocked=1; ct_label.label=label);</code> > + for known connections. > </li> > </ul> > > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 4b52b9953..d28c2fea8 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -301,6 +301,15 @@ > </p> > </column> > > + <column name="options" key="ct_commit_acl_drop"> > + <p> > + If set to <code>true</code>, connections dropped/rejected by ACLs > + with label are committed to DROP conntrack zone of > + <code>logical_switch</code>. By default this option is set to > + <code>false</code>. > + </p> > + </column> > + > <group title="Options for configuring interconnection route advertisement"> > <p> > These options control how routes are advertised between OVN > @@ -2106,12 +2115,10 @@ or > <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. > + tracking entry. The value should be a valid 32-bit unsigned integer. > + This value can help in debugging from connection tracking 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. > + which is causing a "leaked" connection. > </p> > </column> > <column name="priority"> > diff --git a/ovn-sb.xml b/ovn-sb.xml > index a77f8f4ef..f01f9a927 100644 > --- a/ovn-sb.xml > +++ b/ovn-sb.xml > @@ -1408,6 +1408,28 @@ > </p> > </dd> > > + <dt><code>ct_commit_drop { };</code></dt> > + <dt><code>ct_commit_drop { ct_mark=<var>value[/mask]</var>; };</code></dt> > + <dt><code>ct_commit_drop { ct_label=<var>value[/mask]</var>; };</code></dt> > + <dt><code>ct_commit_drop { ct_mark=<var>value[/mask]</var>; ct_label=<var>value[/mask]</var>; };</code></dt> > + <dd> > + <p> > + Commit the flow to a connection tracking entry in the > + logical switch DROP zone. This action is identical to > + <code>ct_commit</code> besides the connection tracking entry > + the flow is committed to. > + </p> > + > + <p> > + This action is useful to commit connection tracking state for > + packets before they are dropped for debugging purposes. > + Committing the packets in a separate zone ensures that connection > + tracking state of dropped connections doesn't interact with the > + state of allowed connections and allows the DROP zone to be > + managed separately. > + </p> > + </dd> > + > <dt><code>ct_dnat;</code></dt> > <dt><code>ct_dnat(<var>IP</var>);</code></dt> > <dd> > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index 2fffe1850..cfbb5e45a 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -222,7 +222,8 @@ ovn_nbctl_test_acl() { > 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]) > + AT_CHECK([ovn-nbctl $2 --label=1235 acl-add $1 to-lport 70 icmp reject]) > + AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 to-lport 50 ip drop]) > AT_CHECK([ovn-nbctl $2 --apply-after-lb acl-add $1 from-lport 500 tcp allow]) > AT_CHECK([ovn-nbctl $2 --apply-after-lb acl-add $1 from-lport 300 tcp drop]) > AT_CHECK([ovn-nbctl $2 --apply-after-lb acl-add $1 from-lport 300 udp allow]) > @@ -232,10 +233,6 @@ ovn_nbctl_test_acl() { > 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]) > > @@ -256,7 +253,8 @@ from-lport 300 (udp) allow [[after-lb]] > 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 > + to-lport 70 (icmp) reject label=1235 > + to-lport 50 (ip) drop label=1234 > ]) > > dnl Delete in one direction. > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 3fa02d2b3..9fbb82e37 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -4241,88 +4241,141 @@ ovn_start > check ovn-nbctl ls-add sw0 > check ovn-nbctl lsp-add sw0 sw0p1 > > +AS_BOX([Create ACLs with labels]) > check ovn-nbctl --wait=sb --label=1234 acl-add sw0 to-lport 1002 tcp allow-related > +check ovn-nbctl --wait=sb --label=2345 acl-add sw0 to-lport 1001 ip drop > check ovn-nbctl --wait=sb --label=1234 acl-add sw0 from-lport 1002 tcp allow-related > +check ovn-nbctl --wait=sb --label=2345 acl-add sw0 from-lport 1001 ip reject > > ovn-sbctl dump-flows sw0 > sw0flows > AT_CAPTURE_FILE([sw0flows]) > > -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table=?/'], [0], [dnl > - table=? (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > - table=? (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_in_acl ), priority=2001 , match=((reg0[[10]] == 1) && ip), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) > + table=??(ls_in_acl ), priority=2001 , match=((reg0[[9]] == 1) && ip), action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) > + table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=??(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 | sed 's/table=../table=??/'], [0], [dnl > +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;) > table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) > table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.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 -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_out_acl ), priority=2001 , match=(reg0[[10]] == 1 && (ip)), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; /* drop */) > + table=??(ls_out_acl ), priority=2001 , match=(reg0[[9]] == 1 && (ip)), action=(/* drop */) > + table=??(ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=??(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_mark.blocked = 0; }; next;) > - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) > +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_out_stateful ), priority=0 , match=(1), action=(next;) > + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) > + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) > +]) > + > +AS_BOX([Set ct_commit_acl_drop option]) > +check ovn-nbctl --wait=sb set NB_Global . options:ct_commit_acl_drop=true > + > +ovn-sbctl dump-flows sw0 > sw0flows > +AT_CAPTURE_FILE([sw0flows]) > + > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_in_acl ), priority=2001 , match=((reg0[[10]] == 1) && ip), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) > + table=??(ls_in_acl ), priority=2001 , match=((reg0[[9]] == 1) && ip), action=(ct_commit_drop { ct_label.label = 2345; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) > + table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=??(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 | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) > + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) > ]) > > -# Add new ACL without label > +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_out_acl ), priority=2001 , match=(reg0[[10]] == 1 && (ip)), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; /* drop */) > + table=??(ls_out_acl ), priority=2001 , match=(reg0[[9]] == 1 && (ip)), action=(ct_commit_drop { ct_label.label = 2345; }; /* drop */) > + table=??(ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=??(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 | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_out_stateful ), priority=0 , match=(1), action=(next;) > + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) > + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) > +]) > + > +AS_BOX([Add ACLs without labels]) > check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 udp allow-related > +check ovn-nbctl --wait=sb acl-add sw0 to-lport 1001 ip6 drop > check ovn-nbctl --wait=sb acl-add sw0 from-lport 1002 udp allow-related > +check ovn-nbctl --wait=sb acl-add sw0 from-lport 1001 ip6 reject > > ovn-sbctl dump-flows sw0 > sw0flows > AT_CAPTURE_FILE([sw0flows]) > > -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table=?/'], [0], [dnl > - table=? (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > - table=? (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > - table=? (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > - table=? (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) > -]) > -AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_in_acl ), priority=2001 , match=((reg0[[10]] == 1) && ip), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) > + table=??(ls_in_acl ), priority=2001 , match=((reg0[[10]] == 1) && ip6), action=(ct_commit { ct_mark.blocked = 1; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) > + table=??(ls_in_acl ), priority=2001 , match=((reg0[[9]] == 1) && ip), action=(ct_commit_drop { ct_label.label = 2345; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) > + table=??(ls_in_acl ), priority=2001 , match=((reg0[[9]] == 1) && ip6), action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) > + table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > + table=??(ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=??(ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) > +]) > +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;) > table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) > table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.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 -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_out_acl ), priority=2001 , match=(reg0[[10]] == 1 && (ip)), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; /* drop */) > + table=??(ls_out_acl ), priority=2001 , match=(reg0[[10]] == 1 && (ip6)), action=(ct_commit { ct_mark.blocked = 1; }; /* drop */) > + table=??(ls_out_acl ), priority=2001 , match=(reg0[[9]] == 1 && (ip)), action=(ct_commit_drop { ct_label.label = 2345; }; /* drop */) > + table=??(ls_out_acl ), priority=2001 , match=(reg0[[9]] == 1 && (ip6)), action=(/* drop */) > + table=??(ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=??(ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > + table=??(ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) > + table=??(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_mark.blocked = 0; }; next;) > - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) > +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_out_stateful ), priority=0 , match=(1), action=(next;) > + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) > + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) > ]) > > -# Delete new ACL with label > +AS_BOX([Delete ACLs with labels]) > check ovn-nbctl --wait=sb acl-del sw0 to-lport 1002 tcp > +check ovn-nbctl --wait=sb acl-del sw0 to-lport 1001 ip > check ovn-nbctl --wait=sb acl-del sw0 from-lport 1002 tcp > +check ovn-nbctl --wait=sb acl-del sw0 from-lport 1001 ip > > ovn-sbctl dump-flows sw0 > sw0flows > AT_CAPTURE_FILE([sw0flows]) > > -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table=?/'], [0], [dnl > - table=? (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > - table=? (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) > +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_in_acl ), priority=2001 , match=((reg0[[10]] == 1) && ip6), action=(ct_commit { ct_mark.blocked = 1; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) > + table=??(ls_in_acl ), priority=2001 , match=((reg0[[9]] == 1) && ip6), action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) > + table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > + table=??(ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) > ]) > -AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl > +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;) > table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) > table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.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 -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_out_acl ), priority=2001 , match=(reg0[[10]] == 1 && (ip6)), action=(ct_commit { ct_mark.blocked = 1; }; /* drop */) > + table=??(ls_out_acl ), priority=2001 , match=(reg0[[9]] == 1 && (ip6)), action=(/* drop */) > + table=??(ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) > + table=??(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_mark.blocked = 0; }; next;) > - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) > +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl > + table=??(ls_out_stateful ), priority=0 , match=(1), action=(next;) > + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) > + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) > ]) > AT_CLEANUP > ]) > diff --git a/tests/ovn.at b/tests/ovn.at > index e9b8bc677..fc7abfdad 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1323,6 +1323,58 @@ ct_label = 0xcafe > ct_label.blocked = 1/1 > Field ct_label.blocked is not modifiable. > > +# ct_commit_drop > +ct_commit_drop; > + encodes as ct(commit,zone=NXM_NX_REG9[0..15]) > + has prereqs ip > +ct_commit_drop { }; > + formats as ct_commit_drop; > + encodes as ct(commit,zone=NXM_NX_REG9[0..15]) > + has prereqs ip > +ct_commit_drop { ct_mark=1; }; > + formats as ct_commit_drop { ct_mark = 1; }; > + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1->ct_mark)) > + has prereqs ip > +ct_commit_drop { ct_mark=1/1; }; > + formats as ct_commit_drop { ct_mark = 1/1; }; > + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1/0x1->ct_mark)) > + has prereqs ip > +ct_commit_drop { ct_label=1; }; > + formats as ct_commit_drop { ct_label = 1; }; > + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1->ct_label)) > + has prereqs ip > +ct_commit_drop { ct_label=1/1; }; > + formats as ct_commit_drop { ct_label = 1/1; }; > + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1/0x1->ct_label)) > + has prereqs ip > +ct_commit_drop { ct_mark=1; ct_label=2; }; > + formats as ct_commit_drop { ct_mark = 1; ct_label = 2; }; > + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label)) > + has prereqs ip > + > +ct_commit_drop { ct_label=0x01020304050607080910111213141516; }; > + formats as ct_commit_drop { ct_label = 0x1020304050607080910111213141516; }; > + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1020304050607080910111213141516->ct_label)) > + has prereqs ip > +ct_commit_drop { ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000; }; > + formats as ct_commit_drop { ct_label = 0x1000000000000000000000000000000/0x1000000000000000000000000000000; }; > + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1000000000000000000000000000000/0x1000000000000000000000000000000->ct_label)) > + has prereqs ip > +ct_commit_drop { ct_label=18446744073709551615; }; > + formats as ct_commit_drop { ct_label = 18446744073709551615; }; > + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0xffffffffffffffff->ct_label)) > + has prereqs ip > +ct_commit_drop { ct_label[0..47] = 0x00000f040201; ct_label[48..63] = 0x0002; }; > + formats as ct_commit_drop { ct_label[0..47] = 0xf040201; ct_label[48..63] = 0x2; }; > + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0xf040201/0xffffffffffff->ct_label,set_field:0x2000000000000/0xffff000000000000->ct_label)) > + has prereqs ip > +ct_commit_drop { ct_label=18446744073709551616; }; > + Decimal constants must be less than 2**64. > +ct_commit_drop { ct_label=0x181716151413121110090807060504030201; }; > + 141-bit constant is not compatible with 128-bit field ct_label. > +ct_commit_drop { ip4.dst = 192.168.0.1; }; > + Field ip4.dst is not modifiable. > + > # ct_dnat > ct_dnat; > encodes as ct(table=19,zone=NXM_NX_REG11[0..15],nat) > @@ -33637,7 +33689,7 @@ ovn-nbctl lsp-add sw0 sw0-port1 -- lsp-set-addresses sw0-port1 "50:54:00:00:00:0 > ovs-vsctl set interface p0 external-ids:iface-id=sw0-port0 > ovs-vsctl set interface p1 external-ids:iface-id=sw0-port1 > check ovn-nbctl --wait=hv sync > -as hv1 ovs-ofctl dump-flows br-int | sed 's/cookie=0x.*, duration=.*, table/cookie=??, duration=??, table/' | sed 's/load:0x.->NXM_NX_REG1/load:0x?->NXM_NX_REG1/g' | sed 's/idle_age=[[0-9]], //g' | sort > offlows1 > +as hv1 ovs-ofctl dump-flows br-int | sed 's/cookie=0x.*, duration=.*, table/cookie=??, duration=??, table/' | sed 's/load:0x.->NXM_NX_REG\([[19]]\)/load:0x?->NXM_NX_REG\1/g' | sed 's/idle_age=[[0-9]], //g' | sort > offlows1 > > ovs-vsctl set interface p0 external-ids:iface-id=foo0 > ovs-vsctl set interface p1 external-ids:iface-id=foo1 > @@ -33664,7 +33716,7 @@ sleep 0.5 > # Restart SB > AT_CHECK([kill -CONT $(cat ovn-sb/ovsdb-server.pid)]) > check ovn-nbctl --wait=hv sync > -as hv1 ovs-ofctl dump-flows br-int | sed 's/cookie=0x.*, duration=.*, table/cookie=??, duration=??, table/' | sed 's/load:0x.->NXM_NX_REG1/load:0x?->NXM_NX_REG1/g' | sed 's/idle_age=[[0-9]], //g' | sort > offlows2 > +as hv1 ovs-ofctl dump-flows br-int | sed 's/cookie=0x.*, duration=.*, table/cookie=??, duration=??, table/' | sed 's/load:0x.->NXM_NX_REG\([[19]]\)/load:0x?->NXM_NX_REG\1/g' | sed 's/idle_age=[[0-9]], //g' | sort > offlows2 > AT_CHECK([diff offlows1 offlows2]) > > ovn-nbctl ls-del sw0 > @@ -34689,3 +34741,37 @@ check_packets > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-controller - check logical_switch drop ct zone]) > +ovn_start > +net_add n1 > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > + > +check ovn-nbctl ls-add ls0 > +check ovn-nbctl lsp-add ls0 lsp0 > +check ovn-nbctl lsp-set-addresses lsp0 "00:00:00:00:00:02 10.0.0.2" > + > +ovs-vsctl -- add-port br-int vif0 -- set interface vif0 external-ids:iface-id=lsp0 > + > +# Wait for ovn-northd and ovn-controller to catch up. > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > + > +# logical_switch should have a CT zone for connections dropped by labeled ACLs > +ls_uuid=$(fetch_column Datapath_Binding _uuid external_ids:name=ls0) > +AT_CHECK([as hv1 ovn-appctl -t ovn-controller ct-zone-list | \ > +grep ${ls_uuid}_drop -c], [0], [1 > +]) > + > +# Check that register storing the drop ct zone is not cleared. > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=39 | grep -w 'priority=1' | ofctl_strip_all], [0], [dnl > + table=39, priority=1,metadata=0x1 actions=load:0->NXM_NX_REG0[[]],load:0->NXM_NX_REG1[[]],load:0->NXM_NX_REG2[[]],load:0->NXM_NX_REG3[[]],load:0->NXM_NX_REG4[[]],load:0->NXM_NX_REG5[[]],load:0->NXM_NX_REG6[[]],load:0->NXM_NX_REG7[[]],load:0->NXM_NX_REG8[[]],resubmit(,40) > +]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > index ae4d6c403..74fa04539 100644 > --- a/utilities/ovn-nbctl.c > +++ b/utilities/ovn-nbctl.c > @@ -2367,13 +2367,6 @@ nbctl_acl_add(struct ctl_context *ctx) > /* 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) { > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > index e5766ed67..5cede3527 100644 > --- a/utilities/ovn-trace.c > +++ b/utilities/ovn-trace.c > @@ -3356,6 +3356,8 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len, > break; > case OVNACT_SAMPLE: > break; > + case OVNACT_CT_COMMIT_DROP: > + break; > } > } > ofpbuf_uninit(&stack); > -- > 2.22.3 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> On 18 Mar 2023, at 01:04, Numan Siddique <numans@ovn.org> wrote: > > On Mon, Feb 13, 2023 at 11:36 AM Abhiram Sangana > <sangana.abhiram@nutanix.com> wrote: >> >> This patch adds support to commit connections dropped/rejected by >> ACLs to the connection tracking table. Dropped connections are >> committed to conntrack only if NB_Global options:ct_commit_acl_drop >> is set to true (false by default) and ACL dropping/rejecting the >> connection has label configured. The dropped connections are >> committed in a separate conntrack zone so that they can be managed >> independently and do not interact with the connection tracking state >> of allowed connections. >> >> This provides a new approach to identify connections dropped by ACLs >> besides the existing ACL logging and drop sampling approaches. >> >> Each logical switch is assigned a new conntrack zone for committing >> dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE. >> A new lflow action "ct_commit_drop" is introduced that commits flows >> to connection tracking table in a zone identified by >> MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action >> and non-empty label translates to include "ct_commit_drop" in its >> actions instead of simply dropping/rejecting the packet. >> >> Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com> > Hi Numan, Thanks for reviewing the patch. > Hi Abhiram, > > Sorry for the delays in the reviews. > > Overall the patch looks good to me. > > I do have a few comments and suggestions. > > Instead of adding a global option, how about an option per ACL (in the > ACL options column) ? > Users can enable or disable conntrack commit per ACL. Sure, will do that. > > Are you going to use this feature all the time in your deployment ? > Or enable it only when debugging an issue ? > If you want to enable it during debugging, is it a bit of a hassle to > enable the ct_commit option for the interested ACL ? Yes, we are planning to use this feature all the time in our deployment. So, it shouldn't be an issue to enable ct_commit option for interested ACLs. > > I'm a little bit concerned with allocating a zone id for each logical > switch even if there are no ACLs configured with this option. > But I think it's not a big deal. Yes, I did try to look into allocating CT zone on an as-needed basis but had some difficulty implementing it. Will try to explore further in a subsequent patch. Thanks, Abhiram > > Thanks > Numan
On 3/17/23 20:34, Numan Siddique wrote: > On Mon, Feb 13, 2023 at 11:36 AM Abhiram Sangana > <sangana.abhiram@nutanix.com> wrote: >> >> This patch adds support to commit connections dropped/rejected by >> ACLs to the connection tracking table. Dropped connections are >> committed to conntrack only if NB_Global options:ct_commit_acl_drop >> is set to true (false by default) and ACL dropping/rejecting the >> connection has label configured. The dropped connections are >> committed in a separate conntrack zone so that they can be managed >> independently and do not interact with the connection tracking state >> of allowed connections. >> >> This provides a new approach to identify connections dropped by ACLs >> besides the existing ACL logging and drop sampling approaches. >> >> Each logical switch is assigned a new conntrack zone for committing >> dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE. >> A new lflow action "ct_commit_drop" is introduced that commits flows >> to connection tracking table in a zone identified by >> MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action >> and non-empty label translates to include "ct_commit_drop" in its >> actions instead of simply dropping/rejecting the packet. >> >> Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com> > > Hi Abhiram, > Hi Abhiram, > Sorry for the delays in the reviews. > > Overall the patch looks good to me. > > I do have a few comments and suggestions. > > Instead of adding a global option, how about an option per ACL (in the > ACL options column) ? > Users can enable or disable conntrack commit per ACL. > +1 for this, finer granularity is probably better. I only have a few minor comments below. > Are you going to use this feature all the time in your deployment ? > Or enable it only when debugging an issue ? > If you want to enable it during debugging, is it a bit of a hassle to > enable the ct_commit option for the interested ACL ? > > > I'm a little bit concerned with allocating a zone id for each logical > switch even if there are no ACLs configured with this option. > But I think it's not a big deal. > > Thanks > Numan > >> --- >> controller/ovn-controller.c | 14 +++- >> controller/physical.c | 32 ++++++++- >> include/ovn/actions.h | 1 + >> include/ovn/logical-fields.h | 1 + >> lib/actions.c | 65 +++++++++++++++++ >> lib/ovn-util.c | 4 +- >> lib/ovn-util.h | 2 +- >> northd/northd.c | 25 ++++++- >> northd/ovn-northd.8.xml | 30 +++++++- >> ovn-nb.xml | 17 +++-- >> ovn-sb.xml | 22 ++++++ >> tests/ovn-nbctl.at | 10 ++- >> tests/ovn-northd.at | 133 ++++++++++++++++++++++++----------- >> tests/ovn.at | 90 +++++++++++++++++++++++- >> utilities/ovn-nbctl.c | 7 -- >> utilities/ovn-trace.c | 2 + Please also add a NEWS entry. >> 16 files changed, 383 insertions(+), 72 deletions(-) >> >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >> index 265740cab..e8f0b7242 100644 >> --- a/controller/ovn-controller.c >> +++ b/controller/ovn-controller.c >> @@ -734,8 +734,8 @@ update_ct_zones(const struct shash *binding_lports, >> HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { >> /* XXX Add method to limit zone assignment to logical router >> * datapaths with NAT */ >> - char *dnat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "dnat"); >> - char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat"); >> + char *dnat = alloc_ct_zone_key(&ld->datapath->header_.uuid, "dnat"); >> + char *snat = alloc_ct_zone_key(&ld->datapath->header_.uuid, "snat"); >> sset_add(&all_users, dnat); >> sset_add(&all_users, snat); >> >> @@ -745,6 +745,14 @@ update_ct_zones(const struct shash *binding_lports, >> } >> free(dnat); >> free(snat); >> + >> + /* Zone for committing connections dropped by ACLs with labels. */ >> + if (ld->is_switch) { >> + char *drop = alloc_ct_zone_key( >> + &ld->datapath->header_.uuid, "drop"); >> + sset_add(&all_users, drop); >> + free(drop); >> + } >> } >> >> /* Delete zones that do not exist in above sset. */ >> @@ -2420,7 +2428,7 @@ ct_zones_datapath_binding_handler(struct engine_node *node, void *data) >> /* Check if the requested snat zone has changed for the datapath >> * or not. If so, then fall back to full recompute of >> * ct_zone engine. */ >> - char *snat_dp_zone_key = alloc_nat_zone_key(&dp->header_.uuid, "snat"); >> + char *snat_dp_zone_key = alloc_ct_zone_key(&dp->header_.uuid, "snat"); >> struct simap_node *simap_node = simap_find(&ct_zones_data->current, >> snat_dp_zone_key); >> free(snat_dp_zone_key); >> diff --git a/controller/physical.c b/controller/physical.c >> index 4dcf44e01..3c573c492 100644 >> --- a/controller/physical.c >> +++ b/controller/physical.c >> @@ -60,6 +60,7 @@ struct zone_ids { >> int ct; /* MFF_LOG_CT_ZONE. */ >> int dnat; /* MFF_LOG_DNAT_ZONE. */ >> int snat; /* MFF_LOG_SNAT_ZONE. */ >> + int drop; /* MFF_LOG_ACL_DROP_ZONE. */ >> }; >> >> struct tunnel { >> @@ -204,14 +205,18 @@ get_zone_ids(const struct sbrec_port_binding *binding, >> >> const struct uuid *key = &binding->datapath->header_.uuid; >> >> - char *dnat = alloc_nat_zone_key(key, "dnat"); >> + char *dnat = alloc_ct_zone_key(key, "dnat"); >> zone_ids.dnat = simap_get(ct_zones, dnat); >> free(dnat); >> >> - char *snat = alloc_nat_zone_key(key, "snat"); >> + char *snat = alloc_ct_zone_key(key, "snat"); >> zone_ids.snat = simap_get(ct_zones, snat); >> free(snat); >> >> + char *drop = alloc_ct_zone_key(key, "drop"); >> + zone_ids.drop = simap_get(ct_zones, drop); >> + free(drop); >> + >> return zone_ids; >> } >> >> @@ -830,6 +835,9 @@ put_zones_ofpacts(const struct zone_ids *zone_ids, struct ofpbuf *ofpacts_p) >> if (zone_ids->snat) { >> put_load(zone_ids->snat, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p); >> } >> + if (zone_ids->drop) { >> + put_load(zone_ids->drop, MFF_LOG_ACL_DROP_ZONE, 0, 32, ofpacts_p); >> + } >> } >> } >> >> @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key, >> pb->header_.uuid.parts[0], &match, ofpacts_p, >> &pb->header_.uuid); >> >> + if (zone_ids->drop) { >> + /* Table 39, Priority 1. >> + * ======================= >> + * >> + * Clear the logical registers (for consistent behavior with packets >> + * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */ >> + match_init_catchall(&match); >> + ofpbuf_clear(ofpacts_p); >> + match_set_metadata(&match, htonll(dp_key)); >> + for (int i = 0; i < MFF_N_LOG_REGS; i++) { >> + if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) { >> + put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p); >> + } >> + } Why do we need this? Don't we load the CT zone anyway for "to-lport" ACLs? Can't we also load the drop zone in the same place? >> + put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, ofpacts_p); >> + ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 1, >> + pb->datapath->header_.uuid.parts[0], &match, >> + ofpacts_p, &pb->datapath->header_.uuid); >> + } >> + >> /* Table 39, Priority 100. >> * ======================= >> * >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >> index 6ca08b3c6..c7a6785d3 100644 >> --- a/include/ovn/actions.h >> +++ b/include/ovn/actions.h >> @@ -125,6 +125,7 @@ struct ovn_extend_table; >> OVNACT(COMMIT_LB_AFF, ovnact_commit_lb_aff) \ >> OVNACT(CHK_LB_AFF, ovnact_result) \ >> OVNACT(SAMPLE, ovnact_sample) \ >> + OVNACT(CT_COMMIT_DROP, ovnact_nest) \ >> >> /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */ >> enum OVS_PACKED_ENUM ovnact_type { >> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h >> index a7b64ef67..c7ea9e0ce 100644 >> --- a/include/ovn/logical-fields.h >> +++ b/include/ovn/logical-fields.h >> @@ -47,6 +47,7 @@ enum ovn_controller_event { >> #define MFF_LOG_REG0 MFF_REG0 >> #define MFF_LOG_LB_ORIG_DIP_IPV4 MFF_REG1 >> #define MFF_LOG_LB_ORIG_TP_DPORT MFF_REG2 >> +#define MFF_LOG_ACL_DROP_ZONE MFF_REG9 >> >> #define MFF_LOG_XXREG0 MFF_XXREG0 >> #define MFF_LOG_LB_ORIG_DIP_IPV6 MFF_XXREG1 >> diff --git a/lib/actions.c b/lib/actions.c >> index 2da5a696b..d86b1efbc 100644 >> --- a/lib/actions.c >> +++ b/lib/actions.c >> @@ -5210,6 +5210,69 @@ encode_CHK_LB_AFF(const struct ovnact_result *res, >> MLF_USE_LB_AFF_SESSION_BIT, ofpacts); >> } >> >> +static void >> +parse_ct_commit_drop(struct action_context *ctx) >> +{ >> + if (ctx->lexer->token.type == LEX_T_LCURLY) { >> + parse_nested_action(ctx, OVNACT_CT_COMMIT_DROP, "ip", >> + WR_CT_COMMIT); >> + } else { >> + /* Add an empty nested action to allow for "ct_commit_drop;" syntax */ >> + add_prerequisite(ctx, "ip"); >> + struct ovnact_nest *on = ovnact_put(ctx->ovnacts, >> + OVNACT_CT_COMMIT_DROP, >> + OVNACT_ALIGN(sizeof *on)); >> + on->nested_len = 0; >> + on->nested = NULL; >> + } >> +} >> + >> +static void >> +format_CT_COMMIT_DROP(const struct ovnact_nest *on, struct ds *s) >> +{ >> + if (on->nested_len) { >> + format_nested_action(on, "ct_commit_drop", s); >> + } else { >> + ds_put_cstr(s, "ct_commit_drop;"); >> + } >> +} >> + >> +static void >> +encode_CT_COMMIT_DROP(const struct ovnact_nest *on, >> + const struct ovnact_encode_params *ep OVS_UNUSED, >> + struct ofpbuf *ofpacts) >> +{ >> + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); >> + ct->flags = NX_CT_F_COMMIT; >> + ct->recirc_table = NX_CT_RECIRC_NONE; >> + ct->zone_src.field = mf_from_id(MFF_LOG_ACL_DROP_ZONE); >> + ct->zone_src.ofs = 0; >> + ct->zone_src.n_bits = 16; >> + >> + /* If the datapath supports all-zero SNAT then use it to avoid tuple >> + * collisions at commit time between NATed and firewalled-only sessions. >> + */ >> + if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) { >> + size_t nat_offset = ofpacts->size; >> + ofpbuf_pull(ofpacts, nat_offset); >> + >> + struct ofpact_nat *nat = ofpact_put_NAT(ofpacts); >> + nat->flags = 0; >> + nat->range_af = AF_UNSPEC; >> + nat->flags |= NX_NAT_F_SRC; >> + ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); >> + ct = ofpacts->header; >> + } I'm not completely sure about the all-zero SNAT for dropped packets. Do we really need it? Thanks, Dumitru
Hi Dumitru, Thanks for reviewing the patch. > On 23 Mar 2023, at 20:59, Dumitru Ceara <dceara@redhat.com> wrote: > > On 3/17/23 20:34, Numan Siddique wrote: >> On Mon, Feb 13, 2023 at 11:36 AM Abhiram Sangana >> <sangana.abhiram@nutanix.com> wrote: >>> >>> This patch adds support to commit connections dropped/rejected by >>> ACLs to the connection tracking table. Dropped connections are >>> committed to conntrack only if NB_Global options:ct_commit_acl_drop >>> is set to true (false by default) and ACL dropping/rejecting the >>> connection has label configured. The dropped connections are >>> committed in a separate conntrack zone so that they can be managed >>> independently and do not interact with the connection tracking state >>> of allowed connections. >>> >>> This provides a new approach to identify connections dropped by ACLs >>> besides the existing ACL logging and drop sampling approaches. >>> >>> Each logical switch is assigned a new conntrack zone for committing >>> dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE. >>> A new lflow action "ct_commit_drop" is introduced that commits flows >>> to connection tracking table in a zone identified by >>> MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action >>> and non-empty label translates to include "ct_commit_drop" in its >>> actions instead of simply dropping/rejecting the packet. >>> >>> Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com> >> >> Hi Abhiram, >> > > Hi Abhiram, > >> Sorry for the delays in the reviews. >> >> Overall the patch looks good to me. >> >> I do have a few comments and suggestions. >> >> Instead of adding a global option, how about an option per ACL (in the >> ACL options column) ? >> Users can enable or disable conntrack commit per ACL. >> > > +1 for this, finer granularity is probably better. Sure, will add an option per ACL. > I only have a few minor comments below. > >> Are you going to use this feature all the time in your deployment ? >> Or enable it only when debugging an issue ? >> If you want to enable it during debugging, is it a bit of a hassle to >> enable the ct_commit option for the interested ACL ? >> >> >> I'm a little bit concerned with allocating a zone id for each logical >> switch even if there are no ACLs configured with this option. >> But I think it's not a big deal. >> >> Thanks >> Numan >> >>> --- >>> controller/ovn-controller.c | 14 +++- >>> controller/physical.c | 32 ++++++++- >>> include/ovn/actions.h | 1 + >>> include/ovn/logical-fields.h | 1 + >>> lib/actions.c | 65 +++++++++++++++++ >>> lib/ovn-util.c | 4 +- >>> lib/ovn-util.h | 2 +- >>> northd/northd.c | 25 ++++++- >>> northd/ovn-northd.8.xml | 30 +++++++- >>> ovn-nb.xml | 17 +++-- >>> ovn-sb.xml | 22 ++++++ >>> tests/ovn-nbctl.at | 10 ++- >>> tests/ovn-northd.at | 133 ++++++++++++++++++++++++----------- >>> tests/ovn.at | 90 +++++++++++++++++++++++- >>> utilities/ovn-nbctl.c | 7 -- >>> utilities/ovn-trace.c | 2 + > > Please also add a NEWS entry. Sure. > >>> 16 files changed, 383 insertions(+), 72 deletions(-) >>> >>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>> index 265740cab..e8f0b7242 100644 >>> --- a/controller/ovn-controller.c >>> +++ b/controller/ovn-controller.c >>> @@ -734,8 +734,8 @@ update_ct_zones(const struct shash *binding_lports, >>> HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { >>> /* XXX Add method to limit zone assignment to logical router >>> * datapaths with NAT */ >>> - char *dnat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "dnat"); >>> - char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat"); >>> + char *dnat = alloc_ct_zone_key(&ld->datapath->header_.uuid, "dnat"); >>> + char *snat = alloc_ct_zone_key(&ld->datapath->header_.uuid, "snat"); >>> sset_add(&all_users, dnat); >>> sset_add(&all_users, snat); >>> >>> @@ -745,6 +745,14 @@ update_ct_zones(const struct shash *binding_lports, >>> } >>> free(dnat); >>> free(snat); >>> + >>> + /* Zone for committing connections dropped by ACLs with labels. */ >>> + if (ld->is_switch) { >>> + char *drop = alloc_ct_zone_key( >>> + &ld->datapath->header_.uuid, "drop"); >>> + sset_add(&all_users, drop); >>> + free(drop); >>> + } >>> } >>> >>> /* Delete zones that do not exist in above sset. */ >>> @@ -2420,7 +2428,7 @@ ct_zones_datapath_binding_handler(struct engine_node *node, void *data) >>> /* Check if the requested snat zone has changed for the datapath >>> * or not. If so, then fall back to full recompute of >>> * ct_zone engine. */ >>> - char *snat_dp_zone_key = alloc_nat_zone_key(&dp->header_.uuid, "snat"); >>> + char *snat_dp_zone_key = alloc_ct_zone_key(&dp->header_.uuid, "snat"); >>> struct simap_node *simap_node = simap_find(&ct_zones_data->current, >>> snat_dp_zone_key); >>> free(snat_dp_zone_key); >>> diff --git a/controller/physical.c b/controller/physical.c >>> index 4dcf44e01..3c573c492 100644 >>> --- a/controller/physical.c >>> +++ b/controller/physical.c >>> @@ -60,6 +60,7 @@ struct zone_ids { >>> int ct; /* MFF_LOG_CT_ZONE. */ >>> int dnat; /* MFF_LOG_DNAT_ZONE. */ >>> int snat; /* MFF_LOG_SNAT_ZONE. */ >>> + int drop; /* MFF_LOG_ACL_DROP_ZONE. */ >>> }; >>> >>> struct tunnel { >>> @@ -204,14 +205,18 @@ get_zone_ids(const struct sbrec_port_binding *binding, >>> >>> const struct uuid *key = &binding->datapath->header_.uuid; >>> >>> - char *dnat = alloc_nat_zone_key(key, "dnat"); >>> + char *dnat = alloc_ct_zone_key(key, "dnat"); >>> zone_ids.dnat = simap_get(ct_zones, dnat); >>> free(dnat); >>> >>> - char *snat = alloc_nat_zone_key(key, "snat"); >>> + char *snat = alloc_ct_zone_key(key, "snat"); >>> zone_ids.snat = simap_get(ct_zones, snat); >>> free(snat); >>> >>> + char *drop = alloc_ct_zone_key(key, "drop"); >>> + zone_ids.drop = simap_get(ct_zones, drop); >>> + free(drop); >>> + >>> return zone_ids; >>> } >>> >>> @@ -830,6 +835,9 @@ put_zones_ofpacts(const struct zone_ids *zone_ids, struct ofpbuf *ofpacts_p) >>> if (zone_ids->snat) { >>> put_load(zone_ids->snat, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p); >>> } >>> + if (zone_ids->drop) { >>> + put_load(zone_ids->drop, MFF_LOG_ACL_DROP_ZONE, 0, 32, ofpacts_p); >>> + } >>> } >>> } >>> >>> @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key, >>> pb->header_.uuid.parts[0], &match, ofpacts_p, >>> &pb->header_.uuid); >>> >>> + if (zone_ids->drop) { >>> + /* Table 39, Priority 1. >>> + * ======================= >>> + * >>> + * Clear the logical registers (for consistent behavior with packets >>> + * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */ >>> + match_init_catchall(&match); >>> + ofpbuf_clear(ofpacts_p); >>> + match_set_metadata(&match, htonll(dp_key)); >>> + for (int i = 0; i < MFF_N_LOG_REGS; i++) { >>> + if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) { >>> + put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p); >>> + } >>> + } > > Why do we need this? Don't we load the CT zone anyway for "to-lport" > ACLs? Can't we also load the drop zone in the same place? Yes, we are loading the drop zone (reg 9) in the same place as CT zone for “to-lport” ACLs (reg 13) but this happens before table=39 where registers 0 to 9 are cleared. > >>> + put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, ofpacts_p); >>> + ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 1, >>> + pb->datapath->header_.uuid.parts[0], &match, >>> + ofpacts_p, &pb->datapath->header_.uuid); >>> + } >>> + >>> /* Table 39, Priority 100. >>> * ======================= >>> * >>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >>> index 6ca08b3c6..c7a6785d3 100644 >>> --- a/include/ovn/actions.h >>> +++ b/include/ovn/actions.h >>> @@ -125,6 +125,7 @@ struct ovn_extend_table; >>> OVNACT(COMMIT_LB_AFF, ovnact_commit_lb_aff) \ >>> OVNACT(CHK_LB_AFF, ovnact_result) \ >>> OVNACT(SAMPLE, ovnact_sample) \ >>> + OVNACT(CT_COMMIT_DROP, ovnact_nest) \ >>> >>> /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */ >>> enum OVS_PACKED_ENUM ovnact_type { >>> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h >>> index a7b64ef67..c7ea9e0ce 100644 >>> --- a/include/ovn/logical-fields.h >>> +++ b/include/ovn/logical-fields.h >>> @@ -47,6 +47,7 @@ enum ovn_controller_event { >>> #define MFF_LOG_REG0 MFF_REG0 >>> #define MFF_LOG_LB_ORIG_DIP_IPV4 MFF_REG1 >>> #define MFF_LOG_LB_ORIG_TP_DPORT MFF_REG2 >>> +#define MFF_LOG_ACL_DROP_ZONE MFF_REG9 >>> >>> #define MFF_LOG_XXREG0 MFF_XXREG0 >>> #define MFF_LOG_LB_ORIG_DIP_IPV6 MFF_XXREG1 >>> diff --git a/lib/actions.c b/lib/actions.c >>> index 2da5a696b..d86b1efbc 100644 >>> --- a/lib/actions.c >>> +++ b/lib/actions.c >>> @@ -5210,6 +5210,69 @@ encode_CHK_LB_AFF(const struct ovnact_result *res, >>> MLF_USE_LB_AFF_SESSION_BIT, ofpacts); >>> } >>> >>> +static void >>> +parse_ct_commit_drop(struct action_context *ctx) >>> +{ >>> + if (ctx->lexer->token.type == LEX_T_LCURLY) { >>> + parse_nested_action(ctx, OVNACT_CT_COMMIT_DROP, "ip", >>> + WR_CT_COMMIT); >>> + } else { >>> + /* Add an empty nested action to allow for "ct_commit_drop;" syntax */ >>> + add_prerequisite(ctx, "ip"); >>> + struct ovnact_nest *on = ovnact_put(ctx->ovnacts, >>> + OVNACT_CT_COMMIT_DROP, >>> + OVNACT_ALIGN(sizeof *on)); >>> + on->nested_len = 0; >>> + on->nested = NULL; >>> + } >>> +} >>> + >>> +static void >>> +format_CT_COMMIT_DROP(const struct ovnact_nest *on, struct ds *s) >>> +{ >>> + if (on->nested_len) { >>> + format_nested_action(on, "ct_commit_drop", s); >>> + } else { >>> + ds_put_cstr(s, "ct_commit_drop;"); >>> + } >>> +} >>> + >>> +static void >>> +encode_CT_COMMIT_DROP(const struct ovnact_nest *on, >>> + const struct ovnact_encode_params *ep OVS_UNUSED, >>> + struct ofpbuf *ofpacts) >>> +{ >>> + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); >>> + ct->flags = NX_CT_F_COMMIT; >>> + ct->recirc_table = NX_CT_RECIRC_NONE; >>> + ct->zone_src.field = mf_from_id(MFF_LOG_ACL_DROP_ZONE); >>> + ct->zone_src.ofs = 0; >>> + ct->zone_src.n_bits = 16; >>> + >>> + /* If the datapath supports all-zero SNAT then use it to avoid tuple >>> + * collisions at commit time between NATed and firewalled-only sessions. >>> + */ >>> + if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) { >>> + size_t nat_offset = ofpacts->size; >>> + ofpbuf_pull(ofpacts, nat_offset); >>> + >>> + struct ofpact_nat *nat = ofpact_put_NAT(ofpacts); >>> + nat->flags = 0; >>> + nat->range_af = AF_UNSPEC; >>> + nat->flags |= NX_NAT_F_SRC; >>> + ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); >>> + ct = ofpacts->header; >>> + } > > I'm not completely sure about the all-zero SNAT for dropped packets. Do > we really need it? > Yeah, I don’t think we need it. I copied the contents from an existing function. Will remove it. > Thanks, > Dumitru Thanks, Abhiram Sangana
On 3/29/23 12:00, Abhiram Sangana wrote: >>>> @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key, >>>> pb->header_.uuid.parts[0], &match, ofpacts_p, >>>> &pb->header_.uuid); >>>> >>>> + if (zone_ids->drop) { >>>> + /* Table 39, Priority 1. >>>> + * ======================= >>>> + * >>>> + * Clear the logical registers (for consistent behavior with packets >>>> + * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */ >>>> + match_init_catchall(&match); >>>> + ofpbuf_clear(ofpacts_p); >>>> + match_set_metadata(&match, htonll(dp_key)); >>>> + for (int i = 0; i < MFF_N_LOG_REGS; i++) { >>>> + if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) { >>>> + put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p); >>>> + } >>>> + } >> Why do we need this? Don't we load the CT zone anyway for "to-lport" >> ACLs? Can't we also load the drop zone in the same place? > Yes, we are loading the drop zone (reg 9) in the same place as CT zone > for “to-lport” ACLs (reg 13) but this happens before table=39 where > registers 0 to 9 are cleared. Oh, I see now, it's because the drop zone is stored in reg9. I wanted to suggest not allowing reg9 to be used as logical register anymore but that's not really easy because it's used in the router pipeline: /* Register to store the result of check_pkt_larger action. */ #define REGBIT_PKT_LARGER "reg9[1]" #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]" #define REGBIT_LOOKUP_NEIGHBOR_IP_RESULT "reg9[3]" #define REGBIT_DST_NAT_IP_LOCAL "reg9[4]" #define REGBIT_KNOWN_ECMP_NH "reg9[5]" #define REGBIT_KNOWN_LB_SESSION "reg9[6]" [...] #define REG_ORIG_TP_DPORT_ROUTER "reg9[16..31]" Can we reuse one of the router-specific zones registers instead? #define MFF_LOG_DNAT_ZONE MFF_REG11 /* conntrack dnat zone for gateway router * (32 bits). */ #define MFF_LOG_SNAT_ZONE MFF_REG12 /* conntrack snat zone for gateway router * (32 bits). */ Regards, Dumitru
> On 29 Mar 2023, at 16:21, Dumitru Ceara <dceara@redhat.com> wrote: > > On 3/29/23 12:00, Abhiram Sangana wrote: >>>>> @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key, >>>>> pb->header_.uuid.parts[0], &match, ofpacts_p, >>>>> &pb->header_.uuid); >>>>> >>>>> + if (zone_ids->drop) { >>>>> + /* Table 39, Priority 1. >>>>> + * ======================= >>>>> + * >>>>> + * Clear the logical registers (for consistent behavior with packets >>>>> + * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */ >>>>> + match_init_catchall(&match); >>>>> + ofpbuf_clear(ofpacts_p); >>>>> + match_set_metadata(&match, htonll(dp_key)); >>>>> + for (int i = 0; i < MFF_N_LOG_REGS; i++) { >>>>> + if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) { >>>>> + put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p); >>>>> + } >>>>> + } >>> Why do we need this? Don't we load the CT zone anyway for "to-lport" >>> ACLs? Can't we also load the drop zone in the same place? >> Yes, we are loading the drop zone (reg 9) in the same place as CT zone >> for “to-lport” ACLs (reg 13) but this happens before table=39 where >> registers 0 to 9 are cleared. > > Oh, I see now, it's because the drop zone is stored in reg9. I wanted > to suggest not allowing reg9 to be used as logical register anymore but > that's not really easy because it's used in the router pipeline: > > /* Register to store the result of check_pkt_larger action. */ > #define REGBIT_PKT_LARGER "reg9[1]" > #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]" > #define REGBIT_LOOKUP_NEIGHBOR_IP_RESULT "reg9[3]" > #define REGBIT_DST_NAT_IP_LOCAL "reg9[4]" > #define REGBIT_KNOWN_ECMP_NH "reg9[5]" > #define REGBIT_KNOWN_LB_SESSION "reg9[6]" > > [...] > #define REG_ORIG_TP_DPORT_ROUTER "reg9[16..31]" > > Can we reuse one of the router-specific zones registers instead? > > #define MFF_LOG_DNAT_ZONE MFF_REG11 /* conntrack dnat zone for gateway > router > * (32 bits). */ > #define MFF_LOG_SNAT_ZONE MFF_REG12 /* conntrack snat zone for gateway > router > * (32 bits). */ Currently, we seem to be allocating SNAT and DNAT zones for logical switches too and loading registers 11 and 12. bb83b543-0d9d-409b-832c-4fc235355289 is a logical_switch. $ ovn-appctl ct-zone-list bb83b543-0d9d-409b-832c-4fc235355289_dnat 1 sw0p1 3 bb83b543-0d9d-409b-832c-4fc235355289_snat 2 bb83b543-0d9d-409b-832c-4fc235355289_drop 4 cookie=0x2547c5b0, duration=27.257s, table=0, n_packets=0, n_bytes=0, idle_age=27, priority=100,in_port=1 actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],load:0x1->OXM_OF_METADATA[],load:0x1->NXM_NX_REG14[],resubmit(,8) cookie=0x2547c5b0, duration=27.258s, table=38, n_packets=0, n_bytes=0, idle_age=27, priority=100,reg15=0x1,metadata=0x1 actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],resubmit(,39) Is it ok to reuse these registers? Thanks, Abhiram > Regards, > Dumitru >
On 4/3/23 11:47, Abhiram Sangana wrote: > > >> On 29 Mar 2023, at 16:21, Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 3/29/23 12:00, Abhiram Sangana wrote: >>>>>> @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key, >>>>>> pb->header_.uuid.parts[0], &match, ofpacts_p, >>>>>> &pb->header_.uuid); >>>>>> >>>>>> + if (zone_ids->drop) { >>>>>> + /* Table 39, Priority 1. >>>>>> + * ======================= >>>>>> + * >>>>>> + * Clear the logical registers (for consistent behavior with packets >>>>>> + * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */ >>>>>> + match_init_catchall(&match); >>>>>> + ofpbuf_clear(ofpacts_p); >>>>>> + match_set_metadata(&match, htonll(dp_key)); >>>>>> + for (int i = 0; i < MFF_N_LOG_REGS; i++) { >>>>>> + if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) { >>>>>> + put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p); >>>>>> + } >>>>>> + } >>>> Why do we need this? Don't we load the CT zone anyway for "to-lport" >>>> ACLs? Can't we also load the drop zone in the same place? >>> Yes, we are loading the drop zone (reg 9) in the same place as CT zone >>> for “to-lport” ACLs (reg 13) but this happens before table=39 where >>> registers 0 to 9 are cleared. >> >> Oh, I see now, it's because the drop zone is stored in reg9. I wanted >> to suggest not allowing reg9 to be used as logical register anymore but >> that's not really easy because it's used in the router pipeline: >> >> /* Register to store the result of check_pkt_larger action. */ >> #define REGBIT_PKT_LARGER "reg9[1]" >> #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]" >> #define REGBIT_LOOKUP_NEIGHBOR_IP_RESULT "reg9[3]" >> #define REGBIT_DST_NAT_IP_LOCAL "reg9[4]" >> #define REGBIT_KNOWN_ECMP_NH "reg9[5]" >> #define REGBIT_KNOWN_LB_SESSION "reg9[6]" >> >> [...] >> #define REG_ORIG_TP_DPORT_ROUTER "reg9[16..31]" >> >> Can we reuse one of the router-specific zones registers instead? >> >> #define MFF_LOG_DNAT_ZONE MFF_REG11 /* conntrack dnat zone for gateway >> router >> * (32 bits). */ >> #define MFF_LOG_SNAT_ZONE MFF_REG12 /* conntrack snat zone for gateway >> router >> * (32 bits). */ > > Currently, we seem to be allocating SNAT and DNAT zones for > logical switches too and loading registers 11 and 12. > You're right, we are. And we're also using the SNAT zone for LB hairpin. But AFAICT we don't use the DNAT zone in the switch pipeline (and I don't think we'll ever use it). > bb83b543-0d9d-409b-832c-4fc235355289 is a logical_switch. > > $ ovn-appctl ct-zone-list > bb83b543-0d9d-409b-832c-4fc235355289_dnat 1 > sw0p1 3 > bb83b543-0d9d-409b-832c-4fc235355289_snat 2 > bb83b543-0d9d-409b-832c-4fc235355289_drop 4 > > cookie=0x2547c5b0, duration=27.257s, table=0, n_packets=0, n_bytes=0, idle_age=27, priority=100,in_port=1 actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],load:0x1->OXM_OF_METADATA[],load:0x1->NXM_NX_REG14[],resubmit(,8) > > cookie=0x2547c5b0, duration=27.258s, table=38, n_packets=0, n_bytes=0, idle_age=27, priority=100,reg15=0x1,metadata=0x1 actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],resubmit(,39) > > Is it ok to reuse these registers? > I think it's OK to reuse the DNAT register; Numan do you agree? Thanks!
On Mon, Apr 3, 2023 at 5:55 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 4/3/23 11:47, Abhiram Sangana wrote: > > > > > >> On 29 Mar 2023, at 16:21, Dumitru Ceara <dceara@redhat.com> wrote: > >> > >> On 3/29/23 12:00, Abhiram Sangana wrote: > >>>>>> @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key, > >>>>>> pb->header_.uuid.parts[0], &match, ofpacts_p, > >>>>>> &pb->header_.uuid); > >>>>>> > >>>>>> + if (zone_ids->drop) { > >>>>>> + /* Table 39, Priority 1. > >>>>>> + * ======================= > >>>>>> + * > >>>>>> + * Clear the logical registers (for consistent behavior with packets > >>>>>> + * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */ > >>>>>> + match_init_catchall(&match); > >>>>>> + ofpbuf_clear(ofpacts_p); > >>>>>> + match_set_metadata(&match, htonll(dp_key)); > >>>>>> + for (int i = 0; i < MFF_N_LOG_REGS; i++) { > >>>>>> + if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) { > >>>>>> + put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p); > >>>>>> + } > >>>>>> + } > >>>> Why do we need this? Don't we load the CT zone anyway for "to-lport" > >>>> ACLs? Can't we also load the drop zone in the same place? > >>> Yes, we are loading the drop zone (reg 9) in the same place as CT zone > >>> for “to-lport” ACLs (reg 13) but this happens before table=39 where > >>> registers 0 to 9 are cleared. > >> > >> Oh, I see now, it's because the drop zone is stored in reg9. I wanted > >> to suggest not allowing reg9 to be used as logical register anymore but > >> that's not really easy because it's used in the router pipeline: > >> > >> /* Register to store the result of check_pkt_larger action. */ > >> #define REGBIT_PKT_LARGER "reg9[1]" > >> #define REGBIT_LOOKUP_NEIGHBOR_RESULT "reg9[2]" > >> #define REGBIT_LOOKUP_NEIGHBOR_IP_RESULT "reg9[3]" > >> #define REGBIT_DST_NAT_IP_LOCAL "reg9[4]" > >> #define REGBIT_KNOWN_ECMP_NH "reg9[5]" > >> #define REGBIT_KNOWN_LB_SESSION "reg9[6]" > >> > >> [...] > >> #define REG_ORIG_TP_DPORT_ROUTER "reg9[16..31]" > >> > >> Can we reuse one of the router-specific zones registers instead? > >> > >> #define MFF_LOG_DNAT_ZONE MFF_REG11 /* conntrack dnat zone for gateway > >> router > >> * (32 bits). */ > >> #define MFF_LOG_SNAT_ZONE MFF_REG12 /* conntrack snat zone for gateway > >> router > >> * (32 bits). */ > > > > Currently, we seem to be allocating SNAT and DNAT zones for > > logical switches too and loading registers 11 and 12. > > > > You're right, we are. And we're also using the SNAT zone for LB > hairpin. But AFAICT we don't use the DNAT zone in the switch pipeline > (and I don't think we'll ever use it). > > > bb83b543-0d9d-409b-832c-4fc235355289 is a logical_switch. > > > > $ ovn-appctl ct-zone-list > > bb83b543-0d9d-409b-832c-4fc235355289_dnat 1 > > sw0p1 3 > > bb83b543-0d9d-409b-832c-4fc235355289_snat 2 > > bb83b543-0d9d-409b-832c-4fc235355289_drop 4 > > > > cookie=0x2547c5b0, duration=27.257s, table=0, n_packets=0, n_bytes=0, idle_age=27, priority=100,in_port=1 actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],load:0x1->OXM_OF_METADATA[],load:0x1->NXM_NX_REG14[],resubmit(,8) > > > > cookie=0x2547c5b0, duration=27.258s, table=38, n_packets=0, n_bytes=0, idle_age=27, priority=100,reg15=0x1,metadata=0x1 actions=load:0x3->NXM_NX_REG13[],load:0x1->NXM_NX_REG11[],load:0x2->NXM_NX_REG12[],load:0x4->NXM_NX_REG9[],resubmit(,39) > > > > Is it ok to reuse these registers? > > > > I think it's OK to reuse the DNAT register; Numan do you agree? In the switch pipeline we do NAT on the port zones and for the load balancer in the snat zone. So I think it should be fine. Thanks Numan > > Thanks! > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 265740cab..e8f0b7242 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -734,8 +734,8 @@ update_ct_zones(const struct shash *binding_lports, HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { /* XXX Add method to limit zone assignment to logical router * datapaths with NAT */ - char *dnat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "dnat"); - char *snat = alloc_nat_zone_key(&ld->datapath->header_.uuid, "snat"); + char *dnat = alloc_ct_zone_key(&ld->datapath->header_.uuid, "dnat"); + char *snat = alloc_ct_zone_key(&ld->datapath->header_.uuid, "snat"); sset_add(&all_users, dnat); sset_add(&all_users, snat); @@ -745,6 +745,14 @@ update_ct_zones(const struct shash *binding_lports, } free(dnat); free(snat); + + /* Zone for committing connections dropped by ACLs with labels. */ + if (ld->is_switch) { + char *drop = alloc_ct_zone_key( + &ld->datapath->header_.uuid, "drop"); + sset_add(&all_users, drop); + free(drop); + } } /* Delete zones that do not exist in above sset. */ @@ -2420,7 +2428,7 @@ ct_zones_datapath_binding_handler(struct engine_node *node, void *data) /* Check if the requested snat zone has changed for the datapath * or not. If so, then fall back to full recompute of * ct_zone engine. */ - char *snat_dp_zone_key = alloc_nat_zone_key(&dp->header_.uuid, "snat"); + char *snat_dp_zone_key = alloc_ct_zone_key(&dp->header_.uuid, "snat"); struct simap_node *simap_node = simap_find(&ct_zones_data->current, snat_dp_zone_key); free(snat_dp_zone_key); diff --git a/controller/physical.c b/controller/physical.c index 4dcf44e01..3c573c492 100644 --- a/controller/physical.c +++ b/controller/physical.c @@ -60,6 +60,7 @@ struct zone_ids { int ct; /* MFF_LOG_CT_ZONE. */ int dnat; /* MFF_LOG_DNAT_ZONE. */ int snat; /* MFF_LOG_SNAT_ZONE. */ + int drop; /* MFF_LOG_ACL_DROP_ZONE. */ }; struct tunnel { @@ -204,14 +205,18 @@ get_zone_ids(const struct sbrec_port_binding *binding, const struct uuid *key = &binding->datapath->header_.uuid; - char *dnat = alloc_nat_zone_key(key, "dnat"); + char *dnat = alloc_ct_zone_key(key, "dnat"); zone_ids.dnat = simap_get(ct_zones, dnat); free(dnat); - char *snat = alloc_nat_zone_key(key, "snat"); + char *snat = alloc_ct_zone_key(key, "snat"); zone_ids.snat = simap_get(ct_zones, snat); free(snat); + char *drop = alloc_ct_zone_key(key, "drop"); + zone_ids.drop = simap_get(ct_zones, drop); + free(drop); + return zone_ids; } @@ -830,6 +835,9 @@ put_zones_ofpacts(const struct zone_ids *zone_ids, struct ofpbuf *ofpacts_p) if (zone_ids->snat) { put_load(zone_ids->snat, MFF_LOG_SNAT_ZONE, 0, 32, ofpacts_p); } + if (zone_ids->drop) { + put_load(zone_ids->drop, MFF_LOG_ACL_DROP_ZONE, 0, 32, ofpacts_p); + } } } @@ -896,6 +904,26 @@ put_local_common_flows(uint32_t dp_key, pb->header_.uuid.parts[0], &match, ofpacts_p, &pb->header_.uuid); + if (zone_ids->drop) { + /* Table 39, Priority 1. + * ======================= + * + * Clear the logical registers (for consistent behavior with packets + * that get tunneled) except MFF_LOG_ACL_DROP_ZONE. */ + match_init_catchall(&match); + ofpbuf_clear(ofpacts_p); + match_set_metadata(&match, htonll(dp_key)); + for (int i = 0; i < MFF_N_LOG_REGS; i++) { + if ((MFF_REG0 + i) != MFF_LOG_ACL_DROP_ZONE) { + put_load(0, MFF_REG0 + i, 0, 32, ofpacts_p); + } + } + put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, ofpacts_p); + ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 1, + pb->datapath->header_.uuid.parts[0], &match, + ofpacts_p, &pb->datapath->header_.uuid); + } + /* Table 39, Priority 100. * ======================= * diff --git a/include/ovn/actions.h b/include/ovn/actions.h index 6ca08b3c6..c7a6785d3 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -125,6 +125,7 @@ struct ovn_extend_table; OVNACT(COMMIT_LB_AFF, ovnact_commit_lb_aff) \ OVNACT(CHK_LB_AFF, ovnact_result) \ OVNACT(SAMPLE, ovnact_sample) \ + OVNACT(CT_COMMIT_DROP, ovnact_nest) \ /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */ enum OVS_PACKED_ENUM ovnact_type { diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h index a7b64ef67..c7ea9e0ce 100644 --- a/include/ovn/logical-fields.h +++ b/include/ovn/logical-fields.h @@ -47,6 +47,7 @@ enum ovn_controller_event { #define MFF_LOG_REG0 MFF_REG0 #define MFF_LOG_LB_ORIG_DIP_IPV4 MFF_REG1 #define MFF_LOG_LB_ORIG_TP_DPORT MFF_REG2 +#define MFF_LOG_ACL_DROP_ZONE MFF_REG9 #define MFF_LOG_XXREG0 MFF_XXREG0 #define MFF_LOG_LB_ORIG_DIP_IPV6 MFF_XXREG1 diff --git a/lib/actions.c b/lib/actions.c index 2da5a696b..d86b1efbc 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -5210,6 +5210,69 @@ encode_CHK_LB_AFF(const struct ovnact_result *res, MLF_USE_LB_AFF_SESSION_BIT, ofpacts); } +static void +parse_ct_commit_drop(struct action_context *ctx) +{ + if (ctx->lexer->token.type == LEX_T_LCURLY) { + parse_nested_action(ctx, OVNACT_CT_COMMIT_DROP, "ip", + WR_CT_COMMIT); + } else { + /* Add an empty nested action to allow for "ct_commit_drop;" syntax */ + add_prerequisite(ctx, "ip"); + struct ovnact_nest *on = ovnact_put(ctx->ovnacts, + OVNACT_CT_COMMIT_DROP, + OVNACT_ALIGN(sizeof *on)); + on->nested_len = 0; + on->nested = NULL; + } +} + +static void +format_CT_COMMIT_DROP(const struct ovnact_nest *on, struct ds *s) +{ + if (on->nested_len) { + format_nested_action(on, "ct_commit_drop", s); + } else { + ds_put_cstr(s, "ct_commit_drop;"); + } +} + +static void +encode_CT_COMMIT_DROP(const struct ovnact_nest *on, + const struct ovnact_encode_params *ep OVS_UNUSED, + struct ofpbuf *ofpacts) +{ + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); + ct->flags = NX_CT_F_COMMIT; + ct->recirc_table = NX_CT_RECIRC_NONE; + ct->zone_src.field = mf_from_id(MFF_LOG_ACL_DROP_ZONE); + ct->zone_src.ofs = 0; + ct->zone_src.n_bits = 16; + + /* If the datapath supports all-zero SNAT then use it to avoid tuple + * collisions at commit time between NATed and firewalled-only sessions. + */ + if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) { + size_t nat_offset = ofpacts->size; + ofpbuf_pull(ofpacts, nat_offset); + + struct ofpact_nat *nat = ofpact_put_NAT(ofpacts); + nat->flags = 0; + nat->range_af = AF_UNSPEC; + nat->flags |= NX_NAT_F_SRC; + ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); + ct = ofpacts->header; + } + + size_t set_field_offset = ofpacts->size; + ofpbuf_pull(ofpacts, set_field_offset); + + ovnacts_encode(on->nested, on->nested_len, ep, ofpacts); + ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); + ct = ofpacts->header; + ofpact_finish(ofpacts, &ct->ofpact); +} + /* Parses an assignment or exchange or put_dhcp_opts action. */ static void parse_set_action(struct action_context *ctx) @@ -5415,6 +5478,8 @@ parse_action(struct action_context *ctx) parse_commit_lb_aff(ctx, ovnact_put_COMMIT_LB_AFF(ctx->ovnacts)); } else if (lexer_match_id(ctx->lexer, "sample")) { parse_sample(ctx); + } else if (lexer_match_id(ctx->lexer, "ct_commit_drop")) { + parse_ct_commit_drop(ctx); } else { lexer_syntax_error(ctx->lexer, "expecting action"); } diff --git a/lib/ovn-util.c b/lib/ovn-util.c index f3665b89f..646aff6ef 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -443,12 +443,12 @@ split_addresses(const char *addresses, struct svec *ipv4_addrs, destroy_lport_addresses(&laddrs); } -/* Allocates a key for NAT conntrack zone allocation for a provided +/* Allocates a key for conntrack zone allocation for a provided * 'key' record and a 'type'. * * It is the caller's responsibility to free the allocated memory. */ char * -alloc_nat_zone_key(const struct uuid *key, const char *type) +alloc_ct_zone_key(const struct uuid *key, const char *type) { return xasprintf(UUID_FMT"_%s", UUID_ARGS(key), type); } diff --git a/lib/ovn-util.h b/lib/ovn-util.h index cd19919cb..170685872 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -92,7 +92,7 @@ const char *find_lport_address(const struct lport_addresses *laddrs, void split_addresses(const char *addresses, struct svec *ipv4_addrs, struct svec *ipv6_addrs); -char *alloc_nat_zone_key(const struct uuid *key, const char *type); +char *alloc_ct_zone_key(const struct uuid *key, const char *type); const char *default_nb_db(void); const char *default_sb_db(void); diff --git a/northd/northd.c b/northd/northd.c index 77e105b86..bc39e9bc6 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -84,6 +84,10 @@ static bool use_ct_inv_match = true; */ static bool default_acl_drop; +/* If this option is 'true', drop/reject ACLs with label are translated to + * commit flows to conntrack besides dropping/rejecting them. */ +static bool ct_commit_acl_drop; + #define MAX_OVN_TAGS 4096 /* Pipeline stages. */ @@ -311,7 +315,7 @@ enum ovn_stage { * +----+----------------------------------------------+---+-----------------------------------+ * | R8 | LB_AFF_MATCH_PORT | * +----+----------------------------------------------+ - * | R9 | UNUSED | + * | R9 | ACL DROP CT ZONE | * +----+----------------------------------------------+ * * Logical Router pipeline: @@ -6493,6 +6497,14 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_clear(match); ds_clear(actions); ds_put_cstr(match, REGBIT_ACL_HINT_DROP " == 1"); + /* If the ACL has a label, we commit the packet in + * a separate zone for debugging purposes before + * rejecting/dropping it. */ + if (ct_commit_acl_drop && acl->label) { + ds_put_format(actions, "ct_commit_drop { " + "ct_label.label = %"PRId64"; }; ", + acl->label); + } if (!strcmp(acl->action, "reject")) { build_reject_acl_rules(od, lflows, stage, acl, match, actions, &acl->header_, meter_groups); @@ -6519,8 +6531,15 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, ds_clear(match); ds_clear(actions); ds_put_cstr(match, REGBIT_ACL_HINT_BLOCK " == 1"); - ds_put_format(actions, "ct_commit { %s = 1; }; ", + ds_put_format(actions, "ct_commit { %s = 1; ", ct_blocked_match); + /* Update ct_label.label to reflect the new policy matching + * the connection. */ + if (acl->label) { + ds_put_format(actions, "ct_label.label = %"PRId64"; ", + acl->label); + } + ds_put_cstr(actions, "}; "); if (!strcmp(acl->action, "reject")) { build_reject_acl_rules(od, lflows, stage, acl, match, actions, &acl->header_, meter_groups); @@ -16236,6 +16255,8 @@ ovnnb_db_run(struct northd_input *input_data, check_lsp_is_up = !smap_get_bool(&nb->options, "ignore_lsp_down", true); default_acl_drop = smap_get_bool(&nb->options, "default_acl_drop", false); + ct_commit_acl_drop = smap_get_bool(&nb->options, + "ct_commit_acl_drop", false); install_ls_lb_from_router = smap_get_bool(&nb->options, "install_ls_lb_from_router", diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 3d7a92ea8..74f34cfe8 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -727,13 +727,26 @@ action for TCP connections,<code>icmp4/icmp6</code> action for UDP connections, and <code>sctp_abort {output <-%gt; inport; next(pipeline=egress,table=5);}</code> action for SCTP associations. + Additionally, if the ACL has a <code>label</code> and + <ref column="options:ct_commit_acl_drop" table="NB_Global" + db="OVN_Northbound"/> column of <ref table="NB_Global" + db="OVN_Northbound"/> table is <code>true</code>, the translated + action includes <code>ct_commit_drop(ct_label.label=label)</code>. </li> <li> Other ACLs translate to <code>drop;</code> for new or untracked connections and <code>ct_commit(ct_label=1/1);</code> for known connections. Setting <code>ct_label</code> marks a connection as one that was previously allowed, but should no longer be - allowed due to a policy change. + allowed due to a policy change. If the ACLs have <code>label</code>, + then they instead translate to + <code>ct_commit_drop(ct_label.label=label)</code> for + new and untracked connections if + <ref column="options:ct_commit_acl_drop" table="NB_Global" + db="OVN_Northbound"/> column of <ref table="NB_Global" + db="OVN_Northbound"/> table is also set to <code>true</code> and + <code>ct_commit(ct_label.blocked=1; ct_label.label=label);</code> + for known connections. </li> </ul> @@ -1165,13 +1178,26 @@ action for TCP connections,<code>icmp4/icmp6</code> action for UDP connections, and <code>sctp_abort {output <-%gt; inport; next(pipeline=egress,table=5);}</code> action for SCTP associations. + Additionally, if the ACL has a <code>label</code> and + <ref column="options:ct_commit_acl_drop" table="NB_Global" + db="OVN_Northbound"/> column of <ref table="NB_Global" + db="OVN_Northbound"/> table is <code>true</code>, the translated + action includes <code>ct_commit_drop(ct_label.label=label)</code>. </li> <li> Other apply-after-lb ACLs translate to <code>drop;</code> for new or untracked connections and <code>ct_commit(ct_label=1/1);</code> for known connections. Setting <code>ct_label</code> marks a connection as one that was previously allowed, but should no longer be - allowed due to a policy change. + allowed due to a policy change. If the ACLs have <code>label</code>, + then they instead translate to + <code>ct_commit_drop(ct_label.label=label)</code> for + new and untracked connections if + <ref column="options:ct_commit_acl_drop" table="NB_Global" + db="OVN_Northbound"/> column of <ref table="NB_Global" + db="OVN_Northbound"/> table is also set to <code>true</code> and + <code>ct_commit(ct_label.blocked=1; ct_label.label=label);</code> + for known connections. </li> </ul> diff --git a/ovn-nb.xml b/ovn-nb.xml index 4b52b9953..d28c2fea8 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -301,6 +301,15 @@ </p> </column> + <column name="options" key="ct_commit_acl_drop"> + <p> + If set to <code>true</code>, connections dropped/rejected by ACLs + with label are committed to DROP conntrack zone of + <code>logical_switch</code>. By default this option is set to + <code>false</code>. + </p> + </column> + <group title="Options for configuring interconnection route advertisement"> <p> These options control how routes are advertised between OVN @@ -2106,12 +2115,10 @@ or <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. + tracking entry. The value should be a valid 32-bit unsigned integer. + This value can help in debugging from connection tracking 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. + which is causing a "leaked" connection. </p> </column> <column name="priority"> diff --git a/ovn-sb.xml b/ovn-sb.xml index a77f8f4ef..f01f9a927 100644 --- a/ovn-sb.xml +++ b/ovn-sb.xml @@ -1408,6 +1408,28 @@ </p> </dd> + <dt><code>ct_commit_drop { };</code></dt> + <dt><code>ct_commit_drop { ct_mark=<var>value[/mask]</var>; };</code></dt> + <dt><code>ct_commit_drop { ct_label=<var>value[/mask]</var>; };</code></dt> + <dt><code>ct_commit_drop { ct_mark=<var>value[/mask]</var>; ct_label=<var>value[/mask]</var>; };</code></dt> + <dd> + <p> + Commit the flow to a connection tracking entry in the + logical switch DROP zone. This action is identical to + <code>ct_commit</code> besides the connection tracking entry + the flow is committed to. + </p> + + <p> + This action is useful to commit connection tracking state for + packets before they are dropped for debugging purposes. + Committing the packets in a separate zone ensures that connection + tracking state of dropped connections doesn't interact with the + state of allowed connections and allows the DROP zone to be + managed separately. + </p> + </dd> + <dt><code>ct_dnat;</code></dt> <dt><code>ct_dnat(<var>IP</var>);</code></dt> <dd> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at index 2fffe1850..cfbb5e45a 100644 --- a/tests/ovn-nbctl.at +++ b/tests/ovn-nbctl.at @@ -222,7 +222,8 @@ ovn_nbctl_test_acl() { 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]) + AT_CHECK([ovn-nbctl $2 --label=1235 acl-add $1 to-lport 70 icmp reject]) + AT_CHECK([ovn-nbctl $2 --label=1234 acl-add $1 to-lport 50 ip drop]) AT_CHECK([ovn-nbctl $2 --apply-after-lb acl-add $1 from-lport 500 tcp allow]) AT_CHECK([ovn-nbctl $2 --apply-after-lb acl-add $1 from-lport 300 tcp drop]) AT_CHECK([ovn-nbctl $2 --apply-after-lb acl-add $1 from-lport 300 udp allow]) @@ -232,10 +233,6 @@ ovn_nbctl_test_acl() { 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]) @@ -256,7 +253,8 @@ from-lport 300 (udp) allow [[after-lb]] 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 + to-lport 70 (icmp) reject label=1235 + to-lport 50 (ip) drop label=1234 ]) dnl Delete in one direction. diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 3fa02d2b3..9fbb82e37 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -4241,88 +4241,141 @@ ovn_start check ovn-nbctl ls-add sw0 check ovn-nbctl lsp-add sw0 sw0p1 +AS_BOX([Create ACLs with labels]) check ovn-nbctl --wait=sb --label=1234 acl-add sw0 to-lport 1002 tcp allow-related +check ovn-nbctl --wait=sb --label=2345 acl-add sw0 to-lport 1001 ip drop check ovn-nbctl --wait=sb --label=1234 acl-add sw0 from-lport 1002 tcp allow-related +check ovn-nbctl --wait=sb --label=2345 acl-add sw0 from-lport 1001 ip reject ovn-sbctl dump-flows sw0 > sw0flows AT_CAPTURE_FILE([sw0flows]) -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table=?/'], [0], [dnl - table=? (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) - table=? (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_in_acl ), priority=2001 , match=((reg0[[10]] == 1) && ip), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) + table=??(ls_in_acl ), priority=2001 , match=((reg0[[9]] == 1) && ip), action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) + table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=??(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 | sed 's/table=../table=??/'], [0], [dnl +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;) table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.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 -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_out_acl ), priority=2001 , match=(reg0[[10]] == 1 && (ip)), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; /* drop */) + table=??(ls_out_acl ), priority=2001 , match=(reg0[[9]] == 1 && (ip)), action=(/* drop */) + table=??(ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=??(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_mark.blocked = 0; }; next;) - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_out_stateful ), priority=0 , match=(1), action=(next;) + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) +]) + +AS_BOX([Set ct_commit_acl_drop option]) +check ovn-nbctl --wait=sb set NB_Global . options:ct_commit_acl_drop=true + +ovn-sbctl dump-flows sw0 > sw0flows +AT_CAPTURE_FILE([sw0flows]) + +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_in_acl ), priority=2001 , match=((reg0[[10]] == 1) && ip), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) + table=??(ls_in_acl ), priority=2001 , match=((reg0[[9]] == 1) && ip), action=(ct_commit_drop { ct_label.label = 2345; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) + table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=??(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 | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) ]) -# Add new ACL without label +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_out_acl ), priority=2001 , match=(reg0[[10]] == 1 && (ip)), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; /* drop */) + table=??(ls_out_acl ), priority=2001 , match=(reg0[[9]] == 1 && (ip)), action=(ct_commit_drop { ct_label.label = 2345; }; /* drop */) + table=??(ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=??(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 | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_out_stateful ), priority=0 , match=(1), action=(next;) + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) +]) + +AS_BOX([Add ACLs without labels]) check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 udp allow-related +check ovn-nbctl --wait=sb acl-add sw0 to-lport 1001 ip6 drop check ovn-nbctl --wait=sb acl-add sw0 from-lport 1002 udp allow-related +check ovn-nbctl --wait=sb acl-add sw0 from-lport 1001 ip6 reject ovn-sbctl dump-flows sw0 > sw0flows AT_CAPTURE_FILE([sw0flows]) -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table=?/'], [0], [dnl - table=? (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) - table=? (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) - table=? (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) - table=? (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) -]) -AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_in_acl ), priority=2001 , match=((reg0[[10]] == 1) && ip), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) + table=??(ls_in_acl ), priority=2001 , match=((reg0[[10]] == 1) && ip6), action=(ct_commit { ct_mark.blocked = 1; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) + table=??(ls_in_acl ), priority=2001 , match=((reg0[[9]] == 1) && ip), action=(ct_commit_drop { ct_label.label = 2345; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) + table=??(ls_in_acl ), priority=2001 , match=((reg0[[9]] == 1) && ip6), action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) + table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) + table=??(ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=??(ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) +]) +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;) table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.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 -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_out_acl ), priority=2001 , match=(reg0[[10]] == 1 && (ip)), action=(ct_commit { ct_mark.blocked = 1; ct_label.label = 2345; }; /* drop */) + table=??(ls_out_acl ), priority=2001 , match=(reg0[[10]] == 1 && (ip6)), action=(ct_commit { ct_mark.blocked = 1; }; /* drop */) + table=??(ls_out_acl ), priority=2001 , match=(reg0[[9]] == 1 && (ip)), action=(ct_commit_drop { ct_label.label = 2345; }; /* drop */) + table=??(ls_out_acl ), priority=2001 , match=(reg0[[9]] == 1 && (ip6)), action=(/* drop */) + table=??(ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=??(ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) + table=??(ls_out_acl ), priority=2002 , match=(reg0[[8]] == 1 && (tcp)), action=(reg0[[1]] = 1; reg0[[13]] = 1; reg3 = 1234; next;) + table=??(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_mark.blocked = 0; }; next;) - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_out_stateful ), priority=0 , match=(1), action=(next;) + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) ]) -# Delete new ACL with label +AS_BOX([Delete ACLs with labels]) check ovn-nbctl --wait=sb acl-del sw0 to-lport 1002 tcp +check ovn-nbctl --wait=sb acl-del sw0 to-lport 1001 ip check ovn-nbctl --wait=sb acl-del sw0 from-lport 1002 tcp +check ovn-nbctl --wait=sb acl-del sw0 from-lport 1001 ip ovn-sbctl dump-flows sw0 > sw0flows AT_CAPTURE_FILE([sw0flows]) -AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 | sort | sed 's/table=./table=?/'], [0], [dnl - table=? (ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) - table=? (ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_in_acl ), priority=2001 , match=((reg0[[10]] == 1) && ip6), action=(ct_commit { ct_mark.blocked = 1; }; reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) + table=??(ls_in_acl ), priority=2001 , match=((reg0[[9]] == 1) && ip6), action=(reg0 = 0; reject { /* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ outport <-> inport; next(pipeline=egress,table=5); };) + table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) + table=??(ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 && (udp)), action=(next;) ]) -AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=../table=??/'], [0], [dnl +AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;) table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.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 -w "ls_out_acl" sw0flows | grep -e 2002 -e 2001 | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_out_acl ), priority=2001 , match=(reg0[[10]] == 1 && (ip6)), action=(ct_commit { ct_mark.blocked = 1; }; /* drop */) + table=??(ls_out_acl ), priority=2001 , match=(reg0[[9]] == 1 && (ip6)), action=(/* drop */) + table=??(ls_out_acl ), priority=2002 , match=(reg0[[7]] == 1 && (udp)), action=(reg0[[1]] = 1; next;) + table=??(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_mark.blocked = 0; }; next;) - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) +AT_CHECK([grep "ls_out_stateful" sw0flows | sort | sed 's/table=[[0-9]]*\s*/table=??/'], [0], [dnl + table=??(ls_out_stateful ), priority=0 , match=(1), action=(next;) + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;) + table=??(ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 && reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; next;) ]) AT_CLEANUP ]) diff --git a/tests/ovn.at b/tests/ovn.at index e9b8bc677..fc7abfdad 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -1323,6 +1323,58 @@ ct_label = 0xcafe ct_label.blocked = 1/1 Field ct_label.blocked is not modifiable. +# ct_commit_drop +ct_commit_drop; + encodes as ct(commit,zone=NXM_NX_REG9[0..15]) + has prereqs ip +ct_commit_drop { }; + formats as ct_commit_drop; + encodes as ct(commit,zone=NXM_NX_REG9[0..15]) + has prereqs ip +ct_commit_drop { ct_mark=1; }; + formats as ct_commit_drop { ct_mark = 1; }; + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1->ct_mark)) + has prereqs ip +ct_commit_drop { ct_mark=1/1; }; + formats as ct_commit_drop { ct_mark = 1/1; }; + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1/0x1->ct_mark)) + has prereqs ip +ct_commit_drop { ct_label=1; }; + formats as ct_commit_drop { ct_label = 1; }; + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1->ct_label)) + has prereqs ip +ct_commit_drop { ct_label=1/1; }; + formats as ct_commit_drop { ct_label = 1/1; }; + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1/0x1->ct_label)) + has prereqs ip +ct_commit_drop { ct_mark=1; ct_label=2; }; + formats as ct_commit_drop { ct_mark = 1; ct_label = 2; }; + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label)) + has prereqs ip + +ct_commit_drop { ct_label=0x01020304050607080910111213141516; }; + formats as ct_commit_drop { ct_label = 0x1020304050607080910111213141516; }; + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1020304050607080910111213141516->ct_label)) + has prereqs ip +ct_commit_drop { ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000; }; + formats as ct_commit_drop { ct_label = 0x1000000000000000000000000000000/0x1000000000000000000000000000000; }; + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0x1000000000000000000000000000000/0x1000000000000000000000000000000->ct_label)) + has prereqs ip +ct_commit_drop { ct_label=18446744073709551615; }; + formats as ct_commit_drop { ct_label = 18446744073709551615; }; + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0xffffffffffffffff->ct_label)) + has prereqs ip +ct_commit_drop { ct_label[0..47] = 0x00000f040201; ct_label[48..63] = 0x0002; }; + formats as ct_commit_drop { ct_label[0..47] = 0xf040201; ct_label[48..63] = 0x2; }; + encodes as ct(commit,zone=NXM_NX_REG9[0..15],exec(set_field:0xf040201/0xffffffffffff->ct_label,set_field:0x2000000000000/0xffff000000000000->ct_label)) + has prereqs ip +ct_commit_drop { ct_label=18446744073709551616; }; + Decimal constants must be less than 2**64. +ct_commit_drop { ct_label=0x181716151413121110090807060504030201; }; + 141-bit constant is not compatible with 128-bit field ct_label. +ct_commit_drop { ip4.dst = 192.168.0.1; }; + Field ip4.dst is not modifiable. + # ct_dnat ct_dnat; encodes as ct(table=19,zone=NXM_NX_REG11[0..15],nat) @@ -33637,7 +33689,7 @@ ovn-nbctl lsp-add sw0 sw0-port1 -- lsp-set-addresses sw0-port1 "50:54:00:00:00:0 ovs-vsctl set interface p0 external-ids:iface-id=sw0-port0 ovs-vsctl set interface p1 external-ids:iface-id=sw0-port1 check ovn-nbctl --wait=hv sync -as hv1 ovs-ofctl dump-flows br-int | sed 's/cookie=0x.*, duration=.*, table/cookie=??, duration=??, table/' | sed 's/load:0x.->NXM_NX_REG1/load:0x?->NXM_NX_REG1/g' | sed 's/idle_age=[[0-9]], //g' | sort > offlows1 +as hv1 ovs-ofctl dump-flows br-int | sed 's/cookie=0x.*, duration=.*, table/cookie=??, duration=??, table/' | sed 's/load:0x.->NXM_NX_REG\([[19]]\)/load:0x?->NXM_NX_REG\1/g' | sed 's/idle_age=[[0-9]], //g' | sort > offlows1 ovs-vsctl set interface p0 external-ids:iface-id=foo0 ovs-vsctl set interface p1 external-ids:iface-id=foo1 @@ -33664,7 +33716,7 @@ sleep 0.5 # Restart SB AT_CHECK([kill -CONT $(cat ovn-sb/ovsdb-server.pid)]) check ovn-nbctl --wait=hv sync -as hv1 ovs-ofctl dump-flows br-int | sed 's/cookie=0x.*, duration=.*, table/cookie=??, duration=??, table/' | sed 's/load:0x.->NXM_NX_REG1/load:0x?->NXM_NX_REG1/g' | sed 's/idle_age=[[0-9]], //g' | sort > offlows2 +as hv1 ovs-ofctl dump-flows br-int | sed 's/cookie=0x.*, duration=.*, table/cookie=??, duration=??, table/' | sed 's/load:0x.->NXM_NX_REG\([[19]]\)/load:0x?->NXM_NX_REG\1/g' | sed 's/idle_age=[[0-9]], //g' | sort > offlows2 AT_CHECK([diff offlows1 offlows2]) ovn-nbctl ls-del sw0 @@ -34689,3 +34741,37 @@ check_packets OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-controller - check logical_switch drop ct zone]) +ovn_start +net_add n1 +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 + +check ovn-nbctl ls-add ls0 +check ovn-nbctl lsp-add ls0 lsp0 +check ovn-nbctl lsp-set-addresses lsp0 "00:00:00:00:00:02 10.0.0.2" + +ovs-vsctl -- add-port br-int vif0 -- set interface vif0 external-ids:iface-id=lsp0 + +# Wait for ovn-northd and ovn-controller to catch up. +wait_for_ports_up +check ovn-nbctl --wait=hv sync + +# logical_switch should have a CT zone for connections dropped by labeled ACLs +ls_uuid=$(fetch_column Datapath_Binding _uuid external_ids:name=ls0) +AT_CHECK([as hv1 ovn-appctl -t ovn-controller ct-zone-list | \ +grep ${ls_uuid}_drop -c], [0], [1 +]) + +# Check that register storing the drop ct zone is not cleared. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=39 | grep -w 'priority=1' | ofctl_strip_all], [0], [dnl + table=39, priority=1,metadata=0x1 actions=load:0->NXM_NX_REG0[[]],load:0->NXM_NX_REG1[[]],load:0->NXM_NX_REG2[[]],load:0->NXM_NX_REG3[[]],load:0->NXM_NX_REG4[[]],load:0->NXM_NX_REG5[[]],load:0->NXM_NX_REG6[[]],load:0->NXM_NX_REG7[[]],load:0->NXM_NX_REG8[[]],resubmit(,40) +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +]) diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c index ae4d6c403..74fa04539 100644 --- a/utilities/ovn-nbctl.c +++ b/utilities/ovn-nbctl.c @@ -2367,13 +2367,6 @@ nbctl_acl_add(struct ctl_context *ctx) /* 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) { diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c index e5766ed67..5cede3527 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -3356,6 +3356,8 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len, break; case OVNACT_SAMPLE: break; + case OVNACT_CT_COMMIT_DROP: + break; } } ofpbuf_uninit(&stack);
This patch adds support to commit connections dropped/rejected by ACLs to the connection tracking table. Dropped connections are committed to conntrack only if NB_Global options:ct_commit_acl_drop is set to true (false by default) and ACL dropping/rejecting the connection has label configured. The dropped connections are committed in a separate conntrack zone so that they can be managed independently and do not interact with the connection tracking state of allowed connections. This provides a new approach to identify connections dropped by ACLs besides the existing ACL logging and drop sampling approaches. Each logical switch is assigned a new conntrack zone for committing dropped flows. The zone is loaded into register MFF_LOG_ACL_DROP_ZONE. A new lflow action "ct_commit_drop" is introduced that commits flows to connection tracking table in a zone identified by MFF_LOG_ACL_DROP_ZONE register. An ACL with "drop" or "reject" action and non-empty label translates to include "ct_commit_drop" in its actions instead of simply dropping/rejecting the packet. Signed-off-by: Abhiram Sangana <sangana.abhiram@nutanix.com> --- controller/ovn-controller.c | 14 +++- controller/physical.c | 32 ++++++++- include/ovn/actions.h | 1 + include/ovn/logical-fields.h | 1 + lib/actions.c | 65 +++++++++++++++++ lib/ovn-util.c | 4 +- lib/ovn-util.h | 2 +- northd/northd.c | 25 ++++++- northd/ovn-northd.8.xml | 30 +++++++- ovn-nb.xml | 17 +++-- ovn-sb.xml | 22 ++++++ tests/ovn-nbctl.at | 10 ++- tests/ovn-northd.at | 133 ++++++++++++++++++++++++----------- tests/ovn.at | 90 +++++++++++++++++++++++- utilities/ovn-nbctl.c | 7 -- utilities/ovn-trace.c | 2 + 16 files changed, 383 insertions(+), 72 deletions(-)