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

CMP-2615: Add a check aggregate to the compliance scan metadata #588

Merged
merged 1 commit into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/apis/compliance/v1alpha1/compliancescan_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ const ComplianceScanRescanAnnotation = "compliance.openshift.io/rescan"
// "api-checks" in the annotation.
const ComplianceScanTimeoutAnnotation = "compliance.openshift.io/timeout"

// ComplianceCheckCountAnnotation indicates the number of checks
// that a ComplianceScan has generated during the scan
const ComplianceCheckCountAnnotation = "compliance.openshift.io/check-count"

// ComplianceScanLabel serves as an indicator for which ComplianceScan
// owns the referenced object
const ComplianceScanLabel = "compliance.openshift.io/scan-name"
Expand Down
34 changes: 34 additions & 0 deletions pkg/controller/compliancescan/compliancescan_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"io/ioutil"
"math"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -286,6 +287,7 @@ func (r *ReconcileComplianceScan) phasePendingHandler(instance *compv1alpha1.Com
// Remove annotation if needed
if instance.NeedsRescan() {
instanceCopy := instance.DeepCopy()
delete(instanceCopy.Annotations, compv1alpha1.ComplianceCheckCountAnnotation)
delete(instanceCopy.Annotations, compv1alpha1.ComplianceScanRescanAnnotation)
delete(instanceCopy.Annotations, compv1alpha1.ComplianceScanTimeoutAnnotation)
err := r.Client.Update(context.TODO(), instanceCopy)
Expand Down Expand Up @@ -649,6 +651,26 @@ func (r *ReconcileComplianceScan) phaseAggregatingHandler(h scanTypeHandler, log
instance.Status.ErrorMessage = err.Error()
}

// count the number of checks that were run
checkCount, err := r.fetchResultCounts(instance, logger)
if err != nil {
logger.Error(err, "Cannot fetch the number of checks")
rhmdnd marked this conversation as resolved.
Show resolved Hide resolved
return reconcile.Result{}, err
}

instanceCopy := instance.DeepCopy()

if instanceCopy.Annotations == nil {
instanceCopy.Annotations = make(map[string]string)
}

// adding check count annotation
instanceCopy.Annotations[compv1alpha1.ComplianceCheckCountAnnotation] = strconv.Itoa(checkCount)

if err := r.Client.Update(context.TODO(), instanceCopy); err != nil {
logger.Error(err, "Cannot update the scan with the check count")
rhmdnd marked this conversation as resolved.
Show resolved Hide resolved
return reconcile.Result{}, err
}
instance.Status.Phase = compv1alpha1.PhaseDone
instance.Status.EndTimestamp = &metav1.Time{Time: time.Now()}
instance.Status.SetConditionReady()
Expand All @@ -661,6 +683,18 @@ func (r *ReconcileComplianceScan) phaseAggregatingHandler(h scanTypeHandler, log
return reconcile.Result{}, nil
}

func (r *ReconcileComplianceScan) fetchResultCounts(scan *compv1alpha1.ComplianceScan, logger logr.Logger) (int, error) {
var checkList compv1alpha1.ComplianceCheckResultList
checkListOpts := client.MatchingLabels{
compv1alpha1.ComplianceScanLabel: scan.Name,
}
if err := r.Client.List(context.TODO(), &checkList, &checkListOpts); err != nil {
logger.Error(err, "Cannot list the check results")
rhmdnd marked this conversation as resolved.
Show resolved Hide resolved
return 0, err
}
return len(checkList.Items), nil
}

func (r *ReconcileComplianceScan) phaseDoneHandler(h scanTypeHandler, instance *compv1alpha1.ComplianceScan, logger logr.Logger, doDelete bool) (reconcile.Result, error) {
var err error
logger.Info("Phase: Done")
Expand Down
7 changes: 7 additions & 0 deletions tests/e2e/framework/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,13 @@ func (f *Framework) WaitForScanStatus(namespace, name string, targetStatus compv
return false, nil
}

if exampleComplianceScan.Status.Phase != compv1alpha1.PhaseDone && exampleComplianceScan.Status.Phase != compv1alpha1.PhasePending {
// check we should not have checkCount annotation
if exampleComplianceScan.Annotations[compv1alpha1.ComplianceCheckCountAnnotation] != "" {
return true, fmt.Errorf("compliancescan %s has unset checkCount annotation", name)
}
}

if exampleComplianceScan.Status.Phase == targetStatus {
return true, nil
}
Expand Down
35 changes: 35 additions & 0 deletions tests/e2e/framework/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,41 @@ func (f *Framework) AssertMustHaveParsedProfiles(pbName, productType, productNam
return nil
}

// AssertScanHasTotalCheckCounts asserts that the scan has the expected total check counts
func (f *Framework) AssertScanHasTotalCheckCounts(namespace, scanName string) error {
// check if scan has annotation
var scan compv1alpha1.ComplianceScan
key := types.NamespacedName{Namespace: namespace, Name: scanName}
if err := f.Client.Get(context.Background(), key, &scan); err != nil {
return err
}
if scan.Annotations == nil {
return fmt.Errorf("expected annotations to be not nil")
}
if scan.Annotations[compv1alpha1.ComplianceCheckCountAnnotation] == "" {
return fmt.Errorf("expected %s to be not empty", compv1alpha1.ComplianceCheckCountAnnotation)
}

gotCheckCount, err := strconv.Atoi(scan.Annotations[compv1alpha1.ComplianceCheckCountAnnotation])
if err != nil {
return fmt.Errorf("failed to convert %s to int: %w", compv1alpha1.ComplianceCheckCountAnnotation, err)
}

var checkList compv1alpha1.ComplianceCheckResultList
checkListOpts := client.MatchingLabels{
compv1alpha1.ComplianceScanLabel: scanName,
}
if err := f.Client.List(context.TODO(), &checkList, &checkListOpts); err != nil {
return err
}

if gotCheckCount != len(checkList.Items) {
return fmt.Errorf("expected %s to be %d, got %d instead", compv1alpha1.ComplianceCheckCountAnnotation, len(checkList.Items), gotCheckCount)
}

return nil
}

// AssertRuleCheckTypeChangedAnnotationKey asserts that the rule check type changed annotation key exists
func (f *Framework) AssertRuleCheckTypeChangedAnnotationKey(namespace, ruleName, lastCheckType string) error {
var r compv1alpha1.Rule
Expand Down
96 changes: 96 additions & 0 deletions tests/e2e/serial/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,104 @@ func TestSuiteScan(t *testing.T) {
f.AssertCheckRemediation(checkVsyscall.Name, checkVsyscall.Namespace, true)
}

// ensure scan has total check counts annotation
err = f.AssertScanHasTotalCheckCounts(f.OperatorNamespace, workerScanName)
Copy link

Choose a reason for hiding this comment

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

We could assert each scan in the suite has this, too.

Somewhat related, it would be nice to have an E2E assertion (or set of assertions) that check the ComplianceScan object that we could use consistently across the suite. If this test is ever refactored, and this assertion goes missing, our test coverage for the check-count annotation goes with it.

Copy link
Author

Choose a reason for hiding this comment

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

somthing like all scans in the suite has that?

Copy link

Choose a reason for hiding this comment

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

Yeah - it could be a part of the assertion to check for a successful or done scan.

if err != nil {
t.Fatal(err)
}

err = f.AssertScanHasTotalCheckCounts(f.OperatorNamespace, masterScanName)
if err != nil {
t.Fatal(err)
}

}

// TestSuiteScanHasCheckCounts tests that the scan has the correct check counts
func TestSuiteScanHasCheckCounts(t *testing.T) {
f := framework.Global
suiteName := "test-suite-two-scans-check-counts"
workerScanName := fmt.Sprintf("%s-workers-scan", suiteName)
selectWorkers := map[string]string{
"node-role.kubernetes.io/worker": "",
}

masterScanName := fmt.Sprintf("%s-masters-scan", suiteName)
selectMasters := map[string]string{
"node-role.kubernetes.io/master": "",
}

exampleComplianceSuite := &compv1alpha1.ComplianceSuite{
ObjectMeta: metav1.ObjectMeta{
Name: suiteName,
Namespace: f.OperatorNamespace,
},
Spec: compv1alpha1.ComplianceSuiteSpec{
ComplianceSuiteSettings: compv1alpha1.ComplianceSuiteSettings{
AutoApplyRemediations: false,
},
Scans: []compv1alpha1.ComplianceScanSpecWrapper{
{
ComplianceScanSpec: compv1alpha1.ComplianceScanSpec{
ContentImage: contentImagePath,
Profile: "xccdf_org.ssgproject.content_profile_moderate",
Content: framework.RhcosContentFile,
NodeSelector: selectWorkers,
ComplianceScanSettings: compv1alpha1.ComplianceScanSettings{
Debug: true,
},
},
Name: workerScanName,
},
{
ComplianceScanSpec: compv1alpha1.ComplianceScanSpec{
ContentImage: contentImagePath,
Profile: "xccdf_org.ssgproject.content_profile_moderate",
Content: framework.RhcosContentFile,
NodeSelector: selectMasters,
ComplianceScanSettings: compv1alpha1.ComplianceScanSettings{
Debug: true,
},
},
Name: masterScanName,
},
},
},
}

err := f.Client.Create(context.TODO(), exampleComplianceSuite, nil)
Copy link

Choose a reason for hiding this comment

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

What's the rationale behind creating a ComplianceSuite directly as opposed to using a ScanSettingBinding?

if err != nil {
t.Fatal(err)
}
defer f.Client.Delete(context.TODO(), exampleComplianceSuite)

// Ensure that all the scans in the suite have finished and are marked as Done
err = f.WaitForSuiteScansStatus(f.OperatorNamespace, suiteName, compv1alpha1.PhaseDone, compv1alpha1.ResultNonCompliant)
if err != nil {
t.Fatal(err)
}

// At this point, both scans should be non-compliant given our current content
Copy link

@rhmdnd rhmdnd Aug 28, 2024

Choose a reason for hiding this comment

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

nit: These two assertions aren't hurting anything, but they're probably not overly useful for this particular test since we really just care about asserting the check counts for each scan.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Agree with Lance.. And actually suggest removing these assertions for NonCompliant scans.
When/if rhcos4 moderate profile starts passing out of the box this test will start failing, XD.

err = f.AssertScanIsNonCompliant(workerScanName, f.OperatorNamespace)
if err != nil {
t.Fatal(err)
}
err = f.AssertScanIsNonCompliant(masterScanName, f.OperatorNamespace)
if err != nil {
t.Fatal(err)
}

// ensure scan has total check counts annotation
err = f.AssertScanHasTotalCheckCounts(f.OperatorNamespace, workerScanName)
if err != nil {
t.Fatal(err)
}

err = f.AssertScanHasTotalCheckCounts(f.OperatorNamespace, masterScanName)
if err != nil {
t.Fatal(err)
}
}
func TestScanHasProfileGUID(t *testing.T) {
f := framework.Global
bindingName := framework.GetObjNameFromTest(t)
Expand Down