[ovs-dev] [PATCH ovn] ovn-northd: Support mixing stateless/stateful ACLs with acl-stateful-bypass.

Dumitru Ceara dceara at redhat.com
Wed Aug 19 13:18:30 UTC 2020


On 8/19/20 2:44 PM, Numan Siddique wrote:
> On Wed, Aug 19, 2020 at 5:50 PM Dumitru Ceara <dceara at redhat.com> wrote:
>>
>> On 8/17/20 5:49 PM, Numan Siddique wrote:
>>> On Mon, Aug 17, 2020 at 8:57 PM Dumitru Ceara <dceara at redhat.com> wrote:
>>>>
>>>> On 8/17/20 4:32 PM, Numan Siddique wrote:
>>>>> On Wed, Aug 12, 2020 at 5:52 PM Dumitru Ceara <dceara at redhat.com> wrote:
>>>>>>
>>>>>> A new configuration option is added to Logical_Switch:
>>>>>> other_config:acl-stateful-bypass. This optional value determines which
>>>>>> traffic should completely bypass connection tracking when ACLs are
>>>>>> processed.
>>>>>>
>>>>>> In specific scenarios CMSs can predetermine which traffic must be
>>>>>> firewalled statefully or not, e.g., UDP vs TCP. However, until now, if
>>>>>> at least one stateful ACL (allow-related) is configured on the switch,
>>>>>> all traffic gets sent to connection tracking. This induces a hit in
>>>>>> performance when forwarding packets that don't need stateful processing.
>>>>>>
>>>>>> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
>>>>>
>>>>> Hi Dumitru,
>>>>>
>>>>> Thanks for the patch. I have few comments.
>>>>>
>>>>
>>>> Hi Numan,
>>>>
>>>> Thanks for the review.
>>>>
>>>>> 1. From what I understand, the packet now is not sent to the conntrack
>>>>> in the ingress pipeline, but it
>>>>>     is still sent in the egress pipeline
>>>>>
>>>>
>>>> Actually, it shouldn't be sent to conntrack in the egress pipeline
>>>> either if it matches the acl-stateful-bypass filter.
>>>
>>> I think if the logical switch has a load balancer, then the packet
>>> will go to conntrack in the egress
>>> pipeline right ?
>>
>> If the logical switch has a load balancer then the packet will go to
>> conntrack for load balancing but if it matched the acl-stateful-bypass
>> filter it will not go to conntrack for ACLs. So the number of
>> recirculations is lowered.
> 
> Right now we send the pkt to conntrack in egress pipeline if there is
> even one LB.

Right, I was confused and I thought you meant the case when
REGBIT_CONNTRACK_NAT == 1 in S_SWITCH_OUT_STATEFUL, sorry.

> With your patch, it will be avoided if the pkt doesn't match on a load
> balancer ?
> 
We'll still have to send the packets to conntrack in the egress stage if
there are load balancers configured (REGBIT_CONNTRACK_DEFRAG == 1). This
is to avoid breaking replies to load balanced traffic.

The patch will only save the cost of conntrack for ACLs in the ingress
pipeline in case load balancers are configured.

Regards,
Dumitru

