Security Group perms of FromPort 0 and ToPort -1

[jacobo]

Security Group perms of FromPort 0 and ToPort -1

Fix generation of the FromPort and ToPort paramters when authorizing or revoking permissions on a security group.

Using min and max to get the parts of a range can somtimes return nil, using begin and end will always return the actual values the range was created with

Example: (0..-1).min is nil and (0..-1).max is also nil

Error message from AWS would be:

Invalid value 'Must specify both from and to ports with ICMP.' for portRange

[geemus]

I can see 0 as min, but why -1 as the default for max? Why not 0 for both? Or maybe even some other value? Sorry if I just lack context/understanding on this, haven't messed with this feature in ages.

[jacobo]

because (0..0).max == 0 and (0..-1).max == nil ... really just workaround the shortcoming of ruby ranges...

an alternative might be something like (0..-1).to_s.split("..").map(&:to_i)

If we could go back in time and just accept 2 arguments instead of a range it would probably be cleaner..

Or maybe we need a new object PortRange...
class PortRange; def initialize(min,max) ...

[geemus]

Well, my question was maybe rather unclear. More directly, what is the context in which 0..-1 is the intended/desired value?

[jacobo]

Apparently 0 to -1 is a valid ICMP port range... we were trying to delete a security group, and had to delete permissions, ran across a permission with port range 0 to -1 so generated a call to fog with (0..-1), which turned into an amazon call which was missing "FromPort" and "ToPort", because both the min and max of that range were nil and thus the call erred and failed. This patch "fixes" that problem.

[geemus]

Got it. How about we just allow you to pass from/to port options explicitly in the options hash to override the range values? Seems like that would be more explicit/less magical. I just worry that these defaults might work in the particular case you describe but cause unintended issues in other cases. What do you think?

[jacobo]

Ok, sorry, I guess my initial "fix" was a bit hasty, here's a better one I think (It is a bit strange that the methods are called begin and end but they appear to work better than min and max)

[geemus]

@jacobo no worries, the quickest fix is usually the right one to reach for. I just was wary of side effects. I didn't even know about begin/end, sounds promising though.

[jacobo]

soo... what's next, merge? do we need tests?

[geemus]

@jacobo yes. Sorry, I didn't realize the begin/end changes were already done. Thanks!