Re: [ovs-dev] [PATCH ovn 8/8] northd: Add ACL Sampling.

2024-07-10 Thread Dumitru Ceara
On 7/9/24 13:27, Adrián Moreno wrote:
> On Mon, Jul 08, 2024 at 01:24:14PM GMT, Dumitru Ceara wrote:
>> From: Adrian Moreno 
>>
>> Introduce a new table called Sample where per-flow IPFIX configuration
>> can be specified.
>> Also, reference rows from such table from the ACL table to enable the
>> configuration of ACL sampling. If enabled, northd will add a sample
>> action to each ACL related logical flow.
>>
>> Packets that hit stateful ACLs are sampled in different ways depending
>> whether they are initiating a new session or are just forwarded on an
>> existing (already allowed) session.  Two new columns ("sample_new" and
>> "sample_est") are added to the ACL table to allow for potentially
>> different sampling rates for the two cases.
>>
>> Note: If an ACL has both sampling enabled and a label associated to it
>> then the label value overrides the observation point ID defined in the
>> sample configuration.  This is a side effect of the implementation
>> (observation point IDs are stored in conntrack in the same part of the
>> ct_label where ACL labels are also stored).  The two features
>> (sampling and ACL labels) serve however similar purposes so it's not
>> expected that they're both enabled together.
>>
>> When sampling is enabled on an ACL additional logical flows are created
>> for that ACL (one for stateless ACLs and 3 for stateful ACLs) in the ACL
>> action stage of the logical pipeline.  These additional flows match on a
>> combination of conntrack state values and observation point id values
>> (either against a logical register or against the stored ct_label state)
>> in order to determine whether the packets hitting the ACLs must be
>> sampled or not.  This comes with a slight increase in the number of
>> logical flows and in the number of OpenFlow rules.  The number of
>> additional flows _does not_ depend on the original ACL match or action.
>>
>> New --sample-new and --sample-est optional arguments are added to the
>> 'ovn-nbctl acl-add' command to allow configuring these new types of
>> sampling for ACLs.
>>
>> An example workflow of configuring ACL samples is:
>>   # Create Sampling_App mappings for ACL traffic types:
>>   ovn-nbctl create Sampling_App name="acl-new-traffic-sampling" \
>> id="42"
>>   ovn-nbctl create sampling_app name="acl-est-traffic-sampling" \
>>  id="43"
>>   # Create two sample collectors, one that samples all packets (c1)
>>   # and another one that samples with a probability of 10% (c2):
>>   c1=$(ovn-nbctl create Sample_Collector name=c1 \
>>probability=65535 set_id=1)
>>   c2=$(ovn-nbctl create Sample_Collector name=c2 \
>>probability=6553 set_id=2)
>>   # Create two sample configurations (for new and for established
>>   # traffic):
>>   s1=$(ovn-nbctl create sample collector="$c1 $c2" metadata=4301)
>>   s2=$(ovn-nbctl create sample collector="$c1 $c2" metadata=4302)
>>   # Create an ingress ACL to allow IP traffic:
>>   ovn-nbctl --sample-new=$s1 --sample-est=$s2 acl-add ls \
>> from-lport 1 "ip" allow-related
>>
>> The config above will generate IPFIX samples with:
>> - observation domain id set to 42 (Sampling_App
>>   "acl-new-traffic-sampling" config) and observation point id
>>   set to 4301 (Sample s1) for packets that create a new
>>   connection
>> - observation domain id set to 43 (Sampling_app
>>   "acl-est-traffic-sampling" config) and observation point id
>>   set to 4302 (Sample s2) for packets that are part of an already
>>   existing connection
>>
> 
> We assume the dp_key is 0 in this case, right? Maybe worth mentioning
> it.
> 

Well, it will never be 0, I'll update this to mention that the
observation domain ID includes the datapath binding key and this
configured value.

>> Reported-at: https://issues.redhat.com/browse/FDP-305
>> Signed-off-by: Adrian Moreno 
>> Co-authored-by: Dumitru Ceara 
>> Signed-off-by: Dumitru Ceara 
>> ---
>>  NEWS   |   3 +
>>  include/ovn/logical-fields.h   |   2 +
>>  lib/logical-fields.c   |   6 +
>>  northd/northd.c| 519 +++--
>>  northd/ovn-northd.8.xml|  26 ++
>>  ovn-nb.ovsschema   |  44 ++-
>>  ovn-nb.xml |  56 +++
>>  tests/atlocal.in   |   6 +
>>  tests/ovn-macros.at|   4 +
>>  tests/ovn-nbctl.at |  20 +
>>  tests/ovn-northd.at| 240 ++--
>>  tests/ovn.at   |   3 +
>>  tests/system-common-macros.at  |  11 +
>>  tests/system-ovn.at| 149 +++
>>  utilities/containers/fedora/Dockerfile |   1 +
>>  utilities/containers/ubuntu/Dockerfile |   1 +
>>  utilities/ovn-nbctl.8.xml  |   8 +-
>>  utilities/ovn-nbctl.c  |  43 +-
>>  18 files changed, 1079 insertions(+), 63 deletions(-)
> 
> I'm not

Re: [ovs-dev] [PATCH ovn 8/8] northd: Add ACL Sampling.

2024-07-09 Thread Adrián Moreno
On Mon, Jul 08, 2024 at 01:24:14PM GMT, Dumitru Ceara wrote:
> From: Adrian Moreno 
>
> Introduce a new table called Sample where per-flow IPFIX configuration
> can be specified.
> Also, reference rows from such table from the ACL table to enable the
> configuration of ACL sampling. If enabled, northd will add a sample
> action to each ACL related logical flow.
>
> Packets that hit stateful ACLs are sampled in different ways depending
> whether they are initiating a new session or are just forwarded on an
> existing (already allowed) session.  Two new columns ("sample_new" and
> "sample_est") are added to the ACL table to allow for potentially
> different sampling rates for the two cases.
>
> Note: If an ACL has both sampling enabled and a label associated to it
> then the label value overrides the observation point ID defined in the
> sample configuration.  This is a side effect of the implementation
> (observation point IDs are stored in conntrack in the same part of the
> ct_label where ACL labels are also stored).  The two features
> (sampling and ACL labels) serve however similar purposes so it's not
> expected that they're both enabled together.
>
> When sampling is enabled on an ACL additional logical flows are created
> for that ACL (one for stateless ACLs and 3 for stateful ACLs) in the ACL
> action stage of the logical pipeline.  These additional flows match on a
> combination of conntrack state values and observation point id values
> (either against a logical register or against the stored ct_label state)
> in order to determine whether the packets hitting the ACLs must be
> sampled or not.  This comes with a slight increase in the number of
> logical flows and in the number of OpenFlow rules.  The number of
> additional flows _does not_ depend on the original ACL match or action.
>
> New --sample-new and --sample-est optional arguments are added to the
> 'ovn-nbctl acl-add' command to allow configuring these new types of
> sampling for ACLs.
>
> An example workflow of configuring ACL samples is:
>   # Create Sampling_App mappings for ACL traffic types:
>   ovn-nbctl create Sampling_App name="acl-new-traffic-sampling" \
> id="42"
>   ovn-nbctl create sampling_app name="acl-est-traffic-sampling" \
>   id="43"
>   # Create two sample collectors, one that samples all packets (c1)
>   # and another one that samples with a probability of 10% (c2):
>   c1=$(ovn-nbctl create Sample_Collector name=c1 \
>probability=65535 set_id=1)
>   c2=$(ovn-nbctl create Sample_Collector name=c2 \
>probability=6553 set_id=2)
>   # Create two sample configurations (for new and for established
>   # traffic):
>   s1=$(ovn-nbctl create sample collector="$c1 $c2" metadata=4301)
>   s2=$(ovn-nbctl create sample collector="$c1 $c2" metadata=4302)
>   # Create an ingress ACL to allow IP traffic:
>   ovn-nbctl --sample-new=$s1 --sample-est=$s2 acl-add ls \
> from-lport 1 "ip" allow-related
>
> The config above will generate IPFIX samples with:
> - observation domain id set to 42 (Sampling_App
>   "acl-new-traffic-sampling" config) and observation point id
>   set to 4301 (Sample s1) for packets that create a new
>   connection
> - observation domain id set to 43 (Sampling_app
>   "acl-est-traffic-sampling" config) and observation point id
>   set to 4302 (Sample s2) for packets that are part of an already
>   existing connection
>

We assume the dp_key is 0 in this case, right? Maybe worth mentioning
it.

> Reported-at: https://issues.redhat.com/browse/FDP-305
> Signed-off-by: Adrian Moreno 
> Co-authored-by: Dumitru Ceara 
> Signed-off-by: Dumitru Ceara 
> ---
>  NEWS   |   3 +
>  include/ovn/logical-fields.h   |   2 +
>  lib/logical-fields.c   |   6 +
>  northd/northd.c| 519 +++--
>  northd/ovn-northd.8.xml|  26 ++
>  ovn-nb.ovsschema   |  44 ++-
>  ovn-nb.xml |  56 +++
>  tests/atlocal.in   |   6 +
>  tests/ovn-macros.at|   4 +
>  tests/ovn-nbctl.at |  20 +
>  tests/ovn-northd.at| 240 ++--
>  tests/ovn.at   |   3 +
>  tests/system-common-macros.at  |  11 +
>  tests/system-ovn.at| 149 +++
>  utilities/containers/fedora/Dockerfile |   1 +
>  utilities/containers/ubuntu/Dockerfile |   1 +
>  utilities/ovn-nbctl.8.xml  |   8 +-
>  utilities/ovn-nbctl.c  |  43 +-
>  18 files changed, 1079 insertions(+), 63 deletions(-)

I'm not done reviewing. I need to spend some time understanding how
locial flows are built. Not very familiar with this. Plus I want to run
some tests. However, here are some initial thoughts.

>
> diff --git a/NEWS b/NEWS
> index fcf182bc02..7899c623f2 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -41,

Re: [ovs-dev] [PATCH ovn 8/8] northd: Add ACL Sampling.

2024-07-08 Thread Dumitru Ceara
On 7/8/24 13:24, Dumitru Ceara wrote:
> From: Adrian Moreno 
> 
> Introduce a new table called Sample where per-flow IPFIX configuration
> can be specified.
> Also, reference rows from such table from the ACL table to enable the
> configuration of ACL sampling. If enabled, northd will add a sample
> action to each ACL related logical flow.
> 
> Packets that hit stateful ACLs are sampled in different ways depending
> whether they are initiating a new session or are just forwarded on an
> existing (already allowed) session.  Two new columns ("sample_new" and
> "sample_est") are added to the ACL table to allow for potentially
> different sampling rates for the two cases.
> 
> Note: If an ACL has both sampling enabled and a label associated to it
> then the label value overrides the observation point ID defined in the
> sample configuration.  This is a side effect of the implementation
> (observation point IDs are stored in conntrack in the same part of the
> ct_label where ACL labels are also stored).  The two features
> (sampling and ACL labels) serve however similar purposes so it's not
> expected that they're both enabled together.
> 
> When sampling is enabled on an ACL additional logical flows are created
> for that ACL (one for stateless ACLs and 3 for stateful ACLs) in the ACL
> action stage of the logical pipeline.  These additional flows match on a
> combination of conntrack state values and observation point id values
> (either against a logical register or against the stored ct_label state)
> in order to determine whether the packets hitting the ACLs must be
> sampled or not.  This comes with a slight increase in the number of
> logical flows and in the number of OpenFlow rules.  The number of
> additional flows _does not_ depend on the original ACL match or action.
> 
> New --sample-new and --sample-est optional arguments are added to the
> 'ovn-nbctl acl-add' command to allow configuring these new types of
> sampling for ACLs.
> 
> An example workflow of configuring ACL samples is:
>   # Create Sampling_App mappings for ACL traffic types:
>   ovn-nbctl create Sampling_App name="acl-new-traffic-sampling" \
> id="42"
>   ovn-nbctl create sampling_app name="acl-est-traffic-sampling" \
>   id="43"
>   # Create two sample collectors, one that samples all packets (c1)
>   # and another one that samples with a probability of 10% (c2):
>   c1=$(ovn-nbctl create Sample_Collector name=c1 \
>probability=65535 set_id=1)
>   c2=$(ovn-nbctl create Sample_Collector name=c2 \
>probability=6553 set_id=2)
>   # Create two sample configurations (for new and for established
>   # traffic):
>   s1=$(ovn-nbctl create sample collector="$c1 $c2" metadata=4301)
>   s2=$(ovn-nbctl create sample collector="$c1 $c2" metadata=4302)
>   # Create an ingress ACL to allow IP traffic:
>   ovn-nbctl --sample-new=$s1 --sample-est=$s2 acl-add ls \
> from-lport 1 "ip" allow-related
> 
> The config above will generate IPFIX samples with:
> - observation domain id set to 42 (Sampling_App
>   "acl-new-traffic-sampling" config) and observation point id
>   set to 4301 (Sample s1) for packets that create a new
>   connection
> - observation domain id set to 43 (Sampling_app
>   "acl-est-traffic-sampling" config) and observation point id
>   set to 4302 (Sample s2) for packets that are part of an already
>   existing connection
> 
> Reported-at: https://issues.redhat.com/browse/FDP-305
> Signed-off-by: Adrian Moreno 
> Co-authored-by: Dumitru Ceara 
> Signed-off-by: Dumitru Ceara 
> ---

Recheck-request: github-robot

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH ovn 8/8] northd: Add ACL Sampling.

2024-07-08 Thread 0-day Robot
Bleep bloop.  Greetings Dumitru Ceara, 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 96 characters long (recommended limit is 79)
#177 FILE: northd/northd.c:215:
 * ++--+ X |   
LB_L2_AFF_BACKEND_IP6   |

WARNING: Line is 96 characters long (recommended limit is 79)
#178 FILE: northd/northd.c:216:
 * | R1 | ORIG_DIP_IPV4 (>= IN_PRE_STATEFUL)   | R |(>= 
IN_LB_AFF_CHECK && |

WARNING: Line is 96 characters long (recommended limit is 79)
#179 FILE: northd/northd.c:217:
 * ++--+ E | <= 
IN_LB_AFF_LEARN)   |

WARNING: Line is 96 characters long (recommended limit is 79)
#183 FILE: northd/northd.c:220:
 * | R3 | OBS_POINT_ID_NEW |   |
   |

WARNING: Line is 96 characters long (recommended limit is 79)
#184 FILE: northd/northd.c:221:
 * ||   (>= ACL_EVAL* && <= ACL_ACTION*)   |   |
   |

WARNING: Comment with 'xxx' marker
#577 FILE: northd/northd.c:6840:
/* XXX: ACLs with action "pass" do not support sampling. */

WARNING: Line is 560 characters long (recommended limit is 79)
#1734 FILE: utilities/ovn-nbctl.8.xml:402:
[--type={switch | 
port-group}] [--log] 
[--meter=meter] 
[--severity=severity] 
[--name=name] [--label=label] 
[--sample-new=sample] 
[--sample-est=sample] [--may-exist] 
[--apply-after-lb] [--tier] acl-add 
entity direction priority match 
verdict

Lines checked: 1836, Warnings: 7, 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


[ovs-dev] [PATCH ovn 8/8] northd: Add ACL Sampling.

2024-07-08 Thread Dumitru Ceara
From: Adrian Moreno 

Introduce a new table called Sample where per-flow IPFIX configuration
can be specified.
Also, reference rows from such table from the ACL table to enable the
configuration of ACL sampling. If enabled, northd will add a sample
action to each ACL related logical flow.

Packets that hit stateful ACLs are sampled in different ways depending
whether they are initiating a new session or are just forwarded on an
existing (already allowed) session.  Two new columns ("sample_new" and
"sample_est") are added to the ACL table to allow for potentially
different sampling rates for the two cases.

