-
Notifications
You must be signed in to change notification settings - Fork 19
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
fix: fetch GCP metadata from 169.254.169.254 directly #293
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.
LGTM. We already used 169.254.169.254 for AWS and Azure:
chalk/src/plugins/cloudMetadata.nim
Line 14 in ca217bd
awsBaseUri = "http://169.254.169.254/latest/" |
chalk/src/plugins/cloudMetadata.nim
Line 237 in ca217bd
let resultOpt = hitProviderEndpoint("http://169.254.169.254/metadata/instance?api-version=2021-02-01", newHttpHeaders([("Metadata", "true")])) |
We should probably define 169.254.169.254
as a const
in this file and use that everywhere, but I don't mind leaving that till later.
Agreed - not sure though if there are lists of IPs that differ slightly per provider? this plugin could be split / refactored in the future so left it as such for now as I treated it as a nit as well |
For this particular case, I think we'd be pretty safe even if we added more cloud providers. My understanding is that |
Alibaba Cloud uses a different address: https://www.alibabacloud.com/help/en/ecs/user-guide/view-instance-metadata (not arguing against or for, just providing an example). |
Cool, thanks. Yeah, I've got no context for Alibaba Cloud. Looks like it's different for Tencent Cloud too. |
0d961cc
to
919f14a
Compare
I suggest merging with the edited shorter PR title - commit title would be longer than 72 chars otherwise, which overflows e.g. GitHub's commit view. |
CHANGELOG.md
if necessaryIssue
Fixes #292