Address review comments

Signed-off-by: chiragkyal <ckyal@redhat.com>

Author: chiragkyal

…r/core_validation_helpers_duplication.go …roller/common/core_validation_helpers.gopkg/controller/istiocsr/core_validation_helpers_duplication.go renamed to pkg/controller/common/core_validation_helpers.go pkg/controller/istiocsr/core_validation_helpers_duplication.go renamed to pkg/controller/common/core_validation_helpers.go

@@ -1,4 +1,4 @@
-package istiocsr
+package common
 
 import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -13,6 +13,7 @@ import (
  * Methods here are the replicates of the methods defined in k8s.io/kubernetes/pkg/apis/core/validation package, which
  * is done just because of the lack of better alternative to use private methods.
  * TODO: Remove this source file when validateAffinity method is made public.
+ * Upstream tracker: https://github.com/kubernetes/kubernetes/issues/136422
  */
 
 // validateAffinity checks if given affinities are valid.

pkg/controller/common/validation.go

@@ -0,0 +1,54 @@
+package common
+
+import (
+	"unsafe"
+
+	corev1 "k8s.io/api/core/v1"
+	apivalidation "k8s.io/apimachinery/pkg/api/validation"
+	metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/kubernetes/pkg/apis/core"
+	corevalidation "k8s.io/kubernetes/pkg/apis/core/validation"
+)
+
+// ValidateNodeSelectorConfig validates the NodeSelector configuration using
+// the Kubernetes label validation rules.
+func ValidateNodeSelectorConfig(nodeSelector map[string]string, fldPath *field.Path) error {
+	return metav1validation.ValidateLabels(nodeSelector, fldPath.Child("nodeSelector")).ToAggregate()
+}
+
+// ValidateTolerationsConfig validates the Tolerations configuration using
+// the Kubernetes core toleration validation rules.
+func ValidateTolerationsConfig(tolerations []corev1.Toleration, fldPath *field.Path) error {
+	// convert corev1.Tolerations to core.Tolerations, required for validation.
+	convTolerations := *(*[]core.Toleration)(unsafe.Pointer(&tolerations))
+	return corevalidation.ValidateTolerations(convTolerations, fldPath.Child("tolerations")).ToAggregate()
+}
+
+// ValidateResourceRequirements validates the ResourceRequirements configuration
+// using the Kubernetes core resource requirements validation rules.
+func ValidateResourceRequirements(requirements corev1.ResourceRequirements, fldPath *field.Path) error {
+	// convert corev1.ResourceRequirements to core.ResourceRequirements, required for validation.
+	convRequirements := *(*core.ResourceRequirements)(unsafe.Pointer(&requirements))
+	return corevalidation.ValidateContainerResourceRequirements(&convRequirements, nil, fldPath.Child("resources"), corevalidation.PodValidationOptions{}).ToAggregate()
+}
+
+// ValidateAffinityRules validates the Affinity configuration using
+// the Kubernetes core affinity validation rules.
+func ValidateAffinityRules(affinity *corev1.Affinity, fldPath *field.Path) error {
+	// convert corev1.Affinity to core.Affinity, required for validation.
+	convAffinity := (*core.Affinity)(unsafe.Pointer(affinity))
+	return validateAffinity(convAffinity, corevalidation.PodValidationOptions{}, fldPath.Child("affinity")).ToAggregate()
+}
+
+// ValidateLabelsConfig validates label keys and values using the Kubernetes
+// metadata validation rules.
+func ValidateLabelsConfig(labels map[string]string, fldPath *field.Path) error {
+	return metav1validation.ValidateLabels(labels, fldPath.Child("labels")).ToAggregate()
+}
+
+// ValidateAnnotationsConfig validates annotation keys and sizes using the
+// Kubernetes metadata validation rules.
+func ValidateAnnotationsConfig(annotations map[string]string, fldPath *field.Path) error {
+	return apivalidation.ValidateAnnotations(annotations, fldPath.Child("annotations")).ToAggregate()
+}

pkg/controller/istiocsr/deployments.go

@@ -7,16 +7,12 @@ import (
 	"os"
 	"reflect"
 	"strings"
-	"unsafe"
 
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/validation/field"
-	"k8s.io/kubernetes/pkg/apis/core"
-	corevalidation "k8s.io/kubernetes/pkg/apis/core/validation"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
@@ -188,7 +184,7 @@ func updateResourceRequirement(deployment *appsv1.Deployment, istiocsr *v1alpha1
 	if reflect.ValueOf(istiocsr.Spec.IstioCSRConfig.Resources).IsZero() {
 		return nil
 	}
-	if err := validateResourceRequirements(istiocsr.Spec.IstioCSRConfig.Resources,
+	if err := common.ValidateResourceRequirements(istiocsr.Spec.IstioCSRConfig.Resources,
 		field.NewPath("spec", "istioCSRConfig")); err != nil {
 		return err
 	}
@@ -202,7 +198,7 @@ func updateAffinityRules(deployment *appsv1.Deployment, istiocsr *v1alpha1.Istio
 	if istiocsr.Spec.IstioCSRConfig.Affinity == nil {
 		return nil
 	}
-	if err := validateAffinityRules(istiocsr.Spec.IstioCSRConfig.Affinity,
+	if err := common.ValidateAffinityRules(istiocsr.Spec.IstioCSRConfig.Affinity,
 		field.NewPath("spec", "istioCSRConfig")); err != nil {
 		return err
 	}
@@ -214,7 +210,7 @@ func updatePodTolerations(deployment *appsv1.Deployment, istiocsr *v1alpha1.Isti
 	if istiocsr.Spec.IstioCSRConfig.Tolerations == nil {
 		return nil
 	}
-	if err := validateTolerationsConfig(istiocsr.Spec.IstioCSRConfig.Tolerations,
+	if err := common.ValidateTolerationsConfig(istiocsr.Spec.IstioCSRConfig.Tolerations,
 		field.NewPath("spec", "istioCSRConfig")); err != nil {
 		return err
 	}
@@ -226,7 +222,7 @@ func updateNodeSelector(deployment *appsv1.Deployment, istiocsr *v1alpha1.IstioC
 	if istiocsr.Spec.IstioCSRConfig.NodeSelector == nil {
 		return nil
 	}
-	if err := validateNodeSelectorConfig(istiocsr.Spec.IstioCSRConfig.NodeSelector,
+	if err := common.ValidateNodeSelectorConfig(istiocsr.Spec.IstioCSRConfig.NodeSelector,
 		field.NewPath("spec", "istioCSRConfig")); err != nil {
 		return err
 	}
@@ -642,25 +638,3 @@ func (r *Reconciler) updateWatchLabel(obj client.Object, istiocsr *v1alpha1.Isti
 	return nil
 }
 
-// validateNodeSelectorConfig validates the NodeSelector configuration.
-func validateNodeSelectorConfig(nodeSelector map[string]string, fldPath *field.Path) error {
-	return metav1validation.ValidateLabels(nodeSelector, fldPath.Child("nodeSelector")).ToAggregate()
-}
-
-func validateTolerationsConfig(tolerations []corev1.Toleration, fldPath *field.Path) error {
-	// convert corev1.Tolerations to core.Tolerations, required for validation.
-	convTolerations := *(*[]core.Toleration)(unsafe.Pointer(&tolerations))
-	return corevalidation.ValidateTolerations(convTolerations, fldPath.Child("tolerations")).ToAggregate()
-}
-
-func validateResourceRequirements(requirements corev1.ResourceRequirements, fldPath *field.Path) error {
-	// convert corev1.ResourceRequirements to core.ResourceRequirements, required for validation.
-	convRequirements := *(*core.ResourceRequirements)(unsafe.Pointer(&requirements))
-	return corevalidation.ValidateContainerResourceRequirements(&convRequirements, nil, fldPath.Child("resources"), corevalidation.PodValidationOptions{}).ToAggregate()
-}
-
-func validateAffinityRules(affinity *corev1.Affinity, fldPath *field.Path) error {
-	// convert corev1.Affinity to core.Affinity, required for validation.
-	convAffinity := (*core.Affinity)(unsafe.Pointer(affinity))
-	return validateAffinity(convAffinity, corevalidation.PodValidationOptions{}, fldPath.Child("affinity")).ToAggregate()
-}

pkg/controller/trustmanager/certificates.go

@@ -29,16 +29,16 @@ func (r *Reconciler) createOrApplyIssuer(trustManager *v1alpha1.TrustManager, re
 		return common.FromClientError(err, "failed to check if issuer %q exists", resourceName)
 	}
 	if exists && !issuerModified(desired, existing) {
-		r.log.V(4).Info("issuer already matches desired state, skipping apply", "name", resourceName)
+		r.log.V(4).Info("issuer resource exists and is in desired state", "name", resourceName)
 		return nil
 	}
 
+	r.log.V(2).Info("issuer resource has been modified, updating to desired state", "name", resourceName)
 	if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil {
 		return common.FromClientError(err, "failed to apply issuer %q", resourceName)
 	}
 
 	r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "issuer resource %s applied", resourceName)
-	r.log.V(2).Info("applied issuer", "name", resourceName)
 	return nil
 }
 
@@ -63,16 +63,16 @@ func (r *Reconciler) createOrApplyCertificate(trustManager *v1alpha1.TrustManage
 		return common.FromClientError(err, "failed to check if certificate %q exists", resourceName)
 	}
 	if exists && !certificateModified(desired, existing) {
-		r.log.V(4).Info("certificate already matches desired state, skipping apply", "name", resourceName)
+		r.log.V(4).Info("certificate resource exists and is in desired state", "name", resourceName)
 		return nil
 	}
 
+	r.log.V(2).Info("certificate resource has been modified, updating to desired state", "name", resourceName)
 	if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil {
 		return common.FromClientError(err, "failed to apply certificate %q", resourceName)
 	}
 
 	r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "certificate resource %s applied", resourceName)
-	r.log.V(2).Info("applied certificate", "name", resourceName)
 	return nil
 }
 

pkg/controller/trustmanager/constants.go

@@ -3,6 +3,8 @@ package trustmanager
 import (
 	"os"
 	"time"
+
+	"k8s.io/apimachinery/pkg/util/validation/field"
 )
 
 const (
@@ -12,10 +14,6 @@ const (
 	// ControllerName is the name of the controller used in logs and events.
 	ControllerName = trustManagerCommonName + "-controller"
 
-	// controllerProcessedAnnotation is the annotation added to trustmanager resource once after
-	// successful reconciliation by the controller.
-	controllerProcessedAnnotation = "operator.openshift.io/trust-manager-processed"
-
 	// finalizer name for trustmanager.openshift.operator.io resource.
 	finalizer = "trustmanager.openshift.operator.io/" + ControllerName
 
@@ -55,29 +53,33 @@ const (
 // These must be set explicitly on each resource's .metadata.name and on every
 // field in other resources that references them.
 const (
-	trustManagerServiceAccountName = "trust-manager"
-	trustManagerDeploymentName     = "trust-manager"
+	trustManagerCommonResourceName = "trust-manager"
+
+	trustManagerServiceAccountName = trustManagerCommonResourceName
+	trustManagerDeploymentName     = trustManagerCommonResourceName
 
-	trustManagerServiceName        = "trust-manager"
-	trustManagerMetricsServiceName = "trust-manager-metrics"
+	trustManagerServiceName        = trustManagerCommonResourceName
+	trustManagerMetricsServiceName = trustManagerCommonResourceName + "-metrics"
 
-	trustManagerClusterRoleName        = "trust-manager"
-	trustManagerClusterRoleBindingName = "trust-manager"
+	trustManagerClusterRoleName        = trustManagerCommonResourceName
+	trustManagerClusterRoleBindingName = trustManagerCommonResourceName
 
-	trustManagerRoleName        = "trust-manager"
-	trustManagerRoleBindingName = "trust-manager"
+	trustManagerRoleName        = trustManagerCommonResourceName
+	trustManagerRoleBindingName = trustManagerCommonResourceName
 
-	trustManagerLeaderElectionRoleName        = "trust-manager:leaderelection"
-	trustManagerLeaderElectionRoleBindingName = "trust-manager:leaderelection"
+	trustManagerLeaderElectionRoleName        = trustManagerCommonResourceName + ":leaderelection"
+	trustManagerLeaderElectionRoleBindingName = trustManagerCommonResourceName + ":leaderelection"
 
-	trustManagerIssuerName      = "trust-manager"
-	trustManagerCertificateName = "trust-manager"
-	trustManagerTLSSecretName   = "trust-manager-tls"
+	trustManagerIssuerName      = trustManagerCommonResourceName
+	trustManagerCertificateName = trustManagerCommonResourceName
+	trustManagerTLSSecretName   = trustManagerCommonResourceName + "-tls"
 
-	trustManagerWebhookConfigName = "trust-manager"
+	trustManagerWebhookConfigName = trustManagerCommonResourceName
 )
 
 var (
+	trustManagerConfigFieldPath    = field.NewPath("spec", "trustManagerConfig")
+	controllerConfigFieldPath      = field.NewPath("spec", "controllerConfig")
 	controllerDefaultResourceLabels = map[string]string{
 		"app":                          trustManagerCommonName,
 		"app.kubernetes.io/name":       trustManagerCommonName,

pkg/controller/trustmanager/controller.go

@@ -3,7 +3,6 @@ package trustmanager
 import (
 	"context"
 	"fmt"
-	"reflect"
 
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	appsv1 "k8s.io/api/apps/v1"
@@ -178,10 +177,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 }
 
 func (r *Reconciler) processReconcileRequest(trustManager *v1alpha1.TrustManager, req types.NamespacedName) (ctrl.Result, error) {
-	if !containsProcessedAnnotation(trustManager) && reflect.DeepEqual(trustManager.Status, v1alpha1.TrustManagerStatus{}) {
-		r.log.V(1).Info("starting reconciliation of newly created trustmanager", "name", trustManager.GetName())
-	}
-
 	reconcileErr := r.reconcileTrustManagerDeployment(trustManager)
 	if reconcileErr != nil {
 		r.log.Error(reconcileErr, "failed to reconcile TrustManager deployment", "request", req)

pkg/controller/trustmanager/deployments.go

@@ -32,16 +32,16 @@ func (r *Reconciler) createOrApplyDeployment(trustManager *v1alpha1.TrustManager
 		return common.FromClientError(err, "failed to check if deployment %q exists", deploymentName)
 	}
 	if exists && !deploymentModified(desired, existing) {
-		r.log.V(4).Info("deployment already matches desired state, skipping apply", "name", deploymentName)
+		r.log.V(4).Info("deployment resource exists and is in desired state", "name", deploymentName)
 		return nil
 	}
 
+	r.log.V(2).Info("deployment resource has been modified, updating to desired state", "name", deploymentName)
 	if err := r.Patch(r.ctx, desired, client.Apply, client.FieldOwner(fieldOwner), client.ForceOwnership); err != nil {
 		return common.FromClientError(err, "failed to apply deployment %q", deploymentName)
 	}
 
 	r.eventRecorder.Eventf(trustManager, corev1.EventTypeNormal, "Reconciled", "deployment resource %s applied", deploymentName)
-	r.log.V(2).Info("applied deployment", "name", deploymentName)
 	return nil
 }
 
@@ -66,10 +66,18 @@ func (r *Reconciler) getDeploymentObject(trustManager *v1alpha1.TrustManager, re
 		return nil, common.NewIrrecoverableError(err, "failed to update trust-manager image")
 	}
 
-	updateResourceRequirements(deployment, trustManager)
-	updateAffinityRules(deployment, trustManager)
-	updatePodTolerations(deployment, trustManager)
-	updateNodeSelector(deployment, trustManager)
+	if err := updateResourceRequirements(deployment, trustManager); err != nil {
+		return nil, err
+	}
+	if err := updateAffinityRules(deployment, trustManager); err != nil {
+		return nil, err
+	}
+	if err := updatePodTolerations(deployment, trustManager); err != nil {
+		return nil, err
+	}
+	if err := updateNodeSelector(deployment, trustManager); err != nil {
+		return nil, err
+	}
 
 	return deployment, nil
 }
@@ -146,35 +154,50 @@ func updateImage(deployment *appsv1.Deployment) error {
 	return nil
 }
 
-func updateResourceRequirements(deployment *appsv1.Deployment, trustManager *v1alpha1.TrustManager) {
+func updateResourceRequirements(deployment *appsv1.Deployment, trustManager *v1alpha1.TrustManager) error {
 	resources := trustManager.Spec.TrustManagerConfig.Resources
 	if len(resources.Limits) == 0 && len(resources.Requests) == 0 {
-		return
+		return nil
+	}
+	if err := common.ValidateResourceRequirements(resources, trustManagerConfigFieldPath); err != nil {
+		return err
 	}
 	for i := range deployment.Spec.Template.Spec.Containers {
 		if deployment.Spec.Template.Spec.Containers[i].Name == trustManagerContainerName {
 			deployment.Spec.Template.Spec.Containers[i].Resources = resources
 		}
 	}
+	return nil
 }
 
-func updateAffinityRules(deployment *appsv1.Deployment, trustManager *v1alpha1.TrustManager) {
+func updateAffinityRules(deployment *appsv1.Deployment, trustManager *v1alpha1.TrustManager) error {
 	if trustManager.Spec.TrustManagerConfig.Affinity == nil {
-		return
+		return nil
+	}
+	if err := common.ValidateAffinityRules(trustManager.Spec.TrustManagerConfig.Affinity, trustManagerConfigFieldPath); err != nil {
+		return err
 	}
 	deployment.Spec.Template.Spec.Affinity = trustManager.Spec.TrustManagerConfig.Affinity
+	return nil
 }
 
-func updatePodTolerations(deployment *appsv1.Deployment, trustManager *v1alpha1.TrustManager) {
+func updatePodTolerations(deployment *appsv1.Deployment, trustManager *v1alpha1.TrustManager) error {
 	if trustManager.Spec.TrustManagerConfig.Tolerations == nil {
-		return
+		return nil
+	}
+	if err := common.ValidateTolerationsConfig(trustManager.Spec.TrustManagerConfig.Tolerations, trustManagerConfigFieldPath); err != nil {
+		return err
 	}
 	deployment.Spec.Template.Spec.Tolerations = trustManager.Spec.TrustManagerConfig.Tolerations
+	return nil
 }
 
-func updateNodeSelector(deployment *appsv1.Deployment, trustManager *v1alpha1.TrustManager) {
+func updateNodeSelector(deployment *appsv1.Deployment, trustManager *v1alpha1.TrustManager) error {
 	if trustManager.Spec.TrustManagerConfig.NodeSelector == nil {
-		return
+		return nil
+	}
+	if err := common.ValidateNodeSelectorConfig(trustManager.Spec.TrustManagerConfig.NodeSelector, trustManagerConfigFieldPath); err != nil {
+		return err
 	}
 	if deployment.Spec.Template.Spec.NodeSelector == nil {
 		deployment.Spec.Template.Spec.NodeSelector = make(map[string]string)
@@ -184,6 +207,7 @@ func updateNodeSelector(deployment *appsv1.Deployment, trustManager *v1alpha1.Tr
 	for k, v := range trustManager.Spec.TrustManagerConfig.NodeSelector {
 		deployment.Spec.Template.Spec.NodeSelector[k] = v
 	}
+	return nil
 }
 
 func updateServiceAccountName(deployment *appsv1.Deployment) {