update(libsinsp)!: k8s. fields return a value only when we are in a k8s pod
#1540
Conversation
Signed-off-by: Andrea Terzolo <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
poiana
added
kind/feature
|
/hold |
| case TYPE_K8S_POD_ID: | ||
| if(container_info->m_labels.count("io.kubernetes.pod.uid") > 0) | ||
| { | ||
| m_tstr = container_info->m_labels.at("io.kubernetes.pod.uid"); |
There was a problem hiding this comment.
@Andreagit97 not related to your changes and we can defer addressing them, but I noticed a few issues in current parsing.
For example here the pod id should be the truncated (first 12 char) "io.kubernetes.sandbox.id"). That way it matches how we expose the container id.
There was a problem hiding this comment.
Also the pod id can be also extracted from the ContaienrStatusResponse info field. Happy to help make this more robust in a follow up, not this PR.
"info": {
"sandboxID"
Sharing this as for example I don't see the label "io.kubernetes.sandbox.id" in my settings.
Also am working (finally) on some better unit tests for the container engine, should have it up later today.
There was a problem hiding this comment.
For example here the pod id should be the truncated (first 12 char) "io.kubernetes.sandbox.id"). That way it matches how we expose the container id.
I like the idea of having the pod.id exposed as the container.id but i have 2 questions:
- Are we sure our users don't need the entire
pod.id? Maybe they use it as it is to correlate some info coming from other sources (external to Falco) - Are we sure
12bytes are enough to guarantee the uniqueness of the pod?
BTW yes, i see this as a follow up of this PR
There was a problem hiding this comment.
#1550 SGTM!
I think we should just expose both (truncated and full ids) for both container and pod.
| ASSERT_FALSE(field_has_value(evt, "k8s.deployment.labels")); | ||
| } | ||
|
|
||
| TEST_F(sinsp_with_test_input, K8S_FILTER_check_fields_value) |
There was a problem hiding this comment.
These are all ok, thank you! Will help making them slightly more realistic via mocking API responses, will tag you then @Andreagit97 .
There was a problem hiding this comment.
ok the PR is up #1544 once we adjust pod id we can update those tests.
| auto new_fl = flist.new_filter_check_from_fldname(field_name, &m_inspector, false); | ||
| if(new_fl != nullptr) | ||
| { | ||
| // if we can create a filter check it means that the field exists |
|
|
||
| const auto container_info = m_inspector->m_container_manager.get_container(tinfo->m_container_id); | ||
| // No labels means no k8s metadata. | ||
| if(container_info == nullptr || container_info->m_labels.empty()) |
There was a problem hiding this comment.
ok for now we can improve this later.
|
LGTM label has been added. Git tree hash: bffe592b5e4dd4d3ac89b88cc83cde41e5ff3a71
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, incertum, leogr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area libsinsp
/area tests
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
This PR should address the changes required here #1489 (review) @leogr @incertum. Let me know if we want to proceed in this way. If we agree on these changes, I will close #1489
Which issue(s) this PR fixes:
Special notes for your reviewer:
This should be a breaking change since we are changing the behavior of
k8s.pod.ipandk8s.pod.cni.jsonfields. Now they return something different from<NA>only if we are in a k8s pod, like all the othersk8s.fieldsDoes this PR introduce a user-facing change?: