On Thu, Feb 16, 2023 at 03:05:08PM -0500, Mike Pattrick wrote:
> Currently OVS keeps track of which mirrors that each packet has been
> sent to for the purpose of deduplication. However, this doesn't consider
> that openflow rules can make significant changes to packets after
> ingress.
>
> For example, OVN can create OpenFlow rules that turn an echo request
> into an echo response by flipping source/destination addresses and
> setting the ICMP type to Reply. When a mirror is configured, only the
> request gets mirrored even though a response is recieved.
nit: I think 'received' at the end of the paragraph is a bit confusing.
Received by who? (Yes, I know who :)
> This can cause a false impression of the actual traffic on wire if
> someone inspects the mirror and doesn't see an echo reply even though
> one has been sent.
>
> This patch resets the mirrors every time a packet is modified, so
> mirrors will recieve every copy of a packet that is sent for output.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2155579
> Signed-off-by: Mike Pattrick <m...@redhat.com>
> ---
> ofproto/ofproto-dpif-xlate.c | 37 +++++++++++++++++++++++++++++++-----
> tests/ofproto-dpif.at | 6 +++---
> 2 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a9cf3cbee..34b4b4018 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7078,6 +7078,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> break;
>
> case OFPACT_SET_VLAN_VID:
> + ctx->mirrors = 0;
The pattern of adding some housekeeping to many actions to support a
specific case reminds me of MPLS. My recollection is that although
I used such an approach when adding MPLS support it was subsequently
factored out into (what is now) recirc_for_mpls().
As I recall the main concern was about maintainability. And I think that
concern applies here. F.e. if a new action that modifies packets
is added, will ctx->mirrors = 0 get forgotten?
So I wonder if we can try to handle this a different way.
One idea I had - but have not thought through very thoroughly -
is to make use of the fact that xlate_commit_actions() is called
to append ODP actions corresponding to any packet modifications.
Such an approach may also ensure that the logic only executes
when packets are really modified - though there may be duplication if
there is a modify-output-restore-output sequence.
...
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 222415ac0..b027e8c2c 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -5349,7 +5349,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>
> flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
> AT_CHECK_UNQUOTED([tail -1 stdout], [0],
> - [Datapath actions: 3,push_vlan(vid=17,pcp=0),2
> + [Datapath actions: 3,push_vlan(vid=17,pcp=0),2,3
> ])
Test results look nice :)
...
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
