SSL Health Monitors didn't actually ... check very much

Change HTTPS monitors to be a real check, and add TLS-HELLO type to
perform the older check functionality if desired.
The only reason you would need TLS-HELLO instead of HTTPS is if your
application does client-cert validation, as the HAProxy box won't have a
valid client cert.

Also add missing PING type to the DB, so PING monitors can be used.

Change-Id: I15a79b7fb0c2ff1020090b4057909a1f41a2c8ad

api-ref/source/parameters.yaml

@@ -321,8 +321,8 @@ healthmonitor-timeout-optional:
   type: integer
 healthmonitor-type:
   description: |
-    The type of health monitor.  One of ``HTTP``, ``HTTPS``, ``PING``, or
+    The type of health monitor.  One of ``HTTP``, ``HTTPS``, ``PING``, ``TCP``,
-    ``TCP``.
+    or ``TLS-HELLO``.
   in: body
   required: true
   type: string

api-ref/source/v2/healthmonitor.inc

@@ -115,7 +115,7 @@ At a minimum, you must specify these health monitor attributes:
   times out.
 
 - ``type`` The type of health monitor. One of ``HTTP``, ``HTTPS``, ``PING``,
-  or ``TCP``.
+  ``TCP``, or ``TLS-HELLO``.
 
 Some attributes receive default values if you omit them from the request:
 

doc/source/api/octaviaapi.rst

@@ -1097,7 +1097,7 @@ Health Monitors
 +================+=========+======================================+
 | type           | String  | Type of health monitoring from \     |
 |                |         | the following: ``PING``, ``TCP``, \  |
-|                |         | ``HTTP``, ``HTTPS``                  |
+|                |         | ``HTTP``, ``HTTPS``, ``TLS-HELLO``   |
 +----------------+---------+--------------------------------------+
 | delay          | Integer | Delay between health checks          |
 +----------------+---------+--------------------------------------+

doc/source/guides/basic-cookbook.rst

@@ -602,7 +602,8 @@ generates the health check in your web application:
 
 Other heath monitors
 --------------------
-Other health monitor types include ``PING``, ``TCP`` and ``HTTPS``.
+Other health monitor types include ``PING``, ``TCP``, ``HTTPS``, and
+``TLS-HELLO``.
 
 ``PING`` health monitors send periodic ICMP PING requests to the back-end
 servers. Obviously, your back-end servers must be configured to allow PINGs in
@@ -613,9 +614,14 @@ port. Your custom TCP application should be written to respond OK to the load
 balancer connecting, opening a TCP connection, and closing it again after the
 TCP handshake without sending any data.
 
-``HTTPS`` health monitors operate exactly like HTTP health monitors, except
+``HTTPS`` health monitors operate exactly like HTTP health monitors, but with
-that they also ensure the back-end server responds to SSLv3 client hello
+ssl back-end servers. Unfortunately, this causes problems if the servers are
-messages.
+performing client certificate validation, as HAProxy won't have a valid cert.
+In this case, using ``TLS-HELLO`` type monitoring is an alternative.
+
+``TLS-HELLO`` health monitors simply ensure the back-end server responds to
+SSLv3 client hello messages. It will not check any other health metrics, like
+status code or body contents.
 
 
 Intermediate certificate chains

octavia/api/v2/controllers/health_monitor.py

@@ -183,6 +183,10 @@ class HealthMonitorController(base.BaseController):
                 lock_session, health_monitor)
             db_hm = self._validate_create_hm(lock_session, hm_dict)
             lock_session.commit()
+        except odb_exceptions.DBError:
+            lock_session.rollback()
+            raise exceptions.InvalidOption(
+                value=hm_dict.get('type'), option='type')
         except Exception:
             with excutils.save_and_reraise_exception():
                 lock_session.rollback()

octavia/common/constants.py

