Re: [dev] Proposal for security group promotion to the base compute API

[Tomaz Muraus <to...@apache.org>]

Great, thanks for tackling this.

It's something I already wanted to do for a while now and overall, I'm
+1 for promoting those methods to be part of the standard API.

More comments are in-line.

On Mon, Dec 30, 2013 at 9:47 PM, Chris DeRamus <ch...@gmail.com> wrote:
> I propose the following methods for security group management to be
> promoted and be a part of the standard / base compute API.
>
> * ex_list_security_groups -> list_security_groups()

+1

>
> Returns a list of class SecurityGroup
>
> * ex_create_security_groups -> create_security_group(name, **kwargs)
>
> We will use keyword arguments for this call as there are subtle differences
> between the providers (eg: groups within Amazon VPCs)

I'm -1 for using kwargs here. If there are differences between the
APIs which can't be modeled as part of the standard API, we should use
extension arguments.

Using kwargs here kinda defeats the whole purpose of a standard API.
On top of that, it makes it very unfriendly for the users, and
developers who want to programatically introspect the API.

Sadly, existing code still carries some **kwargs technical debt from
the past which has been causing nothing but pain for our users and us
(developers).

>
> * ex_authorize_security_group_ingress ->
> authorize_security_group_ingress(security_group, security_rule)
>
> Creates an ingress security group rule for a particular security group
> using a combination of protocol, to/from ports, and IP ranges or user group
> pairs.

+1

Maybe even call it "authorize_{ingress,egress}_security_group_rule"?

Keep in mind that the it doesn't need to match the old method names
and now is the perfect time to fix old mistakes and do it "right" :)

>
> * ex_authorize_security_group_egress ->
> authorize_security_group_egress(security_rule)
>
> Creates an egress security group rule for a particular security group using
> a combination of protocol, to/from ports, and IP ranges or user group
> pairs. This will be used by both Amazon and CloudStack as OpenStack does
> not support egress rules.

+1

>
> * ex_revoke_security_group_ingress ->
> revoke_security_group_ingress(security_rule)
>
> Revokes an ingress security group rule for a particular security group
> using a combination of protocol, to/from ports, and IP ranges or user group
> pairs.

+1

>
> * ex_revoke_security_group_egress ->
> revoke_security_group_egress(security_rule)
>
> Revokes an egress security group rule for a particular security group using
> a combination of protocol, to/from ports, and IP ranges or user group
> pairs. This will be used by both Amazon and CloudStack as OpenStack does
> not support egress rules.

Is it maybe possible to just have a single method (e.g.
revoke_security_group_rule) instead of two?

I would imagine that for revoking the rule you just need an ID (please
correct me if I'm wrong).

>
> * ex_delete_security_group -> delete_security_group(security_group)
>
> This will remove a security group using the ID which works across all
> providers.

Only thing which should matter here is the API. The method takes a
SecurityGroup object which means you should have access to both, id
and the name.

How the security group is deleted is an implementation detail and it
doesn't really matter if the driver uses an id or a name.

>
> * ex_delete_security_group_by_id ->
> delete_security_group_by_id(security_group)
>
> Amazon and CloudStack support removing a group by the name or the ID which
> are mutually exclusive. We will support deletion using either.

To avoid the confusion, I would prefer to only have a single method -
previously mentioned "delete_security_group".

>From Zen of Python:

"There should be one-- and preferably only one --obvious way to do it.".

>
> * ex_delete_security_group_by_name ->
> delete_security_group_by_name(security_group)
>
> Amazon and CloudStack support removing a group by the name or the ID which
> are mutually exclusive. We will support deletion using either.

Same as above.

>
> Supporting these calls means that new "SecurityGroup" and
> "SecurityGroupRule" classes which represent security groups and
> ingress/egress rules must be added.
>
> The proposal for these methods and classes are available as a gist via
> https://gist.github.com/cderamus/8182092

Great, I will add some more comments to the gist.

>
> Currently, this functionality is already implemented as the aforementioned
> extension methods in the following drivers:
>
> * CloudStack
> * OpenStack
> * EC2
>
> To preserve backward compatibility, we should also modify existing
> extension methods to call new methods and emit a deprecation warning.
>
> *Open questions*
>
> There are some differences between the various providers that will make
> this a bit complex. Here's some of the issues that I foresee and welcome
> feedback.
>
> 1.) Listing security groups for a particular node/instance becomes
> interesting. OpenStack has a specific call to list the security groups for
> a particular node which is already implemented in the driver. CloudStack
> appears to allow it using a filter when listing security groups and Amazon
> security groups can be applied to both a node, and in the case of a VPC,
> they can be applied to a particular network interface using the
> ModifyNetworkInterfaceAttribute call. There's really no clean way to
> support this so my guess is we should keep ex_ calls within the drivers
> themselves for this type of functionality. Thoughts?

Yes, I'm fine with leaving this non-standard functionality available
via the extension methods.

>
> 2.) Security group rules git a bit dicey and I'm not sure how to best
> tackle the differences. Amazon/OpenStack are virtually identical and use
> from/to ports, IP protocols and CIDR ranges or user group pairs for the
> source. They use -1 to denote all protocols and if there is an ICMP type
> the type in question is put into the from/to port values. CloudStack
> supports TCP/UDP only as valid protocol values and actually has
> icmpcode/icmptype parameters that are used for ICMP rules. For the
> SecurityGroupRule class my thoughts were to make the protocol and from/to
> ports required parameters; however, CloudStack support may throw this off.
> Information on their request parameters can be found at http://goo.gl/Byxi4U
> .

It seems like there should be a "protocol" attribute / argument with
the following possible values: all, tcp, udp, icmp. If the driver
doesn't support the specified protocol, the method should throw.

Maybe we could even add "list_supported_security_groups_protocols"
method here, kinda like the "list_record_types" method we have in the
DNS API.

Actually to be honest, I think this might be an overkill and we should
just keep it simple.

As far as the icmp type stuff goes - it seems like this should be
implemented and available via the extension arguments in the
CloudStack driver.

The proposal still needs some tweaks, but overall I think it's solid
and on the right path.

Re: [dev] Proposal for security group promotion to the base compute API

[Chris DeRamus <ch...@gmail.com>]

Thanks for the response. I'm going to be busy this week working on a few
things, but will likely try to update this with your suggestions later in
the week. Overall I agree with just about everything you've suggested and
will shore up some of the loose ends soon.

Cheers


On Sun, Jan 5, 2014 at 4:07 PM, Tomaz Muraus <to...@apache.org> wrote:

