From 9c2fcbc3e3e72954b0e095e3fc5af46a6f678901 Mon Sep 17 00:00:00 2001 From: Maysa Macedo Date: Tue, 5 Feb 2019 20:49:08 +0000 Subject: [PATCH] Fix SG rules on targetPort update After applying a Network Policy and updating an existent Service so that all 'targetPorts' are allowed by the policy, the SG rules are not being created with the required 'remote_ip_prefix'. Also, when the service is again updated with a 'targetPort' that is not allowed by the policy the respective SG rule is not deleted. This commit fixes the issue by associating 'targetPort' field to the 'LBaaSPortSpec' versioned object, which allows Kuryr to accounts for changes in not only 'name', 'port' and 'protocol' Kubernetes services' fields, but also 'targetPorts'. In addition, the LBaaS SG from the LBaaS state annotation is updated to match the SG stated in the LBaaS spec annotation, which has the updated SG to be applied. Closes-Bug: #1814920 Change-Id: Ifcdd1889a813c1eb078064facfb2ede83a179887 --- kuryr_kubernetes/controller/handlers/lbaas.py | 32 ++++++++++++--- kuryr_kubernetes/objects/lbaas.py | 5 ++- .../unit/controller/handlers/test_lbaas.py | 39 +++++++++++-------- kuryr_kubernetes/tests/unit/test_object.py | 2 +- 4 files changed, 55 insertions(+), 23 deletions(-) diff --git a/kuryr_kubernetes/controller/handlers/lbaas.py b/kuryr_kubernetes/controller/handlers/lbaas.py index fd0f508fd..74469a89c 100644 --- a/kuryr_kubernetes/controller/handlers/lbaas.py +++ b/kuryr_kubernetes/controller/handlers/lbaas.py @@ -122,7 +122,8 @@ class LBaaSSpecHandler(k8s_base.ResourceEventHandler): def _get_service_ports(self, service): return [{'name': port.get('name'), 'protocol': port.get('protocol', 'TCP'), - 'port': port['port']} + 'port': port['port'], + 'targetPort': port['targetPort']} for port in service['spec']['ports']] def _has_port_changes(self, service, lbaas_spec): @@ -131,7 +132,10 @@ class LBaaSSpecHandler(k8s_base.ResourceEventHandler): fields = obj_lbaas.LBaaSPortSpec.fields svc_port_set = {tuple(port[attr] for attr in fields) for port in self._get_service_ports(service)} - spec_port_set = {tuple(getattr(port, attr) for attr in fields) + + spec_port_set = {tuple(getattr(port, attr) + for attr in fields + if port.obj_attr_is_set(attr)) for port in lbaas_spec.ports} if svc_port_set != spec_port_set: @@ -311,11 +315,17 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler): self._is_lbaas_spec_in_sync(endpoints, lbaas_spec)) def _is_lbaas_spec_in_sync(self, endpoints, lbaas_spec): - # REVISIT(ivc): consider other options instead of using 'name' - ep_ports = list(set(port.get('name') + ports = lbaas_spec.ports + ep_ports = list(set((port.get('name'), port.get('port')) + if ports[0].obj_attr_is_set('targetPort') + else port.get('name') for subset in endpoints.get('subsets', []) for port in subset.get('ports', []))) - spec_ports = [port.name for port in lbaas_spec.ports] + + spec_ports = [(port.name, port.targetPort) + if port.obj_attr_is_set('targetPort') + else port.name + for port in ports] return sorted(ep_ports) == sorted(spec_ports) @@ -352,6 +362,7 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler): pool_by_lsnr_port = {(lsnr_by_id[p.listener_id].protocol, lsnr_by_id[p.listener_id].port): p for p in lbaas_state.pools} + # NOTE(yboaron): Since LBaaSv2 doesn't support UDP load balancing, # the LBaaS driver will return 'None' in case of UDP port # listener creation. @@ -387,6 +398,7 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler): except KeyError: LOG.debug("No pool found for port: %r", port_name) continue + if (target_ip, target_port, pool.id) in current_targets: continue # TODO(apuimedo): Do not pass subnet_id at all when in @@ -410,6 +422,7 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler): listener_port = lsnr_by_id[pool.listener_id].port else: listener_port = None + member = self._drv_lbaas.ensure_member( loadbalancer=lbaas_state.loadbalancer, pool=pool, @@ -636,6 +649,15 @@ class LoadBalancerHandler(k8s_base.ResourceEventHandler): lbaas_state.service_pub_ip_info = None changed = True + default_sgs = config.CONF.neutron_defaults.pod_security_groups + lbaas_spec_sgs = lbaas_spec.security_groups_ids + 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 + lbaas_state.loadbalancer = lb return changed diff --git a/kuryr_kubernetes/objects/lbaas.py b/kuryr_kubernetes/objects/lbaas.py index 7e59f9a52..87352ea2e 100644 --- a/kuryr_kubernetes/objects/lbaas.py +++ b/kuryr_kubernetes/objects/lbaas.py @@ -120,12 +120,15 @@ class LBaaSState(k_obj.KuryrK8sObjectBase): @obj_base.VersionedObjectRegistry.register class LBaaSPortSpec(k_obj.KuryrK8sObjectBase): - VERSION = '1.0' + VERSION = '1.1' + # Version 1.0: Initial version + # Version 1.1: Added targetPort field. fields = { 'name': obj_fields.StringField(nullable=True), 'protocol': obj_fields.StringField(), 'port': obj_fields.IntegerField(), + 'targetPort': obj_fields.IntegerField(), } diff --git a/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py b/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py index 9cb315c31..bec577b34 100644 --- a/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py +++ b/kuryr_kubernetes/tests/unit/controller/handlers/test_lbaas.py @@ -201,12 +201,12 @@ class TestLBaaSSpecHandler(test_base.TestCase): def test_get_service_ports(self): m_handler = mock.Mock(spec=h_lbaas.LBaaSSpecHandler) service = {'spec': {'ports': [ - {'port': 1}, - {'port': 2, 'name': 'X', 'protocol': 'UDP'} + {'port': 1, 'targetPort': 1}, + {'port': 2, 'name': 'X', 'protocol': 'UDP', 'targetPort': 2} ]}} expected_ret = [ - {'port': 1, 'name': None, 'protocol': 'TCP'}, - {'port': 2, 'name': 'X', 'protocol': 'UDP'}] + {'port': 1, 'name': None, 'protocol': 'TCP', 'targetPort': 1}, + {'port': 2, 'name': 'X', 'protocol': 'UDP', 'targetPort': 2}] ret = h_lbaas.LBaaSSpecHandler._get_service_ports(m_handler, service) self.assertEqual(expected_ret, ret) @@ -215,13 +215,15 @@ class TestLBaaSSpecHandler(test_base.TestCase): m_handler = mock.Mock(spec=h_lbaas.LBaaSSpecHandler) m_service = mock.MagicMock() m_handler._get_service_ports.return_value = [ - {'port': 1, 'name': 'X', 'protocol': 'TCP'}, + {'port': 1, 'name': 'X', 'protocol': 'TCP', 'targetPort': 1}, ] m_lbaas_spec = mock.MagicMock() m_lbaas_spec.ports = [ - obj_lbaas.LBaaSPortSpec(name='X', protocol='TCP', port=1), - obj_lbaas.LBaaSPortSpec(name='Y', protocol='TCP', port=2), + obj_lbaas.LBaaSPortSpec(name='X', protocol='TCP', port=1, + targetPort=1), + obj_lbaas.LBaaSPortSpec(name='Y', protocol='TCP', port=2, + targetPort=2), ] ret = h_lbaas.LBaaSSpecHandler._has_port_changes( @@ -233,14 +235,16 @@ class TestLBaaSSpecHandler(test_base.TestCase): m_handler = mock.Mock(spec=h_lbaas.LBaaSSpecHandler) m_service = mock.MagicMock() m_handler._get_service_ports.return_value = [ - {'port': 1, 'name': 'X', 'protocol': 'TCP'}, - {'port': 2, 'name': 'Y', 'protocol': 'TCP'} + {'port': 1, 'name': 'X', 'protocol': 'TCP', 'targetPort': 1}, + {'port': 2, 'name': 'Y', 'protocol': 'TCP', 'targetPort': 2} ] m_lbaas_spec = mock.MagicMock() m_lbaas_spec.ports = [ - obj_lbaas.LBaaSPortSpec(name='X', protocol='TCP', port=1), - obj_lbaas.LBaaSPortSpec(name='Y', protocol='TCP', port=2), + obj_lbaas.LBaaSPortSpec(name='X', protocol='TCP', port=1, + targetPort=1), + obj_lbaas.LBaaSPortSpec(name='Y', protocol='TCP', port=2, + targetPort=2), ] ret = h_lbaas.LBaaSSpecHandler._has_port_changes( @@ -673,9 +677,11 @@ class TestLoadBalancerHandler(test_base.TestCase): def test_is_lbaas_spec_in_sync(self): names = ['a', 'b', 'c'] - endpoints = {'subsets': [{'ports': [{'name': n} for n in names]}]} + endpoints = {'subsets': [{'ports': [{'name': n, 'port': 1} + for n in names]}]} lbaas_spec = obj_lbaas.LBaaSServiceSpec(ports=[ - obj_lbaas.LBaaSPortSpec(name=n) for n in reversed(names)]) + obj_lbaas.LBaaSPortSpec(name=n, targetPort=1) + for n in reversed(names)]) m_handler = mock.Mock(spec=h_lbaas.LoadBalancerHandler) ret = h_lbaas.LoadBalancerHandler._is_lbaas_spec_in_sync( @@ -748,10 +754,11 @@ class TestLoadBalancerHandler(test_base.TestCase): ip=vip, project_id=project_id, subnet_id=subnet_id, - ports=[obj_lbaas.LBaaSPortSpec(name=str(port), + ports=[obj_lbaas.LBaaSPortSpec(name=str(t[0]), protocol=prot, - port=port) - for port in set(t[0] for t in targets.values())], + port=t[0], + targetPort=t[1]) + for t in targets.values()], type=lbaas_type) def _generate_endpoints(self, targets): diff --git a/kuryr_kubernetes/tests/unit/test_object.py b/kuryr_kubernetes/tests/unit/test_object.py index 384875b00..b6d95a558 100644 --- a/kuryr_kubernetes/tests/unit/test_object.py +++ b/kuryr_kubernetes/tests/unit/test_object.py @@ -28,7 +28,7 @@ object_data = { 'LBaaSLoadBalancer': '1.3-8bc0a9bdbd160da67572aa38784378d1', 'LBaaSMember': '1.0-a770c6884c27d6d8c21186b27d0e2ccb', 'LBaaSPool': '1.1-6e77370d7632a902445444249eb77b01', - 'LBaaSPortSpec': '1.0-51dfa3436bec32db3614720056fcc83f', + 'LBaaSPortSpec': '1.1-fcfa2fd07f4bc5619b96fa41bcdf6e23', 'LBaaSPubIp': '1.0-83992edec2c60fb4ab8998ea42a4ff74', 'LBaaSRouteNotifEntry': '1.0-dd2f2be956f68814b1f47cb13483a885', 'LBaaSRouteNotifier': '1.0-f0bfd8e772434abe7557930d7e0180c1',