[ovs-dev] [PATCH v2] ofproto-dpif-trace: Improve NAT tracing.

[Ben Pfaff]

On Wed, Jun 17, 2020 at 09:21:57AM +0200, Dumitru Ceara wrote:
> On 6/17/20 12:07 AM, Ben Pfaff wrote:
> > On Fri, Jan 10, 2020 at 10:34:43AM +0100, Dumitru Ceara wrote:
> >> When ofproto/trace detects a recirc action it resumes execution at the
> >> specified next table. However, if the ct action performs SNAT/DNAT,
> >> e.g., ct(commit,nat(src=1.1.1.1:4000),table=42), the src/dst IPs and
> >> ports in the oftrace_recirc_node->flow field are not updated. This leads
> >> to misleading outputs from ofproto/trace as real packets would actually
> >> first get NATed and might match different flows when recirculated.
> >>
> >> Assume the first IP/port from the NAT src/dst action will be used by
> >> conntrack for the translation and update the oftrace_recirc_node->flow
> >> accordingly. This is not entirely correct as conntrack might choose a
> >> different IP/port but the result is more realistic than before.
> >>
> >> This fix covers new connections. However, for reply traffic that executes
> >> actions of the form ct(nat, table=42) we still don't update the flow as
> >> we don't have any information about conntrack state when tracing.
> >>
> >> Also move the oftrace_recirc_node processing out of ofproto_trace()
> >> and to its own function, ofproto_trace_recirc_node() for better
> >> readability/
> >>
> >> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> > 
> > There are ways to make this better, but it's a good improvement already!
> > 
> > I applied this to master.
> > 
> > I folded in the following minor changes:
> > 
> > diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> > index 5f1a05c3a6cc..78a54c715dc7 100644
> > --- a/ofproto/ofproto-dpif-trace.c
> > +++ b/ofproto/ofproto-dpif-trace.c
> > @@ -693,15 +693,14 @@ ofproto_trace_recirc_node(struct oftrace_recirc_node *node,
> >          const struct ofpact_nat *ofn = node->nat_act;
> >  
> >          ds_put_cstr(output, "Replacing src/dst IP/ports to simulate NAT:\n");
> > -        ds_put_cstr(output, " Initial  flow: ");
> > +        ds_put_cstr(output, " Initial flow: ");
> >          oftrace_print_ip_flow(&node->flow, ofn->range_af, output);
> >  
> >          if (ofn->flags & NX_NAT_F_SRC) {
> >              if (ofn->range_af == AF_INET) {
> >                  node->flow.nw_src = ofn->range.addr.ipv4.min;
> >              } else if (ofn->range_af == AF_INET6) {
> > -                memcpy(&node->flow.ipv6_src, &ofn->range.addr.ipv6.min,
> > -                       sizeof node->flow.ipv6_src);
> > +                node->flow.ipv6_src = ofn->range.addr.ipv6.min;
> >              }
> >  
> >              if (ofn->range_af != AF_UNSPEC && ofn->range.proto.min) {
> > @@ -712,8 +711,7 @@ ofproto_trace_recirc_node(struct oftrace_recirc_node *node,
> >              if (ofn->range_af == AF_INET) {
> >                  node->flow.nw_dst = ofn->range.addr.ipv4.min;
> >              } else if (ofn->range_af == AF_INET6) {
> > -                memcpy(&node->flow.ipv6_dst, &ofn->range.addr.ipv6.min,
> > -                       sizeof node->flow.ipv6_dst);
> > +                node->flow.ipv6_dst = ofn->range.addr.ipv6.min;
> >              }
> >  
> >              if (ofn->range_af != AF_UNSPEC && ofn->range.proto.min) {
> > 
> > Thanks,
> > 
> > Ben.
> > 
> 
> Thanks Ben for following up on this! Looks good to me.
> 
> You mentioned there are ways to make this better, I can work on follow
> up patches if you already have suggestions.

I've forgotten what I had in mind!  I do remember discussing this
before; maybe some of my suggestions from then would still be
applicable.