Merge "Fix listener update with SNI certificates"

octavia/api/drivers/utils.py

@@ -221,19 +221,6 @@ def listener_dict_to_provider_dict(listener_dict, for_delete=False):
     if 'tls_certificate_id' in new_listener_dict:
         new_listener_dict['default_tls_container_ref'] = new_listener_dict.pop(
             'tls_certificate_id')
-    if 'sni_containers' in new_listener_dict:
-        sni_refs = []
-        sni_containers = new_listener_dict.pop('sni_containers')
-        for sni in sni_containers:
-            if 'tls_container_id' in sni:
-                sni_refs.append(sni['tls_container_id'])
-            else:
-                raise exceptions.ValidationException(
-                    detail=_('Invalid SNI container on listener'))
-        new_listener_dict['sni_container_refs'] = sni_refs
-    if 'sni_container_refs' in listener_dict:
-        listener_dict['sni_containers'] = listener_dict.pop(
-            'sni_container_refs')
     if 'client_ca_tls_certificate_id' in new_listener_dict:
         new_listener_dict['client_ca_tls_container_ref'] = (
             new_listener_dict.pop('client_ca_tls_certificate_id'))
@@ -246,8 +233,6 @@ def listener_dict_to_provider_dict(listener_dict, for_delete=False):
         SNI_objs = []
         for sni in listener_obj.sni_containers:
             if isinstance(sni, dict):
-                if 'listener' in sni:
-                    del sni['listener']
                 sni_obj = data_models.SNI(**sni)
                 SNI_objs.append(sni_obj)
             elif isinstance(sni, str):
@@ -298,6 +283,20 @@ def listener_dict_to_provider_dict(listener_dict, for_delete=False):
         new_listener_dict['allowed_cidrs'] = [cidr_dict['cidr'] for
                                               cidr_dict in cidrs_dict_list]
 
+    # Format the sni_containers -> sni_container_refs
+    sni_containers = new_listener_dict.pop('sni_containers', None)
+    if sni_containers:
+        new_listener_dict['sni_container_refs'] = []
+        for sni in sni_containers:
+            if isinstance(sni, dict):
+                new_listener_dict['sni_container_refs'].append(
+                    sni['tls_container_id'])
+            elif isinstance(sni, str):
+                new_listener_dict['sni_container_refs'].append(sni)
+            else:
+                raise exceptions.ValidationException(
+                    detail=_('Invalid SNI container on listener'))
+
     # Remove the DB back references
     if 'load_balancer' in new_listener_dict:
         del new_listener_dict['load_balancer']

octavia/api/v2/types/listener.py

@@ -28,6 +28,7 @@ class BaseListenerType(types.BaseType):
     _type_to_model_map = {
         'admin_state_up': 'enabled',
         'default_tls_container_ref': 'tls_certificate_id',
+        'sni_container_refs': 'sni_containers',
         'client_ca_tls_container_ref': 'client_ca_tls_certificate_id',
         'client_crl_container_ref': 'client_crl_container_id'}
     _child_map = {}

octavia/common/data_models.py

@@ -540,6 +540,14 @@ class SNI(BaseDataModel):
         self.listener = listener
         self.tls_container_id = tls_container_id
 
+    # SQLAlchemy kindly attaches the whole listener object so
+    # let's keep this simple by overriding the to_dict for this
+    # object. Otherwise we recurse down the "ghost" listener object.
+    def to_dict(self, **kwargs):
+        return {'tls_container_id': self.tls_container_id,
+                'listener_id': self.listener_id,
+                'position': self.position}
+
 
 class TLSContainer(BaseDataModel):
 

octavia/db/models.py

