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

google: Prevent GCE auth to hide S3 auth #921

Closed
wants to merge 1 commit into
base: trunk
from

Conversation

Projects
None yet
3 participants
@pquentin
Contributor

pquentin commented Oct 24, 2016

Prevent GCE auth to hide S3 auth

Description

We currently authenticate to Google Cloud Storage using Amazon S3 compatibility auth. Our code runs in Kubernetes on Google Container Engine. We tried to upgrade libcloud recently but 3849f65 from @crunk1 prevented us to authenticate. (Interestingly, it's also the commit that made us want to upgrade, since we eventually want to use service accounts.)

The issue happened for two reasons:

  • GoogleAuthType._is_gce() always returns True when the code is run on the Google Container Engine, regardless of the authentication provided (which makes the issue impossible to reproduce in a local Docker environment)
  • GoogleAuthType._is_gcs_s3() is always checked after _is_gce(), so it could not be used on Google Container Engine

This pull request simply changes the order to give S3 higher priority. Note that Installed Applications auth has lower priority still, because it's the default auth when everything else fails. That's OK because I guess it's not possible on GCE. Still, I think the documentation should recommend to always specify the auth type, because explicit is better than implicit and it helps to avoid unclear errors.

done, ready for review

Checklist (tick everything that applies)

google: Prevent GCE auth to hide S3 auth
GoogleAuthType._is_gce() is going to return True on any GCE instance,
but if there are S3 credentials, they should be used.
@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Oct 24, 2016

Contributor

seeking input from @crunk1

Contributor

tonybaloney commented Oct 24, 2016

seeking input from @crunk1

@crunk1

crunk1 approved these changes Oct 24, 2016

@crunk1

This comment has been minimized.

Show comment
Hide comment
@crunk1

crunk1 Oct 24, 2016

Contributor

LGTM. Sorry for the hassle!

Contributor

crunk1 commented Oct 24, 2016

LGTM. Sorry for the hassle!

@pquentin

This comment has been minimized.

Show comment
Hide comment
@pquentin

pquentin Oct 25, 2016

Contributor

@crunk1 No worries! Thanks for your work on libcloud.

Contributor

pquentin commented Oct 25, 2016

@crunk1 No worries! Thanks for your work on libcloud.

@pquentin

This comment has been minimized.

Show comment
Hide comment
@pquentin

pquentin Oct 30, 2016

Contributor

@tonybaloney Anything else I should do? Thanks.

Contributor

pquentin commented Oct 30, 2016

@tonybaloney Anything else I should do? Thanks.

@pquentin

This comment has been minimized.

Show comment
Hide comment
@pquentin

pquentin Nov 9, 2016

Contributor

@asfgit Anything I can do to help get this merged? Thanks.

Contributor

pquentin commented Nov 9, 2016

@asfgit Anything I can do to help get this merged? Thanks.

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Nov 9, 2016

Contributor

hey @pquentin back from travelling. let's get this merged.

Contributor

tonybaloney commented Nov 9, 2016

hey @pquentin back from travelling. let's get this merged.

@asfgit asfgit closed this in 8bedf24 Nov 9, 2016

asfgit pushed a commit that referenced this pull request Nov 9, 2016

asfgit pushed a commit that referenced this pull request Nov 10, 2016

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