From 49c4c8e212d78e6358e227f71209815a4d628902 Mon Sep 17 00:00:00 2001 From: Jannik Hollenbach Date: Wed, 20 Aug 2025 21:18:51 +0200 Subject: [PATCH] Migrate finalizer to avoid warnings from kubernetes about non-recommended finalizer url structure We've been getting this warning logged in the operator for a while now: ```txt {"level":"info","ts":"2025-08-20T19:00:44Z","msg":"metadata.finalizers: \"s3.storage.securecodebox.io\": prefer a domain-qualified finalizer name including a path (/) to avoid accidental conflicts with other finalizer writers"} ``` This should resolve this. Related migration code removal ticket #3225 Signed-off-by: Jannik Hollenbach --- .../execution/scans/scan_controller.go | 70 ++++++-- .../execution/scans/scan_reconciler.go | 18 +- .../execution/scans/scan_reconciler_test.go | 156 ++++++++++++++++++ 3 files changed, 229 insertions(+), 15 deletions(-) diff --git a/operator/controllers/execution/scans/scan_controller.go b/operator/controllers/execution/scans/scan_controller.go index 18b251fddd..696cd3bffd 100644 --- 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 index 77a7185859..0d5622b59a 100644 --- 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 index 8e3a5f1d82..2a43d33288 100644 --- 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(