From d9d81d4755f89a6d45418c009206369799a52428 Mon Sep 17 00:00:00 2001 From: Luis Tomas Bolivar Date: Fri, 4 Oct 2019 13:11:56 +0200 Subject: [PATCH] Ensure ports from pool do not reference deleted SGs/NPs When a NP is deleted its associated SG should be deleted too. However, when using pools, ports may have that SG assigned, not allowing the SG deletion and consequently stoping the kuryrnetpolicy associated object deletion -- which may cause kuryr-controller crashes too Closes-Bug: 1846717 Change-Id: I7ee071b32f08af567dd92bc6e081f2cb4a3b3366 --- kuryr_kubernetes/controller/drivers/base.py | 15 ++++++++ .../controller/drivers/vif_pool.py | 36 +++++++++++++++++++ .../controller/handlers/policy.py | 18 ++++++++++ .../unit/controller/handlers/test_policy.py | 3 ++ 4 files changed, 72 insertions(+) diff --git a/kuryr_kubernetes/controller/drivers/base.py b/kuryr_kubernetes/controller/drivers/base.py index 5354c7897..43274dc6a 100644 --- a/kuryr_kubernetes/controller/drivers/base.py +++ b/kuryr_kubernetes/controller/drivers/base.py @@ -741,6 +741,21 @@ class VIFPoolDriver(PodVIFDriver): """ raise NotImplementedError() + @abc.abstractmethod + def remove_sg_from_pools(self, sg_id, net_id): + """Remove the SG from the ports associated to the pools. + + This method ensure that ports on net_id that belongs to pools and have + the referenced SG are updated to clean up their SGs and put back on + the default pool for that network. + + :param sg_id: Security Group ID that needs to be removed from pool + ports + :param net_id: Network ID associated to the pools to clean up, and + where the ports must belong to. + """ + raise NotImplementedError() + @six.add_metaclass(abc.ABCMeta) class ServicePubIpDriver(DriverBase): diff --git a/kuryr_kubernetes/controller/drivers/vif_pool.py b/kuryr_kubernetes/controller/drivers/vif_pool.py index cea85aed2..82d89bc7a 100644 --- a/kuryr_kubernetes/controller/drivers/vif_pool.py +++ b/kuryr_kubernetes/controller/drivers/vif_pool.py @@ -122,6 +122,9 @@ class NoopVIFPool(base.VIFPoolDriver): def update_vif_sgs(self, pod, sgs): self._drv_vif.update_vif_sgs(pod, sgs) + def remove_sg_from_pools(self, sg_id, net_id): + pass + def sync_pools(self): pass @@ -288,6 +291,33 @@ class BaseVIFPool(base.VIFPoolDriver): def delete_network_pools(self, net_id): raise NotImplementedError() + def remove_sg_from_pools(self, sg_id, net_id): + neutron = clients.get_neutron_client() + for pool_key, pool_ports in self._available_ports_pools.items(): + if self._get_pool_key_net(pool_key) != net_id: + continue + for sg_key, ports in pool_ports.items(): + if sg_id not in sg_key: + continue + # remove the pool associated to that SG + del self._available_ports_pools[pool_key][sg_key] + for port_id in ports: + # remove all SGs from the port to be reused + neutron.update_port( + port_id, + { + "port": { + 'security_groups': [] + } + }) + # add the port to the default pool + self._available_ports_pools[pool_key].setdefault( + tuple([]), []).append(port_id) + # NOTE(ltomasbo): as this ports were not created for this + # pool, ensuring they are used first, marking them as the + # most outdated + self._last_update[pool_key] = {tuple([]): 0} + def _create_healthcheck_file(self): # Note(ltomasbo): Create a health check file when the pre-created # ports are loaded into their corresponding pools. This file is used @@ -1025,6 +1055,12 @@ class MultiVIFPool(base.VIFPoolDriver): pod_vif_type = self._get_pod_vif_type(pod) self._vif_drvs[pod_vif_type].update_vif_sgs(pod, sgs) + def remove_sg_from_pools(self, sg_id, net_id): + for vif_drv in self._vif_drvs.values(): + if str(vif_drv) == 'NoopVIFPool': + continue + vif_drv.remove_sg_from_pools(sg_id, net_id) + def delete_network_pools(self, net_id): for vif_drv in self._vif_drvs.values(): if str(vif_drv) == 'NoopVIFPool': diff --git a/kuryr_kubernetes/controller/handlers/policy.py b/kuryr_kubernetes/controller/handlers/policy.py index 5d1718c55..42bf20e50 100644 --- a/kuryr_kubernetes/controller/handlers/policy.py +++ b/kuryr_kubernetes/controller/handlers/policy.py @@ -20,6 +20,7 @@ from kuryr_kubernetes import clients from kuryr_kubernetes import constants as k_const from kuryr_kubernetes.controller.drivers import base as drivers from kuryr_kubernetes.controller.drivers import utils as driver_utils +from kuryr_kubernetes import exceptions from kuryr_kubernetes.handlers import k8s_base from kuryr_kubernetes import utils @@ -116,6 +117,10 @@ class NetworkPolicyHandler(k8s_base.ResourceEventHandler): oslo_cfg.OptGroup('neutron_defaults')) self._drv_vif_pool.update_vif_sgs(pod, pod_sgs) + # ensure ports at the pool don't have the NP sg associated + net_id = self._get_policy_net_id(policy) + self._drv_vif_pool.remove_sg_from_pools(crd_sg, net_id) + self._drv_policy.release_network_policy(netpolicy_crd) if oslo_cfg.CONF.octavia_defaults.enforce_sg_rules: @@ -149,3 +154,16 @@ class NetworkPolicyHandler(k8s_base.ResourceEventHandler): svc_pods = driver_utils.get_pods({'selector': svc_selector}, svc_namespace).get('items') return any(pod in svc_pods for pod in affected_pods) + + def _get_policy_net_id(self, policy): + policy_ns = policy['metadata']['namespace'] + kuryrnet_name = 'ns-' + str(policy_ns) + + kubernetes = clients.get_kubernetes_client() + try: + net_crd = kubernetes.get('{}/{}'.format( + k_const.K8S_API_CRD_KURYRNETS, kuryrnet_name)) + except exceptions.K8sClientException: + LOG.exception("Kubernetes Client Exception.") + raise + return net_crd['spec']['netId'] diff --git a/kuryr_kubernetes/tests/unit/controller/handlers/test_policy.py b/kuryr_kubernetes/tests/unit/controller/handlers/test_policy.py index 0ea7f1e88..3c1c94256 100644 --- a/kuryr_kubernetes/tests/unit/controller/handlers/test_policy.py +++ b/kuryr_kubernetes/tests/unit/controller/handlers/test_policy.py @@ -78,6 +78,8 @@ class TestPolicyHandler(test_base.TestCase): self._update_vif_sgs.return_value = None self._update_lbaas_sg = self._handler._drv_lbaas.update_lbaas_sg self._update_lbaas_sg.return_value = None + self._remove_sg = self._handler._drv_vif_pool.remove_sg_from_pools + self._remove_sg.return_value = None def _get_knp_obj(self): knp_obj = { @@ -243,3 +245,4 @@ class TestPolicyHandler(test_base.TestCase): self._project_id) self._update_vif_sgs.assert_called_once_with(match_pod, sg1) self._update_lbaas_sg.assert_not_called() + self._remove_sg.assert_called_once()