Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1941941: Narrow connection to the cluster only on namespaceSelector #460

Merged
merged 1 commit into from
Mar 13, 2021
Merged

Bug 1941941: Narrow connection to the cluster only on namespaceSelector #460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
88 changes: 48 additions & 40 deletions kuryr_kubernetes/controller/drivers/network_policy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ def _get_namespaces(self, namespace_selector, namespace=None):

def _parse_selectors(self, rule_block, rule_direction, policy_namespace):
allowed_resources = []
allow_all = False
allowed_cidrs = None
selectors = False
for rule in rule_block.get(rule_direction, []):
namespace_selector = rule.get('namespaceSelector')
Expand All @@ -227,8 +227,13 @@ def _parse_selectors(self, rule_block, rule_direction, policy_namespace):
allowed_resources.extend(self._get_pods(
pod_selector))
else:
# allow from all
allow_all = True
# allow from all the cluster, which means pod subnets and
# service subnet.
allowed_cidrs = utils.get_subnetpool_cidrs(
CONF.namespace_subnet.pod_subnet_pool)
if CONF.octavia_defaults.enforce_sg_rules:
allowed_cidrs.append(utils.get_subnet_cidr(
CONF.neutron_defaults.service_subnet))
elif namespace_selector:
selectors = True
if pod_selector:
Expand Down Expand Up @@ -256,10 +261,10 @@ def _parse_selectors(self, rule_block, rule_direction, policy_namespace):
pod_selector,
namespace=policy_namespace))

return allow_all, selectors, allowed_resources
return allowed_cidrs, selectors, allowed_resources

def _create_sg_rules_with_container_ports(
self, container_ports, allow_all, resource, matched_pods,
self, container_ports, allowed_cidrs, resource, matched_pods,
crd_rules, direction, port, pod_selector=None,
policy_namespace=None):
cidr, ns = self._get_resource_details(resource)
Expand All @@ -280,7 +285,7 @@ def _create_sg_rules_with_container_ports(
matched_pods[container_port].update(pod_info)
else:
matched_pods[container_port] = pod_info
if not allow_all and matched_pods and cidr:
if not allowed_cidrs and matched_pods and cidr:
for container_port, pods in matched_pods.items():
sg_rule = driver_utils.create_security_group_rule_body(
direction, container_port,
Expand All @@ -296,7 +301,8 @@ def _create_sg_rules_with_container_ports(

def _create_sg_rule_body_on_text_port(self, direction, port,
resources, crd_rules, pod_selector,
policy_namespace, allow_all=False):
policy_namespace,
allowed_cidrs=None):
"""Create SG rules when named port is used in the NP rule

In case of ingress, the pods selected by NetworkPolicySpec's
Expand All @@ -315,8 +321,8 @@ def _create_sg_rule_body_on_text_port(self, direction, port,
param crd_rules: list of parsed SG rules
param pod_selector: dict with NetworkPolicySpec's podSelector
param policy_namespace: string with policy namespace
param allow_all: True if should parse a allow from/to all rule,
False otherwise
param allowed_cidrs: None, or a list of cidrs, where/from the traffic
should be allowed.
"""
matched_pods = {}
if direction == "ingress":
Expand All @@ -326,7 +332,7 @@ def _create_sg_rule_body_on_text_port(self, direction, port,
container_ports = driver_utils.get_ports(selected_pod, port)
for resource in resources:
self._create_sg_rules_with_container_ports(
container_ports, allow_all, resource, matched_pods,
container_ports, allowed_cidrs, resource, matched_pods,
crd_rules, direction, port)
elif direction == "egress":
for resource in resources:
Expand All @@ -338,16 +344,16 @@ def _create_sg_rule_body_on_text_port(self, direction, port,
continue
container_ports = driver_utils.get_ports(resource, port)
self._create_sg_rules_with_container_ports(
container_ports, allow_all, resource, matched_pods,
container_ports, allowed_cidrs, resource, matched_pods,
crd_rules, direction, port, pod_selector,
policy_namespace)
if allow_all:
if allowed_cidrs:
for container_port, pods in matched_pods.items():
for ethertype in (constants.IPv4, constants.IPv6):
for cidr in allowed_cidrs:
sg_rule = driver_utils.create_security_group_rule_body(
direction, container_port,
protocol=port.get('protocol'),
ethertype=ethertype,
cidr=cidr,
pods=pods)
crd_rules.append(sg_rule)

Expand Down Expand Up @@ -376,13 +382,20 @@ def _create_sg_rule_on_number_port(self, allowed_resources,

def _create_all_pods_sg_rules(self, port, direction,
sg_rule_body_list, pod_selector,
policy_namespace):
if type(port.get('port')) is not int:
policy_namespace, allowed_cidrs=None):
if not isinstance(port.get('port'), int):
all_pods = driver_utils.get_namespaced_pods().get('items')
self._create_sg_rule_body_on_text_port(
direction, port, all_pods,
sg_rule_body_list, pod_selector, policy_namespace,
allow_all=True)
allowed_cidrs=allowed_cidrs)
elif allowed_cidrs:
for cidr in allowed_cidrs:
sg_rule = driver_utils.create_security_group_rule_body(
direction, port.get('port'),
protocol=port.get('protocol'),
cidr=cidr)
sg_rule_body_list.append(sg_rule)
else:
for ethertype in (constants.IPv4, constants.IPv6):
sg_rule = (
Expand Down Expand Up @@ -413,9 +426,11 @@ def _parse_sg_rules(self, sg_rule_body_list, direction, policy):
It accounts for special cases, such as:
- PolicyTypes stating only Egress: ensuring ingress is not restricted
- PolicyTypes not including Egress: ensuring egress is not restricted
- {} ingress/egress rules: applying default open for all
- {} ingress/egress rules: applying default open for all the cluster
"""
_create_sg_rule_body = driver_utils.create_security_group_rule_body
rule_list = policy['spec'].get(direction)

if not rule_list:
policy_types = policy['spec'].get('policyTypes')
if direction == 'ingress':
Expand Down Expand Up @@ -448,15 +463,16 @@ def _parse_sg_rules(self, sg_rule_body_list, direction, policy):
LOG.debug('Applying default all open policy from %s',
utils.get_res_link(policy))
for ethertype in (constants.IPv4, constants.IPv6):
rule = driver_utils.create_security_group_rule_body(
direction, ethertype=ethertype)
rule = _create_sg_rule_body(direction, ethertype=ethertype)
sg_rule_body_list.append(rule)

for rule_block in rule_list:
LOG.debug('Parsing %(dir)s Rule %(rule)s', {'dir': direction,
'rule': rule_block})
allow_all, selectors, allowed_resources = self._parse_selectors(
rule_block, rule_direction, policy_namespace)
(allowed_cidrs, selectors,
allowed_resources) = self._parse_selectors(rule_block,
rule_direction,
policy_namespace)

ipblock_list = []

Expand All @@ -478,8 +494,8 @@ def _parse_sg_rules(self, sg_rule_body_list, direction, policy):

if 'ports' in rule_block:
for port in rule_block['ports']:
if allowed_resources or allow_all or selectors:
if type(port.get('port')) is not int:
if allowed_resources or allowed_cidrs or selectors:
if not isinstance(port.get('port'), int):
self._create_sg_rule_body_on_text_port(
direction, port, allowed_resources,
sg_rule_body_list, pod_selector,
Expand All @@ -488,40 +504,32 @@ def _parse_sg_rules(self, sg_rule_body_list, direction, policy):
self._create_sg_rule_on_number_port(
allowed_resources, direction, port,
sg_rule_body_list, policy_namespace)
if allow_all:
if allowed_cidrs:
self._create_all_pods_sg_rules(
port, direction, sg_rule_body_list,
pod_selector, policy_namespace)
pod_selector, policy_namespace, allowed_cidrs)
else:
self._create_all_pods_sg_rules(
port, direction, sg_rule_body_list,
pod_selector, policy_namespace)
elif allowed_resources or allow_all or selectors:
elif allowed_resources or allowed_cidrs or selectors:
for resource in allowed_resources:
cidr, namespace = self._get_resource_details(resource)
# NOTE(maysams): Skipping resource that do not have
# an IP assigned. The security group rule creation
# will be triggered again after the resource is running.
if not cidr:
continue
rule = driver_utils.create_security_group_rule_body(
direction,
port_range_min=1,
port_range_max=65535,
cidr=cidr,
namespace=namespace)
rule = _create_sg_rule_body(direction, cidr=cidr,
namespace=namespace)
sg_rule_body_list.append(rule)
if direction == 'egress':
self._create_svc_egress_sg_rule(
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(
direction,
port_range_min=1,
port_range_max=65535,
ethertype=ethertype)
if allowed_cidrs:
for cidr in allowed_cidrs:
rule = _create_sg_rule_body(direction, cidr=cidr)
sg_rule_body_list.append(rule)
else:
LOG.debug('This network policy specifies no %(direction)s '
Expand Down
38 changes: 17 additions & 21 deletions kuryr_kubernetes/tests/unit/controller/drivers/test_network_policy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,12 +341,8 @@ def test_parse_network_policy_rules_with_no_ports(
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,
port_range_max=65535, cidr=subnet_cidr,
namespace=namespace),
mock.call('egress', port_range_min=1,
port_range_max=65535, cidr=subnet_cidr,
namespace=namespace)]
calls = [mock.call('ingress', cidr=subnet_cidr, namespace=namespace),
mock.call('egress', cidr=subnet_cidr, namespace=namespace)]
m_create.assert_has_calls(calls)

@mock.patch.object(network_policy.NetworkPolicyDriver, 'namespaced_pods')
Expand Down Expand Up @@ -430,6 +426,7 @@ def test__create_sg_rule_body_on_text_port_ingress_all(self,
pod_selector = {}
namespace = mock.sentinel.namespace
direction = 'ingress'
cidrs = ['0.0.0.0/0']

m_get_pods.return_value = {'items': [pod]}
m_get_ports.return_value = container_ports
Expand All @@ -440,7 +437,7 @@ def test__create_sg_rule_body_on_text_port_ingress_all(self,
crd_rules,
pod_selector,
namespace,
allow_all=True)
allowed_cidrs=cidrs)

m_get_pods.assert_called_with(pod_selector, namespace)
m_get_ports.assert_called_with(pod, port)
Expand Down Expand Up @@ -468,6 +465,7 @@ def _create_sgr_cont(container_ports, allow_all, resource,
pod_selector = {}
namespace = mock.sentinel.namespace
direction = 'ingress'
cidrs = ['0.0.0.0/0']
self._driver._create_sg_rules_with_container_ports = _create_sgr_cont

m_get_pods.return_value = {'items': [pod]}
Expand All @@ -479,17 +477,15 @@ def _create_sgr_cont(container_ports, allow_all, resource,
crd_rules,
pod_selector,
namespace,
allow_all=True)
allowed_cidrs=cidrs)

m_get_pods.assert_called_with(pod_selector, namespace)
m_get_ports.assert_called_with(pod, port)

calls = [mock.call(direction, container_ports[0][1],
protocol=port['protocol'], ethertype=e,
pods='foo') for e in ('IPv4', 'IPv6')]

m_create_sgr.assert_has_calls(calls)
self.assertEqual(len(crd_rules), 2)
m_create_sgr.assert_called_once_with(direction, container_ports[0][1],
protocol=port['protocol'],
cidr=cidrs[0],
pods='foo')

@mock.patch.object(network_policy.NetworkPolicyDriver,
'_create_sg_rules_with_container_ports')
Expand Down Expand Up @@ -533,6 +529,7 @@ def test__create_sg_rule_body_on_text_port_egress_all(self,
pod_selector = {}
namespace = mock.sentinel.namespace
direction = 'egress'
cidrs = ['0.0.0.0/0']

m_get_ports.return_value = container_ports

Expand All @@ -542,7 +539,7 @@ def test__create_sg_rule_body_on_text_port_egress_all(self,
crd_rules,
pod_selector,
namespace,
allow_all=True)
allowed_cidrs=cidrs)

m_get_ports.assert_called_with(resources[0], port)
self.assertEqual(len(crd_rules), 0)
Expand Down Expand Up @@ -571,6 +568,7 @@ def _create_sgr_cont(container_ports, allow_all, resource,
pod_selector = {}
namespace = mock.sentinel.namespace
direction = 'egress'
cidrs = ['0.0.0.0/0']
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,
Expand All @@ -585,14 +583,12 @@ def _create_sgr_cont(container_ports, allow_all, resource,
crd_rules,
pod_selector,
namespace,
allow_all=True)
allowed_cidrs=cidrs)

m_get_ports.assert_called_with(resources[0], port)
calls = [mock.call(direction, container_ports[0][1],
protocol=port['protocol'], ethertype=e,
pods='foo') for e in ('IPv4', 'IPv6')]
m_create_sgr.assert_has_calls(calls)
self.assertEqual(len(crd_rules), 2)
m_create_sgr.assert_called_once_with(direction, container_ports[0][1],
protocol=port['protocol'],
cidr=cidrs[0], pods='foo')

def test__create_all_pods_sg_rules(self):
port = {'protocol': 'TCP', 'port': 22}
Expand Down
12 changes: 11 additions & 1 deletion kuryr_kubernetes/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,16 +328,26 @@ def get_subnets_cidrs(subnet_ids):


@MEMOIZE
def get_subnetpool_version(subnetpool_id):
def _get_subnetpool(subnetpool_id):
os_net = clients.get_network_client()
try:
subnetpool_obj = os_net.get_subnet_pool(subnetpool_id)
except os_exc.ResourceNotFound:
LOG.exception("Subnetpool %s not found!", subnetpool_id)
raise
return subnetpool_obj


def get_subnetpool_version(subnetpool_id):
subnetpool_obj = _get_subnetpool(subnetpool_id)
return subnetpool_obj.ip_version


def get_subnetpool_cidrs(subnetpool_id):
subnetpool_obj = _get_subnetpool(subnetpool_id)
return subnetpool_obj.prefixes


def extract_pod_annotation(annotation):
obj = objects.base.VersionedObject.obj_from_primitive(annotation)
# FIXME(dulek): This is code to maintain compatibility with Queens. We can
Expand Down