Unify placement controller with internal/common helpers

Use shared EnsureSecret, EnsureNetworkAttachments, and EnsureTopology
from internal/common in the placement API controller.

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: amartyasinha

api/placement/v1beta1/api_types.go

@@ -247,11 +247,26 @@ func SetupDefaults() {
 	SetupPlacementAPIDefaults(placementDefaults)
 }
 
-// GetSecret returns the value of the Nova.Spec.Secret
+// GetSecret returns the value of the PlacementAPI.Spec.Secret
 func (instance PlacementAPI) GetSecret() string {
 	return instance.Spec.Secret
 }
 
+// GetSpecTopologyRef returns the TopologyRef from the Spec
+func (instance *PlacementAPI) GetSpecTopologyRef() *topologyv1.TopoRef {
+	return instance.Spec.TopologyRef
+}
+
+// GetLastAppliedTopology returns the LastAppliedTopology from the Status
+func (instance *PlacementAPI) GetLastAppliedTopology() *topologyv1.TopoRef {
+	return instance.Status.LastAppliedTopology
+}
+
+// SetLastAppliedTopology sets the LastAppliedTopology value in the Status
+func (instance *PlacementAPI) SetLastAppliedTopology(topologyRef *topologyv1.TopoRef) {
+	instance.Status.LastAppliedTopology = topologyRef
+}
+
 // ValidateTopology -
 func (instance *PlacementAPISpecCore) ValidateTopology(
 	basePath *field.Path,

internal/controller/placement/api_controller.go

@@ -19,7 +19,6 @@ package controller
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"maps"
 	"slices"
@@ -40,7 +39,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/go-logr/logr"
-	networkv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
 	topologyv1 "github.com/openstack-k8s-operators/infra-operator/apis/topology/v1beta1"
 	keystonev1 "github.com/openstack-k8s-operators/keystone-operator/api/v1beta1"
 	common "github.com/openstack-k8s-operators/lib-common/modules/common"
@@ -61,6 +59,7 @@ import (
 	mariadbv1 "github.com/openstack-k8s-operators/mariadb-operator/api/v1beta1"
 
 	placementv1 "github.com/openstack-k8s-operators/nova-operator/api/placement/v1beta1"
+	internalcommon "github.com/openstack-k8s-operators/nova-operator/internal/common"
 	placement "github.com/openstack-k8s-operators/nova-operator/internal/placement"
 
 	appsv1 "k8s.io/api/apps/v1"
@@ -70,91 +69,6 @@ import (
 	k8s_errors "k8s.io/apimachinery/pkg/api/errors"
 )
 
-// Static errors for Application Credential handling
-var (
-	ErrACSecretNotFound    = errors.New("ApplicationCredential secret not found")
-	ErrACSecretMissingKeys = errors.New("ApplicationCredential secret missing required keys")
-)
-
-type conditionUpdater interface {
-	Set(c *condition.Condition)
-	MarkTrue(t condition.Type, messageFormat string, messageArgs ...any)
-}
-
-// GetSecret interface defines methods for objects that can provide secret names
-type GetSecret interface {
-	GetSecret() string
-	client.Object
-}
-
-// ensureSecret - ensures that the Secret object exists and the expected fields
-// are in the Secret. It returns a hash of the values of the expected fields.
-func ensureSecret(
-	ctx context.Context,
-	secretName types.NamespacedName,
-	expectedFields []string,
-	reader client.Reader,
-	conditionUpdater conditionUpdater,
-) (string, ctrl.Result, corev1.Secret, error) {
-	secret := &corev1.Secret{}
-	err := reader.Get(ctx, secretName, secret)
-	if err != nil {
-		if k8s_errors.IsNotFound(err) {
-			// This is currently only used to acquire the password secret, which should have been
-			// manually created by the user and referenced in the spec, so we treat this as a
-			// warning because it means that the service will not be able to start.
-			conditionUpdater.Set(condition.FalseCondition(
-				condition.InputReadyCondition,
-				condition.ErrorReason,
-				condition.SeverityWarning,
-				"%s", fmt.Sprintf("Input data resources missing: %s", "secret/"+secretName.Name)))
-			return "",
-				ctrl.Result{},
-				*secret,
-				fmt.Errorf("%w: Secret %s not found", err, secretName)
-		}
-		conditionUpdater.Set(condition.FalseCondition(
-			condition.InputReadyCondition,
-			condition.ErrorReason,
-			condition.SeverityWarning,
-			condition.InputReadyErrorMessage,
-			err.Error()))
-		return "", ctrl.Result{}, *secret, err
-	}
-
-	// collect the secret values the caller expects to exist
-	values := [][]byte{}
-	for _, field := range expectedFields {
-		val, ok := secret.Data[field]
-		if !ok {
-			err := fmt.Errorf("%w: field '%s' not found in secret/%s", util.ErrFieldNotFound, field, secretName.Name)
-			conditionUpdater.Set(condition.FalseCondition(
-				condition.InputReadyCondition,
-				condition.ErrorReason,
-				condition.SeverityWarning,
-				condition.InputReadyErrorMessage,
-				err.Error()))
-			return "", ctrl.Result{}, *secret, err
-		}
-		values = append(values, val)
-	}
-
-	// TODO(gibi): Do we need to watch the Secret for changes?
-
-	hash, err := util.ObjectHash(values)
-	if err != nil {
-		conditionUpdater.Set(condition.FalseCondition(
-			condition.InputReadyCondition,
-			condition.ErrorReason,
-			condition.SeverityWarning,
-			condition.InputReadyErrorMessage,
-			err.Error()))
-		return "", ctrl.Result{}, *secret, err
-	}
-
-	return hash, ctrl.Result{}, *secret, nil
-}
-
 // GetLogger returns a logger object with a prefix of "controller.name" and additional controller context fields
 func (r *PlacementAPIReconciler) GetLogger(ctx context.Context) logr.Logger {
 	return log.FromContext(ctx).WithName("Controllers").WithName("PlacementAPI")
@@ -307,34 +221,26 @@ func (r *PlacementAPIReconciler) Reconcile(ctx context.Context, req ctrl.Request
 	//
 	// check for required OpenStack secret holding passwords for service/admin user and add hash to the vars map
 	//
-	hash, result, secret, err := ensureSecret(
+	hash, result, secret, err := internalcommon.EnsureSecret(
 		ctx,
 		types.NamespacedName{Namespace: instance.Namespace, Name: instance.Spec.Secret},
 		[]string{
 			instance.Spec.PasswordSelectors.Service,
 		},
 		h.GetClient(),
-		&instance.Status.Conditions)
+		&instance.Status.Conditions,
+		time.Second*10,
+	)
 	if err != nil {
-		if k8s_errors.IsNotFound(err) {
-			// Since the service secret should have been manually created by the user and referenced in the spec,
-			// we treat this as a warning because it means that the service will not be able to start.
-			Log.Info(fmt.Sprintf("OpenStack secret %s not found", instance.Spec.Secret))
-			instance.Status.Conditions.Set(condition.FalseCondition(
-				condition.InputReadyCondition,
-				condition.ErrorReason,
-				condition.SeverityWarning,
-				condition.InputReadyWaitingMessage))
-			return ctrl.Result{RequeueAfter: time.Second * 10}, nil
-		}
-		instance.Status.Conditions.Set(condition.FalseCondition(
-			condition.InputReadyCondition,
-			condition.ErrorReason,
-			condition.SeverityWarning,
-			condition.InputReadyErrorMessage,
-			err.Error()))
 		return result, err
 	}
+	if (result != ctrl.Result{}) {
+		// Since the service secret should have been manually created by the user and referenced in the spec,
+		// we treat this as a warning because it means that the service will not be able to start.
+		// InputReady condition and requeue are already set by EnsureSecret above.
+		Log.Info(fmt.Sprintf("OpenStack secret %s not found", instance.Spec.Secret))
+		return result, nil
+	}
 	configMapVars[instance.Spec.Secret] = env.SetValue(hash)
 
 	// all our input checks out so report InputReady
@@ -472,7 +378,7 @@ func (r *PlacementAPIReconciler) Reconcile(ctx context.Context, req ctrl.Request
 
 	instance.Status.Conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage)
 
-	serviceAnnotations, result, err := r.ensureNetworkAttachments(ctx, h, instance)
+	serviceAnnotations, result, err := internalcommon.EnsureNetworkAttachments(ctx, h, instance.Spec.NetworkAttachments, &instance.Status.Conditions, time.Second*10)
 	if (err != nil || result != ctrl.Result{}) {
 		return result, err
 	}
@@ -660,54 +566,6 @@ func (r *PlacementAPIReconciler) ensureServiceExposed(
 	return apiEndpoints, ctrl.Result{}, nil
 }
 
-func (r *PlacementAPIReconciler) ensureNetworkAttachments(
-	ctx context.Context,
-	h *helper.Helper,
-	instance *placementv1.PlacementAPI,
-) (map[string]string, ctrl.Result, error) {
-	var nadAnnotations map[string]string
-	var err error
-
-	Log := r.GetLogger(ctx)
-	// networks to attach to
-	nadList := []networkv1.NetworkAttachmentDefinition{}
-	for _, netAtt := range instance.Spec.NetworkAttachments {
-		nad, err := nad.GetNADWithName(ctx, h, netAtt, instance.Namespace)
-		if err != nil {
-			if k8s_errors.IsNotFound(err) {
-				// Since the net-attach-def CR should have been manually created by the user and referenced in the spec,
-				// we treat this as a warning because it means that the service will not be able to start.
-				Log.Info(fmt.Sprintf("network-attachment-definition %s not found", netAtt))
-				instance.Status.Conditions.Set(condition.FalseCondition(
-					condition.NetworkAttachmentsReadyCondition,
-					condition.ErrorReason,
-					condition.SeverityWarning,
-					condition.NetworkAttachmentsReadyWaitingMessage,
-					netAtt))
-				return nadAnnotations, ctrl.Result{RequeueAfter: time.Second * 10}, nil
-			}
-			instance.Status.Conditions.Set(condition.FalseCondition(
-				condition.NetworkAttachmentsReadyCondition,
-				condition.ErrorReason,
-				condition.SeverityWarning,
-				condition.NetworkAttachmentsReadyErrorMessage,
-				err.Error()))
-			return nadAnnotations, ctrl.Result{}, err
-		}
-
-		if nad != nil {
-			nadList = append(nadList, *nad)
-		}
-	}
-
-	nadAnnotations, err = nad.EnsureNetworksAnnotation(nadList)
-	if err != nil {
-		return nadAnnotations, ctrl.Result{}, fmt.Errorf("failed create network annotation from %s: %w",
-			instance.Spec.NetworkAttachments, err)
-	}
-	return nadAnnotations, ctrl.Result{}, nil
-}
-
 func (r *PlacementAPIReconciler) ensureKeystoneServiceUser(
 	ctx context.Context,
 	h *helper.Helper,
@@ -1268,35 +1126,19 @@ func (r *PlacementAPIReconciler) ensureDeployment(
 	//
 	// Handle Topology
 	//
-	topology, err := topologyv1.EnsureServiceTopology(
+	// If TopologyRef is present and ensureServiceTopology returned a valid
+	// topology object, set .Status.LastAppliedTopology to the referenced one
+	// and mark the condition as true
+	topology, err := internalcommon.EnsureTopology(
 		ctx,
 		h,
-		instance.Spec.TopologyRef,
-		instance.Status.LastAppliedTopology,
+		instance,
 		instance.Name,
+		&instance.Status.Conditions,
 		labels.GetLabelSelector(serviceLabels),
 	)
 	if err != nil {
-		instance.Status.Conditions.Set(condition.FalseCondition(
-			condition.TopologyReadyCondition,
-			condition.ErrorReason,
-			condition.SeverityWarning,
-			condition.TopologyReadyErrorMessage,
-			err.Error()))
-		return ctrl.Result{}, fmt.Errorf("waiting for Topology requirements: %w", err)
-	}
-
-	// If TopologyRef is present and ensureServiceTopology returned a valid
-	// topology object, set .Status.LastAppliedTopology to the referenced one
-	// and mark the condition as true
-	if instance.Spec.TopologyRef != nil {
-		// update the Status with the last retrieved Topology name
-		instance.Status.LastAppliedTopology = instance.Spec.TopologyRef
-		// update the TopologyRef associated condition
-		instance.Status.Conditions.MarkTrue(condition.TopologyReadyCondition, condition.TopologyReadyMessage)
-	} else {
-		// remove LastAppliedTopology from the .Status
-		instance.Status.LastAppliedTopology = nil
+		return ctrl.Result{}, err
 	}
 
 	// Define a new Deployment object
@@ -1472,7 +1314,7 @@ func (r *PlacementAPIReconciler) generateServiceConfigMaps(
 		if err != nil {
 			if k8s_errors.IsNotFound(err) {
 				h.GetLogger().Info("ApplicationCredential secret not found, waiting", "secret", instance.Spec.Auth.ApplicationCredentialSecret)
-				return fmt.Errorf("%w: %s", ErrACSecretNotFound, instance.Spec.Auth.ApplicationCredentialSecret)
+				return fmt.Errorf("%w: %s", internalcommon.ErrACSecretNotFound, instance.Spec.Auth.ApplicationCredentialSecret)
 			}
 			h.GetLogger().Error(err, "Failed to get ApplicationCredential secret", "secret", instance.Spec.Auth.ApplicationCredentialSecret)
 			return err
@@ -1481,7 +1323,7 @@ func (r *PlacementAPIReconciler) generateServiceConfigMaps(
 		acSecretData, okSecret := acSecretObj.Data[keystonev1.ACSecretSecretKey]
 		if !okID || len(acID) == 0 || !okSecret || len(acSecretData) == 0 {
 			h.GetLogger().Info("ApplicationCredential secret missing required keys", "secret", instance.Spec.Auth.ApplicationCredentialSecret)
-			return fmt.Errorf("%w: %s", ErrACSecretMissingKeys, instance.Spec.Auth.ApplicationCredentialSecret)
+			return fmt.Errorf("%w: %s", internalcommon.ErrACSecretMissingKeys, instance.Spec.Auth.ApplicationCredentialSecret)
 		}
 		templateParameters["UseApplicationCredentials"] = true
 		templateParameters["ACID"] = string(acID)