thanos-io / thanos

Highly available Prometheus setup with long term storage capabilities. A CNCF Incubating project.
https://thanos.io
Apache License 2.0
13.12k stars 2.1k forks source link

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

Closed dadux closed 6 years ago

dadux commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

TimSimmons commented 6 years ago

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
abennett commented 6 years ago

Could I take this on?

bwplotka commented 6 years ago

Sure, but what's your plan? (:

Also have in mind this will come soon (common tests for all providers) https://github.com/improbable-eng/thanos/pull/327, but it should change much.

abennett commented 6 years ago

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 commented 6 years ago

SGTM @rackonnoiter

jescarri commented 6 years ago

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 commented 6 years ago

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

bwplotka commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

abennett commented 6 years ago

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!