From b9dd09450863a155490f9bbaf4b0dd1f23a00b02 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) --- octavia/api/drivers/utils.py | 29 ++++++++--------- octavia/api/v2/types/listener.py | 1 + octavia/common/data_models.py | 8 +++++ octavia/db/models.py | 4 +-- octavia/db/repositories.py | 15 ++++++--- octavia/tests/common/sample_data_models.py | 5 ++- .../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 +++++ 10 files changed, 85 insertions(+), 29 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 e7c717fd60..100cd90e0f 100644 --- a/octavia/api/drivers/utils.py +++ b/octavia/api/drivers/utils.py @@ -219,19 +219,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')) @@ -244,8 +231,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, six.string_types): @@ -296,6 +281,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'] diff --git a/octavia/api/v2/types/listener.py b/octavia/api/v2/types/listener.py index 9dc513b5ba..2d3b8c0e89 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 0756d685f8..1c6424b2f9 100644 --- a/octavia/common/data_models.py +++ b/octavia/common/data_models.py @@ -533,6 +533,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 6b943dd6fc..43135b7f9a 100644 --- a/octavia/db/models.py +++ b/octavia/db/models.py @@ -492,8 +492,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 eeb88e17f6..bc12620b7f 100644 --- a/octavia/db/repositories.py +++ b/octavia/db/repositories.py @@ -1071,11 +1071,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. diff --git a/octavia/tests/common/sample_data_models.py b/octavia/tests/common/sample_data_models.py index ecb9d78483..1c004f23fc 100644 --- a/octavia/tests/common/sample_data_models.py +++ b/octavia/tests/common/sample_data_models.py @@ -449,9 +449,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: {}, diff --git a/octavia/tests/functional/api/v2/test_listener.py b/octavia/tests/functional/api/v2/test_listener.py index aff9fb7e37..62ef14a668 100644 --- a/octavia/tests/functional/api/v2/test_listener.py +++ b/octavia/tests/functional/api/v2/test_listener.py @@ -2248,7 +2248,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]} @@ -2261,8 +2283,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 bf26c84593..fb07627544 100644 --- a/octavia/tests/functional/db/test_repositories.py +++ b/octavia/tests/functional/db/test_repositories.py @@ -2364,6 +2364,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 87265ac8f8..37e0a6b728 100644 --- a/octavia/tests/unit/api/drivers/test_utils.py +++ b/octavia/tests/unit/api/drivers/test_utils.py @@ -304,7 +304,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.