From 2c535bd4136ce9071e0b3762a987f8b0a9d403f7 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 --- .../metal3.io/baremetalhost_controller.go | 66 +++++++++++++++---- .../controller/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/internal/controller/metal3.io/baremetalhost_controller.go b/internal/controller/metal3.io/baremetalhost_controller.go index 9b7f6e3470..1f4e15b381 100644 --- a/internal/controller/metal3.io/baremetalhost_controller.go +++ b/internal/controller/metal3.io/baremetalhost_controller.go @@ -39,6 +39,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" @@ -70,13 +71,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. @@ -202,13 +204,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) @@ -555,6 +574,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", @@ -2275,6 +2301,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/internal/controller/metal3.io/host_config_data.go b/internal/controller/metal3.io/host_config_data.go index 534fb93deb..0a3acacac6 100644 --- a/internal/controller/metal3.io/host_config_data.go +++ b/internal/controller/metal3.io/host_config_data.go @@ -7,9 +7,24 @@ import ( metal3api "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1" "github.com/metal3-io/baremetal-operator/pkg/secretutils" 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 { @@ -21,7 +36,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 "", fmt.Errorf("%s secret must be in same namespace as host %s/%s", dataKey, hcd.host.Namespace, hcd.host.Name) } @@ -31,7 +46,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 } @@ -64,6 +79,7 @@ func (hcd *hostConfigData) UserData() (string, error) { hcd.host.Spec.UserData.Name, namespace, "userData", + false, ) } @@ -87,6 +103,7 @@ func (hcd *hostConfigData) NetworkData() (string, error) { networkData.Name, namespace, "networkData", + false, ) if err != nil { _, isNoDataErr := err.(NoDataInSecretError) @@ -103,10 +120,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) @@ -114,6 +133,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 } @@ -132,5 +155,6 @@ func (hcd *hostConfigData) MetaData() (string, error) { hcd.host.Spec.MetaData.Name, namespace, "metaData", + false, ) } diff --git a/internal/controller/metal3.io/host_config_data_test.go b/internal/controller/metal3.io/host_config_data_test.go index dd0148a367..6b6036c66f 100644 --- a/internal/controller/metal3.io/host_config_data_test.go +++ b/internal/controller/metal3.io/host_config_data_test.go @@ -10,6 +10,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" @@ -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(t.Context(), baselog, c, c), + } + + _, err := hcd.PreprovisioningNetworkData() + require.NoError(t, err) + + actualSecret := &corev1.Secret{} + err = c.Get(t.Context(), 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(t.Context(), 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") diff --git a/pkg/secretutils/secret_manager.go b/pkg/secretutils/secret_manager.go index 795b42f7e9..631100e943 100644 --- a/pkg/secretutils/secret_manager.go +++ b/pkg/secretutils/secret_manager.go @@ -146,6 +146,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) {