Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GoogleStorageDriver can now use either our S3 authentication or other… #633

Closed
wants to merge 1 commit into from

Conversation

crunk1
Copy link
Contributor

@crunk1 crunk1 commented Nov 13, 2015

… Cloud OAuth2 authentication methods.

GoogleBaseConnection allows for a GCS_S3 auth type now, but does not handle creating the S3 HMAC header. GoogleBaseConnection still handles OAuth2 for GCE, IA, and SA auth types.

GoogleStorageConnection contains the logic for creating an S3 HMAC auth header and the logic for switching between the oauth2 auth or the S3 HMAC auth.

Changed an InvalidContainerNameError to ContainerError in S3 create_container. The exception was being raised on ANY 400 error, which can be returned for things other than an invalid name. In other words, the exception was a misnomer.

Added tests for new logic.

Did other minor cleanup.

Tests run:
Some manual tests putting, getting, and deleting objects/buckets. I used a Service Account, Installed App creds, and S3 interoperability creds.
libcloud.test.common.test_google
libcloud.test.compute.test_gce (to make sure changes to the GoogleBaseConnection didn't break it)
libcloud.test.storage.test_google_storage
libcloud.test.storage.test_s3

@Kami
Copy link
Member

Kami commented Nov 15, 2015

Thanks.

Since this is a bigger change I would please you to submit an iCLA - https://libcloud.readthedocs.org/en/latest/development.html#contributing-bigger-changes

@crunk1
Copy link
Contributor Author

crunk1 commented Nov 17, 2015

Just submitted an ICLA. I am contributing to this as a Google employee, but the ICLA instructions told me to provide my personal email (crunk1@gmail.com), not my work email (crunkleton@google.com).

@crunk1
Copy link
Contributor Author

crunk1 commented Nov 17, 2015

ICLA has been received and filed.

@crunk1
Copy link
Contributor Author

crunk1 commented Nov 17, 2015

@erjohnso PTAL

@@ -111,6 +116,15 @@ def _is_gce():
return False


def _is_gcs_s3(user_id):
"""Checks S3 key format: 20 alphanumeric chars starting with GOOG."""
return bool(re.match(r'GOOG[A-Z0-9]{16}', user_id))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: probably good enough to just use len() and user_id.startswith vs pulling in the re module

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

@erjohnso
Copy link
Member

lgtm @crunk1! Will let this sit a few days in case @Kami or others want to review too, but I'll plan to merge next week.

@Kami
Copy link
Member

Kami commented Nov 19, 2015

@crunk1 Thanks. I didn't know you work for Google, I would imagine Google already has CCLA on file.

In any case, thanks.

API_VERSION = '2006-03-01'
NAMESPACE = 'http://doc.s3.amazonaws.com/%s' % (API_VERSION)


class GoogleStorageConnection(ConnectionUserAndKey):
def _get_aws_auth_param(method, headers, params, expires, secret_key,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a good idea to move this to libcloud/common/google.py.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some unit tests for this function would also be good.

I believe we have some for AWS so maybe we can borrow or compare with the tests from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Refactored this to use the S3 HMAC Auth method. Also, there were no tests for S3's method, so I added one.

@crunk1
Copy link
Contributor Author

crunk1 commented Nov 20, 2015

@Kami, I believe the Travis failure is not my fault. Please take a look at it/retrigger a build.

@crunk1
Copy link
Contributor Author

crunk1 commented Nov 24, 2015

@Kami, ping. Sorry if that's annoying, this is blocking a Google-internal dependency of mine.

@Kami
Copy link
Member

Kami commented Nov 25, 2015

Changes look good to me, but I will wait for @erjohnso to do a final review and merge it.

@erjohnso
Copy link
Member

@crunk1 - can you squash your commits and rebase to trunk? I'm having a heck of a time applying the patch. That should also trigger a new travis run too, so we can get the assurance that the last failure was a flake.

… Google Cloud Platform OAuth2 authentication methods.

GoogleBaseConnection allows for a GCS_S3 auth type now, but does not handle creating the S3 HMAC header. GoogleBaseConnection still handles OAuth2  for GCE, IA, and SA auth types. Also did some minor cleanup to GoogleBaseConnection.

Refactored S3 HMAC auth signature creation method a bit. Refactored GoogleStorageConnection to use S3's signature method instead of reinventing it. Created a test for S3's HMAC signature method.

Changed an InvalidContainerNameError to ContainerError in S3 create_container. The exception was being raised on ANY 400 error, which can be returned for things other than an invalid name. In other words, the exception was a misnomer.

Added tests for new logic.

Did other minor cleanup.

Tests run:
Some manual tests putting, getting, and deleting objects/buckets. I used a Service Account, Installed App creds, and S3 interoperability creds.
python setup.py test
tox -e lint
@asfgit asfgit closed this in 3849f65 Nov 27, 2015
@tonybaloney
Copy link
Contributor

@crunk1 build passed, merged.
@erjohnso I know its the long weekend so enjoy!

@erjohnso
Copy link
Member

Thanks @tonybaloney!

@crunk1
Copy link
Contributor Author

crunk1 commented Nov 28, 2015

Thanks, everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants