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

documentation: more detailed description re FollowFD/UnfollowFD #2228

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

christian-2
Copy link
Contributor

@christian-2 christian-2 requested review from mtardy and a team as code owners March 15, 2024 14:13
Copy link

netlify bot commented Mar 15, 2024

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 0a2312b
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/65f94562b1cbaf00080715cc
😎 Deploy Preview https://deploy-preview-2228--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mtardy mtardy added area/documentation Improvements or additions to documentation release-note/docs This PR updates the documentation. labels Mar 15, 2024
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Thanks I think it's a cool addition detailing and adding an example. I have some comments however, and also a nit: could you wrap the lines correctly at around 80, it feels a bit weird how it's cut now. Vim can do that with gq for example.

Comment on lines 656 to 658
The `fd_install` kernel function is called each time a file descriptor must be
installed into the file descriptor table of a process, typically referenced
within system calls like `open` or `openat`. It is a good place for tracking
file descriptor and filename matching.
`FollowFD` is typically used at hook points where a file descriptor and its
associated file name appear together. The kernel function `fd_install`
is a case in point.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I get the replacement here, do you think what was said on fd_install was wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just thought the mentioning of a processes file descriptor table might cause confusion opposite the eBPF table where Tetragon keeps associations between file descriptors and file names. I'll put the earlier sentence back in.

While the mapping between the file descriptor and file name remains in place
(i.e. between `FollowFD` and `UnfollowFD` for the same file descriptor) tracing
policies may refer to file names where file descriptors
would otherwise have to serve instead. This offers greater convenience and
Copy link
Member

Choose a reason for hiding this comment

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

I get the meaning here but the "have to serve instead" is a bit difficult to understand. Maybe we can simplify "tracing policies may refer to file names instead of file descriptors"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have no issue with that.

Comment on lines 692 to 693
`/etc/passwd`. The system call `sys_write` only receives a file descriptor
(not a file name) as argument. Yet with a bracketing pair for `FollowFD`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`/etc/passwd`. The system call `sys_write` only receives a file descriptor
(not a file name) as argument. Yet with a bracketing pair for `FollowFD`
`/etc/passwd`. The system call `sys_write` only receives a file descriptor,
not a file name, as an argument. Yet with a bracketing pair for `FollowFD`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

Comment on lines 692 to 693
`/etc/passwd`. The system call `sys_write` only receives a file descriptor
(not a file name) as argument. Yet with a bracketing pair for `FollowFD`
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by "bracketing pair"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using a metaphor that compares FollowFD and UnfollowFD to an opening and closing bracket, respectively. The reference to a file name appears in between, like an element between two brackets in mathematical set notation; hence the pair consisting of FollowFD and UnfollowFD is bracketing the reference to the file name, so to speak. Is it clearer now?

file descriptor and filename matching.
`FollowFD` is typically used at hook points where a file descriptor and its
associated file name appear together. The kernel function `fd_install`
is a case in point.
Copy link
Member

Choose a reason for hiding this comment

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

I think it's my English but I never saw in my life "a case in point" in a documentation 😸

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIK "a case in point" is synonymous with "a good example of". I can use the second, no problem.

Copy link
Contributor

@kevsecurity kevsecurity left a comment

Choose a reason for hiding this comment

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

LGTM

@christian-2
Copy link
Contributor Author

@mtardy Thanks for your careful review. I'll submit another version according to my earlier replies. @kevsecurity Thanks for your approval. Please (both of you) let me know if you have further comments.

Comment on lines 652 to 654
descriptors and filenames. It however needs to maintain a state
correctly, see [`UnfollowFD`](#unfollowfd-action) and
[`CopyFD`](#copyfd-action) related actions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
descriptors and filenames. It however needs to maintain a state
correctly, see [`UnfollowFD`](#unfollowfd-action) and
[`CopyFD`](#copyfd-action) related actions.
descriptors and filenames. After its creation, the mapping can be maintained via [`UnfollowFD`](#unfollowfd-action) and [`CopyFD`](#copyfd-action) actions. Note that proper maintenance of the mapping is up to the tracing policy writer.

@@ -682,11 +686,79 @@ This action uses the dedicated `argFd` and `argName` fields to get respectively
the index of the file descriptor argument and the index of the name argument in
the call.

While the mapping between the file descriptor and file name remains in place
(i.e. between `FollowFD` and `UnfollowFD` for the same file descriptor) tracing
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(i.e. between `FollowFD` and `UnfollowFD` for the same file descriptor) tracing
(that is, between `FollowFD` and `UnfollowFD` for the same file descriptor) tracing

note: we generally try to avoid use of latin abbreviations. See: https://docs.cilium.io/en/stable/contributing/docs/docsstyle/#don-t-use-latin-abbreviations.

@christian-2
Copy link
Contributor Author

@kkourt Thanks also for your review. I'm fine with these changes, plus: I replaced "via" (another Latin term) by "through" and "file name" by "filename" (the latter appears to be the more frequent spelling in the surrounding material.)

@kkourt
Copy link
Contributor

kkourt commented Mar 20, 2024

Keep in mind that you can re-request a review by clicking on the github button "Re-request review".

1

Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thanks!

@kkourt kkourt requested review from mtardy and kkourt March 20, 2024 10:16
Copy link
Member

@mtardy mtardy 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 lot, that's a nice addition on this difficult to understand feature :)

@mtardy
Copy link
Member

mtardy commented Mar 20, 2024

I wanted to just merge this but could you remove the leading "*" in the commit title, I'm wondering if it can create weird stuff when we list the commits then. Maybe replace with docs: or just remove the star :) thanks, sorry for this last nit

Signed-off-by: Christian Hörtnagl <christian2@univie.ac.at>
@christian-2
Copy link
Contributor Author

@mtardy re PR: you are welcome; re no star: you are right, I was following a local convention by habit (now fixed)

@mtardy mtardy merged commit 10188c6 into cilium:main Mar 20, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation release-note/docs This PR updates the documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants