Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Rackspace Cloud Block Storage models have the potential to create unintended resources #1404

Merged
merged 2 commits into from

3 participants

@krames
Owner

This pull request fixes #1402

The save method in both Fog::Rackspace::BlockStorage::Volume and Fog::Rackspace::BlockStorage::Snapshot create new cloud resources every time they are called. This has the potential to create unintended resources.

Kyle Rames updated save method in Fog::Rackspace::BlockStorage::Volume and Fog::…
…Rackspace::BlockStorage::Snapshot to skip creating cloud reources if identity was already set Fixes #1402
1efffe5
@geemus
Owner

@krames - For the sake of consistency with other things in fog this should probably raise an error (rather than returning true) for saves that fail. I think this better communicates what is up and avoids any confusion about save and update being equivalent. See: https://github.com/fog/fog/blob/master/lib/fog/rackspace/models/compute/server.rb#L69 Thoughts?

@geemus
Owner

@krames - also, looks like one of your other commits and this one conflict, so a rebase would be awesome. Thanks!

@krames
Owner

No problem! I will update it to throw an error and rebase the code.

Kyle Rames Updated the save method in Fog::Rackspace::BlockStorage::Volume and F…
…og::Rackspace::BlockStorage::Snapshot to throw an exception if the identity attribute is set per geemus; rebased code to latest master
611870f
@krames
Owner

This should be good to go. @brianhartsock can you review and merge this one for me?

Thanks!

@brianhartsock
Collaborator

Hmm. I wish the API for block and compute had a more well defined "update" (like https://github.com/fog/fog/blob/master/lib/fog/rackspace/models/load_balancers/load_balancer.rb#L166) but they don't...

With that said, LGTM

@brianhartsock brianhartsock merged commit a0d49d3 into from
@alanthing alanthing referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 20, 2012
  1. updated save method in Fog::Rackspace::BlockStorage::Volume and Fog::…

    Kyle Rames authored
    …Rackspace::BlockStorage::Snapshot to skip creating cloud reources if identity was already set Fixes #1402
  2. Updated the save method in Fog::Rackspace::BlockStorage::Volume and F…

    Kyle Rames authored
    …og::Rackspace::BlockStorage::Snapshot to throw an exception if the identity attribute is set per geemus; rebased code to latest master
This page is out of date. Refresh to see the latest.
View
1  lib/fog/rackspace/block_storage.rb
@@ -4,6 +4,7 @@ module Fog
module Rackspace
class BlockStorage < Fog::Service
+ class IdentifierTaken < Fog::Errors::Error; end
class ServiceError < Fog::Rackspace::Errors::ServiceError; end
class InternalServerError < Fog::Rackspace::Errors::InternalServerError; end
class BadRequest < Fog::Rackspace::Errors::BadRequest; end
View
1  lib/fog/rackspace/models/block_storage/snapshot.rb
@@ -26,6 +26,7 @@ def ready?
def save(force = false)
requires :volume_id
+ raise IdentifierTaken.new('Resaving may cause a duplicate snapshot to be created') if identity
data = connection.create_snapshot(volume_id, {
:display_name => display_name,
:display_description => display_description,
View
1  lib/fog/rackspace/models/block_storage/volume.rb
@@ -42,6 +42,7 @@ def create_snapshot(options={})
def save
requires :size
+ raise IdentifierTaken.new('Resaving may cause a duplicate volume to be created') if identity
data = connection.create_volume(size, {
:display_name => display_name,
:display_description => display_description,
View
29 tests/rackspace/models/block_storage/snapshot_tests.rb
@@ -3,18 +3,25 @@
pending if Fog.mocking?
service = Fog::Rackspace::BlockStorage.new
- volume = service.volumes.create({
- :display_name => "fog_#{Time.now.to_i.to_s}",
- :size => 100
- })
+ begin
+ volume = service.volumes.create({
+ :display_name => "fog_#{Time.now.to_i.to_s}",
+ :size => 100
+ })
- volume.wait_for { ready? }
+ volume.wait_for { ready? }
- options = { :display_name => "fog_#{Time.now.to_i.to_s}", :volume_id => volume.id }
- model_tests(service.snapshots, options, false) do
- @instance.wait_for { ready? }
- end
+ options = { :display_name => "fog_#{Time.now.to_i.to_s}", :volume_id => volume.id }
+ model_tests(service.snapshots, options, false) do
+ @instance.wait_for { ready? }
+
+ tests('double save').raises(Fog::Rackspace::BlockStorage::IdentifierTaken) do
+ @instance.save
+ end
+ end
- volume.wait_for { snapshots.empty? }
- volume.destroy
+ volume.wait_for { snapshots.empty? }
+ ensure
+ volume.destroy if volume
+ end
end
View
8 tests/rackspace/models/block_storage/volume_tests.rb
@@ -6,7 +6,11 @@
options = { :display_name => "fog_#{Time.now.to_i.to_s}", :size => 100 }
model_tests(service.volumes, options, false) do
- @instance.wait_for { ready? }
+ @instance.wait_for(timeout=1200) { ready? }
+
+ tests('double save').raises(Fog::Rackspace::BlockStorage::IdentifierTaken) do
+ @instance.save
+ end
tests('#attached?').succeeds do
@instance.state = 'in-use'
@@ -20,7 +24,7 @@
returns(true) { @instance.snapshots.first.id == snapshot.id }
ensure
- snapshot.destroy
+ snapshot.destroy if snapshot
end
end
Something went wrong with that request. Please try again.