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

Socket enricher for containerized gadgets #1997

Merged
merged 1 commit into from
Sep 7, 2023
Merged

Conversation

alban
Copy link
Member

@alban alban commented Sep 4, 2023

Socket enricher for containerized gadgets

Before this patch, the socket enricher was only available for built-in gadgets. This patch adds support for containerized gadgets.

As example, it uses the tcpretrans gadget.

How to use

go run -exec 'sudo -E' ./cmd/ig/... run --prog @./gadgets/trace_tcpretrans_x86.bpf.o --definition @./gadgets/trace_tcpretrans.yaml

Testing done

Generate events with:

$ docker run -ti --rm --cap-add NET_ADMIN --name=netem wbitt/network-multitool -- /bin/bash
bash-5.1# tc qdisc add dev eth0 root netem drop 25% ^C
bash-5.1# wget 1.1.1.1

Observe events with:

$ go run -exec 'sudo -E' ./cmd/ig/... run --prog @./gadgets/trace_tcpretrans_x86.bpf.o --definition @./gadgets/trace_tcpretrans.yaml
INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME     SRC              DST          TASK     PID     UID      GID
netem                     172.17.0.3:35536 1.1.1.1:80   wget     105692  0        0
netem                     172.17.0.3:56494 1.1.1.1:443  wget     105692  0        0

Or:

$ go run -exec 'sudo -E' ./cmd/ig/... run --prog @./gadgets/trace_tcpretrans_x86.bpf.o --definition @./gadgets/trace_tcpretrans.yaml -o yaml
INFO[0000] Experimental features enabled                
container: ""
dst:
  addr: 1.1.1.1
  kind: ""
  name: ""
  namespace: ""
  port: 80
  proto: TCP
  v: 4
gid: 0
hostnetwork: false
mntns: 4026532835
mntns_id: 4026532835
namespace: ""
node: ""
pid: 105842
pod: ""
runtime:
  containerId: 3b45f227d11673f86ecff58fecb0e350f0e807935b9005a87c81dca1be5e3a89
  containerImageName: wbitt/network-multitool
  containerName: netem
  runtimeName: docker
src:
  addr: 172.17.0.3
  kind: ""
  name: ""
  namespace: ""
  port: 58712
  proto: TCP
  v: 4
task: wget
timestamp: "1970-01-01T01:00:00.000000000+01:00"
uid: 0

TODO:

  • Endianness issue with source port and destination port
  • Missing ip source and ip destination (waiting for run: Support endpoint enrichment #1825 before fixing this)
  • Duplicate 'mntns_id' and 'mntns' field

TODO unrelated to this PR (will be fixed in another PR):

  • Wrong timestamp
  • Duplicate 'container' field

Fixes #1920

@alban alban force-pushed the alban_byob_tcpretrans branch 4 times, most recently from 559afd2 to 22478a3 Compare September 5, 2023 10:29
@alban alban marked this pull request as ready for review September 5, 2023 10:29
@alban
Copy link
Member Author

alban commented Sep 5, 2023

I rebased after #1825. I fixed some TODOs but left some for future PRs (see updated PR description).

@@ -176,6 +180,21 @@ func (t *Tracer) installTracer() error {
}
}

// Only create socket enricher if this is used by the tracer
for _, m := range t.spec.Maps {
if m.Name == networktracer.SocketsMapName {
Copy link
Member Author

Choose a reason for hiding this comment

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

At the moment, networktracer.SocketsMapName is defined as follows:

SocketsMapName = "sockets"

What do you think about renaming it to "gadget_sockets" so that it is namespaced correctly for third-party gadget authors?

Choose a reason for hiding this comment

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

Sure, I think we must rename that to avoid collisions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed it with the "ig_" prefix like some other maps and progs.

I also moved the constant from the networktracer package to the socketenricher package where it semantically belongs.

Choose a reason for hiding this comment

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

I renamed it with the "ig_" prefix like some other maps and progs.

I'm trying to use gadget_ everywhere as in https://github.com/inspektor-gadget/inspektor-gadget/blob/main/pkg/gadgets/common/mntns_filter.h for the API. I think we're using ig_ in the programs because the length restriction. However I don't have a preference in any of them, but I'd prefer everything aligned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I renamed to gadget_sockets.

I will merge once the CI finishes.

Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal 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 overall. Some comments, specially regarding the filtering capabilities of this gadget.

gadgets/trace_tcpretrans.bpf.c Outdated Show resolved Hide resolved
pkg/gadgets/run/tracer/tracer.go Outdated Show resolved Hide resolved
pkg/gadgets/run/tracer/tracer.go Show resolved Hide resolved
gadgets/Makefile Show resolved Hide resolved
@@ -176,6 +180,21 @@ func (t *Tracer) installTracer() error {
}
}

// Only create socket enricher if this is used by the tracer
for _, m := range t.spec.Maps {
if m.Name == networktracer.SocketsMapName {

Choose a reason for hiding this comment

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

Sure, I think we must rename that to avoid collisions.

gadgets/trace_tcpretrans.yaml Outdated Show resolved Hide resolved
@alban
Copy link
Member Author

alban commented Sep 6, 2023

Thanks for the review. I've update the PR and it is ready for another review round.

Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

Just one comment about the naming of the map, but it's not blocking. LGTM, thanks!

Before this patch, the socket enricher was only available for built-in
gadgets. This patch adds support for containerized gadgets.

As example, it uses the tcpretrans gadget.

Signed-off-by: Alban Crequy <albancrequy@linux.microsoft.com>
@alban alban merged commit 488ec3b into main Sep 7, 2023
47 checks passed
@alban alban deleted the alban_byob_tcpretrans branch September 7, 2023 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

run: Support socket enricher
2 participants