Re: [ovs-dev] [PATCH v1 0/3] Policy-based routing
Hi Mark Thank for the review. 1. Why not use the ACL table in the logical switch? ACLs in logical switch support permit/deny rules but not reroute. By adding PBR on the router, it is possible to reroute traffic to an endpoint (maybe a firewall or VPN device) on a different subnet. Multiple logical switches can use the same instance of the firewall without the firewall being part of each of the logical switches. For example: Drop all traffic between 10.1.1.0/24 to 12.1.1.0/24 with the below exception: Reroute all traffic from 10.1.1.20 to 12.1.1.20 via 15.1.1.5 (firewall) Can be written as: Priority: 12 match: "inport == lrp-of-15.1.1.1-logical-switch ip4.src == 10.1.1.20 ip4.dst == 12.1.1.20" permit Priority: 11 match: "ip4.src == 10.1.1.20 ip4.dst == 12.1.1.20" reroute 15.1.1.5 Priority: 10 match: "ip4.src == 10.1.1.0/24 ip4.dst == 12.1.1.0/24" drop Having network security at the router level is especially useful when we do multi-tier routing. The top-tier can add security policies while routing traffic between the bottom tier routers. 2. Why a new table in Logical router? The new table overrides the routing decision made by IP_Routing. Hence it needed to be after IP_Routing. It needs to be before ARP so the nexthop could get resolved. 3. Why only ingress? Is there a use-case where we need to add permit/deny rules in the egress pipeline of the router? 4. Stateful policies and Logging - Both Stateful policies and Logging are good features to have and we could enhance PBR policies to support these. Mary On 10/24/18, 8:09 AM, "Mark Michelson" wrote: Hi Mary, thanks for the patchset. At the most basic level, it looks like the new Logical_Router_Policy table is nearly the same as the current ACL table. The differences are: * ACL has a "direction" column * ACL has a "log" column * ACL has an "allow-related" action * Logical_Router_Policy has a "name" column * Logical_Router_Policy has a "nexthop" column * Logical_Router_Policy has a "reroute" action Seeing this makes me wonder why the approach was to create a new table instead of making modifications to the ACL table. Can you share the thought process that led to creating a new table? My thoughts on the matter are that ACLs are well established in OVN and reusing them offers some nice benefits. Seeing the differences also makes me wonder why logical router policies only apply to ingress traffic. Is there a reason why we can't specify a direction like we do with logical switch ACLs? And finally, the logging in ACLs is a nice feature and should also be in router policies. On 10/22/2018 06:24 PM, Mary Manohar wrote: > This patch series implements policy-based routing. > Policy-based routing (PBR) provides a mechanism to configure permit/deny and reroute policies on the router. > Permit/deny policies are similar to OVN ACLs, but exist on the logical-router. > Reroute policies are needed for service-insertion and service-chaining. > Currently, we support only stateless policies. > > To achieve this, we introduced a new table in the ingress pipeline of the Logical-router. > The new table is between the ‘IP Routing’ and the ‘ARP/ND resolution’ table. > This way, PBR can override routing decisions and provide a different next-hop. > > Mary Manohar (3): >[1/3]: Routing policies, add config in schema >[2/3] Routing policies, add routing-policies in ovn-nbctl >[3/3]: Routing policies, ovn-northd changes to handle routing policy > commands. > > ovn/northd/ovn-northd.c | 144 -- > ovn/ovn-nb.ovsschema | 20 - > ovn/ovn-nb.xml| 63 +++ > ovn/utilities/ovn-nbctl.c | 196 ++ > tests/ovn-nbctl.at| 47 +++ > 5 files changed, 463 insertions(+), 7 deletions(-) > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 0/3] Policy-based routing
Hi Mary, thanks for the patchset. At the most basic level, it looks like the new Logical_Router_Policy table is nearly the same as the current ACL table. The differences are: * ACL has a "direction" column * ACL has a "log" column * ACL has an "allow-related" action * Logical_Router_Policy has a "name" column * Logical_Router_Policy has a "nexthop" column * Logical_Router_Policy has a "reroute" action Seeing this makes me wonder why the approach was to create a new table instead of making modifications to the ACL table. Can you share the thought process that led to creating a new table? My thoughts on the matter are that ACLs are well established in OVN and reusing them offers some nice benefits. Seeing the differences also makes me wonder why logical router policies only apply to ingress traffic. Is there a reason why we can't specify a direction like we do with logical switch ACLs? And finally, the logging in ACLs is a nice feature and should also be in router policies. On 10/22/2018 06:24 PM, Mary Manohar wrote: This patch series implements policy-based routing. Policy-based routing (PBR) provides a mechanism to configure permit/deny and reroute policies on the router. Permit/deny policies are similar to OVN ACLs, but exist on the logical-router. Reroute policies are needed for service-insertion and service-chaining. Currently, we support only stateless policies. To achieve this, we introduced a new table in the ingress pipeline of the Logical-router. The new table is between the ‘IP Routing’ and the ‘ARP/ND resolution’ table. This way, PBR can override routing decisions and provide a different next-hop. Mary Manohar (3): [1/3]: Routing policies, add config in schema [2/3] Routing policies, add routing-policies in ovn-nbctl [3/3]: Routing policies, ovn-northd changes to handle routing policy commands. ovn/northd/ovn-northd.c | 144 -- ovn/ovn-nb.ovsschema | 20 - ovn/ovn-nb.xml| 63 +++ ovn/utilities/ovn-nbctl.c | 196 ++ tests/ovn-nbctl.at| 47 +++ 5 files changed, 463 insertions(+), 7 deletions(-) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 0/3] Policy-based routing
On Mon, Oct 22, 2018 at 10:24:03PM +, Mary Manohar wrote: > This patch series implements policy-based routing. > Policy-based routing (PBR) provides a mechanism to configure permit/deny and > reroute policies on the router. > Permit/deny policies are similar to OVN ACLs, but exist on the logical-router. > Reroute policies are needed for service-insertion and service-chaining. > Currently, we support only stateless policies. > > To achieve this, we introduced a new table in the ingress pipeline of the > Logical-router. > The new table is between the ‘IP Routing’ and the ‘ARP/ND resolution’ table. > This way, PBR can override routing decisions and provide a different next-hop. None of these patches have sign-offs. As Numan says, they also have poor commit messages. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 0/3] Policy-based routing
On Tue, Oct 23, 2018 at 3:54 AM Mary Manohar wrote: > This patch series implements policy-based routing. > Policy-based routing (PBR) provides a mechanism to configure permit/deny > and reroute policies on the router. > Permit/deny policies are similar to OVN ACLs, but exist on the > logical-router. > Reroute policies are needed for service-insertion and service-chaining. > Currently, we support only stateless policies. > > To achieve this, we introduced a new table in the ingress pipeline of the > Logical-router. > The new table is between the ‘IP Routing’ and the ‘ARP/ND resolution’ > table. > This way, PBR can override routing decisions and provide a different > next-hop. > > Mary Manohar (3): > [1/3]: Routing policies, add config in schema > [2/3] Routing policies, add routing-policies in ovn-nbctl > [3/3]: Routing policies, ovn-northd changes to handle routing policy > commands. > > ovn/northd/ovn-northd.c | 144 -- > ovn/ovn-nb.ovsschema | 20 - > ovn/ovn-nb.xml| 63 +++ > ovn/utilities/ovn-nbctl.c | 196 > ++ > tests/ovn-nbctl.at| 47 +++ > 5 files changed, 463 insertions(+), 7 deletions(-) > Hi, I didn't get a chance to look into this yet, but It would be helpful if the commit messages are a bit more detailed. Perhaps the details in the patch 0 can go in the commit messages. I see a white space warning in patch 2. .git/rebase-apply/patch:56: trailing whitespace. /* Check if same routing policy already exists. Thanks Numan > -- > 1.8.3.1 > > ___ > 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
