Skip to content

pkg/option: allow policy-filter-map-entries configurable via flag #4331

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+34 −2
Open

pkg/option: allow policy-filter-map-entries configurable via flag #4331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bpf/process/policy_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ struct {

struct {
__uint(type, BPF_MAP_TYPE_HASH_OF_MAPS);
__uint(max_entries, POLICY_FILTER_MAX_POLICIES);
__uint(max_entries, 1); // will be resized by agent when needed
Copy link
Member

@mtardy mtardy Nov 20, 2025

Choose a reason for hiding this comment

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

This is used by policy_filter_cgroup_maps just nearby. Maybe we would need to update both and remove that const?

Copy link
Contributor Author

@kyledong-suse kyledong-suse Nov 21, 2025

Choose a reason for hiding this comment

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

Sure, I think it makes sense to me. I'll update both of them and remove that const.

Copy link
Contributor Author

@kyledong-suse kyledong-suse Dec 1, 2025

Choose a reason for hiding this comment

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

@mtardy Given another think, the POLICY_FILTER_MAX_POLICIES in policy_filter_cgroup_maps represents how many policies can apply to a single cgroup (inner map size of policy_filter_cgroup_maps).
I’m fine to simplify and drive both policy_filter_maps and policy_filter_cgroup_maps from the same policy-filter-map-entries setting and remove that const. However, I just want you to aware that if we want to update both, it would be mixing a global capacity knob with a per-cgroup limit, which could waste memory if we config more policies globally.

__type(key, u32); /* policy id */
__array(
values, struct {
Expand Down
3 changes: 3 additions & 0 deletions docs/data/tetragon_flags.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions pkg/defaults/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ const (

// defaults for the {k,u}retprobes lru cache
DefaultRetprobesCacheSize = 4096

// defaults for the policy filter map
DefaultPolicyFilterMapEntries = 128
)

var (
Expand Down
3 changes: 3 additions & 0 deletions pkg/defaults/defaults_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,7 @@ const (

// defaults for the {k,u}retprobes lru cache
DefaultRetprobesCacheSize = 4096

// defaults for the policy filter map
DefaultPolicyFilterMapEntries = 128
)
5 changes: 5 additions & 0 deletions pkg/option/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@ type config struct {
ExecveMapSize string

RetprobesCacheSize int

PolicyFilterMapEntries int
}

var (
Expand All @@ -155,6 +157,9 @@ var (

// Set default value for {k,u}retprobes lru events cache
RetprobesCacheSize: defaults.DefaultRetprobesCacheSize,

// set default value for the policy filter map
PolicyFilterMapEntries: defaults.DefaultPolicyFilterMapEntries,
}
)

Expand Down
6 changes: 6 additions & 0 deletions pkg/option/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ const (
KeyExecveMapSize = "execve-map-size"

KeyRetprobesCacheSize = "retprobes-cache-size"

KeyPolicyFilterMapEntries = "policy-filter-map-entries"
)

type UsernameMetadaCode int
Expand Down Expand Up @@ -289,6 +291,8 @@ func ReadAndSetFlags() error {
Config.ExecveMapSize = viper.GetString(KeyExecveMapSize)

Config.RetprobesCacheSize = viper.GetInt(KeyRetprobesCacheSize)

Config.PolicyFilterMapEntries = viper.GetInt(KeyPolicyFilterMapEntries)
return nil
}

Expand Down Expand Up @@ -483,4 +487,6 @@ func AddFlags(flags *pflag.FlagSet) {
flags.String(KeyExecveMapSize, "", "Set size for execve_map table (allows K/M/G suffix)")

flags.Int(KeyRetprobesCacheSize, defaults.DefaultRetprobesCacheSize, "Set {k,u}retprobes events cache maximum size")

flags.Int(KeyPolicyFilterMapEntries, defaults.DefaultPolicyFilterMapEntries, "Set entries for policy_filter_map table (default 128)")
Copy link
Member

@mtardy mtardy Nov 20, 2025

Choose a reason for hiding this comment

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

could you be a little bit more explicit in the help what this map is for and why changing its size would matter (why you want to increase or decrease its size). Succinct is still better than extra verbose, but out of context on the map, it's not clear why this matters with this help (at least for me :)).

Copy link
Contributor Author

@kyledong-suse kyledong-suse Nov 21, 2025

Choose a reason for hiding this comment

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

WDYT about this one?
"Set maximum number of policies in policy_filter_maps (default 128). This map restricts tracing policies to specific pods/containers. Increase if you have many policies, decrease to save memory if you have few policies."

Copy link
Member

@mtardy mtardy Nov 21, 2025

Choose a reason for hiding this comment

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

yep it's a nice suggestion! Don't need to repeat the default as cobra should display it in the help.

}
7 changes: 6 additions & 1 deletion pkg/policyfilter/map.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

"github.com/cilium/tetragon/pkg/bpf"
"github.com/cilium/tetragon/pkg/config"
"github.com/cilium/tetragon/pkg/option"
)

const (
Expand Down Expand Up @@ -72,6 +73,10 @@ func newPfMap(enableCgroupMap bool) (PfMap, error) {
return PfMap{}, fmt.Errorf("loading spec for %s failed: %w", objPath, err)
}

if _, ok := spec.Maps["policy_filter_maps"]; ok {
spec.Maps["policy_filter_maps"].MaxEntries = uint32(option.Config.PolicyFilterMapEntries)
}
Comment on lines +76 to +78
Copy link
Member

@mtardy mtardy Nov 20, 2025

Choose a reason for hiding this comment

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

see other comment about using SetMaxEntries, you should see example by grepping


var ret PfMap
if ret.policyMap, err = openMap(spec, MapName, polMapSize); err != nil {
return PfMap{}, fmt.Errorf("opening map %s failed: %w", MapName, err)
Expand All @@ -80,7 +85,7 @@ func newPfMap(enableCgroupMap bool) (PfMap, error) {
if enableCgroupMap {
if ret.cgroupMap, err = openMap(spec, CgroupMapName, polMaxPolicies); err != nil {
releaseMap(ret.policyMap)
return PfMap{}, fmt.Errorf("opening cgroup map %s failed: %w", MapName, err)
return PfMap{}, fmt.Errorf("opening cgroup map %s failed: %w", CgroupMapName, err)
Copy link
Member

@mtardy mtardy Nov 20, 2025

Choose a reason for hiding this comment

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

maybe this could be part of another commit fixing this typo?

Copy link
Contributor Author

@kyledong-suse kyledong-suse Nov 21, 2025

Choose a reason for hiding this comment

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

Sure, I'll remove the typo fix from this commit.
Do you like another commit to fix this typo in the same PR or a separate PR?

Copy link
Member

@mtardy mtardy Nov 21, 2025

Choose a reason for hiding this comment

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

same PR is fine :)

}
}

Expand Down
7 changes: 7 additions & 0 deletions pkg/sensors/program/loader_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
cachedbtf "github.com/cilium/tetragon/pkg/btf"
"github.com/cilium/tetragon/pkg/logger"
"github.com/cilium/tetragon/pkg/logger/logfields"
"github.com/cilium/tetragon/pkg/option"
"github.com/cilium/tetragon/pkg/sensors/unloader"
)

Expand Down Expand Up @@ -976,6 +977,12 @@ func doLoadProgram(
}
}

// Set MaxEntries for policy_filter_maps if it exists in the spec.
// This ensures the spec matches the user-defined value.
if ms, ok := spec.Maps["policy_filter_maps"]; ok {
ms.MaxEntries = uint32(option.Config.PolicyFilterMapEntries)
}

Comment on lines +980 to +985
Copy link
Member

@mtardy mtardy Nov 20, 2025

Choose a reason for hiding this comment

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

ah yes already have a facility to do that higher level in the loader (it's just above your new code btw), please use the SetMaxEntries on the map itself directly, you need to have the map handle.

Copy link
Contributor Author

@kyledong-suse kyledong-suse Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking to reuse SetMaxEntries. However, after digging into the code, I had the following findings:

  • policy_filter_maps is not a program.Map.
  • policy_filter_maps is created as raw ebpf.Map in newPfMap(), which is called during policyfilter.New()
  • policyfilter.GetState() is called in StartSensorManager(), which happens before sensors load.

So your suggestion is to refactor policy_filter_maps from ebpf.Map to program.Map? Also need to refactor this PfMap struct.

Copy link
Member

@mtardy mtardy Nov 21, 2025

Choose a reason for hiding this comment

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

Let's ask since we can before investing time in this :)!

Hey @tpapagian and @kkourt, I see you are the one you touched this code, any reason why you didn't use program.Map in the first place?

Copy link
Contributor

@kkourt kkourt Nov 21, 2025

Choose a reason for hiding this comment

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

Not reason I remember. Is the idea be to make the map a part of the base/exec sensor?

Copy link
Contributor Author

@kyledong-suse kyledong-suse Nov 21, 2025

Choose a reason for hiding this comment

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

IMO, I think it makes sense to add policy_filter_maps to base sensor (similar to execve_map).

Copy link
Contributor Author

@kyledong-suse kyledong-suse Nov 25, 2025

Choose a reason for hiding this comment

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

What do you think @kkourt and @tpapagian?

Copy link
Contributor

@Andreagit97 Andreagit97 Nov 25, 2025

Choose a reason for hiding this comment

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

Sorry if I jump in, but AFAIK policy_filter_maps is a global map and its lifecycle is managed by the policy filter. Updating the maxEntries in pkg/policyfilter/map.go as you did should be enough. Why are we bumping the number of entries also here?

Copy link
Contributor

@Andreagit97 Andreagit97 Nov 25, 2025

Choose a reason for hiding this comment

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

Oh, I see, even if we use the MapReplacements, we need the maxEntries to match the original spec...directly from the cilium/ebpf doc

	// MapReplacements takes a set of Maps that will be used instead of
	// creating new ones when loading the CollectionSpec.
	//
	// For each given Map, there must be a corresponding MapSpec in
	// CollectionSpec.Maps, and its type, key/value size, max entries and flags
	// must match the values of the MapSpec.
	//
	// The given Maps are Clone()d before being used in the Collection, so the
	// caller can Close() them freely when they are no longer needed.
	MapReplacements map[string]*Map

Sorry for the noise

Copy link
Contributor Author

@kyledong-suse kyledong-suse Dec 1, 2025

Choose a reason for hiding this comment

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

@mtardy @kkourt I’ve been thinking more about the refactoring in the past week. IMO, it’s better to create a separate issue and PR for that work. The current issue and PR focus on adding a user-configurable knob for the size of policy_filter_maps. If we include the refactoring in the same PR, it will introduce more churn and go beyond the original scope. What do you think?

// Find all the maps referenced by the program, so we'll rewrite only
// the ones used.
var progSpec *ebpf.ProgramSpec
Expand Down
Loading