| Message ID | 20220124115055.2710841-1-amorenoz@redhat.com |
|---|---|
| State | Accepted |
| Commit | f0a9000ca61602fd0b454318efcba5ac3e1e0744 |
| Headers | show |
| Series | [ovs-dev,v2] ofproto: fix ipfix not always sampling on egress | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 24 Jan 2022, at 12:50, Adrian Moreno wrote: > We are currently requiring in_port to be a valid port number for ipfix > sampling even if the sampling is done on the output port (egress). > > This restriction is not really needed and can affect pipelines that > intentionally set the in_port to OFPP_NONE during flow processing. For > instance, OVN does this, see: > > cfa547821 Fix ovn-controller generated packets from getting dropped for > reject ACL action. > > This patch skips ipfix sampling only if both (ingress and egress) ports > are invalid. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2016346 > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- Thanks for adding the test. Reviewed and tested and it looks good! Acked-by: Eelco Chaudron <echaudro@redhat.com>
On 1/25/22 10:09, Eelco Chaudron wrote: > > > On 24 Jan 2022, at 12:50, Adrian Moreno wrote: > >> We are currently requiring in_port to be a valid port number for ipfix >> sampling even if the sampling is done on the output port (egress). >> >> This restriction is not really needed and can affect pipelines that >> intentionally set the in_port to OFPP_NONE during flow processing. For >> instance, OVN does this, see: >> >> cfa547821 Fix ovn-controller generated packets from getting dropped for >> reject ACL action. >> >> This patch skips ipfix sampling only if both (ingress and egress) ports >> are invalid. >> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2016346 >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >> --- > > Thanks for adding the test. Reviewed and tested and it looks good! > > Acked-by: Eelco Chaudron <echaudro@redhat.com> Thanks, Adrian and Eelco! Applied and backported down to 2.13. Best regards, Ilya Maximets.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index cafcd014a..189276bc1 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3272,7 +3272,9 @@ compose_ipfix_action(struct xlate_ctx *ctx, odp_port_t output_odp_port) struct dpif_ipfix *ipfix = ctx->xbridge->ipfix; odp_port_t tunnel_out_port = ODPP_NONE; - if (!ipfix || ctx->xin->flow.in_port.ofp_port == OFPP_NONE) { + if (!ipfix || + (output_odp_port == ODPP_NONE && + ctx->xin->flow.in_port.ofp_port == OFPP_NONE)) { return; } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 1660b0856..eb634a781 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -7600,7 +7600,7 @@ dnl configure bridge IPFIX and ensure that sample action generation works at the dnl datapath level. AT_SETUP([ofproto-dpif - Bridge IPFIX sanity check]) OVS_VSWITCHD_START -add_of_ports br0 1 2 +add_of_ports br0 1 2 3 dnl Sample every packet using bridge-based sampling. AT_CHECK([ovs-vsctl -- set bridge br0 ipfix=@fix -- \ @@ -7616,6 +7616,28 @@ flow-dump from the main thread: packets:2, bytes:68, used:0.001s, actions:userspace(pid=0,ipfix(output_port=4294967295)) ]) +AT_CHECK([ovs-appctl revalidator/purge]) + +dnl Check sample is performed even if only one of the ports is present. +AT_DATA([flows.txt], [dnl +table=0,in_port=3,tcp actions=load:0xffff->NXM_OF_IN_PORT[],ct(zone=1,table=1) +table=1,tcp, actions=output:2 +]) +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) + +for i in `seq 1 3`; do + AT_CHECK([ovs-appctl netdev-dummy/receive p3 'in_port(3),eth(src=50:54:00:00:00:08,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=192.168.0.2,dst=192.168.0.1,proto=6,tos=0,ttl=64,frag=no)']) +done + +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*\(packets:\)/\1/' | sed 's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl +flow-dump from the main thread: +packets:2, bytes:236, used:0.001s, actions:userspace(pid=0,ipfix(output_port=2)),2 +packets:2, bytes:236, used:0.001s, actions:userspace(pid=0,ipfix(output_port=4294967295)),ct(zone=1),recirc(0x1) +]) + +AT_CHECK([ovs-ofctl del-flows br0 in_port=3]) +AT_CHECK([ovs-ofctl del-flows br0 table=1]) + AT_CHECK([ovs-appctl revalidator/purge]) dnl dnl Add a slowpath meter. The userspace action should be metered.
We are currently requiring in_port to be a valid port number for ipfix sampling even if the sampling is done on the output port (egress). This restriction is not really needed and can affect pipelines that intentionally set the in_port to OFPP_NONE during flow processing. For instance, OVN does this, see: cfa547821 Fix ovn-controller generated packets from getting dropped for reject ACL action. This patch skips ipfix sampling only if both (ingress and egress) ports are invalid. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2016346 Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- ofproto/ofproto-dpif-xlate.c | 4 +++- tests/ofproto-dpif.at | 24 +++++++++++++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-)