Merge lp:~soren/nova/secgroup-fixes into lp:~hudson-openstack/nova/trunk

[Soren Hansen]

Fix a bug that would make spawning new instances fail if no port/protocol is given (for rules granting access for other security groups).

[Soren Hansen (soren)]

https://bugs.launchpad.net/nova/+bug/838419/comments/5 says it works.

[Rick Harris (rconradharris)]

Good bug report, good patch. Nice job guys.

LGTM.

review: Approve

[Vish Ishaya (vishvananda)]

This definitely fixes the linked bug, but it doesn't really address the way that security groups with no source ports are "supposed to" work.

According to this bug:

https://bugs.launchpad.net/nova/+bug/829609

When you specify no source group, amazon actually creates 3 separate rules to allow all tcp udp and icmp traffic from the group. So it seems like empty ones actually should be converted.

I think there are two options for this:

a) create the 3 rules when we first receive the request and migrate old empty rules into three rules

b) continue to allow them to be created as is, but have a conversion that displays them as three rules to the client, and implements allow all when modifying ip tables.

If i read your patch correctly, you've done the part of b which makes them allow all traffic, but we still haven't made them display properly to the client. I think I'm ok with this if we add a bug for fixing the client display as well, although I am concerned about certain edge cases. Like what happens if someone tries to revoke one of the faux rules?

review: Needs Information

[Vish Ishaya (vishvananda)]

Interesting. As far as I can tell, amazon does not support allowing ports from source groups. It is all or nothing:

euca2ools 1.3:

vishvananda@firefly:~$ euca-authorize -P tcp -p 22 -o test test
test test None tcp 22 22 None
InvalidParameterCombination: The parameter 'SourceSecurityGroupName' may not be used in combination with 'FromPort'

euca2ools 1.2:

vagrant@nova:/tmp/bzr/lp819477$ euca-authorize -o test test
GROUP test
PERMISSION test ALLOWS GRPNAME test
vagrant@nova:/tmp/bzr/lp819477$ euca-describe-groups
GROUP 911567224702 test test
PERMISSION 911567224702 test ALLOWS tcp 1 65535 GRPNAME test
PERMISSION 911567224702 test ALLOWS udp 1 65535 GRPNAME test
PERMISSION 911567224702 test ALLOWS icmp -1 -1 GRPNAME test

There doesn't seem to be any way to revoke just one of those rules. So I guess you're fix is fine as long as we change the display of the group to show all 3 on describe.

[Christopher MacGown (0x44)]

This fix looks good, Approve with the change Vish suggested above.

review: Approve

[Soren Hansen (soren)]

Vish, happy with it like this?

[Vish Ishaya (vishvananda)]

This is good, although it might be nice to show both source groups with a specific protocol and without. Seems like it could be accomplished with another if inside the if rule.group_id:

if rule.protocol:
  # show one rule
else:
  # show three rules

Thoughts?

Vish

[Soren Hansen (soren)]

> This is good, although it might be nice to show both source groups with a
> specific protocol and without. Seems like it could be accomplished with
> another if inside the if rule.group_id:
>
> if rule.protocol:
> # show one rule
> else:
> # show three rules
>
> Thoughts?

Depends. Do we want to allow source group filters with protocols and ports and stuff if EC2 doesn't?

[Vish Ishaya (vishvananda)]

Extra feature!

Well it doesn't appear that you changed the actual rule creation code, so it will be supported as is regardless. It just won't display them properly.

[Vish Ishaya (vishvananda)]

ping. I thought you mentioned in irc that you were going to make the change to show 3 or one rule?

[Vish Ishaya (vishvananda)]

thx soren.

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~soren/nova/secgroup-fixes into lp:nova failed. Below is the output from the failed tests.

