[1.10 Backport] Revert "fix: test_sec_audit: ignore cargo audit output"

[roypat]

This reverts commit d761b01. The output
to stdout was fixed in cargo audit 0.21.2, so if we rebuild the docker
container the grep is no longer necessary.

In fact, the grep has broken this test in our nightly pipeline because
it overwrites the return code of cargo audit itself, meaning the non-PR
version of this test (which is supposed to fail if there exist any cargo
audit warnings) was never failing.

(cherry picked from commit 1d98a21)
Signed-off-by: Patrick Roy [email protected]

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

• I have read and understand CONTRIBUTING.md.
• I have run tools/devtool checkstyle to verify that the PR passes the
  automated style checks.
• I have described what is done in these changes, why they are needed, and
  how they are solving the problem in a clear and encompassing way.
• I have updated any relevant documentation (both in code and in the docs)
  in the PR.
• I have mentioned all user-facing changes in CHANGELOG.md.
• If a specific issue led to this PR, this PR closes the issue.
• When making API changes, I have followed the
  Runbook for Firecracker API changes.
• I have tested all new and changed functionalities in unit tests and/or
  integration tests.
• I have linked an issue to every new TODO.

---

• This functionality cannot be added in rust-vmm.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.10%. Comparing base (2392fe9) to head (1ec07ec).
Report is 1 commits behind head on firecracker-v1.10.

Additional details and impacted files

@@                Coverage Diff                 @@
##           firecracker-v1.10    #5101   +/-   ##
==================================================
  Coverage              84.10%   84.10%           
==================================================
  Files                    251      251           
  Lines                  28080    28080           
==================================================
  Hits                   23616    23616           
  Misses                  4464     4464           

Flag	Coverage Δ
5.10-c5n.metal	84.67% <ø> (-0.01%)	⬇️
5.10-m5n.metal	84.65% <ø> (-0.01%)	⬇️
5.10-m6a.metal	83.96% <ø> (ø)
5.10-m6g.metal	80.78% <ø> (ø)
5.10-m6i.metal	84.65% <ø> (-0.01%)	⬇️
5.10-m7g.metal	80.78% <ø> (ø)
6.1-c5n.metal	84.67% <ø> (ø)
6.1-m5n.metal	84.65% <ø> (ø)
6.1-m6a.metal	83.96% <ø> (ø)
6.1-m6g.metal	80.78% <ø> (ø)
6.1-m6i.metal	84.65% <ø> (-0.01%)	⬇️
6.1-m7g.metal	80.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Manciukic]

This is a lot of changes for a one line fix. Is there a way we can fix the audit without bringing in alll these other unrelated changes?

[roypat]

This is a lot of changes for a one line fix. Is there a way we can fix the audit without bringing in alll these other unrelated changes?

I tried building a sort of v75.1 of the docker container specifically for the 1.10 branch that only pulls in the cargo audit update, but that didn't easily work because we don't pin versions in the docker container, and a bunch of our cargo installed tools dont compile on the toolchain specified in the container anymore. And writing even more commits on top of that specifically for the release branch seemed like more tech debt than a series of large, but cleanly applying cherry-picks from main.

[Manciukic]

Maybe we can just hack it to add a -o pipefail and fix the test without removing the grep, thus without requiring to update the cargo audit version:

diff --git a/tests/integration_tests/security/test_sec_audit.py b/tests/integration_tests/security/test_sec_audit.py
index 1ad625cc7..1f393dd6c 100644
--- a/tests/integration_tests/security/test_sec_audit.py
+++ b/tests/integration_tests/security/test_sec_audit.py
@@ -35,6 +35,6 @@ def test_cargo_audit():
         )
 
     git_ab_test_host_command_if_pr(
-        "cargo audit --deny warnings -q --json |grep -Po '{.*}'",
+        "bash -o pipefail -c \"cargo audit --deny warnings -q --json |grep -Po '{.*}'\"",
         comparator=set_did_not_grow_comparator(set_of_vulnerabilities),
     )

I tested it and it's working fine: ./tools/devtool test -- integration_tests/security/test_sec_audit.py

• passes on HEAD of v1.10
• fails on HEAD^ (before @bchalios cargo updates)

[roypat]

Maybe we can just hack it to add a -o pipefail and fix the test without removing the grep, thus without requiring to update the cargo audit version:

diff --git a/tests/integration_tests/security/test_sec_audit.py b/tests/integration_tests/security/test_sec_audit.py
index 1ad625cc7..1f393dd6c 100644
--- a/tests/integration_tests/security/test_sec_audit.py
+++ b/tests/integration_tests/security/test_sec_audit.py
@@ -35,6 +35,6 @@ def test_cargo_audit():
         )
 
     git_ab_test_host_command_if_pr(
-        "cargo audit --deny warnings -q --json |grep -Po '{.*}'",
+        "bash -o pipefail -c \"cargo audit --deny warnings -q --json |grep -Po '{.*}'\"",
         comparator=set_did_not_grow_comparator(set_of_vulnerabilities),
     )

I tested it and it's working fine: ./tools/devtool test -- integration_tests/security/test_sec_audit.py

* passes on `HEAD` of v1.10

* fails on `HEAD^` (before @bchalios cargo updates)

But this won't work in the scenario where we want to run the grep, because it'll just abort without filtering out the unwanted part of the output in case of failures, no?

[Manciukic]

But this won't work in the scenario where we want to run the grep, because it'll just abort without filtering out the unwanted part of the output in case of failures, no?

Isn't that the same behavior of running without the grep on the fixed cargo-audit?

[roypat]

But this won't work in the scenario where we want to run the grep, because it'll just abort without filtering out the unwanted part of the output in case of failures, no?

Isn't that the same behavior of running without the grep on the fixed cargo-audit?

When we run this as an A/B-Test, in python we ignore the return code and only parse the json it outputs to stderr. But for that to work, all non-json parts actually need to be stripped from stderr, e.g. we need to run the grep part even if cargo audit fails.

Now yes, there's way we can hack around all of this by running different commands for A/B and non-A/B, but I admittedly don't see the problem with backporting a handful (or two) of commits 😅

[Manciukic]

When we run this as an A/B-Test, in python we ignore the return code and only parse the json it outputs to stderr. But for that to work, all non-json parts actually need to be stripped from stderr, e.g. we need to run the grep part even if cargo audit fails.

Right, that's why I suggest to keep the grep to make it work for PRs and just add -o pipefail to the command to fix the test for non-PR, in this release branch only, in order to avoid pulling in all these changes.

I tested also the PR scenario in case you don't believe me, adding a debug print(output) to show what's being compared:

$ BUILDKITE_PULL_REQUEST_BASE_BRANCH=350a40e82 BUILDKITE_PULL_REQUEST=true ./tools/devtool test -- -s integration_tests/security/test_sec_audit.py
[...]
integration_tests/security/test_sec_audit.py::test_cargo_audit is_pr(): true
{'database': {'advisory-count': 742, 'last-commit': '825bd26e5e14f8906f9314be1ba6734a753341d1', 'last-updated': '2025-03-12T18:41:50-06:00'}, 'lockfile': {'dependency-count': 192}, 'settings': {'target_arch': None, 'target_os': None, 'severity': None, 'ignore': ['RUSTSEC-2024-0436'], 'informational_warnings': ['unmaintained', 'unsound', 'notice']}, 'vulnerabilities': {'found': False, 'count': 0, 'list': []}, 'warnings': {}}
{'database': {'advisory-count': 742, 'last-commit': '825bd26e5e14f8906f9314be1ba6734a753341d1', 'last-updated': '2025-03-12T18:41:50-06:00'}, 'lockfile': {'dependency-count': 192}, 'settings': {'target_arch': None, 'target_os': None, 'severity': None, 'ignore': ['RUSTSEC-2024-0436'], 'informational_warnings': ['unmaintained', 'unsound', 'notice']}, 'vulnerabilities': {'found': True, 'count': 1, 'list': [{'advisory': {'id': 'RUSTSEC-2024-0402', 'package': 'hashbrown', 'title': 'Borsh serialization of HashMap is non-canonical', 'description': 'The borsh serialization of the HashMap did not follow the borsh specification.\nIt potentially produced non-canonical encodings dependent on insertion order.\nIt also did not perform canonicty checks on decoding.\n\nThis can result in consensus splits and cause equivalent objects to be\nconsidered distinct.\n\nThis was patched in 0.15.1.', 'date': '2024-10-11', 'aliases': [], 'related': [], 'collection': 'crates', 'categories': [], 'keywords': ['borsh'], 'cvss': None, 'informational': None, 'references': [], 'source': None, 'url': 'https://github.com/rust-lang/hashbrown/issues/576', 'withdrawn': None, 'license': 'CC0-1.0'}, 'versions': {'patched': ['>=0.15.1'], 'unaffected': ['<0.15.0']}, 'affected': {'arch': [], 'os': [], 'functions': {'hashbrown::HashMap::borsh_serialize': ['=0.15.0']}}, 'package': {'name': 'hashbrown', 'version': '0.15.0', 'source': 'registry+https://github.com/rust-lang/crates.io-index', 'checksum': '1e087f84d4f86bf4b218b927129862374b72199ae7d8657835f1e89000eea4fb', 'replace': None}}]}, 'warnings': {'unsound': [{'kind': 'unsound', 'package': {'name': 'kvm-ioctls', 'version': '0.19.0', 'source': 'registry+https://github.com/rust-lang/crates.io-index', 'checksum': '337d1afa126368bbd6a5c328048f71a69a737e9afe7e436b392a8f8d770c9171', 'dependencies': [{'name': 'bitflags', 'version': '2.6.0', 'source': 'registry+https://github.com/rust-lang/crates.io-index'}, {'name': 'kvm-bindings', 'version': '0.10.0', 'source': 'registry+https://github.com/rust-lang/crates.io-index'}, {'name': 'libc', 'version': '0.2.161', 'source': 'registry+https://github.com/rust-lang/crates.io-index'}, {'name': 'vmm-sys-util', 'version': '0.12.1', 'source': 'registry+https://github.com/rust-lang/crates.io-index'}], 'replace': None}, 'advisory': {'id': 'RUSTSEC-2024-0428', 'package': 'kvm-ioctls', 'title': 'Undefined behaviour in `kvm_ioctls::ioctls::vm::VmFd::create_device`', 'description': "An issue was identified in the `VmFd::create_device function`, leading to undefined behavior and miscompilations on rustc 1.82.0 and newer due to the function's violation of Rust's pointer safety rules.\n\nThe function downcasted a mutable reference to its `struct kvm_create_device` argument to an immutable pointer, and then proceeded to pass this pointer to a mutating system call. Rustc 1.82.0 and newer elides subsequent reads of this structure's fields, meaning code will not see the value written by the kernel into the `fd` member. Instead, the code will observe the value that this field was initialized to prior to calling `VmFd::create_device` (usually, 0).\n\nThe issue started in kvm-ioctls 0.1.0 and was fixed in 0.19.1 by correctly using\na mutable pointer.", 'date': '2024-12-05', 'aliases': [], 'related': [], 'collection': 'crates', 'categories': [], 'keywords': ['unsound', '1.82'], 'cvss': None, 'informational': 'unsound', 'references': [], 'source': None, 'url': 'https://github.com/rust-vmm/kvm/pull/298', 'withdrawn': None, 'license': 'CC0-1.0'}, 'affected': {'arch': [], 'os': ['linux'], 'functions': {'kvm_ioctls::ioctls::vm::VmFd::create_device': ['<=0.19.0']}}, 'versions': {'patched': ['>=0.19.1'], 'unaffected': []}}]}}
PASSED
[...]

Now yes, there's way we can hack around all of this by running different commands for A/B and non-A/B, but I admittedly don't see the problem with backporting a handful (or two) of commits 😅

We're currently standing at 13 commits and 93 files changed unless you pushed the wrong thing.

[Manciukic]

Oh, I see what you mean now. Bash will not abort grep, it will wait for the entire pipeline to complete.

   The return status of a pipeline is the exit status of the last command,
   unless  the  pipefail  option  is enabled.  If pipefail is enabled, the
   pipeline's return status is the value of the last  (rightmost)  command
   to  exit  with a non-zero status, or zero if all commands exit success‐
   fully.  If the reserved word !  precedes a pipeline, the exit status of
   that  pipeline  is the logical negation of the exit status as described
   above.  The shell waits for all commands in the pipeline  to  terminate
   before returning a value.

[roypat]

Oh, I see what you mean now. Bash will not abort grep, it will wait for the entire pipeline to complete.

   The return status of a pipeline is the exit status of the last command,
   unless  the  pipefail  option  is enabled.  If pipefail is enabled, the
   pipeline's return status is the value of the last  (rightmost)  command
   to  exit  with a non-zero status, or zero if all commands exit success‐
   fully.  If the reserved word !  precedes a pipeline, the exit status of
   that  pipeline  is the logical negation of the exit status as described
   above.  The shell waits for all commands in the pipeline  to  terminate
   before returning a value.

oh! neat, didn't now that. I thought it would just abort before the grep without running it if cargo audit fails.

 We're currently standing at 13 commits and 93 files changed unless you pushed the wrong thing.

I still don't see any problem 😂

but let me update to your proposed on-line fix. Kinda wished we'd done it that way in main as well now, so that all the branches would be in sync, but oh well :(

[Manciukic]

but let me update to your proposed on-line fix. Kinda wished we'd done it that way in main as well now, so that all the branches would be in sync, but oh well :(

I wouldn't worry too much about that. We'll likely never touch this part of the code again and in a few months we'll forget this branch even existed.