internal/appsec/dyngo: atomic instrumentation swapping

[Julio-Guerra]

What does this PR do?

Atomic swapping of AppSec's instrumentations (currently HTTP WAF and gRPC WAF) by reviewing the way dyngo event listeners should be managed. Instead of allowing registering and unregistering event listeners, we rather simplified it so that we can simply atomically swap the root operation instead. This allows a cleaner approach where a root operation can be created async, along with the new set of event listeners, and later on live-swapped, with no more need to deal with concurrent event listeners swapping. The current implementation simply is an atomic pointer swapping.

Overall, this allows a cleaner "pure programming" approach, where an operation is no longer modified once running (assuming all its event listeners are registered on the start events, which is not enforced today), which gives new guarantees on the state of our instrumentation now where new security rules can no longer be partially applied. This PR rather ensures that N security rules updates can safely concurrently live all together.

Motivation

By making the appsec instrumentation modification atomic, we avoid:

• Partial instrumentation states while new rules get applied
• Partial security rules updates while new rules get applied

Describe how to test/QA your changes

Reviewer's Checklist

• Changed code has unit tests for its functionality.
• If this interacts with the agent in a new way, a system test has been added.

[pr-commenter]

Benchmarks

Comparing candidate commit f9ee4bc in PR branch julio.guerra/dyngo-atomic-callback-swapping with baseline commit b438db9 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 0 unstable metrics.

[Hellzy]

I think there's one concerning change that potentially breaks remote activation.

[ahmed-mez]

Mind explaining the rationale behind this change? What's the problem with close(c.stop) ?

[Hellzy]

IIUC this is done to make sure that the goroutine has stopped after exiting this function. Since RC updates allow callbacks to modify anything at any point, making sure that updates aren't still getting applied after exiting Stop() allows a clean shutdown.

[Julio-Guerra]

Yes, the point is to have Stop() "gracefully" stopping, by block the caller until it is done. This basically adds concurrency guarantees to avoid getting remote config updates while stopping. For instance here, without this change, when stopping RC, you could get a RC background goroutine tick at the same time available, leading to an actual RC update. So without blocking the Stop() operation, the subsequent code could be releasing resources needed by the RC goroutine still alive.