CloudTestCase
    test_ajax_console OK 1.89
    test_allocate_address OK 0.27
    test_associate_disassociate_address OK 1.44
    test_authorize_revoke_security_group_ingress_by_id OK 0.36
    test_authorize_security_group_fail_missing_source_group OK 0.27
    test_authorize_security_group_ingress OK 0.29
    test_authorize_security_group_ingress_already_exists OK 0.48
    test_authorize_security_group_ingress_ip_permissions_groups OK 0.33
    test_authorize_security_group_ingress_ip_permissions_ip_rangesOK 0.28
    test_authorize_security_group_ingress_missing_group_name_or_idOK 0.19
    test_authorize_security_group_ingress_missing_protocol_paramsOK 0.25
    test_console_output OK 1.39
    test_create_delete_security_group OK 0.45
    test_create_image OK 4.16
    test_create_snapshot OK 0.46
    test_create_volume_from_snapshot OK 0.63
    test_delete_key_pair OK 0.42
    test_delete_security_group_by_id OK 0.25
    test_delete_security_group_no_params OK 0.20
    test_delete_security_group_with_bad_group_id OK 0.22
    test_delete_security_group_with_bad_name OK 0.23
    test_delete_snapshot OK 0.45
    test_deregister_image OK 0.22
    test_deregister_image_wrong_container_type OK 0.20
    test_describe_addresses OK 0.51
    test_describe_availability_zones OK 0.24
    test_describe_image_attribute OK 0.19
    test_describe_image_attribute_block_device_mapping OK 0.20
    test_describe_image_attribute_root_device_name OK 0.20
    test_describe_image_mapping OK 0.21
    test_describe_images OK 0.20
    test_describe_instance_attribute OK 0.21
    test_describe_instances OK 0.41
    test_describe_instances_bdm OK 1.48
    test_describe_instances_deleted OK 0.27
    test_describe_key_pairs OK 0.69
    test_describe_regions OK 0.22
    test_describe_security_group_ingress_groups OK 0.47
    test_describe_security_groups OK 0.29
    test_describe_security_groups_by_id OK 0.33
    test_describe_snapshots OK 0.23
    test_describe_volumes OK 0.28...

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~soren/nova/secgroup-fixes into lp:nova failed. Below is the output from the failed tests.

CloudTestCase
    test_ajax_console OK 1.88
    test_allocate_address OK 0.26
    test_associate_disassociate_address OK 1.42
    test_authorize_revoke_security_group_ingress_by_id OK 0.35
    test_authorize_security_group_fail_missing_source_group OK 0.27
    test_authorize_security_group_ingress OK 0.28
    test_authorize_security_group_ingress_already_exists OK 0.48
    test_authorize_security_group_ingress_ip_permissions_groups OK 0.33
    test_authorize_security_group_ingress_ip_permissions_ip_rangesOK 0.28
    test_authorize_security_group_ingress_missing_group_name_or_idOK 0.20
    test_authorize_security_group_ingress_missing_protocol_paramsOK 0.26
    test_console_output OK 1.35
    test_create_delete_security_group OK 0.43
    test_create_image OK 5.29
    test_create_snapshot OK 0.46
    test_create_volume_from_snapshot OK 0.63
    test_delete_key_pair OK 0.43
    test_delete_security_group_by_id OK 0.24
    test_delete_security_group_no_params OK 0.19
    test_delete_security_group_with_bad_group_id OK 0.21
    test_delete_security_group_with_bad_name OK 0.22
    test_delete_snapshot OK 0.44
    test_deregister_image OK 0.19
    test_deregister_image_wrong_container_type OK 0.19
    test_describe_addresses OK 0.50
    test_describe_availability_zones OK 0.25
    test_describe_image_attribute OK 0.20
    test_describe_image_attribute_block_device_mapping OK 0.21
    test_describe_image_attribute_root_device_name OK 0.19
    test_describe_image_mapping OK 0.19
    test_describe_images OK 0.20
    test_describe_instance_attribute OK 0.20
    test_describe_instances OK 0.40
    test_describe_instances_bdm OK 1.49
    test_describe_instances_deleted OK 0.28
    test_describe_key_pairs OK 0.58
    test_describe_regions OK 0.22
    test_describe_security_group_ingress_groups OK 0.47
    test_describe_security_groups OK 0.30
    test_describe_security_groups_by_id OK 0.32
    test_describe_snapshots OK 0.24
    test_describe_volumes OK 0.27...

[Dan Prince (dan-prince)]

Soren:

I'm also getting a failure w/ this branch on Smokestack:

(nova.rpc): TRACE: File "/usr/lib/pymodules/python2.6/nova/network/linux_net.py", line 614, in update_dhcp
(nova.rpc): TRACE: _execute(*cmd, run_as_root=True)
(nova.rpc): TRACE: File "/usr/lib/pymodules/python2.6/nova/network/linux_net.py", line 710, in _execute
(nova.rpc): TRACE: return utils.execute(*cmd, **kwargs)
(nova.rpc): TRACE: File "/usr/lib/pymodules/python2.6/nova/utils.py", line 188, in execute
(nova.rpc): TRACE: cmd=' '.join(cmd))
(nova.rpc): TRACE: ProcessExecutionError: Unexpected error while running command.
(nova.rpc): TRACE: Command: sudo FLAGFILE=/etc/nova/nova.conf NETWORK_ID=1 dnsmasq --strict-order --bind-interfaces --interface=xenbr0 --conf-file= --domain=novalocal --pid-file=/var/lib/nova/networks/nova-xenbr0.pid --listen-address=192.168.0.1 --except-interface=lo --dhcp-range=192.168.0.2,static,120s --dhcp-lease-max=256 --dhcp-hostsfile=/var/lib/nova/networks/nova-xenbr0.conf --dhcp-script=/usr/bin/nova-dhcpbridge --leasefile-ro
(nova.rpc): TRACE: Exit code: 2
(nova.rpc): TRACE: Stdout: ''
(nova.rpc): TRACE: Stderr: '\ndnsmasq: failed to create listening socket for fe80::f4d9:9eff:fe86:44cf: No such device or address\n'
(nova.rpc): TRACE:
2011-09-16 17:29:27,469 ERROR nova.rpc [-] Returning exception Unexpected error while running command.

---

Let me know if you need more info.

review: Needs Fixing

[Soren Hansen (soren)]

> Let me know if you need more info.

That would be great. I have no idea what might be causing this and I can't seem to reproduce it.

[Dan Prince (dan-prince)]

Hey Soren,

My bad. The dnsmasq issue isn't from your branch at all. I get this on trunk as well. Sorry man, please disregard the above stack trace.

review: Abstain

=== modified file 'nova/api/ec2/cloud.py'
--- nova/api/ec2/cloud.py	2011-09-15 20:04:31 +0000
+++ nova/api/ec2/cloud.py	2011-09-16 19:31:24 +0000
@@ -594,18 +594,31 @@
         g['ipPermissions'] = []
         for rule in group.rules:
             r = {}
-            r['ipProtocol'] = rule.protocol
-            r['fromPort'] = rule.from_port
-            r['toPort'] = rule.to_port
             r['groups'] = []
             r['ipRanges'] = []
             if rule.group_id:
                 source_group = db.security_group_get(context, rule.group_id)
                 r['groups'] += [{'groupName': source_group.name,
                                  'userId': source_group.project_id}]
+                if rule.protocol:
+                    r['ipProtocol'] = rule.protocol
+                    r['fromPort'] = rule.from_port
+                    r['toPort'] = rule.to_port
+                    g['ipPermissions'] += [dict(r)]
+                else:
+                    for protocol, min_port, max_port in (('icmp', -1, -1),
+                                                         ('tcp', 1, 65535),
+                                                         ('udp', 1, 65536)):
+                        r['ipProtocol'] = protocol
+                        r['fromPort'] = min_port
+                        r['toPort'] = max_port
+                        g['ipPermissions'] += [dict(r)]
             else:
+                r['ipProtocol'] = rule.protocol
+                r['fromPort'] = rule.from_port
+                r['toPort'] = rule.to_port
                 r['ipRanges'] += [{'cidrIp': rule.cidr}]
-            g['ipPermissions'] += [r]
+                g['ipPermissions'] += [r]
         return g
 
     def _rule_args_to_dict(self, context, kwargs):

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-09-14 20:57:15 +0000
+++ nova/db/sqlalchemy/api.py	2011-09-16 19:31:24 +0000
@@ -2827,12 +2827,14 @@
         result = session.query(models.SecurityGroupIngressRule).\
                          filter_by(deleted=can_read_deleted(context)).\
                          filter_by(parent_group_id=security_group_id).\
+                         options(joinedload_all('grantee_group')).\
                          all()
     else:
         # TODO(vish): Join to group and check for project_id
         result = session.query(models.SecurityGroupIngressRule).\
                          filter_by(deleted=False).\
                          filter_by(parent_group_id=security_group_id).\
+                         options(joinedload_all('grantee_group')).\
                          all()
     return result
 

