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

Add trace_tcpdrop image based gadget #2477

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Add trace_tcpdrop image based gadget #2477

merged 1 commit into from
Feb 9, 2024

Conversation

claudiamarcubina
Copy link
Member

Add trace_tcpdrop image based gadget

Adding the conversion of tcpdrop gadget to the image based version.

We have currently no integration tests for tcpdrop, so created this issue for working on that: #2476

Testing done

ubuntu@gifted-rattlesnake:~/go/src/github.com/inspektor-gadget/inspektor-gadget$ sudo -E ./ig run claudiafirstregistry.azurecr.io/gadget:claudia-convert-trace-tcpdrop-gadget
INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME                SRC                      DST                      STATE                TCPFLAGS            REASON              PID                 UID                 GID                 TASK            
netem                                172.17.0.2:37584         1.1.1.1:20480            2                    2                   46                  19342               0                   0                   wget            
netem                                172.17.0.2:37584         1.1.1.1:20480            2                    2                   46                  19342               0                   0                   wget            
netem                                172.17.0.2:32464         1.1.1.1:47873            1                    16                  46                  19342               0                   0                   wget            
netem                                172.17.0.2:32464         1.1.1.1:47873            1                    16                  46                  19342               0                   0                   wget            
netem                                172.17.0.2:32464         1.1.1.1:47873            1                    16                  46                  19342               0                   0                   wget            
netem                                172.17.0.2:32464         1.1.1.1:47873            1                    16                  46                  19342               0                   0                   wget            
netem                                172.17.0.2:32464         1.1.1.1:47873            4                    17                  46                  19342               0                   0                   wget

@claudiamarcubina claudiamarcubina force-pushed the claudia/convert-trace-tcpdrop-gadget branch 2 times, most recently from 2c030e3 to e6ac2bc Compare February 8, 2024 19:03
alban
alban reviewed Feb 8, 2024
View reviewed changes
Copy link
Member

@alban alban left a comment

Choose a reason for hiding this comment

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

Thanks! A few initial comments.

gadgets/trace_tcpdrop/program.bpf.c Outdated Show resolved Hide resolved
gadgets/trace_tcpdrop/program.bpf.c Outdated Show resolved Hide resolved
gadgets/trace_tcpdrop/program.bpf.c Show resolved Hide resolved
@claudiamarcubina claudiamarcubina force-pushed the claudia/convert-trace-tcpdrop-gadget branch 3 times, most recently from ad4d511 to 4c08fe7 Compare February 8, 2024 21:59
This was referenced Feb 8, 2024
Open
Open
mauriciovasquezbernal
mauriciovasquezbernal approved these changes Feb 9, 2024
View reviewed changes
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.

LGTM from code inspection. I haven't been able to test it because I don't have a recent enough kernel available right now to test it.

gadgets/trace_tcpdrop/gadget.yaml Outdated
attributes:
template: pid
- name: tid
description: 'TODO: Fill field description'
Copy link
Member

Choose a reason for hiding this comment

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

nit: fill (or remove) this descriptions.

gadgets/trace_tcpdrop/program.bpf.c
Comment on lines +54 to +63
gadget_mntns_id mount_ns_id;
__u32 pid;
__u32 tid;
__u32 uid;
__u32 gid;
__u8 task[TASK_COMM_LEN];
Copy link
Member

Choose a reason for hiding this comment

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

The original gadget has two instances of this fields:

inspektor-gadget/pkg/gadgets/trace/tcpdrop/tracer/bpf/tcpdrop.h

Lines 37 to 38 in 616a94c

struct proc_ctx proc_current;
struct proc_ctx proc_socket;

I think it's fine to use only one of this for now given that we don't support sub-structures in the event. Would you mind adding a small comment indicating it should be converted to a structure once we support that?

Copy link
Member Author

Choose a reason for hiding this comment

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

✅ Also added an entry to #2316 (comment)

@claudiamarcubina claudiamarcubina force-pushed the claudia/convert-trace-tcpdrop-gadget branch 2 times, most recently from 5c4061e to 55d01a4 Compare February 9, 2024 19:36
@claudiamarcubina
gadgets: Add trace_tcpdrop source and metadata
42a5187 
Signed-off-by: Claudia Marcu <claudiamarcu@microsoft.com>
@claudiamarcubina claudiamarcubina force-pushed the claudia/convert-trace-tcpdrop-gadget branch from 55d01a4 to 42a5187 Compare February 9, 2024 19:39
@claudiamarcubina
Copy link
Member Author

claudiamarcubina commented Feb 9, 2024 •
edited

LGTM from code inspection. I haven't been able to test it because I don't have a recent enough kernel available right now to test it.

Many thanks for the feedback! Will merge once the CI checks have passed.

ubuntu@gifted-rattlesnake:~/go/src/github.com/inspektor-gadget/inspektor-gadget$ sudo -E ./ig run ghcr.io/inspektor-gadget/gadget/trace_tcpdrop:claudia-convert-trace-tcpdrop-gadget
INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME                SRC                      DST                      STATE               TCPFLAGS            REASON              PID                 UID                GID                TASK            
netem                                172.17.0.2:52416         1.1.1.1:47873            tcp_established     16                  …_REASON_QDISC_DROP 32735               0                  0                  wget            
netem                                172.17.0.2:52416         1.1.1.1:47873            tcp_established     16                  …_REASON_QDISC_DROP 32735               0                  0                  wget            
netem                                172.17.0.2:52416         1.1.1.1:47873            tcp_established     24                  …_REASON_QDISC_DROP 32735               0                  0                  wget            
netem                                172.17.0.2:52416         1.1.1.1:47873            tcp_established     16                  …_REASON_QDISC_DROP 32735               0                  0                  wget            
netem                                172.17.0.2:52416         1.1.1.1:47873            tcp_established     16                  …_REASON_QDISC_DROP 32735               0                  0                  wget

@claudiamarcubina claudiamarcubina merged commit b0cdfa2 into main Feb 9, 2024
57 checks passed
@claudiamarcubina claudiamarcubina deleted the claudia/convert-trace-tcpdrop-gadget branch February 9, 2024 19:58
@mauriciovasquezbernal mauriciovasquezbernal mentioned this pull request Feb 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alban alban alban left review comments

@mauriciovasquezbernal mauriciovasquezbernal mauriciovasquezbernal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@claudiamarcubina @alban @mauriciovasquezbernal