From 6f14f351460491dea2d93a883f6c8b0c732d822f Mon Sep 17 00:00:00 2001 From: Jay Mundrawala Date: Mon, 14 Aug 2023 14:50:05 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20ensure=20there=20are=20no=20dupl?= =?UTF-8?q?icates=20in=20the=20resolved=20policy=20notify=20structure?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``` "YYBvk3VCj7E=": { "checksum": "0dp9+bRqUwA=", "qr_id": "//policy.api.mondoo.app/controls/cisc-4-4", "uuid": "YYBvk3VCj7E=", "notify": [ "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "cTocL2OpWZI=", "jQvxGFEgdOc=", "XYqW5cyrngA=", "6GfQ1zmPQWU=", "zpfO4TnlynY=" ], ``` I was seeing things like this. I believe this was a function of each control map contributing an edge in notify --- policy/framework.go | 37 +++++++++++++++++++++---------------- policy/frameworks_test.go | 4 ++-- policy/resolver.go | 9 ++++----- policy/resolver_test.go | 30 ++++++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 23 deletions(-) diff --git a/policy/framework.go b/policy/framework.go index 017cebb4..ae6572f8 100644 --- a/policy/framework.go +++ b/policy/framework.go @@ -25,6 +25,12 @@ type ResolvedFrameworkNode struct { Type ResolvedFrameworkNodeType } +type ResolvedFrameworkReferenceSet map[string]struct{} + +func (r ResolvedFrameworkReferenceSet) Add(mrn string) { + r[mrn] = struct{}{} +} + type ResolvedFramework struct { Mrn string GraphContentChecksum string @@ -32,12 +38,12 @@ type ResolvedFramework struct { // and frameworks. // E.g. ReportTarget[check123] = [controlA, controlB] // E.g. ReportTarget[controlA] = [frameworkX] - ReportTargets map[string][]string + ReportTargets map[string]ResolvedFrameworkReferenceSet // ReportSources tracks all the sources that a control or framework pulls // data from, i.e. all the checks/policies/controls that provide its data. // E.g. ReportSources[controlA] = [check123, check45] // E.g. ReportSources[frameworkX] = [controlA, ...] - ReportSources map[string][]string + ReportSources map[string]ResolvedFrameworkReferenceSet Nodes map[string]ResolvedFrameworkNode } @@ -503,8 +509,8 @@ func (c *ControlMap) refreshMRNs(ownerMRN string, cache *bundleCache) error { func ResolveFramework(mrn string, frameworks map[string]*Framework) *ResolvedFramework { res := &ResolvedFramework{ Mrn: mrn, - ReportTargets: map[string][]string{}, - ReportSources: map[string][]string{}, + ReportTargets: map[string]ResolvedFrameworkReferenceSet{}, + ReportSources: map[string]ResolvedFrameworkReferenceSet{}, Nodes: map[string]ResolvedFrameworkNode{}, } @@ -585,19 +591,16 @@ func (r *ResolvedFramework) addControl(control *ControlMap) { func (r *ResolvedFramework) addReportLink(parent, child ResolvedFrameworkNode) { r.Nodes[parent.Mrn] = parent r.Nodes[child.Mrn] = child - existing, ok := r.ReportTargets[child.Mrn] - if !ok { - r.ReportTargets[child.Mrn] = []string{parent.Mrn} - } else { - r.ReportTargets[child.Mrn] = append(existing, parent.Mrn) - } - existing, ok = r.ReportSources[parent.Mrn] - if !ok { - r.ReportSources[parent.Mrn] = []string{child.Mrn} - } else { - r.ReportSources[parent.Mrn] = append(existing, child.Mrn) + if r.ReportTargets[child.Mrn] == nil { + r.ReportTargets[child.Mrn] = ResolvedFrameworkReferenceSet{} + } + if r.ReportSources[parent.Mrn] == nil { + r.ReportSources[parent.Mrn] = ResolvedFrameworkReferenceSet{} } + + r.ReportTargets[child.Mrn].Add(parent.Mrn) + r.ReportSources[parent.Mrn].Add(child.Mrn) } func (r *ResolvedFramework) TopologicalSort() []string { @@ -631,8 +634,10 @@ func (r *ResolvedFramework) visit(node string, visited map[string]struct{}, sort return } visited[node] = struct{}{} - for _, child := range r.ReportTargets[node] { + for child := range r.ReportTargets[node] { r.visit(child, visited, sorted) + } + *sorted = append(*sorted, node) } diff --git a/policy/frameworks_test.go b/policy/frameworks_test.go index 409a3e1b..b70ac399 100644 --- a/policy/frameworks_test.go +++ b/policy/frameworks_test.go @@ -6,8 +6,8 @@ import ( func TestResolvedFrameworkTopologicalSort(t *testing.T) { framework := &ResolvedFramework{ - ReportTargets: map[string][]string{}, - ReportSources: map[string][]string{}, + ReportTargets: map[string]ResolvedFrameworkReferenceSet{}, + ReportSources: map[string]ResolvedFrameworkReferenceSet{}, Nodes: map[string]ResolvedFrameworkNode{}, } diff --git a/policy/resolver.go b/policy/resolver.go index 93c2177c..775a21a1 100644 --- a/policy/resolver.go +++ b/policy/resolver.go @@ -1425,7 +1425,7 @@ func ensureControlJob(cache *frameworkResolverCache, jobs map[string]*ReportingJ jobs[uuid] = controlJob parents := framework.ReportTargets[controlMrn] - for _, parentMrn := range parents { + for parentMrn := range parents { parentUuid := cache.relativeChecksum(parentMrn) frameworkJob, ok := cache.frameworkJobsByMrn[parentMrn] @@ -1455,7 +1455,7 @@ func (s *LocalServices) jobsToFrameworks(cache *frameworkResolverCache, resolved } func (s *LocalServices) jobsToFrameworksInner(cache *frameworkResolverCache, resolvedFramework *ResolvedFramework, job *CollectorJob, frameworkMrn string, parent *ReportingJob) error { - for _, source := range resolvedFramework.ReportSources[frameworkMrn] { + for source := range resolvedFramework.ReportSources[frameworkMrn] { if childFramework, ok := cache.bundleMap.Frameworks[source]; ok { var reportingJob *ReportingJob if found, ok := cache.frameworkJobsByMrn[childFramework.Mrn]; ok { @@ -1577,7 +1577,7 @@ func (s *LocalServices) jobsToControls(cache *frameworkResolverCache, framework // Avoid adding controls which don't have any active children shouldAdd := false - for _, child := range framework.ReportSources[mrn] { + for child := range framework.ReportSources[mrn] { if _, ok := nuJobs[cache.relativeChecksum(child)]; ok { shouldAdd = true break @@ -1589,14 +1589,13 @@ func (s *LocalServices) jobsToControls(cache *frameworkResolverCache, framework } controlJob := ensureControlJob(cache, nuJobs, mrn, framework, frameworkGroupByControlMrn) - // addedControlJobs[mrn] = controlJob curJob = controlJob case ResolvedFrameworkNodeTypeFramework: curJob = job.ReportingJobs[cache.relativeChecksum(mrn)] } // Ensure that child jobs notify their parents - for _, child := range framework.ReportSources[mrn] { + for child := range framework.ReportSources[mrn] { childJob, ok := nuJobs[cache.relativeChecksum(child)] if !ok { continue diff --git a/policy/resolver_test.go b/policy/resolver_test.go index b5a1aa94..6ba79910 100644 --- a/policy/resolver_test.go +++ b/policy/resolver_test.go @@ -364,6 +364,8 @@ frameworks: title: control3 - uid: control4 title: control4 + - uid: control5 + title: control5 - uid: framework2 name: framework2 groups: @@ -394,6 +396,18 @@ framework_maps: - uid: control4 controls: - uid: control1 +- uid: framework-map2 + framework_owner: + uid: framework1 + policy_dependencies: + - uid: policy1 + controls: + - uid: control4 + controls: + - uid: control1 + - uid: control5 + controls: + - uid: control1 ` t.Run("resolve with correct filters", func(t *testing.T) { @@ -421,6 +435,11 @@ framework_maps: require.NoError(t, err) require.NotNil(t, rp) + // Check that there are no duplicates in the reporting job's notify list + for _, rj := range rp.CollectorJob.ReportingJobs { + requireUnique(t, rj.Notify) + } + require.Len(t, rp.ExecutionJob.Queries, 3) rjTester := frameworkReportingJobTester{ @@ -450,6 +469,7 @@ framework_maps: rjTester.requireReportsTo(controlMrn("control1"), controlMrn("control4")) rjTester.requireReportsTo(controlMrn("control2"), frameworkMrn("framework1")) rjTester.requireReportsTo(controlMrn("control4"), frameworkMrn("framework1")) + rjTester.requireReportsTo(controlMrn("control5"), frameworkMrn("framework1")) rjTester.requireReportsTo(frameworkMrn("framework1"), frameworkMrn("parent-framework")) rjTester.requireReportsTo(frameworkMrn("parent-framework"), "root") @@ -796,3 +816,13 @@ framework_maps: require.Len(t, frameworkJob.ChildJobs, 3) }) } + +func requireUnique(t *testing.T, items []string) { + seen := make(map[string]bool) + for _, item := range items { + if seen[item] { + t.Errorf("duplicate item found: %s", item) + } + seen[item] = true + } +}