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

Add example bytecode image spec + ability for bpfd to pull and extract bytecode from images #102

Merged
merged 5 commits into from
Aug 30, 2022

Conversation

astoycos
Copy link
Member

Starting work on #99

@astoycos astoycos force-pushed the bytecode-image branch 2 times, most recently from d28f3b0 to 50dc5aa Compare August 15, 2022 15:07
@astoycos astoycos marked this pull request as ready for review August 15, 2022 15:08
@astoycos
Copy link
Member Author

The first draft of this is functional, there are still some TODOs in the spec + implementation but I want to get some review before moving forward.

@astoycos astoycos force-pushed the bytecode-image branch 2 times, most recently from d4ab23c to fa41b45 Compare August 15, 2022 15:35

We provide two distinct spec variants here to ensure interoperatiblity with existing registries
and packages which do no support the new custom media types defined here.

Copy link

Choose a reason for hiding this comment

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

Can you add a bullet list with the names of the two spec variants here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, this is a rough draft eventually this may turn into 2 separate documents like solo did for wasm modules here

@astoycos astoycos changed the title WIP Add example bytecode image spec + ability for bpfd to pull and extract bytecode from images Add example bytecode image spec + ability for bpfd to pull and extract bytecode from images Aug 24, 2022
@astoycos astoycos force-pushed the bytecode-image branch 2 times, most recently from 23f91fd to 4b5fb08 Compare August 24, 2022 21:05
@astoycos
Copy link
Member Author

astoycos commented Aug 24, 2022

@dave-tucker This should be ready to go (At least for the quick and dirty version) there's still some todo's and I'd feel better with a few unit tests but for the e2e K8s demo this is a must

LMK what you think specifically about 9f6e937 and 4b5fb08

Copy link
Collaborator

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

A few nits around error handling but overall this LGTM 👍
Needs a rebase also

use serde_json::Value;
use tar::Archive;

const CONTAINERIZED_BYTECODE_PATH: &str = "/etc/bpfd/bytecode/";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be in /var

Copy link
Member Author

Choose a reason for hiding this comment

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

So this might break @Billy99's work... If we're not running with sudo will we have the ability to create/ put files in /var/bpfd/bytecode?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had code changes pending on my branch to change the user from bpfctl to bpfd-clients. I can change the directory with those changes and make sure it has the correct owner.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

See #118
FYI: I change the name if needed.

.expect("Not a valid bytecode image reference");

let protocol = client::ClientProtocol::Https;
// TODO(astoycos): Add option/flag to authenticate against private image repositories
Copy link
Collaborator

Choose a reason for hiding this comment

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

That should live in bpfd.toml we can have something like:

[ registry ]

[ registry.docker-io ]
username = foo
password = bar

We can then infer the registry from the image path and login if required.
Can be added later so lets add an issue to track it,

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good to me I will make an issue to track this

debug! {"Pulling bytecode from image path: {}", image_url}
let image: Reference = image_url
.parse()
.expect("Not a valid bytecode image reference");
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should explicitly handle this error (and other errors here) vs. using expect which will panic.
i.e. .map_err(ImageError::InvalidImageUrl)?.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 let me whip up some custom error types :)

Add some documentation around delivering bytecode to
bpfd via container images.

Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
@astoycos
Copy link
Member Author

Done this should be ready to go barring CI

Andrew Stoycos added 4 commits August 30, 2022 10:41
Add a new flag to bpfctl titled `--from-image`
which will signal that the provided <PATH> is
actually a valid container image URL

Update bpfd to be able to pull and extract EBPF
bytecode from a container image if properly
configured.

Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
This allows us to not have to worry about installing
openSSL on different underlying distros

However for use with fedora we still need to install
perl :/

see -> openssl/openssl#13761

Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
Add option to only compile the bpf dispatcher
written in C using clang.

Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
Update dockerfile to only build C ebpf dispacher program
for now, which speeds up build time.

Move from the nigthly rust image to rust:1.63.0-bullseye.

Signed-off-by: Andrew Stoycos <astoycos@redhat.com>
@dave-tucker
Copy link
Collaborator

rustfmt me please 😆

@astoycos
Copy link
Member Author

I always forget 🥇

@astoycos astoycos merged commit 8d837c1 into bpfman:main Aug 30, 2022
@astoycos astoycos deleted the bytecode-image branch April 27, 2023 20: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.

4 participants