Validate namespaces, so non admin can not create backups outside of ns

Validation ensures the namespace in the VeleroBackup object are the same as the namespace for which NAB resides. Signed-off-by: Michal Pryc <[email protected]>
migtools · Apr 30, 2024 · 152d1c7 · 152d1c7
1 parent 0d18421
commit 152d1c7
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 11 deletions.
diff --git a/internal/common/function/function.go b/internal/common/function/function.go
@@ -77,21 +77,39 @@ func AddNonAdminBackupAnnotations(ownerNamespace string, ownerName string, owner
 	return mergedAnnotations
 }
 
+// containsOnlyNamespace checks if the given namespaces slice contains only the specified namespace
+func containsOnlyNamespace(namespaces []string, namespace string) bool {
+	for _, ns := range namespaces {
+		if ns != namespace {
+			return false
+		}
+	}
+	return true
+}
+
 // GetBackupSpecFromNonAdminBackup return BackupSpec object from NonAdminBackup spec, if no error occurs
 func GetBackupSpecFromNonAdminBackup(nonAdminBackup *nacv1alpha1.NonAdminBackup) (*velerov1api.BackupSpec, error) {
 	if nonAdminBackup == nil {
 		return nil, fmt.Errorf("nonAdminBackup is nil")
 	}
 
-	// TODO check spec?
-
 	if nonAdminBackup.Spec.BackupSpec == nil {
-		return nil, fmt.Errorf("BackupSpec is nil")
+		return nil, fmt.Errorf("BackupSpec is not defined")
 	}
 
+	veleroBackupSpec := nonAdminBackup.Spec.BackupSpec.DeepCopy()
+
 	// TODO: Additional validations, before continuing
 
-	return nonAdminBackup.Spec.BackupSpec.DeepCopy(), nil
+	if veleroBackupSpec.IncludedNamespaces == nil {
+		veleroBackupSpec.IncludedNamespaces = []string{nonAdminBackup.Namespace}
+	} else {
+		if !containsOnlyNamespace(veleroBackupSpec.IncludedNamespaces, nonAdminBackup.Namespace) {
+			return nil, fmt.Errorf("spec.backupSpec.IncludedNamespaces can not contain namespaces other then: %s", nonAdminBackup.Namespace)
+		}
+	}
+
+	return veleroBackupSpec, nil
 }
 
 // GenerateVeleroBackupName generates a Velero backup name based on the provided namespace and NonAdminBackup name.
@@ -185,7 +203,7 @@ func UpdateNonAdminBackupCondition(ctx context.Context, r client.Client, logger
 
 	// Check if the condition is already set to the desired status
 	currentCondition := apimeta.FindStatusCondition(nab.Status.Conditions, string(condition))
-	if currentCondition != nil && currentCondition.Status == conditionStatus {
+	if currentCondition != nil && currentCondition.Status == conditionStatus && currentCondition.Reason == reason && currentCondition.Message == message {
 		// Condition is already set to the desired status, no need to update
 		logger.V(1).Info(fmt.Sprintf("NonAdminBackup Condition is already set to: %s", condition))
 		return false, nil
@@ -262,9 +280,8 @@ func CheckVeleroBackupLabels(backup *velerov1api.Backup) bool {
 	return exists && value == constant.ManagedByLabelValue
 }
 
-// TODO not used
-
 // GetNonAdminBackupFromVeleroBackup return referenced NonAdminBackup object from Velero Backup object, if no error occurs
+// TODO not used
 func GetNonAdminBackupFromVeleroBackup(ctx context.Context, clientInstance client.Client, backup *velerov1api.Backup) (*nacv1alpha1.NonAdminBackup, error) {
 	// Check if the backup has the required annotations to identify the associated NonAdminBackup object
 	logger := log.FromContext(ctx)

diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go
@@ -177,17 +177,19 @@ func TestGetBackupSpecFromNonAdminBackup(t *testing.T) {
 	backupSpec, err = GetBackupSpecFromNonAdminBackup(nonAdminBackup)
 	assert.Error(t, err)
 	assert.Nil(t, backupSpec)
-	assert.Equal(t, "BackupSpec is nil", err.Error())
+	assert.Equal(t, "BackupSpec is not defined", err.Error())
 
 	// Test case: NonAdminBackup with valid BackupSpec
 	backupSpecInput := &velerov1api.BackupSpec{
-		IncludedNamespaces:      []string{"namespace1", "namespace2"},
-		ExcludedNamespaces:      []string{"namespace3"},
+		IncludedNamespaces:      []string{"namespace1"},
 		StorageLocation:         "s3://bucket-name/path/to/backup",
 		VolumeSnapshotLocations: []string{"volume-snapshot-location"},
 	}
 
 	nonAdminBackup = &nacv1alpha1.NonAdminBackup{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "namespace1", // Set the namespace of NonAdminBackup
+		},
 		Spec: nacv1alpha1.NonAdminBackupSpec{
 			BackupSpec: backupSpecInput,
 		},
@@ -196,7 +198,10 @@ func TestGetBackupSpecFromNonAdminBackup(t *testing.T) {
 
 	assert.NoError(t, err)
 	assert.NotNil(t, backupSpec)
-	assert.Equal(t, backupSpecInput, backupSpec)
+	assert.Equal(t, []string{"namespace1"}, backupSpec.IncludedNamespaces) // Check that only the namespace from NonAdminBackup is included
+	assert.Equal(t, backupSpecInput.ExcludedNamespaces, backupSpec.ExcludedNamespaces)
+	assert.Equal(t, backupSpecInput.StorageLocation, backupSpec.StorageLocation)
+	assert.Equal(t, backupSpecInput.VolumeSnapshotLocations, backupSpec.VolumeSnapshotLocations)
 }
 
 func TestGenerateVeleroBackupName(t *testing.T) {

diff --git a/internal/controller/nab_validator.go b/internal/controller/nab_validator.go
@@ -47,7 +47,11 @@ func (r *NonAdminBackupReconciler) ValidateVeleroBackupSpec(ctx context.Context,
 	_, err := function.GetBackupSpecFromNonAdminBackup(nab)
 
 	if err != nil {
+		// Use errMsg if errMsgFromErr is not available, otherwise use errMsgFromErr
 		errMsg := "NonAdminBackup CR does not contain valid BackupSpec"
+		if errMsgFromErr := err.Error(); errMsgFromErr != "" {
+			errMsg = errMsgFromErr
+		}
 		logger.Error(err, errMsg)
 
 		updatedStatus, errUpdateStatus := function.UpdateNonAdminPhase(ctx, r.Client, logger, nab, nacv1alpha1.NonAdminBackupPhaseBackingOff)