Mailing-List: contact dev-help@deltacloud.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@deltacloud.apache.org
Received-SPF: pass (nike.apache.org: domain of mandreou@redhat.com designates
 209.132.183.28 as permitted sender)
Message-ID: <50534409.5060002@redhat.com>
Date: Fri, 14 Sep 2012 17:49:45 +0300
From: "marios@redhat.com" <mandreou@redhat.com>
User-Agent: Mozilla/5.0 (X11; Linux i686;
 rv:15.0) Gecko/20120828 Thunderbird/15.0
MIME-Version: 1.0
To: dev@deltacloud.apache.org
CC: David Lutterkort <lutter@redhat.com>
Subject: Re: [PATCH] Openstack driver - adds forced_authentication for /api...
 checks whether Openstack provider supports buckets collection (is swift
 service
 deployed?)
References: <1347453142-16940-1-git-send-email-marios@redhat.com>
 <1347567539.26683.16.camel@avon.watzmann.net> <5052EBF5.7030507@redhat.com>
In-Reply-To: <5052EBF5.7030507@redhat.com>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit

see what you think of http://tracker.deltacloud.org/set/62
(notes/intro are on the patches sent to this mailing list)

marios


On 14/09/12 11:33, marios@redhat.com wrote:
> Hi David - thanks for having a look - more inline:
> 
> On 13/09/12 23:18, David Lutterkort wrote:
>> On Wed, 2012-09-12 at 15:32 +0300, marios@redhat.com wrote:
>>> From: marios <marios@redhat.com>
>>>
>>> related to https://issues.apache.org/jira/browse/DTACLOUD-316
>>> https://github.com/ruby-openstack/ruby-openstack/pull/13
>>>
>>> Signed-off-by: marios <marios@redhat.com>
>>> ---
>>>  .../deltacloud/drivers/openstack/openstack_driver.rb  | 19 +++++++++++++------
>>>  server/lib/deltacloud/server.rb                       |  6 +++++-
>>>  2 files changed, 18 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/server/lib/deltacloud/drivers/openstack/openstack_driver.rb b/server/lib/deltacloud/drivers/openstack/openstack_driver.rb
>>> index 5b29840..ca824bd 100644
>>> --- a/server/lib/deltacloud/drivers/openstack/openstack_driver.rb
>>> +++ b/server/lib/deltacloud/drivers/openstack/openstack_driver.rb
>>> @@ -40,6 +40,19 @@ module Deltacloud
>>>  
>>>          define_hardware_profile('default')
>>>  
>>> +        def respond_to?(capability)#, credentials)
>>> +          if capability == :buckets
>>> +            begin
>>> +              client = new_client(Deltacloud.config["openstack_creds"], capability)
>>> +            rescue Deltacloud::ExceptionHandler::NotImplemented #OpenStack::Exception::NotImplemented...
>>> +              return false
>>> +            end
>>> +            return true
>>> +          else
>>> +            super(capability)
>>> +          end
>>> +        end
>>
>> This is not a good idea - it means that something as innocuous looking
>> as driver.respond_to?(:buckets) has major sideeffects and fail for all
>> kinds of reasons.
>>
>> The right fix is to change the supported_collections method in
>> deltacloud_helper.rb - this whol capability checking seems way too
>> convoluted to me, anyway.
> 
> sure - the reason I used 'respond_to' rather than 'has_capability' was
> because the check in the collections/buckets.rb looks like:
> 
>  16 module Deltacloud::Collections$
>  17   class Buckets < Base$
>  18 $
>  19     include Deltacloud::Features$
>  20 $
>  21     set :capability, lambda { |m| driver.respond_to? m }$
> 
> as this is repeated across all collections I wanted to maintain the
> consistency. At some point (and my original implementation) I changed
> respond_to? to has_capability? on line 21 above and overrode that method
> in the openstack driver instead. I can easily change this and look at
> overriding supported_collections instead.
> (more below)
> 
>  Why can't we have a
>> supported_collections(credentials) method in the base driver which, for
>> all I care, use introspection to figure out which collections are
>> available, and override that in the OpenStack driver ?
>>
>>>          def hardware_profiles(credentials, opts = {})
>>>            os = new_client(credentials)
>>>            results = []
>>> @@ -351,12 +364,6 @@ private
>>>            end
>>>          end
>>>  
>>> -        def cloudfiles_client(credentials)
>>> -          safely do
>>> -            CloudFiles::Connection.new(:username => credentials.user, :api_key => credentials.password)
>>> -          end
>>> -        end
>>> -
>>>  #NOTE: for the convert_from_foo methods below... openstack-compute
>>>  #gives Hash for 'flavors' but OpenStack::Compute::Flavor for 'flavor'
>>>  #hence the use of 'send' to deal with both cases and save duplication
>>> diff --git a/server/lib/deltacloud/server.rb b/server/lib/deltacloud/server.rb
>>> index c7c6c41..4b9485e 100644
>>> --- a/server/lib/deltacloud/server.rb
>>> +++ b/server/lib/deltacloud/server.rb
>>> @@ -44,8 +44,12 @@ module Deltacloud
>>>      set :config, Deltacloud[:deltacloud]
>>>  
>>>      get '/' do
>>> -      if params[:force_auth]
>>> +      if driver.name == "openstack" or params[:force_auth]
>>
>> Why is this check necessary ? The LazyCredentials helper already throws
>> a 401 if you try to use credentials and they are not provided.
>>
> 
> It's necessary because of the point at which the :buckets capability is
> checked - i.e. in the declaration of :buckets collection, rather than in
> a request. The LazyCredentials aren't in scope (and I couldn't get
> access to 'env' of the request context in order to initialise
> LazyCredentials) at the point where these are needed to query the
> Openstack provider. So the easiest way was to make the check at the
> 'first' request which is the entry point.
> 
>>>          return [401, 'Authentication failed'] unless driver.valid_credentials?(credentials)
>>> +        if driver.name == "openstack"
>>> +          Deltacloud.config["openstack_creds"] = credentials
>>
>> You can't do that - that's a global object shared across all threads.
>>
>>> +          #or here also works: Thread.current["openstack_creds"] = credentials
>>
>> This would be better. Though even better would be a direct
>> supported_collections(credentials) method on the drivers.
> 
> OK - will work on tidying this up today. This code is already pushed
> after the initial ACK but I can just patch ontop of that,
> 
> thanks, marios
> 
> 
>> David
>>
>>
>