> 
>>
>>>
>>> Thanks
>>> Numan
>>>
>>>>
>>>>>    This patch breaks for the below OVN resources and ACL configuration.
>>>>>
>>>>>   ***
>>>>> ovn-nbctl ls-add sw0
>>>>> ovn-nbctl lsp-add sw0 sw0-port1
>>>>> ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:03 10.0.0.3"
>>>>>
>>>>> ovn-nbctl lr-add lr0
>>>>> ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
>>>>> ovn-nbctl lsp-add sw0 sw0-lr0
>>>>> ovn-nbctl lsp-set-type sw0-lr0 router
>>>>> ovn-nbctl lsp-set-addresses sw0-lr0 router
>>>>> ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0
>>>>>
>>>>> ovn-nbctl ls-add public
>>>>> ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.16.0.100/24
>>>>> ovn-nbctl lsp-add public public-lr0
>>>>> ovn-nbctl lsp-set-type public-lr0 router
>>>>> ovn-nbctl lsp-set-addresses public-lr0 router
>>>>> ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
>>>>>
>>>>> # localnet port
>>>>> ovn-nbctl lsp-add public ln-public
>>>>> ovn-nbctl lsp-set-type ln-public localnet
>>>>> ovn-nbctl lsp-set-addresses ln-public unknown
>>>>> ovn-nbctl lsp-set-options ln-public network_name=public
>>>>>
>>>>> # schedule the gw router port to a chassis.
>>>>> ovn-nbctl lrp-set-gateway-chassis lr0-public ovn-gw-1 20
>>>>>
>>>>> # Create NAT entries
>>>>> ovn-nbctl lr-nat-add lr0 snat 172.16.0.100 10.0.0.0/24
>>>>> ovn-nbctl lr-nat-add lr0 dnat_and_snat 172.16.0.110 10.0.0.3 sw0-port1
>>>>> 30:54:00:00:00:03
>>>>>
>>>>> ovn-nbctl pg-add pg0 sw0-port1
>>>>> ovn-nbctl pg-add pg0_drop sw0-port1
>>>>>
>>>>> ovn-nbctl acl-add pg0_drop from-lport 1001 "inport == @pg0_drop && ip" drop
>>>>> ovn-nbctl acl-add pg0_drop to-lport 1001 "outport == @pg0_drop && ip" drop
>>>>>
>>>>> ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && tcp.dst
>>>>> == 80" allow-related
>>>>> ovn-nbctl set logical_switch sw0 other_config:acl-stateful-bypass="tcp"
>>>>> *****
>>>>>
>>>>> Start a nc server on sw0-port1 and connect to the sw0-port1 nc server
>>>>> from outside (i.e the nc client to 172.16.0.110 (North/South
>>>>> traffic)).
>>>>>
>>>>> Without this patch, it works, but with this patch, the reply gets
>>>>> dropped since the packet is not sent to the conntrack now.
>>>>>
>>>>
>>>> I think this needs more documentation on my side. I might be wrong but I
>>>> think we discussed this during the OVN community meeting with Han when I
>>>> first suggested this approach: if the match of an allow-related ACL is a
>>>> superset of the acl-stateful-bypass match then the ACL the ACL is
>>>> implicitly converted to an "allow" ACL.
>>>>
>>>> The CMS should avoid such configurations. I think this is unfortunately
>>>> the only way to go because the packets go to conntrack before the
>>>> IN/OUT_ACL table. in the PRE_STATEFUL stage.
>>>>
>>>> In your case the SYN packet matches the ACL and is allowed while for the
>>>> SYN-ACK packet there's no matching rule except for the drop rule.
>>>
>>> It would be great to add more documentation on it.
>>>
>>
>> Sure, will do.
>>
>>>>
>>>> The configuration that would make this work is:
>>>> ovn-nbctl set logical_switch sw0 other_config:acl-stateful-bypass="tcp"
>>>> ovn-nbctl acl-add pg0_drop from-lport 1001 "inport == @pg0_drop && ip" drop
>>>> ovn-nbctl acl-add pg0_drop to-lport 1001 "outport == @pg0_drop && ip" drop
>>>> ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && tcp.dst
>>>> == 80" allow
>>>> ovn-nbctl acl-add pg0 to-lport 1002 "inport == @pg0 && ip4 && tcp.src ==
>>>> 80" allow
>>>>
>>>> If there would be another allow-related ACL (e.g. for UDP):
>>>> ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && udp.dst
>>>> == 4242" allow-related
>>>>
>>>> Without this patch the two "allow" rules for TCP would implicitly get
>>>> transformed to "allow-related" because all packets would go to conntrack
>>>> and all ACLs are considered stateful.
>>>>
>>>> With this patch, TCP can be processed without any conntrack
>>>> recirculation while for UDP we'd be using conntrack.
>>>>
>>>>>
>>>>> 2. After this patch we see the below lflow for each ACL.
>>>>>
>>>>>     ****
>>>>>     table=4 (ls_out_acl         ), priority=2002 , match=((reg0[7] ==
>>>>> 1 || !ct.trk || (!ct.new && ct.est && !ct.rpl && ct_label.blocked ==
>>>>> 0)) && (ACL MATCH)), action=(next;)
>>>>>     ****
>>>>>
>>>>>     The above flow (with the match - "reg0[7] == 1 " will result in
>>>>> one extra Open vSwitch flow for every ACL.
>>>>>
>>>>>      I am not sure if this can be avoided, but if we can find a way to
>>>>> avoid it would be great. Ignore this comment if this is not possible.
>>>>>
>>>>
>>>> I don't think it can be avoided because we'd have to parse the match
>>>> expression of the ACL to determine if the "acl-stateful-bypass" is a
>>>> subexpression of it.
>>>
>>> My initial thought was to add a high prio (may be 65535) flow in n_acl/out_acl
>>> stage with the action - next if reg0[7] ==1 because we don't need to send
>>> the packet to conntrack. But then realized that we need to handle drop ACLs.
>>>
>>>>
>>>>>
>>>>> 3. The CMS can set any match in the option - "other_config:acl-stateful-bypass".
>>>>>       For example:
>>>>>      ovn-nbctl set logical_switch sw0
>>>>> other_config:acl-stateful-bypass="tcp && tcp.dst >= 1000 && tcp.dst <=
>>>>> 1005"
>>>>>
>>>>>      Although it works, it seems a bit odd to me that a config option
>>>>> can be a match.
>>>>>      Can't we instead add another ACL action ? Maybe "allow-stateless"
>>>>> ? I am not sure if this is feasible, but just a thought.
>>>>>
>>>>
>>>> We could do that but then these "allow-stateless" ACLs would be
>>>> translated to flows in the pre-acl table like the patch does for
>>>> "acl-stateful-bypass" now. So some of the ACLs will generate flows in
>>>> the IN/OUT_ACL table while others will generate flows in the PRE_ACL
>>>> table. Is that acceptable?
>>>
>>> I think it should be fine. But I won't press too hard on it.
>>>
>>
>> Wouldn't it make sense to have a separate table in the NB schema then?
>> Something like "Stateless_Filter"; this would generate the logical flows
>> in the PRE_ACL table.
> 
> Sounds good to me. With this approach, CMS can define multiple such filters.
> 
> Thanks
> Numan
> 
>>
>> Thanks,
>> Dumitru
>>
>>>>
>>>>> 4. A Small minor comment. In the lflow-list, I see below flows. Can
>>>>> you please correct the indentations and spacing.
>>>>>     ********
>>>>>     table=6 (ls_in_acl          ), priority=65535, match=(reg0[7] ==
>>>>> 0&& (ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1))),
>>>>> action=(drop;)
>>>>>   table=6 (ls_in_acl          ), priority=65535, match=(reg0[7]== 0 &&
>>>>> !ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0),
>>>>> action=(next;)
>>>>>   table=6 (ls_in_acl          ), priority=65535, match=(reg0[7]== 0 &&
>>>>> ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked
>>>>> == 0), action=(next;)
>>>>>     ********
>>>>>
>>>>>     Notice - reg0[7] == 0&&... and "reg0[7]== 0 && ....
>>>>
>>>> Thanks, noted, I'll fix it in the next iteration.
>>>
>>> Thanks
>>> Numan
>>>
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
> 



More information about the dev mailing list