Fix listener API handling of None/null updates

The current listener API does not properly handle clearing/reseting
values on update. Some integer only fields, such as connection-limit,
will accept null, but will store the value as "None". These will
will cause failures updating the amphora configuration.

This patch corrects this to appropriately handle None/null updates
to the listener parameters.

Change-Id: I41c9bedd8a3452513af3d409fbacd65ea287f02a
Story: 2005374
Task: 30352

octavia/api/v2/controllers/listener.py

@@ -415,6 +415,34 @@ class ListenersController(base.BaseController):
         if ca_ref or crl_ref:
             self._validate_client_ca_and_crl_refs(ca_ref, crl_ref)
 
+    def _set_default_on_none(self, listener):
+        """Reset settings to their default values if None/null was passed in
+
+        A None/null value can be passed in to clear a value. PUT values
+        that were not provided by the user have a type of wtypes.UnsetType.
+        If the user is attempting to clear values, they should either
+        be set to None (for example in the name field) or they should be
+        reset to their default values.
+        This method is intended to handle those values that need to be set
+        back to a default value.
+        """
+        if listener.connection_limit is None:
+            listener.connection_limit = constants.DEFAULT_CONNECTION_LIMIT
+        if listener.timeout_client_data is None:
+            listener.timeout_client_data = (
+                CONF.haproxy_amphora.timeout_client_data)
+        if listener.timeout_member_connect is None:
+            listener.timeout_member_connect = (
+                CONF.haproxy_amphora.timeout_member_connect)
+        if listener.timeout_member_data is None:
+            listener.timeout_member_data = (
+                CONF.haproxy_amphora.timeout_member_data)
+        if listener.timeout_tcp_inspect is None:
+            listener.timeout_tcp_inspect = (
+                CONF.haproxy_amphora.timeout_tcp_inspect)
+        if listener.client_authentication is None:
+            listener.client_authentication = constants.CLIENT_AUTH_NONE
+
     @wsme_pecan.wsexpose(listener_types.ListenerRootResponse, wtypes.text,
                          body=listener_types.ListenerRootPUT, status_code=200)
     def put(self, id, listener_):
@@ -432,6 +460,8 @@ class ListenersController(base.BaseController):
 
         self._validate_listener_PUT(listener, db_listener)
 
+        self._set_default_on_none(listener)
+
         if listener.default_pool_id:
             self._validate_pool(context.session, load_balancer_id,
                                 listener.default_pool_id, db_listener.protocol)

octavia/api/v2/types/listener.py

@@ -111,7 +111,8 @@ class ListenerPOST(BaseListenerType):
         wtypes.IntegerType(minimum=constants.MIN_PORT_NUMBER,
                            maximum=constants.MAX_PORT_NUMBER), mandatory=True)
     connection_limit = wtypes.wsattr(
-        wtypes.IntegerType(minimum=constants.MIN_CONNECTION_LIMIT), default=-1)
+        wtypes.IntegerType(minimum=constants.MIN_CONNECTION_LIMIT),
+        default=constants.DEFAULT_CONNECTION_LIMIT)
     default_tls_container_ref = wtypes.wsattr(
         wtypes.StringType(max_length=255))
     sni_container_refs = [wtypes.StringType(max_length=255)]
@@ -198,7 +199,8 @@ class ListenerSingleCreate(BaseListenerType):
         wtypes.IntegerType(minimum=constants.MIN_PORT_NUMBER,
                            maximum=constants.MAX_PORT_NUMBER), mandatory=True)
     connection_limit = wtypes.wsattr(
-        wtypes.IntegerType(minimum=constants.MIN_CONNECTION_LIMIT), default=-1)
+        wtypes.IntegerType(minimum=constants.MIN_CONNECTION_LIMIT),
+        default=constants.DEFAULT_CONNECTION_LIMIT)
     default_tls_container_ref = wtypes.wsattr(
         wtypes.StringType(max_length=255))
     sni_container_refs = [wtypes.StringType(max_length=255)]

octavia/common/constants.py

@@ -206,6 +206,7 @@ UPDATE_HEALTH = 'UPDATE_HEALTH'
 MIN_PORT_NUMBER = 1
 MAX_PORT_NUMBER = 65535
 
+DEFAULT_CONNECTION_LIMIT = -1
 MIN_CONNECTION_LIMIT = -1
 
 MIN_WEIGHT = 0

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

@@ -1460,6 +1460,70 @@ class TestListener(base.BaseAPITest):
                          api_listener.get('client_authentication'))
         self.assertIsNone(api_listener.get('client_crl_container_ref'))
 
+    # TODO(johnsom) Fix this when there is a noop certificate manager
+    @mock.patch('octavia.common.tls_utils.cert_parser.load_certificates_data')
+    def test_update_unset_defaults(self, mock_cert_data):
+        self.conf = self.useFixture(oslo_fixture.Config(cfg.CONF))
+        self.conf.config(group='haproxy_amphora', timeout_client_data=20)
+        self.conf.config(group='haproxy_amphora', timeout_member_connect=21)
+        self.conf.config(group='haproxy_amphora', timeout_member_data=22)
+        self.conf.config(group='haproxy_amphora', timeout_tcp_inspect=23)
+
+        self.cert_manager_mock().get_secret.side_effect = [
+            sample_certs.X509_CA_CERT, sample_certs.X509_CA_CRL,
+            sample_certs.X509_CA_CERT, sample_certs.X509_CA_CRL,
+            sample_certs.X509_CA_CERT, sample_certs.X509_CA_CRL,
+            sample_certs.X509_CA_CERT, sample_certs.X509_CA_CRL]
+        cert1 = data_models.TLSContainer(certificate='cert 1')
+        mock_cert_data.return_value = {'tls_cert': cert1}
+        self.cert_manager_mock().get_secret.return_value = (
+            sample_certs.X509_CA_CERT)
+        tls_uuid = uuidutils.generate_uuid()
+        ca_tls_uuid = uuidutils.generate_uuid()
+        crl_tls_uuid = uuidutils.generate_uuid()
+        listener = self.create_listener(
+            constants.PROTOCOL_TERMINATED_HTTPS, 80, self.lb_id,
+            name='listener1', description='desc1',
+            admin_state_up=False, connection_limit=10,
+            default_tls_container_ref=tls_uuid,
+            default_pool_id=self.pool_id, tags=['old_tag'],
+            insert_headers={'X-Forwarded-For': 'true'},
+            timeout_client_data=1, timeout_member_connect=2,
+            timeout_member_data=3, timeout_tcp_inspect=4,
+            client_authentication=constants.CLIENT_AUTH_OPTIONAL,
+            client_crl_container_ref=crl_tls_uuid,
+            client_ca_tls_container_ref=ca_tls_uuid).get(self.root_tag)
+        self.set_lb_status(self.lb_id)
+        unset_params = {
+            'name': None, 'description': None, 'connection_limit': None,
+            'default_tls_container_ref': None, 'sni_container_refs': None,
+            'insert_headers': None, 'timeout_client_data': None,
+            'timeout_member_connect': None, 'timeout_member_data': None,
+            'timeout_tcp_inspect': None, 'client_ca_tls_container_ref': None,
+            'client_authentication': None, 'default_pool_id': None,
+            'client_crl_container_ref': None}
+        body = self._build_body(unset_params)
+        listener_path = self.LISTENER_PATH.format(
+            listener_id=listener['id'])
+        api_listener = self.put(listener_path, body).json.get(self.root_tag)
+
+        self.assertEqual('', api_listener['name'])
+        self.assertEqual('', api_listener['description'])
+        self.assertEqual(constants.DEFAULT_CONNECTION_LIMIT,
+                         api_listener['connection_limit'])
+        self.assertIsNone(api_listener['default_tls_container_ref'])
+        self.assertEqual([], api_listener['sni_container_refs'])
+        self.assertEqual({}, api_listener['insert_headers'])
+        self.assertEqual(20, api_listener['timeout_client_data'])
+        self.assertEqual(21, api_listener['timeout_member_connect'])
+        self.assertEqual(22, api_listener['timeout_member_data'])
+        self.assertEqual(23, api_listener['timeout_tcp_inspect'])
+        self.assertIsNone(api_listener['client_ca_tls_container_ref'])
+        self.assertIsNone(api_listener['client_crl_container_ref'])
+        self.assertEqual(constants.CLIENT_AUTH_NONE,
+                         api_listener['client_authentication'])
+        self.assertIsNone(api_listener['default_pool_id'])
+
     @mock.patch('octavia.common.tls_utils.cert_parser.load_certificates_data')
     def test_update_with_bad_ca_cert(self, mock_cert_data):
         cert1 = data_models.TLSContainer(certificate='cert 1')

releasenotes/notes/fix-listener-API-update-with-null/None-fde2ffab82e783a4.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Fixed an issue where the listener API would accept null/None values for
+    fields that must have a valid value, such as connection-limit.
+    Now when a PUT call is made to one of these fields with null as the value
+    the API will reset the field value to the field default value.