AWS_METADATA_TIMEOUT configuration environment variable #950
Conversation
There was a problem hiding this comment.
Hi @handlerbot,
Thanks for your work here!
While it seems good, I left a few comments for code cosmetic & log cosmetic!
aws/auth_helpers.go
Outdated
| // Keep the timeout low as we don't want to wait in non-EC2 environments | ||
| client.Timeout = 100 * time.Millisecond | ||
| // Keep the default timeout low as we don't want to wait in non-EC2 environments | ||
| timeoutMillis := 100 |
There was a problem hiding this comment.
What do you think of calling this defaultTimeout and rename timeoutString as userTimeout?
There was a problem hiding this comment.
I like userTimeout, but I think I'll go with timeoutMillis -> timeout rather than defaultTimeout. Once a value from the user is assigned to the variable, it's no longer the the default, no? :-)
aws/auth_helpers.go
Outdated
| client.Timeout = 100 * time.Millisecond | ||
| // Keep the default timeout low as we don't want to wait in non-EC2 environments | ||
| timeoutMillis := 100 | ||
| timeoutString := os.Getenv("AWS_METADATA_TIMEOUT_MILLISECONDS") |
There was a problem hiding this comment.
I would set the env variable name as a constant for reusability here and in the log
website/docs/index.html.markdown
Outdated
| @@ -113,6 +113,9 @@ which reduces the chance of leakage. | |||
| You can provide the custom metadata API endpoint via the `AWS_METADATA_ENDPOINT` variable | |||
| which expects the endpoint URL, including the version, and defaults to `http://169.254.169.254:80/latest`. | |||
|
|
|||
| The default deadline for the EC2 metadata API endpoint is 100 milliseconds, but you can | |||
| override the default by setting the `AWS_METADATA_TIMEOUT_MILLISECONDS` variable. | |||
There was a problem hiding this comment.
What do you think of updating this to:
The default deadline for the EC2 metadata API endpoint is 100 milliseconds, which can be overidden by setting the `AWS_METADATA_TIMEOUT_MILLISECONDS` environment variable.
?
|
@Ninir Thanks for the quick review, and the thoughtful feedback! Everything's addressed and ready for next review. |
|
I'm 👌 with the idea overall and leave the final review on @Ninir Just one question from me here - is it worth accepting the standard duration format, e.g. btw. by convention we always leave the Changelog outside of PRs as it's responsibility of the person merging the PR. We can avoid unnecessary conflicts that way. |
radeksimko
added
enhancement
|
Hey @handlerbot Could you estimate the work to make regarding @radeksimko 's proposal? Thanks! |
|
@Ninir @radeksimko Redone to use |
handlerbot
changed the title
handlerbot
changed the title
|
Apologies for nudging, but anything else needed from me on this side? Would love to get this in before Terraform 0.10 hits, though I have no idea when that will be. :-) |
There was a problem hiding this comment.
Hey @handlerbot
This seems all good to me :)
This will probably be in the 0.10, even though there will be multiple betas until the final version :)
Thanks for the work here!
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
We have a custom Terraform authentication server which impersonates the EC2 metadata API endpoint, but our use and implementation would benefit from being able to boost the 100ms API endpoint timeout in our environment.