@@ -502,8 +502,8 @@ class Listener(base_models.BASE, base_models.IdMixin,
     default_pool = orm.relationship("Pool", uselist=False,
                                     back_populates="_default_listeners")
     sni_containers = orm.relationship(
-        'SNI', cascade='delete', uselist=True,
-        backref=orm.backref('listener', uselist=False))
+        'SNI', cascade='all,delete-orphan',
+        uselist=True, backref=orm.backref('listener', uselist=False))
 
     l7policies = orm.relationship(
         'L7Policy', uselist=True, order_by='L7Policy.position',

octavia/db/repositories.py

@@ -1073,11 +1073,16 @@ class ListenerRepository(BaseRepository):
             default_pool_id = model_kwargs.get('default_pool_id')
             if default_pool_id:
                 self._pool_check(session, default_pool_id, listener_id=id)
-            sni_containers = model_kwargs.pop('sni_containers', [])
-            for container_ref in sni_containers:
-                sni = models.SNI(listener_id=id,
-                                 tls_certificate_id=container_ref)
-                listener_db.sni_containers.append(sni)
+            if 'sni_containers' in model_kwargs:
+                # sni_container_refs is being updated. It is either being set
+                # or unset/cleared. We need to update in DB side.
+                containers = model_kwargs.pop('sni_containers', []) or []
+                listener_db.sni_containers = []
+                if containers:
+                    listener_db.sni_containers = [
+                        models.SNI(listener_id=id,
+                                   tls_container_id=container_ref)
+                        for container_ref in containers]
             if 'allowed_cidrs' in model_kwargs:
                 # allowed_cidrs is being updated. It is either being set or
                 # unset/cleared. We need to update in DB side.

octavia/tests/common/sample_data_models.py

@@ -454,9 +454,8 @@ class SampleDriverDataModels(object):
             lib_consts.CONNECTION_LIMIT: 10000,
             constants.TLS_CERTIFICATE_ID: self.default_tls_container_ref,
             lib_consts.DEFAULT_POOL: self.test_pool1_dict,
-            constants.SNI_CONTAINERS: [
-                {constants.TLS_CONTAINER_ID: self.sni_container_ref_1},
-                {constants.TLS_CONTAINER_ID: self.sni_container_ref_2}],
+            constants.SNI_CONTAINERS: [self.sni_container_ref_1,
+                                       self.sni_container_ref_2],
             constants.PEER_PORT: 55,
             lib_consts.L7POLICIES: self.test_l7policies,
             lib_consts.INSERT_HEADERS: {},

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

@@ -2249,14 +2249,15 @@ class TestListener(base.BaseAPITest):
 
     # 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_with_sni_data(self, mock_cert_data):
+    def test_update_tls_terminated_with_sni_data(self, mock_cert_data):
         cert2 = data_models.TLSContainer(certificate='cert 2')
         cert3 = data_models.TLSContainer(certificate='cert 3')
         mock_cert_data.return_value = {'sni_certs': [cert2, cert3]}
         sni_id1 = uuidutils.generate_uuid()
         sni_id2 = uuidutils.generate_uuid()
-        listener = self.create_listener(constants.PROTOCOL_HTTP, 80,
-                                        self.lb_id)
+        listener = self.create_listener(
+            constants.PROTOCOL_TERMINATED_HTTPS, 80, self.lb_id,
+            default_tls_container_ref=uuidutils.generate_uuid())
         self.set_lb_status(self.lb_id)
         listener_path = self.LISTENER_PATH.format(
             listener_id=listener['listener']['id'])
@@ -2265,6 +2266,31 @@ class TestListener(base.BaseAPITest):
         self.put(listener_path,
                  self._build_body({'sni_container_refs': [sni_id1, sni_id2]}))
         get_listener = self.get(listener_path).json['listener']
+        self.assertItemsEqual([sni_id1, sni_id2],
+                              get_listener.get('sni_container_refs'))
+
+    # 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_non_tls_terminated_with_sni_data(self, mock_cert_data):
+        cert2 = data_models.TLSContainer(certificate='cert 2')
+        cert3 = data_models.TLSContainer(certificate='cert 3')
+        mock_cert_data.return_value = {'sni_certs': [cert2, cert3]}
+        sni_id1 = uuidutils.generate_uuid()
+        sni_id2 = uuidutils.generate_uuid()
+        listener = self.create_listener(constants.PROTOCOL_HTTP, 80,
+                                        self.lb_id)
+        self.set_lb_status(self.lb_id)
+        listener_path = self.LISTENER_PATH.format(
+            listener_id=listener['listener']['id'])
+        get_listener = self.get(listener_path).json['listener']
+        self.assertEqual([], get_listener.get('sni_container_refs'))
+        body = self._build_body({'sni_container_refs': [sni_id1, sni_id2]})
+        response = self.put(listener_path, body, status=400).json
+        self.assertEqual(
+            "Validation failure: Certificate container references are only "
+            "allowed on TERMINATED_HTTPS protocol listeners.",
+            response['faultstring'])
+        get_listener = self.get(listener_path).json['listener']
         self.assertEqual([], get_listener.get('sni_container_refs'))
 
     # TODO(johnsom) Fix this when there is a noop certificate manager

octavia/tests/functional/db/test_repositories.py

@@ -2384,6 +2384,16 @@ class TestListenerRepositoryTest(BaseRepositoryTest):
         new_listener = self.listener_repo.get(self.session, id=listener.id)
         self.assertEqual(name_change, new_listener.name)
 
+    def test_update_with_sni(self):
+        listener = self.create_listener(self.FAKE_UUID_1, 80)
+        container1 = {'listener_id': listener.id,
+                      'tls_container_id': self.FAKE_UUID_2}
+        container1_dm = models.SNI(**container1)
+        self.listener_repo.update(self.session, listener.id,
+                                  sni_containers=[self.FAKE_UUID_2])
+        new_listener = self.listener_repo.get(self.session, id=listener.id)
+        self.assertIn(container1_dm, new_listener.sni_containers)
+
     def test_delete(self):
         listener = self.create_listener(self.FAKE_UUID_1, 80)
         self.listener_repo.delete(self.session, id=listener.id)

octavia/tests/unit/api/drivers/test_utils.py

@@ -288,7 +288,7 @@ class TestUtils(base.TestCase):
                                        'sni_certs': [cert2, cert3]}
         # Test with bad SNI content
         test_listener = copy.deepcopy(self.sample_data.test_listener1_dict)
-        test_listener['sni_containers'] = [{}]
+        test_listener['sni_containers'] = [()]
         self.assertRaises(exceptions.ValidationException,
                           utils.listener_dict_to_provider_dict,
                           test_listener)

releasenotes/notes/fix-api-listener-update-sni-containers-6595c52e2de1f621.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Fixed an issue where setting of SNI containers were not being applied on
+    listener update API calls.
+  - |
+    Fixed an Octavia API validation on listener update where SNI containers
+    could be set on non-TERMINATED_HTTPS listeners.