EVEREST-1225 don't edit storages in use

api/validation.go

@@ -101,6 +101,7 @@ var (
 	errDuplicatedStoragePG           = errors.New("postgres clusters can't use the same storage for the different schedules")
 	errStorageChangePG               = errors.New("the existing postgres schedules can't change their storage")
 	errDuplicatedBackupStorage       = errors.New("backup storages with the same url, bucket and url are not allowed")
+	errEditBackupStorageInUse        = errors.New("can't edit bucket or region of the backup storage in use")
 
 	//nolint:gochecknoglobals
 	operatorEngine = map[everestv1alpha1.EngineType]string{
@@ -345,6 +346,23 @@ func (e *EverestServer) validateUpdateBackupStorageRequest(
 		return nil, err
 	}
 
+	c := ctx.Request().Context()
+	used, err := e.kubeClient.IsBackupStorageUsed(c, e.kubeClient.Namespace(), backupStorageName, nil)
+	if err != nil {
+		return nil, err
+	}
+	if used && basicStorageParamsAreChanged(bs, params) {
+		return nil, errEditBackupStorageInUse
+	}
+
+	existingStorages, err := e.kubeClient.ListBackupStorages(c, e.kubeClient.Namespace())
+	if err != nil {
+		return nil, err
+	}
+	if duplicate := validateDuplicateStorageByUpdate(backupStorageName, bs, existingStorages, params); duplicate {
+		return nil, errDuplicatedBackupStorage
+	}
+
 	url := &bs.Spec.EndpointURL
 	if params.Url != nil {
 		if ok := validateURL(*params.Url); !ok {
@@ -406,14 +424,67 @@ func (e *EverestServer) validateUpdateBackupStorageRequest(
 		region = *params.Region
 	}
 
-	err := validateBackupStorageAccess(ctx, string(bs.Spec.Type), url, bucketName, region, accessKey, secretKey, pointer.Get(params.VerifyTLS), pointer.Get(params.ForcePathStyle), l)
+	err = validateBackupStorageAccess(ctx, string(bs.Spec.Type), url, bucketName, region, accessKey, secretKey, pointer.Get(params.VerifyTLS), pointer.Get(params.ForcePathStyle), l)
 	if err != nil {
 		return nil, err
 	}
 
 	return &params, nil
 }
 
+func (params *UpdateBackupStorageParams) regionOrDefault(defaultRegion string) string {
+	if params.Region != nil {
+		return *params.Region
+	}
+	return defaultRegion
+}
+
+func (params *UpdateBackupStorageParams) bucketNameOrDefault(defaultBucketName string) string {
+	if params.BucketName != nil {
+		return *params.BucketName
+	}
+	return defaultBucketName
+}
+
+func (params *UpdateBackupStorageParams) urlOrDefault(defaultURL string) string {
+	if params.Url != nil {
+		return *params.Url
+	}
+	return defaultURL
+}
+
+func validateDuplicateStorageByUpdate(
+	currentStorageName string,
+	currentStorage *everestv1alpha1.BackupStorage,
+	existingStorages *everestv1alpha1.BackupStorageList,
+	params UpdateBackupStorageParams,
+) bool {
+	// Construct the combined key for comparison
+	toCompare := params.regionOrDefault(currentStorage.Spec.Region) +
+		params.bucketNameOrDefault(currentStorage.Spec.Bucket) +
+		params.urlOrDefault(currentStorage.Spec.EndpointURL)
+
+	for _, s := range existingStorages.Items {
+		if s.Name == currentStorageName {
+			continue
+		}
+		if s.Spec.Region+s.Spec.Bucket+s.Spec.EndpointURL == toCompare {
+			return true
+		}
+	}
+	return false
+}
+
+func basicStorageParamsAreChanged(bs *everestv1alpha1.BackupStorage, params UpdateBackupStorageParams) bool {
+	if params.BucketName != nil && bs.Spec.Bucket != pointer.GetString(params.BucketName) {
+		return true
+	}
+	if params.Region != nil && bs.Spec.Region != pointer.GetString(params.Region) {
+		return true
+	}
+	return false
+}
+
 func validateCreateBackupStorageRequest(
 	ctx echo.Context,
 	namespaces []string,

api/validation_test.go

@@ -1121,3 +1121,77 @@ func TestCheckSchedulesChanges(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateDuplicateStorageByUpdate(t *testing.T) {
+	t.Parallel()
+	cases := []struct {
+		name               string
+		storages           []byte
+		currentStorage     []byte
+		currentStorageName string
+		params             UpdateBackupStorageParams
+		isDuplicate        bool
+	}{
+		{
+			name:               "another storage with the same 3 params",
+			currentStorage:     []byte(`{"spec":  {"name": "storageA", "bucket": "bucket2", "region": "region2", "endpointURL":"url2" }}`),
+			storages:           []byte(`{"items": [{"spec":  {"name": "storageB", "bucket": "bucket1", "region": "region1", "endpointURL":"url1" }}]}`),
+			currentStorageName: "storageA",
+			params:             UpdateBackupStorageParams{Url: pointer.ToString("url1"), BucketName: pointer.ToString("bucket1"), Region: pointer.ToString("region1")},
+			isDuplicate:        true,
+		},
+		{
+			name:               "change of url will lead to duplication",
+			currentStorage:     []byte(`{"spec":  {"name": "storageA", "bucket": "bucket2", "region": "region2", "endpointURL":"url2" }}`),
+			storages:           []byte(`{"items": [{"spec":  {"name": "storageB", "bucket": "bucket2", "region": "region2", "endpointURL":"url1" }}]}`),
+			currentStorageName: "storageA",
+			params:             UpdateBackupStorageParams{Url: pointer.ToString("url1")},
+			isDuplicate:        true,
+		},
+		{
+			name:               "change of bucket will lead to duplication",
+			currentStorage:     []byte(`{"spec":  {"name": "storageA", "bucket": "bucket2", "region": "region2", "endpointURL":"url2" }}`),
+			storages:           []byte(`{"items": [{"spec":  {"name": "storageB", "bucket": "bucket1", "region": "region2", "endpointURL":"url2" }}]}`),
+			currentStorageName: "storageA",
+			params:             UpdateBackupStorageParams{BucketName: pointer.ToString("bucket1")},
+			isDuplicate:        true,
+		},
+		{
+			name:               "change of region will lead to duplication",
+			currentStorage:     []byte(`{"spec":  {"name": "storageA", "bucket": "bucket2", "region": "region2", "endpointURL":"url2" }}`),
+			storages:           []byte(`{"items": [{"spec":  {"name": "storageB", "bucket": "bucket2", "region": "region1", "endpointURL":"url2" }}]}`),
+			currentStorageName: "storageA",
+			params:             UpdateBackupStorageParams{Region: pointer.ToString("region1")},
+			isDuplicate:        true,
+		},
+		{
+			name:               "change of region and bucket will lead to duplication",
+			currentStorage:     []byte(`{"spec":  {"name": "storageA", "bucket": "bucket2", "region": "region2", "endpointURL":"url2" }}`),
+			storages:           []byte(`{"items": [{"spec":  {"name": "storageB", "bucket": "bucket1", "region": "region1", "endpointURL":"url2" }}]}`),
+			currentStorageName: "storageA",
+			params:             UpdateBackupStorageParams{Region: pointer.ToString("region1"), BucketName: pointer.ToString("bucket1")},
+			isDuplicate:        true,
+		},
+		{
+			name:               "no other storages: no duplictation",
+			currentStorage:     []byte(`{"spec":  {"name": "storageA", "bucket": "bucket2", "region": "region2", "endpointURL":"url2" }}`),
+			storages:           []byte(`{"items": [{"spec":  {"name": "storageA", "bucket": "bucket2", "region": "region2", "endpointURL":"url2" }}]}`),
+			currentStorageName: "storageA",
+			params:             UpdateBackupStorageParams{Url: pointer.ToString("url1"), BucketName: pointer.ToString("bucket1"), Region: pointer.ToString("region1")},
+			isDuplicate:        false,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			storages := &everestv1alpha1.BackupStorageList{}
+			err := json.Unmarshal(tc.storages, storages)
+			require.NoError(t, err)
+			currentStorage := &everestv1alpha1.BackupStorage{}
+			err = json.Unmarshal(tc.currentStorage, currentStorage)
+			require.NoError(t, err)
+			isDuplicate := validateDuplicateStorageByUpdate(tc.currentStorageName, currentStorage, storages, tc.params)
+			assert.Equal(t, tc.isDuplicate, isDuplicate)
+		})
+	}
+}

pkg/kubernetes/backup_storage.go

@@ -80,11 +80,11 @@ func (k *Kubernetes) IsBackupStorageUsed(ctx context.Context, namespace, backupS
 	}
 
 	for _, namespace := range namespaces {
-		list, err := k.client.ListDatabaseClusters(ctx, namespace, options)
+		dblist, err := k.client.ListDatabaseClusters(ctx, namespace, options)
 		if err != nil {
 			return false, err
 		}
-		if len(list.Items) > 0 {
+		if len(dblist.Items) > 0 {
 			return true, nil
 		}
 		bList, err := k.client.ListDatabaseClusterBackups(ctx, namespace, options)
@@ -98,8 +98,12 @@ func (k *Kubernetes) IsBackupStorageUsed(ctx context.Context, namespace, backupS
 		if err != nil {
 			return false, err
 		}
-		if len(rList.Items) > 0 {
-			return true, nil
+		for _, restore := range rList.Items {
+			for _, db := range dblist.Items {
+				if restore.Spec.DBClusterName == db.Name && !restore.IsComplete(db.Spec.Engine.Type) {
+					return true, nil
+				}
+			}
 		}
 	}