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

Refactor AWS object store auth to be as simple and flexible as Google FindCredentials. #209

Closed
dadux opened this issue Feb 14, 2018 · 14 comments

Comments

@dadux
Copy link

dadux commented Feb 14, 2018

While running in AWS, you can use the instance profile to get an auth token, but currently Thanos doesn't configure the bucket if no credentials are provided.

minio supports IAM :

iam := credentials.NewIAM("")
s3Client, err := minio.NewWithCredentials("s3.amazonaws.com", iam, true, "")

Would you consider a PR ?

@bwplotka
Copy link
Member

bwplotka commented Feb 16, 2018

Hey, first of all, we would like to aim for having a minimum amount of code required to setup each provider's clients. I know it's not easy (:

For example for GCP (GCS + optional tracing) we just create storage.NewClient(context.Background()) ("cloud.google.com/go/storage") that does this by default:

// FindDefaultCredentials searches for "Application Default Credentials".
//
// It looks for credentials in the following places,
// preferring the first location found:
//
//   1. A JSON file whose path is specified by the
//      GOOGLE_APPLICATION_CREDENTIALS environment variable.
//   2. A JSON file in a location known to the gcloud command-line tool.
//      On Windows, this is %APPDATA%/gcloud/application_default_credentials.json.
//      On other systems, $HOME/.config/gcloud/application_default_credentials.json.
//   3. On Google App Engine it uses the appengine.AccessToken function.
//   4. On Google Compute Engine and Google App Engine Managed VMs, it fetches
//      credentials from the metadata server.
//      (In this final case any provided scopes are ignored.)
func FindDefaultCredentials(ctx context.Context, scope ...string) (*DefaultCredentials, error) {
    // First, try the environment variable.
    const envVar = "GOOGLE_APPLICATION_CREDENTIALS"
    if filename := os.Getenv(envVar); filename != "" {
        creds, err := readCredentialsFile(ctx, filename, scope)
        if err != nil {
            return nil, fmt.Errorf("google: error getting credentials using %v environment variable: %v", envVar, err)
        }
        return creds, nil
    }

    // Second, try a well-known file.
    filename := wellKnownFile()
    if creds, err := readCredentialsFile(ctx, filename, scope); err == nil {
        return creds, nil
    } else if !os.IsNotExist(err) {
        return nil, fmt.Errorf("google: error getting credentials using well-known file (%v): %v", filename, err)
    }

    // Third, if we're on Google App Engine use those credentials.
    if appengineTokenFunc != nil && !appengineFlex {
        return &DefaultCredentials{
            ProjectID:   appengineAppIDFunc(ctx),
            TokenSource: AppEngineTokenSource(ctx, scope...),
        }, nil
    }

    // Fourth, if we're on Google Compute Engine use the metadata server.
    if metadata.OnGCE() {
        id, _ := metadata.ProjectID()
        return &DefaultCredentials{
            ProjectID:   id,
            TokenSource: ComputeTokenSource(""),
        }, nil
    }

    // None are found; return helpful error.
    const url = "https://developers.google.com/accounts/docs/application-default-credentials"
    return nil, fmt.Errorf("google: could not find default credentials. See %v for more information.", url)
}

As you can see it supports different things, including specifying envvar, as well as just fetching from GCE metadata server if you are sitting on GCE VM.

If we can construct similar function for AWS (or import anything existing and small), that would be really, really great!

Additionally, it is worth to note that it would nice to support cases when user cannot modify envvars for some reasons (and is not on AWS/GCE VM). Currently our GCP client does not help with that - we need some small changeset in this area as well.

@bwplotka
Copy link
Member

Also it is worth maybe to ping @TimSimmons for some feedback. Tim wrote the S3 client. (:

@TimSimmons
Copy link
Contributor

As long as it's not at the exclusion of the other method, specifying secret/access keys, bucket etc is how you can get access to a lot of other environments that offer S3 capability, but not the explicit IAM functionality. It seems like you could try grabbing that info in the same function where it checks to see if the S3 env vars/flags are set. That seems like a smart solution to me.

if IAM provided:
   use that to configure s3 client
else if s3 creds provided
   use that to configure s3 client
else
   don't configure an s3 client

@bwplotka bwplotka changed the title Use AWS IAM for S3 buckets Refactor AWS object store auth to be as simple and flexible as Google FindCredentials. (remove all boilerplate flags) Mar 14, 2018
@bwplotka bwplotka changed the title Refactor AWS object store auth to be as simple and flexible as Google FindCredentials. (remove all boilerplate flags) Refactor AWS object store auth to be as simple and flexible as Google FindCredentials. Mar 14, 2018
@abennett
Copy link

Could I take this on?

@bwplotka
Copy link
Member

bwplotka commented May 25, 2018

Sure, but what's your plan? (:

Also have in mind this will come soon (common tests for all providers) #327, but it should change much.

@abennett
Copy link

abennett commented May 25, 2018

I essentially just want to somewhat emulate the actual aws-go-sdk with regards to how it tries to resolve the credentials by default by first checking arguments/environmental variables and then using the instance profile/role/metadata/iam route (the official sdk also looksfor the shared secret config, but that doesn't seem within the scope of this issue). The necessary API already exists in minio-go.

The s3 logic in Thanos might need to change a little to account for using roles, which requires an additional and different step compared to using the typical access key/secret key combination.

@bwplotka
Copy link
Member

SGTM @rackonnoiter

@jescarri
Copy link

Ideally it should detect if there's a metadata service answering and get the credentials from there.

If thanos is running on kubernetes that has kube2iam or kiam or in AWS.

Another thing to consider is that those credentials have a reduced lifespan I think 12 hours, so the logic should take that into consideration.

@rackonnoiter does the minio-go has something similar to a CredentialsChainProvider ?

@abennett
Copy link

That's exactly what I was suggesting, @jescarri, and yes.

@bwplotka
Copy link
Member

Is there any library for this we can reuse, without reimplementing all? In the same avoiding pulling whole AWS SDK as dependency would be nice as well (:

@abennett
Copy link

All the necessary pieces should exist in minio-go, so there shouldn't be a need to import anything else. We just have to tweak a few things here and there to account for the usage of roles instead of users. I'm still working through the codebase to see what those changes are exactly.

@abennett
Copy link

Submitted a WIP PR. I didn't seem to find any contributing guidance, could someone point me to the right way to go about testing my changes?

For an overview, these changes can use multiple auth providers and searches in this order:

  1. Manually entered flags
  2. Environmental variables
  3. IAM role through the metadata service

@bwplotka
Copy link
Member

Thanks will take a look. Any particular info you are looking for? What would you expect from contributing guidance?

@abennett
Copy link

I mainly just want to know what checks I need to complete prior to submitting a PR. There also seems to be some dependencies in setting my environment for the tests to work, so explicitly listing them would be appreciated as well.

Thank you, @Bplotka!

wjam added a commit to wjam/thanos that referenced this issue Jul 29, 2018
…mmand line

Support retrieving credentials from IAM instance profile, `~/.aws/credentials` and standard AWS environment variables

Fixes thanos-io#209
wjam added a commit to wjam/thanos that referenced this issue Jul 29, 2018
…mmand line

Support retrieving credentials from IAM instance profile, `~/.aws/credentials` and standard AWS environment variables

Fixes thanos-io#209
bwplotka pushed a commit that referenced this issue Aug 3, 2018
…mmand line (#450)

* Add support for retrieving AWS credentials from sources other than command line

Support retrieving credentials from IAM instance profile, `~/.aws/credentials` and standard AWS environment variables

Fixes #209

* Changes from code review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants