fix: sort and remove empty ResourceNames in role for secrets consistency (cloudnative-pg#2875)

Addresses two inconsistencies in the `ResourceName` sections of the generated roles:
- Ensures entries are sorted, preventing items from appearing out of order.
- Removes empty items

This is achieved using a new utility function, `cleanupResourceList`.

Signed-off-by: YanniHu1996 <yantian.hu@enterprisedb.com>
Signed-off-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Leonardo Cecchi <leonardo.cecchi@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>

Author: 3 people

pkg/specs/roles.go

@@ -17,44 +17,16 @@ limitations under the License.
 package specs
 
 import (
+	"golang.org/x/exp/slices"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	apiv1 "github.com/cloudnative-pg/cloudnative-pg/api/v1"
+	"github.com/cloudnative-pg/cloudnative-pg/pkg/stringset"
 )
 
 // CreateRole create a role with the permissions needed by the instance manager
 func CreateRole(cluster apiv1.Cluster, backupOrigin *apiv1.Backup) rbacv1.Role {
-	involvedSecretNames := []string{
-		cluster.GetReplicationSecretName(),
-		cluster.GetClientCASecretName(),
-		cluster.GetServerCASecretName(),
-		cluster.GetServerTLSSecretName(),
-		cluster.GetApplicationSecretName(),
-		cluster.GetSuperuserSecretName(),
-		cluster.GetLDAPSecretName(),
-	}
-
-	involvedConfigMapNames := []string{
-		cluster.Name,
-	}
-
-	if cluster.Spec.Monitoring != nil {
-		// If custom queries are used, the instance manager need privileges to read those
-		// entries
-		for _, secretName := range cluster.Spec.Monitoring.CustomQueriesSecret {
-			involvedSecretNames = append(involvedSecretNames, secretName.Name)
-		}
-
-		for _, configMapName := range cluster.Spec.Monitoring.CustomQueriesConfigMap {
-			involvedConfigMapNames = append(involvedConfigMapNames, configMapName.Name)
-		}
-	}
-
-	involvedSecretNames = append(involvedSecretNames, backupSecrets(cluster, backupOrigin)...)
-	involvedSecretNames = append(involvedSecretNames, externalClusterSecrets(cluster)...)
-	involvedSecretNames = append(involvedSecretNames, managedRolesSecrets(cluster)...)
-
 	rules := []rbacv1.PolicyRule{
 		{
 			APIGroups: []string{
@@ -67,7 +39,7 @@ func CreateRole(cluster apiv1.Cluster, backupOrigin *apiv1.Backup) rbacv1.Role {
 				"get",
 				"watch",
 			},
-			ResourceNames: involvedConfigMapNames,
+			ResourceNames: getInvolvedConfigMapNames(cluster),
 		},
 		{
 			APIGroups: []string{
@@ -80,7 +52,7 @@ func CreateRole(cluster apiv1.Cluster, backupOrigin *apiv1.Backup) rbacv1.Role {
 				"get",
 				"watch",
 			},
-			ResourceNames: involvedSecretNames,
+			ResourceNames: getInvolvedSecretNames(cluster, backupOrigin),
 		},
 		{
 			APIGroups: []string{
@@ -164,6 +136,55 @@ func CreateRole(cluster apiv1.Cluster, backupOrigin *apiv1.Backup) rbacv1.Role {
 	}
 }
 
+func getInvolvedSecretNames(cluster apiv1.Cluster, backupOrigin *apiv1.Backup) []string {
+	involvedSecretNames := []string{
+		cluster.GetReplicationSecretName(),
+		cluster.GetClientCASecretName(),
+		cluster.GetServerCASecretName(),
+		cluster.GetServerTLSSecretName(),
+		cluster.GetApplicationSecretName(),
+		cluster.GetSuperuserSecretName(),
+		cluster.GetLDAPSecretName(),
+	}
+
+	if cluster.Spec.Monitoring != nil {
+		for _, secretName := range cluster.Spec.Monitoring.CustomQueriesSecret {
+			involvedSecretNames = append(involvedSecretNames, secretName.Name)
+		}
+	}
+
+	involvedSecretNames = append(involvedSecretNames, backupSecrets(cluster, backupOrigin)...)
+	involvedSecretNames = append(involvedSecretNames, externalClusterSecrets(cluster)...)
+	involvedSecretNames = append(involvedSecretNames, managedRolesSecrets(cluster)...)
+
+	return cleanupResourceList(involvedSecretNames)
+}
+
+func getInvolvedConfigMapNames(cluster apiv1.Cluster) []string {
+	involvedConfigMapNames := []string{
+		cluster.Name,
+	}
+
+	if cluster.Spec.Monitoring != nil {
+		// If custom queries are used, the instance manager need privileges to read those
+		// entries
+		for _, configMapName := range cluster.Spec.Monitoring.CustomQueriesConfigMap {
+			involvedConfigMapNames = append(involvedConfigMapNames, configMapName.Name)
+		}
+	}
+
+	return cleanupResourceList(involvedConfigMapNames)
+}
+
+// cleanupResourceList returns a new list with the same elements as resourceList, where
+// the empty and duplicate entries have been removed
+func cleanupResourceList(resourceList []string) []string {
+	result := stringset.From(resourceList).ToSortedList()
+	return slices.DeleteFunc(result, func(s string) bool {
+		return len(s) == 0
+	})
+}
+
 func externalClusterSecrets(cluster apiv1.Cluster) []string {
 	var result []string
 

pkg/specs/roles_test.go

@@ -186,12 +186,50 @@ var _ = Describe("Roles", func() {
 })
 
 var _ = Describe("Secrets", func() {
-	cluster := apiv1.Cluster{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "thisTest",
-			Namespace: "default",
-		},
-	}
+	var (
+		cluster apiv1.Cluster
+		backup  apiv1.Backup
+	)
+
+	BeforeEach(func() {
+		cluster = apiv1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "thisTest",
+				Namespace: "default",
+			},
+		}
+		backup = apiv1.Backup{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "testBackup",
+				Namespace: "default",
+			},
+			Status: apiv1.BackupStatus{
+				BarmanCredentials: apiv1.BarmanCredentials{
+					AWS: &apiv1.S3Credentials{
+						AccessKeyIDReference: &apiv1.SecretKeySelector{
+							LocalObjectReference: apiv1.LocalObjectReference{
+								Name: "aws-status-secret-test",
+							},
+						},
+					},
+					Azure: &apiv1.AzureCredentials{
+						StorageKey: &apiv1.SecretKeySelector{
+							LocalObjectReference: apiv1.LocalObjectReference{
+								Name: "azure-storage-key-secret-test",
+							},
+						},
+					},
+					Google: &apiv1.GoogleCredentials{
+						ApplicationCredentials: &apiv1.SecretKeySelector{
+							LocalObjectReference: apiv1.LocalObjectReference{
+								Name: "google-application-secret-test",
+							},
+						},
+					},
+				},
+			},
+		}
+	})
 
 	It("are properly backed up", func() {
 		secrets := backupSecrets(cluster, nil)
@@ -220,6 +258,29 @@ var _ = Describe("Secrets", func() {
 		secrets = backupSecrets(cluster, nil)
 		Expect(secrets).To(ConsistOf("test-secret", "test-access", "test-endpoint-ca-name"))
 	})
+
+	It("should contain default secrets only", func() {
+		Expect(getInvolvedSecretNames(cluster, nil)).To(Equal([]string{
+			"thisTest-app",
+			"thisTest-ca",
+			"thisTest-replication",
+			"thisTest-server",
+			"thisTest-superuser",
+		}))
+	})
+
+	It("should created an ordered string list with the backup secrets", func() {
+		Expect(getInvolvedSecretNames(cluster, &backup)).To(Equal([]string{
+			"aws-status-secret-test",
+			"azure-storage-key-secret-test",
+			"google-application-secret-test",
+			"thisTest-app",
+			"thisTest-ca",
+			"thisTest-replication",
+			"thisTest-server",
+			"thisTest-superuser",
+		}))
+	})
 })
 
 var _ = Describe("Managed Roles", func() {

pkg/stringset/stringset.go

@@ -17,6 +17,10 @@ limitations under the License.
 // Package stringset implements a basic set of strings
 package stringset
 
+import (
+	"golang.org/x/exp/slices"
+)
+
 // Data represent a set of strings
 type Data struct {
 	innerMap map[string]struct{}
@@ -50,7 +54,7 @@ func (set *Data) Delete(key string) {
 	delete(set.innerMap, key)
 }
 
-// Has check if a string is in the set or not
+// Has checks if a string is in the set or not
 func (set *Data) Has(key string) bool {
 	_, ok := set.innerMap[key]
 	return ok
@@ -71,6 +75,14 @@ func (set *Data) ToList() (result []string) {
 	return
 }
 
+// ToSortedList returns the string container in this set
+// as a sorted string slice
+func (set *Data) ToSortedList() []string {
+	result := set.ToList()
+	slices.Sort(result)
+	return result
+}
+
 // Eq compares two string sets for equality
 func (set *Data) Eq(other *Data) bool {
 	if set == nil || other == nil {

pkg/stringset/stringset_test.go

@@ -57,4 +57,10 @@ var _ = Describe("String set", func() {
 		Expect(From([]string{"one", "two"}).Eq(From([]string{"one", "two", "three"}))).To(BeFalse())
 		Expect(From([]string{"one", "two", "three"}).Eq(From([]string{"one", "two"}))).To(BeFalse())
 	})
+
+	It("constructs a sorted string slice given a set", func() {
+		Expect(From([]string{"one", "two", "three", "four"}).ToSortedList()).To(
+			HaveExactElements("four", "one", "three", "two"))
+		Expect(New().ToList()).To(BeEmpty())
+	})
 })