Merge pull request #215 from MaysaMacedo/lb-sg-update

Bug 1824258: Ensure LB state annotation sg matches the SG on the LB

kuryr_kubernetes/controller/handlers/lbaas.py

@@ -163,6 +163,8 @@ def __init__(self):
         self._drv_pod_project = drv_base.PodProjectDriver.get_instance()
         self._drv_pod_subnets = drv_base.PodSubnetsDriver.get_instance()
         self._drv_service_pub_ip = drv_base.ServicePubIpDriver.get_instance()
+        self._drv_project = drv_base.ServiceProjectDriver.get_instance()
+        self._drv_sg = drv_base.ServiceSecurityGroupsDriver.get_instance()
         # Note(yboaron) LBaaS driver supports 'provider' parameter in
         # Load Balancer creation flow.
         # We need to set the requested load balancer provider
@@ -281,38 +283,25 @@ def _sync_lbaas_members(self, endpoints, lbaas_state, lbaas_spec):
 
         return changed
 
-    def _sync_lbaas_sgs(self, endpoints, lbaas_state, lbaas_spec):
-        # NOTE (maysams) Need to retrieve the LBaaS Spec again due to
-        # the possibility of it being updated after the LBaaS creation
-        # process has started.
+    def _sync_lbaas_sgs(self, endpoints, lbaas_state):
         svc_link = self._get_service_link(endpoints)
         k8s = clients.get_kubernetes_client()
         service = k8s.get(svc_link)
-        lbaas_spec = utils.get_lbaas_spec(service)
 
         lb = lbaas_state.loadbalancer
-        default_sgs = config.CONF.neutron_defaults.pod_security_groups
-        # NOTE(maysams) As the endpoint and svc are annotated with the
-        # 'lbaas_spec' in two separate k8s calls, it's possible that
-        # the endpoint got annotated and the svc haven't due to controller
-        # restarts. For this case, a resourceNotReady exception is raised
-        # till the svc gets annotated with a 'lbaas_spec'.
-        if lbaas_spec:
-            lbaas_spec_sgs = lbaas_spec.security_groups_ids
-        else:
-            raise k_exc.ResourceNotReady(svc_link)
-        if lb.security_groups and lb.security_groups != lbaas_spec_sgs:
-            sgs = [lb_sg for lb_sg in lb.security_groups
-                   if lb_sg not in default_sgs]
-            if lbaas_spec_sgs != default_sgs:
-                sgs.extend(lbaas_spec_sgs)
-            lb.security_groups = sgs
+        # NOTE(maysams) It's possible that while the service annotation
+        # is added the backend pods on that service are not yet created
+        # resulting in no security groups retrieved for the service.
+        # Let's retrieve again to ensure is updated.
+        project_id = self._drv_project.get_project(service)
+        lb_sgs = self._drv_sg.get_security_groups(service, project_id)
+        lb.security_groups = lb_sgs
 
     def _add_new_members(self, endpoints, lbaas_state, lbaas_spec):
         changed = False
 
         try:
-            self._sync_lbaas_sgs(endpoints, lbaas_state, lbaas_spec)
+            self._sync_lbaas_sgs(endpoints, lbaas_state)
         except k_exc.K8sResourceNotFound:
             LOG.debug("The svc has been deleted while processing the endpoints"
                       " update. No need to add new members.")

kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py

@@ -359,6 +359,10 @@ def get_loadbalancer_pool_name(self, lb_name, namespace, svc_name):
 
 class TestLoadBalancerHandler(test_base.TestCase):
 
+    @mock.patch('kuryr_kubernetes.controller.drivers.base.'
+                'ServiceProjectDriver.get_instance')
+    @mock.patch('kuryr_kubernetes.controller.drivers.base.'
+                'ServiceSecurityGroupsDriver.get_instance')
     @mock.patch('kuryr_kubernetes.controller.handlers.lbaas'
                 '.LoadBalancerHandler._cleanup_leftover_lbaas')
     @mock.patch('kuryr_kubernetes.config.CONF')
