OCPBUGS-29208: Fix for ACM Policy Generator Plugin (#751) (#756) (#757)

* This commit fixes an issue happening when using policies generated with 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

* Addressing Jun's comments

* Addinitonal fixes for map key

* fix kuttl test

controllers/clustergroupupgrade_controller.go

@@ -828,7 +828,7 @@ func (r *ClusterGroupUpgradeReconciler) updatePlacementRules(ctx context.Context
 
 	for index, clusterNames := range policiesToUpdate {
 		placementRuleName := utils.GetResourceName(clusterGroupUpgrade, clusterGroupUpgrade.Status.ManagedPoliciesForUpgrade[index].Name+"-placement")
-		if safeName, ok := clusterGroupUpgrade.Status.SafeResourceNames[placementRuleName]; ok {
+		if safeName, ok := clusterGroupUpgrade.Status.SafeResourceNames[utils.PrefixNameWithNamespace(clusterGroupUpgrade.Status.ManagedPoliciesForUpgrade[index].Namespace, placementRuleName)]; ok {
 			err := r.updatePlacementRuleWithClusters(ctx, clusterGroupUpgrade, clusterNames, safeName)
 			if err != nil {
 				return err
@@ -1117,7 +1117,7 @@ func (r *ClusterGroupUpgradeReconciler) copyManagedInformPolicy(
 	if annotations == nil {
 		annotations = make(map[string]string)
 	}
-	annotations[utils.DesiredResourceName] = name
+	annotations[utils.DesiredResourceName] = utils.PrefixNameWithNamespace(managedPolicy.GetNamespace(), name)
 	newPolicy.SetAnnotations(annotations)
 
 	// Set new policy remediationAction.
@@ -1231,15 +1231,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.MaxPolicyNameLengthExcludingTheDot)
 	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.MaxPolicyNameLengthExcludingTheDot)
 	policy.SetName(safeName)
 	if err := controllerutil.SetControllerReference(clusterGroupUpgrade, policy, r.Scheme); err != nil {
 		return err
@@ -1277,7 +1277,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, managedPolicy.GetNamespace(), clusterGroupUpgrade, utils.MaxObjectNameLength)
 	pr := r.newBatchPlacementRule(clusterGroupUpgrade, policyName, safeName, name)
 
 	if err := controllerutil.SetControllerReference(clusterGroupUpgrade, pr, r.Scheme); err != nil {
@@ -1328,7 +1328,7 @@ func (r *ClusterGroupUpgradeReconciler) newBatchPlacementRule(clusterGroupUpgrad
 				utils.ExcludeFromClusterBackup:                         "true",
 			},
 			"annotations": map[string]interface{}{
-				utils.DesiredResourceName: desiredName,
+				utils.DesiredResourceName: utils.PrefixNameWithNamespace(clusterGroupUpgrade.Namespace, desiredName),
 			},
 		},
 		"spec": map[string]interface{}{
@@ -1444,9 +1444,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, managedPolicy.GetNamespace(), clusterGroupUpgrade, utils.MaxObjectNameLength)
 	// Ensure batch placement bindings.
 	pb := r.newBatchPlacementBinding(clusterGroupUpgrade, policyName, placementRuleName, safeName, name)
 
@@ -1507,7 +1506,7 @@ func (r *ClusterGroupUpgradeReconciler) newBatchPlacementBinding(clusterGroupUpg
 				utils.ExcludeFromClusterBackup:                         "true",
 			},
 			"annotations": map[string]interface{}{
-				utils.DesiredResourceName: desiredName,
+				utils.DesiredResourceName: utils.PrefixNameWithNamespace(clusterGroupUpgrade.Namespace, desiredName),
 			},
 		},
 		"placementRef": map[string]interface{}{

controllers/monitoring.go

@@ -210,7 +210,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)
 	mcv, err := utils.EnsureManagedClusterView(
 		ctx, r.Client, safeName, mcvName, clusterName, object.Kind+"."+strings.Split(object.APIVersion, "/")[0],
 		object.Name, *object.Namespace, clusterGroupUpgrade.Namespace+"-"+clusterGroupUpgrade.Name)

controllers/utils/common.go

@@ -3,6 +3,7 @@ package utils
 import (
 	"fmt"
 	"regexp"
+	"unicode/utf8"
 
 	ranv1alpha1 "github.com/openshift-kni/cluster-group-upgrades-operator/api/v1alpha1"
 	"k8s.io/apimachinery/pkg/util/rand"
@@ -32,25 +33,30 @@ 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) string {
 	if clusterGroupUpgrade.Status.SafeResourceNames == nil {
 		clusterGroupUpgrade.Status.SafeResourceNames = make(map[string]string)
 	}
-	safeName, ok := clusterGroupUpgrade.Status.SafeResourceNames[name]
+	safeName, ok := clusterGroupUpgrade.Status.SafeResourceNames[PrefixNameWithNamespace(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)
+		clusterGroupUpgrade.Status.SafeResourceNames[PrefixNameWithNamespace(namespace, name)] = safeName
 	}
 	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) (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 +65,17 @@ 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)
 }
+
+// PrefixNameWithNamespace Prefixes the passed name with the passed namespace. Use '/' as a separator
+func PrefixNameWithNamespace(namespace, name string) string {
+	return namespace + "/" + name
+}

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, "", MaxPolicyNameLengthExcludingTheDot)
+	assert.Equal(t, MaxPolicyNameLengthExcludingTheDot-len(namespace), len(safeName))
+	assert.Equal(t, name[:MaxPolicyNameLengthExcludingTheDot-len(namespace)-6]+"-", safeName[:MaxPolicyNameLengthExcludingTheDot-len(namespace)-5])
 
 	name = "cnfdf18-new-common-cnfdf18-looooong-subscriptions-policy-config"
-	safeName = NewSafeResourceName(name, "", MaxPolicyNameLength, 0)
-	assert.Equal(t, MaxPolicyNameLength, len(safeName))
-	assert.Equal(t, name[:MaxPolicyNameLength-7]+"-", safeName[:MaxPolicyNameLength-6])
+	safeName = NewSafeResourceName(name, "", "", MaxPolicyNameLengthExcludingTheDot)
+	assert.Equal(t, MaxPolicyNameLengthExcludingTheDot, len(safeName))
+	assert.Equal(t, name[:MaxPolicyNameLengthExcludingTheDot-6]+"-", safeName[:MaxPolicyNameLengthExcludingTheDot-5])
 
 	name = "cnfdf18-new-common-cnfdf18-looooong-subscriptions-policy-placement"
-	safeName = NewSafeResourceName(name, "", MaxObjectNameLength, 0)
+	safeName = NewSafeResourceName(name, "", "", MaxObjectNameLength)
 	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", MaxPolicyNameLengthExcludingTheDot)
+	assert.Equal(t, MaxPolicyNameLengthExcludingTheDot-len(namespace), len(safeName))
+	assert.Equal(t, name[:MaxPolicyNameLengthExcludingTheDot-len(namespace)-6]+"-kuttl", safeName)
 
 	name = "cnfdf18-new-common-cnfdf18-looooong-subscriptions-policy-config"
-	safeName = NewSafeResourceName(name, "kuttl", MaxPolicyNameLength, 0)
-	assert.Equal(t, MaxPolicyNameLength, len(safeName))
-	assert.Equal(t, name[:MaxPolicyNameLength-6]+"-kuttl", safeName)
+	safeName = NewSafeResourceName(name, "", "kuttl", MaxPolicyNameLengthExcludingTheDot)
+	assert.Equal(t, MaxPolicyNameLengthExcludingTheDot, len(safeName))
+	assert.Equal(t, name[:MaxPolicyNameLengthExcludingTheDot-6]+"-kuttl", safeName)
 
 	name = "cnfdf18-new-common-cnfdf18-looooong-subscriptions-policy-placement"
-	safeName = NewSafeResourceName(name, "kuttl", MaxObjectNameLength, 0)
+	safeName = NewSafeResourceName(name, "", "kuttl", MaxObjectNameLength)
 	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, "", "", MaxPolicyNameLengthExcludingTheDot)
+	assert.Equal(t, MaxPolicyNameLengthExcludingTheDot, len(safeName))
+	assert.Equal(t, name[:MaxPolicyNameLengthExcludingTheDot-6]+"-", safeName[:MaxPolicyNameLengthExcludingTheDot-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", MaxPolicyNameLengthExcludingTheDot)
+	assert.Equal(t, MaxPolicyNameLengthExcludingTheDot, len(safeName))
+	assert.Equal(t, name[:MaxPolicyNameLengthExcludingTheDot-9]+"-", safeName[:MaxPolicyNameLengthExcludingTheDot-8])
 }

controllers/utils/constants.go

@@ -41,10 +41,12 @@ const (
 
 // CR name length limits and suffix annotation
 const (
-	MaxPolicyNameLength    = 63
-	MaxObjectNameLength    = 253
-	NameSuffixAnnotation   = CsvNamePrefix + "/name-suffix"
-	RandomNameSuffixLength = 5
+	// 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)
+	MaxPolicyNameLengthExcludingTheDot = 62
+	MaxObjectNameLength                = 253
+	NameSuffixAnnotation               = CsvNamePrefix + "/name-suffix"
+	RandomNameSuffixLength             = 5
 )
 
 // Pre-cache constants

controllers/utils/multicloud_util.go

@@ -170,7 +170,7 @@ func EnsureManagedClusterView(
 					"openshift-cluster-group-upgrades/clusterGroupUpgrade": cguLabel,
 				},
 				Annotations: map[string]string{
-					DesiredResourceName: name,
+					DesiredResourceName: PrefixNameWithNamespace(namespace, name),
 				},
 			}
 			viewSpec := viewv1beta1.ViewSpec{

tests/kuttl/tests/adjust-max-concurrency/00-assert.yaml

@@ -18,15 +18,15 @@ status:
   conditions:
   - message: All selected clusters are valid
     reason: ClusterSelectionCompleted
-    status: "True"
+    status: 'True'
     type: ClustersSelected
   - message: Completed validation
     reason: ValidationCompleted
-    status: "True"
+    status: 'True'
     type: Validated
   - message: Not enabled
     reason: NotEnabled
-    status: "False"
+    status: 'False'
     type: Progressing
   copiedPolicies:
   - cgu-adjust-max-conc-policy1-common-cluster-versio-kuttl
@@ -50,10 +50,10 @@ status:
   remediationPlan:
   - - spoke1
   safeResourceNames:
-    cgu-adjust-max-conc-common-cluster-version-policy-config: cgu-adjust-max-conc-common-cluster-version-policy-config-kuttl
-    cgu-adjust-max-conc-common-pao-sub-policy-config: cgu-adjust-max-conc-common-pao-sub-policy-config-kuttl
-    cgu-adjust-max-conc-policy1-common-cluster-version-policy: cgu-adjust-max-conc-policy1-common-cluster-versio-kuttl
-    cgu-adjust-max-conc-policy1-common-cluster-version-policy-placement: cgu-adjust-max-conc-policy1-common-cluster-version-policy-placement-kuttl
-    cgu-adjust-max-conc-policy2-common-pao-sub-policy: cgu-adjust-max-conc-policy2-common-pao-sub-policy-kuttl
-    cgu-adjust-max-conc-policy2-common-pao-sub-policy-placement: cgu-adjust-max-conc-policy2-common-pao-sub-policy-placement-kuttl
+    /cgu-adjust-max-conc-common-cluster-version-policy-config: cgu-adjust-max-conc-common-cluster-version-policy-config-kuttl
+    /cgu-adjust-max-conc-common-pao-sub-policy-config: cgu-adjust-max-conc-common-pao-sub-policy-config-kuttl
+    default/cgu-adjust-max-conc-policy1-common-cluster-version-policy: cgu-adjust-max-conc-policy1-common-cluster-versio-kuttl
+    default/cgu-adjust-max-conc-policy1-common-cluster-version-policy-placement: cgu-adjust-max-conc-policy1-common-cluster-version-policy-placement-kuttl
+    default/cgu-adjust-max-conc-policy2-common-pao-sub-policy: cgu-adjust-max-conc-policy2-common-pao-sub-policy-kuttl
+    default/cgu-adjust-max-conc-policy2-common-pao-sub-policy-placement: cgu-adjust-max-conc-policy2-common-pao-sub-policy-placement-kuttl
   status: {}

tests/kuttl/tests/backup-complete/00-assert.yaml

@@ -30,19 +30,19 @@ status:
   conditions:
   - message: All selected clusters are valid
     reason: ClusterSelectionCompleted
-    status: "True"
+    status: 'True'
     type: ClustersSelected
   - message: Completed validation
     reason: ValidationCompleted
-    status: "True"
+    status: 'True'
     type: Validated
   - message: Backup in progress for 4 clusters
     reason: InProgress
-    status: "False"
+    status: 'False'
     type: BackupSuceeded
   - message: Cluster backup is in progress
     reason: NotStarted
-    status: "False"
+    status: 'False'
     type: Progressing
   copiedPolicies:
   - cgu-policy3-common-ptp-sub-policy-kuttl
@@ -73,14 +73,13 @@ status:
     - spoke1
     - spoke5
   safeResourceNames:
-    cgu-common-ptp-sub-policy-config: cgu-common-ptp-sub-policy-config-kuttl
-    cgu-common-sriov-sub-policy-config: cgu-common-sriov-sub-policy-config-kuttl
-    cgu-policy3-common-ptp-sub-policy: cgu-policy3-common-ptp-sub-policy-kuttl
-    cgu-policy3-common-ptp-sub-policy-placement: cgu-policy3-common-ptp-sub-policy-placement-kuttl
-    cgu-policy4-common-sriov-sub-policy: cgu-policy4-common-sriov-sub-policy-kuttl
-    cgu-policy4-common-sriov-sub-policy-placement: cgu-policy4-common-sriov-sub-policy-placement-kuttl
+    /cgu-common-ptp-sub-policy-config: cgu-common-ptp-sub-policy-config-kuttl
+    /cgu-common-sriov-sub-policy-config: cgu-common-sriov-sub-policy-config-kuttl
+    default/cgu-policy3-common-ptp-sub-policy: cgu-policy3-common-ptp-sub-policy-kuttl
+    default/cgu-policy3-common-ptp-sub-policy-placement: cgu-policy3-common-ptp-sub-policy-placement-kuttl
+    default/cgu-policy4-common-sriov-sub-policy: cgu-policy4-common-sriov-sub-policy-kuttl
+    default/cgu-policy4-common-sriov-sub-policy-placement: cgu-policy4-common-sriov-sub-policy-placement-kuttl
 ---
-#MCAs
 apiVersion: action.open-cluster-management.io/v1beta1
 kind: ManagedClusterAction
 metadata:
@@ -123,7 +122,7 @@ spec:
   actionType: Delete
   kube:
     name: backup-agent
-    resource: clusterrolebinding    
+    resource: clusterrolebinding
 ---
 apiVersion: action.open-cluster-management.io/v1beta1
 kind: ManagedClusterAction
@@ -145,7 +144,7 @@ spec:
   actionType: Delete
   kube:
     name: backup-agent
-    resource: clusterrolebinding    
+    resource: clusterrolebinding
 ---
 apiVersion: action.open-cluster-management.io/v1beta1
 kind: ManagedClusterAction
@@ -167,9 +166,8 @@ spec:
   actionType: Delete
   kube:
     name: backup-agent
-    resource: clusterrolebinding    
+    resource: clusterrolebinding
 ---
-#MCVs
 apiVersion: view.open-cluster-management.io/v1beta1
 kind: ManagedClusterView
 metadata: