From 608bd3e7411b27266db2f55b63eb376b6b10780b Mon Sep 17 00:00:00 2001 From: Mahnoor Asghar Date: Wed, 20 May 2026 15:26:03 +0200 Subject: [PATCH] Fix preprovisioning network Secret lifecycle during BMH deletion Add the same finalizer to Secret referenced by preprovisioningNetworkDataName as the finalizer used for BMC credentials, and release it when the host finishes deleting. Treat a missing Secret as non-fatal during host deletion to avoid RegistrationError noise when namespaces are torn down concurrently. Co-authored-by: Cursor Signed-off-by: Mahnoor Asghar Co-authored-by: Cursor --- .../metal3.io/baremetalhost_controller.go | 66 +++++++++++++++---- controllers/metal3.io/host_config_data.go | 28 +++++++- .../metal3.io/host_config_data_test.go | 58 ++++++++++++++++ pkg/secretutils/secret_manager.go | 7 ++ 4 files changed, 145 insertions(+), 14 deletions(-) diff --git a/controllers/metal3.io/baremetalhost_controller.go b/controllers/metal3.io/baremetalhost_controller.go index 61e6cc7092..dc974cd43d 100644 --- a/controllers/metal3.io/baremetalhost_controller.go +++ b/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) @@ -549,6 +568,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", @@ -2211,6 +2237,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 diff --git a/controllers/metal3.io/host_config_data.go b/controllers/metal3.io/host_config_data.go index 037a0372c4..88d980e5e6 100644 --- a/controllers/metal3.io/host_config_data.go +++ b/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,10 +119,12 @@ 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) @@ -113,6 +132,10 @@ func (hcd *hostConfigData) PreprovisioningNetworkData() (string, error) { 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, ) } diff --git a/controllers/metal3.io/host_config_data_test.go b/controllers/metal3.io/host_config_data_test.go index f7d9ac4a58..72675e2af6 100644 --- a/controllers/metal3.io/host_config_data_test.go +++ b/controllers/metal3.io/host_config_data_test.go @@ -10,6 +10,7 @@ import ( "github.com/metal3-io/baremetal-operator/pkg/secretutils" "github.com/stretchr/testify/assert" 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" @@ -57,6 +58,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) { @@ -84,6 +94,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() + assert.NoError(t, err) + + actualSecret := &corev1.Secret{} + err = c.Get(context.TODO(), types.NamespacedName{Name: "preprov-net-data", Namespace: namespace}, actualSecret) + assert.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() + assert.NoError(t, err) + assert.Empty(t, data) +} + func TestProvisionWithHostConfig(t *testing.T) { testBMCSecret := newBMCCredsSecret(defaultSecretName, "User", "Pass") diff --git a/pkg/secretutils/secret_manager.go b/pkg/secretutils/secret_manager.go index 96d16af7bd..e2fd326fe7 100644 --- a/pkg/secretutils/secret_manager.go +++ b/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) {