image build: add wasm #2127

alban · 2023-10-10T11:43:21Z

image build: add wasm

This PR adds the possibility to add a wasm module in a gadget image.

There are no changes in the CLI flags. This can be used by adding an entry in build.yaml:

wasm: program.go

The following file types are supported:

*.wasm: pre-compiled wasm module
*.go: Inspektor Gadget will compile it with tinygo

The wasm module is not executed. This will be for the next PRs.

How to use

docker build -f Dockerfiles/ebpf-builder.Dockerfile -t ghcr.io/inspektor-gadget/ebpf-builder:alban_wasm2 .
sudo -E ig image build --builder-image=ghcr.io/inspektor-gadget/ebpf-builder:alban_wasm2 -t ghcr.io/inspektor-gadget/gadget/trace_dns:test gadgets/trace_dns/
sudo -E ig run ghcr.io/inspektor-gadget/gadget/trace_dns:test

Testing done

As above.

eiffel-fl

This looks good but I find the integration complicated and I would like to avoid having extra-complexity added each time we want to support something new.
I will test it later.

Dockerfiles/ebpf-builder.Dockerfile

eiffel-fl · 2023-10-11T07:55:42Z

cmd/common/image/Makefile.build

+.PHONY: wasm
+ifeq ($(WASM),)
+wasm:
+	@echo "No wasm file found. Nothing to do."
+else ifeq (go,$(patsubst %.go,go,$(WASM)))
+wasm: $(WASM)
+	tinygo build -o $(OUTPUTDIR)/program.wasm -target=wasi --no-debug $^
+else ifeq (wasm,$(patsubst %.wasm,wasm,$(WASM)))
+wasm: $(WASM)
+	cp $^ $(OUTPUTDIR)/program.wasm
+else
+wasm:
+	@echo "Supported wasm file types: .go, .wasm"
+	@echo "Unsupported file type: $(notdir $(WASM))."
+	@exit 1
+endif


I think this complicated.
First, if there is no wasm, nothing should be printed, ls does not provide "nothing in this directory" is the directory is empty.
Cannot we achieve almost the same with this kind of idea:

%.go: tinygo build -o $(OUTPUTDIR)/program.wasm -target=wasi --no-debug $@ %.wasm: cp $@ $(OUTPUTDIR)/program.wasm

Ok, I will try that way and add the environment variable $(WASM) in the all target.

I do not even see why you need the environment variable.
You do not use an environment variable to indicate path to C files in conventional Makefile.
So, if there are *.go files in the current directory, they should be compiled by tinygo and *.wasm file should be moved to correct destination.

Actually your suggestion does not work because the target name (left of :) should be $(OUTPUTDIR)/program.wasm for both. And the %.go file is the dependency (right of :).

$(OUTPUTDIR)/program.wasm: program.go tinygo build -o $(OUTPUTDIR)/program.wasm -target=wasi --no-debug $@ $(OUTPUTDIR)/program.wasm: program.wasm cp $@ $(OUTPUTDIR)/program.wasm

And Makefile is unable to select the correct target depending on the available file (warning: overriding recipe for target, warning: ignoring old recipe for target).

Alternatively, I could write the if/else/then checks in shell inside an unique target, but I don't see the benefit. It would make the output more complicated to understand in case of error.

What about:

%.wasm: %.go tinygo build -o $@ -target=wasi --no-debug $< program.wasm: %.wasm cp $< $(OUTPUTDIR)/program.wasm

Basically, you have one target to compile from golang to wasm and another to move the file in the correct place.
More generically, why should it be named program.go and program.wasm? Cannot we be name agnostic?

That could work if the source files (in /work) and the generated files (in /out) were in the same directory, but it is not the case. And /work is mounted read-only in the container, so your Makefile won't work.

This is not a 2-step process to build program.wasm. Only one Makefile rule is executed. Either we build from go or we pick up the wasm file provided by the user.

Users can name the source files program.go and program.wasm as they want, I only mandate the extension. Only the generated files is deterministic so image/build.go knows what to copy after the build. But I don't see how the choice of the names help.

OK, let's keep what we have but this is really complicated.
In any case, you should remove this:

@echo "No wasm file found. Nothing to do."

Ok, I removed the echo. I need to keep the rule to distinguish with the error case when there is an invalid file type.

I'm wondering if we can use a different approach by moving more logic to build.go:

Define a explicit wasm target, not part of all

.PHONY: wasm wasm: $(WASM) tinygo build -o $(OUTPUTDIR)/program.wasm -target=wasi --no-debug $^

Add logic to cmd/common/image/build.go to only invoke this if the wasm file is golang and needs to be compiled, otherwise it can just be copied by build.go. (It seems a bit weird to me to use the makefile just to copy a file)

It could be also possible to have wasm part of all, by using this https://stackoverflow.com/a/23964298, but I'm not totally sure if that works fine.

Define a explicit wasm target, not part of all

I think that would generate more lines of code: build.go has two code paths to call the Makefile: buildLocal and buildInContainer. So we would need to decide the target (all or all wasm) in both cases.

Add logic to cmd/common/image/build.go to only invoke this if the wasm file is golang and needs to be compiled, otherwise it can just be copied by build.go. (It seems a bit weird to me to use the makefile just to copy a file)

I changed the logic to avoid the copy in the Makefile.

eiffel-fl · 2023-10-11T07:57:36Z

cmd/common/image/build.go

 		"make", "-f", makefilePath,
 		"-j", fmt.Sprintf("%d", runtime.NumCPU()),
 		"EBPFSOURCE="+conf.EBPFSource,
+		"WASM="+conf.Wasm,
 		"OUTPUTDIR="+output,
 		"CFLAGS="+conf.CFlags,


This whole build command is becoming more and more complicated.
Adding the fact we use it twice, and then need to update it twice, I would not be surprised if one day it explodes in flight.
Not for this PR, but we should add an helper function to craft it.

eiffel-fl · 2023-10-11T07:58:53Z

gadgets/trace_dns/build.yaml

@@ -0,0 +1 @@
+wasm: program.go


I am doubtful regarding this commit.
This indeed proves we can use WASM, but wouldn't a toy example of doing something would help more? We could use it as sort of documentation for people wanting to use WASM.

I wanted to avoid executing Wasm in this PR because I wanted to do the refactor from #2131 first and implement the Wasm execution after the refactoring.

What do you prefer:

I could remove this commit from this first PR and keep it for the next PR

Make a single PR with both the "image build" Wasm feature and the Wasm execution feature

I wanted to avoid executing Wasm in this PR because I wanted to do the refactor from #2131

This makes sense, just keep the commit as is and we will extend it once this issue is closed.

eiffel-fl

I tested it and it works fine:

francis@pwmachine:~/Codes/kinvolk/inspektor-gadget$ sudo -E ./ig image build --builder-image=ghcr.io/inspektor-gadget/ebpf-builder:alban_wasm2 -t ghcr.io/inspektor-gadget/gadget/trace_dns:test gadgets/trace_dns/
INFO[0000] Experimental features enabled                
Successfully built ghcr.io/inspektor-gadget/gadget/trace_dns:test@sha256:2216db6fc45ea5d5079e24da4ee14acddc6a9ca35d5adf2d37ca5041b5c023c3
francis@pwmachine:~/Codes/kinvolk/inspektor-gadget$ sudo -E ig run ghcr.io/inspektor-gadget/gadget/trace_dns:test       alban_wasm2 % u=

sudo: ig : commande introuvable
francis@pwmachine:~/Codes/kinvolk/inspektor-gadget$ sudo -E ./ig run ghcr.io/inspektor-gadget/gadget/trace_dns:test     alban_wasm2 % u=

INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME  SRC                      PID     DST                      UID      GID      NAME                   QR PKT_T… RCODE
wizardly_hawking       10.10.0.3:36798          28237   80.67.169.12:53          100      65534    debdebianorg        0  4      0    
wizardly_hawking       80.67.169.12:53          28237   10.10.0.3:36798          100      65534    debdebianorg        1  0      0

But we need to make the modifications to the image future proof regarding arm64 and I want this to be addressed in this PR.

eiffel-fl · 2023-10-13T07:52:41Z

Dockerfiles/ebpf-builder.Dockerfile

+RUN \
+	wget --quiet https://github.com/tinygo-org/tinygo/releases/download/v0.30.0/tinygo_0.30.0_amd64.deb && \
+	echo abcef56b2ae04e27253df409b32d0b7abb5ae76ed493db75b2f80659cbf59363c85919836116780adcad051c652e991dc46fa6ae7cec31a4fa3bdb68d2123621 tinygo_0.30.0_amd64.deb | sha512sum -c && \
+	dpkg -i tinygo_0.30.0_amd64.deb


Yet another architecture problem:

Suggested change

RUN \

wget --quiet https://github.com/tinygo-org/tinygo/releases/download/v0.30.0/tinygo_0.30.0_amd64.deb && \

echo abcef56b2ae04e27253df409b32d0b7abb5ae76ed493db75b2f80659cbf59363c85919836116780adcad051c652e991dc46fa6ae7cec31a4fa3bdb68d2123621 tinygo_0.30.0_amd64.deb | sha512sum -c && \

dpkg -i tinygo_0.30.0_amd64.deb

DEB=tinygo.deb && \

ARCH=$(dpkg --print-architecture) && \

wget --quiet https://github.com/tinygo-org/tinygo/releases/download/v0.30.0/tinygo_0.30.0_${ARCH}.deb -O $DEB && \

if [ "${ARCH}" == 'amd64' ]; then \

SHA='abcef56b2ae04e27253df409b32d0b7abb5ae76ed493db75b2f80659cbf59363c85919836116780adcad051c652e991dc46fa6ae7cec31a4fa3bdb68d2123621'; \

elif [ "${ARCH}" == 'arm64' ]; then \

SHA='please_compute_it'; \

else \

echo "${ARCH} is not supported" > stderr; \

exit 1; \

fi \

echo $SHA $DEB | sha512sum -c && \

dpkg -i $DEB

Thanks, I picked your changes and modified the following:

sha string

test == does not work in the default shell

add missing &&

eiffel-fl

It looks nice but I first would like to have #2140 merged before, this way we are all set regarding architecture.

alban · 2023-10-17T17:08:37Z

Rebased after #2140.

eiffel-fl

Thank you for this PR! I am curious about what we will do with wasm!

eiffel-fl · 2023-10-18T07:03:21Z

Dockerfiles/ebpf-builder.Dockerfile

+	elif [ "${ARCH}" = 'arm64' ] ; then \
+		SHA='d121940aa1cd366c865f1600c88decf2a9db6891234f738024702891ecff94958848ce6bb3191b3a34250ab7091bc3e35b27cc612d7324f48a4d148869fa306f'; \
+	else \
+		echo "${ARCH} is not supported" > stderr; \


My previous suggestion was semantic because this bash syntax to write to stderr is far from being intuitive:

Suggested change

echo "${ARCH} is not supported" > stderr; \

>&2 echo "${ARCH} is not supported"; \

eiffel-fl · 2023-10-18T07:04:08Z

cmd/common/image/Makefile.build

+	@echo "Supported wasm file types: .go, .wasm"
+	@echo "Unsupported file type: $(notdir $(WASM))."


Same, I would also redirect them to stderr, maybe Makefile provides something to do so.

eiffel-fl · 2023-10-18T07:05:14Z

cmd/common/image/build.go

+	// Only include wasm program if it has been built
+	if _, err := os.Stat(buildOpts.WasmObjectPath); err != nil {
+		buildOpts.WasmObjectPath = ""
 	}


I would rather do the opposite, i.e. leave it empty in the struct definition and it it exists and there is no error set it.

Dockerfiles/ebpf-builder.Dockerfile

cmd/common/image/build.go

mauriciovasquezbernal · 2023-10-19T14:34:18Z

cmd/common/image/Makefile.build

+.PHONY: wasm
+ifeq ($(WASM),)
+wasm:
+	@echo "No wasm file found. Nothing to do."
+else ifeq (go,$(patsubst %.go,go,$(WASM)))
+wasm: $(WASM)
+	tinygo build -o $(OUTPUTDIR)/program.wasm -target=wasi --no-debug $^
+else ifeq (wasm,$(patsubst %.wasm,wasm,$(WASM)))
+wasm: $(WASM)
+	cp $^ $(OUTPUTDIR)/program.wasm
+else
+wasm:
+	@echo "Supported wasm file types: .go, .wasm"
+	@echo "Unsupported file type: $(notdir $(WASM))."
+	@exit 1
+endif


