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, [email protected] 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, [email protected] wrote: >>> From: marios <[email protected]> >>> >>> related to https://issues.apache.org/jira/browse/DTACLOUD-316 >>> https://github.com/ruby-openstack/ruby-openstack/pull/13 >>> >>> Signed-off-by: marios <[email protected]> >>> --- >>> .../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 >> >> >
