[ovs-dev] [PATCH ovn] Exclude inport and outport symbol tables from conjunction

Daniel Alvarez Sanchez dalvarez at redhat.com
Fri Sep 13 21:10:59 UTC 2019


Acked-by: Daniel Alvarez <dalvarez at redhat.com>


On Fri, Sep 13, 2019 at 11:02 PM Mark Michelson <mmichels at redhat.com> wrote:
>
> Acked-by: Mark Michelson
>
> It sucks that we lose the efficiency of the conjunctive match altogether
> on port groups because of this error, but I understand this is a huge
> bug and needs fixing.
If I'm not mistaken, from OpenStack standpoint conjunction was *only*
being used when using port groups and ACLs that matched on port ranges
( e.g tcp.dst >= X && tcp.dst <=Y) which was not working. Therefore
we're not losing performance because it was already broken (given that
there was more than one ACL like that).

>
> Perhaps it would be good to start up a discussion on this list about a
> more longterm solution that would allow for conjunctive matches with no
> ambiguity.
Agreed! We already discussed some ideas on IRC but it'd be awesome to
have a thread and brainstorm there.

Thanks a lot everyone!
Daniel
>
> On 9/13/19 4:49 PM, nusiddiq at redhat.com wrote:
> > From: Numan Siddique <nusiddiq at redhat.com>
> >
> > If there are multiple ACLs associated with a port group and they
> > match on a range of some field, then ovn-controller doesn't install
> > the flows properly and this results in broken ACL functionality.
> >
> > For example, if there is a port group - pg1 with logical ports - [p1, p2]
> > and if there are below ACLs (only match condition is shown)
> >
> > 1 -  outport == @pg1 && ip4 && tcp.dst >= 500 && tcp.dst <= 501
> > 2 -  outport == @pg1 && ip4 && tcp.dst >= 600 && tcp.dst <= 601
> >
> > The first ACL will result in the below OF flows
> >
> > 1.  conj_id=1,tcp
> > 2.  tcp,reg15=0x11: conjunction(1, 1/2)
> > 3.  tcp,reg15=0x12: conjunction(1, 1/2)
> > 5.  tcp,tp_dst=500: conjunction(1, 2/2)
> > 6.  tcp,tp_dst=501: conjunction(1, 2/2)
> >
> > The second ACL will result in the below OF flows
> > 7.  conj_id=2,tcp
> > 8.  tcp,reg15=0x11: conjunction(2, 1/2)
> > 9.  tcp,reg15=0x12: conjunction(2, 1/2)
> > 11. tcp,tp_dst=600: conjunction(2, 2/2)
> > 12. tcp,tp_dst=601: conjunction(2, 3/2)
> >
> > The OF flows (2) and (8) have the exact match but with different action.
> > This results in only one of the flows getting installed. The same goes
> > for the flows (3) and (9). And this completely breaks the ACL functionality
> > for such scenarios.
> >
> > In order to fix this issue, this patch excludes the 'inport' and 'outport' symbols
> > from conjunction. With this patch we will have the below flows.
> >
> > tcp,reg15=0x11,tp_dst=500
> > tcp,reg15=0x11,tp_dst=501
> > tcp,reg15=0x12,tp_dst=500
> > tcp,reg15=0x12,tp_dst=501
> > tcp,reg15=0x13,tp_dst=500
> > tcp,reg15=0x13,tp_dst=501
> > tcp,reg15=0x11,tp_dst=600
> > tcp,reg15=0x11,tp_dst=601
> > tcp,reg15=0x12,tp_dst=600
> > tcp,reg15=0x12,tp_dst=601
> > tcp,reg15=0x13,tp_dst=600
> > tcp,reg15=0x13,tp_dst=601
> >
> > Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> > ---
> >   lib/expr.c   |  2 +-
> >   tests/ovn.at | 26 ++++++++++++++++++++++++++
> >   2 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/expr.c b/lib/expr.c
> > index e4c650f7c..c0871e1e8 100644
> > --- a/lib/expr.c
> > +++ b/lib/expr.c
> > @@ -1499,7 +1499,7 @@ expr_symtab_add_string(struct shash *symtab, const char *name,
> >       const struct mf_field *field = mf_from_id(id);
> >       struct expr_symbol *symbol;
> >
> > -    symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, false,
> > +    symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, true,
> >                           field->writable);
> >       symbol->field = field;
> >       return symbol;
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 2a35b4e15..14d9f59b0 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -589,6 +589,24 @@ ip,reg14=0x6
> >   ipv6,reg14=0x5
> >   ipv6,reg14=0x6
> >   ])
> > +AT_CHECK([expr_to_flow 'inport == {"eth0", "eth1", "eth2"} && ip4 && tcp && tcp.dst == {500, 501}'], [0], [dnl
> > +tcp,reg14=0x5,tp_dst=500
> > +tcp,reg14=0x5,tp_dst=501
> > +tcp,reg14=0x6,tp_dst=500
> > +tcp,reg14=0x6,tp_dst=501
> > +])
> > +AT_CHECK([expr_to_flow 'outport == {"eth0", "eth1", "eth2"} && ip4 && tcp && tcp.src == {400, 401} && tcp.dst == {500, 501}'], [0], [dnl
> > +conj_id=1,tcp,reg15=0x5
> > +conj_id=2,tcp,reg15=0x6
> > +tcp,reg15=0x5,tp_dst=500: conjunction(1, 0/2)
> > +tcp,reg15=0x5,tp_dst=501: conjunction(1, 0/2)
> > +tcp,reg15=0x5,tp_src=400: conjunction(1, 1/2)
> > +tcp,reg15=0x5,tp_src=401: conjunction(1, 1/2)
> > +tcp,reg15=0x6,tp_dst=500: conjunction(2, 0/2)
> > +tcp,reg15=0x6,tp_dst=501: conjunction(2, 0/2)
> > +tcp,reg15=0x6,tp_src=400: conjunction(2, 1/2)
> > +tcp,reg15=0x6,tp_src=401: conjunction(2, 1/2)
> > +])
> >   AT_CHECK([expr_to_flow 'inport == "eth0" && inport == "eth1"'], [0], [dnl
> >   (no flows)
> >   ])
> > @@ -693,6 +711,14 @@ reg15=0x11
> >   reg15=0x12
> >   reg15=0x13
> >   ])
> > +AT_CHECK([expr_to_flow 'outport == @pg1 && ip4.src == {10.0.0.4, 10.0.0.5}'], [0], [dnl
> > +ip,reg15=0x11,nw_src=10.0.0.4
> > +ip,reg15=0x11,nw_src=10.0.0.5
> > +ip,reg15=0x12,nw_src=10.0.0.4
> > +ip,reg15=0x12,nw_src=10.0.0.5
> > +ip,reg15=0x13,nw_src=10.0.0.4
> > +ip,reg15=0x13,nw_src=10.0.0.5
> > +])
> >   AT_CHECK([expr_to_flow 'outport == {@pg_empty}'], [0], [dnl
> >   (no flows)
> >   ])
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list