On Tue, Feb 21, 2023 at 10:11:27AM -0500, Mike Pattrick wrote: > On Tue, Feb 21, 2023 at 5:35 AM Simon Horman <simon.hor...@corigine.com> > wrote: > > > > 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? > > That's a good point. I can wrap this in a switch statement, and will > investigate if xlate_commit_actions is an appropriate location.
Thanks. Just to clarify: my suggestion regarding xlate_commit_actions() was not just that it might be a good location. But also that you may be able to get it, or something it calls, to tell you if any packet modifications were made. > > > Thanks > M > > > > > 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
