uprobe digest partial load - #5301
Conversation
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
1eacf38 to
b035b31
Compare
FedeDP
left a comment
There was a problem hiding this comment.
So, i am bit confused: which is the final state for an uprobe sensor with all its hook in HookState_STATUS_DIGEST_REJECTED state?
Would it be TP_STATE_ENABLED?
My proposal is to add a new state PARTIALLY_ENABLED and:
- all hooks loaded -> TP_STATE_ENABLED
- some hooks loaded -> TP_STATE_PARTIALLY_ENABLED
- no hooks loaded -> TP_STATE_LOAD_ERROR
| for _, sens := range col.sensors { | ||
| pol.Sensors = append(pol.Sensors, sens.GetName()) | ||
| pol.KernelMemoryBytes += uint64(sens.TotalMemlock()) | ||
| sens.SetStatus(&pol) |
There was a problem hiding this comment.
I am wondering whether exposing a Status() API would fit better with other already exposed methods; then let handler.go do the append.
Eg, if you look at other 2 fields in this for loop, we are doing that.
There was a problem hiding this comment.
Thanks, yes, that's better. This approach also helped with you other suggestion, TP_STATE_PARTIALLY_ENABLED.
62d292e to
4cea3ec
Compare
Yes, TP_STATE_ENABLED is the final state in this scenario.
I added TP_STATE_PARTIALLY_ENABLED for "some hooks loaded". But, I did not do "no hooks loaded -> TP_STATE_LOAD_ERROR" Since I am headed towards doing an ignore approach, like what is done here, this situation does not feel like an error to me, because the policy will be rejected unless the user specified that it's OK to ignore all the hooks. Please let me know if you still think that we should set that state of the policy to TP_STATE_LOAD_ERROR when no hook are attached. |
2a6d067 to
78a3227
Compare
In this scenario, i agree with you since it's the user telling us what to do :) |
|
The only thing i am not sure of is:
Do we want to unify the behavior? (Just a discussion, it can be done in subsequent PR of course since it is tangent to this PR) |
olsajiri
left a comment
There was a problem hiding this comment.
I can't really say the HookState is the best way to carry this info, but the ignore part makes sense to me, left one comment, thanks
| } | ||
| } | ||
|
|
||
| hook_status := &tetragon.HookStatus{ |
There was a problem hiding this comment.
we don't use _ in variable name
There was a problem hiding this comment.
Thanks for pointing this out. I've fixed it.
FedeDP
left a comment
There was a problem hiding this comment.
Patch LGTM aside the concern expressed in #5301 (comment) that IMO is not meant to be fixed in this PR.
Thank you very much!
78a3227 to
1f087f6
Compare
How can I improve it? Are you saying that you don't like the structure of the schema that was added to the API, or that we should use a separate, internal-only data structure during sensor construction, and then translate that to HookState only for the API? |
1f087f6 to
af2d396
Compare
|
Here is what kprobes look like after unifying: The policy: |
|
So, i now want a 🍕 😆
I think it looks great, actually; this is what i had in mind. Thanks for doing that.
I agree with you, better not invest too much effort on this until we know there is a real use case for it. |
| hookStatus.State = tetragon.HookState_STATUS_DIGEST_REJECTED | ||
| logger.GetLogger().Info( | ||
| fmt.Sprintf("spec.uprobes[%d]: %s", cfgIdx, mismatchErr.Error()), | ||
| ) |
There was a problem hiding this comment.
Should this be a Debug? Not sure if it's an error condition or not.
There was a problem hiding this comment.
I'd like to keep this as Info. When I spoke with users about what they'd like to see when a partial load occurs, they said they want to see that it happened in tetragon's logs. IMHO, I think having this in the log would help us debug as well.
Logging at info level is consistent with how ignored kprobes are handled:
tetragon/pkg/sensors/tracing/generickprobe.go
Line 420 in 8d10f87
| } else { | ||
| progs, maps, err = createSingleUprobeSensor(polInfo, ids, has) | ||
| } | ||
| if len(ids) != 0 { |
There was a problem hiding this comment.
Just curious, is this check in case all hooks were skipped?
There was a problem hiding this comment.
Yes, this for when all hooks are skipped. I'm trying to avoid loading programs/maps in this case. Without this condition, this happens:
level=warn msg="Server AddTracingPolicy request failed" error="sensor generic_uprobe from collection uprobe-digest-empty failed to load: tetragon, aborting could not load sensor BPF maps: failed to load map 'config_map' for sensor 'generic_uprobe': failed to create map 'config_map': creating map: map create: invalid argument (MaxEntries may be incorrectly set to zero)" metadata.namespace="" metadata.name=uprobe-digest-empty
So instead of make that code cope, I opted to prevent it from running, because loading those program/maps is of no use.
This ties into a discussion about whether a policy should be considered TP_STATE_LOAD_ERROR when all of the hooks are ignored. Folks that advocated for TP_STATE_LOAD_ERROR previously, seem to be OK with TP_STATE_PARTIALLY_ENABLED now, because the user is signaling intent with their ignore configurations.
I wanted to create the empty sensor because that's the way that I communicate which hooks were attached (none in this case). But the consequence of this is that I needed to add and check additional state for the sensor in order to prevent spurious error logs. See the last commit of this series for what that entails. It's also worth noting that I changed kprobes to do this too, for the same reason.
ab51981 to
9e02b82
Compare
mtardy
left a comment
There was a problem hiding this comment.
Docs lgtm, just a few questionns on the STATUS_PARTIAL_CALL_NOT_FOUND = 4; that I might misunderstand.
| } | ||
| } | ||
|
|
||
| func (c *collection) getEnabledState() TracingPolicyState { |
There was a problem hiding this comment.
maybe I would put a name a bit more explicit that states it could return partially enabled in some situations.
There was a problem hiding this comment.
I've change this to getEnabledStateType(). If you can think of a better name, please suggest it, and I'll change it.
| func PrintTracingPolicies(output io.Writer, policies []*tetragon.TracingPolicyStatus, hookStatus bool, skipPolicy func(pol *tetragon.TracingPolicyStatus) bool) { | ||
| // tabwriter config imitates kubectl default output, i.e. 3 spaces padding | ||
| w := tabwriter.NewWriter(output, 0, 0, 3, ' ', 0) | ||
| header := "ID\tNAME\tDOMAIN\tSTATE\tFILTERID\tNAMESPACE\tSENSORS\tKERNELMEMORY\tMODE\tNPOST\tNENFORCE\tNMONITOR" | ||
| if hookStatus { | ||
| header = "ID\tNAME\tDOMAIN\tSECTION\tCFGIDX\tDESCRIPTION\tSTATUS" | ||
| } |
There was a problem hiding this comment.
Could we do better and instead of having the manual --hook-status print the hook status when you hit a partially enabled TP?
Like for a partially enabled TP you maybe inline this new tab print at indentation +1? I wonder if it's too complicated or just doable with tabwriter.
andy@builder:~/src/tetragon$ sudo ./tetra tracingpolicy list
ID NAME DOMAIN STATE FILTERID NAMESPACE SENSORS KERNELMEMORY MODE NPOST NENFORCE NMONITOR
2 uprobe-digest-example grpc partially_enabled 0 (global) generic_uprobe 380.79 kB monitor_only 0 0 0
SECTION CFGIDX DESCRIPTION STATUS
uprobes 0 /home/andy/src/sleep digest_rejected
uprobes 1 /home/andy/src/sleep loaded
uprobes 2 /home/andy/src/my_program loade
3 uprobe-othero-example grpc enabled 1 (global) generic_uprobe 380.79 kB monitor_only 0 0 0
There was a problem hiding this comment.
Ok, I was able to do this by using multiple tabwriters, and manually interleaving the hook status:
andy@builder:~/src/tetragon$ sudo ./tetra tracingpolicy list
ID NAME DOMAIN STATE FILTERID NAMESPACE SENSORS KERNELMEMORY MODE NPOST NENFORCE NMONITOR
2 uprobe-digest-example grpc partially_enabled 0 (global) generic_uprobe 380.79 kB monitor_only 0 0 0
SECTION CFGIDX DESCRIPTION STATUS
uprobes 0 /home/andy/src/sleep digest_rejected
uprobes 1 /home/andy/src/sleep loaded
uprobes 2 /home/andy/src/my_program loaded
3 pizza-and-bpf grpc partially_enabled 0 (global) generic_kprobe 0 B unknown 0 0 0
SECTION CFGIDX DESCRIPTION STATUS
kprobes 0 pizza_is_the_best call_not_found
4 uprobe-selectors grpc enabled 0 (global) generic_uprobe 380.79 kB monitor_only 0 0 0
While I don't think this is a problem, note how the pizza-and-bpf policy has different hook status column widths compared to the uprobe-digest-example hook status.
| // probe was partially loaded due to call not found | ||
| STATUS_PARTIAL_CALL_NOT_FOUND = 4; |
There was a problem hiding this comment.
I don't think this enum entry belongs here but on the upper TP status?
There was a problem hiding this comment.
This is intentional. When a kprobe's hook's call config specifies a list, the hook itself can be partially loaded. This is related to your other comment as well: #5301 (comment)
| if valInfo[i].ignoredCount == 0 { | ||
| hookStatus.State = tetragon.HookState_STATUS_LOADED | ||
| } else { | ||
| hookStatus.State = tetragon.HookState_STATUS_PARTIAL_CALL_NOT_FOUND | ||
| } |
There was a problem hiding this comment.
I'm not following why it's not applied on the TP status not hookStatus
There was a problem hiding this comment.
There is some disussion/explanation about this in this comment.
When a kprobe hook specifies a list for call:, then multiple kprobes are generated, one for each function in the list. Some of those functions may not be found while others are. When this happens, we can have a partial load at the hook level too.
b1d12be to
baa5b82
Compare
In a subsequent commit, when we encounter uprobe binary digest verification failures, we will partially load the policy instead of rejecting the policy in its entirety. We will salvage the uprobes where verification did not fail. With that change, we need a way to communicate which hooks we attached to and which hooks were not attached due to digest mismatch. This interface is meant to expose that. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
This state will reflect that fact that not all hooks were loaded. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
The previous commits altered sensors.proto. This commit is the result of running "make protogen" to generate code based on that interface change. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
This lays the groundwork for sensors to report their hook status. Each sensor type will need an implementation to set their status. Uprobes will be the first sensor to do this in a subsequent commit. The remaining sensors will report status on an as-needed basis. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
This signifies that the policy loaded, but not all hooks were attached. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
Now that digest verification does not cause the policy to be rejected in its entirety, update our docs to reflect this. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
This is similar to Ignore for kprobes, where users can specify a reason to ignore a specific hook given a specific condition. When a hook is ignored, this results in a partial load of the policy. Currently the only reason to ignore a uprobe/hook is that the digest verification failed. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
The previous commits altered pkg/k8s, so we need to run "make crds" in order to do code generation. This commit is the result of running that command. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
Re-generate the tracing-policy reference documentation to reflect the fact that a binaryDigests verification failure will no longer cause a policy to be rejected in its entirety. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
Allow partial policy loading when uprobe binary digest verification fails. Instead of rejecting the policy in its entirety, load the probes for the hooks where digest verification does not fail. The user needs to specify the digestVerificationFailure ignore configuration in order prevent policy rejection. Expose the resulting hook status in the policy status, so that users can can understand which hooks are in effect. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
f0746a1 to
7568105
Compare
This alters the output of "tetra tracingpolicy list" to include the state of the policy's hook status inline, when the overall policy state is partially loaded. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
The previous commit changed the behavior when digest verification failures happen. Before, we would reject the policy in its entirety. Now, we salvage the uprobes within the policy where digest verification did not fail. As such, we need to update the unit tests to reflect this change in behavior. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
This is needed in order to expose the hook status when all hooks are ignored. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
To cover hook statuses for kprobes. Since kprobes allow specifying a list for "Call", a hook can be partially loaded. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
The previous commits altered sensors.proto. This commit is the result of running "make protogen" to generate code based on that interface change. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
This is exposed via TracingPolicyStatus. By setting a hook status that is not STATUS_LOADED, we set the overall TracingPolicyState to TP_STATE_PARTIALLY_ENABLED. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
A previous commit made it so that sensors are still created when no hooks are attached. This was needed in order to convey hook status when every hook is ignored. There are code paths that assume sensor/collection state exists if the sensor was created. These locations need a way to detect if the sensor is empty, in order to avoid logging a spurious error. Signed-off-by: Andy Strohman <astrohma@isovalent.com>
This is the final PR of a series of PRs that implement uprobe binary digest verification. See this parent PR for the list of related PRs and the initial feedback I received before breaking that large parent PR into a series of smaller children PRs.
The core uprobe binary digest validation logic in now in the
mainbranch. The current behavior is to reject a policy in its entirety if any uprobes within the policy fail digest verification. This PR changes that behavior such that we salvage the uprobes that don't fail verification, and therefore partially load the policy. If all the hooks fail digest verification, the policy will still be considered loaded, even though none of its hooks were attached to.Since a policy can be partially loaded for the first time, we need a way to communicate which hooks were attached to and which hooks we did not attach to (due to digest verification failure). This PR extends our API's
TracingPolicyStatusto expose each hook's status. Initially, the hook status will only be exposed for uprobe and kprobe policies, because these are the only type of policies that can currently have partial loads. We can use this framework to expose other policy type's hook status if/when they need the ability to do partial loading.Since our API now exposes the hook status, this PR extends the
tetra tracingpolicy listcommand by interleaving the hook status within its pre-existing output when a policy is found to be partially loaded.The following policy and command output demonstrates how this works.
Tetragon also logs which hooks failed verification when the policy is loaded: