-
Notifications
You must be signed in to change notification settings - Fork 476
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
Use AWS SDK to retrieve ec2 instance identity doc #1369
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah, I don't mind this....
IdentityDocumentURL string `hcl:"identity_document_url"` | ||
IdentitySignatureURL string `hcl:"identity_signature_url"` | ||
EC2MetadataEndpoint string `hcl:"ec2_metadata_endpoint"` | ||
IdentityDocumentURL string `hcl:"identity_document_url"` // Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: prefix vars with Deprecated
(but keep hcl tag as-is) as a visual indicator that new code shouldn't rely on them
config.IdentityDocumentURL = defaultIdentityDocumentURL | ||
} | ||
// If we have a legacy config, ensure both have a value | ||
if config.EC2MetadataEndpoint == "" && (config.IdentitySignatureURL != "" || config.IdentityDocumentURL != "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should log here if deprecated configurables are set and the new endpoint configurable is:
- set (e.g. "deprecated configurable foo is ignored because...")
- not set (.e.g. "deprecated configurable foo will be deprecated in a future release. please use ec2_metadata_endpoint instead")
Addressed feedback (warning logs, use Deprecated prefix on variable names) I rebased on master and force-pushed the updated commit since there was some merge conflicts with my other cleanup PR in #1368 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I had a few nitpicks that are also causing lint failures. I'll be ready to approve and merge once those are fixed.
"fmt" | ||
"github.com/hashicorp/go-hclog" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this import belongs down with the other group...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. Blame my IDE upgrading losing my save-actions auto-goimports :)
"io/ioutil" | ||
"net/http" | ||
"sync" | ||
|
||
awsSdk "github.com/aws/aws-sdk-go/aws" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we aren't consistent in the codebase but named imports should follow package naming conventions (i.e. awssdk
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to awssdk
This keeps the old code, and if either the doc or sig URL is set, it uses the old codepath. After a deprecation period, we can delete it. Signed-off-by: Matthew McPherrin <mmc@squareup.com>
This is an alternative design to #1360
Rather than trying to always use the SDK, the fallback path here uses the old code.
This is thus more compatible, at the expense of being a bit branchier.
Which issue this PR fixes
fixes #1359