| Message ID | dc37f375959c7abcf40ff7edfb5a60239971a97f.1728405774.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] 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 |
Tested this with observability enabled, the logs are as follows: 2024/10/10 15:15:41.438908 group_id=10, obs_domain=50331650, obs_point=1012518368 2024/10/10 15:15:41.438913 OVN-K message: Allowed by network policy iperf:iperf3-server-access-egress, direction Egress 2024/10/10 15:15:41.438918 src=10.131.0.92, dst=10.131.0.93 2024/10/10 15:15:41.439480 group_id=10, obs_domain=50331650, obs_point=1012518368 2024/10/10 15:15:41.439491 decoding failed: find sample failed: object not found 2024/10/10 15:15:41.439493 src=10.131.0.92, dst=10.131.0.93 ...29 more samples like this... 2024/10/10 15:15:41.449298 group_id=10, obs_domain=50331650, obs_point=1012518368 2024/10/10 15:15:41.449440 decoding failed: find sample failed: object not found 2024/10/10 15:15:41.449548 src=10.131.0.92, dst=10.131.0.93 2024/10/10 15:15:41.449828 group_id=10, obs_domain=50331650, obs_point=4102259202 2024/10/10 15:15:41.449990 OVN-K message: Dropped by network policies isolation in namespace iperf, direction Egress 2024/10/10 15:15:41.450000 src=10.131.0.92, dst=10.131.0.93 all the following samples have updated obs_point. Tested-by: Nadia Pinaeva <npinaeva@redhat.com> On Tue, Oct 8, 2024 at 6:45 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> 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> > --- > Changes since v1: > - commit ct_label.obs_point_id set to 0 if the drop acl has no samples > configured > --- > northd/northd.c | 5 ++++- > tests/ovn-northd.at | 20 ++++++++++---------- > 2 files changed, 14 insertions(+), 11 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 0aa0de637..a5bd9fd50 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -7205,7 +7205,10 @@ 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;"); > + uint32_t obs_pid = acl->sample_est ? acl->sample_est->metadata : > 0; > + ds_put_format(actions, > + "ct_commit { ct_mark.blocked = 1; " > + "ct_label.obs_point_id = %"PRIu32"; }; next;", > obs_pid); > ovn_lflow_add_with_hint(lflows, od, stage, priority, > ds_cstr(match), ds_cstr(actions), > &acl->header_, lflow_ref); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index d6a8c4640..10db5398b 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2384,15 +2384,15 @@ AT_CAPTURE_FILE([sw1flows3]) > AT_CHECK([grep "ls_out_acl" sw0flows3 sw1flows3 | grep pg0 | > ovn_strip_lflows], [0], [dnl > sw0flows3: table=??(ls_out_acl_eval ), priority=2001 , > match=(reg0[[7]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; > reg0[[1]] = 1; next;) > sw0flows3: table=??(ls_out_acl_eval ), priority=2001 , > match=(reg0[[8]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; > next;) > -sw0flows3: table=??(ls_out_acl_eval ), priority=2002 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) > +sw0flows3: table=??(ls_out_acl_eval ), priority=2002 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > sw0flows3: table=??(ls_out_acl_eval ), priority=2002 , > match=(reg0[[9]] == 1 && (outport == @pg0 && ip4 && udp)), > action=(reg8[[18]] = 1; next;) > -sw0flows3: table=??(ls_out_acl_eval ), priority=2003 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) > +sw0flows3: table=??(ls_out_acl_eval ), priority=2003 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > sw0flows3: table=??(ls_out_acl_eval ), priority=2003 , > match=(reg0[[9]] == 1 && (outport == @pg0 && ip6 && udp)), > action=(reg8[[18]] = 1; next;) > sw1flows3: table=??(ls_out_acl_eval ), priority=2001 , > match=(reg0[[7]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; > reg0[[1]] = 1; next;) > sw1flows3: table=??(ls_out_acl_eval ), priority=2001 , > match=(reg0[[8]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; > next;) > -sw1flows3: table=??(ls_out_acl_eval ), priority=2002 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) > +sw1flows3: table=??(ls_out_acl_eval ), priority=2002 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > sw1flows3: table=??(ls_out_acl_eval ), priority=2002 , > match=(reg0[[9]] == 1 && (outport == @pg0 && ip4 && udp)), > action=(reg8[[18]] = 1; next;) > -sw1flows3: table=??(ls_out_acl_eval ), priority=2003 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) > +sw1flows3: table=??(ls_out_acl_eval ), priority=2003 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > sw1flows3: table=??(ls_out_acl_eval ), priority=2003 , > match=(reg0[[9]] == 1 && (outport == @pg0 && ip6 && udp)), > action=(reg8[[18]] = 1; next;) > ]) > > @@ -7711,13 +7711,13 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e > "ls_in_acl_hint" lsflows | ovn_strip_lflo > table=??(ls_in_acl_eval ), priority=0 , match=(1), action=(next;) > table=??(ls_in_acl_eval ), priority=1 , match=(ip && !ct.est), > action=(reg0[[1]] = 1; next;) > table=??(ls_in_acl_eval ), priority=1 , match=(ip && ct.est && > ct_mark.blocked == 1), action=(reg0[[1]] = 1; reg8[[16]] = 1; next;) > - table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[10]] == 1 > && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; > next;) > + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[10]] == 1 > && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[9]] == 1 && > (ip4)), action=(reg8[[17]] = 1; next;) > table=??(ls_in_acl_eval ), priority=2002 , match=(reg0[[7]] == 1 && > (ip4 && tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > table=??(ls_in_acl_eval ), priority=2002 , match=(reg0[[8]] == 1 && > (ip4 && tcp)), action=(reg8[[16]] = 1; next;) > table=??(ls_in_acl_eval ), priority=2003 , match=(reg0[[7]] == 1 && > (ip4 && icmp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > table=??(ls_in_acl_eval ), priority=2003 , match=(reg0[[8]] == 1 && > (ip4 && icmp)), action=(reg8[[16]] = 1; next;) > - table=??(ls_in_acl_eval ), priority=2004 , match=(reg0[[10]] == 1 > && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > ct_mark.blocked = 1; }; next;) > + table=??(ls_in_acl_eval ), priority=2004 , match=(reg0[[10]] == 1 > && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) > table=??(ls_in_acl_eval ), priority=2004 , match=(reg0[[9]] == 1 && > (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; next;) > table=??(ls_in_acl_eval ), priority=34000, match=(eth.dst == > $svc_monitor_mac), action=(reg8[[16]] = 1; next;) > table=??(ls_in_acl_eval ), priority=65532, match=(!ct.est && ct.rel > && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(reg0[[17]] = 1; > reg8[[16]] = 1; ct_commit_nat;) > @@ -7761,13 +7761,13 @@ AT_CAPTURE_FILE([lsflows]) > > AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_acl_after_lb_eval), priority=0 , match=(1), > action=(next;) > - table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == > 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; > next;) > + table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == > 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[9]] == > 1 && (ip4)), action=(reg8[[17]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=2002 , match=(reg0[[7]] == > 1 && (ip4 && tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=2002 , match=(reg0[[8]] == > 1 && (ip4 && tcp)), action=(reg8[[16]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[7]] == > 1 && (ip4 && icmp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[8]] == > 1 && (ip4 && icmp)), action=(reg8[[16]] = 1; next;) > - table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > ct_mark.blocked = 1; }; next;) > + table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) > table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[9]] == > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=65532, match=(nd || nd_ra > || nd_rs || mldv1 || mldv2), action=(reg8[[16]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=65532, match=(reg0[[17]] == > 1), action=(reg8[[16]] = 1; next;) > @@ -7816,9 +7816,9 @@ AT_CAPTURE_FILE([lsflows]) > > AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_acl_after_lb_eval), priority=0 , match=(1), > action=(next;) > - table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == > 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; > next;) > + table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == > 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[9]] == > 1 && (ip4)), action=(reg8[[17]] = 1; next;) > - table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > ct_mark.blocked = 1; }; next;) > + table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) > table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[9]] == > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=65532, match=(nd || nd_ra > || nd_rs || mldv1 || mldv2), action=(reg8[[16]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=65532, match=(reg0[[17]] == > 1), action=(reg8[[16]] = 1; next;) > -- > 2.46.2 > >
On Tue, Oct 8, 2024 at 6:45 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> 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> > nit: Double Signed-off-by. --- > Changes since v1: > - commit ct_label.obs_point_id set to 0 if the drop acl has no samples > configured > --- > northd/northd.c | 5 ++++- > tests/ovn-northd.at | 20 ++++++++++---------- > 2 files changed, 14 insertions(+), 11 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 0aa0de637..a5bd9fd50 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -7205,7 +7205,10 @@ 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;"); > + uint32_t obs_pid = acl->sample_est ? acl->sample_est->metadata : > 0; > + ds_put_format(actions, > + "ct_commit { ct_mark.blocked = 1; " > + "ct_label.obs_point_id = %"PRIu32"; }; next;", > obs_pid); > ovn_lflow_add_with_hint(lflows, od, stage, priority, > ds_cstr(match), ds_cstr(actions), > &acl->header_, lflow_ref); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index d6a8c4640..10db5398b 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2384,15 +2384,15 @@ AT_CAPTURE_FILE([sw1flows3]) > AT_CHECK([grep "ls_out_acl" sw0flows3 sw1flows3 | grep pg0 | > ovn_strip_lflows], [0], [dnl > sw0flows3: table=??(ls_out_acl_eval ), priority=2001 , > match=(reg0[[7]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; > reg0[[1]] = 1; next;) > sw0flows3: table=??(ls_out_acl_eval ), priority=2001 , > match=(reg0[[8]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; > next;) > -sw0flows3: table=??(ls_out_acl_eval ), priority=2002 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) > +sw0flows3: table=??(ls_out_acl_eval ), priority=2002 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > sw0flows3: table=??(ls_out_acl_eval ), priority=2002 , > match=(reg0[[9]] == 1 && (outport == @pg0 && ip4 && udp)), > action=(reg8[[18]] = 1; next;) > -sw0flows3: table=??(ls_out_acl_eval ), priority=2003 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) > +sw0flows3: table=??(ls_out_acl_eval ), priority=2003 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > sw0flows3: table=??(ls_out_acl_eval ), priority=2003 , > match=(reg0[[9]] == 1 && (outport == @pg0 && ip6 && udp)), > action=(reg8[[18]] = 1; next;) > sw1flows3: table=??(ls_out_acl_eval ), priority=2001 , > match=(reg0[[7]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; > reg0[[1]] = 1; next;) > sw1flows3: table=??(ls_out_acl_eval ), priority=2001 , > match=(reg0[[8]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; > next;) > -sw1flows3: table=??(ls_out_acl_eval ), priority=2002 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) > +sw1flows3: table=??(ls_out_acl_eval ), priority=2002 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > sw1flows3: table=??(ls_out_acl_eval ), priority=2002 , > match=(reg0[[9]] == 1 && (outport == @pg0 && ip4 && udp)), > action=(reg8[[18]] = 1; next;) > -sw1flows3: table=??(ls_out_acl_eval ), priority=2003 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) > +sw1flows3: table=??(ls_out_acl_eval ), priority=2003 , > match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > sw1flows3: table=??(ls_out_acl_eval ), priority=2003 , > match=(reg0[[9]] == 1 && (outport == @pg0 && ip6 && udp)), > action=(reg8[[18]] = 1; next;) > ]) > > @@ -7711,13 +7711,13 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e > "ls_in_acl_hint" lsflows | ovn_strip_lflo > table=??(ls_in_acl_eval ), priority=0 , match=(1), action=(next;) > table=??(ls_in_acl_eval ), priority=1 , match=(ip && !ct.est), > action=(reg0[[1]] = 1; next;) > table=??(ls_in_acl_eval ), priority=1 , match=(ip && ct.est && > ct_mark.blocked == 1), action=(reg0[[1]] = 1; reg8[[16]] = 1; next;) > - table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[10]] == 1 > && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; > next;) > + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[10]] == 1 > && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[9]] == 1 && > (ip4)), action=(reg8[[17]] = 1; next;) > table=??(ls_in_acl_eval ), priority=2002 , match=(reg0[[7]] == 1 && > (ip4 && tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > table=??(ls_in_acl_eval ), priority=2002 , match=(reg0[[8]] == 1 && > (ip4 && tcp)), action=(reg8[[16]] = 1; next;) > table=??(ls_in_acl_eval ), priority=2003 , match=(reg0[[7]] == 1 && > (ip4 && icmp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > table=??(ls_in_acl_eval ), priority=2003 , match=(reg0[[8]] == 1 && > (ip4 && icmp)), action=(reg8[[16]] = 1; next;) > - table=??(ls_in_acl_eval ), priority=2004 , match=(reg0[[10]] == 1 > && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > ct_mark.blocked = 1; }; next;) > + table=??(ls_in_acl_eval ), priority=2004 , match=(reg0[[10]] == 1 > && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) > table=??(ls_in_acl_eval ), priority=2004 , match=(reg0[[9]] == 1 && > (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; next;) > table=??(ls_in_acl_eval ), priority=34000, match=(eth.dst == > $svc_monitor_mac), action=(reg8[[16]] = 1; next;) > table=??(ls_in_acl_eval ), priority=65532, match=(!ct.est && ct.rel > && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(reg0[[17]] = 1; > reg8[[16]] = 1; ct_commit_nat;) > @@ -7761,13 +7761,13 @@ AT_CAPTURE_FILE([lsflows]) > > AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_acl_after_lb_eval), priority=0 , match=(1), > action=(next;) > - table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == > 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; > next;) > + table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == > 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[9]] == > 1 && (ip4)), action=(reg8[[17]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=2002 , match=(reg0[[7]] == > 1 && (ip4 && tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=2002 , match=(reg0[[8]] == > 1 && (ip4 && tcp)), action=(reg8[[16]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[7]] == > 1 && (ip4 && icmp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[8]] == > 1 && (ip4 && icmp)), action=(reg8[[16]] = 1; next;) > - table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > ct_mark.blocked = 1; }; next;) > + table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) > table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[9]] == > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=65532, match=(nd || nd_ra > || nd_rs || mldv1 || mldv2), action=(reg8[[16]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=65532, match=(reg0[[17]] == > 1), action=(reg8[[16]] = 1; next;) > @@ -7816,9 +7816,9 @@ AT_CAPTURE_FILE([lsflows]) > > AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | > ovn_strip_lflows], [0], [dnl > table=??(ls_in_acl_after_lb_eval), priority=0 , match=(1), > action=(next;) > - table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == > 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; > next;) > + table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == > 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; > ct_label.obs_point_id = 0; }; next;) > table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[9]] == > 1 && (ip4)), action=(reg8[[17]] = 1; next;) > - table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > ct_mark.blocked = 1; }; next;) > + table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) > table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[9]] == > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=65532, match=(nd || nd_ra > || nd_rs || mldv1 || mldv2), action=(reg8[[16]] = 1; next;) > table=??(ls_in_acl_after_lb_eval), priority=65532, match=(reg0[[17]] == > 1), action=(reg8[[16]] = 1; next;) > -- > 2.46.2 > > Looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
On Thu, Nov 7, 2024 at 4:03 AM Ales Musil <amusil@redhat.com> wrote: > > On Tue, Oct 8, 2024 at 6:45 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > 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> > > > > nit: Double Signed-off-by. > > --- > > Changes since v1: > > - commit ct_label.obs_point_id set to 0 if the drop acl has no samples > > configured > > --- > > northd/northd.c | 5 ++++- > > tests/ovn-northd.at | 20 ++++++++++---------- > > 2 files changed, 14 insertions(+), 11 deletions(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index 0aa0de637..a5bd9fd50 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -7205,7 +7205,10 @@ 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;"); > > + uint32_t obs_pid = acl->sample_est ? acl->sample_est->metadata : > > 0; > > + ds_put_format(actions, > > + "ct_commit { ct_mark.blocked = 1; " > > + "ct_label.obs_point_id = %"PRIu32"; }; next;", > > obs_pid); > > ovn_lflow_add_with_hint(lflows, od, stage, priority, > > ds_cstr(match), ds_cstr(actions), > > &acl->header_, lflow_ref); > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index d6a8c4640..10db5398b 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -2384,15 +2384,15 @@ AT_CAPTURE_FILE([sw1flows3]) > > AT_CHECK([grep "ls_out_acl" sw0flows3 sw1flows3 | grep pg0 | > > ovn_strip_lflows], [0], [dnl > > sw0flows3: table=??(ls_out_acl_eval ), priority=2001 , > > match=(reg0[[7]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; > > reg0[[1]] = 1; next;) > > sw0flows3: table=??(ls_out_acl_eval ), priority=2001 , > > match=(reg0[[8]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; > > next;) > > -sw0flows3: table=??(ls_out_acl_eval ), priority=2002 , > > match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), > > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) > > +sw0flows3: table=??(ls_out_acl_eval ), priority=2002 , > > match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), > > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; > > ct_label.obs_point_id = 0; }; next;) > > sw0flows3: table=??(ls_out_acl_eval ), priority=2002 , > > match=(reg0[[9]] == 1 && (outport == @pg0 && ip4 && udp)), > > action=(reg8[[18]] = 1; next;) > > -sw0flows3: table=??(ls_out_acl_eval ), priority=2003 , > > match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), > > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) > > +sw0flows3: table=??(ls_out_acl_eval ), priority=2003 , > > match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), > > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; > > ct_label.obs_point_id = 0; }; next;) > > sw0flows3: table=??(ls_out_acl_eval ), priority=2003 , > > match=(reg0[[9]] == 1 && (outport == @pg0 && ip6 && udp)), > > action=(reg8[[18]] = 1; next;) > > sw1flows3: table=??(ls_out_acl_eval ), priority=2001 , > > match=(reg0[[7]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; > > reg0[[1]] = 1; next;) > > sw1flows3: table=??(ls_out_acl_eval ), priority=2001 , > > match=(reg0[[8]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; > > next;) > > -sw1flows3: table=??(ls_out_acl_eval ), priority=2002 , > > match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), > > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) > > +sw1flows3: table=??(ls_out_acl_eval ), priority=2002 , > > match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), > > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; > > ct_label.obs_point_id = 0; }; next;) > > sw1flows3: table=??(ls_out_acl_eval ), priority=2002 , > > match=(reg0[[9]] == 1 && (outport == @pg0 && ip4 && udp)), > > action=(reg8[[18]] = 1; next;) > > -sw1flows3: table=??(ls_out_acl_eval ), priority=2003 , > > match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), > > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) > > +sw1flows3: table=??(ls_out_acl_eval ), priority=2003 , > > match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), > > action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; > > ct_label.obs_point_id = 0; }; next;) > > sw1flows3: table=??(ls_out_acl_eval ), priority=2003 , > > match=(reg0[[9]] == 1 && (outport == @pg0 && ip6 && udp)), > > action=(reg8[[18]] = 1; next;) > > ]) > > > > @@ -7711,13 +7711,13 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e > > "ls_in_acl_hint" lsflows | ovn_strip_lflo > > table=??(ls_in_acl_eval ), priority=0 , match=(1), action=(next;) > > table=??(ls_in_acl_eval ), priority=1 , match=(ip && !ct.est), > > action=(reg0[[1]] = 1; next;) > > table=??(ls_in_acl_eval ), priority=1 , match=(ip && ct.est && > > ct_mark.blocked == 1), action=(reg0[[1]] = 1; reg8[[16]] = 1; next;) > > - table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[10]] == 1 > > && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; > > next;) > > + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[10]] == 1 > > && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; > > ct_label.obs_point_id = 0; }; next;) > > table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[9]] == 1 && > > (ip4)), action=(reg8[[17]] = 1; next;) > > table=??(ls_in_acl_eval ), priority=2002 , match=(reg0[[7]] == 1 && > > (ip4 && tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > > table=??(ls_in_acl_eval ), priority=2002 , match=(reg0[[8]] == 1 && > > (ip4 && tcp)), action=(reg8[[16]] = 1; next;) > > table=??(ls_in_acl_eval ), priority=2003 , match=(reg0[[7]] == 1 && > > (ip4 && icmp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > > table=??(ls_in_acl_eval ), priority=2003 , match=(reg0[[8]] == 1 && > > (ip4 && icmp)), action=(reg8[[16]] = 1; next;) > > - table=??(ls_in_acl_eval ), priority=2004 , match=(reg0[[10]] == 1 > > && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > > ct_mark.blocked = 1; }; next;) > > + table=??(ls_in_acl_eval ), priority=2004 , match=(reg0[[10]] == 1 > > && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > > ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) > > table=??(ls_in_acl_eval ), priority=2004 , match=(reg0[[9]] == 1 && > > (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; next;) > > table=??(ls_in_acl_eval ), priority=34000, match=(eth.dst == > > $svc_monitor_mac), action=(reg8[[16]] = 1; next;) > > table=??(ls_in_acl_eval ), priority=65532, match=(!ct.est && ct.rel > > && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(reg0[[17]] = 1; > > reg8[[16]] = 1; ct_commit_nat;) > > @@ -7761,13 +7761,13 @@ AT_CAPTURE_FILE([lsflows]) > > > > AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | > > ovn_strip_lflows], [0], [dnl > > table=??(ls_in_acl_after_lb_eval), priority=0 , match=(1), > > action=(next;) > > - table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == > > 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; > > next;) > > + table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == > > 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; > > ct_label.obs_point_id = 0; }; next;) > > table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[9]] == > > 1 && (ip4)), action=(reg8[[17]] = 1; next;) > > table=??(ls_in_acl_after_lb_eval), priority=2002 , match=(reg0[[7]] == > > 1 && (ip4 && tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > > table=??(ls_in_acl_after_lb_eval), priority=2002 , match=(reg0[[8]] == > > 1 && (ip4 && tcp)), action=(reg8[[16]] = 1; next;) > > table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[7]] == > > 1 && (ip4 && icmp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > > table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[8]] == > > 1 && (ip4 && icmp)), action=(reg8[[16]] = 1; next;) > > - table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == > > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > > ct_mark.blocked = 1; }; next;) > > + table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == > > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > > ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) > > table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[9]] == > > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; next;) > > table=??(ls_in_acl_after_lb_eval), priority=65532, match=(nd || nd_ra > > || nd_rs || mldv1 || mldv2), action=(reg8[[16]] = 1; next;) > > table=??(ls_in_acl_after_lb_eval), priority=65532, match=(reg0[[17]] == > > 1), action=(reg8[[16]] = 1; next;) > > @@ -7816,9 +7816,9 @@ AT_CAPTURE_FILE([lsflows]) > > > > AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | > > ovn_strip_lflows], [0], [dnl > > table=??(ls_in_acl_after_lb_eval), priority=0 , match=(1), > > action=(next;) > > - table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == > > 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; > > next;) > > + table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == > > 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; > > ct_label.obs_point_id = 0; }; next;) > > table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[9]] == > > 1 && (ip4)), action=(reg8[[17]] = 1; next;) > > - table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == > > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > > ct_mark.blocked = 1; }; next;) > > + table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == > > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { > > ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) > > table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[9]] == > > 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; next;) > > table=??(ls_in_acl_after_lb_eval), priority=65532, match=(nd || nd_ra > > || nd_rs || mldv1 || mldv2), action=(reg8[[16]] = 1; next;) > > table=??(ls_in_acl_after_lb_eval), priority=65532, match=(reg0[[17]] == > > 1), action=(reg8[[16]] = 1; next;) > > -- > > 2.46.2 > > > > > Looks good to me, thanks. > > Acked-by: Ales Musil <amusil@redhat.com> Thanks. Applied to main and backported to branch-24.09. Numan > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amusil@redhat.com > <https://red.ht/sig> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/northd/northd.c b/northd/northd.c index 0aa0de637..a5bd9fd50 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -7205,7 +7205,10 @@ 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;"); + uint32_t obs_pid = acl->sample_est ? acl->sample_est->metadata : 0; + ds_put_format(actions, + "ct_commit { ct_mark.blocked = 1; " + "ct_label.obs_point_id = %"PRIu32"; }; next;", obs_pid); ovn_lflow_add_with_hint(lflows, od, stage, priority, ds_cstr(match), ds_cstr(actions), &acl->header_, lflow_ref); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index d6a8c4640..10db5398b 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -2384,15 +2384,15 @@ AT_CAPTURE_FILE([sw1flows3]) AT_CHECK([grep "ls_out_acl" sw0flows3 sw1flows3 | grep pg0 | ovn_strip_lflows], [0], [dnl sw0flows3: table=??(ls_out_acl_eval ), priority=2001 , match=(reg0[[7]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) sw0flows3: table=??(ls_out_acl_eval ), priority=2001 , match=(reg0[[8]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; next;) -sw0flows3: table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) +sw0flows3: table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) sw0flows3: table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[9]] == 1 && (outport == @pg0 && ip4 && udp)), action=(reg8[[18]] = 1; next;) -sw0flows3: table=??(ls_out_acl_eval ), priority=2003 , match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) +sw0flows3: table=??(ls_out_acl_eval ), priority=2003 , match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) sw0flows3: table=??(ls_out_acl_eval ), priority=2003 , match=(reg0[[9]] == 1 && (outport == @pg0 && ip6 && udp)), action=(reg8[[18]] = 1; next;) sw1flows3: table=??(ls_out_acl_eval ), priority=2001 , match=(reg0[[7]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) sw1flows3: table=??(ls_out_acl_eval ), priority=2001 , match=(reg0[[8]] == 1 && (outport == @pg0 && ip)), action=(reg8[[16]] = 1; next;) -sw1flows3: table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) +sw1flows3: table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[10]] == 1 && (outport == @pg0 && ip4 && udp)), action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) sw1flows3: table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[9]] == 1 && (outport == @pg0 && ip4 && udp)), action=(reg8[[18]] = 1; next;) -sw1flows3: table=??(ls_out_acl_eval ), priority=2003 , match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) +sw1flows3: table=??(ls_out_acl_eval ), priority=2003 , match=(reg0[[10]] == 1 && (outport == @pg0 && ip6 && udp)), action=(reg8[[18]] = 1; ct_commit { ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) sw1flows3: table=??(ls_out_acl_eval ), priority=2003 , match=(reg0[[9]] == 1 && (outport == @pg0 && ip6 && udp)), action=(reg8[[18]] = 1; next;) ]) @@ -7711,13 +7711,13 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | ovn_strip_lflo table=??(ls_in_acl_eval ), priority=0 , match=(1), action=(next;) table=??(ls_in_acl_eval ), priority=1 , match=(ip && !ct.est), action=(reg0[[1]] = 1; next;) table=??(ls_in_acl_eval ), priority=1 , match=(ip && ct.est && ct_mark.blocked == 1), action=(reg0[[1]] = 1; reg8[[16]] = 1; next;) - table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[10]] == 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[10]] == 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[9]] == 1 && (ip4)), action=(reg8[[17]] = 1; next;) table=??(ls_in_acl_eval ), priority=2002 , match=(reg0[[7]] == 1 && (ip4 && tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) table=??(ls_in_acl_eval ), priority=2002 , match=(reg0[[8]] == 1 && (ip4 && tcp)), action=(reg8[[16]] = 1; next;) table=??(ls_in_acl_eval ), priority=2003 , match=(reg0[[7]] == 1 && (ip4 && icmp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) table=??(ls_in_acl_eval ), priority=2003 , match=(reg0[[8]] == 1 && (ip4 && icmp)), action=(reg8[[16]] = 1; next;) - table=??(ls_in_acl_eval ), priority=2004 , match=(reg0[[10]] == 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) + table=??(ls_in_acl_eval ), priority=2004 , match=(reg0[[10]] == 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) table=??(ls_in_acl_eval ), priority=2004 , match=(reg0[[9]] == 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; next;) table=??(ls_in_acl_eval ), priority=34000, match=(eth.dst == $svc_monitor_mac), action=(reg8[[16]] = 1; next;) table=??(ls_in_acl_eval ), priority=65532, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_mark.blocked == 0), action=(reg0[[17]] = 1; reg8[[16]] = 1; ct_commit_nat;) @@ -7761,13 +7761,13 @@ AT_CAPTURE_FILE([lsflows]) AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | ovn_strip_lflows], [0], [dnl table=??(ls_in_acl_after_lb_eval), priority=0 , match=(1), action=(next;) - table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) + table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[9]] == 1 && (ip4)), action=(reg8[[17]] = 1; next;) table=??(ls_in_acl_after_lb_eval), priority=2002 , match=(reg0[[7]] == 1 && (ip4 && tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) table=??(ls_in_acl_after_lb_eval), priority=2002 , match=(reg0[[8]] == 1 && (ip4 && tcp)), action=(reg8[[16]] = 1; next;) table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[7]] == 1 && (ip4 && icmp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[8]] == 1 && (ip4 && icmp)), action=(reg8[[16]] = 1; next;) - table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) + table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[9]] == 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; next;) table=??(ls_in_acl_after_lb_eval), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(reg8[[16]] = 1; next;) table=??(ls_in_acl_after_lb_eval), priority=65532, match=(reg0[[17]] == 1), action=(reg8[[16]] = 1; next;) @@ -7816,9 +7816,9 @@ AT_CAPTURE_FILE([lsflows]) AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | ovn_strip_lflows], [0], [dnl table=??(ls_in_acl_after_lb_eval), priority=0 , match=(1), action=(next;) - table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) + table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[10]] == 1 && (ip4)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) table=??(ls_in_acl_after_lb_eval), priority=2001 , match=(reg0[[9]] == 1 && (ip4)), action=(reg8[[17]] = 1; next;) - table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; next;) + table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[10]] == 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; ct_label.obs_point_id = 0; }; next;) table=??(ls_in_acl_after_lb_eval), priority=2004 , match=(reg0[[9]] == 1 && (ip4 && ip4.dst == 10.0.0.2)), action=(reg8[[17]] = 1; next;) table=??(ls_in_acl_after_lb_eval), priority=65532, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(reg8[[16]] = 1; next;) table=??(ls_in_acl_after_lb_eval), priority=65532, match=(reg0[[17]] == 1), action=(reg8[[16]] = 1; next;)
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> --- Changes since v1: - commit ct_label.obs_point_id set to 0 if the drop acl has no samples configured --- northd/northd.c | 5 ++++- tests/ovn-northd.at | 20 ++++++++++---------- 2 files changed, 14 insertions(+), 11 deletions(-)