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