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

feat: make logger a standalone package #2600

Closed

Conversation

geyslan
Copy link
Member

@geyslan geyslan commented Jan 17, 2023

Initial Checklist

  • There is an issue describing the need for this PR.

Description (git log)

commit 992c609

logger: make logger a standalone package

Necessary for:

Type of change

  • New feature (non-breaking change adding functionality).

How Has This Been Tested?

sudo ./dist/tracee-ebpf -t uid=1000 -t comm=who --log debug
{"level":"debug","ts":1673991173.4891648,"msg":"osinfo","KERNEL_RELEASE":"5.15.85-1-MANJARO","ARCH":"x86_64","PRETTY_NAME":"\"Manjaro Linux\"","ID":"manjaro","ID_LIKE":"arch","BUILD_ID":"rolling","pkg":"urfave","file":"urfave.go","line":53}
{"level":"debug","ts":1673991173.489193,"msg":"RuntimeSockets: failed to register default","socket":"containerd","error":"failed to register runtime socket stat /var/run/containerd/containerd.sock: no such file or directory","pkg":"flags","file":"containers.go","line":45}
{"level":"debug","ts":1673991173.489201,"msg":"RuntimeSockets: failed to register default","socket":"crio","error":"failed to register runtime socket stat /var/run/crio/crio.sock: no such file or directory","pkg":"flags","file":"containers.go","line":45}
{"level":"debug","ts":1673991173.4892054,"msg":"RuntimeSockets: failed to register default","socket":"podman","error":"failed to register runtime socket stat /var/run/podman/podman.sock: no such file or directory","pkg":"flags","file":"containers.go","line":45}
{"level":"debug","ts":1673991173.4893782,"msg":"osinfo","security_lockdown":"none","pkg":"urfave","file":"urfave.go","line":116}
{"level":"debug","ts":1673991173.4915483,"msg":"BTF","bpfenv":false,"btfenv":false,"vmlinux":true,"pkg":"initialize","file":"bpfobject.go","line":40}
{"level":"debug","ts":1673991173.4915576,"msg":"BPF: using embedded BPF object","pkg":"initialize","file":"bpfobject.go","line":69}
{"level":"debug","ts":1673991173.4931793,"msg":"unpacked CO:RE bpf object file into memory","pkg":"initialize","file":"bpfobject.go","line":144}
{"level":"debug","ts":1673991173.4935548,"msg":"Enricher","error":"error registering enricher: unsupported runtime containerd","pkg":"containers","file":"containers.go","line":64}
{"level":"debug","ts":1673991173.4935637,"msg":"Enricher","error":"error registering enricher: unsupported runtime crio","pkg":"containers","file":"containers.go","line":68}
TIME             UID    COMM             PID     TID     RET              EVENT                ARGS

@geyslan geyslan self-assigned this Jan 17, 2023
@@ -101,3 +103,5 @@ require (
)

replace github.com/kubernetes/cri-api => k8s.io/cri-api v0.23.5-rc.0

replace github.com/aquasecurity/tracee/logger => ./logger
Copy link
Member Author

Choose a reason for hiding this comment

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

To be removed after merge and go get.

@geyslan geyslan marked this pull request as ready for review January 17, 2023 21:46
rafaeldtinoco
rafaeldtinoco previously approved these changes Jan 18, 2023
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco left a comment

Choose a reason for hiding this comment

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

LGTM after a rebase.

Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz left a comment

Choose a reason for hiding this comment

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

I've discussed with @geyslan that a solution with a logger interface on libbpfgo's side should be attempted before going with the go.mod solution for these reasons (IMO):

  1. There are other consumers of libbpfgo besides us. Tying them to our logger from tracee isn't very ok from our end IMO, and I believe it's likely they would set their own logger for libbpfgo if possible (an interface solution would enable that).
  2. This would practically introduce a circular dependency of tracee -> libbpfgo -> tracee logger, where they are not circular only technically by the go.mod. The logger we have in tracee is very much intended for tracee's own particular uses (the log aggregation for example).
  3. Having another go.mod will make development more burdensome whenever we do a change to the logger, as we all know the types module already does. In addition any change in our logger would indirectly affect libbfpgo and may cause unintended behavior.

@rafaeldtinoco
Copy link
Contributor

  • There are other consumers of libbpfgo besides us. Tying them to our logger from tracee isn't very ok from our end IMO, and I believe it's likely they would set their own logger for libbpfgo if possible (an interface solution would enable that).

+1

  • This would practically introduce a circular dependency of tracee -> libbpfgo -> tracee logger, where they are not circular only technically by the go.mod. The logger we have in tracee is very much intended for tracee's own particular uses (the log aggregation for example).

+1

  • Having another go.mod will make development more burdensome whenever we do a change to the logger, as we all know the types module already does. In addition any change in our logger would indirectly affect libbfpgo and may cause unintended behavior.

+1

@rafaeldtinoco rafaeldtinoco dismissed their stale review January 18, 2023 14:13

Per Nadav's comments.

@geyslan
Copy link
Member Author

geyslan commented Jan 18, 2023

As @NDStrahilevitz illuminatingly explained and I agreed, closing this.

@geyslan geyslan closed this Jan 18, 2023
@geyslan geyslan deleted the 2599-2417-logger-standalone branch May 29, 2023 22:19
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

3 participants