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

metadata: issue with DNS search #7165

Closed
howardjohn opened this issue Dec 14, 2022 · 5 comments · Fixed by #7169
Closed

metadata: issue with DNS search #7165

howardjohn opened this issue Dec 14, 2022 · 5 comments · Fixed by #7169
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@howardjohn
Copy link

Client

Go metadata

Environment

All

Go Environment

$ go version
$ go env

Code

e.g.

package main

func main() {
	fmt.Println(metadata.OnGCE())
}

Expected behavior

This returns "true" only on GCE

Actual behavior

There exists some edge cases where this may not be true. To reproduce, you can build the above program. i added some extra logging to the OnGCE call..

$ docker run -v $PWD/main:/main --dns-search com --dns 8.8.8.8 --dns-opt ndots:3 ubuntu /main
[45.33.30.197 173.255.194.134 72.14.178.174 96.126.123.244 45.79.19.196 45.33.2.79 198.58.118.167 45.33.20.235 45.33.18.44 45.33.23.183 45.56.79.23 72.14.185.43] <nil>
false

If we change addrs, err := resolver.LookupHost(ctx, "metadata.google.internal") to addrs, err := resolver.LookupHost(ctx, "metadata.google.internal.") to do an absolute lookup, this is avoided.

You'll note in my example, we resolved metadata.google.internal.com. This has some IPs, but not 169.254.169.254, so we still return false. I could have had a domain, however, that actually does return 169.254.169.254 as an IP address, along with other IPs.

This is extremely niche, but it seems like using the Fully-Qualified Domain Names with a trailing . would be preferred in all cases anyways?

@howardjohn
Copy link
Author

Actually I think a less niche case: when actually on GCE, but if systemInfoSuggestsGCE isn't true, we can get false negatives.

$ docker run -v $PWD/main2:/main --dns-search com --dns 169.254.169.254 --dns-opt ndots:3 ubuntu ./main
[45.56.79.23 45.33.18.44 198.58.118.167 72.14.185.43 96.126.123.244 45.33.2.79 45.33.23.183 45.79.19.196 72.14.178.174 45.33.20.235 173.255.194.134 45.33.30.197] <nil>
false
$ docker run -v $PWD/main2:/main --dns-search com --dns 169.254.169.254 --dns-opt ndots:2 ubuntu ./main
[169.254.169.254] <nil>
true

@codyoss codyoss added the status: investigating The issue is under investigation, which is determined to be non-trivial. label Dec 15, 2022
@codyoss codyoss self-assigned this Dec 15, 2022
@codyoss
Copy link
Member

codyoss commented Dec 15, 2022

To be clear are you saying that just switching this to absolute lookup this resolves the issue are seeing. Or are you saying we should additionally be checking for the well-known IP from the lookup as well?

@howardjohn
Copy link
Author

Or are you saying we should additionally be checking for the well-known IP from the lookup as well?

This is already done, so yes just switching to absolutely lookup

@codyoss
Copy link
Member

codyoss commented Dec 15, 2022

Haha you are right. This seems reasonable to me and likely just an oversight.

@codyoss codyoss added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. and removed status: investigating The issue is under investigation, which is determined to be non-trivial. triage me I really want to be triaged. labels Dec 15, 2022
codyoss added a commit to codyoss/google-cloud-go that referenced this issue Dec 15, 2022
@codyoss
Copy link
Member

codyoss commented Dec 15, 2022

This has been released as compute/metadata@0.2.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants