[ovs-dev] [PATCH v2 ovn] northd: add reject action for lb with no backends

Numan Siddique numans at ovn.org
Tue Dec 8 06:42:05 UTC 2020


On Mon, Dec 7, 2020 at 7:28 PM Lorenzo Bianconi
<lorenzo.bianconi at redhat.com> wrote:
>
> Introduce the capability to create a load balancer with no backends and
> with --reject option in order to send a TCP reset segment (for tcp) or
> an ICMP port unreachable packet (for all other kind of traffic) whenever
> an incoming packet is received for this load-balancer.
>
> Tested-by: Antonio Ojea <aojeagar at redhat.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi at redhat.com>

Hi Lorenzo,

Thanks for the patch. The patch LGTM. Can you please add a test case
or update an existsing
test case for this scenario in ovn-northd.at ?

Thanks
Numan

> ---
> Changes since v1:
> - add reject action for health_check empty lb
> - rename build_lb_vip_ct_lb_actions in build_lb_vip_actions
> - improve documentation
> ---
>  lib/lb.c                  |  2 ++
>  lib/lb.h                  |  1 +
>  northd/ovn-northd.8.xml   | 19 +++++++++++++++++
>  northd/ovn-northd.c       | 44 ++++++++++++++++++++++++++-------------
>  ovn-nb.ovsschema          |  9 ++++++--
>  ovn-nb.xml                | 10 +++++++++
>  tests/system-ovn.at       | 28 ++++++++++++++++++++++++-
>  utilities/ovn-nbctl.8.xml | 11 +++++++++-
>  utilities/ovn-nbctl.c     |  7 ++++++-
>  9 files changed, 111 insertions(+), 20 deletions(-)
>
> diff --git a/lib/lb.c b/lib/lb.c
> index a90042e58..2517c02ef 100644
> --- a/lib/lb.c
> +++ b/lib/lb.c
> @@ -189,6 +189,8 @@ ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb,
>          struct ovn_lb_vip *lb_vip = &lb->vips[n_vips];
>          struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[n_vips];
>
> +        lb_vip->empty_backend_rej = smap_get_bool(&nbrec_lb->options,
> +                                                  "reject", false);
>          if (!ovn_lb_vip_init(lb_vip, node->key, node->value)) {
>              continue;
>          }
> diff --git a/lib/lb.h b/lib/lb.h
> index 6644ad0d8..42c580bd1 100644
> --- a/lib/lb.h
> +++ b/lib/lb.h
> @@ -49,6 +49,7 @@ struct ovn_lb_vip {
>
>      struct ovn_lb_backend *backends;
>      size_t n_backends;
> +    bool empty_backend_rej;
>  };
>
>  struct ovn_lb_backend {
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 294402de3..19f08fc65 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -700,6 +700,16 @@
>          ct_lb(<var>args</var>)</code>, where <var>args</var> contains comma
>          separated IP addresses of the same address family as <var>VIP</var>.
>        </li>
> +
> +      <li>
> +        If the load balancer is created with <code>--reject</code> option and
> +        it has no active backends, a TCP reset segment (for tcp) or an ICMP
> +        port unreachable packet (for all other kind of traffic) will be sent
> +        whenever an incoming packet is received for this load-balancer.
> +        Please note using <code>--reject</code> option will disable
> +        empty_lb SB controller event.
> +      </li>
> +
>        <li>
>          A priority-100 flow commits packets to connection tracker using
>          <code>ct_commit; next;</code> action based on a hint provided by
> @@ -2592,6 +2602,15 @@ icmp6 {
>          packets, the above action will be replaced by
>          <code>flags.force_snat_for_lb = 1; ct_dnat;</code>.
>        </li>
> +
> +      <li>
> +        If the load balancer is created with <code>--reject</code> option and
> +        it has no active backends, a TCP reset segment (for tcp) or an ICMP
> +        port unreachable packet (for all other kind of traffic) will be sent
> +        whenever an incoming packet is received for this load-balancer.
> +        Please note using <code>--reject</code> option will disable
> +        empty_lb SB controller event.
> +      </li>
>      </ul>
>
>      <p>Ingress Table 6: DNAT on Gateway Routers</p>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 403342b0d..470e69465 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -3436,12 +3436,12 @@ ovn_lb_svc_create(struct northd_context *ctx, struct ovn_northd_lb *lb,
>  }
>
>  static
> -void build_lb_vip_ct_lb_actions(struct ovn_lb_vip *lb_vip,
> -                                struct ovn_northd_lb_vip *lb_vip_nb,
> -                                struct ds *action,
> -                                char *selection_fields)
> +void build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
> +                          struct ovn_northd_lb_vip *lb_vip_nb,
> +                          struct ds *action, char *selection_fields,
> +                          bool ls_dp)
>  {
> -    bool skip_hash_fields = false;
> +    bool skip_hash_fields = false, reject = false;
>
>      if (lb_vip_nb->lb_health_check) {
>          ds_put_cstr(action, "ct_lb(backends=");
> @@ -3463,18 +3463,30 @@ void build_lb_vip_ct_lb_actions(struct ovn_lb_vip *lb_vip,
>          }
>
>          if (!n_active_backends) {
> -            skip_hash_fields = true;
> -            ds_clear(action);
> -            ds_put_cstr(action, "drop;");
> +            if (!lb_vip->empty_backend_rej) {
> +                ds_clear(action);
> +                ds_put_cstr(action, "drop;");
> +                skip_hash_fields = true;
> +            } else {
> +                reject = true;
> +            }
>          } else {
>              ds_chomp(action, ',');
>              ds_put_cstr(action, ");");
>          }
> +    } else if (lb_vip->empty_backend_rej && !lb_vip->n_backends) {
> +        reject = true;
>      } else {
>          ds_put_format(action, "ct_lb(backends=%s);", lb_vip_nb->backend_ips);
>      }
>
> -    if (!skip_hash_fields && selection_fields && selection_fields[0]) {
> +    if (reject) {
> +        int stage = ls_dp ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK)
> +                          : ovn_stage_get_table(S_ROUTER_OUT_SNAT);
> +        ds_clear(action);
> +        ds_put_format(action, "reg0 = 0; reject { outport <-> inport; "
> +                      "next(pipeline=egress,table=%d);};", stage);
> +    } else if (!skip_hash_fields && selection_fields && selection_fields[0]) {
>          ds_chomp(action, ';');
>          ds_chomp(action, ')');
>          ds_put_format(action, "; hash_fields=\"%s\");", selection_fields);
> @@ -5065,7 +5077,8 @@ build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap *lflows,
>                            struct nbrec_load_balancer *lb,
>                            int pl, struct shash *meter_groups)
>  {
> -    if (!controller_event_en || lb_vip->n_backends) {
> +    if (!controller_event_en || lb_vip->n_backends ||
> +        lb_vip->empty_backend_rej) {
>          return;
>      }
>
> @@ -5955,8 +5968,8 @@ build_lb_rules(struct ovn_datapath *od, struct hmap *lflows,
>
>          /* New connections in Ingress table. */
>          struct ds action = DS_EMPTY_INITIALIZER;
> -        build_lb_vip_ct_lb_actions(lb_vip, lb_vip_nb, &action,
> -                                   lb->selection_fields);
> +        build_lb_vip_actions(lb_vip, lb_vip_nb, &action,
> +                             lb->selection_fields, true);
>
>          struct ds match = DS_EMPTY_INITIALIZER;
>          ds_put_format(&match, "ct.new && %s.dst == %s", ip_match,
> @@ -9666,8 +9679,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                  struct ovn_lb_vip *lb_vip = &lb->vips[j];
>                  struct ovn_northd_lb_vip *lb_vip_nb = &lb->vips_nb[j];
>                  ds_clear(&actions);
> -                build_lb_vip_ct_lb_actions(lb_vip, lb_vip_nb, &actions,
> -                                           lb->selection_fields);
> +                build_lb_vip_actions(lb_vip, lb_vip_nb, &actions,
> +                                     lb->selection_fields, false);
>
>                  if (!sset_contains(&all_ips, lb_vip->vip_str)) {
>                      sset_add(&all_ips, lb_vip->vip_str);
> @@ -9718,7 +9731,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports,
>                      prio = 120;
>                  }
>
> -                if (od->l3redirect_port) {
> +                if (od->l3redirect_port &&
> +                    (lb_vip->n_backends || !lb_vip->empty_backend_rej)) {
>                      ds_put_format(&match, " && is_chassis_resident(%s)",
>                                    od->l3redirect_port->json_key);
>                  }
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 269e3a888..af77dd138 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
> -    "version": "5.28.0",
> -    "cksum": "610359755 26847",
> +    "version": "5.29.0",
> +    "cksum": "328602112 27064",
>      "tables": {
>          "NB_Global": {
>              "columns": {
> @@ -188,6 +188,11 @@
>                                  ["eth_src", "eth_dst", "ip_src", "ip_dst",
>                                   "tp_src", "tp_dst"]]},
>                               "min": 0, "max": "unlimited"}},
> +                "options": {
> +                     "type": {"key": "string",
> +                              "value": "string",
> +                              "min": 0,
> +                              "max": "unlimited"}},
>                  "external_ids": {
>                      "type": {"key": "string", "value": "string",
>                               "min": 0, "max": "unlimited"}}},
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index c9ab25ceb..02fd70c08 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -1635,6 +1635,16 @@
>          See <em>External IDs</em> at the beginning of this document.
>        </column>
>      </group>
> +    <group title="Load_Balancer options">
> +      <column name="options" key="reject" type='{"type": "boolean"}'>
> +        If the load balancer is created with <code>--reject</code> option and
> +        it has no active backends, a TCP reset segment (for tcp) or an ICMP
> +        port unreachable packet (for all other kind of traffic) will be sent
> +        whenever an incoming packet is received for this load-balancer.
> +        Please note using <code>--reject</code> option will disable empty_lb
> +        SB controller event.
> +      </column>
> +    </group>
>    </table>
>
>    <table name="Load_Balancer_Health_Check" title="load balancer">
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index d59f7c97e..1e73001ab 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -1574,6 +1574,18 @@ OVS_WAIT_UNTIL([
>      grep "selection_method=hash,fields(ip_src,ip_dst,sctp_src,sctp_dst)" -c) -eq 2
>  ])
>
> +ovn-nbctl --reject lb-add lb3 30.0.0.10:80 ""
> +ovn-nbctl ls-lb-add foo lb3
> +# Filter reset segments
> +NS_CHECK_EXEC([foo1], [tcpdump -c 1 -neei foo1 ip[[33:1]]=0x14 > rst.pcap &])
> +sleep 1
> +NS_CHECK_EXEC([foo1], [wget -q 30.0.0.10],[4])
> +
> +OVS_WAIT_UNTIL([
> +    n_reset=$(cat rst.pcap | wc -l)
> +    test "${n_reset}" = "1"
> +])
> +
>  OVS_APP_EXIT_AND_WAIT([ovn-controller])
>
>  as ovn-sb
> @@ -4151,7 +4163,7 @@ ovn-nbctl lsp-set-type sw1-lr0 router
>  ovn-nbctl lsp-set-addresses sw1-lr0 router
>  ovn-nbctl lsp-set-options sw1-lr0 router-port=lr0-sw1
>
> -ovn-nbctl lb-add lb1 10.0.0.10:80 10.0.0.3:80,20.0.0.3:80
> +ovn-nbctl --reject lb-add lb1 10.0.0.10:80 10.0.0.3:80,20.0.0.3:80
>
>  ovn-nbctl --wait=sb set load_balancer . ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2
>  ovn-nbctl --wait=sb set load_balancer . ip_port_mappings:20.0.0.3=sw1-p1:20.0.0.2
> @@ -4266,6 +4278,20 @@ ovn-sbctl list service_monitor
>  OVS_WAIT_UNTIL([test 2 = `ovn-sbctl --bare --columns status find \
>  service_monitor protocol=udp | sed '/^$/d' | grep offline | wc -l`])
>
> +# Stop webserer in sw1-p1
> +pid_file=$(cat l7_pid_file)
> +NS_CHECK_EXEC([sw1-p1], [kill $(cat $pid_file)])
> +
> +NS_CHECK_EXEC([sw0-p2], [tcpdump -c 1 -neei sw0-p2 ip[[33:1]]=0x14 > rst.pcap &])
> +OVS_WAIT_UNTIL([test 2 = `ovn-sbctl --bare --columns status find \
> +service_monitor protocol=tcp | sed '/^$/d' | grep offline | wc -l`])
> +NS_CHECK_EXEC([sw0-p2], [wget 10.0.0.10 -v -o wget$i.log],[4])
> +
> +OVS_WAIT_UNTIL([
> +    n_reset=$(cat rst.pcap | wc -l)
> +    test "${n_reset}" = "1"
> +])
> +
>  OVS_APP_EXIT_AND_WAIT([ovn-controller])
>
>  as ovn-sb
> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> index 59302296b..b9ce43684 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -903,7 +903,7 @@
>
>      <h1>Load Balancer Commands</h1>
>      <dl>
> -      <dt>[<code>--may-exist</code> | <code>--add-duplicate</code>] <code>lb-add</code> <var>lb</var> <var>vip</var> <var>ips</var> [<var>protocol</var>]</dt>
> +        <dt>[<code>--may-exist</code> | <code>--add-duplicate</code> | <code>--reject</code>] <code>lb-add</code> <var>lb</var> <var>vip</var> <var>ips</var> [<var>protocol</var>]</dt>
>        <dd>
>          <p>
>           Creates a new load balancer named <var>lb</var> with the provided
> @@ -936,6 +936,15 @@
>           creates a new load balancer with a duplicate name.
>          </p>
>
> +        <p>
> +         If the load balancer is created with <code>--reject</code> option and
> +         it has no active backends, a TCP reset segment (for tcp) or an ICMP
> +         port unreachable packet (for all other kind of traffic) will be sent
> +         whenever an incoming packet is received for this load-balancer.
> +         Please note using <code>--reject</code> option will disable
> +         empty_lb SB controller event.
> +        </p>
> +
>          <p>
>           The following example adds a load balancer.
>          </p>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index d19e1b6c6..3a95f6b1f 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -2821,6 +2821,7 @@ nbctl_lb_add(struct ctl_context *ctx)
>
>      bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
>      bool add_duplicate = shash_find(&ctx->options, "--add-duplicate") != NULL;
> +    bool empty_backend_rej = shash_find(&ctx->options, "--reject") != NULL;
>
>      const char *lb_proto;
>      bool is_update_proto = false;
> @@ -2934,6 +2935,10 @@ nbctl_lb_add(struct ctl_context *ctx)
>      smap_add(CONST_CAST(struct smap *, &lb->vips),
>              lb_vip_normalized, ds_cstr(&lb_ips_new));
>      nbrec_load_balancer_set_vips(lb, &lb->vips);
> +    if (empty_backend_rej) {
> +        const struct smap options = SMAP_CONST1(&options, "reject", "true");
> +        nbrec_load_balancer_set_options(lb, &options);
> +    }
>  out:
>      ds_destroy(&lb_ips_new);
>
> @@ -6588,7 +6593,7 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>        nbctl_lr_nat_set_ext_ips, NULL, "--is-exempted", RW},
>      /* load balancer commands. */
>      { "lb-add", 3, 4, "LB VIP[:PORT] IP[:PORT]... [PROTOCOL]", NULL,
> -      nbctl_lb_add, NULL, "--may-exist,--add-duplicate", RW },
> +      nbctl_lb_add, NULL, "--may-exist,--add-duplicate,--reject", RW },
>      { "lb-del", 1, 2, "LB [VIP]", NULL, nbctl_lb_del, NULL,
>          "--if-exists", RW },
>      { "lb-list", 0, 1, "[LB]", NULL, nbctl_lb_list, NULL, "", RO },
> --
> 2.28.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list