fix(dataprotection): preserve backup namespace for cross-namespace restores

apis/dataprotection/v1alpha1/restore_types.go

@@ -105,6 +105,7 @@ type BackupRef struct {
 	Name string `json:"name"`
 
 	// Specifies the backup namespace.
+	// Cross-namespace VolumeSnapshot restores are NOT supported.
 	//
 	// +kubebuilder:validation:Required
 	Namespace string `json:"namespace"`

apis/operations/v1alpha1/opsrequest_types.go

@@ -513,8 +513,9 @@ type FromBackup struct {
 	// +kubebuilder:validation:Required
 	Name string `json:"name"`
 
-	// Specifies the namespace of the Backup namespace.
+	// Specifies the namespace of the Backup.
 	// If not specified, the namespace of the OpsRequest will be used.
+	// Cross-namespace VolumeSnapshot restores are NOT supported.
 	// +optional
 	Namespace string `json:"namespace,omitempty"`
 

config/crd/bases/dataprotection.kubeblocks.io_restores.yaml

@@ -83,7 +83,9 @@ spec:
                     description: Specifies the backup name.
                     type: string
                   namespace:
-                    description: Specifies the backup namespace.
+                    description: |-
+                      Specifies the backup namespace.
+                      Cross-namespace VolumeSnapshot restores are NOT supported.
                     type: string
                   sourceTargetName:
                     description: Specifies the source target for restoration, identified

config/crd/bases/operations.kubeblocks.io_opsrequests.yaml

@@ -619,8 +619,9 @@ spec:
                               type: string
                             namespace:
                               description: |-
-                                Specifies the namespace of the Backup namespace.
+                                Specifies the namespace of the Backup.
                                 If not specified, the namespace of the OpsRequest will be used.
+                                Cross-namespace VolumeSnapshot restores are NOT supported.
                               type: string
                             restoreEnv:
                               description: |-

controllers/dataprotection/volumepopulator_controller.go

@@ -502,13 +502,13 @@ func (r *VolumePopulatorReconciler) validateRestoreAndBuildMGR(reqCtx intctrluti
 			}
 		}
 	}
-	if err = r.restoreSystemAccountSecrets(reqCtx, pvc, backupNamespace); err != nil {
-		return nil, err
-	}
 	restoreMgr := dprestore.NewRestoreManager(restore, r.Recorder, r.Scheme, r.Client)
 	if err = dprestore.ValidateAndInitRestoreMGR(reqCtx, r.Client, restoreMgr); err != nil {
 		return nil, err
 	}
+	if err = r.restoreSystemAccountSecrets(reqCtx, pvc, backupNamespace); err != nil {
+		return nil, err
+	}
 	if decision.mode == pvcRestoreModeProvisionOnly {
 		restoreMgr.PrepareDataBackupSets = nil
 	}

controllers/dataprotection/volumepopulator_controller_test.go

@@ -750,6 +750,21 @@ var _ = Describe("Volume Populator Controller test", func() {
 					g.Expect(restoreCondition).ShouldNot(BeNil())
 					g.Expect(restoreCondition.Status).Should(Equal(corev1.ConditionUnknown))
 				})).Should(Succeed())
+
+				populateKey := types.NamespacedName{
+					Namespace: testCtx.DefaultNamespace,
+					Name:      getPopulatePVCName(pvc.UID),
+				}
+				Eventually(testapps.CheckObjExists(&testCtx, populateKey,
+					&corev1.PersistentVolumeClaim{}, true)).Should(Succeed())
+				Eventually(testapps.CheckObjExists(&testCtx, populateKey,
+					&dpv1alpha1.Restore{}, true)).Should(Succeed())
+
+				By("clean resources created by the case")
+				cleanEnv()
+				testapps.DeleteObject(&testCtx, secretKey, &corev1.Secret{})
+				Eventually(testapps.CheckObjExists(&testCtx, secretKey,
+					&corev1.Secret{}, false)).Should(Succeed())
 			})
 
 			It("test VolumePopulator when it fails", func() {
@@ -2711,6 +2726,151 @@ func TestRestoreSystemAccountSecretsUsesShardingSecretName(t *testing.T) {
 		systemAccountSecretName(systemAccountSecretScopeComponent, "cluster", "mysql", "admin"))
 }
 
+func TestValidateRestoreBeforeRestoringSystemAccountSecrets(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, corev1.AddToScheme(scheme))
+	require.NoError(t, kbappsv1.AddToScheme(scheme))
+	require.NoError(t, dpv1alpha1.AddToScheme(scheme))
+	require.NoError(t, workloadsv1.AddToScheme(scheme))
+
+	const (
+		sourceNamespace = "source"
+		targetNamespace = "target"
+		clusterName     = "cluster"
+		componentName   = "mysql"
+		accountName     = "admin"
+	)
+	actionSet := &dpv1alpha1.ActionSet{
+		ObjectMeta: metav1.ObjectMeta{Name: "snapshot-action"},
+		Spec: dpv1alpha1.ActionSetSpec{
+			BackupType: dpv1alpha1.BackupTypeFull,
+			Restore: &dpv1alpha1.RestoreActionSpec{
+				PrepareData: &dpv1alpha1.JobActionSpec{},
+			},
+		},
+	}
+	encryptor := intctrlutil.NewEncryptor(viper.GetString(constant.CfgKeyDPEncryptionKey))
+	encryptedPassword, err := encryptor.Encrypt([]byte("restored-password"))
+	require.NoError(t, err)
+	accounts, err := json.Marshal(map[string]map[string]string{
+		componentName: {accountName: encryptedPassword},
+	})
+	require.NoError(t, err)
+	backup := &dpv1alpha1.Backup{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: sourceNamespace,
+			Name:      "backup",
+			Annotations: map[string]string{
+				constant.EncryptedSystemAccountsAnnotationKey: string(accounts),
+			},
+		},
+		Status: dpv1alpha1.BackupStatus{
+			Phase: dpv1alpha1.BackupPhaseCompleted,
+			BackupMethod: &dpv1alpha1.BackupMethod{
+				Name:            "snapshot",
+				ActionSetName:   actionSet.Name,
+				SnapshotVolumes: ptr.To(true),
+				TargetVolumes:   &dpv1alpha1.TargetVolumeInfo{Volumes: []string{"data"}},
+			},
+		},
+	}
+	cluster := &kbappsv1.Cluster{
+		ObjectMeta: metav1.ObjectMeta{Namespace: targetNamespace, Name: clusterName, UID: types.UID("cluster-uid")},
+		Spec: kbappsv1.ClusterSpec{
+			Restore: &kbappsv1.ClusterRestore{
+				Source: kbappsv1.ClusterRestoreSource{
+					APIGroup:  dptypes.DataprotectionAPIGroup,
+					Kind:      dptypes.BackupKind,
+					Name:      backup.Name,
+					Namespace: backup.Namespace,
+				},
+			},
+		},
+	}
+	component := &kbappsv1.Component{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: targetNamespace,
+			Name:      constant.GenerateClusterComponentName(clusterName, componentName),
+			UID:       types.UID("component-uid"),
+		},
+	}
+	instanceSet := &workloadsv1.InstanceSet{
+		TypeMeta: metav1.TypeMeta{APIVersion: workloadsv1.GroupVersion.String(), Kind: workloadsv1.InstanceSetKind},
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: targetNamespace,
+			Name:      "cluster-mysql",
+			UID:       types.UID("instanceset-uid"),
+			Labels: map[string]string{
+				constant.AppInstanceLabelKey:    clusterName,
+				constant.KBAppComponentLabelKey: componentName,
+			},
+		},
+		Spec: workloadsv1.InstanceSetSpec{
+			Replicas: ptr.To[int32](1),
+			VolumeClaimTemplates: []corev1.PersistentVolumeClaim{{
+				ObjectMeta: metav1.ObjectMeta{Name: "data"},
+			}},
+		},
+	}
+	apiGroup := dptypes.DataprotectionAPIGroup
+	pvc := &corev1.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: targetNamespace,
+			Name:      "data-cluster-mysql-0",
+			Labels: map[string]string{
+				constant.AppInstanceLabelKey:             clusterName,
+				constant.KBAppComponentLabelKey:          componentName,
+				constant.KBAppPodNameLabelKey:            "cluster-mysql-0",
+				constant.VolumeClaimTemplateNameLabelKey: "data",
+			},
+			Annotations: map[string]string{
+				constant.RestoreSourceNamespaceAnnotationKey: sourceNamespace,
+				constant.RestoreVolumeTemplateAnnotationKey:  "data",
+			},
+			OwnerReferences: []metav1.OwnerReference{{
+				APIVersion: workloadsv1.GroupVersion.String(),
+				Kind:       workloadsv1.InstanceSetKind,
+				Name:       instanceSet.Name,
+				UID:        instanceSet.UID,
+				Controller: ptr.To(true),
+			}},
+		},
+		Spec: corev1.PersistentVolumeClaimSpec{
+			DataSourceRef: &corev1.TypedObjectReference{
+				APIGroup: &apiGroup,
+				Kind:     dptypes.BackupKind,
+				Name:     backup.Name,
+			},
+		},
+	}
+	secret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: targetNamespace,
+			Name:      constant.GenerateAccountSecretName(clusterName, componentName, accountName),
+		},
+		Data: map[string][]byte{
+			constant.AccountNameForSecret:   []byte(accountName),
+			constant.AccountPasswdForSecret: []byte("existing-password"),
+		},
+	}
+	reconciler := &VolumePopulatorReconciler{
+		Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(
+			actionSet, backup, cluster, component, instanceSet, secret,
+		).Build(),
+		Scheme: scheme,
+	}
+
+	_, err = reconciler.validateRestoreAndBuildMGR(intctrlutil.RequestCtx{Ctx: context.Background()}, pvc)
+	require.ErrorContains(t, err, "cross-namespace VolumeSnapshot")
+	require.True(t, intctrlutil.IsTargetError(err, intctrlutil.ErrorTypeFatal))
+
+	got := &corev1.Secret{}
+	require.NoError(t, reconciler.Client.Get(context.Background(), client.ObjectKeyFromObject(secret), got))
+	require.Equal(t, []byte("existing-password"), got.Data[constant.AccountPasswdForSecret])
+	require.NotContains(t, got.Annotations, constant.SystemAccountProvisionedAnnotationKey)
+	require.Empty(t, got.OwnerReferences)
+}
+
 func TestRestoreSystemAccountSecretsRestoresComponentAndShardingSecrets(t *testing.T) {
 	scheme := runtime.NewScheme()
 	require.NoError(t, corev1.AddToScheme(scheme))

deploy/helm/crds/dataprotection.kubeblocks.io_restores.yaml

@@ -83,7 +83,9 @@ spec:
                     description: Specifies the backup name.
                     type: string
                   namespace:
-                    description: Specifies the backup namespace.
+                    description: |-
+                      Specifies the backup namespace.
+                      Cross-namespace VolumeSnapshot restores are NOT supported.
                     type: string
                   sourceTargetName:
                     description: Specifies the source target for restoration, identified

deploy/helm/crds/operations.kubeblocks.io_opsrequests.yaml

@@ -619,8 +619,9 @@ spec:
                               type: string
                             namespace:
                               description: |-
-                                Specifies the namespace of the Backup namespace.
+                                Specifies the namespace of the Backup.
                                 If not specified, the namespace of the OpsRequest will be used.
+                                Cross-namespace VolumeSnapshot restores are NOT supported.
                               type: string
                             restoreEnv:
                               description: |-

docs/developer_docs/api-reference/dataprotection.md

@@ -2858,7 +2858,8 @@ string
 </em>
 </td>
 <td>
-<p>Specifies the backup namespace.</p>
+<p>Specifies the backup namespace.
+Cross-namespace VolumeSnapshot restores are NOT supported.</p>
 </td>
 </tr>
 <tr>

docs/developer_docs/api-reference/operations.md

@@ -1089,8 +1089,9 @@ string
 </td>
 <td>
 <em>(Optional)</em>
-<p>Specifies the namespace of the Backup namespace.
-If not specified, the namespace of the OpsRequest will be used.</p>
+<p>Specifies the namespace of the Backup.
+If not specified, the namespace of the OpsRequest will be used.
+Cross-namespace VolumeSnapshot restores are NOT supported.</p>
 </td>
 </tr>
 <tr>

pkg/controller/plan/restore.go

@@ -213,7 +213,7 @@ func (r *RestoreManager) buildPrepareDataRestore(comp *component.SynthesizedComp
 		Spec: dpv1alpha1.RestoreSpec{
 			Backup: dpv1alpha1.BackupRef{
 				Name:             backupObj.Name,
-				Namespace:        r.namespace,
+				Namespace:        backupObj.Namespace,
 				SourceTargetName: sourceTargetName,
 			},
 			RestoreTime: r.RestoreTime,
@@ -275,7 +275,7 @@ func (r *RestoreManager) DoPostReady(comp *component.SynthesizedComponent,
 		Spec: dpv1alpha1.RestoreSpec{
 			Backup: dpv1alpha1.BackupRef{
 				Name:             backupObj.Name,
-				Namespace:        r.namespace,
+				Namespace:        backupObj.Namespace,
 				SourceTargetName: sourceTargetName,
 			},
 			RestoreTime: r.RestoreTime,

pkg/controller/plan/restore_test.go

@@ -236,7 +236,7 @@ func TestRestoreManagerBuildPrepareDataRestore(t *testing.T) {
 		SchedulingPolicy: &appsv1.SchedulingPolicy{NodeName: "node-a"},
 	}
 	backup := &dpv1alpha1.Backup{
-		ObjectMeta: metav1.ObjectMeta{Name: "backup", Namespace: "default"},
+		ObjectMeta: metav1.ObjectMeta{Name: "backup", Namespace: "backup-source"},
 		Status: dpv1alpha1.BackupStatus{
 			Targets: []dpv1alpha1.BackupStatusTarget{{
 				BackupTarget: dpv1alpha1.BackupTarget{
@@ -264,7 +264,7 @@ func TestRestoreManagerBuildPrepareDataRestore(t *testing.T) {
 		return
 	}
 	if restore.Spec.Backup.Name != "backup" ||
-		restore.Spec.Backup.Namespace != "default" ||
+		restore.Spec.Backup.Namespace != "backup-source" ||
 		restore.Spec.Backup.SourceTargetName != "target-a" {
 		t.Fatalf("unexpected backup ref: %#v", restore.Spec.Backup)
 	}

pkg/dataprotection/restore/manager.go

@@ -99,7 +99,7 @@ func (r *RestoreManager) GetBackupActionSetByNamespaced(reqCtx intctrlutil.Reque
 	if backupMethod == nil {
 		return nil, intctrlutil.NewFatalError(fmt.Sprintf(`status.backupMethod of backup "%s" is empty`, backupName))
 	}
-	useVolumeSnapshot := backupMethod.SnapshotVolumes != nil && *backupMethod.SnapshotVolumes
+	useVolumeSnapshot := boolptr.IsSetToTrue(backupMethod.SnapshotVolumes)
 	actionSet, err := utils.GetActionSetByName(reqCtx, cli, backup.Status.BackupMethod.ActionSetName)
 	if err != nil {
 		return nil, err
@@ -256,15 +256,19 @@ func (r *RestoreManager) getBaseBackupActionSetForContinuous(reqCtx intctrlutil.
 		return notFoundLatestBackup()
 	}
 	// 3. get the action set
-	var actionSetName string
-	if latestBackup.Status.BackupMethod != nil {
-		actionSetName = latestBackup.Status.BackupMethod.ActionSetName
+	backupMethod := latestBackup.Status.BackupMethod
+	if backupMethod == nil {
+		return nil, intctrlutil.NewFatalError(fmt.Sprintf(`status.backupMethod of backup "%s" is empty`, latestBackup.Name))
 	}
-	actionSet, err := utils.GetActionSetByName(reqCtx, cli, actionSetName)
+	actionSet, err := utils.GetActionSetByName(reqCtx, cli, backupMethod.ActionSetName)
 	if err != nil {
 		return nil, err
 	}
-	return &BackupActionSet{Backup: latestBackup, ActionSet: actionSet}, nil
+	return &BackupActionSet{
+		Backup:            latestBackup,
+		ActionSet:         actionSet,
+		UseVolumeSnapshot: boolptr.IsSetToTrue(backupMethod.SnapshotVolumes),
+	}, nil
 }
 
 func (r *RestoreManager) listCompletedBackups(reqCtx intctrlutil.RequestCtx, cli client.Client, continuousBackup *dpv1alpha1.Backup, backupType dpv1alpha1.BackupType) ([]dpv1alpha1.Backup, error) {

pkg/dataprotection/restore/manager_test.go

@@ -607,6 +607,26 @@ var _ = Describe("RestoreManager Test", func() {
 				return continuousBackup, restore
 			}
 
+			It("returns a fatal error when the base Backup has no status.backupMethod", func() {
+				baseBackup := mockBackupForRestore(
+					&testCtx, actionSet.Name, testdp.BackupPVCName, true, false, dpv1alpha1.BackupTypeFull,
+					"", "2023-01-01T10:00:00Z", "",
+				)
+				Expect(testapps.ChangeObjStatus(&testCtx, baseBackup, func() {
+					baseBackup.Status.BackupMethod = nil
+				})).Should(Succeed())
+
+				continuousBackup, restore := createContinuousBackupAndRestore("2023-01-01T11:30:00Z")
+				reqCtx := getReqCtx()
+				restoreMGR := NewRestoreManager(restore, recorder, k8sClient.Scheme(), k8sClient)
+				backupSet, err := restoreMGR.GetBackupActionSetByNamespaced(reqCtx, k8sClient, continuousBackup.Name, testCtx.DefaultNamespace)
+				Expect(err).ShouldNot(HaveOccurred())
+
+				err = restoreMGR.BuildContinuousRestoreManager(reqCtx, k8sClient, *backupSet)
+				Expect(err).Should(MatchError(ContainSubstring("status.backupMethod")))
+				Expect(intctrlutil.IsTargetError(err, intctrlutil.ErrorTypeFatal)).Should(BeTrue())
+			})
+
 			It("respects UnifyFullAndContinuousRestore annotation", func() {
 				By("create a completed backup")
 				_ = mockBackupForRestore(&testCtx, actionSet.Name, testdp.BackupPVCName, true, false, dpv1alpha1.BackupTypeFull, "", "2023-01-01T10:00:00Z", "")

pkg/dataprotection/restore/utils.go

@@ -271,7 +271,17 @@ func ValidateAndInitRestoreMGR(reqCtx intctrlutil.RequestCtx,
 	default:
 		err = intctrlutil.NewFatalError(fmt.Sprintf("backup type of %s is empty", backupName))
 	}
-	return err
+	if err != nil {
+		return err
+	}
+	for i := range restoreMgr.PrepareDataBackupSets {
+		prepareDataBackupSet := &restoreMgr.PrepareDataBackupSets[i]
+		if prepareDataBackupSet.UseVolumeSnapshot && prepareDataBackupSet.Backup.Namespace != restoreMgr.Restore.Namespace {
+			return intctrlutil.NewFatalError(fmt.Sprintf("cross-namespace VolumeSnapshot restore from Backup %s/%s is not supported",
+				prepareDataBackupSet.Backup.Namespace, prepareDataBackupSet.Backup.Name))
+		}
+	}
+	return nil
 }
 
 func cutJobName(jobName string) string {