From da5b8f59aac6f748b19f34b006a83f7a5e0e9362 Mon Sep 17 00:00:00 2001 From: Vincent Shen Date: Thu, 8 Feb 2024 16:21:25 -0800 Subject: [PATCH 1/2] OCPBUGS-29272: Delete scan when SSB remove a profile This pr fixes a issue when a profile gets removed from scansettingbinding, the old scan was not deleted when a profile is removed from the existing scansettingbinding, this pr checks that and does the removal so that new scan using that profile can be launch correctly. check OCPBUGS-29272: https://issues.redhat.com/browse/OCPBUGS-29272 --- .../compliancesuite_controller.go | 25 +++- tests/e2e/framework/common.go | 10 ++ tests/e2e/serial/main_test.go | 109 ++++++++++++++++++ 3 files changed, 143 insertions(+), 1 deletion(-) diff --git a/pkg/controller/compliancesuite/compliancesuite_controller.go b/pkg/controller/compliancesuite/compliancesuite_controller.go index 609568997..7919640ed 100644 --- a/pkg/controller/compliancesuite/compliancesuite_controller.go +++ b/pkg/controller/compliancesuite/compliancesuite_controller.go @@ -229,6 +229,7 @@ func (r *ReconcileComplianceSuite) issueValidationError(suite *compv1alpha1.Comp } func (r *ReconcileComplianceSuite) reconcileScans(suite *compv1alpha1.ComplianceSuite, logger logr.Logger) (bool, error) { + requiredScansNames := make(map[string]bool) for idx := range suite.Spec.Scans { scanWrap := &suite.Spec.Scans[idx] scan := &compv1alpha1.ComplianceScan{} @@ -257,8 +258,30 @@ func (r *ReconcileComplianceSuite) reconcileScans(suite *compv1alpha1.Compliance if rescheduleWithDelay || err != nil { return rescheduleWithDelay, err } + requiredScansNames[scan.Name] = true + } + + // check all the scans owned by the suite and see if they are still in the spec + // if not, delete them + scanList := &compv1alpha1.ComplianceScanList{} + listOpts := client.ListOptions{ + LabelSelector: labels.SelectorFromSet(labels.Set{compv1alpha1.SuiteLabel: suite.Name}), + } + if err := r.Client.List(context.TODO(), scanList, &listOpts); err != nil { + return false, err + } - // FIXME: delete scans that went away + for _, scan := range scanList.Items { + if _, ok := requiredScansNames[scan.Name]; !ok { + logger.Info("Deleting scan due to it no longer being in the suite", "ComplianceScan.Name", scan.Name) + r.Recorder.Eventf( + suite, corev1.EventTypeNormal, "SuiteScanDeleted", + "Scan %s is being deleted due to it no longer being in the suite", scan.Name) + // delete the scan + if err := r.Client.Delete(context.TODO(), &scan); err != nil { + return false, err + } + } } return false, nil diff --git a/tests/e2e/framework/common.go b/tests/e2e/framework/common.go index 623d1a6d1..3a435db85 100644 --- a/tests/e2e/framework/common.go +++ b/tests/e2e/framework/common.go @@ -1125,6 +1125,16 @@ func (f *Framework) AssertScanHasValidPVCReferenceWithSize(scanName, size, names return nil } +func (f *Framework) AssertScanExists(name, namespace string) error { + cs := &compv1alpha1.ComplianceScan{} + defer f.logContainerOutput(namespace, name) + err := f.Client.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: namespace}, cs) + if apierrors.IsNotFound(err) { + return fmt.Errorf("Failed to assert ComplianceScan %s exists: %w", name, err) + } + return err +} + func (f *Framework) AssertScanDoesNotExist(name, namespace string) error { cs := &compv1alpha1.ComplianceScan{} defer f.logContainerOutput(namespace, name) diff --git a/tests/e2e/serial/main_test.go b/tests/e2e/serial/main_test.go index b47627e22..c6a1b9785 100644 --- a/tests/e2e/serial/main_test.go +++ b/tests/e2e/serial/main_test.go @@ -1520,6 +1520,115 @@ func TestSuspendScanSetting(t *testing.T) { } } +func TestRemoveProfileScan(t *testing.T) { + f := framework.Global + // Bind the new ScanSetting to a Profile + bindingName := framework.GetObjNameFromTest(t) + "-binding" + scanSettingBinding := compv1alpha1.ScanSettingBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: bindingName, + Namespace: f.OperatorNamespace, + }, + Profiles: []compv1alpha1.NamedObjectReference{ + { + Name: "ocp4-cis", + Kind: "Profile", + APIGroup: "compliance.openshift.io/v1alpha1", + }, + { + Name: "ocp4-moderate", + Kind: "Profile", + APIGroup: "compliance.openshift.io/v1alpha1", + }, + }, + SettingsRef: &compv1alpha1.NamedObjectReference{ + Name: "default", + Kind: "ScanSetting", + APIGroup: "compliance.openshift.io/v1alpha1", + }, + } + if err := f.Client.Create(context.TODO(), &scanSettingBinding, nil); err != nil { + t.Fatal(err) + } + defer f.Client.Delete(context.TODO(), &scanSettingBinding) + + if err := f.WaitForSuiteScansStatus(f.OperatorNamespace, bindingName, compv1alpha1.PhaseDone, compv1alpha1.ResultNonCompliant); err != nil { + t.Fatal(err) + } + + // AssertScanExists check if the scan exists for both profiles + if err := f.AssertScanExists("ocp4-cis", f.OperatorNamespace); err != nil { + t.Fatal(err) + } + + if err := f.AssertScanExists("ocp4-moderate", f.OperatorNamespace); err != nil { + t.Fatal(err) + } + + scanName := "ocp4-moderate" + checkResult := compv1alpha1.ComplianceCheckResult{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s-audit-profile-set", scanName), + Namespace: f.OperatorNamespace, + }, + ID: "xccdf_org.ssgproject.content_rule_audit_profile_set", + Status: compv1alpha1.CheckResultPass, + Severity: compv1alpha1.CheckResultSeverityMedium, + } + err := f.AssertHasCheck(bindingName, scanName, checkResult) + if err != nil { + t.Fatal(err) + } + + // Remove the `ocp4-moderate` profile from the `ScanSettingBinding` + scanSettingBindingUpdate := &compv1alpha1.ScanSettingBinding{} + if err := f.Client.Get(context.TODO(), types.NamespacedName{Namespace: f.OperatorNamespace, Name: bindingName}, scanSettingBindingUpdate); err != nil { + t.Fatalf("failed to get ScanSettingBinding %s", bindingName) + } + + scanSettingBindingUpdate.Profiles = []compv1alpha1.NamedObjectReference{ + { + Name: "ocp4-cis", + Kind: "Profile", + APIGroup: "compliance.openshift.io/v1alpha1", + }, + } + if err := f.Client.Update(context.TODO(), scanSettingBindingUpdate); err != nil { + t.Fatal(err) + } + + var lastErr error + timeouterr := wait.Poll(framework.RetryInterval, framework.Timeout, func() (bool, error) { + if lastErr := f.AssertScanDoesNotExist(scanName, f.OperatorNamespace); lastErr != nil { + log.Printf("Retrying: %s\n", lastErr) + return false, nil + } + if lastErr := f.AssertScanDoesNotContainCheck(scanName, checkResult.Name, f.OperatorNamespace); lastErr != nil { + log.Printf("Retrying: %s\n", lastErr) + // print more info about the found check + ccr := &compv1alpha1.ComplianceCheckResult{} + if err := f.Client.Get(context.TODO(), types.NamespacedName{Namespace: f.OperatorNamespace, Name: checkResult.Name}, ccr); err != nil { + log.Printf("failed to get check %s: %s\n", checkResult.Name, err) + } else { + log.Printf("Object: %v\n", ccr) + } + return false, nil + } + log.Printf("Scan %s doesn't exist anymore\n", scanName) + log.Printf("Check %s doesn't exist anymore\n", checkResult.Name) + return true, nil + }) + + if lastErr != nil { + t.Fatalf("failed to remove profile from ScanSettingBinding: %s", lastErr) + } + + if timeouterr != nil { + t.Fatalf("timed out waiting for scan and check to be removed: %s", timeouterr) + } + +} + func TestSuspendScanSettingDoesNotCreateScan(t *testing.T) { f := framework.Global From 219c377670226970a23d4647e8c8e5b5eb43c7e7 Mon Sep 17 00:00:00 2001 From: Vincent Shen Date: Sun, 3 Mar 2024 23:19:16 -0800 Subject: [PATCH 2/2] Test node profile removal cleanup for SSB --- tests/e2e/serial/main_test.go | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/tests/e2e/serial/main_test.go b/tests/e2e/serial/main_test.go index c6a1b9785..8282113a4 100644 --- a/tests/e2e/serial/main_test.go +++ b/tests/e2e/serial/main_test.go @@ -1540,6 +1540,11 @@ func TestRemoveProfileScan(t *testing.T) { Kind: "Profile", APIGroup: "compliance.openshift.io/v1alpha1", }, + { + Name: "ocp4-cis-node", + Kind: "Profile", + APIGroup: "compliance.openshift.io/v1alpha1", + }, }, SettingsRef: &compv1alpha1.NamedObjectReference{ Name: "default", @@ -1565,6 +1570,14 @@ func TestRemoveProfileScan(t *testing.T) { t.Fatal(err) } + if err := f.AssertScanExists("ocp4-cis-node-master", f.OperatorNamespace); err != nil { + t.Fatal(err) + } + + if err := f.AssertScanExists("ocp4-cis-node-worker", f.OperatorNamespace); err != nil { + t.Fatal(err) + } + scanName := "ocp4-moderate" checkResult := compv1alpha1.ComplianceCheckResult{ ObjectMeta: metav1.ObjectMeta{ @@ -1580,7 +1593,7 @@ func TestRemoveProfileScan(t *testing.T) { t.Fatal(err) } - // Remove the `ocp4-moderate` profile from the `ScanSettingBinding` + // Remove the `ocp4-moderate` and `ocp4-cis-node` profile from the `ScanSettingBinding` scanSettingBindingUpdate := &compv1alpha1.ScanSettingBinding{} if err := f.Client.Get(context.TODO(), types.NamespacedName{Namespace: f.OperatorNamespace, Name: bindingName}, scanSettingBindingUpdate); err != nil { t.Fatalf("failed to get ScanSettingBinding %s", bindingName) @@ -1596,10 +1609,17 @@ func TestRemoveProfileScan(t *testing.T) { if err := f.Client.Update(context.TODO(), scanSettingBindingUpdate); err != nil { t.Fatal(err) } - var lastErr error timeouterr := wait.Poll(framework.RetryInterval, framework.Timeout, func() (bool, error) { - if lastErr := f.AssertScanDoesNotExist(scanName, f.OperatorNamespace); lastErr != nil { + if lastErr := f.AssertScanDoesNotExist("ocp4-moderate", f.OperatorNamespace); lastErr != nil { + log.Printf("Retrying: %s\n", lastErr) + return false, nil + } + if lastErr := f.AssertScanDoesNotExist("ocp4-cis-node-master", f.OperatorNamespace); lastErr != nil { + log.Printf("Retrying: %s\n", lastErr) + return false, nil + } + if lastErr := f.AssertScanDoesNotExist("ocp4-cis-node-worker", f.OperatorNamespace); lastErr != nil { log.Printf("Retrying: %s\n", lastErr) return false, nil } @@ -1614,7 +1634,7 @@ func TestRemoveProfileScan(t *testing.T) { } return false, nil } - log.Printf("Scan %s doesn't exist anymore\n", scanName) + log.Print("Scan ocp4-moderate, ocp4-cis-node-master and ocp4-cis-node-worker do not exist anymore\n") log.Printf("Check %s doesn't exist anymore\n", checkResult.Name) return true, nil }) @@ -1626,7 +1646,6 @@ func TestRemoveProfileScan(t *testing.T) { if timeouterr != nil { t.Fatalf("timed out waiting for scan and check to be removed: %s", timeouterr) } - } func TestSuspendScanSettingDoesNotCreateScan(t *testing.T) {