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.

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: map changes #3328

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

tetragon: map changes #3328

wants to merge 6 commits into from

Conversation

olsajiri
Copy link
Contributor

@olsajiri olsajiri commented Jan 23, 2025

Adding execve-map-entries option to setup entries of execve_map map, plus related loader changes.

Copy link

netlify bot commented Jan 23, 2025

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit f52b3ee
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/67ab2f7f4cad5800083b385d
😎 Deploy Preview https://deploy-preview-3328--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.

@olsajiri olsajiri added the release-note/minor This PR introduces a minor user-visible change label Jan 23, 2025
@olsajiri olsajiri force-pushed the pr/olsajiri/maps branch 2 times, most recently from e6455a4 to 96eade9 Compare January 26, 2025 21:44
pkg/sensors/base/base.go Fixed Show fixed Hide fixed
@olsajiri olsajiri marked this pull request as ready for review January 28, 2025 13:21
@olsajiri olsajiri requested review from mtardy and a team as code owners January 28, 2025 13:21
@olsajiri olsajiri force-pushed the pr/olsajiri/maps branch 3 times, most recently from 02a5e54 to da0b16e Compare January 31, 2025 12:44
@olsajiri olsajiri requested a review from kkourt February 5, 2025 13:37
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.

Thanks, looks good! Just some nits or questions.

@@ -105,7 +105,7 @@ type Map struct {
}

func (m *Map) String() string {
return fmt.Sprintf("Map{Name:%s PinPath:%s}", m.Name, m.PinPath)
return fmt.Sprintf("Map{Name:%s PinPath:%s Owner: %t}", m.Name, m.PinPath, m.IsOwner())
Copy link
Member

Choose a reason for hiding this comment

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

typo, very tiny nit I just noticed:

Suggested change
return fmt.Sprintf("Map{Name:%s PinPath:%s Owner: %t}", m.Name, m.PinPath, m.IsOwner())
return fmt.Sprintf("Map{Name:%s PinPath:%s Owner:%t}", m.Name, m.PinPath, m.IsOwner())

@@ -118,6 +118,8 @@ const (
KeyEventCacheRetryDelay = "event-cache-retry-delay"

KeyCompatibilitySyscall64SizeType = "enable-compatibility-syscall64-size-type"

KeyExecveMapEntries = "execve-map-entries"
Copy link
Member

Choose a reason for hiding this comment

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

regarding naming, fyi that we have something called --process-cache-size and other size related option, so maybe we should do --execve-map-size or --execve-map-max-entries. Unsure it's relevant.

@@ -419,4 +422,6 @@ func AddFlags(flags *pflag.FlagSet) {
flags.Int(KeyEventCacheRetryDelay, defaults.DefaultEventCacheRetryDelay, "Delay in seconds between event cache retries")

flags.Bool(KeyCompatibilitySyscall64SizeType, false, "syscall64 type will produce output of type size (compatibility flag, will be removed in v1.4)")

flags.String(KeyExecveMapEntries, "32768", "Set entries for execve_map table (entries/size/max)")
Copy link
Member

@mtardy mtardy Feb 10, 2025

Choose a reason for hiding this comment

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

could we reuse ExecveMapMaxEntries here? Or put the values in defaults and reuse it in pkg/sensors/base/base.go?

Or put an empty string given your default is based on custom logic, idk

Copy link
Member

Choose a reason for hiding this comment

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

and not sure why you wrote (entries/size/max)? ah I understand now seeing the code, could you maybe be a bit more explicit in the help doc? :)

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'll add the extra option

@@ -252,6 +254,7 @@ func ReadAndSetFlags() error {

Config.CompatibilitySyscall64SizeType = viper.GetBool(KeyCompatibilitySyscall64SizeType)

Config.ExecveMapEntries = viper.GetString(KeyExecveMapEntries)
Copy link
Member

Choose a reason for hiding this comment

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

proposition: maybe you could call GetExecveMapEntries so that you do the flag parsing here and you have a number in Config.ExecveMapEntries?

pkg/sensors/program/loader.go Show resolved Hide resolved
pkg/sensors/base/base.go Show resolved Hide resolved
Passing sensors maps to program loader so the loader
has access to all sensor maps.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding Maps array to loader LoadOpts object as additional source
of user maps for loader maps resolving.

This will allow program loader to use user maps pinned to another
program and will ease up maps sharing for sensors in following
changes.

At the moment for simplicity we will pass all the sensors maps,
so we filter out user maps before using the array.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Now that the loader can access all sensor maps we no longer need
to pin user maps to programs.

Also for static maps we can add new program.MapUserFrom interface
where it's enough just to pass map pointer to create its user map.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding owner info when printing map object, like:
  Map{Name:m1 PinPath:m1 Owner: false}

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding support to allow to setup execve_map max entries,
so we can control the size of this map.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Adding --execve-map-entries and --execve-map-size options to setup
entries of execve_map map.

It's possible to setup execve_map entries directly with:
  --execve-map-entries 100

or just specify the size of the map with:
  --execve-map-size 100M

Adding log line that shows on startup the execve_map entries setup:
  time="2025-02-11T10:57:06Z" level=info msg="Set execve_map entries 118082" size=99M

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR introduces a minor user-visible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants