| Message ID | 20240712151416.992033-6-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Mark Michelson |
| Headers | show |
| Series | Add ACL Sampling using per-flow IPFIX. | 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 | success | github build: passed |
Hi Dumitru, The only complaint I have about this patch is that the comments above build_lb_affinity_ls_flows() need to be updated since they show that REGBIT_CONNTRACK_COMMIT gets reset to 0 as part of their actions. That is pretty trivial and can be fixed when committing. Acked-by: Mark Michelson <mmichels@redhat.com> On 7/12/24 11:14, Dumitru Ceara wrote: > Quoting the ACL label section in the ovn.nb.5 man page: > > Associates an identifier with the ACL. > The same value will be written to corresponding connection > tracker entry. The value should be a valid 32-bit unsigned integer. > This value can help in debugging from connection tracker side. > For example, through this "label" we can backtrack to the ACL rule > which is causing a "leaked" connection. Connection tracker entries are > created only for allowed connections so the label is valid only > for allow and allow-related actions. > > The above states that the ACL label must always be stored in the > connection tracker entry label for allow-related ACLs (regardless of the > direction of the ACL). However, since 74d82e296f80 ("northd: Support > the option to apply from-lport ACLs after load balancer."), the > connection is not re-committed in the ls_in_stateful stage (because it > already was committed as part of the load balancer DNAT). > > Moreover, by not re-committing the connection after LB we also risk not > re-setting any potential ct_mark.blocked value the connection might > have. > > This patch addresses the issue by always committing packets matched by > allow-related (or stateful in general) ACLs even if they were also > committed as part of the load balancing stage. > > There's potentially a slight overhead when doing this (an additional > commit call into conntrack but _no_ recirculation). This is however > acceptable as it is required for a correct packet processing pipeline > implementation. Even without this fix, packets creating new > connections that hit "--apply-after-lb" ACLs trigger a re-commit (for > storing the label and ct_mark.blocked). > > A new test is added to ensure we don't break this functionality in the > future. > > CC: Numan Siddique <numans@ovn.org> > Fixes: 74d82e296f80 ("northd: Support the option to apply from-lport ACLs after load balancer.") > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > northd/northd.c | 8 +--- > tests/ovn-northd.at | 110 +++++++++++++++++++++++++++++++------------- > tests/ovn.at | 4 +- > 3 files changed, 82 insertions(+), 40 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index fcf8f277ac..901b9e9cd1 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -7494,8 +7494,7 @@ build_lb_affinity_ls_flows(struct lflow_table *lflows, > ipv6 ? REG_LB_L2_AFF_BACKEND_IP6 : REG_LB_AFF_BACKEND_IP4; > > /* Prepare common part of affinity LB and affinity learn action. */ > - ds_put_format(&aff_action, REGBIT_CONNTRACK_COMMIT" = 0; %s = %s; ", > - reg_vip, lb_vip->vip_str); > + ds_put_format(&aff_action, "%s = %s; ", reg_vip, lb_vip->vip_str); > ds_put_cstr(&aff_action_learn, "commit_lb_aff(vip = \""); > > if (lb_vip->port_str) { > @@ -7635,11 +7634,6 @@ build_lb_rules(struct lflow_table *lflows, struct ovn_lb_datapaths *lb_dps, > ds_clear(action); > ds_clear(match); > > - /* Make sure that we clear the REGBIT_CONNTRACK_COMMIT flag. Otherwise > - * the load balanced packet will be committed again in > - * S_SWITCH_IN_STATEFUL. */ > - ds_put_format(action, REGBIT_CONNTRACK_COMMIT" = 0; "); > - > /* New connections in Ingress table. */ > const char *meter = NULL; > bool reject = build_lb_vip_actions(lb, lb_vip, lb_vip_nb, action, > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 6802fac380..5acb13c519 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -1413,7 +1413,7 @@ check ovn-nbctl --wait=sb ls-lb-add sw0 lb1 > AT_CAPTURE_FILE([sbflows]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | ovn_strip_lflows], 0, [dnl > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > ]) > > # disabled LSPs should not be a backend of Load Balancer > @@ -1422,7 +1422,7 @@ check ovn-nbctl lsp-set-enabled sw0-p1 disabled > AT_CAPTURE_FILE([sbflows]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | ovn_strip_lflows], 0, [dnl > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=20.0.0.3:80);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=20.0.0.3:80);) > ]) > wait_row_count Service_Monitor 1 > > @@ -1431,7 +1431,7 @@ check ovn-nbctl lsp-set-enabled sw0-p1 enabled > AT_CAPTURE_FILE([sbflows]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | ovn_strip_lflows], 0, [dnl > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > ]) > wait_row_count Service_Monitor 2 > > @@ -1442,7 +1442,7 @@ wait_row_count Service_Monitor 0 > AT_CAPTURE_FILE([sbflows2]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw0 | tee sbflows2 | grep 'priority=120.*backends' | ovn_strip_lflows], [0], > -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > ]) > > AS_BOX([Create the Load_Balancer_Health_Check again.]) > @@ -1454,7 +1454,7 @@ check ovn-nbctl --wait=sb sync > > ovn-sbctl dump-flows sw0 | grep backends | grep priority=120 > lflows.txt > AT_CHECK([cat lflows.txt | ovn_strip_lflows], [0], [dnl > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > ]) > > AS_BOX([Get the uuid of both the service_monitor]) > @@ -1464,7 +1464,7 @@ sm_sw1_p1=$(fetch_column Service_Monitor _uuid logical_port=sw1-p1) > AT_CAPTURE_FILE([sbflows3]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw0 | tee sbflows 3 | grep 'priority=120.*backends' | ovn_strip_lflows], [0], > -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > ]) > > AS_BOX([Set the service monitor for sw1-p1 to offline]) > @@ -1475,7 +1475,7 @@ check ovn-nbctl --wait=sb sync > AT_CAPTURE_FILE([sbflows4]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw0 | tee sbflows4 | grep 'priority=120.*backends' | ovn_strip_lflows], [0], > -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80);) > +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80);) > ]) > > AS_BOX([Set the service monitor for sw0-p1 to offline]) > @@ -1504,7 +1504,7 @@ check ovn-nbctl --wait=sb sync > AT_CAPTURE_FILE([sbflows7]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw0 | tee sbflows7 | grep backends | grep priority=120 | ovn_strip_lflows], 0, > -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > ]) > > AS_BOX([Set the service monitor for sw1-p1 to error]) > @@ -1515,7 +1515,7 @@ check ovn-nbctl --wait=sb sync > ovn-sbctl dump-flows sw0 | grep "ip4.dst == 10.0.0.10 && tcp.dst == 80" \ > | grep priority=120 > lflows.txt > AT_CHECK([cat lflows.txt | grep ls_in_lb | ovn_strip_lflows], [0], [dnl > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80);) > ]) > > AS_BOX([Add one more vip to lb1]) > @@ -1541,8 +1541,8 @@ AT_CAPTURE_FILE([sbflows9]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw0 | tee sbflows9 | grep backends | grep priority=120 | ovn_strip_lflows], > 0, > -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80);) > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:1000);) > +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(ct_lb_mark(backends=10.0.0.3:1000);) > ]) > > AS_BOX([Set the service monitor for sw1-p1 to online]) > @@ -1555,8 +1555,8 @@ AT_CAPTURE_FILE([sbflows10]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw0 | tee sbflows10 | grep backends | grep priority=120 | ovn_strip_lflows], > 0, > -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:1000,20.0.0.3:80);) > +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(ct_lb_mark(backends=10.0.0.3:1000,20.0.0.3:80);) > ]) > > AS_BOX([Associate lb1 to sw1]) > @@ -1565,8 +1565,8 @@ AT_CAPTURE_FILE([sbflows11]) > OVS_WAIT_FOR_OUTPUT( > [ovn-sbctl dump-flows sw1 | tee sbflows11 | grep backends | grep priority=120 | ovn_strip_lflows], > 0, [dnl > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:1000,20.0.0.3:80);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(ct_lb_mark(backends=10.0.0.3:1000,20.0.0.3:80);) > ]) > > AS_BOX([Now create lb2 same as lb1 but udp protocol.]) > @@ -4597,8 +4597,8 @@ check_stateful_flows() { > > AT_CHECK([grep "ls_in_lb " sw0flows | ovn_strip_lflows], [0], [dnl > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.4:8080);) > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.20 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.40:8080);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.4:8080);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.20 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.40:8080);) > ]) > > AT_CHECK([grep "ls_in_stateful" sw0flows | ovn_strip_lflows], [0], [dnl > @@ -4799,6 +4799,51 @@ AT_CHECK([grep "ls_out_stateful" sw0flows | ovn_strip_lflows], [0], [dnl > AT_CLEANUP > ]) > > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([ovn -- ACL label commit - load balancers]) > +ovn_start > + > +dnl One logical switch, two ports, one load balancer and ACLs with label set. > +check ovn-nbctl \ > + -- ls-add ls \ > + -- lsp-add ls lsp1 \ > + -- lsp-set-addresses lsp1 "00:00:00:00:00:01" \ > + -- lsp-add ls lsp2 \ > + -- lsp-set-addresses lsp2 "00:00:00:00:00:02" \ > + -- lb-add lb-test 42.42.42.42:4242 43.43.43.43:4343 udp \ > + -- ls-lb-add ls lb-test > + > +check ovn-nbctl --wait=sb sync > + > +flow="inport == \"lsp1\" && eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02 && ip4.src == 42.42.42.1 && ip4.dst == 42.42.42.42 && ip.ttl==64 && udp.dst == 4242" > + > +AS_BOX([from-lport allow-related ACL]) > +check ovn-nbctl --wait=sb -- acl-del ls -- --label=1234 acl-add ls from-lport 1 ip allow-related > + > +dnl Check that the label is committed to conntrack in the ingress pipeline > +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new ls "$flow" | grep -e ls_in_stateful -A 2 | grep commit], [0], [dnl > + ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; > +]) > + > +AS_BOX([from-lport --apply-after-lb allow-related ACL]) > +check ovn-nbctl --wait=sb -- acl-del ls -- --apply-after-lb --label=1234 acl-add ls from-lport 1 ip allow-related > + > +dnl Check that the label is committed to conntrack in the ingress pipeline > +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new ls "$flow" | grep -e ls_in_stateful -A 2 | grep commit], [0], [dnl > + ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; > +]) > + > +AS_BOX([to-lport allow-related ACL]) > +check ovn-nbctl --wait=sb -- acl-del ls -- --label=1234 acl-add ls to-lport 1 ip allow-related > + > +dnl Check that the label is committed to conntrack in the ingress pipeline > +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new ls "$flow" | grep -e ls_out_stateful -A 2 | grep commit], [0], [dnl > + ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; > +]) > + > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD_NO_HV_PARALLELIZATION([ > AT_SETUP([ovn -- ct.inv usage]) > ovn_start > @@ -7607,7 +7652,7 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | ovn_strip_lflo > > AT_CHECK([grep -e "ls_in_lb " lsflows | ovn_strip_lflows], [0], [dnl > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > - table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.10);) > + table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(ct_lb_mark(backends=10.0.0.10);) > ]) > > AT_CHECK([grep -e "ls_in_stateful" lsflows | ovn_strip_lflows], [0], [dnl > @@ -7662,7 +7707,7 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | ovn_strip_lflo > > AT_CHECK([grep -e "ls_in_lb " lsflows | ovn_strip_lflows], [0], [dnl > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > - table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.10);) > + table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(ct_lb_mark(backends=10.0.0.10);) > ]) > > AT_CHECK([grep -e "ls_in_stateful" lsflows | ovn_strip_lflows], [0], [dnl > @@ -7717,7 +7762,7 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | ovn_strip_lflo > > AT_CHECK([grep -e "ls_in_lb " lsflows | ovn_strip_lflows], [0], [dnl > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > - table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.10);) > + table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(ct_lb_mark(backends=10.0.0.10);) > ]) > > AT_CHECK([grep -e "ls_in_stateful" lsflows | ovn_strip_lflows], [0], [dnl > @@ -9038,13 +9083,13 @@ AT_CAPTURE_FILE([S1flows]) > > AT_CHECK([grep "ls_in_lb " S0flows | ovn_strip_lflows], [0], [dnl > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:80);) > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.11 && tcp.dst == 8080), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:8080);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.2:80);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.11 && tcp.dst == 8080), action=(ct_lb_mark(backends=10.0.0.2:8080);) > ]) > AT_CHECK([grep "ls_in_lb " S1flows | ovn_strip_lflows], [0], [dnl > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:80);) > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.11 && tcp.dst == 8080), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:8080);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.2:80);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.11 && tcp.dst == 8080), action=(ct_lb_mark(backends=10.0.0.2:8080);) > ]) > > ovn-sbctl get datapath S0 _uuid > dp_uuids > @@ -9174,9 +9219,9 @@ AT_CHECK([grep "ls_in_lb_aff_check" S0flows | ovn_strip_lflows], [0], [dnl > ]) > AT_CHECK([grep "ls_in_lb " S0flows | ovn_strip_lflows], [0], [dnl > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);) > - table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) > - table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=20.0.0.2:80);) > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);) > + table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) > + table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=20.0.0.2:80);) > ]) > AT_CHECK([grep "ls_in_lb_aff_learn" S0flows | ovn_strip_lflows], [0], [dnl > table=??(ls_in_lb_aff_learn ), priority=0 , match=(1), action=(next;) > @@ -10261,14 +10306,17 @@ dnl It should not commit anything in the egress pipeline of S1 or in the > dnl ingress pipeline of S2. > flow="inport == \"vm1\" && eth.src == 00:de:ad:01:00:01 && eth.dst == 00:de:ad:fe:00:01 && ip4.src == 173.0.1.2 && ip4.dst == 30.0.0.1 && ip.ttl==64" > > -dnl Check that we only commit once for ACLs, in the egress ACL pipeline > -dnl (in S2, towards vm2). The original problem this test is trying to > -dnl cover was that ct_state wasn't cleared when traversing from s1 -> r1 > -dnl which caused two additional commits to happen: > +dnl Check that we only commit twice for ACLs: > +dnl - in the ingress ACL pipeline (in s1, from vm1) > +dnl - in the egress ACL pipeline (in S2, towards vm2) > +dnl The original problem this test is trying to cover was that ct_state > +dnl wasn't cleared when traversing from s1 -> r1 which caused two additional > +dnl commits to happen: > dnl - in the egress pipeline of S1, when sending the packet out on s1_r1 > dnl - in the ingress pipeline of S2, when processing the packet on s2_r1 > AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new s1 "$flow" | grep -e ls_in_stateful -e ls_out_stateful -A 2 | grep commit], [0], [dnl > ct_commit { ct_mark.blocked = 0; }; > + ct_commit { ct_mark.blocked = 0; }; > ]) > > AT_CLEANUP > diff --git a/tests/ovn.at b/tests/ovn.at > index cecf09a446..877d0dfdba 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -25437,7 +25437,7 @@ OVS_WAIT_FOR_OUTPUT( > ovn-sbctl dump-flows sw0 | grep ct_lb_mark | grep priority=120 | sed 's/table=..//'], 0, > [dnl > (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]] == 1 && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg1 = 10.0.0.10; reg2[[0..15]] = 80; ct_lb_mark;) > - (ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80; hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");) > + (ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80; hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");) > ]) > > AT_CAPTURE_FILE([sbflows2]) > @@ -25636,7 +25636,7 @@ OVS_WAIT_FOR_OUTPUT( > ovn-sbctl dump-flows sw0 | grep ct_lb_mark | grep priority=120 | sed 's/table=..//'], 0, > [dnl > (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]] == 1 && ip6.dst == 2001::a && tcp.dst == 80), action=(xxreg1 = 2001::a; reg2[[0..15]] = 80; ct_lb_mark;) > - (ls_in_lb ), priority=120 , match=(ct.new && ip6.dst == 2001::a && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=[[2001::3]]:80,[[2002::3]]:80; hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");) > + (ls_in_lb ), priority=120 , match=(ct.new && ip6.dst == 2001::a && tcp.dst == 80), action=(ct_lb_mark(backends=[[2001::3]]:80,[[2002::3]]:80; hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");) > ]) > > AT_CAPTURE_FILE([sbflows2])
On Tue, Jul 23, 2024 at 3:05 PM Mark Michelson <mmichels@redhat.com> wrote: > > Hi Dumitru, > > The only complaint I have about this patch is that the comments above > build_lb_affinity_ls_flows() need to be updated since they show that > REGBIT_CONNTRACK_COMMIT gets reset to 0 as part of their actions. That > is pretty trivial and can be fixed when committing. > +! > Acked-by: Mark Michelson <mmichels@redhat.com> Oops. I gave an Ack to v2 of this patch, although I had reviewed v3. Acked-by: Numan Siddique <numans@ovn.org> Numan > > On 7/12/24 11:14, Dumitru Ceara wrote: > > Quoting the ACL label section in the ovn.nb.5 man page: > > > > Associates an identifier with the ACL. > > The same value will be written to corresponding connection > > tracker entry. The value should be a valid 32-bit unsigned integer. > > This value can help in debugging from connection tracker side. > > For example, through this "label" we can backtrack to the ACL rule > > which is causing a "leaked" connection. Connection tracker entries are > > created only for allowed connections so the label is valid only > > for allow and allow-related actions. > > > > The above states that the ACL label must always be stored in the > > connection tracker entry label for allow-related ACLs (regardless of the > > direction of the ACL). However, since 74d82e296f80 ("northd: Support > > the option to apply from-lport ACLs after load balancer."), the > > connection is not re-committed in the ls_in_stateful stage (because it > > already was committed as part of the load balancer DNAT). > > > > Moreover, by not re-committing the connection after LB we also risk not > > re-setting any potential ct_mark.blocked value the connection might > > have. > > > > This patch addresses the issue by always committing packets matched by > > allow-related (or stateful in general) ACLs even if they were also > > committed as part of the load balancing stage. > > > > There's potentially a slight overhead when doing this (an additional > > commit call into conntrack but _no_ recirculation). This is however > > acceptable as it is required for a correct packet processing pipeline > > implementation. Even without this fix, packets creating new > > connections that hit "--apply-after-lb" ACLs trigger a re-commit (for > > storing the label and ct_mark.blocked). > > > > A new test is added to ensure we don't break this functionality in the > > future. > > > > CC: Numan Siddique <numans@ovn.org> > > Fixes: 74d82e296f80 ("northd: Support the option to apply from-lport ACLs after load balancer.") > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > > northd/northd.c | 8 +--- > > tests/ovn-northd.at | 110 +++++++++++++++++++++++++++++++------------- > > tests/ovn.at | 4 +- > > 3 files changed, 82 insertions(+), 40 deletions(-) > > > > diff --git a/northd/northd.c b/northd/northd.c > > index fcf8f277ac..901b9e9cd1 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -7494,8 +7494,7 @@ build_lb_affinity_ls_flows(struct lflow_table *lflows, > > ipv6 ? REG_LB_L2_AFF_BACKEND_IP6 : REG_LB_AFF_BACKEND_IP4; > > > > /* Prepare common part of affinity LB and affinity learn action. */ > > - ds_put_format(&aff_action, REGBIT_CONNTRACK_COMMIT" = 0; %s = %s; ", > > - reg_vip, lb_vip->vip_str); > > + ds_put_format(&aff_action, "%s = %s; ", reg_vip, lb_vip->vip_str); > > ds_put_cstr(&aff_action_learn, "commit_lb_aff(vip = \""); > > > > if (lb_vip->port_str) { > > @@ -7635,11 +7634,6 @@ build_lb_rules(struct lflow_table *lflows, struct ovn_lb_datapaths *lb_dps, > > ds_clear(action); > > ds_clear(match); > > > > - /* Make sure that we clear the REGBIT_CONNTRACK_COMMIT flag. Otherwise > > - * the load balanced packet will be committed again in > > - * S_SWITCH_IN_STATEFUL. */ > > - ds_put_format(action, REGBIT_CONNTRACK_COMMIT" = 0; "); > > - > > /* New connections in Ingress table. */ > > const char *meter = NULL; > > bool reject = build_lb_vip_actions(lb, lb_vip, lb_vip_nb, action, > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 6802fac380..5acb13c519 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -1413,7 +1413,7 @@ check ovn-nbctl --wait=sb ls-lb-add sw0 lb1 > > AT_CAPTURE_FILE([sbflows]) > > OVS_WAIT_FOR_OUTPUT( > > [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | ovn_strip_lflows], 0, [dnl > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > ]) > > > > # disabled LSPs should not be a backend of Load Balancer > > @@ -1422,7 +1422,7 @@ check ovn-nbctl lsp-set-enabled sw0-p1 disabled > > AT_CAPTURE_FILE([sbflows]) > > OVS_WAIT_FOR_OUTPUT( > > [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | ovn_strip_lflows], 0, [dnl > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=20.0.0.3:80);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=20.0.0.3:80);) > > ]) > > wait_row_count Service_Monitor 1 > > > > @@ -1431,7 +1431,7 @@ check ovn-nbctl lsp-set-enabled sw0-p1 enabled > > AT_CAPTURE_FILE([sbflows]) > > OVS_WAIT_FOR_OUTPUT( > > [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | ovn_strip_lflows], 0, [dnl > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > ]) > > wait_row_count Service_Monitor 2 > > > > @@ -1442,7 +1442,7 @@ wait_row_count Service_Monitor 0 > > AT_CAPTURE_FILE([sbflows2]) > > OVS_WAIT_FOR_OUTPUT( > > [ovn-sbctl dump-flows sw0 | tee sbflows2 | grep 'priority=120.*backends' | ovn_strip_lflows], [0], > > -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > ]) > > > > AS_BOX([Create the Load_Balancer_Health_Check again.]) > > @@ -1454,7 +1454,7 @@ check ovn-nbctl --wait=sb sync > > > > ovn-sbctl dump-flows sw0 | grep backends | grep priority=120 > lflows.txt > > AT_CHECK([cat lflows.txt | ovn_strip_lflows], [0], [dnl > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > ]) > > > > AS_BOX([Get the uuid of both the service_monitor]) > > @@ -1464,7 +1464,7 @@ sm_sw1_p1=$(fetch_column Service_Monitor _uuid logical_port=sw1-p1) > > AT_CAPTURE_FILE([sbflows3]) > > OVS_WAIT_FOR_OUTPUT( > > [ovn-sbctl dump-flows sw0 | tee sbflows 3 | grep 'priority=120.*backends' | ovn_strip_lflows], [0], > > -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > ]) > > > > AS_BOX([Set the service monitor for sw1-p1 to offline]) > > @@ -1475,7 +1475,7 @@ check ovn-nbctl --wait=sb sync > > AT_CAPTURE_FILE([sbflows4]) > > OVS_WAIT_FOR_OUTPUT( > > [ovn-sbctl dump-flows sw0 | tee sbflows4 | grep 'priority=120.*backends' | ovn_strip_lflows], [0], > > -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80);) > > +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80);) > > ]) > > > > AS_BOX([Set the service monitor for sw0-p1 to offline]) > > @@ -1504,7 +1504,7 @@ check ovn-nbctl --wait=sb sync > > AT_CAPTURE_FILE([sbflows7]) > > OVS_WAIT_FOR_OUTPUT( > > [ovn-sbctl dump-flows sw0 | tee sbflows7 | grep backends | grep priority=120 | ovn_strip_lflows], 0, > > -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > ]) > > > > AS_BOX([Set the service monitor for sw1-p1 to error]) > > @@ -1515,7 +1515,7 @@ check ovn-nbctl --wait=sb sync > > ovn-sbctl dump-flows sw0 | grep "ip4.dst == 10.0.0.10 && tcp.dst == 80" \ > > | grep priority=120 > lflows.txt > > AT_CHECK([cat lflows.txt | grep ls_in_lb | ovn_strip_lflows], [0], [dnl > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80);) > > ]) > > > > AS_BOX([Add one more vip to lb1]) > > @@ -1541,8 +1541,8 @@ AT_CAPTURE_FILE([sbflows9]) > > OVS_WAIT_FOR_OUTPUT( > > [ovn-sbctl dump-flows sw0 | tee sbflows9 | grep backends | grep priority=120 | ovn_strip_lflows], > > 0, > > -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80);) > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:1000);) > > +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(ct_lb_mark(backends=10.0.0.3:1000);) > > ]) > > > > AS_BOX([Set the service monitor for sw1-p1 to online]) > > @@ -1555,8 +1555,8 @@ AT_CAPTURE_FILE([sbflows10]) > > OVS_WAIT_FOR_OUTPUT( > > [ovn-sbctl dump-flows sw0 | tee sbflows10 | grep backends | grep priority=120 | ovn_strip_lflows], > > 0, > > -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:1000,20.0.0.3:80);) > > +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(ct_lb_mark(backends=10.0.0.3:1000,20.0.0.3:80);) > > ]) > > > > AS_BOX([Associate lb1 to sw1]) > > @@ -1565,8 +1565,8 @@ AT_CAPTURE_FILE([sbflows11]) > > OVS_WAIT_FOR_OUTPUT( > > [ovn-sbctl dump-flows sw1 | tee sbflows11 | grep backends | grep priority=120 | ovn_strip_lflows], > > 0, [dnl > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:1000,20.0.0.3:80);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(ct_lb_mark(backends=10.0.0.3:1000,20.0.0.3:80);) > > ]) > > > > AS_BOX([Now create lb2 same as lb1 but udp protocol.]) > > @@ -4597,8 +4597,8 @@ check_stateful_flows() { > > > > AT_CHECK([grep "ls_in_lb " sw0flows | ovn_strip_lflows], [0], [dnl > > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.4:8080);) > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.20 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.40:8080);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.4:8080);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.20 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.40:8080);) > > ]) > > > > AT_CHECK([grep "ls_in_stateful" sw0flows | ovn_strip_lflows], [0], [dnl > > @@ -4799,6 +4799,51 @@ AT_CHECK([grep "ls_out_stateful" sw0flows | ovn_strip_lflows], [0], [dnl > > AT_CLEANUP > > ]) > > > > +OVN_FOR_EACH_NORTHD_NO_HV([ > > +AT_SETUP([ovn -- ACL label commit - load balancers]) > > +ovn_start > > + > > +dnl One logical switch, two ports, one load balancer and ACLs with label set. > > +check ovn-nbctl \ > > + -- ls-add ls \ > > + -- lsp-add ls lsp1 \ > > + -- lsp-set-addresses lsp1 "00:00:00:00:00:01" \ > > + -- lsp-add ls lsp2 \ > > + -- lsp-set-addresses lsp2 "00:00:00:00:00:02" \ > > + -- lb-add lb-test 42.42.42.42:4242 43.43.43.43:4343 udp \ > > + -- ls-lb-add ls lb-test > > + > > +check ovn-nbctl --wait=sb sync > > + > > +flow="inport == \"lsp1\" && eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02 && ip4.src == 42.42.42.1 && ip4.dst == 42.42.42.42 && ip.ttl==64 && udp.dst == 4242" > > + > > +AS_BOX([from-lport allow-related ACL]) > > +check ovn-nbctl --wait=sb -- acl-del ls -- --label=1234 acl-add ls from-lport 1 ip allow-related > > + > > +dnl Check that the label is committed to conntrack in the ingress pipeline > > +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new ls "$flow" | grep -e ls_in_stateful -A 2 | grep commit], [0], [dnl > > + ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; > > +]) > > + > > +AS_BOX([from-lport --apply-after-lb allow-related ACL]) > > +check ovn-nbctl --wait=sb -- acl-del ls -- --apply-after-lb --label=1234 acl-add ls from-lport 1 ip allow-related > > + > > +dnl Check that the label is committed to conntrack in the ingress pipeline > > +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new ls "$flow" | grep -e ls_in_stateful -A 2 | grep commit], [0], [dnl > > + ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; > > +]) > > + > > +AS_BOX([to-lport allow-related ACL]) > > +check ovn-nbctl --wait=sb -- acl-del ls -- --label=1234 acl-add ls to-lport 1 ip allow-related > > + > > +dnl Check that the label is committed to conntrack in the ingress pipeline > > +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new ls "$flow" | grep -e ls_out_stateful -A 2 | grep commit], [0], [dnl > > + ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; > > +]) > > + > > +AT_CLEANUP > > +]) > > + > > OVN_FOR_EACH_NORTHD_NO_HV_PARALLELIZATION([ > > AT_SETUP([ovn -- ct.inv usage]) > > ovn_start > > @@ -7607,7 +7652,7 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | ovn_strip_lflo > > > > AT_CHECK([grep -e "ls_in_lb " lsflows | ovn_strip_lflows], [0], [dnl > > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > > - table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.10);) > > + table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(ct_lb_mark(backends=10.0.0.10);) > > ]) > > > > AT_CHECK([grep -e "ls_in_stateful" lsflows | ovn_strip_lflows], [0], [dnl > > @@ -7662,7 +7707,7 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | ovn_strip_lflo > > > > AT_CHECK([grep -e "ls_in_lb " lsflows | ovn_strip_lflows], [0], [dnl > > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > > - table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.10);) > > + table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(ct_lb_mark(backends=10.0.0.10);) > > ]) > > > > AT_CHECK([grep -e "ls_in_stateful" lsflows | ovn_strip_lflows], [0], [dnl > > @@ -7717,7 +7762,7 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | ovn_strip_lflo > > > > AT_CHECK([grep -e "ls_in_lb " lsflows | ovn_strip_lflows], [0], [dnl > > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > > - table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.10);) > > + table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(ct_lb_mark(backends=10.0.0.10);) > > ]) > > > > AT_CHECK([grep -e "ls_in_stateful" lsflows | ovn_strip_lflows], [0], [dnl > > @@ -9038,13 +9083,13 @@ AT_CAPTURE_FILE([S1flows]) > > > > AT_CHECK([grep "ls_in_lb " S0flows | ovn_strip_lflows], [0], [dnl > > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:80);) > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.11 && tcp.dst == 8080), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:8080);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.2:80);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.11 && tcp.dst == 8080), action=(ct_lb_mark(backends=10.0.0.2:8080);) > > ]) > > AT_CHECK([grep "ls_in_lb " S1flows | ovn_strip_lflows], [0], [dnl > > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:80);) > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.11 && tcp.dst == 8080), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:8080);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.2:80);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.11 && tcp.dst == 8080), action=(ct_lb_mark(backends=10.0.0.2:8080);) > > ]) > > > > ovn-sbctl get datapath S0 _uuid > dp_uuids > > @@ -9174,9 +9219,9 @@ AT_CHECK([grep "ls_in_lb_aff_check" S0flows | ovn_strip_lflows], [0], [dnl > > ]) > > AT_CHECK([grep "ls_in_lb " S0flows | ovn_strip_lflows], [0], [dnl > > table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) > > - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);) > > - table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) > > - table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=20.0.0.2:80);) > > + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);) > > + table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) > > + table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=20.0.0.2:80);) > > ]) > > AT_CHECK([grep "ls_in_lb_aff_learn" S0flows | ovn_strip_lflows], [0], [dnl > > table=??(ls_in_lb_aff_learn ), priority=0 , match=(1), action=(next;) > > @@ -10261,14 +10306,17 @@ dnl It should not commit anything in the egress pipeline of S1 or in the > > dnl ingress pipeline of S2. > > flow="inport == \"vm1\" && eth.src == 00:de:ad:01:00:01 && eth.dst == 00:de:ad:fe:00:01 && ip4.src == 173.0.1.2 && ip4.dst == 30.0.0.1 && ip.ttl==64" > > > > -dnl Check that we only commit once for ACLs, in the egress ACL pipeline > > -dnl (in S2, towards vm2). The original problem this test is trying to > > -dnl cover was that ct_state wasn't cleared when traversing from s1 -> r1 > > -dnl which caused two additional commits to happen: > > +dnl Check that we only commit twice for ACLs: > > +dnl - in the ingress ACL pipeline (in s1, from vm1) > > +dnl - in the egress ACL pipeline (in S2, towards vm2) > > +dnl The original problem this test is trying to cover was that ct_state > > +dnl wasn't cleared when traversing from s1 -> r1 which caused two additional > > +dnl commits to happen: > > dnl - in the egress pipeline of S1, when sending the packet out on s1_r1 > > dnl - in the ingress pipeline of S2, when processing the packet on s2_r1 > > AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new s1 "$flow" | grep -e ls_in_stateful -e ls_out_stateful -A 2 | grep commit], [0], [dnl > > ct_commit { ct_mark.blocked = 0; }; > > + ct_commit { ct_mark.blocked = 0; }; > > ]) > > > > AT_CLEANUP > > diff --git a/tests/ovn.at b/tests/ovn.at > > index cecf09a446..877d0dfdba 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -25437,7 +25437,7 @@ OVS_WAIT_FOR_OUTPUT( > > ovn-sbctl dump-flows sw0 | grep ct_lb_mark | grep priority=120 | sed 's/table=..//'], 0, > > [dnl > > (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]] == 1 && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg1 = 10.0.0.10; reg2[[0..15]] = 80; ct_lb_mark;) > > - (ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80; hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");) > > + (ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80; hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");) > > ]) > > > > AT_CAPTURE_FILE([sbflows2]) > > @@ -25636,7 +25636,7 @@ OVS_WAIT_FOR_OUTPUT( > > ovn-sbctl dump-flows sw0 | grep ct_lb_mark | grep priority=120 | sed 's/table=..//'], 0, > > [dnl > > (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]] == 1 && ip6.dst == 2001::a && tcp.dst == 80), action=(xxreg1 = 2001::a; reg2[[0..15]] = 80; ct_lb_mark;) > > - (ls_in_lb ), priority=120 , match=(ct.new && ip6.dst == 2001::a && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=[[2001::3]]:80,[[2002::3]]:80; hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");) > > + (ls_in_lb ), priority=120 , match=(ct.new && ip6.dst == 2001::a && tcp.dst == 80), action=(ct_lb_mark(backends=[[2001::3]]:80,[[2002::3]]:80; hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");) > > ]) > > > > AT_CAPTURE_FILE([sbflows2]) > > _______________________________________________ > 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 fcf8f277ac..901b9e9cd1 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -7494,8 +7494,7 @@ build_lb_affinity_ls_flows(struct lflow_table *lflows, ipv6 ? REG_LB_L2_AFF_BACKEND_IP6 : REG_LB_AFF_BACKEND_IP4; /* Prepare common part of affinity LB and affinity learn action. */ - ds_put_format(&aff_action, REGBIT_CONNTRACK_COMMIT" = 0; %s = %s; ", - reg_vip, lb_vip->vip_str); + ds_put_format(&aff_action, "%s = %s; ", reg_vip, lb_vip->vip_str); ds_put_cstr(&aff_action_learn, "commit_lb_aff(vip = \""); if (lb_vip->port_str) { @@ -7635,11 +7634,6 @@ build_lb_rules(struct lflow_table *lflows, struct ovn_lb_datapaths *lb_dps, ds_clear(action); ds_clear(match); - /* Make sure that we clear the REGBIT_CONNTRACK_COMMIT flag. Otherwise - * the load balanced packet will be committed again in - * S_SWITCH_IN_STATEFUL. */ - ds_put_format(action, REGBIT_CONNTRACK_COMMIT" = 0; "); - /* New connections in Ingress table. */ const char *meter = NULL; bool reject = build_lb_vip_actions(lb, lb_vip, lb_vip_nb, action, diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 6802fac380..5acb13c519 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -1413,7 +1413,7 @@ check ovn-nbctl --wait=sb ls-lb-add sw0 lb1 AT_CAPTURE_FILE([sbflows]) OVS_WAIT_FOR_OUTPUT( [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | ovn_strip_lflows], 0, [dnl - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) ]) # disabled LSPs should not be a backend of Load Balancer @@ -1422,7 +1422,7 @@ check ovn-nbctl lsp-set-enabled sw0-p1 disabled AT_CAPTURE_FILE([sbflows]) OVS_WAIT_FOR_OUTPUT( [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | ovn_strip_lflows], 0, [dnl - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=20.0.0.3:80);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=20.0.0.3:80);) ]) wait_row_count Service_Monitor 1 @@ -1431,7 +1431,7 @@ check ovn-nbctl lsp-set-enabled sw0-p1 enabled AT_CAPTURE_FILE([sbflows]) OVS_WAIT_FOR_OUTPUT( [ovn-sbctl dump-flows sw0 | tee sbflows | grep 'priority=120.*backends' | ovn_strip_lflows], 0, [dnl - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) ]) wait_row_count Service_Monitor 2 @@ -1442,7 +1442,7 @@ wait_row_count Service_Monitor 0 AT_CAPTURE_FILE([sbflows2]) OVS_WAIT_FOR_OUTPUT( [ovn-sbctl dump-flows sw0 | tee sbflows2 | grep 'priority=120.*backends' | ovn_strip_lflows], [0], -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) ]) AS_BOX([Create the Load_Balancer_Health_Check again.]) @@ -1454,7 +1454,7 @@ check ovn-nbctl --wait=sb sync ovn-sbctl dump-flows sw0 | grep backends | grep priority=120 > lflows.txt AT_CHECK([cat lflows.txt | ovn_strip_lflows], [0], [dnl - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) ]) AS_BOX([Get the uuid of both the service_monitor]) @@ -1464,7 +1464,7 @@ sm_sw1_p1=$(fetch_column Service_Monitor _uuid logical_port=sw1-p1) AT_CAPTURE_FILE([sbflows3]) OVS_WAIT_FOR_OUTPUT( [ovn-sbctl dump-flows sw0 | tee sbflows 3 | grep 'priority=120.*backends' | ovn_strip_lflows], [0], -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) ]) AS_BOX([Set the service monitor for sw1-p1 to offline]) @@ -1475,7 +1475,7 @@ check ovn-nbctl --wait=sb sync AT_CAPTURE_FILE([sbflows4]) OVS_WAIT_FOR_OUTPUT( [ovn-sbctl dump-flows sw0 | tee sbflows4 | grep 'priority=120.*backends' | ovn_strip_lflows], [0], -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80);) +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80);) ]) AS_BOX([Set the service monitor for sw0-p1 to offline]) @@ -1504,7 +1504,7 @@ check ovn-nbctl --wait=sb sync AT_CAPTURE_FILE([sbflows7]) OVS_WAIT_FOR_OUTPUT( [ovn-sbctl dump-flows sw0 | tee sbflows7 | grep backends | grep priority=120 | ovn_strip_lflows], 0, -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) ]) AS_BOX([Set the service monitor for sw1-p1 to error]) @@ -1515,7 +1515,7 @@ check ovn-nbctl --wait=sb sync ovn-sbctl dump-flows sw0 | grep "ip4.dst == 10.0.0.10 && tcp.dst == 80" \ | grep priority=120 > lflows.txt AT_CHECK([cat lflows.txt | grep ls_in_lb | ovn_strip_lflows], [0], [dnl - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80);) ]) AS_BOX([Add one more vip to lb1]) @@ -1541,8 +1541,8 @@ AT_CAPTURE_FILE([sbflows9]) OVS_WAIT_FOR_OUTPUT( [ovn-sbctl dump-flows sw0 | tee sbflows9 | grep backends | grep priority=120 | ovn_strip_lflows], 0, -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80);) - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:1000);) +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(ct_lb_mark(backends=10.0.0.3:1000);) ]) AS_BOX([Set the service monitor for sw1-p1 to online]) @@ -1555,8 +1555,8 @@ AT_CAPTURE_FILE([sbflows10]) OVS_WAIT_FOR_OUTPUT( [ovn-sbctl dump-flows sw0 | tee sbflows10 | grep backends | grep priority=120 | ovn_strip_lflows], 0, -[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:1000,20.0.0.3:80);) +[ table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(ct_lb_mark(backends=10.0.0.3:1000,20.0.0.3:80);) ]) AS_BOX([Associate lb1 to sw1]) @@ -1565,8 +1565,8 @@ AT_CAPTURE_FILE([sbflows11]) OVS_WAIT_FOR_OUTPUT( [ovn-sbctl dump-flows sw1 | tee sbflows11 | grep backends | grep priority=120 | ovn_strip_lflows], 0, [dnl - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:1000,20.0.0.3:80);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.40 && tcp.dst == 1000), action=(ct_lb_mark(backends=10.0.0.3:1000,20.0.0.3:80);) ]) AS_BOX([Now create lb2 same as lb1 but udp protocol.]) @@ -4597,8 +4597,8 @@ check_stateful_flows() { AT_CHECK([grep "ls_in_lb " sw0flows | ovn_strip_lflows], [0], [dnl table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.4:8080);) - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.20 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.40:8080);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.4:8080);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.20 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.40:8080);) ]) AT_CHECK([grep "ls_in_stateful" sw0flows | ovn_strip_lflows], [0], [dnl @@ -4799,6 +4799,51 @@ AT_CHECK([grep "ls_out_stateful" sw0flows | ovn_strip_lflows], [0], [dnl AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([ovn -- ACL label commit - load balancers]) +ovn_start + +dnl One logical switch, two ports, one load balancer and ACLs with label set. +check ovn-nbctl \ + -- ls-add ls \ + -- lsp-add ls lsp1 \ + -- lsp-set-addresses lsp1 "00:00:00:00:00:01" \ + -- lsp-add ls lsp2 \ + -- lsp-set-addresses lsp2 "00:00:00:00:00:02" \ + -- lb-add lb-test 42.42.42.42:4242 43.43.43.43:4343 udp \ + -- ls-lb-add ls lb-test + +check ovn-nbctl --wait=sb sync + +flow="inport == \"lsp1\" && eth.src == 00:00:00:00:00:01 && eth.dst == 00:00:00:00:00:02 && ip4.src == 42.42.42.1 && ip4.dst == 42.42.42.42 && ip.ttl==64 && udp.dst == 4242" + +AS_BOX([from-lport allow-related ACL]) +check ovn-nbctl --wait=sb -- acl-del ls -- --label=1234 acl-add ls from-lport 1 ip allow-related + +dnl Check that the label is committed to conntrack in the ingress pipeline +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new ls "$flow" | grep -e ls_in_stateful -A 2 | grep commit], [0], [dnl + ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; +]) + +AS_BOX([from-lport --apply-after-lb allow-related ACL]) +check ovn-nbctl --wait=sb -- acl-del ls -- --apply-after-lb --label=1234 acl-add ls from-lport 1 ip allow-related + +dnl Check that the label is committed to conntrack in the ingress pipeline +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new ls "$flow" | grep -e ls_in_stateful -A 2 | grep commit], [0], [dnl + ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; +]) + +AS_BOX([to-lport allow-related ACL]) +check ovn-nbctl --wait=sb -- acl-del ls -- --label=1234 acl-add ls to-lport 1 ip allow-related + +dnl Check that the label is committed to conntrack in the ingress pipeline +AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new ls "$flow" | grep -e ls_out_stateful -A 2 | grep commit], [0], [dnl + ct_commit { ct_mark.blocked = 0; ct_label.label = reg3; }; +]) + +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD_NO_HV_PARALLELIZATION([ AT_SETUP([ovn -- ct.inv usage]) ovn_start @@ -7607,7 +7652,7 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | ovn_strip_lflo AT_CHECK([grep -e "ls_in_lb " lsflows | ovn_strip_lflows], [0], [dnl table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) - table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.10);) + table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(ct_lb_mark(backends=10.0.0.10);) ]) AT_CHECK([grep -e "ls_in_stateful" lsflows | ovn_strip_lflows], [0], [dnl @@ -7662,7 +7707,7 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | ovn_strip_lflo AT_CHECK([grep -e "ls_in_lb " lsflows | ovn_strip_lflows], [0], [dnl table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) - table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.10);) + table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(ct_lb_mark(backends=10.0.0.10);) ]) AT_CHECK([grep -e "ls_in_stateful" lsflows | ovn_strip_lflows], [0], [dnl @@ -7717,7 +7762,7 @@ AT_CHECK([grep -e "ls_in_acl.*eval" -e "ls_in_acl_hint" lsflows | ovn_strip_lflo AT_CHECK([grep -e "ls_in_lb " lsflows | ovn_strip_lflows], [0], [dnl table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) - table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.10);) + table=??(ls_in_lb ), priority=110 , match=(ct.new && ip4.dst == 10.0.0.2), action=(ct_lb_mark(backends=10.0.0.10);) ]) AT_CHECK([grep -e "ls_in_stateful" lsflows | ovn_strip_lflows], [0], [dnl @@ -9038,13 +9083,13 @@ AT_CAPTURE_FILE([S1flows]) AT_CHECK([grep "ls_in_lb " S0flows | ovn_strip_lflows], [0], [dnl table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:80);) - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.11 && tcp.dst == 8080), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:8080);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.2:80);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.11 && tcp.dst == 8080), action=(ct_lb_mark(backends=10.0.0.2:8080);) ]) AT_CHECK([grep "ls_in_lb " S1flows | ovn_strip_lflows], [0], [dnl table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:80);) - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.11 && tcp.dst == 8080), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:8080);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.2:80);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.11 && tcp.dst == 8080), action=(ct_lb_mark(backends=10.0.0.2:8080);) ]) ovn-sbctl get datapath S0 _uuid > dp_uuids @@ -9174,9 +9219,9 @@ AT_CHECK([grep "ls_in_lb_aff_check" S0flows | ovn_strip_lflows], [0], [dnl ]) AT_CHECK([grep "ls_in_lb " S0flows | ovn_strip_lflows], [0], [dnl table=??(ls_in_lb ), priority=0 , match=(1), action=(next;) - table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);) - table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) - table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg0[[1]] = 0; reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=20.0.0.2:80);) + table=??(ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 172.16.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.2:80,20.0.0.2:80);) + table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 10.0.0.2 && reg8[[0..15]] == 80), action=(reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=10.0.0.2:80);) + table=??(ls_in_lb ), priority=150 , match=(reg9[[6]] == 1 && ct.new && ip4.dst == 172.16.0.10 && reg4 == 20.0.0.2 && reg8[[0..15]] == 80), action=(reg1 = 172.16.0.10; reg2[[0..15]] = 80; ct_lb_mark(backends=20.0.0.2:80);) ]) AT_CHECK([grep "ls_in_lb_aff_learn" S0flows | ovn_strip_lflows], [0], [dnl table=??(ls_in_lb_aff_learn ), priority=0 , match=(1), action=(next;) @@ -10261,14 +10306,17 @@ dnl It should not commit anything in the egress pipeline of S1 or in the dnl ingress pipeline of S2. flow="inport == \"vm1\" && eth.src == 00:de:ad:01:00:01 && eth.dst == 00:de:ad:fe:00:01 && ip4.src == 173.0.1.2 && ip4.dst == 30.0.0.1 && ip.ttl==64" -dnl Check that we only commit once for ACLs, in the egress ACL pipeline -dnl (in S2, towards vm2). The original problem this test is trying to -dnl cover was that ct_state wasn't cleared when traversing from s1 -> r1 -dnl which caused two additional commits to happen: +dnl Check that we only commit twice for ACLs: +dnl - in the ingress ACL pipeline (in s1, from vm1) +dnl - in the egress ACL pipeline (in S2, towards vm2) +dnl The original problem this test is trying to cover was that ct_state +dnl wasn't cleared when traversing from s1 -> r1 which caused two additional +dnl commits to happen: dnl - in the egress pipeline of S1, when sending the packet out on s1_r1 dnl - in the ingress pipeline of S2, when processing the packet on s2_r1 AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --ct new s1 "$flow" | grep -e ls_in_stateful -e ls_out_stateful -A 2 | grep commit], [0], [dnl ct_commit { ct_mark.blocked = 0; }; + ct_commit { ct_mark.blocked = 0; }; ]) AT_CLEANUP diff --git a/tests/ovn.at b/tests/ovn.at index cecf09a446..877d0dfdba 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -25437,7 +25437,7 @@ OVS_WAIT_FOR_OUTPUT( ovn-sbctl dump-flows sw0 | grep ct_lb_mark | grep priority=120 | sed 's/table=..//'], 0, [dnl (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]] == 1 && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg1 = 10.0.0.10; reg2[[0..15]] = 80; ct_lb_mark;) - (ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80; hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");) + (ls_in_lb ), priority=120 , match=(ct.new && ip4.dst == 10.0.0.10 && tcp.dst == 80), action=(ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80; hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");) ]) AT_CAPTURE_FILE([sbflows2]) @@ -25636,7 +25636,7 @@ OVS_WAIT_FOR_OUTPUT( ovn-sbctl dump-flows sw0 | grep ct_lb_mark | grep priority=120 | sed 's/table=..//'], 0, [dnl (ls_in_pre_stateful ), priority=120 , match=(reg0[[2]] == 1 && ip6.dst == 2001::a && tcp.dst == 80), action=(xxreg1 = 2001::a; reg2[[0..15]] = 80; ct_lb_mark;) - (ls_in_lb ), priority=120 , match=(ct.new && ip6.dst == 2001::a && tcp.dst == 80), action=(reg0[[1]] = 0; ct_lb_mark(backends=[[2001::3]]:80,[[2002::3]]:80; hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");) + (ls_in_lb ), priority=120 , match=(ct.new && ip6.dst == 2001::a && tcp.dst == 80), action=(ct_lb_mark(backends=[[2001::3]]:80,[[2002::3]]:80; hash_fields="ip_dst,ip_src,tcp_dst,tcp_src");) ]) AT_CAPTURE_FILE([sbflows2])
Quoting the ACL label section in the ovn.nb.5 man page: Associates an identifier with the ACL. The same value will be written to corresponding connection tracker entry. The value should be a valid 32-bit unsigned integer. This value can help in debugging from connection tracker side. For example, through this "label" we can backtrack to the ACL rule which is causing a "leaked" connection. Connection tracker entries are created only for allowed connections so the label is valid only for allow and allow-related actions. The above states that the ACL label must always be stored in the connection tracker entry label for allow-related ACLs (regardless of the direction of the ACL). However, since 74d82e296f80 ("northd: Support the option to apply from-lport ACLs after load balancer."), the connection is not re-committed in the ls_in_stateful stage (because it already was committed as part of the load balancer DNAT). Moreover, by not re-committing the connection after LB we also risk not re-setting any potential ct_mark.blocked value the connection might have. This patch addresses the issue by always committing packets matched by allow-related (or stateful in general) ACLs even if they were also committed as part of the load balancing stage. There's potentially a slight overhead when doing this (an additional commit call into conntrack but _no_ recirculation). This is however acceptable as it is required for a correct packet processing pipeline implementation. Even without this fix, packets creating new connections that hit "--apply-after-lb" ACLs trigger a re-commit (for storing the label and ct_mark.blocked). A new test is added to ensure we don't break this functionality in the future. CC: Numan Siddique <numans@ovn.org> Fixes: 74d82e296f80 ("northd: Support the option to apply from-lport ACLs after load balancer.") Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- northd/northd.c | 8 +--- tests/ovn-northd.at | 110 +++++++++++++++++++++++++++++++------------- tests/ovn.at | 4 +- 3 files changed, 82 insertions(+), 40 deletions(-)