eclipse-kuksa / kuksa-databroker

A modern in-vehicle VSS (Vehicle Signal Specification) server written in RUST
https://eclipse-kuksa.github.io/kuksa-website/
Apache License 2.0
12 stars 11 forks source link

CycloneDX SBOM in artifacts #24

Closed SebastianSchildt closed 2 months ago

SebastianSchildt commented 2 months ago

Ported and improved version of from https://github.com/eclipse/kuksa.val/pull/756

Main change in generated artifacts:

Creates and includes CycloneDX Software Bill of Materials (SBOM) for the databroker. This helps with compliance using kuksa in a commercial context, and is generally also one of the accepted formats fulfilling EU CRA SBOM requirements

"Collateral improvements"

codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 48.57%. Comparing base (e628c46) to head (008d5c7). Report is 13 commits behind head on main.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #24 +/- ## ========================================== - Coverage 48.61% 48.57% -0.05% ========================================== Files 31 31 Lines 10879 10866 -13 ========================================== - Hits 5289 5278 -11 + Misses 5590 5588 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

erikbosch commented 2 months ago

General comments:

I believe this change will break the https://github.com/boschglobal/kuksa-databroker/blob/feature/cyclonedx-sbom/.github/workflows/create_draft_release.yml. Hopefully not if we use latest kuksa-action dash version. Anyway, should better be tested.

Adding @lukasmittag and @argerus as reviewers, so that they are aware of the upcoming change and can protest, if needed

SebastianSchildt commented 2 months ago

I think I addressed all comments, I just did not add "how to build docker locally" documentation. As it was not there before, do you think it is required? The Dockerfile itself is not "magic", and there is a hint, you need to run the build script first inline

erikbosch commented 2 months ago

I get some weird errors in https://github.com/boschglobal/kuksa-databroker/actions/runs/8966502432/job/24622725710

i.e. when running https://github.com/eclipse-kuksa/kuksa-databroker/blob/main/.github/workflows/create_draft_release.yml on this branch

Run which cross || cargo install cross
  which cross || cargo install cross
  cargo install cargo-license cargo-cyclonedx
  pip install "git+https://github.com/eclipse-kuksa/kuksa-common.git@6f3d7627760582d8ba83cc8a0f7449d00fffee84#subdirectory=sbom-tools"
  shell: /usr/bin/bash -e {0}
  env:
    CARGO_TERM_COLOR: always
/home/runner/.cargo/bin/cross
    Updating crates.io index
error: binary `cargo-license` already exists in destination
Add --force to overwrite
error: binary `cargo-cyclonedx` already exists in destination
Add --force to overwrite
     Summary Failed to install cargo-license, cargo-cyclonedx (see error(s) above).
error: some crates failed to install
Error: Process completed with exit code 101.

Could it possibly be that we use same cache key for databroker and databroker-cli? I will do some troubleshooting

key: databroker-release-${{ matrix.platform.name }}-${{ hashFiles('**/Cargo.lock') }}

argerus commented 2 months ago
error: binary `cargo-license` already exists in destination
Add --force to overwrite
error: binary `cargo-cyclonedx` already exists in destination
Add --force to overwrite
     Summary Failed to install cargo-license, cargo-cyclonedx (see error(s) above).
error: some crates failed to install
Error: Process completed with exit code 101.

Could it possibly be that we use same cache key for databroker and databroker-cli? I will do some troubleshooting

No, but that is also not right.

As the message says the binaries are already installed (cached & restored) and trying to install them again will generate this error unless --force is added.

This doesn't happen for cargo cross, since it's only installed if the binary is missing with:

   which cross || cargo install cross
SebastianSchildt commented 2 months ago
error: binary `cargo-license` already exists in destination
Add --force to overwrite
error: binary `cargo-cyclonedx` already exists in destination
Add --force to overwrite
     Summary Failed to install cargo-license, cargo-cyclonedx (see error(s) above).
error: some crates failed to install
Error: Process completed with exit code 101.

Could it possibly be that we use same cache key for databroker and databroker-cli? I will do some troubleshooting

No, but that is also not right.

As the message says the binaries are already installed (cached & restored) and trying to install them again will generate this error unless --force is added.

This doesn't happen for cargo cross, since it's only installed if the binary is missing with:

   which cross || cargo install cross

Interesting. I only did observe this with cross, thus I used the "which pattern" I found in the workflows here. I could use the same pattern for the other cargo tools, but question: Would we risk of running with "arbitrarily" old version for a long time, and once github throws cache suddenly get a new one?

Is there any way to always "upgrade" to a new stable version, if it exists? I think "--force" would always reinstall, that would work, but cost time...

SebastianSchildt commented 2 months ago

Could it possibly be that we use same cache key for databroker and databroker-cli? I will do some troubleshooting

key: databroker-release-${{ matrix.platform.name }}-${{ hashFiles('**/Cargo.lock') }}

As @argerus said, that is not the root cause, but has been fixed meanwhile

erikbosch commented 2 months ago

Could it possibly be that we use same cache key for databroker and databroker-cli? I will do some troubleshooting

No, but that is also not right.

As the message says the binaries are already installed (cached & restored) and trying to install them again will generate this error unless --force is added.

This doesn't happen for cargo cross, since it's only installed if the binary is missing with:

   which cross || cargo install cross

I got the impression that an error no longer shall be given upon reinstall based on the discussion in https://github.com/rust-lang/cargo/issues/6727. When running cargo install locally I do not get any error when trying to install cargo-license even if it is already there

erik@debian6:~/kuksa-databroker$ cargo install cargo-license
    Updating crates.io index
     Ignored package `cargo-license v0.6.1` is already installed, use --force to override
erik@debian6:~/kuksa-databroker$ echo $?
0
erik@debian6:~/kuksa-databroker$ cargo install cargo-license
    Updating crates.io index
     Ignored package `cargo-license v0.6.1` is already installed, use --force to override
erik@debian6:~/kuksa-databroker$ echo $?
0
erikbosch commented 2 months ago

I did some more tests on the cargo install issue. According to the links below --force shall not be needed. But it seems that we better should add two more files to the caching; ~/.cargo/.crates.toml and ~/.cargo/.crates2.json. With that https://github.com/boschglobal/kuksa-databroker/actions/workflows/create_draft_release.yml seems to work as expected.

  - uses: actions/cache@v4
      with:
        path: |
          ~/.cargo/bin/
          ~/.cargo/registry/index/
          ~/.cargo/registry/cache/
          ~/.cargo/git/db/
          ~/.cargo/.crates.toml
          ~/.cargo/.crates2.json
          target/

References:

SebastianSchildt commented 2 months ago

@erikbosch I just googled the same :D Will try. Still trying to move things....

SebastianSchildt commented 2 months ago

Moved all the scripts now, and (hopefully) fixed the cargo install issues

@erikbosch I also moved the prepare_release.sh script and tried to "blindly" fix it, so please take a look it is doing, what you expect.

SebastianSchildt commented 2 months ago

thanks for testing @erikbosch , will make some minor adjustments tomorrow, and maybe solve the target caching problem. I just think we really, really NEED correct(er), better standard compliant SBOMS, even though this now feels like refactoring whole build 🗡️ Well, as long as stuff gets better....

SebastianSchildt commented 2 months ago

I would appreciate tif this to be merged now. This is quite large now, and the thing we NEED is sboms.

Nothing against optimising it further (maybe make tickets), but we should move forward so I (or someone) don't need to port over all this stuff again in some weeks

erikbosch commented 2 months ago

Any objections to merging @argerus?

@SebastianSchildt - I approved but I believe you have to make sure that John approves or alternatively that you dismiss his review to be able to merge