@@ -30,8 +30,10 @@ HEALTH_MONITOR_PING = 'PING'
 HEALTH_MONITOR_TCP = 'TCP'
 HEALTH_MONITOR_HTTP = 'HTTP'
 HEALTH_MONITOR_HTTPS = 'HTTPS'
+HEALTH_MONITOR_TLS_HELLO = 'TLS-HELLO'
 SUPPORTED_HEALTH_MONITOR_TYPES = (HEALTH_MONITOR_HTTP, HEALTH_MONITOR_HTTPS,
-                                  HEALTH_MONITOR_PING, HEALTH_MONITOR_TCP)
+                                  HEALTH_MONITOR_PING, HEALTH_MONITOR_TCP,
+                                  HEALTH_MONITOR_TLS_HELLO)
 HEALTH_MONITOR_HTTP_METHOD_GET = 'GET'
 HEALTH_MONITOR_HTTP_METHOD_HEAD = 'HEAD'
 HEALTH_MONITOR_HTTP_METHOD_POST = 'POST'

octavia/common/jinja/haproxy/templates/macros.j2

@@ -151,8 +151,14 @@ frontend {{ listener.id }}
         {% else %}
             {% set monitor_port_opt = "" %}
         {% endif %}
-        {% set hm_opt = " check inter %ds fall %d rise %d%s%s"|format(
+        {% if pool.health_monitor.type == constants.HEALTH_MONITOR_HTTPS %}
-            pool.health_monitor.delay, pool.health_monitor.fall_threshold,
+            {% set monitor_ssl_opt = " check-ssl verify none" %}
+        {% else %}
+            {% set monitor_ssl_opt = "" %}
+        {% endif %}
+        {% set hm_opt = " check%s inter %ds fall %d rise %d%s%s"|format(
+            monitor_ssl_opt, pool.health_monitor.delay,
+            pool.health_monitor.fall_threshold,
             pool.health_monitor.rise_threshold, monitor_addr_opt,
             monitor_port_opt) %}
     {% else %}
@@ -218,7 +224,7 @@ backend {{ pool.id }}
     pool.health_monitor.url_path }}
     http-check expect rstatus {{ pool.health_monitor.expected_codes }}
         {% endif %}
-        {% if pool.health_monitor.type == constants.HEALTH_MONITOR_HTTPS %}
+        {% if pool.health_monitor.type == constants.HEALTH_MONITOR_TLS_HELLO %}
     option ssl-hello-chk
         {% endif %}
     {% endif %}

octavia/db/migration/alembic_migrations/versions/e6672bda93bf_add_ping_and_tlshello_monitor_types.py

