OCPBUGS-3009: Prune stale CCRs before aggregating scan results

Previously, the compliance operator would leave CCRs around and then
just overwrite them on subsequent scans. While the most recent scan data
was accurate, because it was overwriting existing check results, it gave
the impression that some changes weren't taking effect.

For example, if you create a tailored profile, run a scan, exclude a
rule, and rerun the scan, it appears the change you just made never took
effect because the result from the rule you ignored still exists.

To avoid this, let's check for any check results at scan time and make
sure we clean them up before we aggregate the new results.

CHANGELOG.md

@@ -13,7 +13,10 @@ Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Fixes
 
--
+- `ComplianceCheckResult` objects are automatically cleaned up on each scan.
+  This prevents the [illusion](https://issues.redhat.com/browse/OCPBUGS-3009)
+  that changes to profiles, like excluding rules, aren't taking effect because
+  stale `ComplianceCheckResult` objects are leftover from previous runs.
 
 ### Internal Changes
 

pkg/controller/compliancescan/compliancescan_controller.go

@@ -421,6 +421,17 @@ func (r *ReconcileComplianceScan) phaseRunningHandler(h scanTypeHandler, logger
 	return reconcile.Result{}, nil
 }
 
+func (r *ReconcileComplianceScan) deleteComplianceCheckResults(scan *compv1alpha1.ComplianceScan, logger logr.Logger) error {
+	logger.Info(fmt.Sprintf("Cleaning up stale ComplianceCheckResults associated with %s before aggregating fresh results.", scan.Name))
+	inNs := client.InNamespace(common.GetComplianceOperatorNamespace())
+	withLabel := client.MatchingLabels{compv1alpha1.ComplianceScanLabel: scan.Name}
+	err := r.Client.DeleteAllOf(context.Background(), &compv1alpha1.ComplianceCheckResult{}, inNs, withLabel)
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
 func (r *ReconcileComplianceScan) updateScanStatusOnTimeout(scan *compv1alpha1.ComplianceScan, timeoutNodes []string, logger logr.Logger) (reconcile.Result, error) {
 	remRetries := scan.Status.RemainingRetries
 	var err error
@@ -604,6 +615,13 @@ func (r *ReconcileComplianceScan) phaseDoneHandler(h scanTypeHandler, instance *
 			return reconcile.Result{}, err
 		}
 
+		// Let's clean up any stale ComplianceCheckResults from
+		// previous scans before we kick off a rescan.
+		if err = r.deleteComplianceCheckResults(instance, logger); err != nil {
+			logger.Info(fmt.Sprintf("Unable to prune stale ComplanceCheckResults for %s", instance.Name), err)
+			return reconcile.Result{}, err
+		}
+
 		if instance.NeedsRescan() {
 			if err = r.deleteResultConfigMaps(instance, logger); err != nil {
 				logger.Error(err, "Cannot delete result ConfigMaps")

pkg/controller/compliancescan/compliancescan_controller_test.go

@@ -109,6 +109,8 @@ var _ = Describe("Testing compliancescan controller phases", func() {
 		objs = append(objs, nodeinstance1, nodeinstance2, caSecret, serverSecret, clientSecret, ns)
 		scheme := scheme.Scheme
 		scheme.AddKnownTypes(compv1alpha1.SchemeGroupVersion, compliancescaninstance)
+		scheme.AddKnownTypes(compv1alpha1.SchemeGroupVersion, &compv1alpha1.ComplianceCheckResult{})
+		scheme.AddKnownTypes(compv1alpha1.SchemeGroupVersion, &compv1alpha1.ComplianceCheckResultList{})
 
 		client := fake.NewFakeClientWithScheme(scheme, objs...)
 		var err error

tests/e2e/framework/common.go

@@ -1451,3 +1451,46 @@ func (f *Framework) AssertHasCheck(suiteName, scanName string, check compv1alpha
 
 	return nil
 }
+
+func (f *Framework) AssertScanDoesNotContainCheck(scanName, checkName, namespace string) error {
+	var getCheck compv1alpha1.ComplianceCheckResult
+	err := f.Client.Get(context.TODO(), types.NamespacedName{Name: checkName, Namespace: namespace}, &getCheck)
+	if apierrors.IsNotFound(err) {
+		// The check doesn't exist at all, which means it also doesn't exist in *any* scans
+		log.Printf("didn't find ComplianceCheckResult %s", checkName)
+		return nil
+	} else if err != nil {
+		return err
+	}
+	// Make sure the check we found actually belongs to the scan we're given. This is extra
+	// validation in case the same check is used across other scans.
+	if getCheck.Labels[compv1alpha1.ComplianceScanLabel] == scanName {
+		return fmt.Errorf("found ComplianceCheckResult %s in ComplianceScan %s", checkName, scanName)
+	}
+	return nil
+}
+
+func (f *Framework) ReRunScan(scanName, namespace string) error {
+	scanKey := types.NamespacedName{Name: scanName, Namespace: namespace}
+	err := backoff.Retry(func() error {
+		foundScan := &compv1alpha1.ComplianceScan{}
+		geterr := f.Client.Get(context.TODO(), scanKey, foundScan)
+		if geterr != nil {
+			return geterr
+		}
+
+		scapCopy := foundScan.DeepCopy()
+		if scapCopy.Annotations == nil {
+			scapCopy.Annotations = make(map[string]string)
+		}
+		scapCopy.Annotations[compv1alpha1.ComplianceScanRescanAnnotation] = ""
+		return f.Client.Update(context.TODO(), scapCopy)
+	}, defaultBackoff)
+
+	if err != nil {
+		return fmt.Errorf("couldn't update scan to re-launch it: %w", err)
+	}
+
+	log.Println("Scan re-launched")
+	return nil
+}

tests/e2e/parallel/main_test.go

@@ -2795,3 +2795,111 @@ func TestScheduledSuiteTimeoutFail(t *testing.T) {
 		t.Fatal("The scan should have the timeout annotation")
 	}
 }
+
+func TestScanCleansUpComplianceCheckResults(t *testing.T) {
+	f := framework.Global
+	t.Parallel()
+
+	tpName := framework.GetObjNameFromTest(t)
+	bindingName := tpName + "-binding"
+
+	// create a tailored profile
+	tp := &compv1alpha1.TailoredProfile{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      tpName,
+			Namespace: f.OperatorNamespace,
+		},
+		Spec: compv1alpha1.TailoredProfileSpec{
+			Title:       tpName,
+			Description: tpName,
+			Extends:     "ocp4-cis",
+		},
+	}
+
+	err := f.Client.Create(context.TODO(), tp, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Client.Delete(context.TODO(), tp)
+
+	// run a scan
+	ssb := compv1alpha1.ScanSettingBinding{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      bindingName,
+			Namespace: f.OperatorNamespace,
+		},
+		Profiles: []compv1alpha1.NamedObjectReference{
+			{
+				Name:     tpName,
+				Kind:     "TailoredProfile",
+				APIGroup: "compliance.openshift.io/v1alpha1",
+			},
+		},
+		SettingsRef: &compv1alpha1.NamedObjectReference{
+			Name:     "default",
+			Kind:     "ScanSetting",
+			APIGroup: "compliance.openshift.io/v1alpha1",
+		},
+	}
+	err = f.Client.Create(context.TODO(), &ssb, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Client.Delete(context.TODO(), &ssb)
+
+	if err := f.WaitForSuiteScansStatus(f.OperatorNamespace, bindingName, compv1alpha1.PhaseDone, compv1alpha1.ResultNonCompliant); err != nil {
+		t.Fatal(err)
+	}
+
+	// verify a compliance check result exists
+	checkName := tpName + "-audit-profile-set"
+	checkResult := compv1alpha1.ComplianceCheckResult{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      checkName,
+			Namespace: f.OperatorNamespace,
+		},
+		ID:       "xccdf_org.ssgproject.content_rule_audit_profile_set",
+		Status:   compv1alpha1.CheckResultPass,
+		Severity: compv1alpha1.CheckResultSeverityMedium,
+	}
+	err = f.AssertHasCheck(bindingName, tpName, checkResult)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// update tailored profile to exclude the rule before we kick off another run
+	tpGet := &compv1alpha1.TailoredProfile{}
+	err = f.Client.Get(context.TODO(), types.NamespacedName{Name: tpName, Namespace: f.OperatorNamespace}, tpGet)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tpUpdate := tpGet.DeepCopy()
+	ruleName := "ocp4-audit-profile-set"
+	tpUpdate.Spec.DisableRules = []compv1alpha1.RuleReferenceSpec{
+		{
+			Name:      ruleName,
+			Rationale: "testing to ensure scan results are cleaned up",
+		},
+	}
+
+	err = f.Client.Update(context.TODO(), tpUpdate)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// rerun the scan
+	err = f.ReRunScan(tpName, f.OperatorNamespace)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := f.WaitForSuiteScansStatus(f.OperatorNamespace, bindingName, compv1alpha1.PhaseDone, compv1alpha1.ResultNonCompliant); err != nil {
+		t.Fatal(err)
+	}
+
+	// verify the compliance check result doesn't exist
+	err = f.AssertScanDoesNotContainCheck(tpName, checkName, f.OperatorNamespace)
+	if err != nil {
+		t.Fatal(err)
+	}
+}