> Great, thanks for tackling this.
>
> It's something I already wanted to do for a while now and overall, I'm
> +1 for promoting those methods to be part of the standard API.
>
> More comments are in-line.
>
> On Mon, Dec 30, 2013 at 9:47 PM, Chris DeRamus <ch...@gmail.com>
> wrote:
> > I propose the following methods for security group management to be
> > promoted and be a part of the standard / base compute API.
> >
> > * ex_list_security_groups -> list_security_groups()
>
> +1
>
> >
> > Returns a list of class SecurityGroup
> >
> > * ex_create_security_groups -> create_security_group(name, **kwargs)
> >
> > We will use keyword arguments for this call as there are subtle
> differences
> > between the providers (eg: groups within Amazon VPCs)
>
> I'm -1 for using kwargs here. If there are differences between the
> APIs which can't be modeled as part of the standard API, we should use
> extension arguments.
>
> Using kwargs here kinda defeats the whole purpose of a standard API.
> On top of that, it makes it very unfriendly for the users, and
> developers who want to programatically introspect the API.
>
> Sadly, existing code still carries some **kwargs technical debt from
> the past which has been causing nothing but pain for our users and us
> (developers).
>
> >
> > * ex_authorize_security_group_ingress ->
> > authorize_security_group_ingress(security_group, security_rule)
> >
> > Creates an ingress security group rule for a particular security group
> > using a combination of protocol, to/from ports, and IP ranges or user
> group
> > pairs.
>
> +1
>
> Maybe even call it "authorize_{ingress,egress}_security_group_rule"?
>
> Keep in mind that the it doesn't need to match the old method names
> and now is the perfect time to fix old mistakes and do it "right" :)
>
> >
> > * ex_authorize_security_group_egress ->
> > authorize_security_group_egress(security_rule)
> >
> > Creates an egress security group rule for a particular security group
> using
> > a combination of protocol, to/from ports, and IP ranges or user group
> > pairs. This will be used by both Amazon and CloudStack as OpenStack does
> > not support egress rules.
>
> +1
>
> >
> > * ex_revoke_security_group_ingress ->
> > revoke_security_group_ingress(security_rule)
> >
> > Revokes an ingress security group rule for a particular security group
> > using a combination of protocol, to/from ports, and IP ranges or user
> group
> > pairs.
>
> +1
>
> >
> > * ex_revoke_security_group_egress ->
> > revoke_security_group_egress(security_rule)
> >
> > Revokes an egress security group rule for a particular security group
> using
> > a combination of protocol, to/from ports, and IP ranges or user group
> > pairs. This will be used by both Amazon and CloudStack as OpenStack does
> > not support egress rules.
>
> Is it maybe possible to just have a single method (e.g.
> revoke_security_group_rule) instead of two?
>
> I would imagine that for revoking the rule you just need an ID (please
> correct me if I'm wrong).
>
> >
> > * ex_delete_security_group -> delete_security_group(security_group)
> >
> > This will remove a security group using the ID which works across all
> > providers.
>
> Only thing which should matter here is the API. The method takes a
> SecurityGroup object which means you should have access to both, id
> and the name.
>
> How the security group is deleted is an implementation detail and it
> doesn't really matter if the driver uses an id or a name.
>
> >
> > * ex_delete_security_group_by_id ->
> > delete_security_group_by_id(security_group)
> >
> > Amazon and CloudStack support removing a group by the name or the ID
> which
> > are mutually exclusive. We will support deletion using either.
>
> To avoid the confusion, I would prefer to only have a single method -
> previously mentioned "delete_security_group".
>
> From Zen of Python:
>
> "There should be one-- and preferably only one --obvious way to do it.".
>
> >
> > * ex_delete_security_group_by_name ->
> > delete_security_group_by_name(security_group)
> >
> > Amazon and CloudStack support removing a group by the name or the ID
> which
> > are mutually exclusive. We will support deletion using either.
>
> Same as above.
>
> >
> > Supporting these calls means that new "SecurityGroup" and
> > "SecurityGroupRule" classes which represent security groups and
> > ingress/egress rules must be added.
> >
> > The proposal for these methods and classes are available as a gist via
> > https://gist.github.com/cderamus/8182092
>
> Great, I will add some more comments to the gist.
>
> >
> > Currently, this functionality is already implemented as the
> aforementioned
> > extension methods in the following drivers:
> >
> > * CloudStack
> > * OpenStack
> > * EC2
> >
> > To preserve backward compatibility, we should also modify existing
> > extension methods to call new methods and emit a deprecation warning.
> >
> > *Open questions*
> >
> > There are some differences between the various providers that will make
> > this a bit complex. Here's some of the issues that I foresee and welcome
> > feedback.
> >
> > 1.) Listing security groups for a particular node/instance becomes
> > interesting. OpenStack has a specific call to list the security groups
> for
> > a particular node which is already implemented in the driver. CloudStack
> > appears to allow it using a filter when listing security groups and
> Amazon
> > security groups can be applied to both a node, and in the case of a VPC,
> > they can be applied to a particular network interface using the
> > ModifyNetworkInterfaceAttribute call. There's really no clean way to
> > support this so my guess is we should keep ex_ calls within the drivers
> > themselves for this type of functionality. Thoughts?
>
> Yes, I'm fine with leaving this non-standard functionality available
> via the extension methods.
>
> >
> > 2.) Security group rules git a bit dicey and I'm not sure how to best
> > tackle the differences. Amazon/OpenStack are virtually identical and use
> > from/to ports, IP protocols and CIDR ranges or user group pairs for the
> > source. They use -1 to denote all protocols and if there is an ICMP type
> > the type in question is put into the from/to port values. CloudStack
> > supports TCP/UDP only as valid protocol values and actually has
> > icmpcode/icmptype parameters that are used for ICMP rules. For the
> > SecurityGroupRule class my thoughts were to make the protocol and from/to
> > ports required parameters; however, CloudStack support may throw this
> off.
> > Information on their request parameters can be found at
> http://goo.gl/Byxi4U
> > .
>
> It seems like there should be a "protocol" attribute / argument with
> the following possible values: all, tcp, udp, icmp. If the driver
> doesn't support the specified protocol, the method should throw.
>
> Maybe we could even add "list_supported_security_groups_protocols"
> method here, kinda like the "list_record_types" method we have in the
> DNS API.
>
> Actually to be honest, I think this might be an overkill and we should
> just keep it simple.
>
> As far as the icmp type stuff goes - it seems like this should be
> implemented and available via the extension arguments in the
> CloudStack driver.
>
> The proposal still needs some tweaks, but overall I think it's solid
> and on the right path.
>