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.
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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
