[ovs-dev,ovn] ovn-northd: Add lflows to by pass the svc monitor packets from conntrack.

From: Numan Siddique <numans@ovn.org>

The commit [1] added lflows to by pass the service monitor health check
packets from conntrack. But it missed out adding in the ingress pre_acl
and egress pre_acl of logical switch pipeline.

This patch adds these missing lflows. It also enhanced the system lb health
check tests to add the acls to test this scenario.

[1] - bb9f2b9ce56c("ovn-northd: Consider load balancer active backends in router pipeline)
Fixes: bb9f2b9ce56c("ovn-northd: Consider load balancer active backends in router pipeline)

Reported-by: Maciej Józefczyk <mjozefcz@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/ovn-northd.8.xml | 22 +++++++++++++++++++++-
 northd/ovn-northd.c     | 15 ++++++++++++++-
 tests/system-ovn.at     | 25 +++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 2 deletions(-)

On 3/11/20 7:15 PM, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> The commit [1] added lflows to by pass the service monitor health check
> packets from conntrack. But it missed out adding in the ingress pre_acl
> and egress pre_acl of logical switch pipeline.
> 
> This patch adds these missing lflows. It also enhanced the system lb health
> check tests to add the acls to test this scenario.
> 
> [1] - bb9f2b9ce56c("ovn-northd: Consider load balancer active backends in router pipeline)
> Fixes: bb9f2b9ce56c("ovn-northd: Consider load balancer active backends in router pipeline)
> 
> Reported-by: Maciej Józefczyk <mjozefcz@redhat.com>
> Signed-off-by: Numan Siddique <numans@ovn.org>

Hi Numan,

A few cosmetic issues inline. Aside from that, could we also add a unit
test in ovn.at to cover this scenario?

Thanks,
Dumitru

> ---
>  northd/ovn-northd.8.xml | 22 +++++++++++++++++++++-
>  northd/ovn-northd.c     | 15 ++++++++++++++-
>  tests/system-ovn.at     | 25 +++++++++++++++++++++++++
>  3 files changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index b6cfa3e90..9b44720d1 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -293,6 +293,16 @@
>        priority-110 flow is added to skip over stateful ACLs.
>      </p>
>  
> +    <p>
> +      This table also has a priority-110 flow with the match
> +      <code>eth.dst == <var>E</var></code> for all logical switch
> +      datapaths to move traffic to the next table. Where <var>E</var>
> +      is the service monitor mac defined in the
> +      <ref column="options:svc_monitor_mac" table="NB_Global"
> +      db="OVN_Northbound"/> colum of <ref table="NB_Global"
> +      db="OVN_Northbound"/> table.
> +    </p>
> +
>      <h3>Ingress Table 4: Pre-LB</h3>
>  
>      <p>
> @@ -320,7 +330,7 @@
>  
>      <p>
>        This table also has a priority-110 flow with the match
> -      <code>eth.src == <var>E</var></code> for all logical switch
> +      <code>eth.dst == <var>E</var></code> for all logical switch
>        datapaths to move traffic to the next table. Where <var>E</var>
>        is the service monitor mac defined in the
>        <ref column="options:svc_monitor_mac" table="NB_Global"
> @@ -1295,6 +1305,16 @@ output;
>       <code>to-lport</code> traffic.
>      </p>
>  
> +    <p>
> +      This table also has a priority-110 flow with the match
> +      <code>eth.src == <var>E</var></code> for all logical switch
> +      datapaths to move traffic to the next table. Where <var>E</var>
> +      is the service monitor mac defined in the
> +      <ref column="options:svc_monitor_mac" table="NB_Global"
> +      db="OVN_Northbound"/> colum of <ref table="NB_Global"
> +      db="OVN_Northbound"/> table.
> +    </p>
> +
>      <h3>Egress Table 2: Pre-stateful</h3>
>  
>      <p>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index d42a9892a..52dca48c6 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -4605,6 +4605,16 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
>      ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 0, "1", "next;");
>      ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 0, "1", "next;");
>  
> +    char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac);
> +    ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110, svc_check_match,
> +                  "next;");
> +    free(svc_check_match);
> +
> +    svc_check_match = xasprintf("eth.src == %s", svc_monitor_mac);
> +    ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110, svc_check_match,
> +                  "next;");
> +    free(svc_check_match);
> +
>      /* If there are any stateful ACL rules in this datapath, we must
>       * send all IP packets through the conntrack action, which handles
>       * defragmentation, in order to match L4 headers. */
> @@ -4788,9 +4798,12 @@ build_pre_lb(struct ovn_datapath *od, struct hmap *lflows,
>                    "next;");
>  
>      /* Do not send service monitor packets to conntrack. */
> -    char *svc_check_match = xasprintf("eth.src == %s", svc_monitor_mac);
> +    char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac);
>      ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_LB, 110,
>                    svc_check_match, "next;");
> +    free(svc_check_match);
> +
> +    svc_check_match = xasprintf("eth.src == %s", svc_monitor_mac);
>      ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_LB, 110,
>                    svc_check_match, "next;");
>      free(svc_check_match);
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 9ed3df754..64cb5d60f 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -3304,6 +3304,8 @@ ovs-vsctl \
>  # Start ovn-controller
>  start_daemon ovn-controller
>  
> +
> +

