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

ContainerResource extract container.id in Host env #7437

Closed
wzy531a opened this issue Dec 16, 2022 · 3 comments · Fixed by #8206
Closed

ContainerResource extract container.id in Host env #7437

wzy531a opened this issue Dec 16, 2022 · 3 comments · Fixed by #8206
Assignees
Labels
bug Something isn't working

Comments

@wzy531a
Copy link

wzy531a commented Dec 16, 2022

Describe the bug
When I use javaagent to collect trace in the host environment, I found that there is containerID in the resource.attributes, and the problem seems to be the isValidBase16String judgment method of ContainerResource

Steps to reproduce
The /proc/self/cgroup content line is 1:name=systemd:/user.slice/user-0.slice/session-31207.scope

What did you see instead?
It well extract 31207 as container.id, but the environment is host instead of container.

What version are you using?
otel-javaagent 1.20.2

Environment
centos 7.5

Additional context

@wzy531a wzy531a added the bug Something isn't working label Dec 16, 2022
@trask
Copy link
Member

trask commented Apr 2, 2023

cc @breedx-splk in case you have any ideas

@breedx-splk
Copy link
Contributor

Relates to #7176. Maybe relates to golang #3508.

@breedx-splk breedx-splk self-assigned this Apr 3, 2023
@breedx-splk
Copy link
Contributor

I agree this is a bug. Seems like the ContainerResourceProvider and its collaborator ContainerResource shouldn't be returning IDs unless the process is actually containerized. I'll have a look at guarding for this.

trask pushed a commit that referenced this issue Apr 4, 2023
Resolves #7437.

A few caveats about this. The TL;DR on #7437 is that a non-containerized
process was reporting a `container.id` attribute. The submitter narrowed
it down and I was able to confirm with the test case in this PR.

I hunted for other means for code to determine if it's containerized
with the idea to not even do the parsing if not containerized, but I
couldn't find anything useful. In fact, most approaches of detecting
containerization at all do involve parsing cgroups. Wacky.

So I attempted to verify that container IDs should always be 64
characters. I found:
* podman - docs
[here](https://docs.podman.io/en/latest/markdown/podman-container-inspect.1.html)
"Container ID (full 64-char hash)"
* docker - UID generator source
[here](https://github.com/moby/moby/blob/634a848b8e3bdd8aed834559f3b2e0dfc7f5ae3a/pkg/stringid/stringid.go#L36)
shows 32 bytes (and even guards against fully numeric!)
* lxc [man page
](https://linuxcontainers.org/lxc/manpages/man1/lxc-info.1.html)says
"container identifier format is an alphanumeric string". If this maps
into cgroups (no idea!), it would have already been broken in some cases
because we enforce hex.

I'm a little concerned about this approach because the [otel
spec](https://github.com/open-telemetry/opentelemetry-specification/blob/94c9c75c4f82fbda08bddff02ce9a0c582fdd55c/specification/resource/semantic_conventions/container.md)
suggests that "The UUID might be abbreviated.", but it's
unclear/non-specific about the circumstances that might cause this.

Open to hearing about why the approach presented here is a bad idea. 🙃
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants