Hi Dumitru, Thanks for reviewing the patch.
> On 23 Mar 2023, at 20:59, Dumitru Ceara <[email protected]> wrote: > > On 3/17/23 20:34, Numan Siddique wrote: >> On Mon, Feb 13, 2023 at 11:36 AM Abhiram Sangana >> <[email protected]> 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 <[email protected]> >> >> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
