Browse files

Fix bug causing inconsistent values for security group names

Prior to this commit, when SecurityGroups#ensure_group was
called with a list of ports that there was not yet a
security group for, the array of ports would get wrapped
in `Set.new` twice; once in the body of #group_id and once
in the body of #ensure_group.

It turns out that, based on the implementation of #group_id,
there are cases when you'd get back a different group_id when
you converted the array to a set twice.  So, e.g.:

    Zlib.crc32(Set.new([22,8140]).inspect) !=
Zlib.crc32(Set.new(Set.new([22,8140])).inspect)

but...

    Zlib.crc32(Set.new([22,8080]).inspect) ==
Zlib.crc32(Set.new(Set.new([22,8080])).inspect)

Good times.  This commit tweaks the tests so that they would
repro this issue, and also cleans up the handling of the calls
to `Set.new` so as to fix the bug.
  • Loading branch information...
1 parent 009dc45 commit 456f26cce9aae75fec54c02acc17a8dab00a9151 @cprice404 cprice404 committed Oct 3, 2012
Showing with 27 additions and 10 deletions.
  1. +11 −3 lib/blimpy/securitygroups.rb
  2. +16 −7 spec/blimpy/securitygroups_spec.rb
View
14 lib/blimpy/securitygroups.rb
@@ -8,19 +8,21 @@ def self.group_id(ports)
return nil
end
- ports = Set.new(ports)
+ unless ports.is_a? Set
+ ports = Set.new(ports)
+ end
+
# Lolwut, #hash is inconsistent between ruby processes
"Blimpy-#{Zlib.crc32(ports.inspect)}"
end
def self.ensure_group(fog, ports)
name = group_id(ports)
- ports = Set.new(ports)
exists = fog.security_groups.get(name)
if exists.nil?
- create_group(fog, ports)
+ name = create_group(fog, ports)
end
name
end
@@ -29,9 +31,15 @@ def self.create_group(fog, ports)
name = group_id(ports)
group = fog.security_groups.create(:name => name,
:description => "Custom Blimpy security group for #{ports.to_a}")
+
+ unless ports.is_a? Set
+ ports = Set.new(ports)
+ end
+
ports.each do |port|
group.authorize_port_range(port .. port)
end
+ name
end
end
end
View
23 spec/blimpy/securitygroups_spec.rb
@@ -3,7 +3,11 @@
describe Blimpy::SecurityGroups do
let(:fog) { mock('Fog object') }
- let(:ports) { [22, 8080] }
+ # Due to the implementation of the group_id method, [22,8140] can trigger
+ # a failure case that is not triggered by [22,8080].
+ # Zlib.crc32(Set.new(Set.new([22,8140])).inspect) != Zlib.crc32(Set.new([22,8140]).inspect), at least in ruby 1.8.7
+ let(:ports) { [22, 8140 ] }
+ let(:expected_group_name) { subject.group_id(ports) }
describe '#group_id' do
it 'should return nil for an empty port Array' do
@@ -28,17 +32,22 @@
it 'should bail and not try to create the group' do
fog.stub_chain(:security_groups, :get).and_return(true)
subject.should_receive(:create_group).never
- subject.should_receive(:group_id).and_return('fake-id')
name = subject.ensure_group(fog, ports)
- name.should == 'fake-id'
+ name.should == expected_group_name
end
end
context "for a group that doesn't exist" do
+ let(:sec_groups) { mock('Fog Security Groups object') }
+ let(:group) { mock('Fog SecurityGroup') }
+
it 'should create the group' do
- fog.stub_chain(:security_groups, :get).and_return(nil)
- subject.should_receive(:create_group).once
- subject.ensure_group(fog, ports)
+ fog.stub(:security_groups).and_return(sec_groups)
+ sec_groups.should_receive(:get).with(expected_group_name).and_return(nil)
+ sec_groups.should_receive(:create).and_return(group)
+ group.stub(:authorize_port_range)
+ name = subject.ensure_group(fog, ports)
+ name.should == expected_group_name
end
end
end
@@ -48,7 +57,7 @@
it 'should authorize the port ranges for every port' do
fog.stub_chain(:security_groups, :create).and_return(group)
group.should_receive(:authorize_port_range).with(22..22)
- group.should_receive(:authorize_port_range).with(8080..8080)
+ group.should_receive(:authorize_port_range).with(8140..8140)
subject.create_group(fog, ports)
end
end

0 comments on commit 456f26c

Please sign in to comment.