-
Notifications
You must be signed in to change notification settings - Fork 376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tetragon: sensor cleanup fixes #2968
Conversation
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
0391ee3
to
c0d2d7d
Compare
We did not properly cleanup the directory hierarchy we create for policy/sensor/program. Adding the missing cleanup and adding error check when creating the directories. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Currently we do not remove map/progs bpffs hierarchy directories if we fail to load the sensor for some reason, fixing that. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Moving loop out of loadMaps function, it will ease up following patch, there's no functional change. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
c0d2d7d
to
5ea987f
Compare
Currently we do not unload progs/maps that were loaded prior the fail to load the sensor for some reason, fixing that. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding tests for proper cleanup after sensor unload. Signed-off-by: Jiri Olsa <jolsa@kernel.org>
5ea987f
to
5309737
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good and I'm happy to merge it as is.
One thought I had was that maybe we would want to add a waning if the last sensor did not manage to remove the policy directory. So maybe something like:
diff --git a/pkg/sensors/collection.go b/pkg/sensors/collection.go
index 979517b61..6e4f8b2ba 100644
--- a/pkg/sensors/collection.go
+++ b/pkg/sensors/collection.go
@@ -5,9 +5,12 @@ package sensors
import (
"fmt"
+ "os"
+ "path/filepath"
"sync"
"github.com/cilium/tetragon/api/v1/tetragon"
+ "github.com/cilium/tetragon/pkg/option"
"github.com/cilium/tetragon/pkg/tracingpolicy"
"go.uber.org/multierr"
)
@@ -134,6 +137,15 @@ func (c *collection) unload() error {
if err != nil {
return fmt.Errorf("failed to unload all sensors from collection %s: %w", c.name, err)
}
+
+ if tp := c.tracingpolicy; tp != nil {
+ tpPath := filepath.Join(option.Config.BpfDir, sanitize(tp.TpName()))
+ _, err := os.Stat(path)
+ if os.IsNotExist(err) {
+ return fmt.Erorrf("failed to remove policy direcotry: %s", tpPath)
+ }
+ }
+
return nil
}
As I was writing the above, however, I realized that we might have a more significant problem. Two policies might have the same name if they belong to different k8s namespaces. See for example #2337.
So I think we need something like:
diff --git a/pkg/sensors/tracing/policyhandler.go b/pkg/sensors/tracing/policyhandler.go
index 16603d3ab..a799a975b 100644
--- a/pkg/sensors/tracing/policyhandler.go
+++ b/pkg/sensors/tracing/policyhandler.go
@@ -25,6 +25,10 @@ func (h policyHandler) PolicyHandler(
) (sensors.SensorIface, error) {
policyName := policy.TpName()
+ if tpNs, ok := tp.(tracingpolicy.TracingPolicyNamespaced); ok {
+ policyName = fmt.Printf("%s/%s", tpNs, policyName)
+ }
+
spec := policy.TpSpec()
yea, we need to fix that.. let's address that in separate PR, thanks |
do better job on cleaning up after sensors is unloaded or fails to load