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

🐛 Fix progress for unscored assets #424

Merged
merged 2 commits into from
Mar 10, 2023
Merged

Conversation

czunker
Copy link
Contributor

@czunker czunker commented Mar 9, 2023

Before, the progress bars looked like this, when unscored assets where included:

 kube-system/coredns                                ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────   0% score: U
 mondoo-operator/mondoo-client-scan-api             ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────   0% score: U
 mondoo-operator/mondoo-operator-controller-manager ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────   0% score: U
 mondoo-operator/mondoo-client-k8s-scan             ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% score: A
 mondoo-operator/mondoo-client-node-minikube        ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% score: A

 5/5 scanned                                        ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━──────────────────────────────────────────────────────────────────────────────────────────────────────────────  40%

Now, it looks like this:

 kube-system/coredns                                ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────  n/a score: U
 mondoo-operator/mondoo-client-scan-api             ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────  n/a score: U
 mondoo-operator/mondoo-operator-controller-manager ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────  n/a score: U
 mondoo-operator/mondoo-client-k8s-scan             ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% score: A
 mondoo-operator/mondoo-client-node-minikube        ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% score: A

 2/5 scanned 3/5 n/a                                ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100%

@czunker czunker requested a review from preslavgerchev March 9, 2023 08:03
@@ -595,7 +595,11 @@ func (s *localAssetScanner) run() (*AssetReport, error) {
return ar, err
}
s.ProgressReporter.Score(report.Score.Rating().Letter())
s.ProgressReporter.Completed()
if report.Score.Rating().Letter() == "U" {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, this is different. It is not an error, the asset matched a policy, but didn't get scored.

I used this policy to provoke this:

  - asset_filter:
      query: platform.name == "k8s-deployment"
    scoring_queries:
    data_queries:

@preslavgerchev In which situation did you see the problem with the progress bar?

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested this with the Kubernetes Best Practices by Mondoo policy, however it had most of its controls disabled or ignored, leading to the situation you described above (matching filter, no queries).

i can provide the exact policy (with ignored queries) if needed

Copy link
Member

Choose a reason for hiding this comment

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

I am not 100% sure I understand what we are solving here then. In case policies applied it was scored. Feels like the issue is with the policy itself.

Copy link
Member

Choose a reason for hiding this comment

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

Preslav showed me the issue. I agree that we need to fix the overall progress, but reading the score itself feels wrong. The progress bar should not care about the rating at all. We can move forward with this approach as mitigation but we need a better solution for that handling going forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up is #439

if !ok {
continue
}
if len(report.Scores) >= 0 {
Copy link
Contributor

@preslavgerchev preslavgerchev Mar 9, 2023

Choose a reason for hiding this comment

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

was this meant to be > 0 instead of >= 0 ? else the check does not make a lot of sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good catch. Thanks.

@czunker czunker force-pushed the christian/fix_unscored_progress branch from e213aa3 to d3c0cbb Compare March 9, 2023 13:14
@czunker czunker merged commit f285d42 into main Mar 10, 2023
@czunker czunker deleted the christian/fix_unscored_progress branch March 10, 2023 12:07
@github-actions github-actions bot locked and limited conversation to collaborators Mar 10, 2023
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.

3 participants