On 8/17/20 5:49 PM, Numan Siddique wrote: > On Mon, Aug 17, 2020 at 8:57 PM Dumitru Ceara <dce...@redhat.com> wrote: >> >> On 8/17/20 4:32 PM, Numan Siddique wrote: >>> On Wed, Aug 12, 2020 at 5:52 PM Dumitru Ceara <dce...@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 <dce...@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. > > 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. 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
