From 60ffb8f705c1d2b273443165073491bc97d2a536 Mon Sep 17 00:00:00 2001 From: Adit Sarfaty Date: Mon, 7 Nov 2016 10:39:03 +0200 Subject: [PATCH] NSX|V support security groups rules with policy configuration The nsxv configuration allow_tenant_rules_with_policy (False by default) allows tenants to create regular security groups with rules when use_nsx_policies is True. Those security groups rules will be evaluated after the relevant policies. Change-Id: I95a49505e3575938c2ecfe482bde50ba37ca1f8e --- devstack/lib/vmware_nsx_v | 1 + vmware_nsx/common/config.py | 4 + vmware_nsx/plugins/nsx_v/plugin.py | 56 ++++++---- .../extensions/test_security_group_policy.py | 100 +++++++++++++++--- 4 files changed, 127 insertions(+), 34 deletions(-) diff --git a/devstack/lib/vmware_nsx_v b/devstack/lib/vmware_nsx_v index e69ec1aca3..aea7fd0b0e 100644 --- a/devstack/lib/vmware_nsx_v +++ b/devstack/lib/vmware_nsx_v @@ -120,6 +120,7 @@ function neutron_plugin_configure_service { _nsxv_ini_set use_dvs_features "$NSXV_USE_DVS_FEATURES" _nsxv_ini_set use_nsx_policies "$NSXV_USE_NSX_POLICIES" _nsxv_ini_set default_policy_id "$NSXV_DEFAULT_POLICY_ID" + _nsxv_ini_set allow_tenant_rules_with_policy "$NSXV_ALLOW_TENANT_RULES_WITH_POLICY" } function neutron_plugin_setup_interface_driver { diff --git a/vmware_nsx/common/config.py b/vmware_nsx/common/config.py index 262f35e772..2389ddb84d 100644 --- a/vmware_nsx/common/config.py +++ b/vmware_nsx/common/config.py @@ -623,6 +623,10 @@ nsxv_opts = [ cfg.StrOpt('default_policy_id', help=_("(Optional) If use_nsx_policies is True, this policy " "will be used as the default policy for new tenants.")), + cfg.BoolOpt('allow_tenant_rules_with_policy', default=False, + help=_("(Optional) If use_nsx_policies is True, this value " + "will determine if a tenants can add rules to their " + "security groups.")), ] # Register the configuration options diff --git a/vmware_nsx/plugins/nsx_v/plugin.py b/vmware_nsx/plugins/nsx_v/plugin.py index 79d625eeac..942c89ae80 100644 --- a/vmware_nsx/plugins/nsx_v/plugin.py +++ b/vmware_nsx/plugins/nsx_v/plugin.py @@ -3034,11 +3034,12 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin, context, securitygroup): nsx_sg_id = self._create_nsx_security_group(context, securitygroup) - if self._use_nsx_policies: + policy = securitygroup.get(sg_policy.POLICY) + if self._use_nsx_policies and policy: # When using policies - no rules should be created. # just add the security group to the policy on the backend. self._update_nsx_security_group_policies( - securitygroup[sg_policy.POLICY], None, nsx_sg_id) + policy, None, nsx_sg_id) # Delete the neutron default rules (do not exist on the backend) if securitygroup.get(ext_sg.SECURITYGROUPRULES): @@ -3064,11 +3065,12 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin, self._add_member_to_security_group(self.sg_container_id, nsx_sg_id) - def _validate_security_group(self, security_group, default_sg, - from_create=True): + def _validate_security_group(self, context, security_group, default_sg, + id=None): if self._use_nsx_policies: new_policy = None - if from_create: + sg_with_policy = False + if not id: # called from create_security_group # must have a policy: if not security_group.get(sg_policy.POLICY): @@ -3076,21 +3078,29 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin, # For default sg the default policy will be used security_group[sg_policy.POLICY] = ( cfg.CONF.nsxv.default_policy_id) - else: + elif not cfg.CONF.nsxv.allow_tenant_rules_with_policy: msg = _('A security group must be assigned to a ' 'policy') raise n_exc.InvalidInput(error_message=msg) - #TODO(asarfaty): add support for tenant sg with rules - new_policy = security_group[sg_policy.POLICY] + + new_policy = security_group.get(sg_policy.POLICY) + sg_with_policy = True if new_policy else False else: - # called from update_security_group + # called from update_security_group. + # Check if the existing security group has policy or not + sg_with_policy = self._is_policy_security_group(context, id) if sg_policy.POLICY in security_group: new_policy = security_group[sg_policy.POLICY] - if not new_policy: - msg = _('A security group must be assigned to a ' - 'policy') + if sg_with_policy and not new_policy: + # cannot remove a policy from an existing sg + msg = (_('Security group %s must be assigned to a ' + 'policy') % id) + raise n_exc.InvalidInput(error_message=msg) + if not sg_with_policy and new_policy: + # cannot add a policy to a non-policy security group + msg = (_('Cannot add policy to an existing security ' + 'group %s') % id) raise n_exc.InvalidInput(error_message=msg) - #TODO(asarfaty): add support for tenant sg with rules # validate that the new policy exists if new_policy and not self.nsx_v.vcns.validate_inventory( @@ -3099,7 +3109,7 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin, raise n_exc.InvalidInput(error_message=msg) # Do not support logging with policy - if security_group.get(sg_logging.LOGGING): + if sg_with_policy and security_group.get(sg_logging.LOGGING): msg = _('Cannot support logging when using NSX policies') raise n_exc.InvalidInput(error_message=msg) else: @@ -3112,7 +3122,7 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin, """Create a security group.""" sg_data = security_group['security_group'] sg_id = sg_data["id"] = str(uuid.uuid4()) - self._validate_security_group(sg_data, default_sg, from_create=True) + self._validate_security_group(context, sg_data, default_sg) with context.session.begin(subtransactions=True): if sg_data.get(provider_sg.PROVIDER): @@ -3177,7 +3187,7 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin, def update_security_group(self, context, id, security_group): s = security_group['security_group'] - self._validate_security_group(s, False, from_create=False) + self._validate_security_group(context, s, False, id=id) nsx_sg_id = nsx_db.get_nsx_security_group_id(context.session, id) section_uri = self._get_section_uri(context.session, id) section_needs_update = False @@ -3194,10 +3204,11 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin, # security groups with NSX policy - update the backend policy attached # to the security group - if self._use_nsx_policies and sg_policy.POLICY in sg_data: - self._update_security_group_with_policy(s, sg_data, nsx_sg_id) + if (self._use_nsx_policies and + self._is_policy_security_group(context, id)): + if sg_policy.POLICY in sg_data: + self._update_security_group_with_policy(s, sg_data, nsx_sg_id) - if self._use_nsx_policies: # The rest of the update are not relevant to policies security # groups as there is no matching section self._process_security_group_properties_update( @@ -3331,10 +3342,11 @@ class NsxVPluginV2(addr_pair_db.AllowedAddressPairsMixin, sg_rules = security_group_rules['security_group_rules'] sg_id = sg_rules[0]['security_group_rule']['security_group_id'] - if self._use_nsx_policies: + if (self._use_nsx_policies and + self._is_policy_security_group(context, sg_id)): # If policies are enabled - creating rules is forbidden - msg = _('Cannot create rules cannot for security group %s with' - ' a policy') % sg_id + msg = (_('Cannot create rules for security group %s with' + ' a policy') % sg_id) raise n_exc.InvalidInput(error_message=msg) self._prevent_non_admin_delete_provider_sg(context, sg_id) diff --git a/vmware_nsx/tests/unit/extensions/test_security_group_policy.py b/vmware_nsx/tests/unit/extensions/test_security_group_policy.py index 2ab4ab3094..1f6d81c1ea 100644 --- a/vmware_nsx/tests/unit/extensions/test_security_group_policy.py +++ b/vmware_nsx/tests/unit/extensions/test_security_group_policy.py @@ -19,7 +19,9 @@ import webob.exc from neutron.api.v2 import attributes as attr from neutron import context from neutron.tests.unit.extensions import test_securitygroup +from neutron_lib import constants +from vmware_nsx.extensions import securitygrouplogging as ext_logging from vmware_nsx.extensions import securitygrouppolicy as ext_policy from vmware_nsx.tests.unit.nsx_v import test_plugin from vmware_nsx.tests.unit.nsx_v.vshield import fake_vcns @@ -40,22 +42,30 @@ class SecGroupPolicyExtensionTestCase( super(SecGroupPolicyExtensionTestCase, self).setUp( plugin=plugin, ext_mgr=ext_mgr) self._tenant_id = 'foobar' - # add policy security group attribute + # add policy & logging security group attribute attr.RESOURCE_ATTRIBUTE_MAP['security_groups'].update( ext_policy.RESOURCE_ATTRIBUTE_MAP['security_groups']) + attr.RESOURCE_ATTRIBUTE_MAP['security_groups'].update( + ext_logging.RESOURCE_ATTRIBUTE_MAP['security_groups']) def tearDown(self): # remove policy security group attribute del attr.RESOURCE_ATTRIBUTE_MAP['security_groups']['policy'] super(SecGroupPolicyExtensionTestCase, self).tearDown() - def _create_secgroup_with_policy(self, policy_id): + def _create_secgroup_with_policy(self, policy_id, logging=False): body = {'security_group': {'name': 'sg-policy', 'tenant_id': self._tenant_id, - 'policy': policy_id}} + 'policy': policy_id, + 'logging': logging}} security_group_req = self.new_create_request('security-groups', body) return security_group_req.get_response(self.ext_api) + def _get_secgroup_with_policy(self): + policy_id = 'policy-5' + res = self._create_secgroup_with_policy(policy_id) + return self.deserialize(self.fmt, res) + def test_secgroup_create_with_policy(self): policy_id = 'policy-5' res = self._create_secgroup_with_policy(policy_id) @@ -74,7 +84,14 @@ class SecGroupPolicyExtensionTestCase( res = self._create_secgroup_with_policy(policy_id) self.assertEqual(400, res.status_int) + def test_secgroup_create_with_policy_and_logging(self): + # We do not support policy & logging together + policy_id = 'policy-5' + res = self._create_secgroup_with_policy(policy_id, logging=True) + self.assertEqual(400, res.status_int) + def test_secgroup_update_with_policy(self): + # Test that updating the policy is allowed old_policy = 'policy-5' new_policy = 'policy-6' res = self._create_secgroup_with_policy(old_policy) @@ -86,30 +103,38 @@ class SecGroupPolicyExtensionTestCase( self.assertEqual(new_policy, updated_sg['security_group']['policy']) def test_secgroup_update_no_policy_change(self): + # Test updating without changing the policy old_policy = 'policy-5' + desc = 'abc' res = self._create_secgroup_with_policy(old_policy) sg = self.deserialize(self.fmt, res) - data = {'security_group': {'description': 'abc'}} + data = {'security_group': {'description': desc}} req = self.new_update_request('security-groups', data, sg['security_group']['id']) updated_sg = self.deserialize(self.fmt, req.get_response(self.ext_api)) self.assertEqual(old_policy, updated_sg['security_group']['policy']) + self.assertEqual(desc, updated_sg['security_group']['description']) def test_secgroup_update_remove_policy(self): - old_policy = 'policy-5' - new_policy = None - res = self._create_secgroup_with_policy(old_policy) - sg = self.deserialize(self.fmt, res) - data = {'security_group': {'policy': new_policy}} + # removing the policy is not allowed + sg = self._get_secgroup_with_policy() + data = {'security_group': {'policy': None}} + req = self.new_update_request('security-groups', data, + sg['security_group']['id']) + res = req.get_response(self.ext_api) + self.assertEqual(400, res.status_int) + + def test_secgroup_update_add_logging(self): + # We do not support policy & logging together + sg = self._get_secgroup_with_policy() + data = {'security_group': {'logging': True}} req = self.new_update_request('security-groups', data, sg['security_group']['id']) res = req.get_response(self.ext_api) self.assertEqual(400, res.status_int) def test_non_admin_cannot_delete_policy_sg_and_admin_can(self): - policy_id = 'policy-5' - res = self._create_secgroup_with_policy(policy_id) - sg = self.deserialize(self.fmt, res) + sg = self._get_secgroup_with_policy() sg_id = sg['security_group']['id'] # Try deleting the request as a normal user returns forbidden @@ -121,3 +146,54 @@ class SecGroupPolicyExtensionTestCase( # can be deleted though as admin self._delete('security-groups', sg_id, expected_code=webob.exc.HTTPNoContent.code) + + def test_create_rule(self): + sg = self._get_secgroup_with_policy() + rule = self._build_security_group_rule( + sg['security_group']['id'], 'ingress', + constants.PROTO_NAME_TCP, '22', '22') + res = self._create_security_group_rule(self.fmt, rule) + self.deserialize(self.fmt, res) + self.assertEqual(400, res.status_int) + + +class SecGroupPolicyExtensionTestCaseWithRules( + SecGroupPolicyExtensionTestCase): + + def setUp(self, plugin=PLUGIN_NAME, ext_mgr=None): + cfg.CONF.set_override('allow_tenant_rules_with_policy', + True, group='nsxv') + super(SecGroupPolicyExtensionTestCaseWithRules, self).setUp( + plugin=plugin, ext_mgr=ext_mgr) + + def test_secgroup_create_without_policy(self): + # in case allow_tenant_rules_with_policy is True, it is allowed to + # create a regular sg + res = self._create_secgroup_with_policy(None) + sg = self.deserialize(self.fmt, res) + self.assertIsNone(sg['security_group']['policy']) + + def test_secgroup_create_without_policy_update_policy(self): + # Create a regular security group. adding the policy later should fail + res = self._create_secgroup_with_policy(None) + sg = self.deserialize(self.fmt, res) + data = {'security_group': {'policy': 'policy-1'}} + req = self.new_update_request('security-groups', data, + sg['security_group']['id']) + res = req.get_response(self.ext_api) + self.assertEqual(400, res.status_int) + + def test_secgroup_create_without_policy_and_rule(self): + # Test that regular security groups can have rules + res = self._create_secgroup_with_policy(None) + sg = self.deserialize(self.fmt, res) + self.assertIsNone(sg['security_group']['policy']) + + rule = self._build_security_group_rule( + sg['security_group']['id'], 'ingress', + constants.PROTO_NAME_TCP, '22', '22') + res = self._create_security_group_rule(self.fmt, rule) + rule_data = self.deserialize(self.fmt, res) + self.assertEqual( + sg['security_group']['id'], + rule_data['security_group_rule']['security_group_id'])