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

add checks for lost samples and empty record in capabilities tracer #2350

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

matthyx
Copy link
Contributor

@matthyx matthyx commented Jan 9, 2024

add checks for lost samples and empty record in capabilities tracer

https://kubernetes.slack.com/archives/CSYL75LF6/p1704806538839969

How to use

[ describe what reviewers need to do in order to validate this PR ]

Testing done

[Describe the testing you have done before submitting this PR. Please include both the commands you issued as well as the output you got.]

@matthyx matthyx requested a review from alban as a code owner January 9, 2024 15:44
Copy link
Member

@eiffel-fl eiffel-fl left a comment

Choose a reason for hiding this comment

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

Hi!

Thank you for the fix!
I will run the CI and test it locally in the meanwhile but everything looks fine from code inspection.
Can you please git blame and add a Fixes: tag to your commit message?

Best regards.

@matthyx
Copy link
Contributor Author

matthyx commented Jan 10, 2024

@eiffel-fl what should I do with git blame?

@eiffel-fl
Copy link
Member

@eiffel-fl what should I do with git blame?

Sorry, use git blame pkg/gadgets/trace/capabilities/tracer/tracer.go to find the original commit which added the file you modify.

@matthyx
Copy link
Contributor Author

matthyx commented Jan 10, 2024

Sorry, use git blame pkg/gadgets/trace/capabilities/tracer/tracer.go to find the original commit which added the file you modify.

Ok will do right after my daily

@matthyx
Copy link
Contributor Author

matthyx commented Jan 10, 2024

@eiffel-fl like that? I have added it to the line before Signed-off-by:

@eiffel-fl
Copy link
Member

@eiffel-fl like that? I have added it to the line before Signed-off-by:

Normally, we also add the title of the commit within bracket, like:
e33ef85
But it will do the trick! Thank you for adding it!

I tested it locally and everything seems to be OK, sadly I cannot reproduce your bug:

$ sudo ./ig trace capabilities                               (remotes/matthyx/panic) %
RUNTIME.CONTAINERNAME                PID        COMM             SYSCALL               CAP CAPNAME            AUDIT               VERDICT
practical_dhawan                     18337      chown            fchownat              0   CHOWN              1                   Allow  
practical_dhawan                     18337      chown            fchownat              0   CHOWN              1                   Allow  
practical_dhawan                     18338      chmod            fchmodat              3   FOWNER             1                   Allow  
practical_dhawan                     18338      chmod            fchmodat              4   FSETID             1                   Allow 

Thinking more to this, maybe you can add a sentence in the commit message about your modification avoiding a panic?

Fixes: 39aefb9 ("pkg/gadgets: Add capabilities CO-RE tracer.")
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
@matthyx
Copy link
Contributor Author

matthyx commented Jan 10, 2024

Thinking more to this, maybe you can add a sentence in the commit message about your modification avoiding a panic?

done

@eiffel-fl eiffel-fl merged commit 7e977db into inspektor-gadget:main Jan 10, 2024
53 checks passed
@eiffel-fl
Copy link
Member

Thank you for the fix!

@matthyx matthyx deleted the panic branch January 10, 2024 11:42
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.

None yet

2 participants