tracee-rules: evaluate parsed input with OPA #829
Conversation
|
|
|
@danielpacak you mention that this PR is just partially implementing the optimization, do you want to merge it, or it's draft until you add the part in consumeSources? If you want to add one by one, then I think the |
Hey @itaysk Sorry for the confusion. I believe we should merge #830 first because we do need ToUnstructured method to do any optimisation. Then I'll rebase this one and add missing code in consumeSources. Does it make sense? |
| @@ -166,6 +189,15 @@ func (engine *Engine) consumeSources(done <-chan bool) { | |||
| } | |||
| } | |||
|
|
|||
| func (engine *Engine) dispatchEvent(s types.Signature, pe regosig.ParsedEvent, event types.Event) { | |||
| switch { | |||
| case strings.Contains(reflect.TypeOf(s).String(), "rego"): | |||
There was a problem hiding this comment.
See here for a quick benchmark: https://gist.github.com/simar7/7f671bf4ce4bd685d4c9346f44c1e9c1
There was a problem hiding this comment.
@danielpacak @simar7 why are we not using the type assert/switch here? I remember Daniel mentioned something the doesn't work but isn't this what we want? https://play.golang.org/p/en8jJCHWiqw
There was a problem hiding this comment.
I think our case is more like https://play.golang.org/p/wETpR604pdj
@itaysk I tried one more time using switch type and the following snippet seems to be working:
func (engine *Engine) dispatchEvent(s types.Signature, pe ParsedEvent) {
switch s.(type) {
case *regosig.RegoSignature: // Make sure to use pointer because we use pointer receiver for the RegoSignature implementation
engine.signatures[s] <- pe
default:
engine.signatures[s] <- pe.Event
}
}However, it will introduce the cycle because of the engine.ParsedEvent type. We could fix that by moving the ParsedEvent to a shared package, maybe types package?
@simar7 Could you explain what was the advantage of using reflect.TypeOf(s) here?
There was a problem hiding this comment.
There is no advantage in this case. The only problem there is that we lose testability if we hardcode the type to *regosig.RegoSignature.
In the test for consumeSources we use a fake type called regoFakeSignature
tracee/tracee-rules/engine/engine_test.go
Line 16 in 6df0969
default path instead which is not the desired behaviour.
func (engine *Engine) dispatchEvent(s types.Signature, pe ParsedEvent) {
switch s.(type) {
case *regosig.RegoSignature: // Note this isn't the same as regoFakeSignature
engine.signatures[s] <- pe
default:
engine.signatures[s] <- pe.Event
}
}
The other approach if we really wanted to use switch type assert would be to manually list each gosig as a case (since they are all defined types) and default for each regosig. So something like the following:
switch s.(type) {
case *stdioOverSocket, *antiDebugging:
engine.signatures[s] <- pe.Event
default:
engine.signatures[s] <- pe
}
}
This doesn't scale well as it would require a change to this function every time a new Go signature is added/used.
For the above reasons I went with a reflect.TypeOf(s) instead. If I've missed something or if there is a better way to do it, happy to understand and discuss.
There was a problem hiding this comment.
if the only holdback from doing it using type assertion (assuming we agree it's better) is just the test, I'd sacrifice it and not he production code. But I think this is a symptom of a bigger problem, the fact that engine is knowing which concrete signature types it's invoking is breaking the entire premise of the Signature interface. I don't have a better suggestion yet, just wanted to bring this point to attention. @simar7 @danielpacak
|
Observation: While this is a step in the right direction, we still have more work to do. This is how the flamegraph looks today: And this is how it looks after implementing this change: As you can see the JSON round tripper was replaced with our InterfaceToValue call. While quantitatively this is a minor improvement, three additional tracks to work on can be identified as follows:
|
With OPA Go SDK we can significantly improve performance by passing rego.EvalParsedInput instead of rego.EvalInput option to rego.Eval() method. Co-authored-by: Simar <simar@linux.com> Signed-off-by: Simar <simar@linux.com> Signed-off-by: Daniel Pacak <pacak.daniel@gmail.com>
|
thanks for working on merging this. do we have a sense on how much this improves by? I believe @simar7 told my ~10%, is that true? @danielpacak |
@itaysk Here are sample results from the benchmark of rego.Eval input versus rego.EvalParsedInput for 6 (open source) and 71 (closed source) signatures with 2 input events that trigger findings. Evaluating a prepared query seems to be 3-4 times faster. However, overall performance of the app depends on the workload, i.e. the stream of input events and how often an event is selected for evaluation (signatureIndex impl.). |
With OPA Go SDK we can significantly improve performance by passing
rego.EvalParsedInput instead of rego.EvalInput option to rego.Eval()
method.
Co-authored-by: Simar simar@linux.com
Signed-off-by: Simar simar@linux.com
Signed-off-by: Daniel Pacak pacak.daniel@gmail.com