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

Add support for S3 #3

Merged
merged 9 commits into from
Oct 25, 2015
Merged

Add support for S3 #3

merged 9 commits into from
Oct 25, 2015

Conversation

dancannon
Copy link
Contributor

As discussed in #2 I have worked on adding support for S3, I have also included the features requested in the mentioned ticket, to summarise this adds the following features:

  • Detect S3 URLS in the following formats (this means that any S3 urls will no longer be fetched using the HTTP getter)
    • BUCKET.s3.amazonaws.com/PATH
    • BUCKET.s3-REGION.amazonaws.com/PATH
    • s3.amazonaws.com/BUCKET/PATH
    • s3-REGION.amazonaws.com/BUCKET/PATH
  • Also supported is the ability to override the default authentication method and the version being "getted" (this is only supported by GetFile). For example:
    • s3-REGION.amazonaws.com/BUCKET/PATH?aws_access_key_id=foo&aws_access_key_secret=bar&aws_access_token=baz
    • s3-REGION.amazonaws.com/BUCKET/PATH?version=1234
  • Using Get all files with the given prefix are fetched and stored in the destination path relative to the given prefix
  • Using GetFile fetches the given file from S3 and is stored at the destination path. As mentioned it is also possible to fetch a versioned S3 object.

If you have any further questions/feature suggestions let me know, thanks.

return "s3::" + url.String(), true, nil
}

// func (d *S3Detector) detectSSH(src string) (string, bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this was left over from a copy-paste

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing now, thanks.

@catsby
Copy link
Contributor

catsby commented Oct 23, 2015

Hey @dancannon – great start!
When I add my credentials to my environment, I get a single failure:

--- FAIL: TestS3Getter (0.00s)
    get_s3_test.go:21: err: URL is not a valid S3 URL

Is that expected, because I don't have access to that URL? It sounds like an invalid URL, not just a permissions thing

@dancannon
Copy link
Contributor Author

It sounds like something else is going on, could you please paste a sample URL that has the same format?

@catsby
Copy link
Contributor

catsby commented Oct 23, 2015

This is the test and url that's failing

The URL is https://s3-eu-west-1.amazonaws.com/hailo-s3-test

cUrl suggest the url itself is fine:

$ curl -I https://s3-eu-west-1.amazonaws.com/hailo-s3-test
HTTP/1.1 200 OK

@dancannon
Copy link
Contributor Author

Ah sorry about that!! I had added some more validation which wasnt quite correct. The tests should pass now.

)

const (
vhostFormat = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused constant, remove?

@mitchellh
Copy link
Contributor

The only thing that appears to be missing is loading credentials from environment, profile, etc. The AWS SDK does this for you but you have to make sure to just enable the credential lookup thing. If you already do this and I missed it or you need more guidance, let me know.

Your tests look great.

@dancannon
Copy link
Contributor Author

Thanks for looking at this so quickly, regarding the AWS credentials I was under the impression that if you dont pass any Credentials to the config then DefaultChainCredentials is used by default which looks up the credentials. Also I tested this code with a private S3 bucket and it seemed to pick up the credentials.

If this is incorrect let me know and I will look into it again.

@mitchellh
Copy link
Contributor

We should explicitly setup the credential chain. By using DefaultChainCredentials it allows an external caller to modify it and we don't really want to allow that. Once that fix is in, I'll merge

@dancannon dancannon force-pushed the s3 branch 2 times, most recently from 9a46df2 to fb2986d Compare October 25, 2015 10:09
@dancannon
Copy link
Contributor Author

I have made the suggested change, however now that you had added CI the tests are failing as travis does not have any AWS credentials.

Have you got any ideas for how we could fix this?

@mitchellh
Copy link
Contributor

I'll take a look locally. Thanks!

@mitchellh mitchellh merged commit fb2986d into hashicorp:master Oct 25, 2015
eastebry added a commit that referenced this pull request May 24, 2022
Co-authored-by: Kent 'picat' Gruber <kent@hashicorp.com>
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.

3 participants