Repository: openshift/assisted-service (language: python)

Issue:
MGMT-19141: Ensure no finalizer is set for BMHs without the infraenv label
Previously BMAC was setting our finalizer for every BMH in the cluster and only removing them when we either saw the delete agent annotation or the paused annotation. This meant that no other BMHs could be removed without manually removing this finalizer.

This commit ensures the finalizer handling sees the finalizer being removed as a change and sets the "dirty" return flag accordingly and also ensures we never set out finalizer on a BMH without an infraenv label (one we don't own).

## List all the issues related to this PR

Resolves https://issues.redhat.com/browse/MGMT-19141

- [ ] New Feature <!-- new functionality -->
- [ ] Enhancement <!-- refactor, code changes, improvement, that won't add new features -->
- [x] Bug fix
- [ ] Tests
- [ ] Documentation
- [ ] CI/CD <!-- Notice that changes for Dockerfiles/Jenkinsfiles aren't tested in CI due to a known bug. -->

## What environments does this code impact?

- [ ] Automation (CI, tools, etc)
- [ ] Cloud
- [x] Operator Managed Deployments
- [ ] None

## How was this code tested?

Manually tested by deploying to an environment with BMHs for multiple purposes.
Saw that finalizers were no longer being set for BMHs without the infraenv label.

<!-- Please, select one or more if needed: -->

- [ ] assisted-test-infra environment
- [x] dev-scripts environment
- [ ] Reviewer's test appreciated
- [x] Waiting for CI to do a full test run
- [x] Manual (Elaborate on how it was tested)
- [ ] No tests needed

## Checklist

- [x] Title and description added to both, commit and PR.
- [x] Relevant issues have been associated (see [CONTRIBUTING] guide)
- [x] This change does not require a documentation update (docstring, `docs`, README, etc)
- [x] Does this change include unit-tests (note that code changes require unit-tests)

## Reviewers Checklist

- Are the title and description (in both PR and commit) meaningful and clear?
- Is there a bug required (and linked) for this change?
- Should this PR be backported?



Discussion / hints:

Fix patch:
diff --git a/internal/controller/controllers/bmh_agent_controller.go b/internal/controller/controllers/bmh_agent_controller.go
index 43907ae95ec..4f08d38d372 100644
--- a/internal/controller/controllers/bmh_agent_controller.go
+++ b/internal/controller/controllers/bmh_agent_controller.go
@@ -337,21 +337,23 @@ func (r *BMACReconciler) Reconcile(origCtx context.Context, req ctrl.Request) (c
 // Finalizer handling is only run if the user has annotated the BMH and converged flow is enabled,
 // or, when the BMH is annotated with 'paused' annotation.
 func (r *BMACReconciler) handleBMHFinalizer(ctx context.Context, log logrus.FieldLogger, bmh *bmh_v1alpha1.BareMetalHost, agent *aiv1beta1.Agent) reconcileResult {
+	// return immediately and ensure our finalizer doesn't exist if the BMH is not one of ours (has the infraenv label)
+	if _, present := bmh.Labels[BMH_INFRA_ENV_LABEL]; !present {
+		return reconcileComplete{stop: true, dirty: controllerutil.RemoveFinalizer(bmh, BMH_FINALIZER_NAME)}
+	}
+
+	if bmh.ObjectMeta.DeletionTimestamp.IsZero() {
+		return reconcileComplete{dirty: controllerutil.AddFinalizer(bmh, BMH_FINALIZER_NAME)}
+	}
+
 	var removeAgentOnDelete bool
 	if _, has_annotation := bmh.GetAnnotations()[BMH_DELETE_ANNOTATION]; has_annotation && r.ConvergedFlowEnabled {
 		removeAgentOnDelete = true
 	}
 	_, bmhPaused := bmh.GetAnnotations()[BMH_PAUSED_ANNOTATION]
 
-	// Register a finalizer if it's absent
+	// Register a finalizer if it's absent and should be present, remove the finalizer if it's present and should be absent
 	bmhHasFinalizer := funk.ContainsString(bmh.GetFinalizers(), BMH_FINALIZER_NAME)
-	if bmh.ObjectMeta.DeletionTimestamp.IsZero() {
-		if !bmhHasFinalizer {
-			controllerutil.AddFinalizer(bmh, BMH_FINALIZER_NAME)
-			return reconcileComplete{dirty: true}
-		}
-		return reconcileComplete{}
-	}
 
 	// BMH is being deleted and finalizer is gone, ensure detached and paused are gone and return
 	if !bmhHasFinalizer {
@@ -371,8 +373,8 @@ func (r *BMACReconciler) handleBMHFinalizer(ctx context.Context, log logrus.Fiel
 			removeBMHDetachedAnnotation(log, bmh)
 		}
 		removeBMHPausedAnnotation(log, bmh)
-		controllerutil.RemoveFinalizer(bmh, BMH_FINALIZER_NAME)
-		dirty := removeAgentOnDelete || bmhPaused
+		finalizersUpdated := controllerutil.RemoveFinalizer(bmh, BMH_FINALIZER_NAME)
+		dirty := removeAgentOnDelete || bmhPaused || finalizersUpdated
 		return reconcileComplete{stop: true, dirty: dirty}
 	}
 


Test patch:
diff --git a/internal/controller/controllers/bmh_agent_controller_test.go b/internal/controller/controllers/bmh_agent_controller_test.go
index 94e082838e4..aa058d466c6 100644
--- a/internal/controller/controllers/bmh_agent_controller_test.go
+++ b/internal/controller/controllers/bmh_agent_controller_test.go
@@ -62,28 +62,17 @@ var _ = Describe("bmac reconcile", func() {
 		mockCtrl.Finish()
 	})
 
-	It("bmh reconcile: no labels", func() {
+	It("with no labels the finalizer should not be set", func() {
 		host := newBMH("bmh-reconcile", &bmh_v1alpha1.BareMetalHostSpec{})
 		Expect(c.Create(ctx, host)).To(BeNil())
 
 		result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
 		Expect(err).To(BeNil())
 		Expect(result).To(Equal(ctrl.Result{}))
-	})
-
-	It("adds a finalizer to the BMH when it has the management annotation", func() {
-		host := newBMH("bmh-reconcile", &bmh_v1alpha1.BareMetalHostSpec{})
-		host.ObjectMeta.SetAnnotations(map[string]string{BMH_DELETE_ANNOTATION: "true"})
-		Expect(c.Create(ctx, host)).To(BeNil())
-
-		bmhr.ConvergedFlowEnabled = true
-		result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
-		Expect(err).To(BeNil())
-		Expect(result).To(Equal(ctrl.Result{}))
 
 		key := types.NamespacedName{Name: host.Name, Namespace: host.Namespace}
 		Expect(c.Get(ctx, key, host)).To(Succeed())
-		Expect(host.GetFinalizers()).To(ContainElement(BMH_FINALIZER_NAME))
+		Expect(host.GetFinalizers()).ToNot(ContainElement(BMH_FINALIZER_NAME))
 	})
 
 	Describe("queue bmh request for agent", func() {
@@ -273,17 +262,40 @@ var _ = Describe("bmac reconcile", func() {
 				Expect(err).To(BeNil())
 				Expect(updatedHost.ObjectMeta.Annotations).NotTo(HaveKey(BMH_INSPECT_ANNOTATION))
 			})
+
+			It("should not add a finalizer", func() {
+				result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
+				Expect(err).To(BeNil())
+				Expect(result).To(Equal(ctrl.Result{}))
+
+				updatedHost := &bmh_v1alpha1.BareMetalHost{}
+				err = c.Get(ctx, types.NamespacedName{Name: host.Name, Namespace: testNamespace}, updatedHost)
+				Expect(err).To(BeNil())
+				Expect(host.GetFinalizers()).ToNot(ContainElement(BMH_FINALIZER_NAME))
+			})
 		})
 
 		Context("with an existing infraEnv without ISODownloadURL", func() {
-			It("should requeue the reconcile", func() {
+			BeforeEach(func() {
 				infraEnv := newInfraEnvImage("testInfraEnv", testNamespace, v1beta1.InfraEnvSpec{})
 				Expect(c.Create(ctx, infraEnv)).To(BeNil())
+			})
 
+			It("should requeue the reconcile", func() {
 				result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
 				Expect(err).To(BeNil())
 				Expect(result).To(Equal(ctrl.Result{}))
 			})
+
+			It("should add a finalizer", func() {
+				result, err := bmhr.Reconcile(ctx, newBMHRequest(host))
+				Expect(err).To(BeNil())
+				Expect(result).To(Equal(ctrl.Result{}))
+
+				key := types.NamespacedName{Name: host.Name, Namespace: host.Namespace}
+				Expect(c.Get(ctx, key, host)).To(Succeed())
+				Expect(host.GetFinalizers()).To(ContainElement(BMH_FINALIZER_NAME))
+			})
 		})
 
 		Context("with an existing infraEnv with ISODownloadURL", func() {
@@ -2575,6 +2587,7 @@ var _ = Describe("handleBMHFinalizer", func() {
 			SpokeK8sClientFactory: mockClientFactory,
 		}
 		bmh = newBMH("testBMH", &bmh_v1alpha1.BareMetalHostSpec{})
+		bmh.Labels = map[string]string{BMH_INFRA_ENV_LABEL: "testInfraEnv"}
 		bmh.Annotations = map[string]string{BMH_DELETE_ANNOTATION: "true"}
 	})
 
@@ -2681,6 +2694,27 @@ var _ = Describe("handleBMHFinalizer", func() {
 			Expect(err).NotTo(HaveOccurred())
 		})
 
+		It("removes the finalizer without delete annotations or paused", func() {
+			bmh.Annotations = nil
+			bmh.ObjectMeta.Finalizers = []string{BMH_FINALIZER_NAME}
+			res := bmhr.handleBMHFinalizer(ctx, bmhr.Log, bmh, nil)
+			Expect(res.Dirty()).To(BeTrue())
+			Expect(bmh.GetFinalizers()).NotTo(ContainElement(BMH_FINALIZER_NAME))
+			_, err := res.Result()
+			Expect(err).NotTo(HaveOccurred())
+		})
+
+		It("removes the finalizer when infraenv is not set", func() {
+			bmh.Annotations = nil
+			bmh.Labels = nil
+			bmh.ObjectMeta.Finalizers = []string{BMH_FINALIZER_NAME}
+			res := bmhr.handleBMHFinalizer(ctx, bmhr.Log, bmh, nil)
+			Expect(res.Dirty()).To(BeTrue())
+			Expect(bmh.GetFinalizers()).NotTo(ContainElement(BMH_FINALIZER_NAME))
+			_, err := res.Result()
+			Expect(err).NotTo(HaveOccurred())
+		})
+
 		Context("with a matching agent", func() {
 			var (
 				agent           *v1beta1.Agent