gadgets/run: Support eBPF parameters #2119

burak-ok · 2023-10-09T11:09:58Z

Gadgets usually define different constants in the eBPF code to change
its behaviour. This commit implements the logic allow users setting
these flags from the CLI.

The gadget information to carry the different parameters exposed by the
bpf programs. This is gathered from the list provided in the gadget
definition and by using the BTF information of the gadget.

This PR replaces #2058

How to test

(Optional) Build your gadget image

$ cd gadgets/trace/exec
$ sudo -E ig image build -t ghcr.io/burak-ok/trace_exec:latest .
INFO[0000] Experimental features enabled                
Successfully built ghcr.io/burak-ok/trace_exec:latest@sha256:fe644cdbf0f175c36727d4bc4e55d60b27f28d756f96f426e1e4313fa8c742e

Execute the gadget using different flags (events are generated with setuidgid 1000:1000 cat /dev/null in a container)

$ go run -exec 'sudo -E' ./cmd/ig/... run ghcr.io/burak-ok/trace_exec:latest
INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME                               PID     COMM             UID      GID      RET       FNAME                                                
mycontainer                                         363944  setuidgid        0        0        -2        /etc/ld.so.cache                                     
mycontainer                                         363944  setuidgid        0        0        -2        /lib/x86_64-linux-gnu/glibc-hwcaps/x86-64-v3/libm.so.
mycontainer                                         363944  setuidgid        0        0        -2        /lib/x86_64-linux-gnu/glibc-hwcaps/x86-64-v2/libm.so.
mycontainer                                         363944  setuidgid        0        0        -2        /lib/x86_64-linux-gnu/tls/x86_64/x86_64/libm.so.6    
mycontainer                                         363944  setuidgid        0        0        -2        /lib/x86_64-linux-gnu/tls/x86_64/libm.so.6           
mycontainer                                         363944  setuidgid        0        0        -2        /lib/x86_64-linux-gnu/tls/x86_64/libm.so.6           
mycontainer                                         363944  setuidgid        0        0        -2        /lib/x86_64-linux-gnu/tls/libm.so.6                  
...
mycontainer                                         363944  cat              1000     1000     3         /lib/libc.so.6                                       
mycontainer                                         363944  cat              1000     1000     3         /dev/nul

Both, events with 0 and 1000 uid are captured

Capture only events from 1000

$ go run -exec 'sudo -E' ./cmd/ig/... run ghcr.io/burak-ok/trace_exec:latest --uid 1000
INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME                               PID     COMM             UID      GID      RET       FNAME                                                
mycontainer                                         364336  cat              1000     1000     -2        /etc/ld.so.cache                                     
mycontainer                                         364336  cat              1000     1000     -2        /lib/x86_64-linux-gnu/glibc-hwcaps/x86-64-v3/libm.so.
mycontainer                                         364336  cat              1000     1000     -2        /lib/x86_64-linux-gnu/glibc-hwcaps/x86-64-v2/libm.so.
mycontainer                                         364336  cat              1000     1000     -2        /lib/x86_64-linux-gnu/tls/x86_64/x86_64/libm.so.6    
mycontainer                                         364336  cat              1000     1000     -2        /lib/x86_64-linux-gnu/tls/x86_64/libm.so.6
...
mycontainer                                         364336  cat              1000     1000     3         /lib/libc.so.6                                       
mycontainer                                         364336  cat              1000     1000     3         /dev/null

Fixes #1927

burak-ok · 2023-10-09T11:11:18Z

Currently the compilation errors are resulting from the branch this PR is based on

alban · 2023-10-09T16:03:25Z

I think it's better to have generic CLI flags:

# ig run ghcr.io/burak-ok/trace_exec:latest --param uid=1000 --param gid=1000

Or in the short form:

# ig run ghcr.io/burak-ok/trace_exec:latest -p uid=1000 -p gid=1000

This is similar to systemd-run --property=NAME=VALUE

This would avoid conflicts with other flags such as --authfile, --filter, --output etc.

cobra should support repeated flags with https://pkg.go.dev/github.com/spf13/pflag#StringSlice

mauriciovasquezbernal · 2023-10-09T18:32:36Z

I think it's better to have generic CLI flags:

The advantage of explicitly exposing the flags is that the user can understand which ones are available and their documentation.

This would avoid conflicts with other flags such as --authfile, --filter, --output etc.

If this is really a concern we could try to namespace the gadget flags or to print a warning when a flag overlaps.

alban · 2023-10-09T19:17:36Z

The advantage of explicitly exposing the flags is that the user can understand which ones are available and their documentation.

Do you mean by using --help? But the documentation can also describe the parameters available even with a generic flag, like --output is doing for the columns.

If this is really a concern we could try to namespace the gadget flags or to print a warning when a flag overlaps.

Do you mean namespacing like this?

# ig run ghcr.io/burak-ok/trace_exec:latest --param-uid=1000 --param-gid=1000

That would work for me, although it is longer to type than -p uid=1000.

The advantage of a generic flag is that it is clearer that the flag comes from the specific gadget.

mauriciovasquezbernal · 2023-10-09T19:28:04Z

Do you mean by using --help? But the documentation can also describe the parameters available even with a generic flag, like --output is doing for the columns.

Yes, that's right. But I think they're more discoverable if exposed directly as flags on the CLI instead of being under a -p // --params flag. Let's ping @blixtra about this.

burak-ok · 2023-10-13T10:52:20Z

I added a GADGET_PARAM macro to indicate a parameter which should be configurable through userspace. Additionally now the validation and generation of the gadget param metadata is added.

As seen in trace_exec/program.bpf.c the GADGET_PARAM macro is optional. If the metadata has the correct definition of the parameter it is not needed. Therefore optional. It indeed increases readability for users and developoers looking only in the eBPF code

For a test builder image please use mine:

go run --exec "sudo -E" ./cmd/ig/... image build ./gadgets/trace_exec/ -t trace_exec --update-metadata --builder-image ghcr.io/burak-ok/ebpf-builder-burak:latest -v

A rebase will happen once #2107 is merged or there were larger changes in that PR. Then I can polish all little details

eiffel-fl

I have some comments but it is looking good!
I will thoroughly review and test it once #2107 is merged.

eiffel-fl · 2023-10-13T11:07:12Z

cmd/common/registry.go

+			onlyArgs := []string{}
+			for i := 0; i < len(args); i++ {
+				arg := args[i]
+				if arg[0] == '-' {
+					// --bar=foo: skip them
+					if strings.Contains(arg, "=") {
+						continue
+					}
+
+					// --bar with default value, skip it without skipping next arg
+					var name string
+					if arg[0] == '-' {
+						if len(arg) >= 2 && arg[1] == '-' {
+							name = arg[2:]
+						} else {
+							name = arg[1:]
+						}
+					}


In C, I would say OK for this code as there is no other mean.
But in golang, you can definitely use strings.Split() to get all the arguments as an array (["foo", "--bar=corge", "-c", "--quux"]), you can also use strings.HasPrefix() to check if argument begins with --.

But in golang, you can definitely use strings.Split() to get all the arguments as an array (["foo", "--bar=corge", "-c", "--quux"])

The args variable is already such a slice

you can also use strings.HasPrefix() to check if argument begins with --.

You brought up an idea and then I just used .TrimLeft to remove the dashes (Be it 1 or 2)

pkg/gadgets/run/tracer/run.go

pkg/gadgets/run/tracer/tracer.go

mauriciovasquezbernal

I like how it's going, some initial comments.

Are you planning to add tests for Validate() and Populate() as done in #2107 and #1866?

cmd/common/registry.go

pkg/gadgets/run/types/types.go

pkg/gadgets/run/types/metadata.go

mauriciovasquezbernal · 2023-10-17T20:33:01Z

pkg/gadgets/run/types/metadata.go

 )

+type EBPFParam struct {
+	params.ParamDesc `yaml:",inline"`


I think it's fine to use params.ParamDesc as base, however I want to check @flyth's opinion.

pkg/gadgets/run/tracer/run.go

gadgets/trace_exec/program.bpf.c

gadgets/trace_open/gadget.yaml

pkg/gadgets/run/tracer/run.go

mauriciovasquezbernal · 2023-10-17T21:29:50Z

btw, I extracted some commits to https://github.com/inspektor-gadget/inspektor-gadget/pull/2152/commits in order to make the review of this easier.

burak-ok · 2023-10-18T15:23:22Z

I like how it's going, some initial comments.

Are you planning to add tests for Validate() and Populate() as done in #2107 and #1866?

Thanks for reminding me. Planned and forgot :D

btw, I extracted some commits to https://github.com/inspektor-gadget/inspektor-gadget/pull/2152/commits in order to make the review of this easier.

Nice, thank you :) I will remove the commits once the metadata branch is rebased/merged

eiffel-fl

It is looking good, I am nonetheless wondering about the macro as it would be really helpful for users to not need it and only consider parameters regarding their elf locations.

cmd/common/registry.go

eiffel-fl · 2023-10-19T08:18:30Z

include/gadget/macros.h

+// GADGET_PARAM is used to indicate that a given variable is used as a parameter for a given eBPF
+// Users of Inspektor Gadget can set these values from userspace
+#define GADGET_PARAM(name) \
+	const void * gadget_param_##name __attribute__((unused));


I am really wondering about this one.
The parameters are volatile which should already prevent the compiler to remove them.
Moreover, I think we can basically states that everything in data which is const is a parameter?

I think the idea here is to have the metadata as the single source of truth.

Additionally if we would "mark" every global const volatile variable as a parameter all variables from the includes will be also shown for the gadget.
For example we include the mntns_filter.h header which would in turn expose the gadget_filter_by_mntns variable directly to the user. For that variable we have another specific flag since we need to do more for filtering by mntns than setting the variable to true.
But on the other hand we could just filter out all global const volatile variables which have a gadget_ prefix.

In the end for me the GADGET_PARAM macro is more useful.

Other thoughts?

The GADGET_PARAM macro is fine and we can perfectly go with this in a first version.
But removing this would remove some work needed by the users (i.e. not forget using the macro).

Additionally if we would "mark" every global const volatile variable as a parameter all variables from the includes will be also shown for the gadget.
For example we include the mntns_filter.h header which would in turn expose the gadget_filter_by_mntns variable directly to the user. For that variable we have another specific flag since we need to do more for filtering by mntns than setting the variable to true.

This is indeed a problem...

Note that, the metadata would still be used to set the parameters even if we do not use macro.

Note that, the metadata would still be used to set the parameters even if we do not use macro.

Correct and this is intended and what I also like

pkg/gadgets/run/types/metadata.go

eiffel-fl · 2023-10-19T08:36:31Z

pkg/gadgets/run/types/metadata.go

+	_, ok = btfConst.Type.(*btf.Volatile)
+	if !ok {
+		result = multierror.Append(result, fmt.Errorf("%q is not a volatile", name))
+		return result
+	}


I guess so, otherwise the compiler would just replace all the occurrence of the variable by its value (i.e. 0).

eiffel-fl · 2023-10-19T12:48:39Z

I tested it but it does not recognize the given arguments:

$ sudo -E ./ig image build --local -t ghcr.io/burak-ok/trace_exec:latest .
INFO[0000] Experimental features enabled                
Successfully built ghcr.io/burak-ok/trace_exec:latest@sha256:4ab6510a958b81728bab0e23ac79fb52d7dc29387c06dcf5aec62771fedf70de
$ sudo -E ./ig image list                               burak/mauro/gadget-params % u=
INFO[0000] Experimental features enabled                
REPOSITORY                                                     TAG                                                           DIGEST      
ghcr.io/burak-ok/trace_exec                                    latest                                                        4ab6510a958b
$ sudo -E ./ig run ghcr.io/burak-ok/trace_exec:latest --uid 100   
INFO[0000] Experimental features enabled                
WARN[0000] GadgetMetadata.EBPFParams: map[]             
Error: unknown flag: --uid

mauriciovasquezbernal

Looks great!

I left some comments. I'll do a final pass later on.

pkg/gadgets/run/tracer/run.go

pkg/gadgets/run/types/metadata.go

pkg/gadgets/run/tracer/run.go

testdata/populate_metadata1.bpf.c

pkg/gadgets/run/types/metadata_test.go

mauriciovasquezbernal

IMO it's ready to go. I tried rebasing #1866 on top of this and everything seems to be working fine. The UX will be much better once we have #935 in place.

Thanks a lot for handling this.

However, as I wrote some of the code in this PR, I'll like somebody else to check this before merging. @alban do you think it's fine to continue with the CLI flags and then explore the possibility of having a general -p flag? (#2119 (comment))

pkg/gadgets/run/types/metadata.go

include/gadget/macros.h

eiffel-fl

I tested it and it works perfectly:

$ sudo -E ./ig run ghcr.io/inspektor-gadget/gadget/trace_exec:test --ignore-failed=false
INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME            PID               PPID              COMM              UID              GID              RETVAL          
flamboyant_hoover                45139             45102             bash              0                0                -2              
^C%                                                                                                                                      $ sudo -E ./ig run ghcr.io/inspektor-gadget/gadget/trace_open:test --failed
INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME          PID              COMM             UID              GID              RET   FNAME                           
flamboyant_hoover              45309            cat              0                0                -2    /tmp/foobar

This is really a cool feature to have!

eiffel-fl · 2023-10-20T07:58:57Z

cmd/common/registry.go

@@ -16,6 +16,7 @@ package common



Can you please take another look at the commit message, I do not understand the first sentence of the second paragraph.

I hope it is better now

cmd/common/registry.go

burak-ok · 2023-10-23T14:18:32Z

Will wait for #2174 to be reviewed and merged first. This PR triggeres the bug that is getting fixed in #2174 very often when using the flags

Gadgets usually define different constants in the eBPF code to change its behaviour. This commit implements the logic allow users setting these flags from the CLI. The parameters are gathered from a list provided in the gadget metadata definition. Then the variable names are compared with the BTF information and the corresponding variables will be replaced with the user provided values. Co-authored-by: Burak Ok <burakok@mircosoft.com> Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com> Signed-off-by: Burak Ok <burakok@microsoft.com>

Signed-off-by: Burak Ok <burakok@microsoft.com>

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com> Signed-off-by: Burak Ok <burakok@microsoft.com>

burak-ok · 2023-10-27T14:41:25Z

Will wait for #2174 to be reviewed and merged first. This PR triggeres the bug that is getting fixed in #2174 very often when using the flags

The bug is fixed and I also tested the failing integration test locally - it works :)

burak-ok requested review from flyth and mauriciovasquezbernal as code owners October 9, 2023 11:09

burak-ok mentioned this pull request Oct 9, 2023

gadgets/run: Support eBPF parameters #2058

Closed

1 task

burak-ok linked an issue Oct 9, 2023 that may be closed by this pull request

run: Support passing parameters to containerized gadgets #1927

Closed

mauriciovasquezbernal force-pushed the mauricio/new-metadata-file branch from 8d19cd6 to 08b97ff Compare October 9, 2023 12:44

mauriciovasquezbernal requested a review from alban as a code owner October 9, 2023 12:44

mauriciovasquezbernal force-pushed the mauricio/new-metadata-file branch 11 times, most recently from 7095108 to db876d1 Compare October 12, 2023 16:51

burak-ok force-pushed the burak/mauro/gadget-params branch from 0c2beeb to 2b5ccb8 Compare October 13, 2023 10:52

burak-ok requested review from eiffel-fl and mqasimsarfraz as code owners October 13, 2023 10:52

eiffel-fl reviewed Oct 13, 2023

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/new-metadata-file branch 2 times, most recently from 787de8a to 86c4c54 Compare October 13, 2023 21:50

burak-ok force-pushed the burak/mauro/gadget-params branch from 2b5ccb8 to a3dd846 Compare October 16, 2023 15:52

mauriciovasquezbernal force-pushed the mauricio/new-metadata-file branch 3 times, most recently from 6bc7926 to 571f837 Compare October 17, 2023 19:09

mauriciovasquezbernal mentioned this pull request Oct 17, 2023

pkg/params: Implement some extra functions #2152

Merged

mauriciovasquezbernal reviewed Oct 17, 2023

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/new-metadata-file branch 2 times, most recently from b248be5 to 9516b8d Compare October 18, 2023 16:41

burak-ok force-pushed the burak/mauro/gadget-params branch from a3dd846 to 1b0831f Compare October 18, 2023 18:04

Base automatically changed from mauricio/new-metadata-file to main October 18, 2023 21:03

eiffel-fl reviewed Oct 19, 2023

View reviewed changes

burak-ok force-pushed the burak/mauro/gadget-params branch 2 times, most recently from b8fc45a to 16f3e5e Compare October 19, 2023 12:39

mauriciovasquezbernal reviewed Oct 19, 2023

View reviewed changes

burak-ok force-pushed the burak/mauro/gadget-params branch from 16f3e5e to b24dbcc Compare October 19, 2023 16:03

mauriciovasquezbernal approved these changes Oct 19, 2023

View reviewed changes

pkg/gadgets/run/types/metadata.go Outdated Show resolved Hide resolved

include/gadget/macros.h Outdated Show resolved Hide resolved

eiffel-fl approved these changes Oct 20, 2023

View reviewed changes

burak-ok force-pushed the burak/mauro/gadget-params branch from b24dbcc to 937178d Compare October 20, 2023 10:06

mauriciovasquezbernal and others added 4 commits October 27, 2023 16:35

gadgets/run: Add test for EBPFParam

1e4be9b

Signed-off-by: Burak Ok <burakok@microsoft.com>

gadgets/trace/open: Remove unused const

5b3c826

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com> Signed-off-by: Burak Ok <burakok@microsoft.com>

gadgets/trace/open,exec: Add bpf parameters

ced35d7

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com> Signed-off-by: Burak Ok <burakok@microsoft.com>

burak-ok force-pushed the burak/mauro/gadget-params branch from 937178d to ced35d7 Compare October 27, 2023 14:41

burak-ok merged commit 4610dc2 into main Oct 27, 2023
50 checks passed

burak-ok deleted the burak/mauro/gadget-params branch October 27, 2023 15:45

alban mentioned this pull request Jan 17, 2024

[RFE] Documenting eBPF parameters in image-based gadgets #2374

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gadgets/run: Support eBPF parameters #2119

gadgets/run: Support eBPF parameters #2119

burak-ok commented Oct 9, 2023

burak-ok commented Oct 9, 2023

alban commented Oct 9, 2023

mauriciovasquezbernal commented Oct 9, 2023

alban commented Oct 9, 2023

mauriciovasquezbernal commented Oct 9, 2023

burak-ok commented Oct 13, 2023 •

edited

eiffel-fl left a comment

eiffel-fl Oct 13, 2023

burak-ok Oct 16, 2023

mauriciovasquezbernal left a comment

mauriciovasquezbernal Oct 17, 2023

mauriciovasquezbernal commented Oct 17, 2023

burak-ok commented Oct 18, 2023

eiffel-fl left a comment

eiffel-fl Oct 19, 2023

burak-ok Oct 19, 2023

eiffel-fl Oct 19, 2023

burak-ok Oct 19, 2023

eiffel-fl Oct 19, 2023

eiffel-fl commented Oct 19, 2023

mauriciovasquezbernal left a comment

mauriciovasquezbernal left a comment

eiffel-fl left a comment

eiffel-fl Oct 20, 2023

burak-ok Oct 20, 2023

burak-ok commented Oct 23, 2023

burak-ok commented Oct 27, 2023

gadgets/run: Support eBPF parameters #2119

gadgets/run: Support eBPF parameters #2119

Conversation

burak-ok commented Oct 9, 2023

How to test

burak-ok commented Oct 9, 2023

alban commented Oct 9, 2023

mauriciovasquezbernal commented Oct 9, 2023

alban commented Oct 9, 2023

mauriciovasquezbernal commented Oct 9, 2023

burak-ok commented Oct 13, 2023 • edited

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal commented Oct 17, 2023

burak-ok commented Oct 18, 2023

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiffel-fl commented Oct 19, 2023

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burak-ok commented Oct 23, 2023

burak-ok commented Oct 27, 2023

burak-ok commented Oct 13, 2023 •

edited