Repository: metal3-io/baremetal-operator
Pull request #935 (2021-07-09 20:24:06)

Create a SecretManager type to manage Secrets
De-duplicate the labelling implementations for BMC creds and user data, into a single SecretManager type in a new secretutils package. Also adds the flexibility to add owner references that are not controller references. As much code as possible related to caching/watching of Secrets is moved into this new package.


Patch:
diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go
index 30c06ab5..8edf57b8 100644
--- a/controllers/metal3.io/baremetalhost_controller.go
+++ b/controllers/metal3.io/baremetalhost_controller.go
@@ -36,7 +36,6 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
@@ -44,6 +43,7 @@ import (
 	"github.com/metal3-io/baremetal-operator/pkg/bmc"
 	"github.com/metal3-io/baremetal-operator/pkg/hardware"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
+	"github.com/metal3-io/baremetal-operator/pkg/secretutils"
 	"github.com/metal3-io/baremetal-operator/pkg/utils"
 )
 
@@ -54,9 +54,6 @@ const (
 	rebootAnnotationPrefix        = "reboot.metal3.io"
 	inspectAnnotationPrefix       = "inspect.metal3.io"
 	hardwareDetailsAnnotation     = inspectAnnotationPrefix + "/hardwaredetails"
-
-	LabelEnvironmentName  = "environment.metal3.io"
-	LabelEnvironmentValue = "baremetal"
 )
 
 // BareMetalHostReconciler reconciles a BareMetalHost object
@@ -815,10 +812,9 @@ func (r *BareMetalHostReconciler) actionPreparing(prov provisioner.Provisioner,
 // Start/continue provisioning if we need to.
 func (r *BareMetalHostReconciler) actionProvisioning(prov provisioner.Provisioner, info *reconcileInfo) actionResult {
 	hostConf := &hostConfigData{
-		host:      info.host,
-		log:       info.log.WithName("host_config_data"),
-		client:    r,
-		apiReader: r.APIReader,
+		host:          info.host,
+		log:           info.log.WithName("host_config_data"),
+		secretManager: r.secretManager(info.log),
 	}
 	info.log.Info("provisioning")
 
@@ -1005,12 +1001,9 @@ func (r *BareMetalHostReconciler) manageHostPower(prov provisioner.Provisioner,
 		"reboot process", desiredPowerOnState != info.host.Spec.Online)
 
 	if desiredPowerOnState {
-		provResult, err = prov.PowerOn(info.host.Status.ErrorType == metal3v1alpha1.PowerManagementError)
+		provResult, err = prov.PowerOn()
 	} else {
-		if info.host.Status.ErrorCount > 0 {
-			desiredRebootMode = metal3v1alpha1.RebootModeHard
-		}
-		provResult, err = prov.PowerOff(desiredRebootMode, info.host.Status.ErrorType == metal3v1alpha1.PowerManagementError)
+		provResult, err = prov.PowerOff(desiredRebootMode)
 	}
 	if err != nil {
 		return actionError{errors.Wrap(err, "failed to manage power state of host")}
@@ -1222,25 +1215,26 @@ func (r *BareMetalHostReconciler) setErrorCondition(request ctrl.Request, host *
 	return
 }
 
+func (r *BareMetalHostReconciler) secretManager(log logr.Logger) secretutils.SecretManager {
+	return secretutils.NewSecretManager(log, r.Client, r.APIReader)
+}
+
 // Retrieve the secret containing the credentials for talking to the BMC.
-func (r *BareMetalHostReconciler) getBMCSecretAndSetOwner(request ctrl.Request, host *metal3v1alpha1.BareMetalHost) (bmcCredsSecret *corev1.Secret, err error) {
+func (r *BareMetalHostReconciler) getBMCSecretAndSetOwner(request ctrl.Request, host *metal3v1alpha1.BareMetalHost) (*corev1.Secret, error) {
 
 	if host.Spec.BMC.CredentialsName == "" {
 		return nil, &EmptyBMCSecretError{message: "The BMC secret reference is empty"}
 	}
 
-	bmcCredsSecret, err = getSecret(r.Client, r.APIReader, host.CredentialsKey())
-	if err != nil {
-		return nil, &ResolveBMCSecretRefError{message: fmt.Sprintf("The BMC secret %s does not exist", host.CredentialsKey())}
-	}
+	reqLogger := r.Log.WithValues("baremetalhost", request.NamespacedName)
+	secretManager := r.secretManager(reqLogger)
 
-	// Make sure the secret has the correct owner as soon as we can.
-	// This can return an SaveBMCSecretOwnerError
-	// which isn't handled causing us to immediately try again
-	// which seems fine as we expect this to be a transient failure
-	err = r.setBMCCredentialsSecretOwner(request, host, bmcCredsSecret)
+	bmcCredsSecret, err := secretManager.AcquireSecret(host.CredentialsKey(), host, true)
 	if err != nil {
-		return bmcCredsSecret, err
+		if k8serrors.IsNotFound(err) {
+			return nil, &ResolveBMCSecretRefError{message: fmt.Sprintf("The BMC secret %s does not exist", host.CredentialsKey())}
+		}
+		return nil, err
 	}
 
 	return bmcCredsSecret, nil
@@ -1288,26 +1282,6 @@ func (r *BareMetalHostReconciler) buildAndValidateBMCCredentials(request ctrl.Re
 	return bmcCreds, bmcCredsSecret, nil
 }
 
-func (r *BareMetalHostReconciler) setBMCCredentialsSecretOwner(request ctrl.Request, host *metal3v1alpha1.BareMetalHost, secret *corev1.Secret) (err error) {
-	reqLogger := r.Log.WithValues("baremetalhost", request.NamespacedName)
-	if metav1.IsControlledBy(secret, host) && metav1.HasLabel(secret.ObjectMeta, LabelEnvironmentName) {
-		return nil
-	}
-	reqLogger.Info("updating owner of secret")
-	err = controllerutil.SetControllerReference(host, secret, r.Scheme())
-	if err != nil {
-		return &SaveBMCSecretOwnerError{message: fmt.Sprintf("cannot set owner: %q", err.Error())}
-	}
-	reqLogger.Info("updating secret environment label", "secret", secret.Name, "namespace", secret.Namespace)
-	metav1.SetMetaDataLabel(&secret.ObjectMeta, LabelEnvironmentName, LabelEnvironmentValue)
-
-	err = r.Update(context.TODO(), secret)
-	if err != nil {
-		return &SaveBMCSecretOwnerError{message: fmt.Sprintf("cannot update secret: %q", err.Error())}
-	}
-	return nil
-}
-
 func (r *BareMetalHostReconciler) publishEvent(request ctrl.Request, event corev1.Event) {
 	reqLogger := r.Log.WithValues("baremetalhost", request.NamespacedName)
 	reqLogger.Info("publishing event", "reason", event.Reason, "message", event.Message)
diff --git a/controllers/metal3.io/errors.go b/controllers/metal3.io/errors.go
index 5d5ff61b..beea98b9 100644
--- a/controllers/metal3.io/errors.go
+++ b/controllers/metal3.io/errors.go
@@ -37,17 +37,6 @@ func (e ResolveBMCSecretRefError) Error() string {
 		e.message)
 }
 
-// SaveBMCSecretOwnerError is returned when we
-// fail to set the owner of a secret
-type SaveBMCSecretOwnerError struct {
-	message string
-}
-
-func (e SaveBMCSecretOwnerError) Error() string {
-	return fmt.Sprintf("Failed to set owner of BMC secret %s",
-		e.message)
-}
-
 // NoDataInSecretError is returned when host configuration
 // data were not found in referenced secret
 type NoDataInSecretError struct {
diff --git a/controllers/metal3.io/host_config_data.go b/controllers/metal3.io/host_config_data.go
index 73a88287..ab2f330e 100644
--- a/controllers/metal3.io/host_config_data.go
+++ b/controllers/metal3.io/host_config_data.go
@@ -1,54 +1,19 @@
 package controllers
 
 import (
-	"context"
-	"fmt"
-
 	"github.com/go-logr/logr"
-	"github.com/pkg/errors"
-	corev1 "k8s.io/api/core/v1"
-	k8serrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
+	"github.com/metal3-io/baremetal-operator/pkg/secretutils"
 )
 
 // hostConfigData is an implementation of host configuration data interface.
 // Object is able to retrive data from secrets referenced in a host spec
 type hostConfigData struct {
-	host      *metal3v1alpha1.BareMetalHost
-	log       logr.Logger
-	client    client.Client
-	apiReader client.Reader
-}
-
-// Generic method for retrieving a secret. If the secret is not found in the
-// filtered cache, then the object is retrieved directly from the API
-func getSecret(client client.Client, apiReader client.Reader, secretKey types.NamespacedName) (secret *corev1.Secret, err error) {
-
-	secret = &corev1.Secret{}
-
-	// Look for secret in the filtered cache
-	err = client.Get(context.TODO(), secretKey, secret)
-	if err == nil {
-		return secret, nil
-	}
-	if !k8serrors.IsNotFound(err) {
-		return nil, err
-	}
-
-	// Secret not in cache; check API directly for unlabelled Secret
-	err = apiReader.Get(context.TODO(), secretKey, secret)
-	if err != nil {
-		if k8serrors.IsNotFound(err) {
-			return nil, err
-		}
-		return nil, err
-	}
-
-	return secret, nil
+	host          *metal3v1alpha1.BareMetalHost
+	log           logr.Logger
+	secretManager secretutils.SecretManager
 }
 
 // Generic method for data extraction from a Secret. Function uses dataKey
@@ -60,19 +25,9 @@ func (hcd *hostConfigData) getSecretData(name, namespace, dataKey string) (strin
 		Namespace: namespace,
 	}
 
-	secret, err := getSecret(hcd.client, hcd.apiReader, key)
+	secret, err := hcd.secretManager.ObtainSecret(key)
 	if err != nil {
-		return "", errors.Wrap(err, fmt.Sprintf("failed to fetch user data from secret %s defined in namespace %s", name, namespace))
-	}
-
-	if !metav1.HasLabel(secret.ObjectMeta, LabelEnvironmentName) {
-		hcd.log.Info("updating secret environment label", "secret", secret.Name, "namespace", secret.Namespace)
-		metav1.SetMetaDataLabel(&secret.ObjectMeta, LabelEnvironmentName, LabelEnvironmentValue)
-
-		err = hcd.client.Update(context.TODO(), secret)
-		if err != nil {
-			return "", errors.Wrap(err, fmt.Sprintf("failed to updated secret %s defined in namespace %s", name, namespace))
-		}
+		return "", err
 	}
 
 	data, ok := secret.Data[dataKey]
diff --git a/main.go b/main.go
index 06831ffa..0b9947e3 100644
--- a/main.go
+++ b/main.go
@@ -22,8 +22,6 @@ import (
 	"os"
 	"runtime"
 
-	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	k8sruntime "k8s.io/apimachinery/pkg/runtime"
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
@@ -33,12 +31,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
 	metal3iov1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
-	controllers "github.com/metal3-io/baremetal-operator/controllers/metal3.io"
 	metal3iocontroller "github.com/metal3-io/baremetal-operator/controllers/metal3.io"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/demo"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/fixture"
 	"github.com/metal3-io/baremetal-operator/pkg/provisioner/ironic"
+	"github.com/metal3-io/baremetal-operator/pkg/secretutils"
 	"github.com/metal3-io/baremetal-operator/pkg/version"
 	// +kubebuilder:scaffold:imports
 )
@@ -123,11 +121,7 @@ func main() {
 		HealthProbeBindAddress:  healthAddr,
 
 		NewCache: cache.BuilderWithOptions(cache.Options{
-			SelectorsByObject: cache.SelectorsByObject{
-				&corev1.Secret{}: {
-					Label: labels.SelectorFromSet(labels.Set{controllers.LabelEnvironmentName: controllers.LabelEnvironmentValue}),
-				},
-			},
+			SelectorsByObject: secretutils.AddSecretSelector(nil),
 		}),
 	})
 	if err != nil {
diff --git a/pkg/provisioner/demo/demo.go b/pkg/provisioner/demo/demo.go
index be79a6f4..00741b33 100644
--- a/pkg/provisioner/demo/demo.go
+++ b/pkg/provisioner/demo/demo.go
@@ -305,7 +305,7 @@ func (p *demoProvisioner) Detach() (result provisioner.Result, err error) {
 
 // PowerOn ensures the server is powered on independently of any image
 // provisioning operation.
-func (p *demoProvisioner) PowerOn(force bool) (result provisioner.Result, err error) {
+func (p *demoProvisioner) PowerOn() (result provisioner.Result, err error) {
 
 	hostName := p.objectMeta.Name
 	switch hostName {
@@ -326,7 +326,7 @@ func (p *demoProvisioner) PowerOn(force bool) (result provisioner.Result, err er
 
 // PowerOff ensures the server is powered off independently of any image
 // provisioning operation.
-func (p *demoProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode, force bool) (result provisioner.Result, err error) {
+func (p *demoProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (result provisioner.Result, err error) {
 
 	hostName := p.objectMeta.Name
 	switch hostName {
diff --git a/pkg/provisioner/fixture/fixture.go b/pkg/provisioner/fixture/fixture.go
index b3eee1cf..ab0b2e21 100644
--- a/pkg/provisioner/fixture/fixture.go
+++ b/pkg/provisioner/fixture/fixture.go
@@ -291,7 +291,7 @@ func (p *fixtureProvisioner) Detach() (result provisioner.Result, err error) {
 
 // PowerOn ensures the server is powered on independently of any image
 // provisioning operation.
-func (p *fixtureProvisioner) PowerOn(force bool) (result provisioner.Result, err error) {
+func (p *fixtureProvisioner) PowerOn() (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is powered on")
 
 	if !p.state.poweredOn {
@@ -307,7 +307,7 @@ func (p *fixtureProvisioner) PowerOn(force bool) (result provisioner.Result, err
 
 // PowerOff ensures the server is powered off independently of any image
 // provisioning operation.
-func (p *fixtureProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode, force bool) (result provisioner.Result, err error) {
+func (p *fixtureProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is powered off")
 
 	if p.state.poweredOn {
diff --git a/pkg/provisioner/ironic/errors.go b/pkg/provisioner/ironic/errors.go
index 8a69c0cb..c3384bcd 100644
--- a/pkg/provisioner/ironic/errors.go
+++ b/pkg/provisioner/ironic/errors.go
@@ -9,6 +9,15 @@ func (e SoftPowerOffUnsupportedError) Error() string {
 	return "soft power off is unsupported on BMC"
 }
 
+// SoftPowerOffFailed is returned when the soft power off command
+// finishes with failure.
+type SoftPowerOffFailed struct {
+}
+
+func (e SoftPowerOffFailed) Error() string {
+	return "Soft power off has failed on BMC"
+}
+
 // HostLockedError is returned when the BMC host is
 // locked.
 type HostLockedError struct {
diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go
index eb42929c..7d4a3580 100644
--- a/pkg/provisioner/ironic/ironic.go
+++ b/pkg/provisioner/ironic/ironic.go
@@ -1519,7 +1519,7 @@ func (p *ironicProvisioner) changePower(ironicNode *nodes.Node, target nodes.Tar
 
 // PowerOn ensures the server is powered on independently of any image
 // provisioning operation.
-func (p *ironicProvisioner) PowerOn(force bool) (result provisioner.Result, err error) {
+func (p *ironicProvisioner) PowerOn() (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is powered on")
 
 	ironicNode, err := p.getNode()
@@ -1535,39 +1535,46 @@ func (p *ironicProvisioner) PowerOn(force bool) (result provisioner.Result, err
 			p.log.Info("waiting for power status to change")
 			return operationContinuing(powerRequeueDelay)
 		}
-		if ironicNode.LastError != "" && !force {
-			p.log.Info("PowerOn operation failed", "msg", ironicNode.LastError)
-			return operationFailed(fmt.Sprintf("PowerOn operation failed: %s",
-				ironicNode.LastError))
-		}
-		if result, err = p.changePower(ironicNode, nodes.PowerOn); err == nil {
-			p.publisher("PowerOn", "Host powered on")
-			return result, nil
-		}
+		result, err = p.changePower(ironicNode, nodes.PowerOn)
 		switch err.(type) {
+		case nil:
 		case HostLockedError:
-			return retryAfterDelay(powerRequeueDelay)
 		default:
-			return transientError(errors.Wrap(err, "failed to PowerOn node"))
+			return transientError(errors.Wrap(err, "failed to power on host"))
 		}
+		p.publisher("PowerOn", "Host powered on")
 	}
+
 	return result, nil
 }
 
 // PowerOff ensures the server is powered off independently of any image
 // provisioning operation.
-func (p *ironicProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode, force bool) (result provisioner.Result, err error) {
+func (p *ironicProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (result provisioner.Result, err error) {
 	p.log.Info(fmt.Sprintf("ensuring host is powered off (mode: %s)", rebootMode))
 
-	if rebootMode == metal3v1alpha1.RebootModeSoft {
-		return p.softPowerOff()
+	if rebootMode == metal3v1alpha1.RebootModeHard {
+		result, err = p.hardPowerOff()
+	} else {
+		result, err = p.softPowerOff()
 	}
-	// Reboot mode is hard or force flag is set
-	return p.hardPowerOff(force)
+	if err != nil {
+		switch err.(type) {
+		// In case of soft power off is unsupported or has failed,
+		// we activate hard power off.
+		case SoftPowerOffUnsupportedError, SoftPowerOffFailed:
+			return p.hardPowerOff()
+		case HostLockedError:
+			return retryAfterDelay(powerRequeueDelay)
+		default:
+			return transientError(err)
+		}
+	}
+	return result, nil
 }
 
 // hardPowerOff sends 'power off' request to BM node and waits for the result
-func (p *ironicProvisioner) hardPowerOff(force bool) (result provisioner.Result, err error) {
+func (p *ironicProvisioner) hardPowerOff() (result provisioner.Result, err error) {
 	p.log.Info("ensuring host is powered off by \"hard power off\" command")
 
 	ironicNode, err := p.getNode()
@@ -1576,22 +1583,13 @@ func (p *ironicProvisioner) hardPowerOff(force bool) (result provisioner.Result,
 	}
 
 	if ironicNode.PowerState != powerOff {
-		if ironicNode.LastError != "" && !force {
-			p.log.Info("hard power off error", "msg", ironicNode.LastError)
-			return operationFailed(ironicNode.LastError)
-		}
 		if ironicNode.TargetPowerState == powerOff {
 			p.log.Info("waiting for power status to change")
 			return operationContinuing(powerRequeueDelay)
 		}
 		result, err = p.changePower(ironicNode, nodes.PowerOff)
 		if err != nil {
-			switch err.(type) {
-			case HostLockedError:
-				return retryAfterDelay(powerRequeueDelay)
-			default:
-				return transientError(errors.Wrap(err, "failed to power off host"))
-			}
+			return transientError(errors.Wrap(err, "failed to power off host"))
 		}
 		p.publisher("PowerOff", "Host powered off")
 		return result, err
@@ -1623,17 +1621,11 @@ func (p *ironicProvisioner) softPowerOff() (result provisioner.Result, err error
 		// If the target state is unset while the last error is set,
 		// then the last execution of soft power off has failed.
 		if targetState == "" && ironicNode.LastError != "" {
-			p.log.Info("soft power off error", "msg", ironicNode.LastError)
-			return operationFailed(ironicNode.LastError)
+			return result, SoftPowerOffFailed{}
 		}
 		result, err = p.changePower(ironicNode, nodes.SoftPowerOff)
 		if err != nil {
-			switch err.(type) {
-			case HostLockedError:
-				return retryAfterDelay(powerRequeueDelay)
-			default:
-				return transientError(errors.Wrap(err, "failed to power off host"))
-			}
+			return transientError(err)
 		}
 		p.publisher("PowerOff", "Host soft powered off")
 	}
diff --git a/pkg/provisioner/provisioner.go b/pkg/provisioner/provisioner.go
index a579e44c..39d241b8 100644
--- a/pkg/provisioner/provisioner.go
+++ b/pkg/provisioner/provisioner.go
@@ -144,12 +144,12 @@ type Provisioner interface {
 
 	// PowerOn ensures the server is powered on independently of any image
 	// provisioning operation.
-	PowerOn(force bool) (result Result, err error)
+	PowerOn() (result Result, err error)
 
 	// PowerOff ensures the server is powered off independently of any image
 	// provisioning operation. The boolean argument may be used to specify
 	// if a hard reboot (force power off) is required - true if so.
-	PowerOff(rebootMode metal3v1alpha1.RebootMode, force bool) (result Result, err error)
+	PowerOff(rebootMode metal3v1alpha1.RebootMode) (result Result, err error)
 
 	// IsReady checks if the provisioning backend is available to accept
 	// all the incoming requests.
diff --git a/pkg/secretutils/label.go b/pkg/secretutils/label.go
new file mode 100644
index 00000000..d50b9d88
--- /dev/null
+++ b/pkg/secretutils/label.go
@@ -0,0 +1,32 @@
+package secretutils
+
+import (
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/labels"
+	"sigs.k8s.io/controller-runtime/pkg/cache"
+)
+
+const (
+	LabelEnvironmentName  = "environment.metal3.io"
+	LabelEnvironmentValue = "baremetal"
+)
+
+// AddSecretSelector adds a selector to a cache.SelectorsByObject that filters
+// Secrets so that only those labelled as part of the baremetal environment get
+// cached. The input may be nil.
+func AddSecretSelector(selectors cache.SelectorsByObject) cache.SelectorsByObject {
+	secret := &corev1.Secret{}
+	newSelectors := cache.SelectorsByObject{
+		secret: {
+			Label: labels.SelectorFromSet(
+				labels.Set{
+					LabelEnvironmentName: LabelEnvironmentValue,
+				}),
+		},
+	}
+	if selectors == nil {
+		return newSelectors
+	}
+	selectors[secret] = newSelectors[secret]
+	return selectors
+}
diff --git a/pkg/secretutils/secret_manager.go b/pkg/secretutils/secret_manager.go
new file mode 100644
index 00000000..b7a525ed
--- /dev/null
+++ b/pkg/secretutils/secret_manager.go
@@ -0,0 +1,140 @@
+package secretutils
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/go-logr/logr"
+	"github.com/pkg/errors"
+	corev1 "k8s.io/api/core/v1"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+)
+
+// SecretManager is a type for fetching Secrets whether or not they are in the
+// client cache, labelling so that they will be included in the client cache,
+// and optionally setting an owner reference.
+type SecretManager struct {
+	log       logr.Logger
+	client    client.Client
+	apiReader client.Reader
+}
+
+// NewSecretManager returns a new SecretManager
+func NewSecretManager(log logr.Logger, cacheClient client.Client, apiReader client.Reader) SecretManager {
+	return SecretManager{
+		log:       log.WithName("secret_manager"),
+		client:    cacheClient,
+		apiReader: apiReader,
+	}
+}
+
+// findSecret retrieves a Secret from the cache if it is available, and from the
+// k8s API if not.
+func (sm *SecretManager) findSecret(key types.NamespacedName) (secret *corev1.Secret, err error) {
+	secret = &corev1.Secret{}
+
+	// Look for secret in the filtered cache
+	err = sm.client.Get(context.TODO(), key, secret)
+	if err == nil {
+		return secret, nil
+	}
+	if !k8serrors.IsNotFound(err) {
+		return nil, err
+	}
+
+	// Secret not in cache; check API directly for unlabelled Secret
+	err = sm.apiReader.Get(context.TODO(), key, secret)
+	if err != nil {
+		return nil, err
+	}
+
+	return secret, nil
+}
+
+// claimSecret ensures that the Secret has a label that will ensure it is
+// present in the cache (and that we can watch for changes), and optionally
+// that it has a particular owner reference.
+func (sm *SecretManager) claimSecret(secret *corev1.Secret, owner client.Object, ownerIsController bool) error {
+	log := sm.log.WithValues("secret", secret.Name, "secretNamespace", secret.Namespace)
+	needsUpdate := false
+	if !metav1.HasLabel(secret.ObjectMeta, LabelEnvironmentName) {
+		log.Info("settting secret environment label")
+		metav1.SetMetaDataLabel(&secret.ObjectMeta, LabelEnvironmentName, LabelEnvironmentValue)
+		needsUpdate = true
+	}
+	if owner != nil {
+		ownerLog := log.WithValues(
+			"ownerKind", owner.GetObjectKind().GroupVersionKind().Kind,
+			"owner", owner.GetNamespace()+"/"+owner.GetName(),
+			"ownerUID", owner.GetUID())
+		if ownerIsController {
+			if !metav1.IsControlledBy(secret, owner) {
+				ownerLog.Info("setting secret controller reference")
+				if err := controllerutil.SetControllerReference(owner, secret, sm.client.Scheme()); err != nil {
+					return errors.Wrap(err, "failed to set secret controller reference")
+				}
+				needsUpdate = true
+			}
+		} else {
+			alreadyOwned := false
+			ownerUID := owner.GetUID()
+			for _, ref := range secret.GetOwnerReferences() {
+				if ref.UID == ownerUID {
+					alreadyOwned = true
+					break
+				}
+			}
+			if !alreadyOwned {
+				ownerLog.Info("setting secret owner reference")
+				if err := controllerutil.SetOwnerReference(owner, secret, sm.client.Scheme()); err != nil {
+					return errors.Wrap(err, "failed to set secret owner reference")
+				}
+				needsUpdate = true
+			}
+		}
+	}
+
+	if needsUpdate {
+		if err := sm.client.Update(context.TODO(), secret); err != nil {
+			return errors.Wrap(err, fmt.Sprintf("failed to update secret %s in namespace %s", secret.ObjectMeta.Name, secret.ObjectMeta.Namespace))
+		}
+	}
+
+	return nil
+}
+
+// obtainSecretForOwner retrieves a Secret and ensures that it has a label that
+// will ensure it is present in the cache (and that we can watch for changes),
+// and optionally that it has a particular owner reference. The owner reference
+// may optionally be a controller reference.
+func (sm *SecretManager) obtainSecretForOwner(key types.NamespacedName, owner client.Object, ownerIsController bool) (*corev1.Secret, error) {
+	secret, err := sm.findSecret(key)
+	if err != nil {
+		return nil, errors.Wrap(err, fmt.Sprintf("failed to fetch secret %s in namespace %s", key.Name, key.Namespace))
+	}
+	err = sm.claimSecret(secret, owner, ownerIsController)
+
+	return secret, err
+}
+
+// AcquireSecret retrieves a Secret and ensures that it has a label that will
+// ensure it is present in the cache (and that we can watch for changes), and
+// that it has a particular owner reference. The owner reference may optionally
+// be a controller reference.
+func (sm *SecretManager) AcquireSecret(key types.NamespacedName, owner client.Object, ownerIsController bool) (*corev1.Secret, error) {
+	if owner == nil {
+		panic("AcquireSecret called with no owner")
+	}
+
+	return sm.obtainSecretForOwner(key, owner, ownerIsController)
+}
+
+// ObtainSecret retrieves a Secret and ensures that it has a label that will
+// ensure it is present in the cache (and that we can watch for changes).
+func (sm *SecretManager) ObtainSecret(key types.NamespacedName) (*corev1.Secret, error) {
+	return sm.obtainSecretForOwner(key, nil, false)
+}


Test patch:
diff --git a/controllers/metal3.io/baremetalhost_controller_test.go b/controllers/metal3.io/baremetalhost_controller_test.go
index 09a9ae07..3a5b05b4 100644
--- a/controllers/metal3.io/baremetalhost_controller_test.go
+++ b/controllers/metal3.io/baremetalhost_controller_test.go
@@ -705,7 +705,7 @@ func TestSecretUpdateOwnerRefAndEnvironmentLabelOnStartup(t *testing.T) {
 	assert.True(t, *secret.OwnerReferences[0].Controller)
 	assert.True(t, *secret.OwnerReferences[0].BlockOwnerDeletion)
 
-	assert.Equal(t, LabelEnvironmentValue, secret.Labels[LabelEnvironmentName])
+	assert.Equal(t, "baremetal", secret.Labels["environment.metal3.io"])
 
 }
 
diff --git a/controllers/metal3.io/host_config_data_test.go b/controllers/metal3.io/host_config_data_test.go
index 237356a2..6891226d 100644
--- a/controllers/metal3.io/host_config_data_test.go
+++ b/controllers/metal3.io/host_config_data_test.go
@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	metal3v1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
+	"github.com/metal3-io/baremetal-operator/pkg/secretutils"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -66,11 +67,11 @@ func TestLabelSecrets(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			host := newHost("host", tc.hostSpec)
 			c := fakeclient.NewClientBuilder().Build()
+			baselog := ctrl.Log.WithName("controllers").WithName("BareMetalHost")
 			hcd := &hostConfigData{
-				host:      host,
-				log:       ctrl.Log.WithName("controllers").WithName("BareMetalHost").WithName("host_config_data"),
-				client:    c,
-				apiReader: c,
+				host:          host,
+				log:           baselog.WithName("host_config_data"),
+				secretManager: secretutils.NewSecretManager(baselog, c, c),
 			}
 
 			secret := newSecret(tc.name, map[string]string{"value": "somedata"})
@@ -81,7 +82,7 @@ func TestLabelSecrets(t *testing.T) {
 
 			actualSecret := &corev1.Secret{}
 			c.Get(context.TODO(), types.NamespacedName{Name: tc.name, Namespace: namespace}, actualSecret)
-			assert.Equal(t, actualSecret.Labels[LabelEnvironmentName], LabelEnvironmentValue)
+			assert.Equal(t, "baremetal", actualSecret.Labels["environment.metal3.io"])
 		})
 	}
 
@@ -298,11 +299,11 @@ func TestProvisionWithHostConfig(t *testing.T) {
 			c.Create(goctx.TODO(), testBMCSecret)
 			c.Create(goctx.TODO(), tc.UserDataSecret)
 			c.Create(goctx.TODO(), tc.NetworkDataSecret)
+			baselog := ctrl.Log.WithName("controllers").WithName("BareMetalHost")
 			hcd := &hostConfigData{
-				host:      tc.Host,
-				log:       ctrl.Log.WithName("controllers").WithName("BareMetalHost").WithName("host_config_data"),
-				client:    c,
-				apiReader: c,
+				host:          tc.Host,
+				log:           baselog.WithName("host_config_data"),
+				secretManager: secretutils.NewSecretManager(baselog, c, c),
 			}
 
 			actualUserData, err := hcd.UserData()
diff --git a/controllers/metal3.io/host_state_machine_test.go b/controllers/metal3.io/host_state_machine_test.go
index 4982fbd7..acab71fb 100644
--- a/controllers/metal3.io/host_state_machine_test.go
+++ b/controllers/metal3.io/host_state_machine_test.go
@@ -1172,11 +1172,11 @@ func (m *mockProvisioner) Detach() (result provisioner.Result, err error) {
 	return res, err
 }
 
-func (m *mockProvisioner) PowerOn(force bool) (result provisioner.Result, err error) {
+func (m *mockProvisioner) PowerOn() (result provisioner.Result, err error) {
 	return m.getNextResultByMethod("PowerOn"), err
 }
 
-func (m *mockProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode, force bool) (result provisioner.Result, err error) {
+func (m *mockProvisioner) PowerOff(rebootMode metal3v1alpha1.RebootMode) (result provisioner.Result, err error) {
 	return m.getNextResultByMethod("PowerOff"), err
 }
 
diff --git a/pkg/provisioner/ironic/power_test.go b/pkg/provisioner/ironic/power_test.go
index 0a899d5f..2db33570 100644
--- a/pkg/provisioner/ironic/power_test.go
+++ b/pkg/provisioner/ironic/power_test.go
@@ -20,13 +20,11 @@ func TestPowerOn(t *testing.T) {
 	nodeUUID := "33ce8659-7400-4c68-9535-d10766f07a58"
 	cases := []struct {
 		name   string
-		force  bool
 		ironic *testserver.IronicMock
 
 		expectedDirty        bool
 		expectedError        bool
 		expectedRequestAfter int
-		expectedErrorResult  bool
 	}{
 		{
 			name: "node-already-power-on",
@@ -77,30 +75,6 @@ func TestPowerOn(t *testing.T) {
 			expectedRequestAfter: 10,
 			expectedDirty:        true,
 		},
-		{
-			name: "power-on with LastError",
-			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
-				PowerState:       powerOff,
-				TargetPowerState: powerOff,
-				UUID:             nodeUUID,
-				LastError:        "power on failed",
-			}),
-			expectedRequestAfter: 0,
-			expectedDirty:        false,
-			expectedErrorResult:  true,
-		},
-		{
-			name:  "power-on with LastError",
-			force: true,
-			ironic: testserver.NewIronic(t).Ready().Node(nodes.Node{
-				PowerState:       powerOff,
-				TargetPowerState: powerOff,
-				UUID:             nodeUUID,
-				LastError:        "power on failed",
-			}),
-			expectedError:       true,
-			expectedErrorResult: false,
-		},
 	}
 
 	for _, tc := range cases {
@@ -127,7 +101,7 @@ func TestPowerOn(t *testing.T) {
 				t.Fatalf("could not create provisioner: %s", err)
 			}
 
-			result, err := prov.PowerOn(tc.force)
+			result, err := prov.PowerOn()
 
 			assert.Equal(t, tc.expectedDirty, result.Dirty)
 			assert.Equal(t, time.Second*time.Duration(tc.expectedRequestAfter), result.RequeueAfter)
@@ -136,9 +110,6 @@ func TestPowerOn(t *testing.T) {
 			} else {
 				assert.Error(t, err)
 			}
-			if tc.expectedErrorResult {
-				assert.Contains(t, result.ErrorMessage, "PowerOn operation failed")
-			}
 		})
 	}
 }
@@ -149,12 +120,10 @@ func TestPowerOff(t *testing.T) {
 	cases := []struct {
 		name   string
 		ironic *testserver.IronicMock
-		force  bool
 
 		expectedDirty        bool
 		expectedError        bool
 		expectedRequestAfter int
-		expectedErrorResult  bool
 		rebootMode           metal3v1alpha1.RebootMode
 	}{
 		{
@@ -218,44 +187,6 @@ func TestPowerOff(t *testing.T) {
 			expectedRequestAfter: 10,
 			expectedDirty:        true,
 		},
-		{
-			name: "power-off soft with force",
-			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
-				PowerState:           powerOn,
-				TargetPowerState:     powerOn,
-				TargetProvisionState: "",
-				UUID:                 nodeUUID,
-			}),
-			rebootMode:    metal3v1alpha1.RebootModeSoft,
-			force:         true,
-			expectedDirty: true,
-		},
-		{
-			name: "power-off hard with LastError",
-			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
-				PowerState:           powerOn,
-				TargetPowerState:     powerOn,
-				TargetProvisionState: "",
-				UUID:                 nodeUUID,
-				LastError:            "hard power off failed",
-			}),
-			rebootMode:          metal3v1alpha1.RebootModeHard,
-			expectedDirty:       false,
-			expectedErrorResult: true,
-		},
-		{
-			name: "power-off hard with force",
-			ironic: testserver.NewIronic(t).WithDefaultResponses().Node(nodes.Node{
-				PowerState:           powerOn,
-				TargetPowerState:     powerOn,
-				TargetProvisionState: "",
-				UUID:                 nodeUUID,
-				LastError:            "hard power off failed",
-			}),
-			rebootMode:    metal3v1alpha1.RebootModeHard,
-			force:         true,
-			expectedDirty: true,
-		},
 	}
 
 	for _, tc := range cases {
@@ -283,7 +214,7 @@ func TestPowerOff(t *testing.T) {
 			}
 
 			// We pass the RebootMode type here to define the reboot action
-			result, err := prov.PowerOff(tc.rebootMode, tc.force)
+			result, err := prov.PowerOff(tc.rebootMode)
 
 			assert.Equal(t, tc.expectedDirty, result.Dirty)
 			assert.Equal(t, time.Second*time.Duration(tc.expectedRequestAfter), result.RequeueAfter)
@@ -292,10 +223,6 @@ func TestPowerOff(t *testing.T) {
 			} else {
 				assert.Error(t, err)
 			}
-			if tc.expectedErrorResult {
-				assert.Contains(t, result.ErrorMessage, "hard power off failed")
-			}
-
 		})
 	}
 }