secureCodeBox · Reet00 · Aug 25, 2025 · Aug 20, 2025
diff --git a/operator/controllers/execution/scans/scan_controller.go b/operator/controllers/execution/scans/scan_controller.go
@@ -43,7 +43,9 @@ var (
 
 // Finalizer to delete related files in s3 when the scan gets deleted
 // https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#finalizers
-var s3StorageFinalizer = "s3.storage.securecodebox.io"
+var s3StorageFinalizer = "s3.storage.securecodebox.io/scan-files"
+// Legacy finalizer name for backward compatibility during migration
+var s3StorageFinalizerLegacy = "s3.storage.securecodebox.io"
 
 // +kubebuilder:rbac:groups=execution.securecodebox.io,resources=scans,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=execution.securecodebox.io,resources=scans/status,verbs=get;update;patch
@@ -140,23 +142,18 @@ func (r *ScanReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 var errNotFound = "The specified key does not exist."
 
 func (r *ScanReconciler) handleFinalizer(scan *executionv1.Scan) error {
-	if containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizer) {
-		bucketName := os.Getenv("S3_BUCKET")
-		r.Log.V(3).Info("Deleting External Files from FileStorage", "ScanUID", scan.UID)
-
-		rawResultUrl := getPresignedUrlPath(*scan, scan.Status.RawResultFile)
-		err := r.MinioClient.RemoveObject(context.Background(), bucketName, rawResultUrl, minio.RemoveObjectOptions{})
-		if err != nil && err.Error() != errNotFound {
-			return err
-		}
-
-		findingsJsonUrl := getPresignedUrlPath(*scan, "findings.json")
-		err = r.MinioClient.RemoveObject(context.Background(), bucketName, findingsJsonUrl, minio.RemoveObjectOptions{})
+	// Handle migration from legacy finalizer
+	if err := r.migrateFinalizer(scan); err != nil {
+		return err
+	}
 
-		if err != nil && err.Error() != errNotFound {
+	// Check if we have the s3 storage finalizer
+	if containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizer) {
+		if err := r.cleanupS3Files(scan); err != nil {
 			return err
 		}
 
+		// Remove the s3 storage finalizer
 		scan.ObjectMeta.Finalizers = removeString(scan.ObjectMeta.Finalizers, s3StorageFinalizer)
 		if err := r.Update(context.Background(), scan); err != nil {
 			return err
@@ -165,6 +162,51 @@ func (r *ScanReconciler) handleFinalizer(scan *executionv1.Scan) error {
 	return nil
 }
 
+// todo: remove this with v6.0.0
+// migrateFinalizer handles migration from legacy finalizer
+func (r *ScanReconciler) migrateFinalizer(scan *executionv1.Scan) error {
+	if !containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy) {
+		return nil
+	}
+
+	r.Log.Info("Migrating legacy finalizer", "scan", scan.Name, "namespace", scan.Namespace, "legacy", s3StorageFinalizerLegacy, "current", s3StorageFinalizer)
+
+	// Clean up S3 files using legacy finalizer logic
+	if err := r.cleanupS3Files(scan); err != nil {
+		return err
+	}
+
+	// Remove legacy finalizer - no need to add current one since scan is being deleted
+	scan.ObjectMeta.Finalizers = removeString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy)
+	if err := r.Update(context.Background(), scan); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// cleanupS3Files removes scan-related files from S3 storage
+func (r *ScanReconciler) cleanupS3Files(scan *executionv1.Scan) error {
+	bucketName := os.Getenv("S3_BUCKET")
+	r.Log.V(3).Info("Deleting External Files from FileStorage", "ScanUID", scan.UID)
+
+	// Clean up raw results file
+	rawResultUrl := getPresignedUrlPath(*scan, scan.Status.RawResultFile)
+	err := r.MinioClient.RemoveObject(context.Background(), bucketName, rawResultUrl, minio.RemoveObjectOptions{})
+	if err != nil && err.Error() != errNotFound {
+		return err
+	}
+
+	// Clean up findings.json file
+	findingsJsonUrl := getPresignedUrlPath(*scan, "findings.json")
+	err = r.MinioClient.RemoveObject(context.Background(), bucketName, findingsJsonUrl, minio.RemoveObjectOptions{})
+	if err != nil && err.Error() != errNotFound {
+		return err
+	}
+
+	return nil
+}
+
 // PresignedGetURL returns a presigned URL from the s3 (or compatible) serice.
 func (r *ScanReconciler) PresignedGetURL(scan executionv1.Scan, filename string, duration time.Duration) (string, error) {
 	bucketName := os.Getenv("S3_BUCKET")

diff --git a/operator/controllers/execution/scans/scan_reconciler.go b/operator/controllers/execution/scans/scan_reconciler.go
@@ -41,9 +41,25 @@ func (r *ScanReconciler) startScan(scan *executionv1.Scan) error {
 		return nil
 	}
 
-	// Add s3 storage finalizer to scan
+	// Add s3 storage finalizer to scan (and migrate legacy finalizer if needed)
+	updated := false
+
+	// Migrate legacy finalizer for active scans
+	if containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy) && !containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizer) {
+		log.Info("Migrating legacy finalizer for active scan", "legacy", s3StorageFinalizerLegacy, "current", s3StorageFinalizer)
+		scan.ObjectMeta.Finalizers = removeString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy)
+		scan.ObjectMeta.Finalizers = append(scan.ObjectMeta.Finalizers, s3StorageFinalizer)
+		updated = true
+	}
+
+	// Add s3 storage finalizer if it doesn't exist
 	if !containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizer) {
 		scan.ObjectMeta.Finalizers = append(scan.ObjectMeta.Finalizers, s3StorageFinalizer)
+		updated = true
+	}
+
+	// Update scan if finalizers were modified
+	if updated {
 		if err := r.Update(context.Background(), scan); err != nil {
 			return err
 		}

diff --git a/operator/controllers/execution/scans/scan_reconciler_test.go b/operator/controllers/execution/scans/scan_reconciler_test.go
@@ -19,6 +19,162 @@ import (
 var namespace = "test-namespace"
 var reconciler = &ScanReconciler{}
 var _ = Describe("ScanControllers", func() {
+	Context("Finalizer Migration", func() {
+		var scan *executionv1.Scan
+
+		BeforeEach(func() {
+			scan = &executionv1.Scan{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: namespace,
+					Name:      "test-scan",
+					Finalizers: []string{},
+				},
+				Spec: executionv1.ScanSpec{
+					ScanType:   "nmap",
+					Parameters: []string{"example.com"},
+				},
+				Status: executionv1.ScanStatus{
+					RawResultFile: "raw-results.json",
+				},
+			}
+		})
+
+		It("should handle legacy finalizer migration logic", func() {
+			// Set up scan with legacy finalizer
+			scan.ObjectMeta.Finalizers = []string{s3StorageFinalizerLegacy, "other-finalizer"}
+
+			// Test that legacy finalizer is present initially
+			Expect(containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy)).To(BeTrue())
+
+			// Test that it would be detected for migration (without actually running migration
+			// which requires MinioClient setup)
+			hasLegacy := containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy)
+			Expect(hasLegacy).To(BeTrue())
+
+			// Simulate the migration logic manually (what migrateFinalizer would do)
+			if hasLegacy {
+				scan.ObjectMeta.Finalizers = removeString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy)
+			}
+
+			// After migration, legacy finalizer should be removed
+			Expect(containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy)).To(BeFalse())
+			// Other finalizers should remain
+			Expect(containsString(scan.ObjectMeta.Finalizers, "other-finalizer")).To(BeTrue())
+		})
+
+		It("should not migrate when legacy finalizer is not present", func() {
+			// Set up scan without legacy finalizer
+			scan.ObjectMeta.Finalizers = []string{s3StorageFinalizer}
+
+			mockReconciler := &ScanReconciler{}
+			err := mockReconciler.migrateFinalizer(scan)
+
+			// Should return nil (no migration needed)
+			Expect(err).To(BeNil())
+			// Should still have the new finalizer
+			Expect(containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizer)).To(BeTrue())
+		})
+
+		It("should not migrate when no finalizers are present", func() {
+			// Set up scan without any finalizers
+			scan.ObjectMeta.Finalizers = []string{}
+
+			// Test detection logic (no migration needed)
+			hasLegacy := containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy)
+			Expect(hasLegacy).To(BeFalse())
+
+			// Should have no finalizers
+			Expect(len(scan.ObjectMeta.Finalizers)).To(Equal(0))
+		})
+
+		It("should handle active scan migration from legacy finalizer", func() {
+			// Set up scan with legacy finalizer (simulating existing scan)
+			scan.ObjectMeta.Finalizers = []string{s3StorageFinalizerLegacy}
+
+			// Simulate the active scan migration logic from startScan function
+			updated := false
+
+			// Check if migration is needed
+			if containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy) && !containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizer) {
+				scan.ObjectMeta.Finalizers = removeString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy)
+				scan.ObjectMeta.Finalizers = append(scan.ObjectMeta.Finalizers, s3StorageFinalizer)
+				updated = true
+			}
+
+			// Verify migration occurred
+			Expect(updated).To(BeTrue())
+			Expect(containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy)).To(BeFalse())
+			Expect(containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizer)).To(BeTrue())
+		})
+
+		It("should not migrate active scan when current finalizer already exists", func() {
+			// Set up scan with both finalizers (edge case)
+			scan.ObjectMeta.Finalizers = []string{s3StorageFinalizerLegacy, s3StorageFinalizer}
+
+			// Simulate the active scan migration logic
+			updated := false
+
+			// Check migration condition (should not migrate if current finalizer exists)
+			if containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy) && !containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizer) {
+				// This block should not execute
+				updated = true
+			}
+
+			// Verify no migration occurred
+			Expect(updated).To(BeFalse())
+			Expect(containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizerLegacy)).To(BeTrue())
+			Expect(containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizer)).To(BeTrue())
+		})
+
+		It("should add s3 storage finalizer to scan without any finalizers", func() {
+			// Set up scan without finalizers
+			scan.ObjectMeta.Finalizers = []string{}
+
+			// Simulate adding s3 storage finalizer
+			updated := false
+
+			if !containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizer) {
+				scan.ObjectMeta.Finalizers = append(scan.ObjectMeta.Finalizers, s3StorageFinalizer)
+				updated = true
+			}
+
+			// Verify finalizer was added
+			Expect(updated).To(BeTrue())
+			Expect(containsString(scan.ObjectMeta.Finalizers, s3StorageFinalizer)).To(BeTrue())
+			Expect(len(scan.ObjectMeta.Finalizers)).To(Equal(1))
+		})
+	})
+
+	Context("Helper Functions", func() {
+		It("should correctly identify when string contains finalizer", func() {
+			finalizers := []string{"other-finalizer", s3StorageFinalizer, "another-finalizer"}
+			Expect(containsString(finalizers, s3StorageFinalizer)).To(BeTrue())
+			Expect(containsString(finalizers, s3StorageFinalizerLegacy)).To(BeFalse())
+			Expect(containsString(finalizers, "non-existent")).To(BeFalse())
+		})
+
+		It("should correctly remove string from slice", func() {
+			originalSlice := []string{"first", s3StorageFinalizerLegacy, "last"}
+			result := removeString(originalSlice, s3StorageFinalizerLegacy)
+
+			Expect(len(result)).To(Equal(2))
+			Expect(result).To(Equal([]string{"first", "last"}))
+			Expect(containsString(result, s3StorageFinalizerLegacy)).To(BeFalse())
+		})
+
+		It("should handle removing non-existent string", func() {
+			originalSlice := []string{"first", "second", "third"}
+			result := removeString(originalSlice, "non-existent")
+
+			Expect(len(result)).To(Equal(3))
+			Expect(result).To(Equal(originalSlice))
+		})
+
+		It("should handle empty slice", func() {
+			result := removeString([]string{}, "any-string")
+			Expect(len(result)).To(Equal(0))
+		})
+	})
 	Context("checkIfTTLSecondsAfterFinishedIsCompleted", func() {
 		It("should return true if TTLSecondsAfterFinished is set", func() {
 			finishTime := time.Date(