Refactor network attachments

In Tempest and Tobiko controllers there is a duplicate code
regarding network attachments. To make it easier to maintain,
let's put the code in one place.

Author: kstrenkova

controllers/common.go

@@ -12,8 +12,11 @@ import (
 	"crypto/sha256"
 
 	"github.com/go-logr/logr"
+	networkv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
+	"github.com/openstack-k8s-operators/lib-common/modules/common/condition"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/configmap"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/helper"
+	nad "github.com/openstack-k8s-operators/lib-common/modules/common/networkattachment"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/pvc"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/util"
 	"gopkg.in/yaml.v3"
@@ -548,6 +551,101 @@ func GetCommonRbacRules(privileged bool) []rbacv1.PolicyRule {
 	return []rbacv1.PolicyRule{rbacPolicyRule}
 }
 
+// EnsureNetworkAttachments fetches NetworkAttachmentDefinitions and creates annotations
+func (r *Reconciler) EnsureNetworkAttachments(
+	ctx context.Context,
+	log logr.Logger,
+	helper *helper.Helper,
+	networkAttachments []string,
+	namespace string,
+	conditions *condition.Conditions,
+) (map[string]string, ctrl.Result, error) {
+	nadList := []networkv1.NetworkAttachmentDefinition{}
+	for _, netAtt := range networkAttachments {
+		nad, err := nad.GetNADWithName(ctx, helper, netAtt, 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))
+				conditions.Set(condition.FalseCondition(
+					condition.NetworkAttachmentsReadyCondition,
+					condition.ErrorReason,
+					condition.SeverityWarning,
+					condition.NetworkAttachmentsReadyWaitingMessage,
+					netAtt))
+				return nil, ctrl.Result{RequeueAfter: time.Second * 10}, nil
+			}
+			conditions.Set(condition.FalseCondition(
+				condition.NetworkAttachmentsReadyCondition,
+				condition.ErrorReason,
+				condition.SeverityWarning,
+				condition.NetworkAttachmentsReadyErrorMessage,
+				err.Error()))
+			return nil, ctrl.Result{}, err
+		}
+
+		if nad != nil {
+			nadList = append(nadList, *nad)
+		}
+	}
+
+	serviceAnnotations, err := nad.EnsureNetworksAnnotation(nadList)
+	if err != nil {
+		return nil, ctrl.Result{}, fmt.Errorf("failed create network annotation from %s: %w",
+			networkAttachments, err)
+	}
+
+	return serviceAnnotations, ctrl.Result{}, nil
+}
+
+// VerifyNetworkAttachments verifies network status on the pod and updates conditions
+func (r *Reconciler) VerifyNetworkAttachments(
+	ctx context.Context,
+	helper *helper.Helper,
+	instance client.Object,
+	networkAttachments []string,
+	serviceLabels map[string]string,
+	nextWorkflowStep int,
+	conditions *condition.Conditions,
+	networkAttachmentStatus *map[string][]string,
+) (ctrl.Result, error) {
+	if !r.PodExists(ctx, instance, nextWorkflowStep) {
+		return ctrl.Result{}, nil
+	}
+
+	networkReady, status, err := nad.VerifyNetworkStatusFromAnnotation(
+		ctx,
+		helper,
+		networkAttachments,
+		serviceLabels,
+		1,
+	)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	*networkAttachmentStatus = status
+
+	if networkReady {
+		conditions.MarkTrue(
+			condition.NetworkAttachmentsReadyCondition,
+			condition.NetworkAttachmentsReadyMessage)
+	} else {
+		err := fmt.Errorf("%w: %s", ErrNetworkAttachmentsMismatch, networkAttachments)
+		conditions.Set(condition.FalseCondition(
+			condition.NetworkAttachmentsReadyCondition,
+			condition.ErrorReason,
+			condition.SeverityWarning,
+			condition.NetworkAttachmentsReadyErrorMessage,
+			err.Error()))
+
+		return ctrl.Result{}, err
+	}
+
+	return ctrl.Result{}, nil
+}
+
 // EnsureCloudsConfigMapExists ensures that frameworks like Tobiko and Horizon have password values
 // present in clouds.yaml. This code ensures that we set a default value of
 // 12345678 when password value is missing in the clouds.yaml

