Skip to content

Commit c17d6a0

Browse files
adam-jian-zhangblackpiglet
authored andcommitted
Fix DataUpload list scope in CSI PVC backup plugin
The `getDataUpload` function in the CSI PVC backup plugin was previously making a cluster-scoped list query to retrieve DataUpload CRs. In environments with strict minimum-privilege RBAC, this would fail with forbidden errors. This explicitly passes the backup namespace into the `ListOptions` when calling `crClient.List`, correctly scoping the queries to the backup's namespace. Unit tests have also been updated to ensure cross-namespace queries are rejected appropriately. Signed-off-by: Adam Zhang <adam.zhang@broadcom.com>
1 parent a6488a9 commit c17d6a0

3 files changed

Lines changed: 69 additions & 13 deletions

File tree

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix issue #9703, fix CSI PVC Backup Plugin list options to only list in installed namespace

pkg/backup/actions/csi/pvc_action.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -467,7 +467,7 @@ func (p *pvcBackupItemAction) Progress(
467467
return progress, biav2.InvalidOperationIDError(operationID)
468468
}
469469

470-
dataUpload, err := getDataUpload(context.Background(), p.crClient, operationID)
470+
dataUpload, err := getDataUpload(context.Background(), p.crClient, backup.Namespace, operationID)
471471
if err != nil {
472472
p.log.Errorf(
473473
"fail to get DataUpload for backup %s/%s by operation ID %s: %s",
@@ -512,7 +512,7 @@ func (p *pvcBackupItemAction) Cancel(operationID string, backup *velerov1api.Bac
512512
return biav2.InvalidOperationIDError(operationID)
513513
}
514514

515-
dataUpload, err := getDataUpload(context.Background(), p.crClient, operationID)
515+
dataUpload, err := getDataUpload(context.Background(), p.crClient, backup.Namespace, operationID)
516516
if err != nil {
517517
p.log.Errorf(
518518
"fail to get DataUpload for backup %s/%s: %s",
@@ -605,10 +605,12 @@ func createDataUpload(
605605
func getDataUpload(
606606
ctx context.Context,
607607
crClient crclient.Client,
608+
namespace string,
608609
operationID string,
609610
) (*velerov2alpha1.DataUpload, error) {
610611
dataUploadList := new(velerov2alpha1.DataUploadList)
611612
err := crClient.List(ctx, dataUploadList, &crclient.ListOptions{
613+
Namespace: namespace,
612614
LabelSelector: labels.SelectorFromSet(
613615
map[string]string{velerov1api.AsyncOperationIDLabel: operationID},
614616
),

pkg/backup/actions/csi/pvc_action_test.go

Lines changed: 64 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,28 @@ func TestProgress(t *testing.T) {
307307
operationID: "testing",
308308
expectedErr: "not found DataUpload for operationID testing",
309309
},
310+
{
311+
name: "DataUpload in different namespace is not found",
312+
backup: builder.ForBackup("velero", "test").Result(),
313+
dataUpload: &velerov2alpha1.DataUpload{
314+
TypeMeta: metav1.TypeMeta{
315+
Kind: "DataUpload",
316+
APIVersion: "v2alpha1",
317+
},
318+
ObjectMeta: metav1.ObjectMeta{
319+
Namespace: "other-namespace",
320+
Name: "testing",
321+
Labels: map[string]string{
322+
velerov1api.AsyncOperationIDLabel: "testing",
323+
},
324+
},
325+
Status: velerov2alpha1.DataUploadStatus{
326+
Phase: velerov2alpha1.DataUploadPhaseFailed,
327+
},
328+
},
329+
operationID: "testing",
330+
expectedErr: "not found DataUpload for operationID testing",
331+
},
310332
{
311333
name: "DataUpload is found",
312334
backup: builder.ForBackup("velero", "test").Result(),
@@ -375,15 +397,15 @@ func TestCancel(t *testing.T) {
375397
tests := []struct {
376398
name string
377399
backup *velerov1api.Backup
378-
dataUpload velerov2alpha1.DataUpload
400+
dataUpload *velerov2alpha1.DataUpload
379401
operationID string
380-
expectedErr error
402+
expectedErr string
381403
expectedDataUpload velerov2alpha1.DataUpload
382404
}{
383405
{
384406
name: "Cancel DataUpload",
385407
backup: builder.ForBackup("velero", "test").Result(),
386-
dataUpload: velerov2alpha1.DataUpload{
408+
dataUpload: &velerov2alpha1.DataUpload{
387409
TypeMeta: metav1.TypeMeta{
388410
Kind: "DataUpload",
389411
APIVersion: velerov2alpha1.SchemeGroupVersion.String(),
@@ -414,6 +436,31 @@ func TestCancel(t *testing.T) {
414436
},
415437
},
416438
},
439+
{
440+
name: "DataUpload cannot be found",
441+
backup: builder.ForBackup("velero", "test").Result(),
442+
operationID: "testing",
443+
expectedErr: "not found DataUpload for operationID testing",
444+
},
445+
{
446+
name: "DataUpload in different namespace is not found",
447+
backup: builder.ForBackup("velero", "test").Result(),
448+
dataUpload: &velerov2alpha1.DataUpload{
449+
TypeMeta: metav1.TypeMeta{
450+
Kind: "DataUpload",
451+
APIVersion: velerov2alpha1.SchemeGroupVersion.String(),
452+
},
453+
ObjectMeta: metav1.ObjectMeta{
454+
Namespace: "other-namespace",
455+
Name: "testing",
456+
Labels: map[string]string{
457+
velerov1api.AsyncOperationIDLabel: "testing",
458+
},
459+
},
460+
},
461+
operationID: "testing",
462+
expectedErr: "not found DataUpload for operationID testing",
463+
},
417464
}
418465

419466
for _, tc := range tests {
@@ -426,17 +473,23 @@ func TestCancel(t *testing.T) {
426473
crClient: crClient,
427474
}
428475

429-
err := crClient.Create(t.Context(), &tc.dataUpload)
430-
require.NoError(t, err)
476+
if tc.dataUpload != nil {
477+
err := crClient.Create(t.Context(), tc.dataUpload)
478+
require.NoError(t, err)
479+
}
431480

432-
err = pvcBIA.Cancel(tc.operationID, tc.backup)
433-
require.NoError(t, err)
481+
err := pvcBIA.Cancel(tc.operationID, tc.backup)
482+
if tc.expectedErr != "" {
483+
require.EqualError(t, err, tc.expectedErr)
484+
} else {
485+
require.NoError(t, err)
434486

435-
du := new(velerov2alpha1.DataUpload)
436-
err = crClient.Get(t.Context(), crclient.ObjectKey{Namespace: tc.dataUpload.Namespace, Name: tc.dataUpload.Name}, du)
437-
require.NoError(t, err)
487+
du := new(velerov2alpha1.DataUpload)
488+
err = crClient.Get(t.Context(), crclient.ObjectKey{Namespace: tc.dataUpload.Namespace, Name: tc.dataUpload.Name}, du)
489+
require.NoError(t, err)
438490

439-
require.True(t, cmp.Equal(tc.expectedDataUpload, *du, cmpopts.IgnoreFields(velerov2alpha1.DataUpload{}, "ResourceVersion")))
491+
require.True(t, cmp.Equal(tc.expectedDataUpload, *du, cmpopts.IgnoreFields(velerov2alpha1.DataUpload{}, "ResourceVersion")))
492+
}
440493
})
441494
}
442495
}

0 commit comments

Comments
 (0)