I'm wondering if we can use a different approach by moving more logic to build.go:

Define a explicit wasm target, not part of all

.PHONY: wasm wasm: $(WASM) tinygo build -o $(OUTPUTDIR)/program.wasm -target=wasi --no-debug $^

Add logic to cmd/common/image/build.go to only invoke this if the wasm file is golang and needs to be compiled, otherwise it can just be copied by build.go. (It seems a bit weird to me to use the makefile just to copy a file)

It could be also possible to have wasm part of all, by using this https://stackoverflow.com/a/23964298, but I'm not totally sure if that works fine.

alban · 2023-10-19T14:41:53Z

pkg/oci/build.go

@@ -208,6 +210,13 @@ func createImageIndex(ctx context.Context, target oras.Target, o *BuildGadgetIma
 		}
 		layers = append(layers, manifestDesc)
 	}
+	if o.WasmObjectPath != "" {
+		manifestDesc, err := createManifestForTarget(ctx, target, o.MetadataPath, o.WasmObjectPath, "wasm")


One issue to fix:

createManifestForTarget uses the following ebpf mediatype instead of a wasm mediatype:

eBPFObjectMediaType = "application/vnd.gadget.ebpf.program.v1+binary"

So the manifest looks like:

{ "schemaVersion": 2, "config": { "mediaType": "application/vnd.gadget.config.v1+yaml", "digest": "sha256:56077f72620cce86e2c59aa3b0f0fb77764ca9e5d87c2a1cc7ac01459533934c", "size": 2057, "annotations": { "org.opencontainers.image.title": "config.yaml" } }, "layers": [ { "mediaType": "application/vnd.gadget.ebpf.program.v1+binary", "digest": "sha256:cc75bbd53264752cdad8ea56b004a71efdad52f7014bbf1c1f651877ef3a0661", "size": 19944, "annotations": { "org.opencontainers.image.authors": "TODO: authors", "org.opencontainers.image.description": "TODO: description", "org.opencontainers.image.title": "program.o" } } ] }

I can file a follow-up issue for this.

What about modifying the type depending on the target?

Or adding abstraction function:

createManifestForeBPFTarget(...) { progDesc, err := createeBPFProgram() return createManifestForTarget(progDesc, ...) } createManifestForWasmTarget(...) { progDesc, err := createWasmProgram() return createManifestForTarget(progDesc, ...) } createManifestForTarget(progDesc prog.Descriptor, ...) { // ... }

What about modifying the type depending on the target?

Thanks! I used that solution for simplicity.

Tested with: docker build -f Dockerfiles/ebpf-builder.Dockerfile -t ghcr.io/inspektor-gadget/ebpf-builder:alban_wasm2 . sudo -E ig image build --builder-image=ghcr.io/inspektor-gadget/ebpf-builder:alban_wasm2 -t ghcr.io/inspektor-gadget/gadget/trace_dns:test gadgets/trace_dns/ sudo -E ig run ghcr.io/inspektor-gadget/gadget/trace_dns:test Signed-off-by: Alban Crequy <albancrequy@linux.microsoft.com>

The wasm module is written in Go but does not do anything yet. Signed-off-by: Alban Crequy <albancrequy@linux.microsoft.com>

alban requested review from mauriciovasquezbernal, eiffel-fl, mqasimsarfraz and flyth as code owners October 10, 2023 11:43

eiffel-fl reviewed Oct 11, 2023

View reviewed changes

eiffel-fl reviewed Oct 13, 2023

View reviewed changes

alban force-pushed the alban_wasm2 branch 2 times, most recently from afc0752 to b94cf41 Compare October 16, 2023 16:21

eiffel-fl reviewed Oct 17, 2023

View reviewed changes

alban force-pushed the alban_wasm2 branch from b94cf41 to 4376ab3 Compare October 17, 2023 17:07

eiffel-fl approved these changes Oct 18, 2023

View reviewed changes

alban force-pushed the alban_wasm2 branch 2 times, most recently from 03b5bd9 to b35b4c3 Compare October 19, 2023 14:36

mauriciovasquezbernal reviewed Oct 19, 2023

View reviewed changes

alban commented Oct 19, 2023

View reviewed changes

alban force-pushed the alban_wasm2 branch 2 times, most recently from b291e8f to 494539f Compare October 23, 2023 19:02

alban added 2 commits October 24, 2023 14:23

gadgets/trace_dns: add empty wasm module

40b707f

The wasm module is written in Go but does not do anything yet. Signed-off-by: Alban Crequy <albancrequy@linux.microsoft.com>

alban force-pushed the alban_wasm2 branch from 494539f to 40b707f Compare October 24, 2023 13:24

alban merged commit 9537d19 into main Oct 24, 2023
50 checks passed

alban deleted the alban_wasm2 branch October 24, 2023 15:12

alban mentioned this pull request Nov 6, 2023

[RFE] handling events in userspace with wasm #2202

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image build: add wasm #2127

image build: add wasm #2127

alban commented Oct 10, 2023

eiffel-fl left a comment

eiffel-fl Oct 11, 2023

alban Oct 11, 2023

eiffel-fl Oct 11, 2023

alban Oct 11, 2023

eiffel-fl Oct 11, 2023

alban Oct 11, 2023

eiffel-fl Oct 12, 2023 •

edited

alban Oct 16, 2023

mauriciovasquezbernal Oct 19, 2023

alban Oct 23, 2023

eiffel-fl Oct 11, 2023

eiffel-fl Oct 11, 2023

alban Oct 11, 2023

eiffel-fl Oct 11, 2023 •

edited

eiffel-fl left a comment

eiffel-fl Oct 13, 2023

alban Oct 16, 2023

eiffel-fl left a comment

alban commented Oct 17, 2023

eiffel-fl left a comment

eiffel-fl Oct 18, 2023

eiffel-fl Oct 18, 2023

eiffel-fl Oct 18, 2023

mauriciovasquezbernal Oct 19, 2023

alban Oct 19, 2023

alban Oct 23, 2023

eiffel-fl Oct 24, 2023

eiffel-fl Oct 24, 2023

alban Oct 24, 2023

-RUN \
-	wget --quiet https://github.com/tinygo-org/tinygo/releases/download/v0.30.0/tinygo_0.30.0_amd64.deb && \
-	echo abcef56b2ae04e27253df409b32d0b7abb5ae76ed493db75b2f80659cbf59363c85919836116780adcad051c652e991dc46fa6ae7cec31a4fa3bdb68d2123621 tinygo_0.30.0_amd64.deb | sha512sum -c && \
-	dpkg -i tinygo_0.30.0_amd64.deb
+	DEB=tinygo.deb && \
+	ARCH=$(dpkg --print-architecture) && \
+	wget --quiet https://github.com/tinygo-org/tinygo/releases/download/v0.30.0/tinygo_0.30.0_${ARCH}.deb -O $DEB && \
+	if [ "${ARCH}" == 'amd64' ]; then \
+		SHA='abcef56b2ae04e27253df409b32d0b7abb5ae76ed493db75b2f80659cbf59363c85919836116780adcad051c652e991dc46fa6ae7cec31a4fa3bdb68d2123621'; \
+	elif [ "${ARCH}" == 'arm64' ]; then \
+		SHA='please_compute_it'; \
+	else \
+		echo "${ARCH} is not supported" > stderr; \
+		exit 1; \
+	fi \
+	echo $SHA $DEB | sha512sum -c && \
+	dpkg -i $DEB

	echo "${ARCH} is not supported" > stderr; \
	>&2 echo "${ARCH} is not supported"; \

		@echo "Supported wasm file types: .go, .wasm"
		@echo "Unsupported file type: $(notdir $(WASM))."

image build: add wasm #2127

image build: add wasm #2127

Conversation

alban commented Oct 10, 2023

image build: add wasm

How to use

Testing done

eiffel-fl 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

eiffel-fl Oct 12, 2023 • edited

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

eiffel-fl Oct 11, 2023 • edited

Choose a reason for hiding this comment

eiffel-fl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiffel-fl left a comment

Choose a reason for hiding this comment

alban commented Oct 17, 2023

eiffel-fl 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiffel-fl Oct 12, 2023 •

edited

eiffel-fl Oct 11, 2023 •

edited