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

tetragon: Change uprobe spec #1975

Merged
merged 2 commits into from
Jan 17, 2024
Merged

tetragon: Change uprobe spec #1975

merged 2 commits into from
Jan 17, 2024

Conversation

olsajiri
Copy link
Contributor

@olsajiri olsajiri commented Jan 14, 2024

Currently we define uprobe with path/symbol path, which not
handy when you have more symbols from single path to probe.

Changing the spec so the uprobe is defined by path and array
of symbols, like:

 spec:
   uprobes:
   - path: /bin/bash
     symbols:
     - "_start"
     - "main"
       "builtin_help"
TracingPolicy: Replace symbol field (string) with symbols (array of strings) in uprobe spec. If using policies with uprobes, you need to replace the symbol field.

Currently we define uprobe with path/symbol path, which not
handy when you have more symbols from single path to probe.

Changing the spec so the uprobe is defined by path and array
of symbols, like:

 spec:
   uprobes:
   - path: /bin/bash
     symbols:
     - "_start"
     - "main"
       "builtin_help"

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri olsajiri added the release-note/minor This PR introduces a minor user-visible change label Jan 14, 2024
Adding support to generate uprobe policy that contains
all function symbols from the bspecified binary, like:

  $ tetra tracingpolicy generate uprobes --binary /bin//bash | head -20
  apiVersion: cilium.io/v1alpha1
  kind: TracingPolicy
  metadata:
    creationTimestamp: "2024-01-14T22:33:21Z"
    name: uprobes
  spec:
    uprobes:
    - message: ""
      path: /bin//bash
      symbols:
      - rl_old_menu_complete
      - maybe_make_export_env
      - initialize_shell_builtins
      - extglob_pattern_p
      - dispose_cond_node
      - decode_prompt_string
      - show_var_attributes
      - push_var_context
      - buffered_ungetchar
      - isnetconn

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
@olsajiri olsajiri force-pushed the pr/olsajiri/uprobe_spec_change branch from 8c84716 to e0a2eaa Compare January 14, 2024 22:40
@olsajiri olsajiri marked this pull request as ready for review January 15, 2024 11:54
@olsajiri olsajiri requested a review from a team as a code owner January 15, 2024 11:54
@olsajiri olsajiri requested a review from tixxdz January 15, 2024 11:54
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.

Hey, what was the motivation behind this? I guess it's a bit similar to what we do in kprobe with list, but also a bit different.

Could you update the documentation on uprobe as well? :)

@olsajiri
Copy link
Contributor Author

Hey, what was the motivation behind this? I guess it's a bit similar to what we do in kprobe with list, but also a bit different.

it matches the kernel interface where you register symbols for given path/binary
and also the common usecase is to register more symbols for binary, so the current
implementation is bit cumbersome because you always need to specify binary/path
for each symbol

Could you update the documentation on uprobe as well? :)

yep.. I was checking on that, but we did not add any so far,
so there was nothing to fix ;-) I'll add some paragraph

@mtardy
Copy link
Member

mtardy commented Jan 16, 2024

Hey, what was the motivation behind this? I guess it's a bit similar to what we do in kprobe with list, but also a bit different.

it matches the kernel interface where you register symbols for given path/binary and also the common usecase is to register more symbols for binary, so the current implementation is bit cumbersome because you always need to specify binary/path for each symbol

Could you update the documentation on uprobe as well? :)

yep.. I was checking on that, but we did not add any so far, so there was nothing to fix ;-) I'll add some paragraph

Cool thanks for the details. It was just added indeed, it's still very limited so it should be quick to update, https://tetragon.io/docs/concepts/tracing-policy/hooks/#uprobes.

Copy link
Contributor

@jrfastab jrfastab left a comment

Choose a reason for hiding this comment

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

well the generate stuff is fun.

@jrfastab jrfastab merged commit 64e5baf into main Jan 17, 2024
32 checks passed
@jrfastab jrfastab deleted the pr/olsajiri/uprobe_spec_change branch January 17, 2024 06:23
@lambdanis lambdanis added release-note/breaking-changes and removed release-note/minor This PR introduces a minor user-visible change labels Mar 15, 2024
@lambdanis
Copy link
Contributor

Hey, replacing a required field in the CRD is a breaking change. Technically this should mean moving to v1alpha2 I believe. But at the very least it should be documented.

@olsajiri could you add a release note instructing users how to upgrade? It would be nice to update the examples too.

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.

4 participants