Neutron SecGroup API should validate group name

I'm not sure this is needed if "API to add instance to a SecGroup should take ID not Name" (https://bugs.launchpad.net/nova/+bug/1161473) gets done.

I think its still worth fixing as a bug in the current API. Any change from names to uuids would presumably need to be part of v3

Understood. You're right, the bug I linked will be part of v3.

I was preparing the patch in order to add validation to QuantumSecurityGroupAPI that actually consist in moving the validate_property method from nova/compute/api.py to nova/network/security_group/security_group_base.py.

This modification imposes those two tests functions must be changed:
1.nova.tests.api.openstack.compute.contrib.test_quantum_security_groups.TestQuantumSecurityGroups.test_get_instance_security_groups (commit https://github.com/openstack/nova/commit/afeb95dfc924ed7e768d76bc5ae4bd4c55e9dcb3
2. nova.tests.api.openstack.compute.contrib.test_quantum_security_groups.TestQuantumSecurityGroups.test_get_instances_security_groups_bindings (commit https://github.com/openstack/nova/commit/5d40fb635b1abac3828245124e392a4c9af52f60)

The primal goal of these two tests is to not test the empty names or descriptions, but to return all the instance_id's and security groups for a given tenant.

Also if I add validation to QuantumSecurityGroupAPI there are several tests functions concerning empty/nonstring names in nova/tests/api/openstack/compute/contrib/test_quantum_security_groups.py that must be also updated (that are empty now)

So what do you think? I’m new in this community so I would like to have your opinion before committing the patch

Hi,

I don't see why you need to move the validate_property() method - isn't it more just a question of modifying the (currently null) validate_property method in networks/quantumv2/security_group/quantum_driver.py ?

Updating unit tests when a new constraint is added is to be expected - I recently added a constraint to the vm_state checks for launched_at which meant that lot of data used in unrelated unit tests had to be modified. The tests weren't wrong, they just didn't factor in this new constraint.

Hi,
thank you Phil for your comment
so the idea to move the validation method is to avoid code replication: the two SecurityGroupAPI classes of nova-network and quantum are derived from SecurityGroupBase (defined in nova/network/security_group/security_group_base.py).

Fix proposed to branch: master
Review: https://review.openstack.org/34559

A patch is up in tempest to skip the tests until this is fixed in nova (since the nova review was abandoned):

https://review.openstack.org/#/c/40666/

Talking with Phil today on IRC. Because you can now reference by UUID he said this wasn't really an issue they were into digging into any more. I'm changing to Opinion / Wishlist in case someone wants to make this part better in the future.

See comment #9 (the reset to "confirmed" was by mistake)