From 2611dc3b3ab034dc850222e968930e0ce0f92861 Mon Sep 17 00:00:00 2001 From: Roman Dobosz Date: Thu, 4 Feb 2021 12:36:03 +0100 Subject: [PATCH] Make parse_network_policy_rules private. Network policy parse_network_policy_rules is used only within the NetworkPolicyDriver class. Let's make it private, as it should be. Also, changed layout of the code a bit, just to easily distinguish between helper methods from signature of the class. Change-Id: Ic13393c841f04e6748f3fe716656cb5a8b3dcd71 --- .../controller/drivers/network_policy.py | 164 +++++++++--------- .../controller/drivers/test_network_policy.py | 16 +- 2 files changed, 90 insertions(+), 90 deletions(-) diff --git a/kuryr_kubernetes/controller/drivers/network_policy.py b/kuryr_kubernetes/controller/drivers/network_policy.py index 46c60e2ce..d76f763d7 100644 --- a/kuryr_kubernetes/controller/drivers/network_policy.py +++ b/kuryr_kubernetes/controller/drivers/network_policy.py @@ -39,6 +39,58 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): self.kubernetes = clients.get_kubernetes_client() self.nodes_subnets_driver = base.NodesSubnetsDriver.get_instance() + def affected_pods(self, policy, selector=None): + if selector is not None: + pod_selector = selector + else: + pod_selector = policy['spec'].get('podSelector') + if pod_selector: + policy_namespace = policy['metadata']['namespace'] + pods = driver_utils.get_pods(pod_selector, policy_namespace) + return pods.get('items') + else: + # NOTE(ltomasbo): It affects all the pods on the namespace + return self.namespaced_pods(policy) + + def create_security_group(self, knp, project_id): + sg_name = ("sg-" + knp['metadata']['namespace'] + "-" + + knp['metadata']['name']) + desc = ("Kuryr-Kubernetes Network Policy %s SG" % + utils.get_res_unique_name(knp)) + try: + # Create initial security group + sg = self.os_net.create_security_group(name=sg_name, + project_id=project_id, + description=desc) + driver_utils.tag_neutron_resources([sg]) + # NOTE(dulek): Neutron populates every new SG with two rules + # allowing egress on IPv4 and IPv6. This collides with + # how network policies are supposed to work, because + # initially even egress traffic should be blocked. + # To work around this we will delete those two SG + # rules just after creation. + for sgr in sg.security_group_rules: + self.os_net.delete_security_group_rule(sgr['id']) + except (os_exc.SDKException, exceptions.ResourceNotReady): + LOG.exception("Error creating security group for network policy " + " %s", knp['metadata']['name']) + raise + + return sg.id + + def delete_np_sg(self, sg_id): + try: + self.os_net.delete_security_group(sg_id) + except os_exc.ConflictException: + LOG.debug("Security Group %s still in use!", sg_id) + # raising ResourceNotReady to retry this action in case ports + # associated to affected pods are not updated on time, i.e., + # they are still using the security group to be removed + raise exceptions.ResourceNotReady(sg_id) + except os_exc.SDKException: + LOG.exception("Error deleting security group %s.", sg_id) + raise + def ensure_network_policy(self, policy): """Create security group rules out of network policies @@ -64,28 +116,6 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): else: self._patch_knp_crd(policy, i_rules, e_rules, knp) - def _convert_old_sg_rule(self, rule): - del rule['security_group_rule']['id'] - del rule['security_group_rule']['security_group_id'] - result = { - 'sgRule': rule['security_group_rule'], - } - - if 'namespace' in rule: - result['namespace'] = rule['namespace'] - - if 'remote_ip_prefixes' in rule: - result['affectedPods'] = [] - for ip, namespace in rule['remote_ip_prefixes']: - if not ip: - continue - result['affectedPods'].append({ - 'podIP': ip, - 'podNamespace': namespace, - }) - - return result - def get_from_old_crd(self, netpolicy): name = netpolicy['metadata']['name'][3:] # Remove 'np-' namespace = netpolicy['metadata']['namespace'] @@ -123,13 +153,41 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): return knp + def namespaced_pods(self, policy): + pod_namespace = policy['metadata']['namespace'] + pods = self.kubernetes.get('{}/namespaces/{}/pods'.format( + constants.K8S_API_BASE, pod_namespace)) + return pods.get('items') + + def _convert_old_sg_rule(self, rule): + del rule['security_group_rule']['id'] + del rule['security_group_rule']['security_group_id'] + result = { + 'sgRule': rule['security_group_rule'], + } + + if 'namespace' in rule: + result['namespace'] = rule['namespace'] + + if 'remote_ip_prefixes' in rule: + result['affectedPods'] = [] + for ip, namespace in rule['remote_ip_prefixes']: + if not ip: + continue + result['affectedPods'].append({ + 'podIP': ip, + 'podNamespace': namespace, + }) + + return result + def _get_security_group_rules_from_network_policy(self, policy): """Get security group rules required to represent an NP This method creates the security group rules bodies coming out of a network policies' parsing. """ - i_rules, e_rules = self.parse_network_policy_rules(policy) + i_rules, e_rules = self._parse_network_policy_rules(policy) # Add default rules to allow traffic from host and svc subnet i_rules += self._get_default_np_rules() @@ -165,32 +223,6 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): return rules - def create_security_group(self, knp, project_id): - sg_name = ("sg-" + knp['metadata']['namespace'] + "-" + - knp['metadata']['name']) - desc = ("Kuryr-Kubernetes Network Policy %s SG" % - utils.get_res_unique_name(knp)) - try: - # Create initial security group - sg = self.os_net.create_security_group(name=sg_name, - project_id=project_id, - description=desc) - driver_utils.tag_neutron_resources([sg]) - # NOTE(dulek): Neutron populates every new SG with two rules - # allowing egress on IPv4 and IPv6. This collides with - # how network policies are supposed to work, because - # initially even egress traffic should be blocked. - # To work around this we will delete those two SG - # rules just after creation. - for sgr in sg.security_group_rules: - self.os_net.delete_security_group_rule(sgr['id']) - except (os_exc.SDKException, exceptions.ResourceNotReady): - LOG.exception("Error creating security group for network policy " - " %s", knp['metadata']['name']) - raise - - return sg.id - def _get_pods(self, pod_selector, namespace=None, namespace_selector=None): matching_pods = {"items": []} if namespace_selector: @@ -605,7 +637,7 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): return False return True - def parse_network_policy_rules(self, policy): + def _parse_network_policy_rules(self, policy): """Create security group rule bodies out of network policies. Whenever a notification from the handler 'on-present' method is @@ -621,19 +653,6 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): return ingress_sg_rule_body_list, egress_sg_rule_body_list - def delete_np_sg(self, sg_id): - try: - self.os_net.delete_security_group(sg_id) - except os_exc.ConflictException: - LOG.debug("Security Group %s still in use!", sg_id) - # raising ResourceNotReady to retry this action in case ports - # associated to affected pods are not updated on time, i.e., - # they are still using the security group to be removed - raise exceptions.ResourceNotReady(sg_id) - except os_exc.SDKException: - LOG.exception("Error deleting security group %s.", sg_id) - raise - def release_network_policy(self, policy): return self._del_knp_crd(policy) @@ -727,25 +746,6 @@ class NetworkPolicyDriver(base.NetworkPolicyDriver): "KuryrNetworkPolicy CRD %s." % name) raise - def affected_pods(self, policy, selector=None): - if selector is not None: - pod_selector = selector - else: - pod_selector = policy['spec'].get('podSelector') - if pod_selector: - policy_namespace = policy['metadata']['namespace'] - pods = driver_utils.get_pods(pod_selector, policy_namespace) - return pods.get('items') - else: - # NOTE(ltomasbo): It affects all the pods on the namespace - return self.namespaced_pods(policy) - - def namespaced_pods(self, policy): - pod_namespace = policy['metadata']['namespace'] - pods = self.kubernetes.get('{}/namespaces/{}/pods'.format( - constants.K8S_API_BASE, pod_namespace)) - return pods.get('items') - def _get_resource_details(self, resource): namespace = None if self._is_pod(resource): 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 daedcb6e0..2a2569ae5 100644 --- a/kuryr_kubernetes/tests/unit/controller/drivers/test_network_policy.py +++ b/kuryr_kubernetes/tests/unit/controller/drivers/test_network_policy.py @@ -179,7 +179,7 @@ class TestNetworkPolicyDriver(test_base.TestCase): @mock.patch.object(network_policy.NetworkPolicyDriver, '_create_knp_crd') @mock.patch.object(network_policy.NetworkPolicyDriver, - 'parse_network_policy_rules') + '_parse_network_policy_rules') @mock.patch.object(utils, 'get_subnet_cidr') def test_ensure_network_policy(self, m_utils, m_parse, m_add_crd, m_get_crd, m_get_default): @@ -196,7 +196,7 @@ class TestNetworkPolicyDriver(test_base.TestCase): @mock.patch.object(network_policy.NetworkPolicyDriver, '_get_knp_crd') @mock.patch.object(network_policy.NetworkPolicyDriver, - 'parse_network_policy_rules') + '_parse_network_policy_rules') @mock.patch.object(utils, 'get_subnet_cidr') def test_ensure_network_policy_with_k8s_exc(self, m_utils, m_parse, m_get_crd, m_get_default): @@ -213,7 +213,7 @@ class TestNetworkPolicyDriver(test_base.TestCase): '_get_knp_crd', return_value=None) @mock.patch.object(network_policy.NetworkPolicyDriver, '_create_knp_crd') @mock.patch.object(network_policy.NetworkPolicyDriver, - 'parse_network_policy_rules') + '_parse_network_policy_rules') @mock.patch.object(utils, 'get_subnet_cidr') def test_ensure_network_policy_error_add_crd( self, m_utils, m_parse, m_add_crd, m_get_crd, m_get_default): @@ -263,7 +263,7 @@ class TestNetworkPolicyDriver(test_base.TestCase): namespace = 'myproject' m_get_namespaces.return_value = [get_namespace_obj()] m_get_resource_details.return_value = subnet_cidr, namespace - self._driver.parse_network_policy_rules(self._policy) + self._driver._parse_network_policy_rules(self._policy) m_get_namespaces.assert_called() m_get_resource_details.assert_called() m_create.assert_called() @@ -277,7 +277,7 @@ class TestNetworkPolicyDriver(test_base.TestCase): policy = self._policy.copy() policy['spec']['ingress'] = [{}] policy['spec']['egress'] = [{}] - self._driver.parse_network_policy_rules(policy) + self._driver._parse_network_policy_rules(policy) m_get_ns.assert_not_called() calls = [mock.call('ingress', ethertype='IPv4'), mock.call('ingress', ethertype='IPv6'), @@ -294,7 +294,7 @@ class TestNetworkPolicyDriver(test_base.TestCase): [{'port': 6379, 'protocol': 'TCP'}]}] policy['spec']['egress'] = [{'ports': [{'port': 6379, 'protocol': 'TCP'}]}] - self._driver.parse_network_policy_rules(policy) + self._driver._parse_network_policy_rules(policy) m_create_all_pods_sg_rules.assert_called() @mock.patch.object(network_policy.NetworkPolicyDriver, @@ -315,7 +315,7 @@ class TestNetworkPolicyDriver(test_base.TestCase): 'TCP'}], 'to': [{'ipBlock': {'cidr': '10.0.0.0/24'}}]}] - self._driver.parse_network_policy_rules(policy) + self._driver._parse_network_policy_rules(policy) m_create_sg_rule.assert_called() @mock.patch('kuryr_kubernetes.controller.drivers.utils.get_services') @@ -338,7 +338,7 @@ class TestNetworkPolicyDriver(test_base.TestCase): 'project': 'myproject'}}} policy['spec']['egress'] = [{'to': [selectors]}] policy['spec']['ingress'] = [{'from': [selectors]}] - self._driver.parse_network_policy_rules(policy) + self._driver._parse_network_policy_rules(policy) m_get_namespaces.assert_called() m_get_resource_details.assert_called() calls = [mock.call('ingress', port_range_min=1,