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) {