controllers/tempest_controller.go

@@ -20,16 +20,13 @@ import (
 	"context"
 	"fmt"
 	"strconv"
-	"time"
 
 	"github.com/go-logr/logr"
-	networkv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
 	"github.com/openstack-k8s-operators/lib-common/modules/common"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/condition"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/configmap"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/helper"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/labels"
-	nad "github.com/openstack-k8s-operators/lib-common/modules/common/networkattachment"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/util"
 	testv1beta1 "github.com/openstack-k8s-operators/test-operator/api/v1beta1"
 	"github.com/openstack-k8s-operators/test-operator/pkg/tempest"
@@ -248,40 +245,16 @@ func (r *TempestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re
 	instance.Status.Conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage)
 	// Generate ConfigMaps - end
 
-	nadList := []networkv1.NetworkAttachmentDefinition{}
-	for _, netAtt := range instance.Spec.NetworkAttachments {
-		nad, err := nad.GetNADWithName(ctx, helper, 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 ctrl.Result{RequeueAfter: time.Second * 10}, nil
-			}
-			instance.Status.Conditions.Set(condition.FalseCondition(
-				condition.NetworkAttachmentsReadyCondition,
-				condition.ErrorReason,
-				condition.SeverityWarning,
-				condition.NetworkAttachmentsReadyErrorMessage,
-				err.Error()))
-			return ctrl.Result{}, err
-		}
-
-		if nad != nil {
-			nadList = append(nadList, *nad)
-		}
-	}
-
-	serviceAnnotations, err := nad.EnsureNetworksAnnotation(nadList)
-	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed create network annotation from %s: %w",
-			instance.Spec.NetworkAttachments, err)
+	serviceAnnotations, ctrlResult, err := r.EnsureNetworkAttachments(
+		ctx,
+		Log,
+		helper,
+		instance.Spec.NetworkAttachments,
+		instance.Namespace,
+		&instance.Status.Conditions,
+	)
+	if err != nil || (ctrlResult != ctrl.Result{}) {
+		return ctrlResult, err
 	}
 
 	// Create a new pod
@@ -335,37 +308,19 @@ func (r *TempestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re
 	// Create a new pod - end
 
 	// NetworkAttachments
-	if r.PodExists(ctx, instance, nextWorkflowStep) {
-		networkReady, networkAttachmentStatus, err := nad.VerifyNetworkStatusFromAnnotation(
-			ctx,
-			helper,
-			instance.Spec.NetworkAttachments,
-			serviceLabels,
-			1,
-		)
-		if err != nil {
-			return ctrl.Result{}, err
-		}
-
-		instance.Status.NetworkAttachments = networkAttachmentStatus
-
-		if networkReady {
-			instance.Status.Conditions.MarkTrue(
-				condition.NetworkAttachmentsReadyCondition,
-				condition.NetworkAttachmentsReadyMessage)
-		} else {
-			err := fmt.Errorf("%w: %s", ErrNetworkAttachmentsMismatch, instance.Spec.NetworkAttachments)
-			instance.Status.Conditions.Set(condition.FalseCondition(
-				condition.NetworkAttachmentsReadyCondition,
-				condition.ErrorReason,
-				condition.SeverityWarning,
-				condition.NetworkAttachmentsReadyErrorMessage,
-				err.Error()))
-
-			return ctrl.Result{}, err
-		}
+	ctrlResult, err = r.VerifyNetworkAttachments(
+		ctx,
+		helper,
+		instance,
+		instance.Spec.NetworkAttachments,
+		serviceLabels,
+		nextWorkflowStep,
+		&instance.Status.Conditions,
+		&instance.Status.NetworkAttachments,
+	)
+	if err != nil || (ctrlResult != ctrl.Result{}) {
+		return ctrlResult, err
 	}
-	// NetworkAttachments - end
 
 	return ctrl.Result{}, nil
 }

controllers/tobiko_controller.go

@@ -20,16 +20,13 @@ import (
 	"context"
 	"fmt"
 	"strconv"
-	"time"
 
 	"github.com/go-logr/logr"
-	networkv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
 	"github.com/openstack-k8s-operators/lib-common/modules/common"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/condition"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/configmap"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/env"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/helper"
-	nad "github.com/openstack-k8s-operators/lib-common/modules/common/networkattachment"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/util"
 	testv1beta1 "github.com/openstack-k8s-operators/test-operator/api/v1beta1"
 	"github.com/openstack-k8s-operators/test-operator/pkg/tobiko"
@@ -221,74 +218,32 @@ func (r *TobikoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 	}
 	// Create PersistentVolumeClaim - end
 
-	nadList := []networkv1.NetworkAttachmentDefinition{}
-	for _, netAtt := range instance.Spec.NetworkAttachments {
-		nad, err := nad.GetNADWithName(ctx, helper, 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 ctrl.Result{RequeueAfter: time.Second * 10}, nil
-			}
-			instance.Status.Conditions.Set(condition.FalseCondition(
-				condition.NetworkAttachmentsReadyCondition,
-				condition.ErrorReason,
-				condition.SeverityWarning,
-				condition.NetworkAttachmentsReadyErrorMessage,
-				err.Error()))
-			return ctrl.Result{}, err
-		}
-
-		if nad != nil {
-			nadList = append(nadList, *nad)
-		}
-	}
-
-	serviceAnnotations, err := nad.EnsureNetworksAnnotation(nadList)
-	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed create network annotation from %s: %w",
-			instance.Spec.NetworkAttachments, err)
+	serviceAnnotations, ctrlResult, err := r.EnsureNetworkAttachments(
+		ctx,
+		Log,
+		helper,
+		instance.Spec.NetworkAttachments,
+		instance.Namespace,
+		&instance.Status.Conditions,
+	)
+	if err != nil || (ctrlResult != ctrl.Result{}) {
+		return ctrlResult, err
 	}
 
 	// NetworkAttachments
-	if r.PodExists(ctx, instance, nextWorkflowStep) {
-		networkReady, networkAttachmentStatus, err := nad.VerifyNetworkStatusFromAnnotation(
-			ctx,
-			helper,
-			instance.Spec.NetworkAttachments,
-			serviceLabels,
-			1,
-		)
-		if err != nil {
-			return ctrl.Result{}, err
-		}
-
-		instance.Status.NetworkAttachments = networkAttachmentStatus
-
-		if networkReady {
-			instance.Status.Conditions.MarkTrue(
-				condition.NetworkAttachmentsReadyCondition,
-				condition.NetworkAttachmentsReadyMessage)
-		} else {
-			err := fmt.Errorf("%w: %s", ErrNetworkAttachmentsMismatch, instance.Spec.NetworkAttachments)
-			instance.Status.Conditions.Set(condition.FalseCondition(
-				condition.NetworkAttachmentsReadyCondition,
-				condition.ErrorReason,
-				condition.SeverityWarning,
-				condition.NetworkAttachmentsReadyErrorMessage,
-				err.Error()))
-
-			return ctrl.Result{}, err
-		}
+	ctrlResult, err = r.VerifyNetworkAttachments(
+		ctx,
+		helper,
+		instance,
+		instance.Spec.NetworkAttachments,
+		serviceLabels,
+		nextWorkflowStep,
+		&instance.Status.Conditions,
+		&instance.Status.NetworkAttachments,
+	)
+	if err != nil || (ctrlResult != ctrl.Result{}) {
+		return ctrlResult, err
 	}
-	// NetworkAttachments - end
 
 	// Create Job
 	mountCerts := r.CheckSecretExists(ctx, instance, "combined-ca-bundle")