Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🧹 Prevent panic for unsupported operating systems #4009

Merged
merged 1 commit into from
May 23, 2024

Conversation

czunker
Copy link
Contributor

@czunker czunker commented May 16, 2024

Follow-up to #4008

@czunker
Copy link
Contributor Author

czunker commented May 16, 2024

I tested this, by removing the linux filter from the sbom policy and run it on my local linux system:

→ discover related assets for 1 asset(s)
FTL failed to parse bom error="no data points found"
Process 111189 has exited with status 1

Copy link
Contributor

github-actions bot commented May 16, 2024

Test Results

2 993 tests   2 992 ✅  1m 28s ⏱️
  329 suites      1 💤
   23 files        0 ❌

Results for commit 9c766e2.

♻️ This comment has been updated with latest results.

sbom/sbom.go Outdated
@@ -72,6 +72,9 @@ func GenerateBom(r *reporter.Report) ([]*Sbom, error) {

// extract os packages and python packages
dataPoints := r.Data[mrn]
if dataPoints == nil {
return nil, fmt.Errorf("no data points found")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have no data points, this is not an error in the conversion. We should just return the current constructed bom then without any additional package data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the operating system is not supported, the complete sbom query pack is not executed and we don't have any data:

→ discover related assets for 1 asset(s)

Process 158394 has exited with status 0

Shouldn't we at least show some info?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea. What we could do is, we set the error state, add a message and at least fill in the asset information. Here is the failed state https://github.com/mondoohq/cnquery/blob/main/sbom/sbom.proto#L38 that we can set here https://github.com/mondoohq/cnquery/blob/main/sbom/sbom.proto#L65. We can set the error message here https://github.com/mondoohq/cnquery/blob/main/sbom/sbom.proto#L81

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to store the error in the report but not error the report generation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way the CLI still does not show any error, but the json output includes it:

{
  "generator": {
    "vendor": "Mondoo, Inc.",
    "name": "cnquery",
    "url": "https://mondoo.com"
  },
  "timestamp": "2024-05-21T08:31:32Z",
  "status": 3,
  "asset": {
    "name": "X1",
    "platform": {}
  },
  "error_message": "no data points found"
}

Follow-up to #4008

Signed-off-by: Christian Zunker <christian@mondoo.com>
@czunker czunker force-pushed the czunker/prevent_sbom_panic branch from c5fbd0c to 9c766e2 Compare May 21, 2024 08:33
@czunker czunker requested a review from chris-rock May 21, 2024 08:34
Copy link
Member

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the updated error handling.

@chris-rock chris-rock merged commit 755dea0 into main May 23, 2024
15 checks passed
@chris-rock chris-rock deleted the czunker/prevent_sbom_panic branch May 23, 2024 08:35
@github-actions github-actions bot locked and limited conversation to collaborators May 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants