Fail validation when security groups and interfaces conflict

The 'SecurityGroups' property of the instance is not taken into account
by Nova when the 'NetworkInterfaces' property is also defined. Instead the
security groups should be applied to the network interface resource(s).

This issue has already raised a couple of invalid bugs on Launchpad.

Change-Id: I5ae2ba356518d391893c2a479c5952b13b6e8d55

diff --git a/heat/engine/resources/instance.py b/heat/engine/resources/instance.py
index 0381f960661bffc2c06a6f71319e259933f8c957..5fbc863016289acf66f0b7d261f96d2e15c9b44e 100644 (file)
--- a/heat/engine/resources/instance.py
+++ b/heat/engine/resources/instance.py
@@ -269,7 +269,7 @@ class Instance(resource.Resource):
 
         return nics
 
-    def handle_create(self):
+    def _get_security_groups(self):
         security_groups = []
         for property in ('SecurityGroups', 'SecurityGroupIds'):
             if self.properties.get(property) is not None:
@@ -277,6 +277,10 @@ class Instance(resource.Resource):
                     security_groups.append(sg)
         if not security_groups:
             security_groups = None
+        return security_groups
+
+    def handle_create(self):
+        security_groups = self._get_security_groups()
 
         userdata = self.properties['UserData'] or ''
         flavor = self.properties['InstanceType']
@@ -405,19 +409,21 @@ class Instance(resource.Resource):
             return res
 
         # check validity of key
-        try:
-            key_name = self.properties['KeyName']
-            if key_name is None:
-                return
-        except ValueError:
-            return
-        else:
+        key_name = self.properties.get('KeyName', None)
+        if key_name:
             keypairs = self.nova().keypairs.list()
-            for k in keypairs:
-                if k.name == key_name:
-                    return
-        return {'Error':
-                'Provided KeyName is not registered with nova'}
+            if not any(k.name == key_name for k in keypairs):
+                return {'Error':
+                        'Provided KeyName is not registered with nova'}
+
+        # check validity of security groups vs. network interfaces
+        security_groups = self._get_security_groups()
+        if security_groups and self.properties.get('NetworkInterfaces'):
+            return {'Error':
+                    'Cannot define both SecurityGroups/SecurityGroupIds and '
+                    'NetworkInterfaces properties.'}
+
+        return
 
     def _delete_server(self, server):
         '''

diff --git a/heat/tests/test_validate.py b/heat/tests/test_validate.py
index 019a1ac9554706854f326989e2d357eae8adc554..5d3bfc1d6c91f138571ff941e520ff4672419225 100644 (file)
--- a/heat/tests/test_validate.py
+++ b/heat/tests/test_validate.py
@@ -341,6 +341,94 @@ test_template_volume_snapshot = '''
 }
 '''
 
+test_unregistered_key = '''
+{
+  "AWSTemplateFormatVersion" : "2010-09-09",
+  "Description" : "test.",
+  "Parameters" : {
+
+    "KeyName" : {
+''' + \
+    '"Description" : "Name of an existing EC2' + \
+    'KeyPair to enable SSH access to the instances",' + \
+    '''
+          "Type" : "String"
+        }
+      },
+
+      "Resources" : {
+        "Instance": {
+          "Type": "AWS::EC2::Instance",
+          "Properties": {
+            "ImageId": "image_name",
+            "InstanceType": "m1.large",
+            "KeyName": { "Ref" : "KeyName" }
+          }
+        }
+      }
+    }
+    '''
+
+test_template_invalid_secgroups = '''
+{
+  "AWSTemplateFormatVersion" : "2010-09-09",
+  "Description" : "test.",
+  "Parameters" : {
+
+    "KeyName" : {
+''' + \
+    '"Description" : "Name of an existing EC2' + \
+    'KeyPair to enable SSH access to the instances",' + \
+    '''
+          "Type" : "String"
+        }
+      },
+
+      "Resources" : {
+        "Instance": {
+          "Type": "AWS::EC2::Instance",
+          "Properties": {
+            "ImageId": "image_name",
+            "InstanceType": "m1.large",
+            "KeyName": { "Ref" : "KeyName" },
+            "SecurityGroups": [ "default" ],
+            "NetworkInterfaces": [ "mgmt", "data" ]
+          }
+        }
+      }
+    }
+    '''
+
+test_template_invalid_secgroupids = '''
+{
+  "AWSTemplateFormatVersion" : "2010-09-09",
+  "Description" : "test.",
+  "Parameters" : {
+
+    "KeyName" : {
+''' + \
+    '"Description" : "Name of an existing EC2' + \
+    'KeyPair to enable SSH access to the instances",' + \
+    '''
+          "Type" : "String"
+        }
+      },
+
+      "Resources" : {
+        "Instance": {
+          "Type": "AWS::EC2::Instance",
+          "Properties": {
+            "ImageId": "image_name",
+            "InstanceType": "m1.large",
+            "KeyName": { "Ref" : "KeyName" },
+            "SecurityGroupIds": [ "default" ],
+            "NetworkInterfaces": [ "mgmt", "data" ]
+          }
+        }
+      }
+    }
+    '''
+
 
 class validateTest(HeatTestCase):
     def setUp(self):
@@ -487,3 +575,44 @@ class validateTest(HeatTestCase):
         engine = service.EngineService('a', 't')
         res = dict(engine.validate_template(None, t))
         self.assertEqual(res, {'Description': u'test.', 'Parameters': {}})
+
+    def test_unregistered_key(self):
+        t = template_format.parse(test_unregistered_key)
+        template = parser.Template(t)
+        params = parser.Parameters(
+            'test_stack', template, {'KeyName': 'not_registered'})
+        stack = parser.Stack(None, 'test_stack', template, params)
+
+        self.m.StubOutWithMock(instances.Instance, 'nova')
+        instances.Instance.nova().AndReturn(self.fc)
+        instances.Instance.nova().AndReturn(self.fc)
+        self.m.ReplayAll()
+
+        resource = stack.resources['Instance']
+        self.assertNotEqual(resource.validate(), None)
+
+    def test_invalid_security_groups_with_nics(self):
+        t = template_format.parse(test_template_invalid_secgroups)
+        template = parser.Template(t)
+        params = parser.Parameters('test_stack', template, {'KeyName': 'test'})
+        stack = parser.Stack(None, 'test_stack', template, params)
+
+        self.m.StubOutWithMock(instances.Instance, 'nova')
+        instances.Instance.nova().AndReturn(self.fc)
+        self.m.ReplayAll()
+
+        resource = stack.resources['Instance']
+        self.assertNotEqual(resource.validate(), None)
+
+    def test_invalid_security_group_ids_with_nics(self):
+        t = template_format.parse(test_template_invalid_secgroupids)
+        template = parser.Template(t)
+        params = parser.Parameters('test_stack', template, {'KeyName': 'test'})
+        stack = parser.Stack(None, 'test_stack', template, params)
+
+        self.m.StubOutWithMock(instances.Instance, 'nova')
+        instances.Instance.nova().AndReturn(self.fc)
+        self.m.ReplayAll()
+
+        resource = stack.resources['Instance']
+        self.assertNotEqual(resource.validate(), None)