@@ -372,20 +376,29 @@ class TestLoadBalancerHandler(test_base.TestCase):
                 '.LBaaSDriver.get_instance')
     def test_init(self, m_get_drv_lbaas, m_get_drv_project,
                   m_get_drv_subnets, m_get_drv_service_pub_ip, m_cfg,
-                  m_cleanup_leftover_lbaas):
+                  m_cleanup_leftover_lbaas,
+                  m_get_svc_sg_drv, m_get_svc_drv_project):
         m_get_drv_lbaas.return_value = mock.sentinel.drv_lbaas
         m_get_drv_project.return_value = mock.sentinel.drv_project
         m_get_drv_subnets.return_value = mock.sentinel.drv_subnets
         m_get_drv_service_pub_ip.return_value = mock.sentinel.drv_lb_ip
+        m_get_svc_drv_project.return_value = mock.sentinel.drv_svc_project
+        m_get_svc_sg_drv.return_value = mock.sentinel.drv_sg
         m_cfg.kubernetes.endpoints_driver_octavia_provider = 'default'
         handler = h_lbaas.LoadBalancerHandler()
 
         self.assertEqual(mock.sentinel.drv_lbaas, handler._drv_lbaas)
         self.assertEqual(mock.sentinel.drv_project, handler._drv_pod_project)
         self.assertEqual(mock.sentinel.drv_subnets, handler._drv_pod_subnets)
         self.assertEqual(mock.sentinel.drv_lb_ip, handler._drv_service_pub_ip)
+        self.assertEqual(mock.sentinel.drv_svc_project, handler._drv_project)
+        self.assertEqual(mock.sentinel.drv_sg, handler._drv_sg)
         self.assertIsNone(handler._lb_provider)
 
+    @mock.patch('kuryr_kubernetes.controller.drivers.base.'
+                'ServiceProjectDriver.get_instance')
+    @mock.patch('kuryr_kubernetes.controller.drivers.base.'
+                'ServiceSecurityGroupsDriver.get_instance')
     @mock.patch('kuryr_kubernetes.controller.handlers.lbaas'
                 '.LoadBalancerHandler._cleanup_leftover_lbaas')
     @mock.patch('kuryr_kubernetes.config.CONF')
@@ -399,18 +412,23 @@ def test_init(self, m_get_drv_lbaas, m_get_drv_project,
                 '.LBaaSDriver.get_instance')
     def test_init_provider_ovn(self, m_get_drv_lbaas, m_get_drv_project,
                                m_get_drv_subnets, m_get_drv_service_pub_ip,
-                               m_cfg, m_cleanup_leftover_lbaas):
+                               m_cfg, m_cleanup_leftover_lbaas,
+                               m_get_svc_sg_drv, m_get_svc_drv_project):
         m_get_drv_lbaas.return_value = mock.sentinel.drv_lbaas
         m_get_drv_project.return_value = mock.sentinel.drv_project
         m_get_drv_subnets.return_value = mock.sentinel.drv_subnets
         m_get_drv_service_pub_ip.return_value = mock.sentinel.drv_lb_ip
+        m_get_svc_drv_project.return_value = mock.sentinel.drv_svc_project
+        m_get_svc_sg_drv.return_value = mock.sentinel.drv_sg
         m_cfg.kubernetes.endpoints_driver_octavia_provider = 'ovn'
         handler = h_lbaas.LoadBalancerHandler()
 
         self.assertEqual(mock.sentinel.drv_lbaas, handler._drv_lbaas)
         self.assertEqual(mock.sentinel.drv_project, handler._drv_pod_project)
         self.assertEqual(mock.sentinel.drv_subnets, handler._drv_pod_subnets)
         self.assertEqual(mock.sentinel.drv_lb_ip, handler._drv_service_pub_ip)
+        self.assertEqual(mock.sentinel.drv_svc_project, handler._drv_project)
+        self.assertEqual(mock.sentinel.drv_sg, handler._drv_sg)
         self.assertEqual('ovn', handler._lb_provider)
 
     @mock.patch('kuryr_kubernetes.utils.get_lbaas_spec')