Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
@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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants