bpf: fix: Simplify and fix test structure validation logic #41139
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cf4d8c2 to
1012556
Compare
Simplify the logic a bit to get rid of the secondary regex match inside
checkProgExists and instead defer the existence check for
progs.checkProg until after the first pass. Just ensure uniqueness of
each setup/pktgen/check program along the way with checkProgUnique.
Before, the logic would not catch if a check is not defined for a test
until later when a panic occurs, since progs.checkProg would be nil, but
now existence of a CHECK program is enforced.
BEFORE
======
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x7db080]
goroutine 844 [running]:
testing.tRunner.func1.2({0x2a02a40, 0x4ef3880})
/usr/local/go/src/testing/testing.go:1734 +0x21c
testing.tRunner.func1()
/usr/local/go/src/testing/testing.go:1737 +0x35e
panic({0x2a02a40?, 0x4ef3880?})
/usr/local/go/src/runtime/panic.go:792 +0x132
github.com/cilium/ebpf.(*Program).Type(0x4e5f5a0?)
/go/src/github.com/cilium/cilium/vendor/github.com/cilium/ebpf/prog.go:573
github.com/cilium/cilium/bpf/tests/bpftest.loadAndRunSpec.subTest.func4(0xc0007028c0)
/go/src/github.com/cilium/cilium/bpf/tests/bpftest/bpf_test.go:334 +0x9a
testing.tRunner(0xc0007028c0, 0xc0008eb230)
/usr/local/go/src/testing/testing.go:1792 +0xf4
created by testing.(*T).Run in goroutine 973
/usr/local/go/src/testing/testing.go:1851 +0x413
FAIL github.com/cilium/cilium/bpf/tests/bpftest 8.257s
AFTER
=====
--- FAIL: TestBPF (20.63s)
--- FAIL: TestBPF/skip_tunnel_nodeport_nat.o (0.21s)
bpf_test.go:224: ... does not contain a check program for the ... test.
Don't rely on the program name at all, only the section names. Before,
weird edge cases could occur with strings.Replace() if the test program
name itself had the words "setup", "pktgen", or "check" in them. This
change uncovered the case with nat4_port_allocation where the test name
in the CHECK for the nat4_port_allocation_tcp and
nat4_port_allocation_udp tests was wrong, so fix this.
Signed-off-by: Jordan Rife <jrife@google.com>
1012556 to
737ac5a
Compare
|
/test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Originally I made this change as part of #40759, but I'm moving it to its own PR, since it seems like a nice standalone fix.
Simplify the logic a bit to get rid of the secondary regex match inside
checkProgExistsand instead defer the existence check forprogs.checkProguntil after the first pass. Just ensure uniqueness of each setup/pktgen/check program along the way withcheckProgUnique. Before, the logic would not catch if a check is not defined for a test until later when a panic occurs, sinceprogs.checkProgwould be nil, but now existence of aCHECKprogram is enforced.Before
If
CHECKwas not defined.After
If
CHECKwas not defined.Don't rely on the program name at all, only the section names. Before, weird edge cases could occur with
strings.Replace()if the test program name itself had the words "setup", "pktgen", or "check" in them. This change uncovered the case withnat4_port_allocationwhere the test name in theCHECKfor thenat4_port_allocation_tcpandnat4_port_allocation_udptests was wrong, so fix this.