class SecurityGroupDbMixin has diverged from SecurityGroupPluginBase

[Tomoe Sugihara]

Title says it all.
One notable divergence is the following method signitures:

DB mixin:

    def create_security_group(self, context, security_group, default_sg=False):

Base:

    def create_security_group(self, context, security_group):

[Aaron Rosen (arosen)]

Hi Tomoe,

In my opinion this is fine. The create_security group() in the DB Mixin has the extra param 'default_sg' in order to break out of a recursive loop with it's self in case of creating the default security group. IMO this should be marked to invalid if no one disagrees.

Thanks,

Aaron

[Akihiro Motoki (amotoki)]

I agree with Aaron.
SecurityGroupPluginBase defines methods the API layer expects.
IMO it is not a problem the implementation (db mixin in this case) defines additional parameters
as long as it does not break the base API.

[Tomoe Sugihara (tomoe)]

Thanks for the comments, Aron and Akihiro.

I gave a little more thought on this. So, I think DB mixin code is the one that needs to be fixed.

Instead of overloading the create_security_group() method with default_sg parameter,
it should be doing something like this to keep the interface clean:

    def create_security_group(self, context, security_group):
        return self._do_create_security_group(self, context, security_group, default_sg=False)

On problem I had was I was trying to implement SG methods in my plugin with the DB mixin class mixed in.
I first saw the Base class to figure out the interface and I started implementing some methods with the signatures defined in Base , calling super methods inside. Here's the problem. Since the interface doesn't match with DbMixin, I get signature mismatch error.

There's also discrepancy between the Base and DB mixin; update_security_group() is defined in Base but not implemented in Db mixin. Should it be implemented, or removed from the interface?

What do you guys think?

[Akihiro Motoki (amotoki)]

I got your point. Your solution introducing _do_create_security_group sounds reasonable to me.
Each implementation may define extra arguments, but sg db mixin is a kind of base class. So it is better not to change the original interface as long as possible.

About update_security_group, allow_put of all attributes are set to False in the attribute map at the moment. It means update_security_group is never called if I understand correctly.
The best solution is to disable PUT for security group in the API layer, but I am not sure there is an easy way to disable a particular method (update_security_group in this case).
Other solution is just to remove update_security_group from the base class. Perhaps it can work. Do you test it?

[Tomoe Sugihara (tomoe)]

Hi Aaron, Akihiro,

Actually, I have a fundamental question about SG data model. Do you know why the name is not unique constrained while Amazon SG (VPC and EC2) is? I think that is complicating things here. If name were unique, you wouldn't need that extra sg_default parameter, instead you could just use the name field.

[Salvatore Orlando (salvatore-orlando)]

Hi Tomoe!
I think the SG API is just behaving in the same ways as the other areas of the Quantum API, where names are not unique.
This discussion goes abck to last July when we finalized Qauntum API v2.
The details should be here: http://lists.openstack.org/pipermail/openstack-dev/2012-July/000080.html

[Tomoe Sugihara (tomoe)]

Hi Salvatore!

Thanks for the link. But I just found a problem here. What if you have multiple"default" SGs. How would nova be able to set the REAL "default" SG to VM ports upon launching VM? Would it assign all or whichever nova finds first or whatever else? Unless there's a way to tell real default SG, it'd be ambiguous.
IMHO, the name in SG should be unique (per tenant of course) to simplify things, and I don't think the ability to have the same name among multiple resources buy us much.

What do you think?

[Aaron Rosen (arosen)]

Hi Tomie,

