[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