This commit fixes an issue happening when using policies generated wi…

…th ACM Policy Generator Plugin.

For ConfigurationPolicy kind, the name of the object and the inner template are the same when generated with ACM Gen.
In comparison the PGT plugin, adds "-config" to the name of the inner template. this leads to overloaded keys in the
clusterGroupUpgrade.Status.SafeResourceNames map.

Changes:
 - namespace+"/"+name key used in the clusterGroupUpgrade.Status.SafeResourceNames map
 - use of namespace instead of namespace length in GetSafeResourceName
 - use of utf8.RuneCountInString() instead of len to support non english characters (see policy web hook at https://github.com/stolostron/governance-policy-propagator/blob/feb9abb241954d7ce64054d7c621b423deece8cf/api/v1/policy_webhook.go#L86-L87 ).
 - adding logs to display the safenames and lenghts

controllers/clustergroupupgrade_controller.go

@@ -1234,15 +1234,15 @@ func (r *ClusterGroupUpgradeReconciler) updateConfigurationPolicyName(
 
 	metadataContent := metadata.(map[string]interface{})
 	name := utils.GetResourceName(clusterGroupUpgrade, metadataContent["name"].(string))
-	safeName := utils.GetSafeResourceName(name, clusterGroupUpgrade, utils.MaxPolicyNameLength, 0)
+	safeName := utils.GetSafeResourceName(name, "", clusterGroupUpgrade, utils.MaxPolicyNameLength, &r.Log)
 	metadataContent["name"] = safeName
 }
 
 func (r *ClusterGroupUpgradeReconciler) createNewPolicyFromStructure(
 	ctx context.Context, clusterGroupUpgrade *ranv1alpha1.ClusterGroupUpgrade, policy *unstructured.Unstructured) error {
 
 	name := policy.GetName()
-	safeName := utils.GetSafeResourceName(name, clusterGroupUpgrade, utils.MaxPolicyNameLength, len(policy.GetNamespace())+1)
+	safeName := utils.GetSafeResourceName(name, policy.GetNamespace(), clusterGroupUpgrade, utils.MaxPolicyNameLength, &r.Log)
 	policy.SetName(safeName)
 	if err := controllerutil.SetControllerReference(clusterGroupUpgrade, policy, r.Scheme); err != nil {
 		return err
@@ -1280,7 +1280,7 @@ func (r *ClusterGroupUpgradeReconciler) createNewPolicyFromStructure(
 func (r *ClusterGroupUpgradeReconciler) ensureBatchPlacementRule(ctx context.Context, clusterGroupUpgrade *ranv1alpha1.ClusterGroupUpgrade, policyName string, managedPolicy *unstructured.Unstructured) (string, error) {
 
 	name := utils.GetResourceName(clusterGroupUpgrade, managedPolicy.GetName()+"-placement")
-	safeName := utils.GetSafeResourceName(name, clusterGroupUpgrade, utils.MaxObjectNameLength, 0)
+	safeName := utils.GetSafeResourceName(name, "", clusterGroupUpgrade, utils.MaxObjectNameLength, &r.Log)
 	pr := r.newBatchPlacementRule(clusterGroupUpgrade, policyName, safeName, name)
 
 	if err := controllerutil.SetControllerReference(clusterGroupUpgrade, pr, r.Scheme); err != nil {
@@ -1447,9 +1447,8 @@ func (r *ClusterGroupUpgradeReconciler) isUpgradeComplete(ctx context.Context, c
 
 func (r *ClusterGroupUpgradeReconciler) ensureBatchPlacementBinding(
 	ctx context.Context, clusterGroupUpgrade *ranv1alpha1.ClusterGroupUpgrade, policyName, placementRuleName string, managedPolicy *unstructured.Unstructured) error {
-
 	name := utils.GetResourceName(clusterGroupUpgrade, managedPolicy.GetName()+"-placement")
-	safeName := utils.GetSafeResourceName(name, clusterGroupUpgrade, utils.MaxObjectNameLength, 0)
+	safeName := utils.GetSafeResourceName(name, "", clusterGroupUpgrade, utils.MaxObjectNameLength, &r.Log)
 	// Ensure batch placement bindings.
 	pb := r.newBatchPlacementBinding(clusterGroupUpgrade, policyName, placementRuleName, safeName, name)
 

controllers/monitoring.go

@@ -206,7 +206,7 @@ func (r *ClusterGroupUpgradeReconciler) processMonitoredObject(
 	// Get the managedClusterView for the monitored object contained in the current managedPolicy.
 	// If missing, then return error.
 	mcvName := utils.GetMultiCloudObjectName(clusterGroupUpgrade, object.Kind, object.Name)
-	safeName := utils.GetSafeResourceName(mcvName, clusterGroupUpgrade, utils.MaxObjectNameLength, 0)
+	safeName := utils.GetSafeResourceName(mcvName, "", clusterGroupUpgrade, utils.MaxObjectNameLength, &r.Log)
 	mcv, err := utils.EnsureManagedClusterView(
 		ctx, r.Client, safeName, mcvName, clusterName, object.Kind+"."+strings.Split(object.APIVersion, "/")[0],
 		object.Name, *object.Namespace, clusterGroupUpgrade.Name, clusterGroupUpgrade.Namespace)

controllers/utils/common.go

@@ -3,7 +3,9 @@ package utils
 import (
 	"fmt"
 	"regexp"
+	"unicode/utf8"
 
+	"github.com/go-logr/logr"
 	ranv1alpha1 "github.com/openshift-kni/cluster-group-upgrades-operator/pkg/api/clustergroupupgrades/v1alpha1"
 	"k8s.io/apimachinery/pkg/util/rand"
 )
@@ -32,25 +34,37 @@ func GetMinOf3(number1, number2, number3 int) int {
 }
 
 // GetSafeResourceName returns the safename if already allocated in the map and creates a new one if not
-func GetSafeResourceName(name string, clusterGroupUpgrade *ranv1alpha1.ClusterGroupUpgrade, maxLength, spareLength int) string {
+func GetSafeResourceName(name, namespace string, clusterGroupUpgrade *ranv1alpha1.ClusterGroupUpgrade, maxLength int, log *logr.Logger) string {
 	if clusterGroupUpgrade.Status.SafeResourceNames == nil {
 		clusterGroupUpgrade.Status.SafeResourceNames = make(map[string]string)
 	}
-	safeName, ok := clusterGroupUpgrade.Status.SafeResourceNames[name]
+	safeName, ok := clusterGroupUpgrade.Status.SafeResourceNames[namespace+"/"+name]
+
 	if !ok {
-		safeName = NewSafeResourceName(name, clusterGroupUpgrade.GetAnnotations()[NameSuffixAnnotation], maxLength, spareLength)
-		clusterGroupUpgrade.Status.SafeResourceNames[name] = safeName
+		safeName = NewSafeResourceName(name, namespace, clusterGroupUpgrade.GetAnnotations()[NameSuffixAnnotation], maxLength, log)
+		clusterGroupUpgrade.Status.SafeResourceNames[namespace+"/"+name] = safeName
+	} else {
+		if log != nil {
+			log.Info("safename already in  clusterGroupUpgrade.Status.SafeResourceNames",
+				"safename", safeName,
+				"namespace", namespace,
+				"safeName+namespace length <= 62", utf8.RuneCountInString(safeName+namespace))
+		}
 	}
 	return safeName
 }
 
+const (
+	finalDashLength = 1
+)
+
 // NewSafeResourceName creates a safe name to use with random suffix and possible truncation based on limits passed in
-func NewSafeResourceName(name, suffix string, maxLength, spareLength int) string {
+func NewSafeResourceName(name, namespace, suffix string, maxLength int, log *logr.Logger) (safename string) {
 	if suffix == "" {
 		suffix = rand.String(RandomNameSuffixLength)
 	}
-	suffixLength := len(suffix)
-	maxGeneratedNameLength := maxLength - suffixLength - spareLength - 1
+	suffixLength := utf8.RuneCountInString(suffix)
+	maxGeneratedNameLength := maxLength - suffixLength - utf8.RuneCountInString(namespace) - finalDashLength
 	var base string
 	if len(name) > maxGeneratedNameLength {
 		base = name[:maxGeneratedNameLength]
@@ -59,12 +73,22 @@ func NewSafeResourceName(name, suffix string, maxLength, spareLength int) string
 	}
 
 	// Make sure base ends in '-' or an alphanumerical character.
-	for !regexp.MustCompile(`^[a-zA-Z0-9-]*$`).MatchString(base[len(base)-1:]) {
-		base = base[:len(base)-1]
+	for !regexp.MustCompile(`^[a-zA-Z0-9-]*$`).MatchString(base[utf8.RuneCountInString(base)-1:]) {
+		base = base[:utf8.RuneCountInString(base)-1]
 	}
 
 	// The newSafeResourceName should match regex
 	// `[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*` as per
 	// https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
-	return fmt.Sprintf("%s-%s", base, suffix)
+
+	safename = fmt.Sprintf("%s-%s", base, suffix)
+
+	if log != nil {
+		log.Info("safename",
+			"safename", safename,
+			"namespace", namespace,
+			"maxGeneratedNameLength", maxGeneratedNameLength,
+			"safeName+namespace length <= 62", utf8.RuneCountInString(safename+namespace))
+	}
+	return safename
 }

controllers/utils/common_test.go

@@ -10,42 +10,42 @@ func TestNewSafeResourceNames(t *testing.T) {
 
 	name := "cnfdf18-new-common-cnfdf18-looooong-subscriptions-policy"
 	namespace := "ztp-install"
-	safeName := NewSafeResourceName(name, "", MaxPolicyNameLength, len(namespace)+1)
-	assert.Equal(t, MaxPolicyNameLength-len(namespace)-1, len(safeName))
-	assert.Equal(t, name[:MaxPolicyNameLength-len(namespace)-7]+"-", safeName[:MaxPolicyNameLength-len(namespace)-6])
+	safeName := NewSafeResourceName(name, namespace, "", MaxPolicyNameLength, nil)
+	assert.Equal(t, MaxPolicyNameLength-len(namespace), len(safeName))
+	assert.Equal(t, name[:MaxPolicyNameLength-len(namespace)-6]+"-", safeName[:MaxPolicyNameLength-len(namespace)-5])
 
 	name = "cnfdf18-new-common-cnfdf18-looooong-subscriptions-policy-config"
-	safeName = NewSafeResourceName(name, "", MaxPolicyNameLength, 0)
+	safeName = NewSafeResourceName(name, "", "", MaxPolicyNameLength, nil)
 	assert.Equal(t, MaxPolicyNameLength, len(safeName))
-	assert.Equal(t, name[:MaxPolicyNameLength-7]+"-", safeName[:MaxPolicyNameLength-6])
+	assert.Equal(t, name[:MaxPolicyNameLength-6]+"-", safeName[:MaxPolicyNameLength-5])
 
 	name = "cnfdf18-new-common-cnfdf18-looooong-subscriptions-policy-placement"
-	safeName = NewSafeResourceName(name, "", MaxObjectNameLength, 0)
+	safeName = NewSafeResourceName(name, "", "", MaxObjectNameLength, nil)
 	assert.Equal(t, len(name)+6, len(safeName))
 	assert.Equal(t, name+"-", safeName[:len(name)+1])
 
 	name = "cnfdf18-new-common-cnfdf18-looooong-subscriptions-policy"
-	safeName = NewSafeResourceName(name, "kuttl", MaxPolicyNameLength, len(namespace)+1)
-	assert.Equal(t, MaxPolicyNameLength-len(namespace)-1, len(safeName))
-	assert.Equal(t, name[:MaxPolicyNameLength-len(namespace)-7]+"-kuttl", safeName)
+	safeName = NewSafeResourceName(name, namespace, "kuttl", MaxPolicyNameLength, nil)
+	assert.Equal(t, MaxPolicyNameLength-len(namespace), len(safeName))
+	assert.Equal(t, name[:MaxPolicyNameLength-len(namespace)-6]+"-kuttl", safeName)
 
 	name = "cnfdf18-new-common-cnfdf18-looooong-subscriptions-policy-config"
-	safeName = NewSafeResourceName(name, "kuttl", MaxPolicyNameLength, 0)
+	safeName = NewSafeResourceName(name, "", "kuttl", MaxPolicyNameLength, nil)
 	assert.Equal(t, MaxPolicyNameLength, len(safeName))
 	assert.Equal(t, name[:MaxPolicyNameLength-6]+"-kuttl", safeName)
 
 	name = "cnfdf18-new-common-cnfdf18-looooong-subscriptions-policy-placement"
-	safeName = NewSafeResourceName(name, "kuttl", MaxObjectNameLength, 0)
+	safeName = NewSafeResourceName(name, "", "kuttl", MaxObjectNameLength, nil)
 	assert.Equal(t, len(name)+6, len(safeName))
 	assert.Equal(t, name+"-kuttl", safeName)
 
 	name = "cgu-sriov-cloudransno-site9-spree-lb-du-cvslcm-4.14.0-rc.4-config"
-	safeName = NewSafeResourceName(name, "", MaxPolicyNameLength, 0)
-	assert.Equal(t, MaxPolicyNameLength-1, len(safeName))
-	assert.Equal(t, name[:MaxPolicyNameLength-7]+"-", safeName[:MaxPolicyNameLength-6])
+	safeName = NewSafeResourceName(name, "", "", MaxPolicyNameLength, nil)
+	assert.Equal(t, MaxPolicyNameLength, len(safeName))
+	assert.Equal(t, name[:MaxPolicyNameLength-6]+"-", safeName[:MaxPolicyNameLength-5])
 
 	name = "cgu-sriov-cloudransno-site9-spree-lb-du-cvslcm-4.14.0-rc.4"
-	safeName = NewSafeResourceName(name, "", MaxPolicyNameLength, 8)
-	assert.Equal(t, MaxPolicyNameLength-9, len(safeName))
-	assert.Equal(t, name[:MaxPolicyNameLength-15]+"-", safeName[:MaxPolicyNameLength-14])
+	safeName = NewSafeResourceName(name, "", "12345678", MaxPolicyNameLength, nil)
+	assert.Equal(t, MaxPolicyNameLength, len(safeName))
+	assert.Equal(t, name[:MaxPolicyNameLength-9]+"-", safeName[:MaxPolicyNameLength-8])
 }

controllers/utils/constants.go

@@ -41,7 +41,9 @@ const (
 
 // CR name length limits and suffix annotation
 const (
-	MaxPolicyNameLength    = 63
+	// Maximum length of policy + namespace (not including extra separator dot so 63 -1 = 62)
+	// this is calculated with utf8.RuneCountInString(policy.Name)+utf8.RuneCountInString+utf8.RuneCountInString(policy.Namespace)
+	MaxPolicyNameLength    = 62
 	MaxObjectNameLength    = 253
 	NameSuffixAnnotation   = CsvNamePrefix + "/name-suffix"
 	RandomNameSuffixLength = 5