Adding SBOM components and integration tests #831

a-ovchinnikov · 2025-02-10T20:02:50Z

This change adds SBOM components generation and integration tests to finalize Cargo support implementation. It also fixes a problem of missing git dependencies substitution: git dependencies have to be explicitly replaced with a local copy. This change deals with this by collecting cargo output which is one substitution away from a correct config file template.

Maintainers will complete the following section

Commit messages are descriptive enough
Code coverage from testing does not decrease and new code is covered
Docs updated (if applicable)
Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

approve GitHub Actions workflows by clicking a button
approve the Red Hat Trusted App Pipeline container build by commenting /ok-to-test
(as is the standard for Pipelines as Code)

cachi2/core/package_managers/cargo/main.py

slimreaper35 · 2025-02-11T12:31:51Z

cachi2/core/package_managers/cargo/main.py

+    mk_purl = lambda n, v: PackageURL(type="cargo", name=n, version=v).to_string()  # noqa: E731
+    mk_comp = lambda n, v: Component(name=n, version=v, purl=mk_purl(n, v))  # noqa: E731
+    components = [mk_comp(n, v) for n, v in name_versions]


I know the PURL spec is extremely vague, but I am missing here the qualifiers and a subpath that we use in other package managers' backends and some differentiation among various dependency types that are mentioned in the design doc...

Plus, the information about dev package managers, but that could done separately.

We could add source=registry+https://github.com/... qualifier here which would let us differentiate between oure git dependencies and registry dependencies. IIRC we should not see any other in the wild. I think I'll try parsing the lock file after all. Could you please elaborate on "the information about dev package managers"?

cachi2/core/package_managers/cargo/main.py

tests/unit/package_managers/cargo/test_main.py

slimreaper35 · 2025-02-11T13:04:51Z

tests/integration/test_cargo.py

+        pytest.param(
+            utils.TestParameters(
+                branch="cargo/mixed-git-crate-dependency",
+                packages=({"path": ".", "type": "cargo"},),
+                flags=["--dev-package-managers"],
+                check_output=False,
+                check_deps_checksums=False,
+                check_vendor_checksums=False,
+                expected_exit_code=0,
+                expected_output="",
+            ),
+            id="mixed_git_crate_dependency",


This is a duplicate test already present as e2e

I'd rather kept it to make spotting failures easier: if this one passes and e2e fails then we would instantly know that fetch is fine.

I guess this is the same topic as discussed in #832 (comment).

BTW I removed duplicate tests for bundler in #783..

test_e2e_x is a basically superset of test_x_packages, it adds utils.build_image_and_check_cmd.
In total, there are only two function calls, so I don't see any big investigation here after a failed test.

50% reduction of potentially problematic are at a glance is quite a feat, I'd say :)

What's the problem here? If the function that prefetches dependencies fails, we know that the prefetching of dependencies has failed; similarly, if the building image fails, we know that the building image has failed.

Do you suggest dropping all three ITs in favor of the unified e2e?

Yes. But it should be a collective decision :)

tests/integration/test_cargo.py

tests/integration/test_data/cargo_mixed_dep/container/Containerfile

slimreaper35 · 2025-02-11T13:15:08Z

tests/integration/test_cargo.py

+        pytest.param(
+            utils.TestParameters(
+                branch="cargo/just-a-crate-dependency",
+                packages=({"path": ".", "type": "cargo"},),
+                flags=["--dev-package-managers"],
+                check_output=False,
+                check_deps_checksums=False,
+                check_vendor_checksums=False,
+                expected_exit_code=0,
+                expected_output="",
+            ),
+            id="just_a_crate_dependency",
+        ),
+        pytest.param(
+            utils.TestParameters(
+                branch="cargo/just-a-git-dependency",
+                packages=({"path": ".", "type": "cargo"},),
+                flags=["--dev-package-managers"],
+                check_output=False,
+                check_deps_checksums=False,
+                check_vendor_checksums=False,
+                expected_exit_code=0,
+                expected_output="",
+            ),
+            id="just_a_git_dependency",


I think we could merge these test cases IMO to a more complex one. Having a git dependency and crate dependency together is a common scenario IMO. I would maybe add a negative scenario instead, for example missing Cargo.lock etc.

When things go wrong in a complex scenario one would need to invest more time into figuring out which part caused the issue. Having separate tests helps in making sure that individual types work fine. I thought about negative tests like missing lock file or project config, but could not come up with a realistic scenario in which that could happen. Hermeto will be used for building public projects with at least some following and likely some CI. These projects will be collected by a tool which is extremely unlikely to loose a file. In the extremely unlikely case of one of the files missing cargo itself would exit with a very non-zero exit code and would pull everything down with it which is a good thing -- we'll have this behavior logged and users would know that something is very wrong with their repository. It does feel a bit like testing thin wrappers around stdlib functions just in case. I might be wrong in my analysis so please correct me if I missed something.

When things go wrong in a complex scenario one would need to invest more time into figuring out which part caused the issue. Having separate tests helps in making sure that individual types work fine.

Depends, these tests are fairly cheap compared to e2e, but then if e2e is a superset, then I also don't see that much value in individual positive tests, because positive cases should be covered in an e2e. Testing negative scenarios individually is a different story.

We have an older IT, which is now a public archive, created by @brunoapimentel - https://github.com/cachito-testing/cachi2-rust.

I always thought about e2e tests to be more or less real-world projects with many dependencies. Personally, I don't like the idea of having simple tests just to have less work to do when something fails.

The point of having simple tests is the same as for having well organized code -- to compartmentalize behavior in manageable components. I can easily buy the argument for having more tests, some of which could be quite complex, but I still think that having simpler tests increases precision.

cachi2/core/package_managers/cargo/main.py

eskultety · 2025-02-11T12:51:14Z

tests/integration/test_data/cargo_mixed_dep/container/Containerfile

@@ -0,0 +1,9 @@
+FROM docker.io/rust:1.67


The current stable version is 1.84. Have we freezed our implementation at a given release? Please refresh my memory because I don't remember.
In any case, these Dockerfiles are not checked by dependabot which means that we might be stuck at this particular version of rust for a very long time until someone notices. That said, latest isn't an option because with a potential release 2.0 it might instantly break our tests. I think with rust releases the same logic applies as with go - major version guarantees backwards compatibility (https://rust-for-linux.com/unstable-features) so I'd prefer rust:1 instead. And since these are mostly trivial Dockerfiles, ultimately (it's on my todo list) we should convert all of these (whichever can!) to alpine, so in this case rust:1-alpine please.

Actually we don't even need to wait until 2.0 to get things broken: 1.84 generates lock file v4 while 1.67 generates v3. The remnants of my experiments with versions could be seen below (sed update). Thank you for the hint, I'll switch to rust:1-alpine.

Hold on, I'm getting confused now.

1.84 generates lock file v4 while 1.67 generates v3.

What does ^this mean for us? Are we entering a yarn-like territory with these versions or are we completely unaffected by different lockfile versions? Can you elaborate some more please?

v4 could be incompatible with v3 and older versions of cargo would refuse dealing with lock files generated by newer ones.

cachi2/core/package_managers/cargo/main.py

a-ovchinnikov · 2025-02-12T02:08:27Z

The new revision has components generation unified with a dedicated dataclass, package qualifiers are added to purls, Containerfile is switched to a different base image.

cachi2/core/package_managers/cargo/main.py

eskultety · 2025-02-12T11:36:10Z

tests/integration/test_cargo.py

+        pytest.param(
+            utils.TestParameters(
+                branch="cargo/just-a-crate-dependency",
+                packages=({"path": ".", "type": "cargo"},),
+                flags=["--dev-package-managers"],
+                check_output=False,
+                check_deps_checksums=False,
+                check_vendor_checksums=False,
+                expected_exit_code=0,
+                expected_output="",
+            ),
+            id="just_a_crate_dependency",
+        ),
+        pytest.param(
+            utils.TestParameters(
+                branch="cargo/just-a-git-dependency",
+                packages=({"path": ".", "type": "cargo"},),
+                flags=["--dev-package-managers"],
+                check_output=False,
+                check_deps_checksums=False,
+                check_vendor_checksums=False,
+                expected_exit_code=0,
+                expected_output="",
+            ),
+            id="just_a_git_dependency",


When things go wrong in a complex scenario one would need to invest more time into figuring out which part caused the issue. Having separate tests helps in making sure that individual types work fine.

Depends, these tests are fairly cheap compared to e2e, but then if e2e is a superset, then I also don't see that much value in individual positive tests, because positive cases should be covered in an e2e. Testing negative scenarios individually is a different story.

eskultety · 2025-02-12T11:40:49Z

tests/integration/test_data/cargo_mixed_dep/container/Containerfile

@@ -0,0 +1,9 @@
+FROM docker.io/rust:1.67


Hold on, I'm getting confused now.

1.84 generates lock file v4 while 1.67 generates v3.

What does ^this mean for us? Are we entering a yarn-like territory with these versions or are we completely unaffected by different lockfile versions? Can you elaborate some more please?

cachi2/core/package_managers/cargo/main.py

This change makes Cargo package manager generate components to populate resulting SBOM. Signed-off-by: Alexey Ovchinnikov <[email protected]>

Previously only crates.io was replaced with vendored directory. This does not work with git-based projects. This change fetches cargo-provided config and updates it to make it relocatable. This commit also removes tests for creation of .cargo/config.toml and corresponding code: it should not be created at this moment, the config will be injected later by inject-files command basing on generated template. Signed-off-by: Alexey Ovchinnikov <[email protected]>

This commit introduces integration tests for cargo and an e2e test. Signed-off-by: Alexey Ovchinnikov <[email protected]>

a-ovchinnikov force-pushed the issue796 branch 2 times, most recently from ead6a4b to 1564e0e Compare February 10, 2025 20:05

slimreaper35 reviewed Feb 11, 2025

View reviewed changes

eskultety reviewed Feb 11, 2025

View reviewed changes

a-ovchinnikov force-pushed the issue796 branch 3 times, most recently from 1063b9f to e42c386 Compare February 12, 2025 02:06

eskultety reviewed Feb 12, 2025

View reviewed changes

slimreaper35 reviewed Feb 12, 2025

View reviewed changes

cachi2/core/package_managers/cargo/main.py Outdated Show resolved Hide resolved

a-ovchinnikov force-pushed the issue796 branch 4 times, most recently from 46a07e2 to d0f903e Compare February 12, 2025 21:01

a-ovchinnikov added 3 commits February 12, 2025 15:13

cargo: Adding components

4b4db36

This change makes Cargo package manager generate components to populate resulting SBOM. Signed-off-by: Alexey Ovchinnikov <[email protected]>

cargo: Adding integration tests

3dc587c

This commit introduces integration tests for cargo and an e2e test. Signed-off-by: Alexey Ovchinnikov <[email protected]>

a-ovchinnikov force-pushed the issue796 branch from d0f903e to 3dc587c Compare February 12, 2025 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding SBOM components and integration tests #831

Adding SBOM components and integration tests #831

a-ovchinnikov commented Feb 10, 2025

slimreaper35 Feb 11, 2025

a-ovchinnikov Feb 11, 2025

slimreaper35 Feb 11, 2025

a-ovchinnikov Feb 11, 2025

slimreaper35 Feb 12, 2025

a-ovchinnikov Feb 12, 2025

slimreaper35 Feb 13, 2025

a-ovchinnikov Feb 13, 2025

slimreaper35 Feb 13, 2025

slimreaper35 Feb 11, 2025

a-ovchinnikov Feb 11, 2025

eskultety Feb 12, 2025

slimreaper35 Feb 12, 2025

a-ovchinnikov Feb 12, 2025

eskultety Feb 11, 2025

a-ovchinnikov Feb 11, 2025

eskultety Feb 12, 2025

a-ovchinnikov Feb 12, 2025

a-ovchinnikov commented Feb 12, 2025

eskultety Feb 12, 2025

eskultety Feb 12, 2025

Adding SBOM components and integration tests #831

Are you sure you want to change the base?

Adding SBOM components and integration tests #831

Conversation

a-ovchinnikov commented Feb 10, 2025

Maintainers will complete the following section

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

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

a-ovchinnikov commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment