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

@@ -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 {

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

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,
+    def _validate_security_group(self, context, security_group, default_sg,
-                                 from_create=True):
+                                 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:
+                    if sg_with_policy and not new_policy:
-                        msg = _('A security group must be assigned to a '
+                        # cannot remove a policy from an existing sg
-                                'policy')
+                        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:
+        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'
+            msg = (_('Cannot create rules for security group %s with'
-                    ' a policy') % sg_id
+                     ' a policy') % sg_id)
             raise n_exc.InvalidInput(error_message=msg)
 
         self._prevent_non_admin_delete_provider_sg(context, sg_id)

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'
+        # removing the policy is not allowed
-        new_policy = None
+        sg = self._get_secgroup_with_policy()
-        res = self._create_secgroup_with_policy(old_policy)
+        data = {'security_group': {'policy': None}}
-        sg = self.deserialize(self.fmt, res)
+        req = self.new_update_request('security-groups', data,
-        data = {'security_group': {'policy': new_policy}}
+                                      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'
+        sg = self._get_secgroup_with_policy()
-        res = self._create_secgroup_with_policy(policy_id)
-        sg = self.deserialize(self.fmt, res)
         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'])