Hi Lorenzo,

There are some specific interactions that I think should be documented based on how this patch is written.

* If reject is set on the load balancer, this supersedes the controller_event option. Therefore if both are configured, no controller events are created. * The reject option does not function if load balancers have health checks enabled.

With regards to these interactions, I understand why the first was done. I don't understand why reject is disabled if health checks are enabled.

On 12/3/20 9:03 AM, Lorenzo Bianconi 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 <[email protected]>
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
  lib/lb.c                  |  2 ++
  lib/lb.h                  |  1 +
  northd/ovn-northd.8.xml   | 15 +++++++++++++++
  northd/ovn-northd.c       | 19 ++++++++++++++-----
  ovn-nb.ovsschema          |  9 +++++++--
  ovn-nb.xml                |  8 ++++++++
  tests/system-ovn.at       | 12 ++++++++++++
  utilities/ovn-nbctl.8.xml |  9 ++++++++-
  utilities/ovn-nbctl.c     |  7 ++++++-
  9 files changed, 73 insertions(+), 9 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..9f0134393 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -700,6 +700,14 @@
          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.
+      </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 +2600,13 @@ 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.
+      </li>
      </ul>
<p>Ingress Table 6: DNAT on Gateway Routers</p>
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 4ca027165..2af6df14a 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -3438,7 +3438,7 @@ 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)
+                                char *selection_fields, bool ls_dp)
  {
      bool skip_hash_fields = false;
@@ -3470,6 +3470,13 @@ void build_lb_vip_ct_lb_actions(struct ovn_lb_vip *lb_vip,
              ds_put_cstr(action, ");");
          }
      } else {
+        if (lb_vip->empty_backend_rej && !lb_vip->n_backends) {
+            int stage = ls_dp ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK)
+                              : ovn_stage_get_table(S_ROUTER_OUT_SNAT);
+            ds_put_format(action, "reg0 = 0; reject { outport <-> inport; "
+                          "next(pipeline=egress,table=%d);};", stage);
+            return;
+        }
          ds_put_format(action, "ct_lb(backends=%s);", lb_vip_nb->backend_ips);

Two things:
1) It makes perfect sense to add this code to this function, but it now makes the function name misleading. The function name implies that only ct_lb() actions are created. However, it can now also create a reject action. I think the function should be renamed. A good choice might be "build_lb_vip_actions" (just remove "ct_lb" from the name) 2) This is a nit, and it's probably personal preference, but I think this code is structured oddly. It currently is

} else {
    if (condition) {
      <code>
      return;
    }
    <implied else code>
}

I think it reads more easily as

} else if (condition) {
    <code>
    return;
} else {
    <explicit else code>
}

      }
@@ -5023,7 +5030,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;
      }
@@ -5914,7 +5922,7 @@ 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);
+                                   lb->selection_fields, true);
struct ds match = DS_EMPTY_INITIALIZER;
          ds_put_format(&match, "ct.new && %s.dst == %s", ip_match,
@@ -9620,7 +9628,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
*ports,
                  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);
+                                           lb->selection_fields, false);
if (!sset_contains(&all_ips, lb_vip->vip_str)) {
                      sset_add(&all_ips, lb_vip->vip_str);
@@ -9671,7 +9679,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 a457b9cee..6de33674f 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -1615,6 +1615,14 @@
          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.
+      </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..7526f9147 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
diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
index 59302296b..545ec422c 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,13 @@
           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.
+        </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 },


_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to