These two newlines above are not really needed.

>  ovn-nbctl ls-add sw0
>  
>  ovn-nbctl lsp-add sw0 sw0-p1
> @@ -3314,12 +3316,35 @@ ovn-nbctl lsp-add sw0 sw0-p2
>  ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4"
>  ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4"
>  
> +# Create port group and ACLs for sw0 ports.
> +ovn-nbctl pg-add pg0_drop sw0-p1 sw0-p2
> +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 pg-add pg0 sw0-p1 sw0-p2
> +ovn-nbctl acl-add pg0 from-lport 1002 "inport == @pg0 && ip4" allow-related
> +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src == 0.0.0.0/0 && icmp4" allow-related
> +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related
> +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src == 0.0.0.0/0 && udp && udp.dst == 80" allow-related
> +
> +

One newline should be enough to keep it in the same style as the rest of
the function.

>  # Create the second logical switch with one port
>  ovn-nbctl ls-add sw1
>  ovn-nbctl lsp-add sw1 sw1-p1
>  ovn-nbctl lsp-set-addresses sw1-p1 "40:54:00:00:00:03 20.0.0.3"
>  ovn-nbctl lsp-set-port-security sw1-p1 "40:54:00:00:00:03 20.0.0.3"
>  
> +# Create port group and ACLs for sw1 ports.
> +ovn-nbctl pg-add pg1_drop sw1-p1
> +ovn-nbctl acl-add pg1_drop from-lport 1001 "inport == @pg1_drop && ip" drop
> +ovn-nbctl acl-add pg1_drop to-lport 1001 "outport == @pg1_drop && ip" drop
> +
> +ovn-nbctl pg-add pg1 sw1-p1
> +ovn-nbctl acl-add pg1 from-lport 1002 "inport == @pg1 && ip4" allow-related
> +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && icmp4" allow-related
> +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related
> +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && udp && udp.dst == 80" allow-related
> +
>  # Create a logical router and attach both logical switches
>  ovn-nbctl lr-add lr0
>  ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
>

