| Message ID | 2b94713da835e9886d69511321b46518c7262c2f.1728377302.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] northd: Commit ct_label.obs_point_id for blocked connections. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On 10/8/24 10:59, Lorenzo Bianconi wrote: > Considering the following configuration: > > $ovn-nbctl acl-list sw01 > from-lport 100 (inport == "sw01-port1" && udp.dst == 5201) allow-related [after-lb] > from-lport 10 (inport == "sw01-port1" && udp) drop [after-lb] > > $ovn-nbctl list acl > _uuid : e440336a-84d3-4a6d-95a9-edd1db1c3631 > action : drop > direction : from-lport > external_ids : {} > label : 0 > log : false > match : "inport == \"sw01-port1\" && udp" > meter : [] > name : [] > options : {apply-after-lb="true"} > priority : 10 > sample_est : ac6a6efc-a2e0-4d68-b5f8-8cd91113e554 > sample_new : 5cdad2ab-4390-4772-ac40-74aa2980c06e > severity : [] > tier : 0 > > _uuid : 85ef08d7-aacc-41d7-b808-6ab011edd753 > action : allow-related > direction : from-lport > external_ids : {} > label : 0 > log : false > match : "inport == \"sw01-port1\" && udp.dst == 5201" > meter : [] > name : [] > options : {apply-after-lb="true"} > priority : 100 > sample_est : 143ce7e2-fd13-4d5e-930c-133d5cf87d0d > sample_new : 1d1a0a05-2a8a-4c72-ad35-77d7e2908183 > severity : [] > tier : 0 > > If the priority-100 acl is removed, the udp traffic with destination port > 5201 will be dropped however ovn-controller will continue sampling the > existing connection with the observationPointID associated to the removed > acl. Fix the issue updating the ct_label.obs_point_id for the connection > marked with ct_mark.blocked. > > Fixes: d15b12da6fe6 ("northd: Add ACL Sampling.") > Repoerted-at: https://issues.redhat.com/browse/FDP-819 > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- Hi Lorenzo, Could you please also add tests for these scenarios? > northd/northd.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 2c4703301..d5b9a54b2 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -7205,7 +7205,14 @@ consider_acl(struct lflow_table *lflows, const struct ovn_datapath *od, > /* For drop ACLs just sample all packets as "new" packets. */ > build_acl_sample_label_action(actions, acl, acl->sample_new, NULL, > obs_stage); > - ds_put_cstr(actions, "ct_commit { ct_mark.blocked = 1; }; next;"); > + if (acl->sample_est) { > + ds_put_format(actions, > + "ct_commit { ct_mark.blocked = 1; " > + "ct_label.obs_point_id = %"PRIu32"; }; next;", > + (uint32_t) acl->sample_est->metadata); > + } else { > + ds_put_cstr(actions, "ct_commit { ct_mark.blocked = 1; }; next;"); > + } Will this ct_commit change the value of ct_label.obs_point_id to 0? I guess not. Shouldn't we explicitly do that here? Otherwise, if the drop ACL doesn't have sampling enabled, I'm afraid we'll still generate samples with the stale point ID. > ovn_lflow_add_with_hint(lflows, od, stage, priority, > ds_cstr(match), ds_cstr(actions), > &acl->header_, lflow_ref); Regards, Dumitru
On Tue, Oct 08, 2024 at 11:51:54AM GMT, Dumitru Ceara wrote: > On 10/8/24 10:59, Lorenzo Bianconi wrote: > > Considering the following configuration: > > > > $ovn-nbctl acl-list sw01 > > from-lport 100 (inport == "sw01-port1" && udp.dst == 5201) allow-related [after-lb] > > from-lport 10 (inport == "sw01-port1" && udp) drop [after-lb] > > > > $ovn-nbctl list acl > > _uuid : e440336a-84d3-4a6d-95a9-edd1db1c3631 > > action : drop > > direction : from-lport > > external_ids : {} > > label : 0 > > log : false > > match : "inport == \"sw01-port1\" && udp" > > meter : [] > > name : [] > > options : {apply-after-lb="true"} > > priority : 10 > > sample_est : ac6a6efc-a2e0-4d68-b5f8-8cd91113e554 > > sample_new : 5cdad2ab-4390-4772-ac40-74aa2980c06e > > severity : [] > > tier : 0 > > > > _uuid : 85ef08d7-aacc-41d7-b808-6ab011edd753 > > action : allow-related > > direction : from-lport > > external_ids : {} > > label : 0 > > log : false > > match : "inport == \"sw01-port1\" && udp.dst == 5201" > > meter : [] > > name : [] > > options : {apply-after-lb="true"} > > priority : 100 > > sample_est : 143ce7e2-fd13-4d5e-930c-133d5cf87d0d > > sample_new : 1d1a0a05-2a8a-4c72-ad35-77d7e2908183 > > severity : [] > > tier : 0 > > > > If the priority-100 acl is removed, the udp traffic with destination port > > 5201 will be dropped however ovn-controller will continue sampling the > > existing connection with the observationPointID associated to the removed > > acl. Fix the issue updating the ct_label.obs_point_id for the connection > > marked with ct_mark.blocked. > > > > Fixes: d15b12da6fe6 ("northd: Add ACL Sampling.") > > Repoerted-at: https://issues.redhat.com/browse/FDP-819 small typo: s/Repoerted-at/Reported-at/ > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > Hi Lorenzo, > > Could you please also add tests for these scenarios? > > > northd/northd.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 2c4703301..d5b9a54b2 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -7205,7 +7205,14 @@ consider_acl(struct lflow_table *lflows, const struct ovn_datapath *od, > > /* For drop ACLs just sample all packets as "new" packets. */ > > build_acl_sample_label_action(actions, acl, acl->sample_new, NULL, > > obs_stage); > > - ds_put_cstr(actions, "ct_commit { ct_mark.blocked = 1; }; next;"); > > + if (acl->sample_est) { > > + ds_put_format(actions, > > + "ct_commit { ct_mark.blocked = 1; " > > + "ct_label.obs_point_id = %"PRIu32"; }; next;", > > + (uint32_t) acl->sample_est->metadata); > > + } else { > > + ds_put_cstr(actions, "ct_commit { ct_mark.blocked = 1; }; next;"); > > + } > > Will this ct_commit change the value of ct_label.obs_point_id to 0? I > guess not. Shouldn't we explicitly do that here? Otherwise, if the > drop ACL doesn't have sampling enabled, I'm afraid we'll still generate > samples with the stale point ID. > Maybe a stupid question: A zero obs_point_id (zero label) is still a valid one from OVS's pov (probably not for the reader), right? If we cannot avoid emitting the sample based on the mark, how would we know if the label should be used or not? Thanks. Adrián > > ovn_lflow_add_with_hint(lflows, od, stage, priority, > > ds_cstr(match), ds_cstr(actions), > > &acl->header_, lflow_ref); > > Regards, > Dumitru >
On 10/8/24 13:16, Adrián Moreno wrote: > On Tue, Oct 08, 2024 at 11:51:54AM GMT, Dumitru Ceara wrote: >> On 10/8/24 10:59, Lorenzo Bianconi wrote: >>> Considering the following configuration: >>> >>> $ovn-nbctl acl-list sw01 >>> from-lport 100 (inport == "sw01-port1" && udp.dst == 5201) allow-related [after-lb] >>> from-lport 10 (inport == "sw01-port1" && udp) drop [after-lb] >>> >>> $ovn-nbctl list acl >>> _uuid : e440336a-84d3-4a6d-95a9-edd1db1c3631 >>> action : drop >>> direction : from-lport >>> external_ids : {} >>> label : 0 >>> log : false >>> match : "inport == \"sw01-port1\" && udp" >>> meter : [] >>> name : [] >>> options : {apply-after-lb="true"} >>> priority : 10 >>> sample_est : ac6a6efc-a2e0-4d68-b5f8-8cd91113e554 >>> sample_new : 5cdad2ab-4390-4772-ac40-74aa2980c06e >>> severity : [] >>> tier : 0 >>> >>> _uuid : 85ef08d7-aacc-41d7-b808-6ab011edd753 >>> action : allow-related >>> direction : from-lport >>> external_ids : {} >>> label : 0 >>> log : false >>> match : "inport == \"sw01-port1\" && udp.dst == 5201" >>> meter : [] >>> name : [] >>> options : {apply-after-lb="true"} >>> priority : 100 >>> sample_est : 143ce7e2-fd13-4d5e-930c-133d5cf87d0d >>> sample_new : 1d1a0a05-2a8a-4c72-ad35-77d7e2908183 >>> severity : [] >>> tier : 0 >>> >>> If the priority-100 acl is removed, the udp traffic with destination port >>> 5201 will be dropped however ovn-controller will continue sampling the >>> existing connection with the observationPointID associated to the removed >>> acl. Fix the issue updating the ct_label.obs_point_id for the connection >>> marked with ct_mark.blocked. >>> >>> Fixes: d15b12da6fe6 ("northd: Add ACL Sampling.") >>> Repoerted-at: https://issues.redhat.com/browse/FDP-819 > > small typo: s/Repoerted-at/Reported-at/ > >>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >>> >>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >>> --- >> >> Hi Lorenzo, >> >> Could you please also add tests for these scenarios? >> >>> northd/northd.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/northd/northd.c b/northd/northd.c >>> index 2c4703301..d5b9a54b2 100644 >>> --- a/northd/northd.c >>> +++ b/northd/northd.c >>> @@ -7205,7 +7205,14 @@ consider_acl(struct lflow_table *lflows, const struct ovn_datapath *od, >>> /* For drop ACLs just sample all packets as "new" packets. */ >>> build_acl_sample_label_action(actions, acl, acl->sample_new, NULL, >>> obs_stage); >>> - ds_put_cstr(actions, "ct_commit { ct_mark.blocked = 1; }; next;"); >>> + if (acl->sample_est) { >>> + ds_put_format(actions, >>> + "ct_commit { ct_mark.blocked = 1; " >>> + "ct_label.obs_point_id = %"PRIu32"; }; next;", >>> + (uint32_t) acl->sample_est->metadata); >>> + } else { >>> + ds_put_cstr(actions, "ct_commit { ct_mark.blocked = 1; }; next;"); >>> + } >> >> Will this ct_commit change the value of ct_label.obs_point_id to 0? I >> guess not. Shouldn't we explicitly do that here? Otherwise, if the >> drop ACL doesn't have sampling enabled, I'm afraid we'll still generate >> samples with the stale point ID. >> > > Maybe a stupid question: > A zero obs_point_id (zero label) is still a valid one from OVS's pov > (probably not for the reader), right? If we cannot avoid emitting the 0 is not a valid sample ID (obs_point_id) from OVN's perspective. From ovn-nb.ovsschema: "metadata": {"type": {"key": {"type": "integer", "minInteger": 1, "maxInteger": 4294967295}, "min": 1, "max":1}} > sample based on the mark, how would we know if the label should be > used or not? > I'm not sure I follow this part. ct_mark.blocked doesn't decide whether a sample should be generated or not. It only is used to "tag" sessions that used to be allowed and now are not anymore (the ACL that used to allow them was removed). From a sampling perspective, after the original ACL is removed we need to either: a. sample with the drop ACL configured sample ID, if that's non-zero b. don't sample, if the drop ACL doesn't have sampling configured Regards, Dumitru
diff --git a/northd/northd.c b/northd/northd.c index 2c4703301..d5b9a54b2 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -7205,7 +7205,14 @@ consider_acl(struct lflow_table *lflows, const struct ovn_datapath *od, /* For drop ACLs just sample all packets as "new" packets. */ build_acl_sample_label_action(actions, acl, acl->sample_new, NULL, obs_stage); - ds_put_cstr(actions, "ct_commit { ct_mark.blocked = 1; }; next;"); + if (acl->sample_est) { + ds_put_format(actions, + "ct_commit { ct_mark.blocked = 1; " + "ct_label.obs_point_id = %"PRIu32"; }; next;", + (uint32_t) acl->sample_est->metadata); + } else { + ds_put_cstr(actions, "ct_commit { ct_mark.blocked = 1; }; next;"); + } ovn_lflow_add_with_hint(lflows, od, stage, priority, ds_cstr(match), ds_cstr(actions), &acl->header_, lflow_ref);
Considering the following configuration: $ovn-nbctl acl-list sw01 from-lport 100 (inport == "sw01-port1" && udp.dst == 5201) allow-related [after-lb] from-lport 10 (inport == "sw01-port1" && udp) drop [after-lb] $ovn-nbctl list acl _uuid : e440336a-84d3-4a6d-95a9-edd1db1c3631 action : drop direction : from-lport external_ids : {} label : 0 log : false match : "inport == \"sw01-port1\" && udp" meter : [] name : [] options : {apply-after-lb="true"} priority : 10 sample_est : ac6a6efc-a2e0-4d68-b5f8-8cd91113e554 sample_new : 5cdad2ab-4390-4772-ac40-74aa2980c06e severity : [] tier : 0 _uuid : 85ef08d7-aacc-41d7-b808-6ab011edd753 action : allow-related direction : from-lport external_ids : {} label : 0 log : false match : "inport == \"sw01-port1\" && udp.dst == 5201" meter : [] name : [] options : {apply-after-lb="true"} priority : 100 sample_est : 143ce7e2-fd13-4d5e-930c-133d5cf87d0d sample_new : 1d1a0a05-2a8a-4c72-ad35-77d7e2908183 severity : [] tier : 0 If the priority-100 acl is removed, the udp traffic with destination port 5201 will be dropped however ovn-controller will continue sampling the existing connection with the observationPointID associated to the removed acl. Fix the issue updating the ct_label.obs_point_id for the connection marked with ct_mark.blocked. Fixes: d15b12da6fe6 ("northd: Add ACL Sampling.") Repoerted-at: https://issues.redhat.com/browse/FDP-819 Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- northd/northd.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)