Currently, there is no upstream version of security groups that allows nova to do this stuff with quantum. That said, I'm working on a nova-security -group proxy that allows nova's security group calls to be proxied to quantum (https://review.openstack.org/#/c/21041/ )

Also, it does do it via tenant in order to figure out which default security group belongs to who it doesn't just use the name.

[Tomoe Sugihara (tomoe)]

Hi Aaron,

Sorry I didn't explain it well enough. My concern was what happens if you have multiple SG with same name in a single tenant.
Right now nothing prevents you from creating multiple SG with same name (although people wouldn't want to do that usually). In which case, tenant_id + name would be ambiguous.

Thanks for the link to the gerrit review for nova's quantum SG integration. I'll take a look.

Thanks!

[Aaron Rosen (arosen)]

There is no issue with this. The quantum api does not accept names only ID's so this is not a problem as the ids are unique. If you pass in a name the python-quantumclient will look up the id for that name and if there are multiple matches it will tell you to use the id.

[Tomoe Sugihara (tomoe)]

Hi Aaron,

I think I got it. Looking at your code in nova side, If you boot a server w/o sg, it still add an entry only with "name", and it'd fail if you happen to have multiple SGs with name "default" (which is unlikely) . I was thinking the fact that it'd fail is a problem, but you don't think so. I still feel like the name field + tenant_id should be unique constrained to simplify things, but I think I understand how it is now.

Thanks again for the explanations :-)

[Aaron Rosen (arosen)]

FYI, for all resouces in quantum name+tenant is not unique: i.e you can do quantum net-create net1 ; quantum net-create net1; is allowed same with security groups.

[Tomoe Sugihara (tomoe)]

Got it. Thanks.

[Akihiro Motoki (amotoki)]

In addition to what Aaron answered, DB security group a security group named with 'default' is treated specially.
We cannot create multiple SGs named with 'default' and we cannot delete 'default' SG.

If nova security group proxy breaks it, that should be a bug of it.

[Akihiro Motoki (amotoki)]

Hi Tomoe,
A general discussion is going in the bug.

You mentioned several points and they are worth fixed. Could you clarify what this bug covers?
If there are other issues, please file a new bug.

[Tomoe Sugihara (tomoe)]

Hi Aaron, Akihiro,

Sorry for bugging you a lot about "default" SG thing. You are right.
Although I was able to create multiple SGs with "default" for some reason ( I didn't turn on proxy mode ) by hitting quantum api directly, but I started seeing validation error from _validate_name_not_default() when I tried to create 2nd "default".
Thanks!

[Aaron Rosen (arosen)]

Yes, 'default' is the one exception this is the indended behavor.

[Tomoe Sugihara (tomoe)]

Hi Akihiro,

Sure. Sorry for cluttering this page.
This bug is for the divergence of API between SecurityGroupPluginBase and SecurityGroupDbMixin besides extra default parameters.

So, I was going to remove update_security_group and add ABCMeta in the base class since allow_put is False, but is that really what we want in the long term? Shouldn't update_ be implemented so users can update at least the description (not sure about the name) field?

[Aaron Rosen (arosen)]

Hi Tomoe,

Not being able to update the name or the description was an oversite I believe. If you want to give a crack at adding that I opened a bug here that you can assign yourself: https://bugs.launchpad.net/quantum/+bug/1124865

[Tomoe Sugihara (tomoe)]

Hi Aaron,

That makes sense and thanks for filing a bug. Let me see if I can work on that.

Meanwhile I'll remove the update method for now from the base class as it's confusing. That should be put it back when it is addressed.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/21490
Committed: http://github.com/openstack/quantum/commit/434534652d8f350cd3419605b8d8d22b078727b1
Submitter: Jenkins
Branch: master

commit 434534652d8f350cd3419605b8d8d22b078727b1
Author: Tomoe Sugihara <email address hidden>
Date: Mon Feb 18 15:24:55 2013 +0900

    Fix SG interface to reflect the reality

    The signitures of abstract methods in SecurityGroupPluginBase
    has diverged from db mixin implementation.
    This patch updates the methods to fix the divergence, mainly
    by removing update method from the base. Note that there's an
    issue for missing update(bug #1124865).

    Fixes: bug #1119080

    Change-Id: I6631b86ab2fdf4b81ad575e9a8f92dfe041ffe9a
    Signed-off-by: Tomoe Sugihara <email address hidden>