Use Aya in the userspace #142

vadorovsky · 2022-01-17T00:10:14Z

This change replaces libbpf-rs with Aya as a loader of eBPF programs in
the userspace part in lockc.

eBPF programs still remain written in C and are going to be rewritten in
Rust in separate changes.

Another change is change of logging library to tracing.

Fixes: #135
Fixes: #106
Signed-off-by: Michal Rostecki mrostecki@opensuse.org

vadorovsky · 2022-01-17T00:13:33Z

I would say it's ready for review, since the code on lockc side is most likely not going to radically change.

Why I keep it as a draft? The problem is that we discovered some bugs in Aya, which has some troubles with loading lockc eBPF programs. For now we are using partial fixes from Dave Tucker's branch, not upstreamed yet:

https://github.com/dave-tucker/aya/tree/lockc

We are also discussing those problems every day on Aya Discord server (https://discord.gg/xHW2cb2N6G)

vadorovsky · 2022-01-18T23:29:18Z

It fully works and is ready for review! 🎉

So far it uses the following fork and branch of Aya:
https://github.com/dave-tucker/aya/tree/lockc

But all those fixes should land in the main branch soon.

flavio

I cannot comment about the BPF-related parts. As for the others, I left some comments.

Sorry for being pedantic about structured logging, I think it's super important to have it and it's something that must be enforced always while writing code

lockc/src/main.rs

flavio · 2022-01-20T08:56:59Z

lockc/src/main.rs

+    let log_level = match env::var("LOCKC_DEBUG") {
+        Ok(_) => LevelFilter::Debug,
+        Err(_) => LevelFilter::Info,
+    };


I think it would be nice to handle all of that via clap, which allows both cli and env variables to be set. Plus it does many kind of checks and generates a nice help output.

This is something I would not do as part of this PR, but I would file an issue and work on that later on if I were you

flavio · 2022-01-20T08:58:38Z

lockc/src/main.rs

+    TermLogger::init(
+        LevelFilter::Debug,
+        ConfigBuilder::new()
+            .set_target_level(log_level)
+            .set_location_level(log_level)
+            .build(),
+        TerminalMode::Mixed,
+        ColorChoice::Auto,
+    )?;


Another issue that could be filed for future work: it's pretty easy to use a JSON formatter that prints logs in a "machine-friendly" way.

I would offer the user a choice between two console output format: pretty/human and json. If you look into kubewarden/policy-server you can find the code to deal with that :)

flavio · 2022-01-20T09:09:52Z

lockc/src/runc.rs

+            .build()?
+            .block_on(self.add_container(container_id, pid, policy_level))?;
+
+        Ok(())


lockc/src/runc.rs

lockc/src/sysutils.rs

lockc/src/utils.rs

flavio · 2022-01-22T06:51:43Z

lockc/src/bpfstructs.rs

