OCPBUGS-3009: Prune stale CCRs before aggregating scan results

CHANGELOG.md

@@ -97,6 +97,10 @@ Versioning](https://semver.org/spec/v2.0.0.html).
 
 - The operator now parses links from the compliance content and renders it in
   custom resources accordingly.
+- `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.
 
 - The operator have the ability to hide warnings for certain failed to fetched
   resources, this is useful when the user does not want to see the warnings

cmd/manager/aggregator.go

@@ -38,6 +38,7 @@ import (
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/version"
@@ -557,6 +558,33 @@ func createResults(crClient aggregatorCrClient, scan *compv1alpha1.ComplianceSca
 	// aggregator need to know it's using gRPC under the hood, probably
 	// not).
 
+	// Find all the existing scan results. As we iterate through the list
+	// of the most recent results below, we should remove entries from the
+	// list of existing results. By the end of the loop, we should have a
+	// list of results that are effectively stale. For example, checks that
+	// have been disabled through a tailored profile. If we don't clean
+	// these up during the aggregation phase, it will look like they're
+	// still valid, even though they're from an old scan.
+	staleComplianceCheckResults := make(map[string]compv1alpha1.ComplianceCheckResult)
+	complianceCheckResults := compv1alpha1.ComplianceCheckResultList{}
+	withLabel := map[string]string{
+		compv1alpha1.ComplianceScanLabel: scan.Name,
+	}
+	lo := runtimeclient.ListOptions{
+		Namespace:     scan.Namespace,
+		LabelSelector: labels.SelectorFromSet(withLabel),
+	}
+	err := crClient.getClient().List(context.TODO(), &complianceCheckResults, &lo)
+	if err != nil {
+		return fmt.Errorf("Unable to fetch existing ComplianceCheckResultList: %w", err)
+	}
+	for _, r := range complianceCheckResults.Items {
+		// Use a map so that we can find specific
+		// ComplianceCheckResults without iterating over the list for
+		// every new result from the latest scan.
+		staleComplianceCheckResults[r.Name] = r
+	}
+
 	for _, pr := range consistentResults {
 		if pr == nil || pr.CheckResult == nil {
 			cmdLog.Info("nil result or result.check, this shouldn't happen")
@@ -592,6 +620,14 @@ func createResults(crClient aggregatorCrClient, scan *compv1alpha1.ComplianceSca
 		if err := createOrUpdateOneResult(crClient, scan, checkResultLabels, checkResultAnnotations, checkResultExists, pr.CheckResult); err != nil {
 			return fmt.Errorf("cannot create or update checkResult %s: %v", pr.CheckResult.Name, err)
 		}
+
+		// Remove the ComplianceCheckResult from the list of stale
+		// results so we don't delete it later.
+		_, ok := staleComplianceCheckResults[foundCheckResult.Name]
+		if ok {
+			delete(staleComplianceCheckResults, foundCheckResult.Name)
+		}
+
 		// Handle forwarding.
 		f.SendComplianceCheckResult(pr.CheckResult)
 
@@ -615,6 +651,18 @@ func createResults(crClient aggregatorCrClient, scan *compv1alpha1.ComplianceSca
 		}
 	}
 
+	// If there are any ComplianceCheckResults left in
+	// staleComplianceCheckResults, they were from previous scans and we
+	// should delete them. Otherwise, we give users the impression changes
+	// they've made to their scans, profiles, or settings haven't taken
+	// effect.
+	for _, result := range staleComplianceCheckResults {
+		err := crClient.getClient().Delete(context.TODO(), &result)
+		if err != nil {
+			return fmt.Errorf("Unable to delete stale ComplianceCheckResult %s: %w", result.Name, err)
+		}
+	}
+
 	return nil
 }
 

config/rbac/remediation_aggregator_role.yaml

@@ -52,8 +52,10 @@ rules:
     verbs:
       - create
       - get
+      - list
       - update
       - patch
+      - delete
   - apiGroups:
       - compliance.openshift.io
     resources:

tests/e2e/framework/common.go

@@ -1647,6 +1647,26 @@ func (f *Framework) AssertCheckRemediation(name, namespace string, shouldHaveRem
 	return nil
 }
 
+func (f *Framework) AssertRemediationExists(name, namespace string) error {
+	var r compv1alpha1.ComplianceRemediation
+	err := f.Client.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: namespace}, &r)
+	if err != nil {
+		return fmt.Errorf("Failed to assert ComplianceRemediation %s exists: %w", name, err)
+	}
+	return nil
+}
+
+func (f *Framework) AssertRemediationDoesNotExists(name, namespace string) error {
+	var r compv1alpha1.ComplianceRemediation
+	err := f.Client.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: namespace}, &r)
+	if apierrors.IsNotFound(err) {
+		return nil
+	} else if err != nil {
+		return fmt.Errorf("Failed to assert ComplianceRemediation %s does not exist: %w", name, err)
+	}
+	return fmt.Errorf("Failed to assert ComplianceRemediation %s does not exist.", name)
+}
+
 func (f *Framework) TaintNode(node *core.Node, taint core.Taint) error {
 	taintedNode := node.DeepCopy()
 	if taintedNode.Spec.Taints == nil {
@@ -2334,3 +2354,21 @@ func (f *Framework) AssertCronJobIsNotSuspended(name string) error {
 	log.Printf("CronJob %s is active", name)
 	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
+}

tests/e2e/parallel/main_test.go

@@ -2855,5 +2855,118 @@ func TestRuleHasProfileAnnotation(t *testing.T) {
 			t.Fatalf("expected to find profile %s in rule %s", profileName, rule.Name)
 		}
 	}
+}
+
+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.CheckResultFail,
+		Severity: compv1alpha1.CheckResultSeverityMedium,
+	}
+	err = f.AssertHasCheck(bindingName, tpName, checkResult)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := f.AssertRemediationExists(checkName, f.OperatorNamespace); 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, which will also
+	// mean the compliance remediation should also be gone
+	if err = f.AssertScanDoesNotContainCheck(tpName, checkName, f.OperatorNamespace); err != nil {
+		t.Fatal(err)
+	}
+	if err = f.AssertRemediationDoesNotExists(checkName, f.OperatorNamespace); err != nil {
+		t.Fatal(err)
+	}
 }

tests/e2e/serial/main_test.go

@@ -401,6 +401,15 @@ func TestAutoRemediate(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// Fetch remediation here so we can clean up the machine config later.
+	// We do this before the rescan takes place because the rescan will
+	// prune the remediation after the check passes.
+	rem := &compv1alpha1.ComplianceRemediation{}
+	err = f.Client.Get(context.TODO(), types.NamespacedName{Name: remName, Namespace: f.OperatorNamespace}, rem)
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	// We can re-run the scan at this moment and check that it's now compliant
 	// and it's reflected in a CheckResult
 	err = f.ReRunScan(scanName, f.OperatorNamespace)
@@ -436,15 +445,9 @@ func TestAutoRemediate(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// The test should not leave junk around, let's remove the MC and wait for the nodes to stabilize
-	// again
-	log.Printf("Removing applied remediation\n")
-	// Fetch remediation here so it can be deleted
-	rem := &compv1alpha1.ComplianceRemediation{}
-	err = f.Client.Get(context.TODO(), types.NamespacedName{Name: remName, Namespace: f.OperatorNamespace}, rem)
-	if err != nil {
-		t.Fatal(err)
-	}
+	// The test should not leave junk around, let's remove the MC and wait
+	// for the nodes to stabilize again
+	log.Printf("Removing applied machine config\n")
 	mcfgToBeDeleted := rem.Spec.Current.Object.DeepCopy()
 	mcfgToBeDeleted.SetName(rem.GetMcName())
 	err = f.Client.Delete(context.TODO(), mcfgToBeDeleted)