Repository navigation
fix: honor matchExpressions in resource selectors - #71
Conversation
Selectors that only use matchExpressions were treated as empty, so the Tetragon PodSelector and the Kive trap matches ended up cluster-wide. Tetragon now receives the expressions, and resource filters with expressions are skipped for Kive, which cannot express them.
The loops that copied the match labels into a KiveTrapMatch iterated over all resource filters of the trap instead of the current one, so the labels of every filter ended up in every match. Resource filters are matched with a logical OR, so this made each match narrower than intended, and it also defeated the skip for filters that use selector.matchExpressions, whose labels still leaked into the matches of the other filters.
A trap whose resource filters use selector.matchExpressions gets a Kive policy without any match, so the honeytoken is planted but nothing watches it. Until now the only hint was a log line on info level, while the status of the DeceptionPolicy reported a successful deployment. Such traps are now reported as failed captor deployments with a dedicated reason, so that the gap is visible in the status. Tetragon policies have the opposite problem: they have a single podSelector, but resource filters are matched with a logical OR, so merging the selectors of several filters narrows down the selection. That cannot be expressed in one policy, so we at least log a warning when it happens.
The test asserted the same pods as the test without a namespace, and no pod outside the namespace carried the label, so it also passed when the namespace was ignored. It now adds a pod with the label in another namespace.
| matchingByNamespace := []client.Object{} // The objects that match the namespaces for this ResourceFilter | ||
| matchingByLabels := []client.Object{} // The objects that match the labels for this ResourceFilter | ||
|
|
||
| selectorIsEmpty := resourceFilter.Selector == nil || |
| // UnsupportedSelectors is set if the captor cannot watch all resources that the trap selects, | ||
| // because the deployment strategy cannot express the selectors of the trap. | ||
| UnsupportedSelectors bool | ||
| } |
There was a problem hiding this comment.
[ARCHITECTURE] Please change how this is handled here: I would prefer if the Kive-Unsupported-Selectors case gets a loud error that is placed in Errors and UnsupportedSelectors being set. This way, ImpliesSuccess needs no special handling, a success shall only happen if there are no errors; that contract should stay clean.
Having UnsupportedSelectors still makes sense for the same reason as we have MissingTetragon; simply to pick a nicer status code than a plain "Failed" or "Error"
There was a problem hiding this comment.
done. the Kive-unsupported case now returns ErrKiveUnsupportedSelectors in Errors and sets UnsupportedSelectors next to it, so ImpliesSuccess is back to "no errors" with no special-casing. the flag only selects the status reason now, like MissingTetragon does. added a test that the flag alone keeps success, only the error flips it
| // A TracingPolicy has a single PodSelector, but resource filters are matched with a | ||
| // logical OR, so filters that carry selectors cannot be translated without losing precision. | ||
| if countResourceFiltersWithSelector(trap) > 1 { | ||
| log.Info("WARNING: multiple resource filters have a selector, but a Tetragon tracing policy has only one podSelector, "+ |
There was a problem hiding this comment.
[ISSUE] Wouldn't it be wise then to make the captor filter BROADER, intentionally risking that it might match much more than what was deployed? The net effect is that Tetragon will generate more logs and the alert-forwarder is our point where we could still filter them? I think this is better than trying to be more precise and missing to watch certain items? Still, a warning should be logged when this occurs.
Also, we still have the exact-filepath filter, so realistically there shouldn't be a lot of noise from broadening the filter, we will just end up watching more pods than we should.
Also, I know that currently no post-filtering happens in the alert-forwarder; but this is out-of-scope here; this PR can intentionally deliver a broad filter, and a follow-up issue + PR should later introduce post-filtering in the alert forwarder to possibly narrow and filter-out alerts by re-checking the full filter. This is then likely something that makes sense to merge with the alert-forwarder-rewrite to Go, so that the alert forwarder would use the same filtering functions as the controller.
There was a problem hiding this comment.
went broader. one thing i ran into: the Tetragon policy is cluster-wide and namespaces never reach the podSelector, so a labeled filter mixed with a namespace-only filter was narrowing and dropping pods too, not just the multi-selector case. so the rule keys off the filter count now: a single podSelector can represent the trap only with exactly one filter (filters are OR'd). with more than one, the podSelector stays empty and the captor leans on the file path, watching a superset instead of missing pods. warning logged when that happens. a lone selector with labels + expressions is still copied exactly. tests cover the mixed case. alert-forwarder post-filtering as a follow-up sounds right, out of scope here
There was a problem hiding this comment.
Thanks, I think this is a good middle ground right now, although I see that we have some severe limitations with the mix of Koney's selectors, Tetragon's selectors, and Kive's selectors ... there are likely some corner cases that are still buggy, but I wouldn't worry about this for now. ;)
| CaptorsDeployedReason_UnsupportedSelectors = "UnsupportedSelectors" | ||
|
|
||
| CaptorsDeployedMessage_MissingTetragon = "Cannot deploy captors without Tetragon" | ||
| CaptorsDeployedMessage_UnsupportedSelectors = "Captors do not watch all selected resources, because Kive cannot evaluate selector.matchExpressions" |
There was a problem hiding this comment.
[NIT] I would prefer a briefer message because tooling often only shows a few lines here, e.g. "selector.matchExpressions is not supported with Kive"
There was a problem hiding this comment.
done, now "selector.matchExpressions is not supported with Kive". reused the same string for the sentinel error so the log and the status line match
|
Thanks for the PR, good fixes overall, please consider my important architectural comments. Also, please also update the
I think that is the only location where something changes, unless I missed something. |
|
README done: dropped the not-implemented note on matchExpressions, wrote the current behavior (decoys go to the matching pods/deployments, tetragon captors watch them) and the Kive limit (captors skip pods selected via matchExpressions, reported with the UnsupportedSelectors reason). didn't find another spot that needed touching |
closes #68
matchResources.any[].selectoris ametav1.LabelSelectorbut onlymatchLabelswas ever read, so a selector that only usesmatchExpressionswas treated as no selector at all and the trap went to every workload in the namespacewhat changed:
matching.go— a selector counts as non-empty when either field is set, and listing goes throughmetav1.LabelSelectorAsSelectorso expressions actually narrow the resultutils.go— the TetragonPodSelectornow carries the expressions too (converted to the slim type)utils.go— Kive can't expressmatchExpressionsat all (KiveTrapMatchonly hasmatchLabels), so filters that use them are skipped instead of emitting a labels-only match that would match everything. while fixing that I also had to stop the label loops from walking every filter, they were merging labels across unrelated filters into each matchdp_trap_types.go—IsValidno longer treats an expressions-only selector as emptyCaptorsDeployed=Falsewith a reason, otherwise you get a healthy looking policy with a trap that nothing watchestests cover each of those, including that namespaces still narrow an expressions selector, that labels of one filter don't leak into another, and that the merged Tetragon selector warns when more than one filter carries a selector