+        let mut id_b = CString::new(id)?.into_bytes_with_nul();
+        id_b.resize(CONTAINER_ID_LIMIT as usize, 0);
+        Ok(container_id {
+            id: id_b.try_into().unwrap(),


You should not unwrap here, rather bubble up the error. This function already returns a Result

The main reason why I did unwrap here is that I didn't find any way to successfully convert TryInto errors into anything which can be listed in thiserror::Error enums. But it seems that the cleanest way to handle that will be converting TryInto error into some custom error with map_err. At least that's how this StackOverflow topic concludes:

https://stackoverflow.com/questions/66508331/cast-error-from-try-into-to-custom-error

Yes, this is what I would do. Eventually define a new error type with thiserror and then rely onmap_err to do the conversion

lockc/src/main.rs

lockc/src/maps.rs

lockc/src/runc.rs

vadorovsky · 2022-01-24T17:19:19Z

@flavio PTAL

To sum it up:

I changed the logging library to tracing, handled all levels and json/text formatter as clippy options
made log messages structured

What I didn't to was removal of Ok(()) lines. In the most of cases, changing foo()? to foo() as the last line of a method/function, the underlying error doesn't get converted to a thiserror::Error enum instance. To make it work, I would rather have to convert foo()? into foo().map_err(|e| MyErrorEnum::from(e))?. That seems uglier for me than just using foo()?; and Ok(()).

Since we are dropping the usage of libbpf-rs, we are going to keep only C source files (to be built directly with Clang). Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

Follow up changes are goinng to handle container add/delete operations in the userspace. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

We are going to dron the usage of libbpf-rs at all. The first step is to remove libbpf-cargo dependency from our build. This change replaces it with direct usage of clang and libbpf headers copied from libbpf-sys. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

BTF maps defined in SEC(".maps") are still bonkers in Aya. Let's switch to SEC("maps/...") temporarily. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

flavio · 2022-01-26T14:56:02Z

lockc/src/runc.rs

+            bundle = bundle_path.display().to_string().as_str(),
+            config = config_path.display().to_string().as_str(),


tracing handles that in a nice way, there's no need to call display()...:

debug!( bundle = %bundle_path, config = ?config_path, "bundle uses display, while config uses debug");

This is documented here

flavio

It looks good to me. There are some minor improvements that can be done, see the latest comment I left

This change replaces libbpf-rs with Aya as a loader of eBPF programs in the userspace part in lockc. eBPF programs still remain written in C and are going to be rewritten in Rust in separate changes. Another change is change of logging library to tracing. Fixes: lockc-project#135 Fixes: lockc-project#106 Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

In pull request lockc-project#142 we made a mistake of converting lockc into full binary crate, therefore renaming lockcd to lockc and removing a possibility of creating multiple binaries (if there will be a need - i.e. for some CLI tool). This change fixes that by creating a bin/ directory with binaries again. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

vadorovsky changed the title ~~Use Aya in the userspac~~ Use Aya in the userspace Jan 17, 2022

vadorovsky requested review from mjura, willfindlay and flavio January 17, 2022 00:16

vadorovsky force-pushed the aya-userspace branch 4 times, most recently from 17cc60f to ed35f8f Compare January 18, 2022 23:23

vadorovsky marked this pull request as ready for review January 18, 2022 23:29

vadorovsky force-pushed the aya-userspace branch 3 times, most recently from 8eade52 to c50679d Compare January 19, 2022 17:33

flavio requested changes Jan 20, 2022

View reviewed changes

vadorovsky force-pushed the aya-userspace branch 2 times, most recently from 666f986 to 025fae4 Compare January 21, 2022 16:19

flavio reviewed Jan 22, 2022

View reviewed changes

vadorovsky force-pushed the aya-userspace branch 6 times, most recently from 4c46033 to 7b8e79f Compare January 24, 2022 17:15

vadorovsky force-pushed the aya-userspace branch 3 times, most recently from 92f3472 to eac6ed3 Compare January 25, 2022 15:16

vadorovsky added 2 commits January 25, 2022 17:33

bpf: Stop using bpf/ dir as a Rust module

526d3a1

Since we are dropping the usage of libbpf-rs, we are going to keep only C source files (to be built directly with Clang). Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

bpf: Remove uprobe programs

ed0ccb1

Follow up changes are goinng to handle container add/delete operations in the userspace. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

vadorovsky added 4 commits January 25, 2022 17:33

bpf: Use BPF_PROG macro in sched_process_fork program

b1111d0

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

bpf: Rename map sections

ad5dd74

BTF maps defined in SEC(".maps") are still bonkers in Aya. Let's switch to SEC("maps/...") temporarily. Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

bpf: Stop hashing strings, use container ID as string

2e4b839

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>

vadorovsky force-pushed the aya-userspace branch 2 times, most recently from 2ce4a93 to 204d92a Compare January 26, 2022 14:02

flavio reviewed Jan 26, 2022

View reviewed changes

flavio approved these changes Jan 26, 2022

View reviewed changes

vadorovsky force-pushed the aya-userspace branch from 204d92a to aaa170a Compare January 26, 2022 15:39

vadorovsky force-pushed the aya-userspace branch from aaa170a to 87c4995 Compare January 26, 2022 16:04

vadorovsky merged commit c82d824 into lockc-project:main Jan 26, 2022

vadorovsky deleted the aya-userspace branch January 26, 2022 16:33

vadorovsky mentioned this pull request Jan 26, 2022

Ensure that the binary is named lockcd #160

Merged

vadorovsky mentioned this pull request Feb 9, 2022

Fix lockc on Kubernetes #168

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Aya in the userspace #142

Use Aya in the userspace #142

vadorovsky commented Jan 17, 2022 •

edited

Loading

vadorovsky commented Jan 17, 2022 •

edited

Loading

vadorovsky commented Jan 18, 2022

flavio left a comment

flavio Jan 20, 2022

flavio Jan 20, 2022

flavio Jan 20, 2022

flavio Jan 22, 2022

vadorovsky Jan 24, 2022

flavio Jan 26, 2022

vadorovsky commented Jan 24, 2022

flavio Jan 26, 2022

vadorovsky Jan 26, 2022

flavio left a comment

		bundle = bundle_path.display().to_string().as_str(),
		config = config_path.display().to_string().as_str(),

Use Aya in the userspace #142

Use Aya in the userspace #142

Conversation

vadorovsky commented Jan 17, 2022 • edited Loading

vadorovsky commented Jan 17, 2022 • edited Loading

vadorovsky commented Jan 18, 2022

flavio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadorovsky commented Jan 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flavio left a comment

Choose a reason for hiding this comment

vadorovsky commented Jan 17, 2022 •

edited

Loading

vadorovsky commented Jan 17, 2022 •

edited

Loading