=== modified file 'nova/tests/api/ec2/test_cloud.py'
--- nova/tests/api/ec2/test_cloud.py	2011-09-13 21:04:13 +0000
+++ nova/tests/api/ec2/test_cloud.py	2011-09-16 19:31:24 +0000
@@ -305,6 +305,61 @@
                   'ip_protocol': u'tcp'}]}
         self.assertTrue(authz(self.context, group_name=sec['name'], **kwargs))
 
+    def test_describe_security_group_ingress_groups(self):
+        kwargs = {'project_id': self.context.project_id, 'name': 'test'}
+        sec1 = db.security_group_create(self.context, kwargs)
+        sec2 = db.security_group_create(self.context,
+                                       {'project_id': 'someuser',
+                                        'name': 'somegroup1'})
+        sec3 = db.security_group_create(self.context,
+                                       {'project_id': 'someuser',
+                                        'name': 'othergroup2'})
+        authz = self.cloud.authorize_security_group_ingress
+        kwargs = {'ip_permissions': [
+                  {'groups': {'1': {'user_id': u'someuser',
+                                    'group_name': u'somegroup1'}}},
+                  {'ip_protocol': 'tcp',
+                   'from_port': 80,
+                   'to_port': 80,
+                   'groups': {'1': {'user_id': u'someuser',
+                                    'group_name': u'othergroup2'}}}]}
+        self.assertTrue(authz(self.context, group_name=sec1['name'], **kwargs))
+        describe = self.cloud.describe_security_groups
+        groups = describe(self.context, group_name=['test'])
+        self.assertEquals(len(groups['securityGroupInfo']), 1)
+        actual_rules = groups['securityGroupInfo'][0]['ipPermissions']
+        self.assertEquals(len(actual_rules), 4)
+        expected_rules = [{'fromPort': -1,
+                           'groups': [{'groupName': 'somegroup1',
+                                       'userId': 'someuser'}],
+                           'ipProtocol': 'icmp',
+                           'ipRanges': [],
+                           'toPort': -1},
+                          {'fromPort': 1,
+                           'groups': [{'groupName': u'somegroup1',
+                                       'userId': u'someuser'}],
+                           'ipProtocol': 'tcp',
+                           'ipRanges': [],
+                           'toPort': 65535},
+                          {'fromPort': 1,
+                           'groups': [{'groupName': u'somegroup1',
+                                       'userId': u'someuser'}],
+                           'ipProtocol': 'udp',
+                           'ipRanges': [],
+                           'toPort': 65536},
+                          {'fromPort': 80,
+                           'groups': [{'groupName': u'othergroup2',
+                                       'userId': u'someuser'}],
+                           'ipProtocol': u'tcp',
+                           'ipRanges': [],
+                           'toPort': 80}]
+        for rule in expected_rules:
+            self.assertTrue(rule in actual_rules)
+
+        db.security_group_destroy(self.context, sec3['id'])
+        db.security_group_destroy(self.context, sec2['id'])
+        db.security_group_destroy(self.context, sec1['id'])
+
     def test_revoke_security_group_ingress(self):
         kwargs = {'project_id': self.context.project_id, 'name': 'test'}
         sec = db.security_group_create(self.context, kwargs)

=== modified file 'nova/tests/test_api.py'
--- nova/tests/test_api.py	2011-08-12 04:03:37 +0000
+++ nova/tests/test_api.py	2011-09-16 19:31:24 +0000
@@ -515,7 +515,7 @@
         # be good enough for that.
         for group in rv:
             if group.name == security_group_name:
-                self.assertEquals(len(group.rules), 1)
+                self.assertEquals(len(group.rules), 3)
                 self.assertEquals(len(group.rules[0].grants), 1)
                 self.assertEquals(str(group.rules[0].grants[0]), '%s-%s' %
                                   (other_security_group_name, 'fake'))

=== modified file 'nova/virt/libvirt/firewall.py'
--- nova/virt/libvirt/firewall.py	2011-08-26 17:38:20 +0000
+++ nova/virt/libvirt/firewall.py	2011-09-16 19:31:24 +0000
@@ -663,7 +663,9 @@
                 if version == 6 and rule.protocol == 'icmp':
                     protocol = 'icmpv6'
 
-                args = ['-j ACCEPT', '-p', protocol]
+                args = ['-j ACCEPT']
+                if protocol:
+                    args += ['-p', protocol]
 
                 if protocol in ['udp', 'tcp']:
                     if rule.from_port == rule.to_port: