Merge pull request #494 from MahnoorAsghar/preprovisioning-4.19

OCPBUGS-87966: Fix preprovisioning network Secret lifecycle during BMH deletion

Author: openshift-merge-bot[bot]

controllers/metal3.io/baremetalhost_controller.go

@@ -38,6 +38,7 @@ import (
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -69,13 +70,14 @@ type BareMetalHostReconciler struct {
 // Instead of passing a zillion arguments to the action of a phase,
 // hold them in a context.
 type reconcileInfo struct {
-	ctx               context.Context
-	log               logr.Logger
-	host              *metal3api.BareMetalHost
-	request           ctrl.Request
-	bmcCredsSecret    *corev1.Secret
-	events            []corev1.Event
-	postSaveCallbacks []func()
+	ctx                              context.Context
+	log                              logr.Logger
+	host                             *metal3api.BareMetalHost
+	request                          ctrl.Request
+	bmcCredsSecret                   *corev1.Secret
+	preprovisioningNetworkDataSecret *corev1.Secret
+	events                           []corev1.Event
+	postSaveCallbacks                []func()
 }
 
 // match the provisioner.EventPublisher interface.
@@ -201,13 +203,30 @@ func (r *BareMetalHostReconciler) Reconcile(ctx context.Context, request ctrl.Re
 		}
 	}
 
+	var preprovisioningNetworkDataSecret *corev1.Secret
+	if host.Spec.PreprovisioningNetworkDataName != "" &&
+		host.Status.Provisioning.State != metal3api.StateNone &&
+		host.Status.Provisioning.State != metal3api.StateUnmanaged {
+		preprovisioningNetworkDataSecret, err = r.acquirePreprovisioningNetworkDataSecret(ctx, host)
+		if err != nil {
+			if hostInDeletionFlow(host) && k8serrors.IsNotFound(err) {
+				preprovisioningNetworkDataSecret = &corev1.Secret{}
+			} else if !hostInDeletionFlow(host) {
+				reqLogger.Info("failed to acquire preprovisioning network data secret", "error", err)
+			} else {
+				return ctrl.Result{}, fmt.Errorf("failed to acquire preprovisioning network data secret during deletion: %w", err)
+			}
+		}
+	}
+
 	initialState := host.Status.Provisioning.State
 	info := &reconcileInfo{
-		ctx:            ctx,
-		log:            reqLogger.WithValues("provisioningState", initialState),
-		host:           host,
-		request:        request,
-		bmcCredsSecret: bmcCredsSecret,
+		ctx:                              ctx,
+		log:                              reqLogger.WithValues("provisioningState", initialState),
+		host:                             host,
+		request:                          request,
+		bmcCredsSecret:                   bmcCredsSecret,
+		preprovisioningNetworkDataSecret: preprovisioningNetworkDataSecret,
 	}
 
 	prov, err := r.ProvisionerFactory.NewProvisioner(ctx, provisioner.BuildHostData(*host, *bmcCreds), info.publishEvent)
@@ -554,6 +573,13 @@ func (r *BareMetalHostReconciler) actionDeleting(prov provisioner.Provisioner, i
 		return actionError{err}
 	}
 
+	if info.preprovisioningNetworkDataSecret != nil && info.preprovisioningNetworkDataSecret.Name != "" {
+		err = secretManager.ReleaseSecret(info.preprovisioningNetworkDataSecret)
+		if err != nil {
+			return actionError{err}
+		}
+	}
+
 	info.host.Finalizers = utils.FilterStringFromList(
 		info.host.Finalizers, metal3api.BareMetalHostFinalizer)
 	info.log.Info("cleanup is complete, removed finalizer",
@@ -2220,6 +2246,22 @@ func (r *BareMetalHostReconciler) getBMCSecretAndSetOwner(ctx context.Context, r
 	return bmcCredsSecret, nil
 }
 
+// acquirePreprovisioningNetworkDataSecret claims the Secret referenced by
+// spec.preprovisioningNetworkDataName with a finalizer so it is not removed
+// before the host finishes deletion. Callers must ensure
+// spec.preprovisioningNetworkDataName is set.
+func (r *BareMetalHostReconciler) acquirePreprovisioningNetworkDataSecret(ctx context.Context, host *metal3api.BareMetalHost) (*corev1.Secret, error) {
+	secretManager := r.secretManager(ctx, r.Log.WithValues(
+		"baremetalhost", types.NamespacedName{Namespace: host.Namespace, Name: host.Name},
+	))
+	key := types.NamespacedName{
+		Name:      host.Spec.PreprovisioningNetworkDataName,
+		Namespace: host.Namespace,
+	}
+
+	return secretManager.ObtainSecretWithFinalizer(key, host.Status.Provisioning.State != metal3api.StateDeleting)
+}
+
 func credentialsFromSecret(bmcCredsSecret *corev1.Secret) *bmc.Credentials {
 	// We trim surrounding whitespace because those characters are
 	// unlikely to be part of the username or password and it is

controllers/metal3.io/host_config_data.go

@@ -6,9 +6,24 @@ import (
 	"github.com/metal3-io/baremetal-operator/pkg/secretutils"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 )
 
+// hostInDeletionFlow reports whether the host is being removed. During this
+// window a missing preprovisioning network Secret should not block progress.
+func hostInDeletionFlow(host *metal3api.BareMetalHost) bool {
+	if !host.DeletionTimestamp.IsZero() {
+		return true
+	}
+	switch host.Status.Provisioning.State {
+	case metal3api.StateDeleting, metal3api.StatePoweringOffBeforeDelete:
+		return true
+	default:
+		return false
+	}
+}
+
 // hostConfigData is an implementation of host configuration data interface.
 // Object is able to retrieve data from secrets referenced in a host spec.
 type hostConfigData struct {
@@ -20,7 +35,7 @@ type hostConfigData struct {
 // Generic method for data extraction from a Secret. Function uses dataKey
 // parameter to detirmine which data to return in case secret contins multiple
 // keys.
-func (hcd *hostConfigData) getSecretData(name, namespace, dataKey string) (string, error) {
+func (hcd *hostConfigData) getSecretData(name, namespace, dataKey string, addFinalizer bool) (string, error) {
 	if namespace != hcd.host.Namespace {
 		return "", errors.Errorf("%s secret must be in same namespace as host %s/%s", dataKey, hcd.host.Namespace, hcd.host.Name)
 	}
@@ -30,7 +45,7 @@ func (hcd *hostConfigData) getSecretData(name, namespace, dataKey string) (strin
 		Namespace: namespace,
 	}
 
-	secret, err := hcd.secretManager.ObtainSecret(key)
+	secret, err := hcd.secretManager.ObtainSecretWithFinalizer(key, addFinalizer)
 	if err != nil {
 		return "", err
 	}
@@ -63,6 +78,7 @@ func (hcd *hostConfigData) UserData() (string, error) {
 		hcd.host.Spec.UserData.Name,
 		namespace,
 		"userData",
+		false,
 	)
 }
 
@@ -86,6 +102,7 @@ func (hcd *hostConfigData) NetworkData() (string, error) {
 		networkData.Name,
 		namespace,
 		"networkData",
+		false,
 	)
 	if err != nil {
 		_, isNoDataErr := err.(NoDataInSecretError)
@@ -102,17 +119,23 @@ func (hcd *hostConfigData) PreprovisioningNetworkData() (string, error) {
 	if hcd.host.Spec.PreprovisioningNetworkDataName == "" {
 		return "", nil
 	}
+	addFinalizer := !hostInDeletionFlow(hcd.host)
 	networkDataRaw, err := hcd.getSecretData(
 		hcd.host.Spec.PreprovisioningNetworkDataName,
 		hcd.host.Namespace,
 		"networkData",
+		addFinalizer,
 	)
 	if err != nil {
 		_, isNoDataErr := err.(NoDataInSecretError)
 		if isNoDataErr {
 			hcd.log.Info("PreprovisioningNetworkData networkData key is not set, returning empty data")
 			return "", nil
 		}
+		if k8serrors.IsNotFound(err) && hostInDeletionFlow(hcd.host) {
+			hcd.log.Info("PreprovisioningNetworkData secret not found during host deletion, returning empty data")
+			return "", nil
+		}
 	}
 	return networkDataRaw, err
 }
@@ -131,5 +154,6 @@ func (hcd *hostConfigData) MetaData() (string, error) {
 		hcd.host.Spec.MetaData.Name,
 		namespace,
 		"metaData",
+		false,
 	)
 }

controllers/metal3.io/host_config_data_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -58,6 +59,15 @@ func TestLabelSecrets(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "preprovisioning-network-data",
+			getter: func(hcd *hostConfigData) (string, error) {
+				return hcd.PreprovisioningNetworkData()
+			},
+			hostSpec: &metal3api.BareMetalHostSpec{
+				PreprovisioningNetworkDataName: "preprovisioning-network-data",
+			},
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -85,6 +95,54 @@ func TestLabelSecrets(t *testing.T) {
 	}
 }
 
+func TestAcquirePreprovisioningNetworkSecret(t *testing.T) {
+	host := newHost("host", &metal3api.BareMetalHostSpec{
+		PreprovisioningNetworkDataName: "preprov-net-data",
+	})
+	host.Status.Provisioning.State = metal3api.StateRegistering
+
+	secret := newSecret("preprov-net-data", map[string]string{"networkData": "key: value"})
+	c := fakeclient.NewClientBuilder().WithObjects(host, secret).Build()
+	baselog := ctrl.Log.WithName("controllers").WithName("BareMetalHost")
+	hcd := &hostConfigData{
+		host:          host,
+		log:           baselog.WithName("host_config_data"),
+		secretManager: secretutils.NewSecretManager(context.TODO(), baselog, c, c),
+	}
+
+	_, err := hcd.PreprovisioningNetworkData()
+	require.NoError(t, err)
+
+	actualSecret := &corev1.Secret{}
+	err = c.Get(context.TODO(), types.NamespacedName{Name: "preprov-net-data", Namespace: namespace}, actualSecret)
+	require.NoError(t, err)
+	assert.Equal(t, secretutils.LabelEnvironmentValue, actualSecret.Labels[secretutils.LabelEnvironmentName])
+	assert.Contains(t, actualSecret.Finalizers, secretutils.SecretsFinalizer)
+	assert.Empty(t, actualSecret.OwnerReferences)
+}
+
+func TestPreprovisioningNetworkSecretNotFoundDuringDeletion(t *testing.T) {
+	host := newHost("host", &metal3api.BareMetalHostSpec{
+		PreprovisioningNetworkDataName: "missing-preprov-net",
+	})
+	host.Status.Provisioning.State = metal3api.StatePoweringOffBeforeDelete
+	now := metav1.Now()
+	host.DeletionTimestamp = &now
+	host.Finalizers = []string{metal3api.BareMetalHostFinalizer}
+
+	c := fakeclient.NewClientBuilder().WithObjects(host).Build()
+	baselog := ctrl.Log.WithName("controllers").WithName("BareMetalHost")
+	hcd := &hostConfigData{
+		host:          host,
+		log:           baselog.WithName("host_config_data"),
+		secretManager: secretutils.NewSecretManager(context.TODO(), baselog, c, c),
+	}
+
+	data, err := hcd.PreprovisioningNetworkData()
+	require.NoError(t, err)
+	assert.Empty(t, data)
+}
+
 func TestProvisionWithHostConfig(t *testing.T) {
 	testBMCSecret := newBMCCredsSecret(defaultSecretName, "User", "Pass")
 

pkg/secretutils/secret_manager.go

@@ -147,6 +147,13 @@ func (sm *SecretManager) ObtainSecret(key types.NamespacedName) (*corev1.Secret,
 	return sm.obtainSecretForOwner(key, nil, false)
 }
 
+// ObtainSecretWithFinalizer retrieves a Secret and ensures that it has a label
+// that will ensure it is present in the cache, and optionally adds the secrets
+// manager finalizer without setting an owner reference.
+func (sm *SecretManager) ObtainSecretWithFinalizer(key types.NamespacedName, addFinalizer bool) (*corev1.Secret, error) {
+	return sm.obtainSecretForOwner(key, nil, addFinalizer)
+}
+
 // ReleaseSecret removes secrets manager finalizer from specified secret when needed.
 func (sm *SecretManager) ReleaseSecret(secret *corev1.Secret) error {
 	if !utils.StringInList(secret.Finalizers, SecretsFinalizer) {