Refactor common controller functions

This patch removes duplicate code from common controller functions.
In the future the code will be easier to maintain thanks to these
small changes.

This patch includes:
- GetContainerImage function refactor
- GetPodName function refactor
- Functions moved thanks to being Tempest specific

Author: kstrenkova

controllers/ansibletest_controller.go

@@ -209,7 +209,7 @@ func (r *AnsibleTestReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	podName := r.GetPodName(instance, nextWorkflowStep)
 	envVars := r.PrepareAnsibleEnv(instance)
 	logsPVCName := r.GetPVCLogsName(instance, 0)
-	containerImage, err := r.GetContainerImage(ctx, instance.Spec.ContainerImage, instance)
+	containerImage, err := r.GetContainerImage(ctx, instance)
 	if err != nil {
 		return ctrl.Result{}, err
 	}

controllers/common.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"reflect"
 	"strconv"
+	"strings"
 	"time"
 
 	"crypto/sha256"
@@ -15,7 +16,6 @@ import (
 	"github.com/openstack-k8s-operators/lib-common/modules/common/helper"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/pvc"
 	"github.com/openstack-k8s-operators/lib-common/modules/common/util"
-	v1beta1 "github.com/openstack-k8s-operators/test-operator/api/v1beta1"
 	"gopkg.in/yaml.v3"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -30,15 +30,13 @@ import (
 )
 
 const (
-	podNameStepInfix         = "-s"
-	envVarsConfigMapInfix    = "-env-vars-s"
-	customDataConfigMapInfix = "-custom-data-s"
-	workflowStepNumInvalid   = -1
-	workflowStepNameInvalid  = "no-name"
-	workflowStepLabel        = "workflowStep"
-	instanceNameLabel        = "instanceName"
-	operatorNameLabel        = "operator"
-
+	podNameStepInfix           = "-s"
+	envVarsConfigMapInfix      = "-env-vars-s"
+	customDataConfigMapInfix   = "-custom-data-s"
+	workflowStepNameInvalid    = "no-name"
+	workflowStepLabel          = "workflowStep"
+	instanceNameLabel          = "instanceName"
+	operatorNameLabel          = "operator"
 	testOperatorLockName       = "test-operator-lock"
 	testOperatorLockOwnerField = "owner"
 )
@@ -82,6 +80,15 @@ var (
 
 	// ErrLockFieldMissing indicates that a required field is missing in the lock config map.
 	ErrLockFieldMissing = errors.New("field is missing in the config map")
+
+	// ErrFieldExpectedStruct indicates attempting to access a field on a non-struct type.
+	ErrFieldExpectedStruct = errors.New("field cannot be accessed: expected struct")
+
+	// ErrFieldNilPointer indicates attempting to dereference a nil pointer.
+	ErrFieldNilPointer = errors.New("field cannot be accessed: nil pointer")
+
+	// ErrFieldNotFound indicates a field name does not exist on the struct.
+	ErrFieldNotFound = errors.New("field not found")
 )
 
 // Reconciler provides common functionality for all test framework reconcilers
@@ -253,164 +260,68 @@ func (r *Reconciler) GetLastPod(
 	return maxPod, nil
 }
 
-// GetEnvVarsConfigMapName returns the name of the environment variables ConfigMap for the given instance and workflow step
-func GetEnvVarsConfigMapName(instance interface{}, workflowStepNum int) string {
-	if _, ok := instance.(*v1beta1.Tobiko); ok {
-		return "not-implemented"
-	} else if typedInstance, ok := instance.(*v1beta1.Tempest); ok {
-		return typedInstance.Name + envVarsConfigMapInfix + strconv.Itoa(workflowStepNum)
-	}
-
-	return "not-implemented"
-}
-
-// GetCustomDataConfigMapName returns the name of the custom data ConfigMap for the given instance and workflow step
-func GetCustomDataConfigMapName(instance interface{}, workflowStepNum int) string {
-	if _, ok := instance.(*v1beta1.Tobiko); ok {
-		return "not-implemented"
-	} else if typedInstance, ok := instance.(*v1beta1.Tempest); ok {
-		return typedInstance.Name + customDataConfigMapInfix + strconv.Itoa(workflowStepNum)
-	}
-
-	return "not-implemented"
-}
-
 // GetContainerImage returns the container image to use for the given instance, either from the provided parameter or from configuration
 func (r *Reconciler) GetContainerImage(
 	ctx context.Context,
-	containerImage string,
 	instance interface{},
 ) (string, error) {
-	cm := &corev1.ConfigMap{}
-	testOperatorConfigMapName := "test-operator-config"
-	if typedInstance, ok := instance.(*v1beta1.Tempest); ok {
-		if len(containerImage) > 0 {
-			return containerImage, nil
-		}
-
-		objectKey := client.ObjectKey{Namespace: typedInstance.Namespace, Name: testOperatorConfigMapName}
-		err := r.Client.Get(ctx, objectKey, cm)
-		if err != nil {
-			return "", err
-		}
-
-		if cm.Data == nil {
-			return util.GetEnvVar("RELATED_IMAGE_TEST_TEMPEST_IMAGE_URL_DEFAULT", ""), nil
-
-		}
-
-		if cmImage, exists := cm.Data["tempest-image"]; exists {
-			return cmImage, nil
-		}
-
-		return util.GetEnvVar("RELATED_IMAGE_TEST_TEMPEST_IMAGE_URL_DEFAULT", ""), nil
-	} else if typedInstance, ok := instance.(*v1beta1.Tobiko); ok {
-		if len(containerImage) > 0 {
-			return containerImage, nil
-		}
-
-		objectKey := client.ObjectKey{Namespace: typedInstance.Namespace, Name: testOperatorConfigMapName}
-		err := r.Client.Get(ctx, objectKey, cm)
-		if err != nil {
-			return "", err
-		}
-
-		if cm.Data == nil {
-			return util.GetEnvVar("RELATED_IMAGE_TEST_TOBIKO_IMAGE_URL_DEFAULT", ""), nil
-
-		}
-
-		if cmImage, exists := cm.Data["tobiko-image"]; exists {
-			return cmImage, nil
-		}
-
-		return util.GetEnvVar("RELATED_IMAGE_TEST_TOBIKO_IMAGE_URL_DEFAULT", ""), nil
-	} else if typedInstance, ok := instance.(*v1beta1.HorizonTest); ok {
-		if len(containerImage) > 0 {
-			return containerImage, nil
-		}
-
-		objectKey := client.ObjectKey{Namespace: typedInstance.Namespace, Name: testOperatorConfigMapName}
-		err := r.Client.Get(ctx, objectKey, cm)
-		if err != nil {
-			return "", err
-		}
-
-		if cm.Data == nil {
-			return util.GetEnvVar("RELATED_IMAGE_TEST_HORIZONTEST_IMAGE_URL_DEFAULT", ""), nil
-
-		}
+	v := reflect.ValueOf(instance)
 
-		if cmImage, exists := cm.Data["horizontest-image"]; exists {
-			return cmImage, nil
-		}
+	spec, err := SafetyCheck(v, "Spec")
+	if err != nil {
+		return "", err
+	}
 
-		return util.GetEnvVar("RELATED_IMAGE_TEST_HORIZONTEST_IMAGE_URL_DEFAULT", ""), nil
-	} else if typedInstance, ok := instance.(*v1beta1.AnsibleTest); ok {
-		if len(containerImage) > 0 {
-			return containerImage, nil
-		}
+	containerImage := GetStringField(spec, "ContainerImage")
+	if containerImage != "" {
+		return containerImage, nil
+	}
 
-		objectKey := client.ObjectKey{Namespace: typedInstance.Namespace, Name: testOperatorConfigMapName}
-		err := r.Client.Get(ctx, objectKey, cm)
-		if err != nil {
-			return "", err
-		}
+	namespace := GetStringField(v, "Namespace")
+	kind := GetStringField(v, "Kind")
 
-		if cm.Data == nil {
-			return util.GetEnvVar("RELATED_IMAGE_TEST_ANSIBLETEST_IMAGE_URL_DEFAULT", ""), nil
-		}
+	cm := &corev1.ConfigMap{}
+	objectKey := client.ObjectKey{Namespace: namespace, Name: "test-operator-config"}
+	if err := r.Client.Get(ctx, objectKey, cm); err != nil {
+		return "", err
+	}
 
-		if cmImage, exists := cm.Data["ansibletest-image"]; exists {
-			return cmImage, nil
+	imageKey := strings.ToLower(kind) + "-image"
+	if cm.Data != nil {
+		if image, exists := cm.Data[imageKey]; exists && image != "" {
+			return image, nil
 		}
-
-		return util.GetEnvVar("RELATED_IMAGE_TEST_ANSIBLETEST_IMAGE_URL_DEFAULT", ""), nil
 	}
 
-	return "", nil
+	relatedImage := "RELATED_IMAGE_TEST_" + strings.ToUpper(kind) + "_IMAGE_URL_DEFAULT"
+	return util.GetEnvVar(relatedImage, ""), nil
 }
 
 // GetPodName returns the name of the pod for the given instance and workflow step
-func (r *Reconciler) GetPodName(instance interface{}, workflowStepNum int) string {
-	if typedInstance, ok := instance.(*v1beta1.Tobiko); ok {
-		if len(typedInstance.Spec.Workflow) == 0 || workflowStepNum == workflowStepNumInvalid {
-			return typedInstance.Name
-		}
-
-		workflowStepName := workflowStepNameInvalid
-		if workflowStepNum < len(typedInstance.Spec.Workflow) {
-			workflowStepName = typedInstance.Spec.Workflow[workflowStepNum].StepName
-		}
-
-		return typedInstance.Name + podNameStepInfix + fmt.Sprintf("%02d", workflowStepNum) + "-" + workflowStepName
-	} else if typedInstance, ok := instance.(*v1beta1.Tempest); ok {
-		if len(typedInstance.Spec.Workflow) == 0 || workflowStepNum == workflowStepNumInvalid {
-			return typedInstance.Name
-		}
+func (r *Reconciler) GetPodName(instance interface{}, stepNum int) string {
+	v := reflect.ValueOf(instance)
 
-		workflowStepName := workflowStepNameInvalid
-		if workflowStepNum < len(typedInstance.Spec.Workflow) {
-			workflowStepName = typedInstance.Spec.Workflow[workflowStepNum].StepName
-		}
+	name := GetStringField(v, "Name")
+	spec, err := SafetyCheck(v, "Spec")
+	if err != nil {
+		return name
+	}
 
-		return typedInstance.Name + podNameStepInfix + fmt.Sprintf("%02d", workflowStepNum) + "-" + workflowStepName
-	} else if typedInstance, ok := instance.(*v1beta1.HorizonTest); ok {
-		return typedInstance.Name
-	} else if typedInstance, ok := instance.(*v1beta1.AnsibleTest); ok {
-		if len(typedInstance.Spec.Workflow) == 0 || workflowStepNum == workflowStepNumInvalid {
-			return typedInstance.Name
-		}
+	workflow, err := SafetyCheck(spec, "Workflow")
+	if err != nil || workflow.Len() == 0 {
+		return name
+	}
 
-		workflowStepName := workflowStepNameInvalid
-		if workflowStepNum < len(typedInstance.Spec.Workflow) {
-			workflowStepName = typedInstance.Spec.Workflow[workflowStepNum].StepName
+	// Get workflow step name
+	stepName := workflowStepNameInvalid
+	if stepNum >= 0 && stepNum < workflow.Len() {
+		stepName = GetStringField(workflow.Index(stepNum), "StepName")
+		if stepName == "" {
+			stepName = workflowStepNameInvalid
 		}
-
-		return typedInstance.Name + podNameStepInfix + fmt.Sprintf("%02d", workflowStepNum) + "-" + workflowStepName
 	}
 
-	return workflowStepNameInvalid
+	return name + podNameStepInfix + fmt.Sprintf("%02d", stepNum) + "-" + stepName
 }
 
 // GetPVCLogsName returns the name of the PVC for logs for the given instance and workflow step
@@ -493,11 +404,6 @@ func (r *Reconciler) EnsureLogsPVCExists(
 	return ctrlResult, nil
 }
 
-// GetClient returns the Kubernetes client
-func (r *Reconciler) GetClient() client.Client {
-	return r.Client
-}
-
 // GetLogger returns the logger instance
 func (r *Reconciler) GetLogger() logr.Logger {
 	return r.Log
@@ -624,12 +530,6 @@ func (r *Reconciler) PodExists(ctx context.Context, instance client.Object, work
 	return true
 }
 
-func (r *Reconciler) setConfigOverwrite(customData map[string]string, configOverwrite map[string]string) {
-	for key, data := range configOverwrite {
-		customData[key] = data
-	}
-}
-
 // GetCommonRbacRules returns the common RBAC rules for test operations, with optional privileged permissions
 func GetCommonRbacRules(privileged bool) []rbacv1.PolicyRule {
 	rbacPolicyRule := rbacv1.PolicyRule{
@@ -717,6 +617,37 @@ func EnsureCloudsConfigMapExists(
 	return ctrl.Result{}, nil
 }
 
+// GetStringField returns reflect string field safely
+func GetStringField(v reflect.Value, fieldName string) string {
+	field, err := SafetyCheck(v, fieldName)
+	if err != nil || field.Kind() != reflect.String {
+		return ""
+	}
+
+	return field.String()
+}
+
+// SafetyCheck returns reflect value after checking its validity
+func SafetyCheck(v reflect.Value, fieldName string) (reflect.Value, error) {
+	if v.Kind() == reflect.Ptr {
+		if v.IsNil() {
+			return reflect.Value{}, fmt.Errorf("%s: %w", fieldName, ErrFieldNilPointer)
+		}
+		v = v.Elem()
+	}
+
+	if v.Kind() != reflect.Struct {
+		return reflect.Value{}, fmt.Errorf("%s: %w, got %s", fieldName, ErrFieldExpectedStruct, v.Kind())
+	}
+
+	field := v.FieldByName(fieldName)
+	if !field.IsValid() {
+		return reflect.Value{}, fmt.Errorf("%s: %w", fieldName, ErrFieldNotFound)
+	}
+
+	return field, nil
+}
+
 // IsEmpty checks if the provided value is empty based on its type
 func IsEmpty(value interface{}) bool {
 	if v, ok := value.(reflect.Value); ok {

controllers/horizontest_controller.go

@@ -215,7 +215,7 @@ func (r *HorizonTestReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	envVars := r.PrepareHorizonTestEnvVars(instance)
 	podName := r.GetPodName(instance, 0)
 	logsPVCName := r.GetPVCLogsName(instance, 0)
-	containerImage, err := r.GetContainerImage(ctx, instance.Spec.ContainerImage, instance)
+	containerImage, err := r.GetContainerImage(ctx, instance)
 	if err != nil {
 		return ctrl.Result{}, err
 	}

controllers/tempest_controller.go

@@ -290,7 +290,7 @@ func (r *TempestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (re
 	EnvVarsConfigMapName := GetEnvVarsConfigMapName(instance, nextWorkflowStep)
 	podName := r.GetPodName(instance, nextWorkflowStep)
 	logsPVCName := r.GetPVCLogsName(instance, workflowStepNum)
-	containerImage, err := r.GetContainerImage(ctx, instance.Spec.ContainerImage, instance)
+	containerImage, err := r.GetContainerImage(ctx, instance)
 	if err != nil {
 		return ctrl.Result{}, err
 	}
@@ -589,7 +589,10 @@ func (r *TempestReconciler) generateServiceConfigMaps(
 
 	r.setTempestConfigVars(envVars, customData, instance, workflowStepNum)
 	r.setTempestconfConfigVars(envVars, customData, instance)
-	r.setConfigOverwrite(customData, instance.Spec.ConfigOverwrite)
+
+	for key, data := range instance.Spec.ConfigOverwrite {
+		customData[key] = data
+	}
 
 	envVars["TEMPEST_DEBUG_MODE"] = strconv.FormatBool(instance.Spec.Debug)
 	envVars["TEMPEST_CLEANUP"] = strconv.FormatBool(instance.Spec.Cleanup)
@@ -620,3 +623,13 @@ func (r *TempestReconciler) generateServiceConfigMaps(
 
 	return configmap.EnsureConfigMaps(ctx, h, instance, cms, nil)
 }
+
+// GetEnvVarsConfigMapName returns the name of the environment variables ConfigMap for the given workflow step
+func GetEnvVarsConfigMapName(instance *testv1beta1.Tempest, workflowStepNum int) string {
+	return instance.Name + envVarsConfigMapInfix + strconv.Itoa(workflowStepNum)
+}
+
+// GetCustomDataConfigMapName returns the name of the custom data ConfigMap for the given workflow step
+func GetCustomDataConfigMapName(instance *testv1beta1.Tempest, workflowStepNum int) string {
+	return instance.Name + customDataConfigMapInfix + strconv.Itoa(workflowStepNum)
+}

controllers/tobiko_controller.go

@@ -306,7 +306,7 @@ func (r *TobikoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res
 	envVars := r.PrepareTobikoEnvVars(ctx, serviceLabels, instance, helper, nextWorkflowStep)
 	podName := r.GetPodName(instance, nextWorkflowStep)
 	logsPVCName := r.GetPVCLogsName(instance, workflowStepNum)
-	containerImage, err := r.GetContainerImage(ctx, instance.Spec.ContainerImage, instance)
+	containerImage, err := r.GetContainerImage(ctx, instance)
 	if err != nil {
 		return ctrl.Result{}, err
 	}