Skip to content

Commit

Permalink
ovn: Exclude inport and outport symbol tables from conjunction
Browse files Browse the repository at this point in the history
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 298701d)

Signed-off-by: Ben Pfaff <blp@ovn.org>
  • Loading branch information
numansiddique authored and blp committed Sep 25, 2019
1 parent e6aebc9 commit 1f82d15
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 1 deletion.
2 changes: 1 addition & 1 deletion ovn/lib/expr.c
Expand Up @@ -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;
Expand Down
26 changes: 26 additions & 0 deletions tests/ovn.at
Expand Up @@ -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)
])
Expand Down Expand Up @@ -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)
])
Expand Down

0 comments on commit 1f82d15

Please sign in to comment.