From 9dd75692bd9ddade603918f4a6cd424d655b8b1e Mon Sep 17 00:00:00 2001 From: Umanga Chapagain Date: Wed, 29 Nov 2023 15:22:59 +0530 Subject: [PATCH] validate if cluster FSID is empty When generating replicationID in CreateUniqueReplicationId(), ensure that at least 2 non-empty cluster FSID strings are available. Also, refactor to include sorting logic to enure consistent hashing. Signed-off-by: Umanga Chapagain --- .../agent_mirrorpeer_controller.go | 11 ++- controllers/drpolicy_controller.go | 12 +--- controllers/utils/hash.go | 18 ++++- controllers/utils/hash_test.go | 69 ++++++++++++++++++- 4 files changed, 91 insertions(+), 19 deletions(-) diff --git a/addons/token-exchange/agent_mirrorpeer_controller.go b/addons/token-exchange/agent_mirrorpeer_controller.go index 9b30025a..0061c2ea 100644 --- a/addons/token-exchange/agent_mirrorpeer_controller.go +++ b/addons/token-exchange/agent_mirrorpeer_controller.go @@ -21,7 +21,6 @@ import ( "crypto/sha1" "encoding/hex" "fmt" - "sort" "strconv" "time" @@ -597,14 +596,12 @@ func (r *MirrorPeerReconciler) labelCephClusters(ctx context.Context, scr *multi if found.Labels == nil { found.Labels = make(map[string]string) } - var fsids []string - for _, v := range clusterFSIDs { - fsids = append(fsids, v) + replicationId, err := utils.CreateUniqueReplicationId(clusterFSIDs) + if err != nil { + return err } - // To ensure reliability of hash generation - sort.Strings(fsids) - replicationId := utils.CreateUniqueReplicationId(fsids) + if found.Labels[utils.CephClusterReplicationIdLabel] != replicationId { klog.Infof("adding label %s/%s to cephcluster %s", utils.CephClusterReplicationIdLabel, replicationId, found.Name) found.Labels[utils.CephClusterReplicationIdLabel] = replicationId diff --git a/controllers/drpolicy_controller.go b/controllers/drpolicy_controller.go index 1eb4486d..becb1eaa 100644 --- a/controllers/drpolicy_controller.go +++ b/controllers/drpolicy_controller.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "fmt" - "sort" "time" "github.com/red-hat-storage/odf-multicluster-orchestrator/addons/setup" @@ -166,16 +165,11 @@ func (r *DRPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c func (r *DRPolicyReconciler) createOrUpdateManifestWorkForVRC(ctx context.Context, mp *multiclusterv1alpha1.MirrorPeer, dp *ramenv1alpha1.DRPolicy, clusterFSIDs map[string]string) error { - var fsids []string - for _, v := range clusterFSIDs { - fsids = append(fsids, v) + replicationId, err := utils.CreateUniqueReplicationId(clusterFSIDs) + if err != nil { + return err } - // To ensure reliability of hash generation - sort.Strings(fsids) - - replicationId := utils.CreateUniqueReplicationId(fsids) - for _, pr := range mp.Spec.Items { manifestWorkName := fmt.Sprintf("vrc-%v", utils.FnvHash(dp.Name)) // Two ManifestWork per DRPolicy found := &workv1.ManifestWork{ diff --git a/controllers/utils/hash.go b/controllers/utils/hash.go index 50b01d96..fd9b88b8 100644 --- a/controllers/utils/hash.go +++ b/controllers/utils/hash.go @@ -4,6 +4,7 @@ import ( "crypto/sha512" "fmt" "hash/fnv" + "sort" "strings" ) @@ -34,6 +35,19 @@ func CreateUniqueSecretName(managedCluster, storageClusterNamespace, storageClus return CreateUniqueName(managedCluster, storageClusterNamespace, storageClusterName)[0:39] } -func CreateUniqueReplicationId(fsids []string) string { - return CreateUniqueName(fsids...)[0:39] +func CreateUniqueReplicationId(clusterFSIDs map[string]string) (string, error) { + var fsids []string + for _, v := range clusterFSIDs { + if v != "" { + fsids = append(fsids, v) + } + } + + if len(fsids) < 2 { + return "", fmt.Errorf("replicationID can not be generated due to missing cluster FSID") + } + + // To ensure reliability of hash generation + sort.Strings(fsids) + return CreateUniqueName(fsids...)[0:39], nil } diff --git a/controllers/utils/hash_test.go b/controllers/utils/hash_test.go index d13ed083..545c87de 100644 --- a/controllers/utils/hash_test.go +++ b/controllers/utils/hash_test.go @@ -1,6 +1,8 @@ package utils -import "testing" +import ( + "testing" +) func TestFnvHashTest(t *testing.T) { testCases := []struct { @@ -19,3 +21,68 @@ func TestFnvHashTest(t *testing.T) { } } } + +func TestCreateUniqueReplicationId(t *testing.T) { + type args struct { + fsids map[string]string + } + tests := []struct { + name string + args args + want string + wantErr bool + }{ + { + name: "Case 1: Both FSID available", + args: args{ + fsids: map[string]string{ + "c1": "7e252ee3-abd9-4c54-a4ff-a2fdce8931a0", + "c2": "aacfbd7e-5ced-42a5-bdc2-483fcbe5a29d", + }, + }, + want: "a99df9fc6c52c7ef44222ab38657a0b15628a14", + wantErr: false, + }, + { + name: "Case 2: FSID for C1 is available", + args: args{ + fsids: map[string]string{ + "c1": "7e252ee3-abd9-4c54-a4ff-a2fdce8931a0", + "c2": "", + }, + }, + want: "", + wantErr: true, + }, + { + name: "Case 3: FSID for C2 is available", + args: args{ + fsids: map[string]string{ + "c1": "", + "c2": "aacfbd7e-5ced-42a5-bdc2-483fcbe5a29d", + }, + }, + want: "", + wantErr: true, + }, + { + name: "Case 4: Both FSID unavailable", + args: args{ + fsids: map[string]string{ + "c1": "", + "c2": "", + }, + }, + want: "", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := CreateUniqueReplicationId(tt.args.fsids) + if ((err != nil) != tt.wantErr) || got != tt.want { + t.Errorf("CreateUniqueReplicationId() = %v, want %v", got, tt.want) + } + }) + } +}