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

Improve k8s. fields retrieved from container runtime socket robustness + expose full container id and pod ids #1550

Closed
incertum opened this issue Dec 5, 2023 · 7 comments
Assignees
Labels
kind/feature New feature or request
Milestone

Comments

@incertum
Copy link
Contributor

incertum commented Dec 5, 2023

Follow up to #1540 (comment)

Enhancements:

  • New field container.full_id (or similar naming)
  • New field k8s.pod.full_id (or similar naming)
  • Fix k8s.pod.id (reflects actual sandbox id truncated to 12 chars not the wrong pod uid which is something entirely different)
  • Better resilience to retrieve k8s.ns.name and k8s.pod.* as right now we rely solely on labels from the container status response, but we also have the pod status response readily available since we make that request already anyways.
  • Internally augment the container info struct to support {m_pod_id, m_pod_name, m_ns_name} natively as items
  • Add tests based on new unit tests introduced here new(libsinsp/test): Start dedicated container engine unit testsuite w/ mock CRI API response #1544

CC @Andreagit97 @leogr @gnosek happy to take care of these minor enhancements if you all agree

@incertum incertum added the kind/feature New feature or request label Dec 5, 2023
@incertum
Copy link
Contributor Author

incertum commented Dec 5, 2023

/milestone 0.15.0

/assign

@incertum
Copy link
Contributor Author

incertum commented Dec 5, 2023

In addition, another question @leogr: We continue having confusing around "missing" container fields for host processes. Adopters appear to commonly overlook the host string as container.id. Perhaps it makes sense to return host string for a few more fields, at least the container image and tag related ones?

@leogr
Copy link
Member

leogr commented Dec 11, 2023

In addition, another question @leogr: We continue having confusing around "missing" container fields for host processes. Adopters appear to commonly overlook the host string as container.id. Perhaps it makes sense to return host string for a few more fields, at least the container image and tag-related ones?

Although I agree with the intent, I'm not sure returning host is the correct solution. Imagine the current image name and tag example/example:host. How could one distinguish between host (the actual tag) and host (the placeholder that signals the context)? 🤔

Likely, we need a different way to clearly indicate whether we are in a container context or not. Perhaps another field (eg. container.is_a_container 🤔 )?

@incertum
Copy link
Contributor Author

Hmmm maybe just replace container.image and container.image.repository with host if applicable, I think we already do it for container.name. Those are also really the only fields that trip adopters off as far as I am aware.

Furthermore, once we have all PRs merged that are tagged with libs 0.14.0 milestone will go ahead and open this cleanup PR from the libs repo (not my fork).
@Andreagit97 and @alacuku perhaps it may make sense to collaborate in that PR and implement all additional changes such as reorganizing the code a bit in there.

Plus we should also expose k8s.pod.uid and maybe also container.label and container.labels (the labels cleanup would be on @Andreagit97 plate -- I can do the other cleanup prior).

@leogr
Copy link
Member

leogr commented Dec 12, 2023

Hmmm maybe just replace container.image and container.image.repository with host if applicable, I think we already do it for container.name. Those are also really the only fields that trip adopters off as far as I am aware.

And what if a container image is just named host ? 👼
I believe container.id just works because host is not a valid id. Even with container.name (I don't recall if we use host in this case), it may be misleading (it could be actually called host). It may potentially allow a malicious actor to hide the container by relying on the assumption that the host value is assumed to be a non-container (I know, this is likely not a real scenario, but it is still worth thinking about).

Anyway, I agree we have to improve this, still I'm thinking about an alternative solution to solve the problem, but I haven't found one yet.

Furthermore, once we have all PRs merged that are tagged with libs 0.14.0 milestone will go ahead and open this cleanup PR from the libs repo (not my fork). @Andreagit97 and @alacuku perhaps it may make sense to collaborate in that PR and implement all additional changes such as reorganizing the code a bit in there.

👍

Plus we should also expose k8s.pod.uid and maybe also container.label and container.labels (the labels cleanup would be on @Andreagit97 plate -- I can do the other cleanup prior).

Can you elaborate more on this please?

@incertum
Copy link
Contributor Author

@leogr we can defer the possible host naming fallback.

@leogr and @Andreagit97 PR is up. Since we likely won't release 0.15.0 for Falco 0.37.0 I reduced this PR to the essential changes, deferring any major container engine refactor for now.

@Andreagit97 re labels refactor, up to you to decide when to implement it.

@incertum
Copy link
Contributor Author

/milestone 0.14.0

A new issue will be used to track outstanding improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants