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

🐛 Find variant checks by parent MRN #1408

Merged
merged 5 commits into from
Sep 9, 2024

Conversation

czunker
Copy link
Contributor

@czunker czunker commented Aug 23, 2024

Compliance only knows about parent MRNs and not variant MRNs. To allow a mapping from compliance control to executed check, we need to store the check with it's parent MRN.

Copy link
Contributor

github-actions bot commented Aug 23, 2024

Test Results

  1 files  ±0   24 suites  ±0   20s ⏱️ -1s
403 tests ±0  402 ✅ ±0  1 💤 ±0  0 ❌ ±0 
404 runs  ±0  403 ✅ ±0  1 💤 ±0  0 ❌ ±0 

Results for commit f09d9d0. ± Comparison against base commit 8de4c0f.

♻️ This comment has been updated with latest results.

Compliance only knows about parent MRNs and not variant MRNs.
To allow a mapping from compliance control to executed check, we need to store the check with it's parent MRN.

Signed-off-by: Christian Zunker <christian@mondoo.com>
@czunker czunker force-pushed the czunker/compliance2variant_checks branch from b808ebe to 92be54e Compare August 26, 2024 06:26
policy/resolver.go Outdated Show resolved Hide resolved
Signed-off-by: Christian Zunker <christian@mondoo.com>
@czunker czunker force-pushed the czunker/compliance2variant_checks branch from 6180370 to 9ec2c19 Compare August 28, 2024 09:02
policy/resolver.go Outdated Show resolved Hide resolved
Signed-off-by: Christian Zunker <christian@mondoo.com>
@czunker czunker marked this pull request as ready for review September 3, 2024 06:38
@czunker czunker requested a review from jaym September 3, 2024 06:38
@@ -657,8 +662,15 @@ func (s *LocalServices) tryResolve(ctx context.Context, bundleMrn string, assetF
rj.RefreshChecksum()
}

// resolvedPolicyExecutionChecksum is the GraphExceutionChecksum of the policy and the framework
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't feel right. There is a function called BundleExecutionChecksum. I think this manipulation needs to be done there. The graph execution checksum is passed into this function so we can see if we have something for it or it needs to be recomputed. To me, that means all graph execution checksums need to be calculated the same way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BundleExecutionChecksum is called on line L484. But it does not know about reporting jobs or any other job.

Everything about jobs is kept out of any checksum up to this point. Even the checksums of the reporting jobs get refreshed after the jobs are included in a checksum refresh. L661-L663 vs. L658.

The reporting jobs' checksums are included in refreshChecksums on L710, but at this point they are empty.

I don't know whether this is a bug or intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Signed-off-by: Christian Zunker <christian@mondoo.com>
Signed-off-by: Christian Zunker <christian@mondoo.com>
@@ -234,9 +234,9 @@ policies:
require.NoError(t, err)
require.NotNil(t, rp)
require.Len(t, rp.CollectorJob.ReportingJobs, 5)
ignoreJob := rp.CollectorJob.ReportingJobs["8Sis0SvMbtI="]
ignoreJob := rp.CollectorJob.ReportingJobs["q7gxFtwx4zg="]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These UUIDs changed, because the new BundleExecutionChecksum is included in the UUIDs: https://github.com/mondoohq/cnspec/blob/main/policy/resolver.go#L536

@czunker czunker requested a review from jaym September 9, 2024 04:47
@czunker czunker merged commit fe8d058 into main Sep 9, 2024
14 checks passed
@czunker czunker deleted the czunker/compliance2variant_checks branch September 9, 2024 11:53
@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 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