From abc679c9f23dfc8a784eb350534b0e01e372bdab Mon Sep 17 00:00:00 2001 From: Maysa Macedo Date: Thu, 9 Jul 2020 22:21:57 +0000 Subject: [PATCH] Fix duplicated sg rules on NP crd While handling the creation of a Network Policy it's possible that the CRD is patched with repeated sg rules, which is not allowed resulting in validation error as the repeated sg rules will not have the sg rule id. Closes-Bug: #1887167 Change-Id: Ia7814ddcea0d6948ff280a3e03a896bbc442891c --- .../controller/drivers/network_policy.py | 51 +++++++++---------- .../controller/drivers/test_network_policy.py | 13 +++-- 2 files changed, 34 insertions(+), 30 deletions(-) diff --git a/kuryr_kubernetes/controller/drivers/network_policy.py b/kuryr_kubernetes/controller/drivers/network_policy.py index e70c34b30..a8089e9eb 100644 --- a/kuryr_kubernetes/controller/drivers/network_policy.py +++ b/kuryr_kubernetes/controller/drivers/network_policy.py @@ -314,11 +314,10 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): if sg_rule not in crd_rules: crd_rules.append(sg_rule) if direction == 'egress': - rules = self._create_svc_egress_sg_rule( - sg_id, policy_namespace, resource=resource, - port=container_port, + self._create_svc_egress_sg_rule( + sg_id, policy_namespace, crd_rules, + resource=resource, port=container_port, protocol=port.get('protocol')) - crd_rules.extend(rules) def _create_sg_rule_body_on_text_port(self, sg_id, direction, port, resources, crd_rules, pod_selector, @@ -378,10 +377,9 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): pods=pods) crd_rules.append(sg_rule) if direction == 'egress': - rules = self._create_svc_egress_sg_rule( - sg_id, policy_namespace, port=container_port, - protocol=port.get('protocol')) - crd_rules.extend(rules) + self._create_svc_egress_sg_rule( + sg_id, policy_namespace, crd_rules, + port=container_port, protocol=port.get('protocol')) def _create_sg_rule_on_number_port(self, allowed_resources, sg_id, direction, port, sg_rule_body_list, @@ -401,10 +399,10 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): namespace=ns)) sg_rule_body_list.append(sg_rule) if direction == 'egress': - rule = self._create_svc_egress_sg_rule( - sg_id, policy_namespace, resource=resource, - port=port.get('port'), protocol=port.get('protocol')) - sg_rule_body_list.extend(rule) + self._create_svc_egress_sg_rule( + sg_id, policy_namespace, sg_rule_body_list, + resource=resource, port=port.get('port'), + protocol=port.get('protocol')) def _create_all_pods_sg_rules(self, port, sg_id, direction, sg_rule_body_list, pod_selector, @@ -424,10 +422,10 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): protocol=port.get('protocol'))) sg_rule_body_list.append(sg_rule) if direction == 'egress': - rule = self._create_svc_egress_sg_rule( - sg_id, policy_namespace, port=port.get('port'), + self._create_svc_egress_sg_rule( + sg_id, policy_namespace, sg_rule_body_list, + port=port.get('port'), protocol=port.get('protocol')) - sg_rule_body_list.extend(rule) def _create_default_sg_rule(self, sg_id, direction, sg_rule_body_list): for ethertype in (constants.IPv4, constants.IPv6): @@ -553,8 +551,8 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): sg_rule_body_list.append(rule) if direction == 'egress': rule = self._create_svc_egress_sg_rule( - sg_id, policy_namespace, resource=resource) - sg_rule_body_list.extend(rule) + sg_id, policy_namespace, sg_rule_body_list, + resource=resource) if allow_all: for ethertype in (constants.IPv4, constants.IPv6): rule = driver_utils.create_security_group_rule_body( @@ -564,9 +562,8 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): ethertype=ethertype) sg_rule_body_list.append(rule) if direction == 'egress': - rule = self._create_svc_egress_sg_rule( - sg_id, policy_namespace) - sg_rule_body_list.extend(rule) + self._create_svc_egress_sg_rule( + sg_id, policy_namespace, sg_rule_body_list) else: LOG.debug('This network policy specifies no %(direction)s ' '%(rule_direction)s and no ports: %(policy)s', @@ -575,17 +572,17 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): 'policy': policy['metadata']['selfLink']}) def _create_svc_egress_sg_rule(self, sg_id, policy_namespace, - resource=None, port=None, - protocol=None): - sg_rule_body_list = [] + sg_rule_body_list, resource=None, + port=None, protocol=None): services = driver_utils.get_services() if not resource: svc_subnet = utils.get_subnet_cidr( CONF.neutron_defaults.service_subnet) rule = driver_utils.create_security_group_rule_body( sg_id, 'egress', port, protocol=protocol, cidr=svc_subnet) - sg_rule_body_list.append(rule) - return sg_rule_body_list + if rule not in sg_rule_body_list: + sg_rule_body_list.append(rule) + return for service in services.get('items'): if self._is_pod(resource): @@ -618,8 +615,8 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): rule = driver_utils.create_security_group_rule_body( sg_id, 'egress', port, protocol=protocol, cidr=cluster_ip) - sg_rule_body_list.append(rule) - return sg_rule_body_list + if rule not in sg_rule_body_list: + sg_rule_body_list.append(rule) def _pods_in_ip_block(self, pods, resource): for pod in pods: diff --git a/kuryr_kubernetes/tests/unit/controller/drivers/test_network_policy.py b/kuryr_kubernetes/tests/unit/controller/drivers/test_network_policy.py index b6d8f4226..81fd2e5f8 100644 --- a/kuryr_kubernetes/tests/unit/controller/drivers/test_network_policy.py +++ b/kuryr_kubernetes/tests/unit/controller/drivers/test_network_policy.py @@ -699,6 +699,7 @@ class TestNetworkPolicyDriver(test_base.TestCase): cidr=mock.ANY, protocol='TCP') self.assertEqual(len(crd_rules), 1) + @mock.patch('kuryr_kubernetes.utils.get_subnet_cidr') @mock.patch('kuryr_kubernetes.controller.drivers.utils.' 'create_security_group_rule_body') @mock.patch('kuryr_kubernetes.controller.drivers.utils.get_ports') @@ -706,7 +707,8 @@ class TestNetworkPolicyDriver(test_base.TestCase): def test__create_sg_rule_body_on_text_port_egress_match(self, m_get_pods, m_get_ports, - m_create_sgr): + m_create_sgr, + m_get_subnet_cidr): def _create_sgr_cont(container_ports, allow_all, resource, matched_pods, crd_rules, sg_id, direction, port, @@ -722,6 +724,9 @@ class TestNetworkPolicyDriver(test_base.TestCase): namespace = mock.sentinel.namespace direction = 'egress' self._driver._create_sg_rules_with_container_ports = _create_sgr_cont + m_get_subnet_cidr.return_value = '10.0.0.128/26' + m_create_sgr.side_effect = [mock.sentinel.sgr1, mock.sentinel.sgr2, + mock.sentinel.sgr3] m_get_pods.return_value = {'items': [pod]} m_get_ports.return_value = container_ports @@ -736,12 +741,14 @@ class TestNetworkPolicyDriver(test_base.TestCase): allow_all=True) m_get_ports.assert_called_with(resources[0], port) - calls = [mock.call(self._sg_id, direction, container_ports[0][1], protocol=port['protocol'], ethertype=e, pods='foo') for e in ('IPv4', 'IPv6')] - + calls.append(mock.call(self._sg_id, direction, container_ports[0][1], + protocol=port['protocol'], + cidr='10.0.0.128/26')) m_create_sgr.assert_has_calls(calls) + # NOTE(gryf): there are 3 rules created in case of egress direction, # since additional one is created for specific cidr in service subnet. self.assertEqual(len(crd_rules), 3)