Skip to content
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.

Sign up for GitHub

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

Jump to bottom

tetragon: un/pin fixes #3079

Merged
merged 13 commits into from
Nov 13, 2024
Merged

tetragon: un/pin fixes #3079

merged 13 commits into from
Nov 13, 2024

Conversation

olsajiri
Copy link
Contributor

@olsajiri olsajiri commented Nov 6, 2024

factor sensors unpin-ing

fixes #3033

@olsajiri olsajiri force-pushed the pr/olsajiri/on_exit branch from 0372a21 to a74ba3d Compare November 7, 2024 08:15
@olsajiri olsajiri added the release-note/minor This PR introduces a minor user-visible change label Nov 7, 2024
@olsajiri olsajiri force-pushed the pr/olsajiri/on_exit branch 4 times, most recently from fa5b10d to d4b21c0 Compare November 7, 2024 13:39
@olsajiri olsajiri changed the title Pr/olsajiri/on exit tetragon: un/pin fixes Nov 7, 2024
@olsajiri olsajiri marked this pull request as ready for review November 7, 2024 14:30
@olsajiri olsajiri requested a review from a team as a code owner November 7, 2024 14:30
mtardy
mtardy approved these changes Nov 7, 2024
View reviewed changes
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! The 2 sec sleeps in testing don't really look awesome though, isn't there a way to write something faster and more reliable?

pkg/sensors/tracing/enforcer_test.go
run(1, expected)

// remove the policy and we should get rid of the enforcement
err = mgr.DeleteTracingPolicy(ctx, tp.TpName(), "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the commit you specify "test that simulates enforcement policy unload (not exit)" but you use Delete instead of DisableTracingPolicy, is that intended? You don't exactly test the scenario here #3033, maybe you want to modify one or add another one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, it should be about the same, but I can add that scenario as well, thnx

@olsajiri
Copy link
Contributor Author

olsajiri commented Nov 7, 2024

looks good! The 2 sec sleeps in testing don't really look awesome though, isn't there a way to write something faster and more reliable?

yea can't think of any now.. but will give it some more thought

@olsajiri olsajiri force-pushed the pr/olsajiri/on_exit branch 2 times, most recently from 90706c1 to 758340d Compare November 8, 2024 07:52
kkourt
kkourt reviewed Nov 8, 2024
View reviewed changes
pkg/observer/observer.go Outdated Show resolved Hide resolved
@olsajiri olsajiri force-pushed the pr/olsajiri/on_exit branch from 758340d to 304f7af Compare November 8, 2024 12:53
@netlify Netlify
Copy link

netlify bot commented Nov 8, 2024 •
edited
Loading

✅ Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit cc12099
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/673206b0d464980008aaf5c1
😎 Deploy Preview https://deploy-preview-3079--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

kkourt
kkourt approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@olsajiri
bugtool: Use nil opts when calling ebpf.LoadPinnedMap
0b75e21 
Since linux kernel commit [1] we need to have O_RDWR to get link
object properly.

Use O_RDWR (nil for opts) in ebpf.LoadPinnedMap calls that face the
path for the first time and stumble on link pin and fail.

[1] 25fc94b2f02d bpf: link: Refuse non-O_RDWR flags in BPF_OBJ_GET
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri
tetragon: Add extra sleep in TestEnforcerMulti test
d3268b7 
Now when we create link pins by default all sensor unloads remove link pin
and because bpf pinned links removal is asynchronous, we need to wait to be
sure it's gone.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri
tetragon: Fix kprobe multi attach override link path
b7f1458 
Add proper 'override' suffix to the link path for kprobe multi
attach override link.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri
tetragon: Fix fmodret attach override link path
fddadc1 
Add proper 'override' suffix to the link path for fmodret
attach override link.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri
tetragon: Remove program.LinkPin field
fcbcfcf 
It's not used.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri
tetragon: Unpin link in unloader objects
736dfc1 
Make sure we unpin link when closing the link.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri
tetragon: Pin links unconditionally
6c314d3 
Now when we remove pins when we unload sensor/program,
we can pin links unconditionally.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri
tetragon: Add unpin arg to sensor Unload and Destroy
8bc8f9b 
Propage unpin flag from sensor.Unload/Destroy down to the unloader level
and unpin progs/maps/links only when instructed by the unpin argument.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri
tetragon: Rename unloader.PinUnloader to unloader.ProgUnloader
9407b1b 
It unloads program and it fits better next to the existing
LinkUnloader type.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri
tetragon: Use DestroyHook to remove enforcer handle
6492752 
Currently we remove the enforcer handle when its sensor is unloaded,
which makes enforcer unusable when the tracing policy/sensor is just
disabled and we are not able to enable it back.

Moving the enforcer handle removal to DestroyHook, which will trigger
only when the sensor is removed.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri
tetragon: Factor persistent enforcement tests
c0912c9 
Use sensor manager with the new KeepSensorsOnExit setup
instead of loading sensors manually.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri
tetragon: Add persistent enforcement test for false keep-sensors-on-exit
861bbc0 
Add persistent enforcement test that simulates tetragon normal exit,
WITHOUT KeepSensorsOnExit and make sure the enforcement is removed.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri
tetragon: Add persistent enforcement test for policy unload
cc12099 
Add persistent enforcement test that simulates enforcement policy unload
(not exit) with KeepSensorsOnExit and make sure the enforcement is removed.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri olsajiri force-pushed the pr/olsajiri/on_exit branch from 304f7af to cc12099 Compare November 11, 2024 13:29
@olsajiri olsajiri merged commit 11212cf into main Nov 13, 2024
43 checks passed
@olsajiri olsajiri deleted the pr/olsajiri/on_exit branch November 13, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtardy mtardy mtardy approved these changes

@kkourt kkourt kkourt approved these changes

@kevsecurity kevsecurity Awaiting requested review from kevsecurity kevsecurity is a code owner automatically assigned from cilium/tetragon

Assignees
No one assigned
Labels
release-note/minor This PR introduces a minor user-visible change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Option --keep-sensors-on-exit doesn't work with disabling/enabling policies
3 participants
@olsajiri @kkourt @mtardy