OpenStack Temp URL Support and AWS::File compatibility #1912

Merged
merged 8 commits into from Jun 27, 2013

Conversation

Projects
None yet
4 participants
Contributor

enterprise-rails commented Jun 24, 2013

In the context of enabling the Cloud Foundry Cloud Controller to use OpenStack Swift using the Fog HP storage provider an inconsistency in creating temporary file urls has been discovered.

While Fog::Storage::AWS::File uses File#url to create a temporary url, Fog::Storage::HP::File uses File#temp_signed_url although semantically both methods are equal.
This pull requests adds a File#url method so that both File classes offer the same interface for creating temp urls.

In order to do that another change was needed:
Many OpenStack users suggest the HP adapter to access OpenStack Swift as is it much more stable.
BUT: the HP implementation of File#temp_signed_url is currently not OpenStack compatible.
HP uses a different strategy to create signature urls avoiding the usage of a so called "account meta key" which has to be set for the Swift account for a regular OpenStack Swift.

This pull requests adds a simple logic:
It allows OpenStack users to use the HP adapter and pass "hp_account_meta_key" when creating a storage object.
When hp_account_meta_key is set, the OpenStack signature url strategy is applied.

With this pull request the Cloud Foundry Cloud Controller will be able to use Fog seamlessly to use either AWS S3, HP or OpenStack Swift as a blob storage.

More than this all Fog users will benefit from the consistent API and OpenStack users will be able to use the HP provider's File#url method to create temp urls instead of implementing it separately.

enterprise-rails added some commits Jun 24, 2013

@enterprise-rails enterprise-rails Allows to set the account meta key by setting hp_account_meta_key, ne…
…eded to generate temp urls using the HP provider, explicitly, instead of using hp_secret_key. If hp_account_meta_key is not given hp_secret_key is used as hp_account_meta_key.
a8daf7f
@enterprise-rails enterprise-rails HP uses a different strategy to create the signature that is passed t…
…o swift than OpenStack. As the HP provider is broadly used by OpenStack users the OpenStack strategy is applied when the @hp_account_meta_key is given.
1a151b0
@enterprise-rails enterprise-rails Adds Fog::Storage::HP::File#url method to enable compatibility with F…
…og::Storage::AWS::File
ea6827c

Coverage Status

Coverage remained the same when pulling ea6827c on anynines:master into 3c03e4c on fog:master.

Owner

geemus commented Jun 25, 2013

@enterprise-rails - thanks for the detailed accounting of whats up here. Looks like this introduces some test errors on CI, could you look into those: https://travis-ci.org/fog/fog/jobs/8403328#L393

@rupakg - could you review the changes and see what you think?

Coverage Status

Coverage increased (+0%) when pulling 99b1ae0 on anynines:master into 3c03e4c on fog:master.

Contributor

enterprise-rails commented Jun 26, 2013

Oh indeed. I am sorry. I fixed the code. Now tests pass locally and on travis.

https://travis-ci.org/anynines/fog/builds/8457024

Owner

geemus commented Jun 26, 2013

@enterprise-rails - no worries, thanks. I'll just wait for @rupakg to comment on the rest as he has done most of the HP stuff.

rupakg commented on a8daf7f Jun 26, 2013

@enterprise-rails Thanks for the update. The only concern I have is that I don't want to make the :hp_account_meta_key a 'required' parameter. Currently the temp_url logic was set by HP to work with the hp_secret_key, and the customers are used to that. I don't want to break existing customers by introducing a new 'required' parameter. At the same time being compatible with OS is nice, so I was thinking that:

  1. Make the name of the :hp_account_meta_key more intuitive that it says what it does.
  2. Make :hp_account_meta_key a non-required, 'recognizes' parameter, that users will know to populate when using against OS.

rupakg commented on ea6827c Jun 26, 2013

@enterprise-rails I don't know if I like adding that 'url' method. It does not describe the real purpose, and I have used that to just return the real url of the file/object. See shared_file.rb. I think we need to remove this for now.

rupakg commented on 1a151b0 Jun 26, 2013

@enterprise-rails I see you have made the change to make the :hp_account_meta_key a non-required parameter. So disregard my earlier comment. But, can you please give this parameter a more descriptive name?

rupakg commented on 99b1ae0 Jun 26, 2013

@enterprise-rails The Digest::HMAC.hexdigest method is not compatible with 1.8.7, so please use the other variant as I did in HP's case below. Also, please run your tests against 1.8.7 as well.

@enterprise-rails can you double check if the 'encoded_path' in this statement

hmac_body = "#{method}\n#{expires}\n#{encoded_path}"

should be the unencoded path i.e. sig_path defined at the top. I am guessing that this code should not differ. Please double check.

Member

rupakg commented Jun 26, 2013

@enterprise-rails I have some comments that I noted in the individual commits so they have context. Please let me know if you have any questions. /cc @geemus

Owner

geemus commented Jun 26, 2013

@rupakg - thanks!

Contributor

enterprise-rails commented Jun 26, 2013

@rupakg Thanks a lot for your feedback.

Regarding to the naming of hp_account_meta_key: The corresponding header field is officially named "Account-Meta-Temp-URL-Key" so what do you think of "account_meta_temp_url_key" instead of "hp_account_meta_key"? This will be consistent with the OpenStack documentation and also indicate that this is not a hp specific value.

Regarding to the "url" method. I absolutely agree. The only reason I have added this method is to be consistent with the AWS::File class. So it would be a real added value for the overall fog perspective to have a consistent way dealing with temporary urls among providers.
Whatever name we choose should be chosen for all providers dealing with temp urls, correct?

I will go through other comments separately.

Member

rupakg commented Jun 26, 2013

@enterprise-rails Yeah, I think as far as naming is concerned keeping it non-hp specific definitely helps. Can we name it :os_account_meta_temp_url_key and put a comment next to it saying it is a OS specific key? Your detailed comment in the temp_url method should suffice as well.

Regarding the "url" method, I think we can come up with some solution to rename and use alias to fix the AWS file#url method and probably standardize on file#temp_signed_url. That is just an suggestion. It needs to be vetted against the AWS provider folks. For now, I think I will chose to leave the HP implementation as is. @geemus any thoughts on this particular issue?

Contributor

enterprise-rails commented Jun 26, 2013

@rupakg All done. Renamed hp_account_meta_key to os_account_meta_temp_url_key and also removed HP::File#url.

Contributor

enterprise-rails commented Jun 26, 2013

@rupakg Also tested against 1.8.7.

Coverage Status

Coverage remained the same when pulling 90e50e5 on anynines:master into 3c03e4c on fog:master.

rupakg commented on 90e50e5 Jun 26, 2013

@enterprise-rails Looks good to me.

rupakg commented on 4fe2865 Jun 26, 2013

@enterprise-rails Looks good to me.

Member

rupakg commented Jun 26, 2013

@geemus I am done with the review. All looks good. Feel free to merge.

geemus merged commit 6f254b3 into fog:master Jun 27, 2013

1 check passed

default The Travis CI build passed
Details
Owner

geemus commented Jun 27, 2013

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment