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 <[email protected]> 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.
