[ovs-dev,branch-21.06] northd: support HW VTEP with stateful datapath

A packet going from HW VTEP device to VIF port when arrives to
hypervisor chassis should go through LS ingress pipeline to l2_lkp
stage without any match. In l2_lkp stage an output port is
determined and then packet passed to LS egress pipeline for futher
processing and to VIF port delivery.

Prior to this commit a packet, which was received from HW VTEP
device was dropped in an LS ingress datapath, where stateful services
were defined (ACLs, LBs).

To fix this issue we add a special flag-bit which can be used in LS
pipelines, to check whether the packet came from HW VTEP devices.
In ls_in_pre_acl and ls_in_pre_lb we add new flow with priority 110
to skip such packets.

Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
(cherry picked from commit 62ca8b9620cc1168ace6905575b7d36438363aed)
---
 northd/ovn-northd.8.xml | 28 ++++++++++++++++++++++++++++
 northd/ovn-northd.c     | 14 ++++++++++++++
 northd/ovn_northd.dl    | 33 +++++++++++++++++++++++++++++++--
 ovs                     |  2 +-
 tests/ovn-northd.at     |  2 ++
 5 files changed, 76 insertions(+), 3 deletions(-)

Bleep bloop.  Greetings Vladislav Odintsov, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Numan Siddique <numans@ovn.org>
Lines checked: 231, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

On Sat, Sep 18, 2021 at 8:51 AM Vladislav Odintsov <odivlad@gmail.com> wrote:
>
> A packet going from HW VTEP device to VIF port when arrives to
> hypervisor chassis should go through LS ingress pipeline to l2_lkp
> stage without any match. In l2_lkp stage an output port is
> determined and then packet passed to LS egress pipeline for futher
> processing and to VIF port delivery.
>
> Prior to this commit a packet, which was received from HW VTEP
> device was dropped in an LS ingress datapath, where stateful services
> were defined (ACLs, LBs).
>
> To fix this issue we add a special flag-bit which can be used in LS
> pipelines, to check whether the packet came from HW VTEP devices.
> In ls_in_pre_acl and ls_in_pre_lb we add new flow with priority 110
> to skip such packets.
>
> Signed-off-by: Vladislav Odintsov <odivlad@gmail.com>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> (cherry picked from commit 62ca8b9620cc1168ace6905575b7d36438363aed)

The below system test case fails with this patch.  Please check this
out - https://github.com/numansiddique/ovn/runs/3665362661?check_suite_focus=true

## ----------------------- ##
## ovn 21.06.1 test suite. ##
## ----------------------- ##
134: ovn -- ECMP IPv6 symmetric reply -- ovn-northd -- dp-groups=no
FAILED (system-ovn.at:5539)

## ------------- ##
## Test results. ##
## ------------- ##


The test fails locally too.   I didn't look into the details.

Thanks
Numan

> ---
>  northd/ovn-northd.8.xml | 28 ++++++++++++++++++++++++++++
>  northd/ovn-northd.c     | 14 ++++++++++++++
>  northd/ovn_northd.dl    | 33 +++++++++++++++++++++++++++++++--
>  ovs                     |  2 +-
>  tests/ovn-northd.at     |  2 ++
>  5 files changed, 76 insertions(+), 3 deletions(-)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 890775797..29eaf1864 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -262,6 +262,16 @@
>          logical ports on which port security is not enabled, these advance all
>          packets that match the <code>inport</code>.
>        </li>
> +      <li>
> +        For logical ports of type <code>vtep</code>, the above logical flow
> +        will also apply the action <code>REGBIT_FROM_RAMP = 1;</code> to
> +        indicate that the packet is coming from a RAMP (controller-vtep)
> +        device.  Later pipelines will use this information to skip
> +        sending the packet to the conntrack.  Packets from <code>vtep</code>
> +        logical ports should go though ingress pipeline only to determine
> +        the output port and they should not be subjected to any ACL checks.
> +        Egress pipeline will do the ACL checks.
> +      </li>
>      </ul>
>
>      <p>
> @@ -453,6 +463,15 @@
>        processing.
>      </p>
>
> +    <p>
> +      This table has a priority-110 flow with the match
> +      <code>REGBIT_FROM_RAMP == 1</code> for all logical switch datapaths to
> +      resubmit traffic to the next table. <code>REGBIT_FROM_RAMP</code>
> +      indicates that packet was received from <code>vtep</code> logical ports
> +      and it can be skipped from the stateful ACL processing in the ingress
> +      pipeline.
> +    </p>
> +
>      <p>
>        This table also has a priority-110 flow with the match
>        <code>eth.dst == <var>E</var></code> for all logical switch
> @@ -512,6 +531,15 @@
>        configured. We can now add a lflow to drop ct.inv packets.
>      </p>
>
> +    <p>
> +      This table has a priority-110 flow with the match
> +      <code>REGBIT_FROM_RAMP == 1</code> for all logical switch datapaths to
> +      resubmit traffic to the next table. <code>REGBIT_FROM_RAMP</code>
> +      indicates that packet was received from <code>vtep</code> logical ports
> +      and it can be skipped from the load balancer processing in the ingress
> +      pipeline.
> +    </p>
> +
>      <p>
>        This table also has a priority-110 flow with the match
>        <code>eth.dst == <var>E</var></code> for all logical switch
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index a7f6fdf6b..c2cc9b930 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -236,6 +236,7 @@ enum ovn_stage {
>  #define REGBIT_ACL_HINT_BLOCK     "reg0[10]"
>  #define REGBIT_LKUP_FDB           "reg0[11]"
>  #define REGBIT_HAIRPIN_REPLY      "reg0[12]"
> +#define REGBIT_FROM_RAMP          "reg0[14]"
>
>  #define REG_ORIG_DIP_IPV4         "reg1"
>  #define REG_ORIG_DIP_IPV6         "xxreg1"
> @@ -4823,10 +4824,15 @@ build_lswitch_input_port_sec_op(
>      build_port_security_l2("eth.src", op->ps_addrs, op->n_ps_addrs,
>                             match);
>
> +    if (!strcmp(op->nbsp->type, "vtep")) {
> +        ds_put_format(actions, REGBIT_FROM_RAMP" = 1; ");
> +    }
> +
>      const char *queue_id = smap_get(&op->sb->options, "qdisc_queue_id");
>      if (queue_id) {
>          ds_put_format(actions, "set_queue(%s); ", queue_id);
>      }
> +
>      ds_put_cstr(actions, "next;");
>      ovn_lflow_add_with_hint(lflows, op->od, S_SWITCH_IN_PORT_SEC_L2, 50,
>                              ds_cstr(match), ds_cstr(actions),
> @@ -5070,6 +5076,10 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *port_groups,
>                        "nd || nd_rs || nd_ra || mldv1 || mldv2 || "
>                        "(udp && udp.src == 546 && udp.dst == 547)", "next;");
>
> +        /* Do not send coming from RAMP switch packets to conntrack. */
> +        ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110,
> +                      REGBIT_FROM_RAMP" == 1", "next;");
> +
>          /* Ingress and Egress Pre-ACL Table (Priority 100).
>           *
>           * Regardless of whether the ACL is "from-lport" or "to-lport",
> @@ -5180,6 +5190,10 @@ build_pre_lb(struct ovn_datapath *od, struct hmap *lflows,
>      ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_LB, 110,
>                    "eth.src == $svc_monitor_mac", "next;");
>
> +    /* Do not send coming from RAMP switch packets to conntrack. */
> +    ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_LB, 110,
> +                  REGBIT_FROM_RAMP" == 1", "next;");
> +
>      /* Allow all packets to go to next tables by default. */
>      ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_LB, 0, "1", "next;");
>      ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_LB, 0, "1", "next;");
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 46da9a3a4..cca1c11be 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1561,6 +1561,7 @@ function rEGBIT_ACL_HINT_DROP()    : string = "reg0[9]"
>  function rEGBIT_ACL_HINT_BLOCK()   : string = "reg0[10]"
>  function rEGBIT_LKUP_FDB()         : string = "reg0[11]"
>  function rEGBIT_HAIRPIN_REPLY()    : string = "reg0[12]"
> +function rEGBIT_FROM_RAMP()        : string = "reg0[14]"
>
>  function rEG_ORIG_DIP_IPV4()       : string = "reg1"
>  function rEG_ORIG_DIP_IPV6()       : string = "xxreg1"
> @@ -1934,6 +1935,16 @@ for (&Switch(._uuid = ls_uuid, .has_stateful_acl = true)) {
>           .actions          = "next;",
>           .external_ids     = map_empty());
>
> +    /* Do not send coming from RAMP switch packets to conntrack. */
> +    Flow(.logical_datapath = ls_uuid,
> +         .stage            = s_SWITCH_IN_PRE_ACL(),
> +         .priority         = 110,
> +         .__match          = "${rEGBIT_FROM_RAMP()} == 1",
> +         .actions          = "next;",
> +         .stage_hint       = 0,
> +         .io_port          = None,
> +         .controller_meter = None);
> +
>      /* Ingress and Egress Pre-ACL Table (Priority 100).
>       *
>       * Regardless of whether the ACL is "from-lport" or "to-lport",
> @@ -1988,6 +1999,16 @@ for (&Switch(._uuid = ls_uuid)) {
>           .actions          = "next;",
>           .external_ids     = map_empty());
>
> +    /* Do not send coming from RAMP switch packets to conntrack. */
> +    Flow(.logical_datapath = ls_uuid,
> +         .stage            = s_SWITCH_IN_PRE_LB(),
> +         .priority         = 110,
> +         .__match          = "${rEGBIT_FROM_RAMP()} == 1",
> +         .actions          = "next;",
> +         .stage_hint       = 0,
> +         .io_port          = None,
> +         .controller_meter = None);
> +
>      /* Allow all packets to go to next tables by default. */
>      Flow(.logical_datapath = ls_uuid,
>           .stage            = s_SWITCH_IN_PRE_LB(),
> @@ -3061,10 +3082,18 @@ for (&SwitchPort(.lsp = lsp, .sw = sw, .json_name = json_name, .ps_eth_addresses
>              } else {
>                  "inport == ${json_name} && eth.src == {${ps_eth_addresses.join(\" \")}}"
>              } in
> -        var actions = match (pbinding.options.get("qdisc_queue_id")) {
> +        var actions = {
> +            var ramp = if (lsp.__type == "vtep") {
> +                "${rEGBIT_FROM_RAMP()} = 1; "
> +            } else {
> +                ""
> +            };
> +            var queue = match (pbinding.options.get("qdisc_queue_id")) {
>                  None -> "next;",
>                  Some{id} -> "set_queue(${id}); next;"
> -            } in
> +            };
> +            "${ramp}${queue}"
> +        } in
>          Flow(.logical_datapath = sw._uuid,
>               .stage            = s_SWITCH_IN_PORT_SEC_L2(),
>               .priority         = 50,
> diff --git a/ovs b/ovs
> index a4b04276a..daf627f45 160000
> --- a/ovs
> +++ b/ovs
> @@ -1 +1 @@
> -Subproject commit a4b04276ab5934d087669ff2d191a23931335c87
> +Subproject commit daf627f459ffbc7171d42a2c01f80754bfd54edc
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 55cf0ffd4..9523de377 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3429,6 +3429,7 @@ check_stateful_flows() {
>    table=6 (ls_in_pre_lb       ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(next;)
>    table=6 (ls_in_pre_lb       ), priority=110  , match=(ip && inport == "sw0-lr0"), action=(next;)
>    table=6 (ls_in_pre_lb       ), priority=110  , match=(nd || nd_rs || nd_ra || mldv1 || mldv2), action=(next;)
> +  table=6 (ls_in_pre_lb       ), priority=110  , match=(reg0[[14]] == 1), action=(next;)
>  ])
>
>      AT_CHECK([grep "ls_in_pre_stateful" sw0flows | sort], [0], [dnl
> @@ -3490,6 +3491,7 @@ AT_CHECK([grep "ls_in_pre_lb" sw0flows | sort], [0], [dnl
>    table=6 (ls_in_pre_lb       ), priority=110  , match=(eth.dst == $svc_monitor_mac), action=(next;)
>    table=6 (ls_in_pre_lb       ), priority=110  , match=(ip && inport == "sw0-lr0"), action=(next;)
>    table=6 (ls_in_pre_lb       ), priority=110  , match=(nd || nd_rs || nd_ra || mldv1 || mldv2), action=(next;)
> +  table=6 (ls_in_pre_lb       ), priority=110  , match=(reg0[[14]] == 1), action=(next;)
>  ])
>
>  AT_CHECK([grep "ls_in_pre_stateful" sw0flows | sort], [0], [dnl
> --
> 2.30.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>