@@ -0,0 +1,45 @@
+#    Copyright 2017 GoDaddy
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""add ping and tls-hello monitor types
+
+Revision ID: e6672bda93bf
+Revises: 27e54d00c3cd
+Create Date: 2017-06-21 16:13:09.615651
+
+"""
+
+from alembic import op
+import sqlalchemy as sa
+from sqlalchemy import sql
+
+# revision identifiers, used by Alembic.
+revision = 'e6672bda93bf'
+down_revision = '27e54d00c3cd'
+
+
+def upgrade():
+    insert_table = sql.table(
+        u'health_monitor_type',
+        sql.column(u'name', sa.String),
+        sql.column(u'description', sa.String)
+    )
+
+    op.bulk_insert(
+        insert_table,
+        [
+            {'name': 'PING'},
+            {'name': 'TLS-HELLO'}
+        ]
+    )

octavia/tests/functional/api/v2/test_health_monitor.py

@@ -196,7 +196,7 @@ class TestHealthMonitor(base.BaseAPITest):
             1, 1, 1, 1).get(self.root_tag)
         self.set_lb_status(lb1_id)
         hm3 = self.create_health_monitor(
-            pool3.get('id'), constants.HEALTH_MONITOR_TCP,
+            pool3.get('id'), constants.HEALTH_MONITOR_TLS_HELLO,
             1, 1, 1, 1).get(self.root_tag)
         self.set_lb_status(lb1_id)
         hms = self.get(self.HMS_PATH).json.get(self.root_tag_list)

octavia/tests/unit/common/jinja/haproxy/test_jinja_cfg.py

@@ -150,7 +150,7 @@ class TestHaproxyCfg(base.TestCase):
             sample_configs.sample_base_expected_config(backend=be),
             rendered_obj)
 
-    def test_render_template_https(self):
+    def test_render_template_https_real_monitor(self):
         fe = ("frontend sample_listener_id_1\n"
               "    option tcplog\n"
               "    maxconn 98\n"
@@ -164,6 +164,31 @@ class TestHaproxyCfg(base.TestCase):
               "    timeout check 31s\n"
               "    option httpchk GET /index.html\n"
               "    http-check expect rstatus 418\n"
+              "    fullconn 98\n"
+              "    server sample_member_id_1 10.0.0.99:82 "
+              "weight 13 check check-ssl verify none inter 30s fall 3 rise 2 "
+              "cookie sample_member_id_1\n"
+              "    server sample_member_id_2 10.0.0.98:82 "
+              "weight 13 check check-ssl verify none inter 30s fall 3 rise 2 "
+              "cookie sample_member_id_2\n\n")
+        rendered_obj = self.jinja_cfg.render_loadbalancer_obj(
+            sample_configs.sample_amphora_tuple(),
+            sample_configs.sample_listener_tuple(proto='HTTPS'))
+        self.assertEqual(sample_configs.sample_base_expected_config(
+            frontend=fe, backend=be), rendered_obj)
+
+    def test_render_template_https_hello_monitor(self):
+        fe = ("frontend sample_listener_id_1\n"
+              "    option tcplog\n"
+              "    maxconn 98\n"
+              "    bind 10.0.0.2:443\n"
+              "    mode tcp\n"
+              "    default_backend sample_pool_id_1\n\n")
+        be = ("backend sample_pool_id_1\n"
+              "    mode tcp\n"
+              "    balance roundrobin\n"
+              "    cookie SRV insert indirect nocache\n"
+              "    timeout check 31s\n"
               "    option ssl-hello-chk\n"
               "    fullconn 98\n"
               "    server sample_member_id_1 10.0.0.99:82 "
@@ -174,7 +199,8 @@ class TestHaproxyCfg(base.TestCase):
               "cookie sample_member_id_2\n\n")
         rendered_obj = self.jinja_cfg.render_loadbalancer_obj(
             sample_configs.sample_amphora_tuple(),
-            sample_configs.sample_listener_tuple(proto='HTTPS'))
+            sample_configs.sample_listener_tuple(
+                proto='HTTPS', monitor_proto='TLS-HELLO'))
         self.assertEqual(sample_configs.sample_base_expected_config(
             frontend=fe, backend=be), rendered_obj)
 

octavia/tests/unit/common/sample_configs/sample_configs.py

@@ -405,7 +405,8 @@ def sample_listener_tuple(proto=None, monitor=True, persistence=True,
                           persistence_type=None, persistence_cookie=None,
                           tls=False, sni=False, peer_port=None, topology=None,
                           l7=False, enabled=True, insert_headers=None,
-                          be_proto=None, monitor_ip_port=False):
+                          be_proto=None, monitor_ip_port=False,
+                          monitor_proto=None):
     proto = 'HTTP' if proto is None else proto
     if be_proto is None:
         be_proto = 'HTTP' if proto is 'TERMINATED_HTTPS' else proto
@@ -425,12 +426,12 @@ def sample_listener_tuple(proto=None, monitor=True, persistence=True,
                 proto=be_proto, monitor=monitor, persistence=persistence,
                 persistence_type=persistence_type,
                 persistence_cookie=persistence_cookie,
-                monitor_ip_port=monitor_ip_port),
+                monitor_ip_port=monitor_ip_port, monitor_proto=monitor_proto),
             sample_pool_tuple(
                 proto=be_proto, monitor=monitor, persistence=persistence,
                 persistence_type=persistence_type,
                 persistence_cookie=persistence_cookie, sample_pool=2,
-                monitor_ip_port=monitor_ip_port)]
+                monitor_ip_port=monitor_ip_port, monitor_proto=monitor_proto)]
         l7policies = [
             sample_l7policy_tuple('sample_l7policy_id_1', sample_policy=1),
             sample_l7policy_tuple('sample_l7policy_id_2', sample_policy=2),
@@ -444,7 +445,7 @@ def sample_listener_tuple(proto=None, monitor=True, persistence=True,
                 proto=be_proto, monitor=monitor, persistence=persistence,
                 persistence_type=persistence_type,
                 persistence_cookie=persistence_cookie,
-                monitor_ip_port=monitor_ip_port)]
+                monitor_ip_port=monitor_ip_port, monitor_proto=monitor_proto)]
         l7policies = []
     return in_listener(
         id='sample_listener_id_1',
@@ -458,7 +459,7 @@ def sample_listener_tuple(proto=None, monitor=True, persistence=True,
             proto=be_proto, monitor=monitor, persistence=persistence,
             persistence_type=persistence_type,
             persistence_cookie=persistence_cookie,
-            monitor_ip_port=monitor_ip_port),
+            monitor_ip_port=monitor_ip_port, monitor_proto=monitor_proto),
         connection_limit=98,
         tls_certificate_id='cont_id_1' if tls else '',
         sni_container_ids=['cont_id_2', 'cont_id_3'] if sni else [],
@@ -515,8 +516,10 @@ def sample_tls_container_tuple(id='cont_id_1', certificate=None,
 
 def sample_pool_tuple(proto=None, monitor=True, persistence=True,
                       persistence_type=None, persistence_cookie=None,
-                      sample_pool=1, monitor_ip_port=False):
+                      sample_pool=1, monitor_ip_port=False,
+                      monitor_proto=None):
     proto = 'HTTP' if proto is None else proto
+    monitor_proto = proto if monitor_proto is None else monitor_proto
     in_pool = collections.namedtuple(
         'pool', 'id, protocol, lb_algorithm, members, health_monitor,'
                 'session_persistence, enabled, operating_status')
@@ -531,13 +534,13 @@ def sample_pool_tuple(proto=None, monitor=True, persistence=True,
                    sample_member_tuple('sample_member_id_2', '10.0.0.98',
                                        monitor_ip_port=monitor_ip_port)]
         if monitor is True:
-            mon = sample_health_monitor_tuple(proto=proto)
+            mon = sample_health_monitor_tuple(proto=monitor_proto)
     elif sample_pool == 2:
         id = 'sample_pool_id_2'
         members = [sample_member_tuple('sample_member_id_3', '10.0.0.97',
                                        monitor_ip_port=monitor_ip_port)]
         if monitor is True:
-            mon = sample_health_monitor_tuple(proto=proto, sample_hm=2)
+            mon = sample_health_monitor_tuple(proto=monitor_proto, sample_hm=2)
     return in_pool(
         id=id,
         protocol=proto,

releasenotes/notes/Change-HTTPS-HealthMonitor-functionality-79240ef13e65cd88.yaml

@@ -0,0 +1,14 @@
+---
+features:
+  - |
+    New Health Monitor type "TLS-HELLO" to perform a simple TLS connection.
+upgrade:
+  - |
+    If users have configured Health Monitors of type "HTTPS" and are expecting
+    a simple "TLS-HELLO" check, they will need to recreate their monitor with
+    the new "TLS-HELLO" type.
+fixes:
+  - |
+    Health Monitor type "HTTPS" now correctly performs the configured check.
+    This is done with all certificate validation disabled, so it will not work
+    if backend members are performing client certificate validation.