On Sat, Sep 14, 2019 at 2:41 AM Daniel Alvarez Sanchez <dalva...@redhat.com>
wrote:
> Acked-by: Daniel Alvarez <dalva...@redhat.com>
>
>
> On Fri, Sep 13, 2019 at 11:02 PM Mark Michelson <mmich...@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 for the reviews. I applied this to master.
Agree we can discuss it further and come up with ideas.
I know Dumitru has some idea to make use of conjunctions for port groups.
CC'ing Han if he has any comments on ideas.
Thanks
Numan
> Thanks a lot everyone!
> Daniel
> >
> > On 9/13/19 4:49 PM, nusid...@redhat.com wrote:
> > > From: Numan Siddique <nusid...@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 <nusid...@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
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
