From ec260571ce552f144ac91e359eac9a7e5e6a7e9b Mon Sep 17 00:00:00 2001 From: Carlos Goncalves Date: Thu, 12 Mar 2020 20:30:31 +0000 Subject: [PATCH] Fix listener update with SNI certificates SNI certificates were not being set in the database on listener update. A listener GET would not show the certificates in the sni_container_refs attribute. Also, the API was allowing set of SNI certs on non TERMINATED_HTTPS listeners. Task: 39042 Story: 2007421 Story: 2007430 Change-Id: If5b6411a0b7c75441a406234c2792ea68d35d0fe (cherry picked from commit 9e070e6e6df6468a0b8f792cb089de0d9fc180b5) (cherry picked from commit 7c6fef82ac3afbd6e47d3b2ad6341fc1accb8c54) (cherry picked from commit b9dd09450863a155490f9bbaf4b0dd1f23a00b02) --- octavia/api/drivers/utils.py | 20 ++++++++---- octavia/api/v2/types/listener.py | 1 + octavia/common/data_models.py | 8 +++++ octavia/db/models.py | 4 +-- octavia/db/repositories.py | 15 ++++++--- .../tests/functional/api/v2/test_listener.py | 32 +++++++++++++++++-- .../tests/functional/db/test_repositories.py | 10 ++++++ octavia/tests/unit/api/drivers/test_utils.py | 2 +- ...pdate-sni-containers-6595c52e2de1f621.yaml | 8 +++++ 9 files changed, 83 insertions(+), 17 deletions(-) create mode 100644 releasenotes/notes/fix-api-listener-update-sni-containers-6595c52e2de1f621.yaml diff --git a/octavia/api/drivers/utils.py b/octavia/api/drivers/utils.py index 362581c465..dff4d4a998 100644 --- a/octavia/api/drivers/utils.py +++ b/octavia/api/drivers/utils.py @@ -201,12 +201,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: - new_listener_dict['sni_container_refs'] = new_listener_dict.pop( - 'sni_containers') - 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')) @@ -261,6 +255,20 @@ def listener_dict_to_provider_dict(listener_dict, for_delete=False): listener_obj.client_crl_container_id) new_listener_dict['client_crl_container_data'] = crl_file + # 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'] diff --git a/octavia/api/v2/types/listener.py b/octavia/api/v2/types/listener.py index a3cb641ce9..3735780284 100644 --- a/octavia/api/v2/types/listener.py +++ b/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 = {} diff --git a/octavia/common/data_models.py b/octavia/common/data_models.py index bebd7be5de..1542e90e3f 100644 --- a/octavia/common/data_models.py +++ b/octavia/common/data_models.py @@ -530,6 +530,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): diff --git a/octavia/db/models.py b/octavia/db/models.py index 7d2fe54f98..ddd91c60b9 100644 --- a/octavia/db/models.py +++ b/octavia/db/models.py @@ -491,8 +491,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', diff --git a/octavia/db/repositories.py b/octavia/db/repositories.py index 438c5108fd..a142c17d37 100644 --- a/octavia/db/repositories.py +++ b/octavia/db/repositories.py @@ -1050,11 +1050,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] listener_db.update(model_kwargs) def create(self, session, **model_kwargs): diff --git a/octavia/tests/functional/api/v2/test_listener.py b/octavia/tests/functional/api/v2/test_listener.py index fc4c4af52a..5609847628 100644 --- a/octavia/tests/functional/api/v2/test_listener.py +++ b/octavia/tests/functional/api/v2/test_listener.py @@ -2196,7 +2196,29 @@ 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_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']) + get_listener = self.get(listener_path).json['listener'] + self.assertEqual([], get_listener.get('sni_container_refs')) + 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]} @@ -2209,8 +2231,12 @@ class TestListener(base.BaseAPITest): listener_id=listener['listener']['id']) get_listener = self.get(listener_path).json['listener'] self.assertEqual([], get_listener.get('sni_container_refs')) - self.put(listener_path, - self._build_body({'sni_container_refs': [sni_id1, sni_id2]})) + 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')) diff --git a/octavia/tests/functional/db/test_repositories.py b/octavia/tests/functional/db/test_repositories.py index 6a4d19c207..fe88143774 100644 --- a/octavia/tests/functional/db/test_repositories.py +++ b/octavia/tests/functional/db/test_repositories.py @@ -2363,6 +2363,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) diff --git a/octavia/tests/unit/api/drivers/test_utils.py b/octavia/tests/unit/api/drivers/test_utils.py index 3c534b4e8b..99284bc2f5 100644 --- a/octavia/tests/unit/api/drivers/test_utils.py +++ b/octavia/tests/unit/api/drivers/test_utils.py @@ -291,7 +291,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) diff --git a/releasenotes/notes/fix-api-listener-update-sni-containers-6595c52e2de1f621.yaml b/releasenotes/notes/fix-api-listener-update-sni-containers-6595c52e2de1f621.yaml new file mode 100644 index 0000000000..e8366f7ffd --- /dev/null +++ b/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.