ci: detect outbound internet traffic generated while running tests#31349
ci: detect outbound internet traffic generated while running tests#31349vasild wants to merge 1 commit into
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31349. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsNo conflicts as of last run. |
54a6482 to
3ec89f4
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
Nice. Conecpt ACK! |
|
Concept ACK i'm slightly worried this may generate false positive. As is, this detects traffic on the entire (virtual) machine while running the tests. Are there no other daemons running on the CI instance that could interfere with this? |
3ec89f4 to
3c4b203
Compare
|
@laanwj, Right! And Another source of false positive could be if somebody from the outside initiates communication to the VM to which it responds. E.g. an outsider tries to connect to the VM to which it responds with an outbound packet e.g. TCP RST. At least that should be obvious from the error log, showing the incoming packet first (I just pushed a slight change for that). Maybe also the traffic-from-another-daemon could be obvious - e.g. if there is traffic to |
3c4b203 to
67c6bce
Compare
67c6bce to
1592a7d
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Exactly. For all we know, the CI VM is firewalled off sufficiently that this can't happen, but we don't know.
Ah yes, as long as it's only some extra logging, having a manual factor in this is fine. It only becomes critical if network traffic would cause a CI failure. i'm not aware of a straightforward way to "log network traffic of this process and subproceses only". Yes, it could be done with a linux network namespace, but that's a lot of hassle.
Seeing this, it might already be namespaced. Though a process namespace doesn't necessarily mean the network namespace is isolated. |
1592a7d to
071e43f
Compare
|
My intention here is to fail the CI because otherwise the log will be buried in the CI output and nobody will notice it. It follows that if this fails randomly with false positives when one would have to investigate it manually for arbitrary PRs which is highly highly highly undesirable. |
|
Ran this on my CI runner which has 8.8.8.8 configured as DNS server for docker. https://cirrus-ci.com/task/5500763260059648?logs=ci#L1137 Edit: My understanding is as follows: The DNS requests normally go to a local DNS resolver which then asks an upstream resolver. The upstream resolver (possibly your ISP) indirectly learns that you are running Bitcoin Core tests, even if there was no direct communication over a non-loopback interface. Edit 2: full tcpdump output here: |
This comment was marked as abuse.
This comment was marked as abuse.
|
Concept ACK. Per https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-11-26#1069602: "it turns out the owners of 1.2.3.4, 11.22.33.44 and 8.8.8.8, if they would bother, would know the IP address of every dev who runs the functional tests at home." |
071e43f to
8799018
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
8799018 to
803ed46
Compare
|
About the false positives - I think it is worth trying this in its current mode where any detected traffic is assumed to have originated from the tests and fails the CI. If this ever fails the CI for another reason (false positive), then it would be easy to turn this into a "report in the logs only but don't fail" by removing the About the DNS traffic - I did |
|
Yea. Still failing for me with Podman (5.6.2) on my Fedora box. |
|
From the output, I think there may be several unit tests affected, and they should probably be fixed, even if the GHA CI does not catch this issue. Also, I tried on a fresh Ubuntu VM with a fresh user account (not root) and the issue persists. So I think the issue generally uncovers via podman. |
|
Is the local failure you observe reproducible or sporadic? If it is reproducible maybe you can nail down which test is making the traffic? Previously to find the offending test I bisected the list of tests (used to nail down the traffic from BUILD/bin/test_bitcoin $(BUILD/bin/test_bitcoin --list_content 2>&1 |(IFS="*" ; while read line ; do line_trim="$(sed 's/^[[:space:]]*//' <<<$line)" ; if [ "$line" = "$line_trim" ] ; then s="$line" ; continue ; fi ; echo "--run_test=$s/$line_trim" ; done |sed -n '1,340p'))This can be substituted for: bitcoin/ci/test/03_test_script.sh Lines 178 to 181 in 1af46cf |
Maybe 50%, see my previous comment: #31349 (comment). I guess this makes bisect a bit harder, but it should be possible by running 10 times, or so. |
|
Still fails for me running the CI (via Podman 5.7.1) on my Fedora box (this PR rebased on master): ALL | ✓ Passed | 6980 s (accumulated)
Runtime: 1243 s
+ traffic_monitor_end functional
+ test_name=functional
++ get_interfaces
++ set -o pipefail
++ ifconfig
++ awk -F ':| ' '/^[^[:space:]]/ { if (!match($1, /^lo/)) { print $1 } }'
++ set +o pipefail
+ for ifname in $(get_interfaces)
++ tcpdump_file functional eth0
++ local test_name=functional
++ local interface_name=eth0
++ echo /tmp/tcpdump_functional_eth0
+ f=/tmp/tcpdump_functional_eth0
+ '[' '!' -e /tmp/tcpdump_functional_eth0 ']'
+ chown root:root /tmp/tcpdump_functional_eth0
++ tcpdump -n -r /tmp/tcpdump_functional_eth0 --direction=out tcp or udp
reading from file /tmp/tcpdump_functional_eth0, link-type EN10MB (Ethernet), snapshot length 262144
<snip>
+ echo 'Error: outbound TCP or UDP packets on the non loopback interface generated during functional tests:'
Error: outbound TCP or UDP packets on the non loopback interface generated during functional tests:
+ tcpdump -n -r /tmp/tcpdump_functional_eth0 tcp or udp
reading from file /tmp/tcpdump_functional_eth0, link-type EN10MB (Ethernet), snapshot length 262144
<snip>
+ exit 1
Command '['./ci/test/02_run_container.sh']' returned non-zero exit status 1.
real 43m11.110s
user 0m55.959s
sys 0m45.018s |
|
This is intended to work on CI and it does so well. Should I reintroduce INTERNET_TRAFFIC_EXPECTED to deal with local runs by making it possible to turn these reports into non-fatal errors? |
Being able to run the CI locally is fully supported and a required use case (there are CI jobs which are not run in the main repo). |
|
I tend to agree with @fanquake. Running the CI locally should be easy and supported. We don't want to end up in a place where the CI is basically just a prayer toward Microsoft/GHA to please run the scripts and to please run them correctly. I think the open questions are:
|
I think we don't use static linking in the CI, so |
|
|
|
I've already replied (see the several comments above). To summarize:
Generally, when it comes to CI failures, they should ideally be easily reproducible, and understandable, and actionable. |
|
Also, the CI fails here: |
Link? The idea is to make these checks suppress-able. Or even it can be made to run only in CI and not on local runs by default, if e.g. That would be better than the current situation. Right now it is not running on CI and is not running locally.
It is changing some docker parameters and is using
Those are failures when you ran it locally which I could not reproduce. CI is green and I have no idea what to do from here. I do not think that the current tests we have non-deterministically, sometimes, generate internet traffic and sometimes don't.
True. My thinking is that if it bricks the CI on some PR, then the author of the PR will have an idea which part of their changes are causing it. |
Maybe, if the changes are just adding an obvious remote call. Though, if there is a pre-existing issue in one of the background threads, and the failure is racy, and the pull request triggers this as a side-effect, it will be less clear.
I think suppressions make sense where they are understood to suppress a known false-positive, or a known don't-care scenario. Using a suppression to wholesale disable a feature locally in the presence of true-positives seems odd.
Your suggestion is to run "not on local runs", which I understand as "GitHub-only".
There is more than just myself who ran into the issue. Traffic to As I already mentioned, a solution to this problem would be to print a stacktrace when the call happens, and then abort the program/CI immediately. This would make any failure traceable, and thus hopefully fixable. |
|
How can I move this forward? It is stuck in some local unexplained "internet traffic detected" failures in local runs by @maflcko and @fanquake that I could not reproduce. My suggestion to add (restore) the option to make "internet traffic detected" errors non-fatal so that local runs can always succeed do not seem to gain traction with @maflcko and @fanquake. Further, @maflcko suggests it would be better if a backtrace is printed when a test tries to access internet. I agree this would be better, but there is no patch for that and nobody is working on it as far as I know. For the time being I will be rebasing this more frequently which would do the job of detecting regressions on |
|
I think to move it forward in the current state without the detailed backtrace, it would have to be opt-in. Otherwise, basically all places uses podman would have to opt-out. However, then the value would be limited, because no one would enable it? So it would be a GitHub-CI-only feature, basically? I think there is value here, re-Concept ACK, to clarify. |
|
I think what was going on was this - without I played with this locally with a non-127.0.0.1 resolver and DNS queries were made to the resolver only sometimes. My guess is that the OS does some basic caching, so if e.g. That said, with the addition of
|
ryanofsky
left a comment
There was a problem hiding this comment.
Code review f439bad. Looks good and it would be nice if the dnsseed fix resolves things so the PR isn't blocked.
I think there is a bug that could cause non-port 53 traffic on non-loopback interfaces to be ignored, see comment below.
|
|
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
In ci/test/03_test_script.sh line 224:
local f=$(tcpdump_file "$test_name" "$ifname")
^-- SC2155 (warning): Declare and assign separately to avoid masking return values.
In ci/test/03_test_script.sh line 235:
local out="$(tcpdump -n -r "$f" --direction=out tcp or udp)"
^-^ SC2155 (warning): Declare and assign separately to avoid masking return values.
For more information:
https://www.shellcheck.net/wiki/SC2155 -- Declare and assign separately to ...
^---- ⚠️ Failure generated from lint-shell.py |
|
|
Prevent generating outbound traffic on a non-loopback interface during tests.
node_init_tests/init_test[1111:1111::1]:53. This is achieved by runningtcpdumpduring tests and inspecting its output after the tests. Add--cap-add NET_RAWto docker to be able to runtcpdump.Resolves #31339