diff --git a/octavia/api/drivers/utils.py b/octavia/api/drivers/utils.py index dde3dca9e7..e748f6c7a1 100644 --- a/octavia/api/drivers/utils.py +++ b/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'] diff --git a/octavia/api/v2/types/listener.py b/octavia/api/v2/types/listener.py index d88bbf466a..da82de66ed 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 0e465d7c2f..b126111a90 100644 --- a/octavia/common/data_models.py +++ b/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): diff --git a/octavia/db/models.py b/octavia/db/models.py index 8b889c7512..451b81466e 100644 --- a/octavia/db/models.py +++ b/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', diff --git a/octavia/db/repositories.py b/octavia/db/repositories.py index f47732dacb..9eb7d57b6c 100644 --- a/octavia/db/repositories.py +++ b/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. diff --git a/octavia/tests/common/sample_data_models.py b/octavia/tests/common/sample_data_models.py index d1ac32b3a2..fe0e474d9e 100644 --- a/octavia/tests/common/sample_data_models.py +++ b/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: {}, diff --git a/octavia/tests/functional/api/v2/test_listener.py b/octavia/tests/functional/api/v2/test_listener.py index ad4bfb249c..70b3f0d552 100644 --- a/octavia/tests/functional/api/v2/test_listener.py +++ b/octavia/tests/functional/api/v2/test_listener.py @@ -2249,7 +2249,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]} @@ -2262,8 +2284,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 bff9790a79..29ef1a8ca4 100644 --- a/octavia/tests/functional/db/test_repositories.py +++ b/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) diff --git a/octavia/tests/unit/api/drivers/test_utils.py b/octavia/tests/unit/api/drivers/test_utils.py index 2a18616def..204aabe8f4 100644 --- a/octavia/tests/unit/api/drivers/test_utils.py +++ b/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) 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.