Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack
On Mon, Apr 3, 2023 at 5:55 AM Dumitru Ceara wrote:
>
> On 4/3/23 11:47, Abhiram Sangana wrote:
> >
> >
> >> On 29 Mar 2023, at 16:21, Dumitru Ceara 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], , ofpacts_p,
> >>>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();
> >> +ofpbuf_clear(ofpacts_p);
> >> +match_set_metadata(, 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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack
On 4/3/23 11:47, Abhiram Sangana wrote:
>
>
>> On 29 Mar 2023, at 16:21, Dumitru Ceara 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], , ofpacts_p,
>>>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();
>> +ofpbuf_clear(ofpacts_p);
>> +match_set_metadata(, 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!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack
> On 29 Mar 2023, at 16:21, Dumitru Ceara 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], , ofpacts_p,
>>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();
> +ofpbuf_clear(ofpacts_p);
> +match_set_metadata(, 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
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack
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], , ofpacts_p,
>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();
+ofpbuf_clear(ofpacts_p);
+match_set_metadata(, 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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack
Hi Dumitru,
Thanks for reviewing the patch.
> On 23 Mar 2023, at 20:59, Dumitru Ceara wrote:
>
> On 3/17/23 20:34, Numan Siddique wrote:
>> On Mon, Feb 13, 2023 at 11:36 AM Abhiram Sangana
>> 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
>>
>> 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(>datapath->header_.uuid,
>>> "dnat");
>>> -char *snat = alloc_nat_zone_key(>datapath->header_.uuid,
>>> "snat");
>>> +char *dnat = alloc_ct_zone_key(>datapath->header_.uuid,
>>> "dnat");
>>> +char *snat = alloc_ct_zone_key(>datapath->header_.uuid,
>>> "snat");
>>> sset_add(_users, dnat);
>>> sset_add(_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(
>>> +>datapath->header_.uuid, "drop");
>>> +sset_add(_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(>header_.uuid,
>>> "snat");
>>> +char *snat_dp_zone_key =
Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack
On 3/17/23 20:34, Numan Siddique wrote:
> On Mon, Feb 13, 2023 at 11:36 AM Abhiram Sangana
> 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
>
> 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(>datapath->header_.uuid,
>> "dnat");
>> -char *snat = alloc_nat_zone_key(>datapath->header_.uuid,
>> "snat");
>> +char *dnat = alloc_ct_zone_key(>datapath->header_.uuid, "dnat");
>> +char *snat = alloc_ct_zone_key(>datapath->header_.uuid, "snat");
>> sset_add(_users, dnat);
>> sset_add(_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(
>> +>datapath->header_.uuid, "drop");
>> +sset_add(_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(>header_.uuid,
>> "snat");
>> +char *snat_dp_zone_key = alloc_ct_zone_key(>header_.uuid,
>> "snat");
>> struct simap_node *simap_node = simap_find(_zones_data->current,
>> snat_dp_zone_key);
>> free(snat_dp_zone_key);
>> diff --git a/controller/physical.c
Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack
> On 18 Mar 2023, at 01:04, Numan Siddique wrote: > > On Mon, Feb 13, 2023 at 11:36 AM Abhiram Sangana > 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 > 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack
On Mon, Feb 13, 2023 at 11:36 AM Abhiram Sangana
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
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(>datapath->header_.uuid, "dnat");
> -char *snat = alloc_nat_zone_key(>datapath->header_.uuid, "snat");
> +char *dnat = alloc_ct_zone_key(>datapath->header_.uuid, "dnat");
> +char *snat = alloc_ct_zone_key(>datapath->header_.uuid, "snat");
> sset_add(_users, dnat);
> sset_add(_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(
> +>datapath->header_.uuid, "drop");
> +sset_add(_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(>header_.uuid,
> "snat");
> +char *snat_dp_zone_key = alloc_ct_zone_key(>header_.uuid,
> "snat");
> struct simap_node *simap_node = simap_find(_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; /*
Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack
On 3/3/23 13:20, Abhiram Sangana wrote: > > >> On 13 Feb 2023, at 16:35, Abhiram Sangana >> 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 >> --- >> 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack
> On 13 Feb 2023, at 16:35, Abhiram Sangana 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 > --- > 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2] northd, controller: Commit flows dropped by ACLs to conntrack
References: <20230213163539.4507-1-sangana.abhi...@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:
ct_commit_drop { ct_mark=value[/mask];
};
WARNING: Line is 83 characters long (recommended limit is 79)
#465 FILE: ovn-sb.xml:1413:
ct_commit_drop { ct_label=value[/mask];
};
WARNING: Line is 116 characters long (recommended limit is 79)
#466 FILE: ovn-sb.xml:1414:
ct_commit_drop { ct_mark=value[/mask];
ct_label=value[/mask]; };
Lines checked: 862, Warnings: 3, Errors: 0
Please check this out. If you feel there has been an error, please email
acon...@redhat.com
Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