Note: If an ACL has both sampling enabled and a label associated to it
then the label value overrides the observation point ID defined in the
sample configuration.  This is a side effect of the implementation
(observation point IDs are stored in conntrack in the same part of the
ct_label where ACL labels are also stored).  The two features
(sampling and ACL labels) serve however similar purposes so it's not
expected that they're both enabled together.

When sampling is enabled on an ACL additional logical flows are created
for that ACL (one for stateless ACLs and 3 for stateful ACLs) in the ACL
action stage of the logical pipeline.  These additional flows match on a
combination of conntrack state values and observation point id values
(either against a logical register or against the stored ct_label state)
in order to determine whether the packets hitting the ACLs must be
sampled or not.  This comes with a slight increase in the number of
logical flows and in the number of OpenFlow rules.  The number of
additional flows _does not_ depend on the original ACL match or action.

New --sample-new and --sample-est optional arguments are added to the
'ovn-nbctl acl-add' command to allow configuring these new types of
sampling for ACLs.

An example workflow of configuring ACL samples is:
  # Create Sampling_App mappings for ACL traffic types:
  ovn-nbctl create Sampling_App name="acl-new-traffic-sampling" \
id="42"
  ovn-nbctl create sampling_app name="acl-est-traffic-sampling" \
id="43"
  # Create two sample collectors, one that samples all packets (c1)
  # and another one that samples with a probability of 10% (c2):
  c1=$(ovn-nbctl create Sample_Collector name=c1 \
   probability=65535 set_id=1)
  c2=$(ovn-nbctl create Sample_Collector name=c2 \
   probability=6553 set_id=2)
  # Create two sample configurations (for new and for established
  # traffic):
  s1=$(ovn-nbctl create sample collector="$c1 $c2" metadata=4301)
  s2=$(ovn-nbctl create sample collector="$c1 $c2" metadata=4302)
  # Create an ingress ACL to allow IP traffic:
  ovn-nbctl --sample-new=$s1 --sample-est=$s2 acl-add ls \
from-lport 1 "ip" allow-related

The config above will generate IPFIX samples with:
- observation domain id set to 42 (Sampling_App
  "acl-new-traffic-sampling" config) and observation point id
  set to 4301 (Sample s1) for packets that create a new
  connection
- observation domain id set to 43 (Sampling_app
  "acl-est-traffic-sampling" config) and observation point id
  set to 4302 (Sample s2) for packets that are part of an already
  existing connection

Reported-at: https://issues.redhat.com/browse/FDP-305
Signed-off-by: Adrian Moreno 
Co-authored-by: Dumitru Ceara 
Signed-off-by: Dumitru Ceara 
---
 NEWS   |   3 +
 include/ovn/logical-fields.h   |   2 +
 lib/logical-fields.c   |   6 +
 northd/northd.c| 519 +++--
 northd/ovn-northd.8.xml|  26 ++
 ovn-nb.ovsschema   |  44 ++-
 ovn-nb.xml |  56 +++
 tests/atlocal.in   |   6 +
 tests/ovn-macros.at|   4 +
 tests/ovn-nbctl.at |  20 +
 tests/ovn-northd.at| 240 ++--
 tests/ovn.at   |   3 +
 tests/system-common-macros.at  |  11 +
 tests/system-ovn.at| 149 +++
 utilities/containers/fedora/Dockerfile |   1 +
 utilities/containers/ubuntu/Dockerfile |   1 +
 utilities/ovn-nbctl.8.xml  |   8 +-
 utilities/ovn-nbctl.c  |  43 +-
 18 files changed, 1079 insertions(+), 63 deletions(-)

diff --git a/NEWS b/NEWS
index fcf182bc02..7899c623f2 100644
--- a/NEWS
+++ b/NEWS
@@ -41,6 +41,9 @@ Post v24.03.0
   - The NB_Global.debug_drop_domain_id configured value is now overridden by
 the ID associated with the Sampling_App record created for drop sampling
 (Sampling_App.name configured to be "drop-sampling").
+  - Add support for ACL sampling through the new Sample_Collector and Sample
+tables.  Sampling is supported for both traffic that creates new
+connections and for traffic that is part of an existing connection.
 
 OVN v24.03.0 - 01 Mar 2024
 -