From ae1d1dd51a32d0bafcb9c433bc3a1466a91c2d76 Mon Sep 17 00:00:00 2001 From: Maysa Macedo Date: Wed, 3 Apr 2019 14:35:22 +0000 Subject: [PATCH] Fix LBaaS SG rules update The LBaaS SG update is failing when the pods selected by the selector in the rule block are removed after the pod, on which the policy is enforced, is removed. This commit fixes the issue by changing from LBaaSServiceSpec object to LBaaSLoadBalancer, which is the object type expected by '_apply_members_security_groups' function. Change-Id: I17f2f632e02bc0f46ccc7434173acce68aef957b Closes-Bug: 1823022 --- .../controller/drivers/lbaasv2.py | 15 +++++-- kuryr_kubernetes/controller/handlers/lbaas.py | 33 ++------------- .../unit/controller/handlers/test_lbaas.py | 40 ++++++++++++------- kuryr_kubernetes/utils.py | 28 +++++++++++++ 4 files changed, 67 insertions(+), 49 deletions(-) diff --git a/kuryr_kubernetes/controller/drivers/lbaasv2.py b/kuryr_kubernetes/controller/drivers/lbaasv2.py index ecbfa6856..1e2033e93 100644 --- a/kuryr_kubernetes/controller/drivers/lbaasv2.py +++ b/kuryr_kubernetes/controller/drivers/lbaasv2.py @@ -940,12 +940,19 @@ class LBaaSv2Driver(base.LBaaSDriver): svc_ports = service['spec']['ports'] lbaas_name = "%s/%s" % (svc_namespace, svc_name) - lbaas = utils.get_lbaas_spec(service) + + endpoints_link = utils.get_endpoints_link(service) + k8s = clients.get_kubernetes_client() + endpoint = k8s.get(endpoints_link) + + lbaas = utils.get_lbaas_state(endpoint) if not lbaas: return - lbaas.security_groups_ids = sgs - utils.set_lbaas_spec(service, lbaas) + lbaas_obj = lbaas.loadbalancer + lbaas_obj.security_groups = sgs + + utils.set_lbaas_state(endpoint, lbaas) for port in svc_ports: port_protocol = port['protocol'] @@ -953,6 +960,6 @@ class LBaaSv2Driver(base.LBaaSDriver): target_port = port['targetPort'] sg_rule_name = "%s:%s:%s" % (lbaas_name, port_protocol, lbaas_port) - self._apply_members_security_groups(lbaas, lbaas_port, + self._apply_members_security_groups(lbaas_obj, lbaas_port, target_port, port_protocol, sg_rule_name, sgs) diff --git a/kuryr_kubernetes/controller/handlers/lbaas.py b/kuryr_kubernetes/controller/handlers/lbaas.py index f29077720..34a40a908 100644 --- a/kuryr_kubernetes/controller/handlers/lbaas.py +++ b/kuryr_kubernetes/controller/handlers/lbaas.py @@ -177,7 +177,7 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler): endpoints['metadata']['name']) return - lbaas_state = self._get_lbaas_state(endpoints) + lbaas_state = utils.get_lbaas_state(endpoints) if not lbaas_state: lbaas_state = obj_lbaas.LBaaSState() @@ -209,7 +209,7 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler): # failing items, or validate configuration) to prevent annotation # being out of sync with the actual Neutron state. try: - self._set_lbaas_state(endpoints, lbaas_state) + utils.set_lbaas_state(endpoints, lbaas_state) except k_exc.K8sResourceNotFound: # Note(yboaron) It's impossible to store neutron resources # in K8S object since object was deleted. In that case @@ -220,7 +220,7 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler): def on_deleted(self, endpoints, lbaas_state=None): if lbaas_state is None: - lbaas_state = self._get_lbaas_state(endpoints) + lbaas_state = utils.get_lbaas_state(endpoints) if not lbaas_state: return # NOTE(ivc): deleting pool deletes its members @@ -606,30 +606,3 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler): obj = obj_lbaas.LBaaSServiceSpec.obj_from_primitive(obj_dict) LOG.debug("Got LBaaSServiceSpec from annotation: %r", obj) return obj - - def _set_lbaas_state(self, endpoints, lbaas_state): - # TODO(ivc): extract annotation interactions - if lbaas_state is None: - LOG.debug("Removing LBaaSState annotation: %r", lbaas_state) - annotation = None - else: - lbaas_state.obj_reset_changes(recursive=True) - LOG.debug("Setting LBaaSState annotation: %r", lbaas_state) - annotation = jsonutils.dumps(lbaas_state.obj_to_primitive(), - sort_keys=True) - k8s = clients.get_kubernetes_client() - k8s.annotate(endpoints['metadata']['selfLink'], - {k_const.K8S_ANNOTATION_LBAAS_STATE: annotation}, - resource_version=endpoints['metadata']['resourceVersion']) - - def _get_lbaas_state(self, endpoints): - # TODO(ivc): same as '_set_lbaas_state' - try: - annotations = endpoints['metadata']['annotations'] - annotation = annotations[k_const.K8S_ANNOTATION_LBAAS_STATE] - except KeyError: - return None - obj_dict = jsonutils.loads(annotation) - obj = obj_lbaas.LBaaSState.obj_from_primitive(obj_dict) - LOG.debug("Got LBaaSState from annotation: %r", obj) - return obj diff --git a/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py b/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py index 24d4b22cb..daf0266b5 100644 --- a/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py +++ b/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py @@ -441,7 +441,9 @@ class TestLoadBalancerHandler(test_base.TestCase): self.assertEqual(mock.sentinel.drv_lb_ip, handler._drv_service_pub_ip) self.assertEqual('ovn', handler._lb_provider) - def test_on_present(self): + @mock.patch('kuryr_kubernetes.utils.set_lbaas_state') + @mock.patch('kuryr_kubernetes.utils.get_lbaas_state') + def test_on_present(self, m_get_lbaas_state, m_set_lbaas_state): lbaas_spec = mock.sentinel.lbaas_spec lbaas_spec.type = 'DummyType' lbaas_spec.lb_ip = "1.2.3.4" @@ -461,7 +463,7 @@ class TestLoadBalancerHandler(test_base.TestCase): m_handler = mock.Mock(spec=h_lbaas.LoadBalancerHandler) m_handler._get_lbaas_spec.return_value = lbaas_spec m_handler._should_ignore.return_value = False - m_handler._get_lbaas_state.return_value = lbaas_state + m_get_lbaas_state.return_value = lbaas_state m_handler._sync_lbaas_members.return_value = True m_handler._drv_service_pub_ip = m_drv_service_pub_ip @@ -469,10 +471,10 @@ class TestLoadBalancerHandler(test_base.TestCase): m_handler._get_lbaas_spec.assert_called_once_with(endpoints) m_handler._should_ignore.assert_called_once_with(endpoints, lbaas_spec) - m_handler._get_lbaas_state.assert_called_once_with(endpoints) + m_get_lbaas_state.assert_called_once_with(endpoints) m_handler._sync_lbaas_members.assert_called_once_with( endpoints, lbaas_state, lbaas_spec) - m_handler._set_lbaas_state.assert_called_once_with( + m_set_lbaas_state.assert_called_once_with( endpoints, lbaas_state) m_handler._update_lb_status.assert_not_called() @@ -483,7 +485,10 @@ class TestLoadBalancerHandler(test_base.TestCase): lbaas_state.service_pub_ip_info = None return True - def test_on_present_loadbalancer_service(self): + @mock.patch('kuryr_kubernetes.utils.set_lbaas_state') + @mock.patch('kuryr_kubernetes.utils.get_lbaas_state') + def test_on_present_loadbalancer_service(self, m_get_lbaas_state, + m_set_lbaas_state): lbaas_spec = mock.sentinel.lbaas_spec lbaas_spec.type = 'LoadBalancer' lbaas_spec.lb_ip = "1.2.3.4" @@ -508,7 +513,7 @@ class TestLoadBalancerHandler(test_base.TestCase): m_handler = mock.Mock(spec=h_lbaas.LoadBalancerHandler) m_handler._get_lbaas_spec.return_value = lbaas_spec m_handler._should_ignore.return_value = False - m_handler._get_lbaas_state.return_value = lbaas_state + m_get_lbaas_state.return_value = lbaas_state m_handler._sync_lbaas_members = self._fake_sync_lbaas_members m_handler._drv_service_pub_ip = m_drv_service_pub_ip @@ -516,12 +521,15 @@ class TestLoadBalancerHandler(test_base.TestCase): m_handler._get_lbaas_spec.assert_called_once_with(endpoints) m_handler._should_ignore.assert_called_once_with(endpoints, lbaas_spec) - m_handler._get_lbaas_state.assert_called_once_with(endpoints) - m_handler._set_lbaas_state.assert_called_once_with( + m_get_lbaas_state.assert_called_once_with(endpoints) + m_set_lbaas_state.assert_called_once_with( endpoints, lbaas_state) m_handler._update_lb_status.assert_called() - def test_on_present_rollback(self): + @mock.patch('kuryr_kubernetes.utils.set_lbaas_state') + @mock.patch('kuryr_kubernetes.utils.get_lbaas_state') + def test_on_present_rollback(self, m_get_lbaas_state, + m_set_lbaas_state): lbaas_spec = mock.sentinel.lbaas_spec lbaas_spec.type = 'ClusterIp' lbaas_spec.lb_ip = '1.2.3.4' @@ -540,26 +548,28 @@ class TestLoadBalancerHandler(test_base.TestCase): m_handler = mock.Mock(spec=h_lbaas.LoadBalancerHandler) m_handler._get_lbaas_spec.return_value = lbaas_spec m_handler._should_ignore.return_value = False - m_handler._get_lbaas_state.return_value = lbaas_state + m_get_lbaas_state.return_value = lbaas_state m_handler._sync_lbaas_members.return_value = True - m_handler._set_lbaas_state.side_effect = ( + m_set_lbaas_state.side_effect = ( k_exc.K8sResourceNotFound('ep')) m_handler._drv_service_pub_ip = m_drv_service_pub_ip h_lbaas.LoadBalancerHandler.on_present(m_handler, endpoints) m_handler._get_lbaas_spec.assert_called_once_with(endpoints) m_handler._should_ignore.assert_called_once_with(endpoints, lbaas_spec) - m_handler._get_lbaas_state.assert_called_once_with(endpoints) + m_get_lbaas_state.assert_called_once_with(endpoints) m_handler._sync_lbaas_members.assert_called_once_with( endpoints, lbaas_state, lbaas_spec) - m_handler._set_lbaas_state.assert_called_once_with( + m_set_lbaas_state.assert_called_once_with( endpoints, lbaas_state) m_handler.on_deleted.assert_called_once_with( endpoints, lbaas_state) + @mock.patch('kuryr_kubernetes.utils.get_lbaas_state') @mock.patch('kuryr_kubernetes.objects.lbaas' '.LBaaSServiceSpec') - def test_on_cascade_deleted_lb_service(self, m_svc_spec_ctor): + def test_on_cascade_deleted_lb_service(self, m_svc_spec_ctor, + m_get_lbaas_state): endpoints = mock.sentinel.endpoints empty_spec = mock.sentinel.empty_spec lbaas_state = mock.Mock() @@ -568,7 +578,7 @@ class TestLoadBalancerHandler(test_base.TestCase): m_svc_spec_ctor.return_value = empty_spec m_handler = mock.Mock(spec=h_lbaas.LoadBalancerHandler) - m_handler._get_lbaas_state.return_value = lbaas_state + m_get_lbaas_state.return_value = lbaas_state m_handler._drv_lbaas = mock.Mock() m_handler._drv_service_pub_ip = mock.Mock() diff --git a/kuryr_kubernetes/utils.py b/kuryr_kubernetes/utils.py index 87c3cb1e1..311547d27 100644 --- a/kuryr_kubernetes/utils.py +++ b/kuryr_kubernetes/utils.py @@ -252,6 +252,34 @@ def set_lbaas_spec(service, lbaas_spec): resource_version=service['metadata']['resourceVersion']) +def get_lbaas_state(endpoint): + try: + annotations = endpoint['metadata']['annotations'] + annotation = annotations[constants.K8S_ANNOTATION_LBAAS_STATE] + except KeyError: + return None + obj_dict = jsonutils.loads(annotation) + obj = obj_lbaas.LBaaSState.obj_from_primitive(obj_dict) + LOG.debug("Got LBaaSState from annotation: %r", obj) + return obj + + +def set_lbaas_state(endpoints, lbaas_state): + # TODO(ivc): extract annotation interactions + if lbaas_state is None: + LOG.debug("Removing LBaaSState annotation: %r", lbaas_state) + annotation = None + else: + lbaas_state.obj_reset_changes(recursive=True) + LOG.debug("Setting LBaaSState annotation: %r", lbaas_state) + annotation = jsonutils.dumps(lbaas_state.obj_to_primitive(), + sort_keys=True) + k8s = clients.get_kubernetes_client() + k8s.annotate(endpoints['metadata']['selfLink'], + {constants.K8S_ANNOTATION_LBAAS_STATE: annotation}, + resource_version=endpoints['metadata']['resourceVersion']) + + def get_endpoints_link(service): svc_link = service['metadata']['selfLink'] link_parts = svc_link.split('/')