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

Fetch AWS config from ec2metadata if on an ec2 instance #5

Merged
merged 8 commits into from
Aug 30, 2018

Conversation

jepperson2
Copy link
Contributor

Not the prettiest solution, but there doesn't appear to be native support for fetching the region from the ec2 instance it's running on.

Relevant feature request in aws-go-sdk: aws/aws-sdk-go#1103

@jepperson2
Copy link
Contributor Author

@doodlesbykumbi Any chance you'd have some time to look over this PR?

@doodlesbykumbi
Copy link
Contributor

@jepperson2 Any idea what happens if there is no ec2metadata ?

IMO, this provider should support as many configurations as possible. At the moment the envvar AWS_REGION works but it leaves the determination of that value to the user, which is annoying when you can do all that stuff in Go. It'd be good if we could write something that extracts configuration based on intuitive or well-defined precedence e.g. I think envvars take precedence over all.

@jepperson2
Copy link
Contributor Author

@doodlesbykumbi If there is no ec2metadata it silently fails and keeps going. No effect if not running on an ec2 instance

With the latest commit, this now supports the expected precedence relationship: envvars -> shared config -> ec2metadata region.

@jepperson2
Copy link
Contributor Author

@doodlesbykumbi Any chance you could review this again?

Would love to see this merged and reflected in a release I could use.

@jepperson2
Copy link
Contributor Author

Is there anything else this PR needs before it can be merged?

main.go Outdated
// If the region could not be found in an environment variable or a shared config file,
// create metaSession to fetch the ec2 instance region and pass to the regular Session.
if *sess.Config.Region == "" {
metaSession, _ := session.NewSession()
Copy link
Contributor

Choose a reason for hiding this comment

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

Throwing away the error returned here and below (by Region) seems like it could make problems harder to diagnose. Is there any reason not to call printAndExit if error is non-nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Failing silently here causes a more helpful error to be thrown later.
If I printAndExit here or after fetching the region, it throws: RequestError: send request failed caused by: Get http://169.254.169.254/latest/meta-data/placement/availability-zone: dial tcp 169.254.169.254:80: connect: host is down as opposed to MissingRegion: could not find region configuration when I don't catch this error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct that it would be harder to diagnose a problem in the case that I am on an ec2 instance but this lookup fails for some reason.

If I'm not on an ec2 instance, however, and my config isn't set anywhere, failing silently here provides the MissingRegion:... error.

Copy link
Contributor

Choose a reason for hiding this comment

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

As an alternative, looks like you can use EC2Metadata.Available to determine whether you're on an EC2 instance. So, you could do this, instead:

	if *sess.Config.Region == "" {
		metaSession, err := session.NewSession()
		if err != nil {
			printAndExit(err)
		}
		metaClient := ec2metadata.New(metaSession)
		if metaClient.Available() {
			if region, err := metaClient.Region(); err == nil {
				sess.Config.Region = aws.String(region)
			} else {
				printAndExit(err)
			}
		}
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great suggestion! Thanks, @apotterri! My latest commit reflects these changes

@apotterri apotterri merged commit 1b56b92 into cyberark:master Aug 30, 2018
@apotterri
Copy link
Contributor

Looks great! Thanks very much for doing this.

@jepperson2 jepperson2 deleted the non_env_creds branch August 30, 2018 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants