Merge pull request #351 from kstrenkova/refactor-reconciler-functions

Refactor common controller functions

Author: openshift-merge-bot[bot]

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
 	}