[PATCH conductor] Verify image exists before importing it from provider
Richard Su
rwsu at redhat.com
Thu Dec 22 22:22:44 UTC 2011
On 12/22/2011 11:37 AM, Matt Wagner wrote:
> Resolves https://bugzilla.redhat.com/show_bug.cgi?id=765714
> ---
> src/app/controllers/api/images_controller.rb | 7 +++-
> src/app/controllers/images_controller.rb | 6 ++-
> src/config/locales/en.yml | 2 +
> src/lib/exceptions.rb | 4 ++
> src/lib/image.rb | 8 +++-
> src/spec/controllers/api/images_controller_spec.rb | 41 +++++++++++++++++++-
> 6 files changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/src/app/controllers/api/images_controller.rb b/src/app/controllers/api/images_controller.rb
> index db82553..3d3c448 100644
> --- a/src/app/controllers/api/images_controller.rb
> +++ b/src/app/controllers/api/images_controller.rb
> @@ -68,7 +68,12 @@ module Api
> account = ProviderAccount.find_by_label(req[:params][:provider_account_name])
> raise(Aeolus::Conductor::API::ProviderAccountNotFound.new(404, t("api.error_messages.provider_account_not_found",
> :name => req[:params][:provider_account_name]))) unless account.present?
> - @image = Image.import(account, req[:params][:target_identifier], req[:params][:image_descriptor])
> + begin
> + @image = Image.import(account, req[:params][:target_identifier], req[:params][:image_descriptor])
> + rescue Aeolus::Conductor::UI::ImageNotFound
> + raise(Aeolus::Conductor::API::ImageNotFound.new(404, t("api.error_messages.image_not_found_on_provider",
> + :image => req[:params][:target_identifier])))
> + end
> respond_with(@image)
> end
> rescue ActiveResource::BadRequest => e
> diff --git a/src/app/controllers/images_controller.rb b/src/app/controllers/images_controller.rb
> index d793432..6df9a77 100644
> --- a/src/app/controllers/images_controller.rb
> +++ b/src/app/controllers/images_controller.rb
> @@ -100,7 +100,11 @@ class ImagesController< ApplicationController
> redirect_to image_url(image.id) and return
> rescue Exception => e
> logger.error "Caught exception importing image: #{e.message}"
> - flash[:error] = t("images.import.image_not_imported")
> + if e.is_a?(Aeolus::Conductor::UI::ImageNotFound)
> + flash[:error] = t('images.not_on_provider')
> + else
> + flash[:error] = t("images.import.image_not_imported")
> + end
> redirect_to new_image_url(:tab => 'import')
> end
> end
> diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
> index 6c0ac1b..4456633 100644
> --- a/src/config/locales/en.yml
> +++ b/src/config/locales/en.yml
> @@ -741,6 +741,7 @@ en:
> make_deployable: Automatically make "%{name}" deployable.
> back: Back
> save_template: Save Template
> + not_on_provider: The requested image was not found on the provider.
> template_xml:
> errors:
> xml_parse_error: Failed to parse XML.
> @@ -1188,6 +1189,7 @@ en:
> error_messages:
> build_not_found: Could not find Build %{build}
> image_not_found: Could not find Image %{image}
> + image_not_found_on_provider: "Could not find Image %{image} on provider"
> in_grabbing_target_images: "In grabbing list of target images: %{error}"
> invalid_parameters_for_account_and_target_image: "Invalid parameters for Account: %{account} TargetImage: %{targetimage}"
> no_image_build_or_target_image: "Invalid Parameters: No Image, Build or TargetImage provided in request"
> diff --git a/src/lib/exceptions.rb b/src/lib/exceptions.rb
> index 2068fe6..6c11681 100644
> --- a/src/lib/exceptions.rb
> +++ b/src/lib/exceptions.rb
> @@ -42,5 +42,9 @@ module Aeolus
> class TargetImageStatusNotFound< Error; end
> class TargetNotFound< Error; end
> end
> +
> + module UI
> + class ImageNotFound< StandardError; end
> + end
> end
> end
> diff --git a/src/lib/image.rb b/src/lib/image.rb
> index 2c8db1a..d903e94 100644
> --- a/src/lib/image.rb
> +++ b/src/lib/image.rb
> @@ -16,8 +16,14 @@
>
> class Image
> # Given a ProviderAccount and an AMI/image ID on a provider, plus an optional XML string, use aeolus-image
> - # to import the image. Returns an Aeolus::Image::Factory::Image or returns any raised exceptions
> + # to import the image. Returns an Aeolus::Image::Factory::Image or allows exceptions to bubble up
> def self.import(provider_account, image_id, xml=nil)
> + # Verify that the image exists prior to import
> + conn = provider_account.connect
> + img = conn.image(image_id) rescue nil
> + raise Aeolus::Conductor::UI::ImageNotFound unless img.present?
> + # We have the image name in the cloud provider, so we might as well use it
> + xml ||= "<image><name>#{img.name}</name></image>" if img.name.present?
> provider = provider_account.provider
> account_id = provider_account.credentials_hash['username']
> Aeolus::Image.import(provider.name, provider.provider_type.deltacloud_driver, image_id, account_id, xml)
> diff --git a/src/spec/controllers/api/images_controller_spec.rb b/src/spec/controllers/api/images_controller_spec.rb
> index 09481c8..d3fb24f 100644
> --- a/src/spec/controllers/api/images_controller_spec.rb
> +++ b/src/spec/controllers/api/images_controller_spec.rb
> @@ -257,11 +257,15 @@ describe Api::ImagesController do
> # We previously stubbed this out to return nil... That's inappropriate here:
> Aeolus::Image::Warehouse::Image.stub(:find).and_return(@image)
> @provider_image.stub(:set_attr)
> + @deltacloud_connection = mock("Deltacloud::API")
> + @provider_account.stub(:connect).and_return(@deltacloud_connection)
> + @deltacloud_image = mock("Deltacloud::API::Stateful::Image",
> + :name => 'mock-image')
> + @deltacloud_connection.stub(:image).and_return(@deltacloud_image)
>
> request.env['RAW_POST_DATA'] = xml.to_xml
> post :create
> end
> -
> it { response.response_code == 200 }
> it { response.headers['Content-Type'].should include("application/xml") }
> it "should have an image with correct attributes" do
> @@ -273,6 +277,41 @@ describe Api::ImagesController do
>
> end
>
> + context "when trying to import image that does not exist" do
> + before(:each) do
> + xml = Nokogiri::XML::Builder.new do
> + image {
> + target_identifier "tid2"
> + image_descriptor {
> + child "c3"
> + child "c4"
> + }
> + provider_account_name "mock2"
> + }
> + end
> + Aeolus::Image::Factory::Image.stub(:new).and_return(@image)
> + ProviderAccount.stub(:find_by_label).and_return(@provider_account)
> + @provider_account.stub(:provider).and_return(@provider)
> + @image.stub(:save!)
> + # We previously stubbed this out to return nil... That's inappropriate here:
> + Aeolus::Image::Warehouse::Image.stub(:find).and_return(@image)
> + @provider_image.stub(:set_attr)
> + @deltacloud_connection = mock("Deltacloud::API")
> + @provider_account.stub(:connect).and_return(@deltacloud_connection)
> + @deltacloud_connection.stub(:image).and_raise("no image")
> +
> + request.env['RAW_POST_DATA'] = xml.to_xml
> + post :create
> + end
> + it { response.response_code == 404 }
> + it { response.headers['Content-Type'].should include("application/xml") }
> + it "should include an error" do
> + resp = Hash.from_xml(response.body)
> + resp['error']['message'].should == "Could not find Image tid2 on provider"
> + end
> +
> + end
> +
> end
>
> describe "#destroy" do
Ack. I suggest we change the UI::ImageNotFound exception to
Base::ImageNotFound because raising a API exception from rescuing a UI
exception is a bit confusing.
Verified this also works through aeolus-image:
aeolus-image import --provider_account vsphere --id blah
ERROR: ImageNotFound => Could not find Image blah on provider
More information about the aeolus-devel
mailing list