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

chore: added verifier tests #3433

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AshishNaware
Copy link
Contributor

@AshishNaware AshishNaware commented Feb 24, 2025

Fixes
#229 replace verifier.sh with a go verifier test that uses cilium/ebpf

Description

This change adds go tests using cilium/ebpf library as an alternative to verifier script. The test closely mimics all the actions of verifier.sh. -d flag produces more verbose output than the earlier script.

Changelog

release-note

make verify target now invokes go tests to test object files instead of running verifier script. Below is the sample usage

successful execution:

make verify
=== RUN   TestVerifyTetragonPrograms
--- PASS: TestVerifyTetragonPrograms (11.72s)
PASS
ok  	command-line-arguments	11.743s

Test fails for the invalid file with error message:

make verify
=== RUN   TestVerifyTetragonPrograms
    verify_test.go:95: 
        	Error Trace:	/home/ashish/Desktop/github.com/cilium/tetragon/contrib/verify/verify_test.go:95
        	Error:      	Received unexpected error:
        	            	file /home/ashish/Desktop/github.com/cilium/tetragon/bpf/objs/test.o: bad magic number '[66 47 67 62]' in record at byte 0x0
        	Test:       	TestVerifyTetragonPrograms
        	Messages:   	failed to parse elf file into collection spec
--- FAIL: TestVerifyTetragonPrograms (11.70s)
FAIL
FAIL	command-line-arguments	11.717s
FAIL
make: *** [Makefile:280: verify] Error 1

use debug flag to get verbose output

DEBUG=1 make verify (provides verbose output)
iter:
	  ; struct bpf_prog *prog = ctx->prog;
	 0: LdXMemDW dst: r2 src: r1 off: 8 imm: 0
	  ; if (!prog)
	 1: JEqImm dst: r2 off: 9 imm: 0
	  ; _(id = prog->aux->id);
	 2: LdXMemDW dst: r2 src: r2 off: 56 imm: 0
	 3: LdXMemW dst: r2 src: r2 off: 32 imm: 0
	 4: StXMemW dst: rfp src: r2 off: -4 imm: 0
	  ; seq_write(ctx->meta->seq, &id, sizeof(id));
	 5: LdXMemDW dst: r1 src: r1 off: 0 imm: 0
	  ; seq_write(ctx->meta->seq, &id, sizeof(id));
	 6: LdXMemDW dst: r1 src: r1 off: 0 imm: 0
	 7: MovReg dst: r2 src: rfp
	  ; _(id = prog->aux->id);
	 8: AddImm dst: r2 imm: -4
	  ; seq_write(ctx->meta->seq, &id, sizeof(id));
	 9: MovImm dst: r3 imm: 4
	10: Call FnSeqWrite
	  ; }
	11: MovImm dst: r0 imm: 0
	12: Exit

--- PASS: TestVerifyTetragonPrograms (13.01s)
PASS
ok  	command-line-arguments	13.028s

To provide specific object directory

TETRAGONDIR=<path to dir> make verify

Note:
unlike old script, this test does not provide below metrics
verification time,
stack depth,
processed insns,
max_states_per_insn,
total_states,
peak_states,
mark_read

@AshishNaware AshishNaware requested a review from a team as a code owner February 24, 2025 07:12
Copy link
Contributor

@olsajiri olsajiri left a comment

Choose a reason for hiding this comment

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

hi, looks great, left some comments, please add some info to the changelog, like details about the functionality (way we did it before and now) and difference in the new output, thanks

/^;/ { if (in_func) { print " " $0; in_func=0; } }
/^verification time/ { verify_lines=3 }
/^/ { if (verify_lines) { verify_lines -= 1; print " " $0 } }' < "$OUT"
rm "$OUT"
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure we could easily do this output, nor I'm sure we really need that.. I just don't see it in new code, so would be great to have some comment about that (like in changelog)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added comment in changelog

Copy link
Contributor

Choose a reason for hiding this comment

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

added comment in changelog

@AshishNaware aah I meant commit changelog (not PR) so we have the info in the git log, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. 👍

@AshishNaware AshishNaware force-pushed the go_verifier_test branch 2 times, most recently from 9c77db3 to eba1d72 Compare February 26, 2025 06:59
'make verify' target now runs go tests to test object files instead of running verifier script.

* invoke make verify to run the tests.
* run tests in debug mode: DEBUG=1 make verify
* run tests on specific directory with .o files: TETRAGONDIR=<path_to_dir> make verify
* Note:
	unlike old script, these tests do not provide these metrics: verification time,
	stack depth, processed insns, max_state_per_insn, total_states, peak_states,mark_read

Signed-off-by: Ashish Naware <ashishnaware3@gmail.com>
Copy link

netlify bot commented Feb 28, 2025

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 8f78a54
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/67c138d5bea5e60008c6cb2f
😎 Deploy Preview https://deploy-preview-3433--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@AshishNaware
Copy link
Contributor Author

please add a release-note/* label to the pull request

I am not able to add any labels to this PR

@mtardy mtardy added the release-note/ci This PR makes changes to the CI. label Feb 28, 2025
Copy link
Contributor

@olsajiri olsajiri left a comment

Choose a reason for hiding this comment

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

left few more comments, but otherwise lgtm, thanks

@@ -277,7 +278,7 @@ bpf-test:

.PHONY: verify
verify: tetragon-bpf ## Verify BPF programs.
sudo contrib/verify/verify.sh bpf/objs
sudo DEBUG=${DEBUG} TETRAGONDIR=${TETRAGONDIR} $(shell which go) test contrib/verify/verify_test.go -v
Copy link
Contributor

Choose a reason for hiding this comment

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

please use $(GO)

@@ -19,6 +19,7 @@ GO_TEST_TIMEOUT ?= 20m
E2E_TEST_TIMEOUT ?= 20m
BUILD_PKG_DIR ?= $(shell pwd)/build/$(TARGET_ARCH)
VERSION ?= $(shell git describe --tags --always --exclude '*/*')
TETRAGONDIR ?= $(CURDIR)/bpf/objs
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for TETRAGONDIR variable, please use $(CURDIR)/bpf/objs directly

require.NoError(t, err, "failed to load resources into the kernel")

collection.Close()

Copy link
Contributor

Choose a reason for hiding this comment

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

extra line

collection.Close()

}

Copy link
Contributor

Choose a reason for hiding this comment

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

extra line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants