diff mbox series

[ovs-dev,branch,2.12] ovn: Exclude inport and outport symbol tables from conjunction

Message ID 20190914074917.20927-1-nusiddiq@redhat.com
State Superseded
Headers show
Series [ovs-dev,branch,2.12] ovn: Exclude inport and outport symbol tables from conjunction | expand

Commit Message

Numan Siddique Sept. 14, 2019, 7:49 a.m. UTC 
From: Numan Siddique <nusiddiq@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

Acked-by: Mark Michelson <mmichels@redhat.com>
Acked-by: Daniel Alvarez <dalvarez@redhat.com>
Signed-off-by: Numan Siddique <nusiddiq@redhat.com>

(cherry-picked from ovn commit 298701dbc99645700be41680a43d049cb061847a)
---
 ovn/lib/expr.c |  2 +-
 tests/ovn.at   | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

Comments

0-day Robot Sept. 14, 2019, 7:56 a.m. UTC | #1 
Bleep bloop.  Greetings Numan Siddique, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
Failed to merge in the changes.
Patch failed at 0001 ovn: Exclude inport and outport symbol tables from conjunction
The copy of the patch that failed is found in:
   /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Ben Pfaff Sept. 25, 2019, 5:05 p.m. UTC | #2 
On Sat, Sep 14, 2019 at 01:19:17PM +0530, nusiddiq@redhat.com wrote:
> From: Numan Siddique <nusiddiq@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.

Thanks, applied to branch-2.12.
diff mbox series

Patch

diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index e4c650f7c..c0871e1e8 100644
--- a/ovn/lib/expr.c
+++ b/ovn/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 2361524ff..54aa19bb2 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -573,6 +573,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)
 ])
@@ -677,6 +695,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)
 ])