On Thu, Mar 12, 2020 at 2:43 PM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 3/11/20 7:15 PM, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > The commit [1] added lflows to by pass the service monitor health check
> > packets from conntrack. But it missed out adding in the ingress pre_acl
> > and egress pre_acl of logical switch pipeline.
> >
> > This patch adds these missing lflows. It also enhanced the system lb health
> > check tests to add the acls to test this scenario.
> >
> > [1] - bb9f2b9ce56c("ovn-northd: Consider load balancer active backends in router pipeline)
> > Fixes: bb9f2b9ce56c("ovn-northd: Consider load balancer active backends in router pipeline)
> >
> > Reported-by: Maciej Józefczyk <mjozefcz@redhat.com>
> > Signed-off-by: Numan Siddique <numans@ovn.org>
>
> Hi Numan,
>
> A few cosmetic issues inline. Aside from that, could we also add a unit
> test in ovn.at to cover this scenario?
>

Thanks for the review. I addressed the comments and submitted the v2.

Numan

> Thanks,
> Dumitru
>
> > ---
> >  northd/ovn-northd.8.xml | 22 +++++++++++++++++++++-
> >  northd/ovn-northd.c     | 15 ++++++++++++++-
> >  tests/system-ovn.at     | 25 +++++++++++++++++++++++++
> >  3 files changed, 60 insertions(+), 2 deletions(-)
> >
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index b6cfa3e90..9b44720d1 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -293,6 +293,16 @@
> >        priority-110 flow is added to skip over stateful ACLs.
> >      </p>
> >
> > +    <p>
> > +      This table also has a priority-110 flow with the match
> > +      <code>eth.dst == <var>E</var></code> for all logical switch
> > +      datapaths to move traffic to the next table. Where <var>E</var>
> > +      is the service monitor mac defined in the
> > +      <ref column="options:svc_monitor_mac" table="NB_Global"
> > +      db="OVN_Northbound"/> colum of <ref table="NB_Global"
> > +      db="OVN_Northbound"/> table.
> > +    </p>
> > +
> >      <h3>Ingress Table 4: Pre-LB</h3>
> >
> >      <p>
> > @@ -320,7 +330,7 @@
> >
> >      <p>
> >        This table also has a priority-110 flow with the match
> > -      <code>eth.src == <var>E</var></code> for all logical switch
> > +      <code>eth.dst == <var>E</var></code> for all logical switch
> >        datapaths to move traffic to the next table. Where <var>E</var>
> >        is the service monitor mac defined in the
> >        <ref column="options:svc_monitor_mac" table="NB_Global"
> > @@ -1295,6 +1305,16 @@ output;
> >       <code>to-lport</code> traffic.
> >      </p>
> >
> > +    <p>
> > +      This table also has a priority-110 flow with the match
> > +      <code>eth.src == <var>E</var></code> for all logical switch
> > +      datapaths to move traffic to the next table. Where <var>E</var>
> > +      is the service monitor mac defined in the
> > +      <ref column="options:svc_monitor_mac" table="NB_Global"
> > +      db="OVN_Northbound"/> colum of <ref table="NB_Global"
> > +      db="OVN_Northbound"/> table.
> > +    </p>
> > +
> >      <h3>Egress Table 2: Pre-stateful</h3>
> >
> >      <p>
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index d42a9892a..52dca48c6 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -4605,6 +4605,16 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
> >      ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 0, "1", "next;");
> >      ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 0, "1", "next;");
> >
> > +    char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac);
> > +    ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110, svc_check_match,
> > +                  "next;");
> > +    free(svc_check_match);
> > +
> > +    svc_check_match = xasprintf("eth.src == %s", svc_monitor_mac);
> > +    ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110, svc_check_match,
> > +                  "next;");
> > +    free(svc_check_match);
> > +
> >      /* If there are any stateful ACL rules in this datapath, we must
> >       * send all IP packets through the conntrack action, which handles
> >       * defragmentation, in order to match L4 headers. */
> > @@ -4788,9 +4798,12 @@ build_pre_lb(struct ovn_datapath *od, struct hmap *lflows,
> >                    "next;");
> >
> >      /* Do not send service monitor packets to conntrack. */
> > -    char *svc_check_match = xasprintf("eth.src == %s", svc_monitor_mac);
> > +    char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac);
> >      ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_LB, 110,
> >                    svc_check_match, "next;");
> > +    free(svc_check_match);
> > +
> > +    svc_check_match = xasprintf("eth.src == %s", svc_monitor_mac);
> >      ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_LB, 110,
> >                    svc_check_match, "next;");
> >      free(svc_check_match);
> > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > index 9ed3df754..64cb5d60f 100644
> > --- a/tests/system-ovn.at
> > +++ b/tests/system-ovn.at
> > @@ -3304,6 +3304,8 @@ ovs-vsctl \
> >  # Start ovn-controller
> >  start_daemon ovn-controller
> >
> > +
> > +
>
> These two newlines above are not really needed.
>
> >  ovn-nbctl ls-add sw0
> >
> >  ovn-nbctl lsp-add sw0 sw0-p1
> > @@ -3314,12 +3316,35 @@ ovn-nbctl lsp-add sw0 sw0-p2
> >  ovn-nbctl lsp-set-addresses sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> >  ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4"
> >
> > +# Create port group and ACLs for sw0 ports.
> > +ovn-nbctl pg-add pg0_drop sw0-p1 sw0-p2
> > +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 pg-add pg0 sw0-p1 sw0-p2
> > +ovn-nbctl acl-add pg0 from-lport 1002 "inport == @pg0 && ip4" allow-related
> > +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src == 0.0.0.0/0 && icmp4" allow-related
> > +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related
> > +ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && ip4.src == 0.0.0.0/0 && udp && udp.dst == 80" allow-related
> > +
> > +
>
> One newline should be enough to keep it in the same style as the rest of
> the function.
>
> >  # Create the second logical switch with one port
> >  ovn-nbctl ls-add sw1
> >  ovn-nbctl lsp-add sw1 sw1-p1
> >  ovn-nbctl lsp-set-addresses sw1-p1 "40:54:00:00:00:03 20.0.0.3"
> >  ovn-nbctl lsp-set-port-security sw1-p1 "40:54:00:00:00:03 20.0.0.3"
> >
> > +# Create port group and ACLs for sw1 ports.
> > +ovn-nbctl pg-add pg1_drop sw1-p1
> > +ovn-nbctl acl-add pg1_drop from-lport 1001 "inport == @pg1_drop && ip" drop
> > +ovn-nbctl acl-add pg1_drop to-lport 1001 "outport == @pg1_drop && ip" drop
> > +
> > +ovn-nbctl pg-add pg1 sw1-p1
> > +ovn-nbctl acl-add pg1 from-lport 1002 "inport == @pg1 && ip4" allow-related
> > +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && icmp4" allow-related
> > +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && tcp && tcp.dst == 80" allow-related
> > +ovn-nbctl acl-add pg1 to-lport 1002 "outport == @pg1 && ip4 && ip4.src == 0.0.0.0/0 && udp && udp.dst == 80" allow-related
> > +
> >  # Create a logical router and attach both logical switches
> >  ovn-nbctl lr-add lr0
> >  ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev