Update CRD and logic to enable Non Admin BSL enforcements (#236)

Signed-off-by: Michal Pryc <[email protected]>

Author: mpryc

api/v1alpha1/nonadminbackupstoragelocationrequest_types.go

@@ -46,12 +46,13 @@ const (
 // NonAdminBackupStorageLocationRequestSpec defines the desired state of NonAdminBackupStorageLocationRequest
 type NonAdminBackupStorageLocationRequestSpec struct {
 	// approvalDecision is the decision of the cluster admin on the Requested NonAdminBackupStorageLocation creation.
+	// The value may be set to either approve or reject.
 	// +optional
 	ApprovalDecision NonAdminBSLRequest `json:"approvalDecision,omitempty"`
 }
 
-// NonAdminBackupStorageLocationRequestStatusInfo contains information of the related NonAdminBackupStorageLocation object.
-type NonAdminBackupStorageLocationRequestStatusInfo struct {
+// SourceNonAdminBSL contains information of the NonAdminBackupStorageLocation object that triggered NonAdminBSLRequest
+type SourceNonAdminBSL struct {
 	// requestedSpec contains the requested Velero BackupStorageLocation spec from the NonAdminBackupStorageLocation
 	// +optionl
 	RequestedSpec *velerov1.BackupStorageLocationSpec `json:"requestedSpec"`
@@ -71,10 +72,11 @@ type NonAdminBackupStorageLocationRequestStatusInfo struct {
 
 // NonAdminBackupStorageLocationRequestStatus defines the observed state of NonAdminBackupStorageLocationRequest
 type NonAdminBackupStorageLocationRequestStatus struct {
+	// nonAdminBackupStorageLocation contains information of the NonAdminBackupStorageLocation object that triggered NonAdminBSLRequest
 	// +optional
-	NonAdminBackupStorageLocationRequestStatusInfo *NonAdminBackupStorageLocationRequestStatusInfo `json:"nonAdminBackupStorageLocation,omitempty"`
-	// phase is a simple one high-level summary of the lifecycle of an NonAdminBackupStorageLocation.
+	SourceNonAdminBSL *SourceNonAdminBSL `json:"nonAdminBackupStorageLocation,omitempty"`
 
+	// phase represents the current state of the NonAdminBSLRequest. It can be either Pending, Approved or Rejected.
 	// +optional
 	Phase NonAdminBSLRequestPhase `json:"phase,omitempty"`
 }

api/v1alpha1/zz_generated.deepcopy.go

@@ -200,6 +200,7 @@ func main() {
 		RequireApprovalForBSL: *dpaConfiguration.RequireApprovalForBSL,
 		SyncPeriod:            dpaConfiguration.BackupSyncPeriod.Duration,
 		DefaultSyncPeriod:     defaultSyncPeriod,
+		EnforcedBslSpec:       dpaConfiguration.EnforceBSLSpec,
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to setup NonAdminBackupStorageLocation controller with manager")
 		os.Exit(1)
@@ -255,6 +256,7 @@ func getDPAConfiguration(restConfig *rest.Config, oadpNamespace string) (v1alpha
 		},
 		EnforceBackupSpec:     &velerov1.BackupSpec{},
 		EnforceRestoreSpec:    &velerov1.RestoreSpec{},
+		EnforceBSLSpec:        &v1alpha1.EnforceBackupStorageLocationSpec{},
 		RequireApprovalForBSL: ptr.To(false),
 	}
 	var defaultSyncPeriod *time.Duration
@@ -281,6 +283,9 @@ func getDPAConfiguration(restConfig *rest.Config, oadpNamespace string) (v1alpha
 			if nonAdmin.EnforceRestoreSpec != nil {
 				dpaConfiguration.EnforceRestoreSpec = nonAdmin.EnforceRestoreSpec
 			}
+			if nonAdmin.EnforceBSLSpec != nil {
+				dpaConfiguration.EnforceBSLSpec = nonAdmin.EnforceBSLSpec
+			}
 			if nonAdmin.GarbageCollectionPeriod != nil {
 				dpaConfiguration.GarbageCollectionPeriod.Duration = nonAdmin.GarbageCollectionPeriod.Duration
 			}

cmd/main.go

@@ -44,8 +44,9 @@ spec:
               state of NonAdminBackupStorageLocationRequest
             properties:
               approvalDecision:
-                description: approvalDecision is the decision of the cluster admin
-                  on the Requested NonAdminBackupStorageLocation creation.
+                description: |-
+                  approvalDecision is the decision of the cluster admin on the Requested NonAdminBackupStorageLocation creation.
+                  The value may be set to either approve or reject.
                 enum:
                 - approve
                 - reject
@@ -57,8 +58,8 @@ spec:
               state of NonAdminBackupStorageLocationRequest
             properties:
               nonAdminBackupStorageLocation:
-                description: NonAdminBackupStorageLocationRequestStatusInfo contains
-                  information of the related NonAdminBackupStorageLocation object.
+                description: nonAdminBackupStorageLocation contains information of
+                  the NonAdminBackupStorageLocation object that triggered NonAdminBSLRequest
                 properties:
                   nacuuid:
                     description: nacuuid references the NonAdminBackupStorageLocation
@@ -162,7 +163,8 @@ spec:
                 - requestedSpec
                 type: object
               phase:
-                description: NonAdminBSLRequestPhase is the phase of the NonAdminBackupStorageLocationRequest
+                description: phase represents the current state of the NonAdminBSLRequest.
+                  It can be either Pending, Approved or Rejected.
                 enum:
                 - Pending
                 - Approved

config/crd/bases/oadp.openshift.io_nonadminbackupstoragelocationrequests.yaml

@@ -2,37 +2,47 @@
 
 ## Abstract
 
-Non Admin Controller (NAC) restricts the usage of OADP operator with NonAdminBackups and NonAdminRestores.
-Admin users may want to further restrict this by restricting NonAdminBackup/NonAdminRestore spec fields values.
+Non Admin Controller (NAC) restricts the usage of OADP operator with NonAdminBackups, NonAdminRestores and NonAdminBackupStorageLocations.
+Admin users may want to further restrict this by restricting NonAdminBackup/NonAdminRestore/NonAdminBackupStorageLocation spec fields values.
 
 ## Background
 
-Non Admin Controller (NAC) adds the ability to admin users restrict the use of OADP operator for non admin users, by only allowing them to create backup/restores from their namespaces with NonAdminBackups/NonAdminRestores.
+Non Admin Controller (NAC) adds the ability to admin users restrict the use of OADP operator for non admin users, by only allowing them to
+create backup/restore/backupstoragelocation objects from their namespaces with NonAdminBackups/NonAdminRestores/NonAdminBackupStorageLocations.
 Admin users may want to further restrict non admin users operations, like forcing a specific time to live (TTL) for NonAdminBackups associated Velero Backups.
-This design enables admin users to set custom default values for NonAdminBackup/NonAdminRestore spec fields, which can not be overridden by non-admin users.
+This design enables admin users to set custom default values for NonAdminBackup/NonAdminRestore/NonAdminBackupStorageLocation spec fields,
+which can not be overridden by non-admin users.
 
 ## Goals
 
 Enable admin users to
 - set custom default values for NonAdminBackup spec.backupSpec fields, which can not be overridden
 - set custom default values for NonAdminRestore spec.restoreSpec fields, which can not be overridden
+- set custom default values for NonAdminBackupStorageLocation spec.backupStorageLocationSpec fields, which can not be overridden
 
 Also
 - Show custom default values validation errors in NAC object statuses and in NAC logs
 
 ## Non Goals
 
-- Show NonAdminBackup spec.backupSpec fields/NonAdminRestore spec.restoreSpec fields custom default values to non admin users
-- Prevent non admin users to create NonAdminBackup/NonAdminRestore with overridden defaults
+- Show the custom default values to non admin users in NonAdminBackup/NonAdminRestore/NonAdminBackupStorageLocation spec fields
+- Prevent non admin users to create NonAdminBackup/NonAdminRestore/NonAdminBackupStorageLocation with overridden defaults
 - Allow admin users to set second level defaults (for example, NonAdminBackup `spec.backupSpec.labelSelector` can have a custom default value, but not just `spec.backupSpec.labelSelector.matchLabels`)
 - Check if there are on-going NAC operations prior to recreating NAC Pod
-- Allow admin users to enforce falsy values (like empty maps or empty lists) for NonAdminBackup spec.backupSpec fields/NonAdminRestore spec.restoreSpec fields
+- Allow admin users to enforce falsy values (like empty maps or empty lists) for NonAdminBackup spec.backupSpec fields/NonAdminRestore spec.restoreSpec fields/NonAdminBackupStorageLocation spec.backupStorageLocationSpec fields
 
 ## High-Level Design
 
-A field will be added to OADP DPA object. With it, admin users will be able to select which NonAdminBackup `spec.backupSpec` fields have custom default (and enforced) values. NAC will respect the set values. If a NonAdminBackup is created with fields overriding any enforced values, it will fail validation prior to creating an associated Velero Backup.
+New fields will be added to the OADP DPA object, allowing admin users to define custom default and enforced values for specific fields in NonAdminBackup, NonAdminRestore, and NonAdminBackupStorageLocation specifications. The NAC will enforce these values accordingly.
 
-Another field will be added to OADP DPA object. With it, admin users will be able to select which NonAdminRestore `spec.restoreSpec` fields have custom default (and enforced) values. NAC will respect the set values. If a NonAdminRestore is created with fields overriding any enforced values, it will fail validation prior to creating an associated Velero Restore.
+- **NonAdminBackup:**
+  Admin users can specify which `spec.backupSpec` fields have custom default and enforced values. If a NonAdminBackup is created with values that override enforced settings, it will fail validation before creating an associated Velero Backup.
+
+- **NonAdminRestore:**
+  Admin users can define enforced and default values for `spec.restoreSpec` fields. Any NonAdminRestore that attempts to override enforced values will fail validation before creating an associated Velero Restore.
+
+- **NonAdminBackupStorageLocation:**
+  Admin users can set enforced and default values for `spec.backupStorageLocationSpec` fields, except for spec.backupStorageLocationSpec.default, which is not included in the enforcement BSL Spec. If a NonAdminBackupStorageLocation attempts to override enforced values, it will fail validation before creating an associated Velero BackupStorageLocation.
 
 If admin user changes any enforced field value, NAC Pod is recreated to always be up to date with admin user enforcements.
 
@@ -42,7 +52,16 @@ If admin user changes any enforced field value, NAC Pod is recreated to always b
 
 Field `spec.nonAdmin.enforceBackupSpec`, of the same type as the Velero Backup Spec, will be added to OADP DPA object.
 
-With it, admin users will be able to select which NonAdminBackup `spec.backupSpec` fields have custom default (and enforced) values.
+Field `spec.nonAdmin.enforceRestoreSpec`, of the same type as the Velero Restore Spec, will be added to OADP DPA object.
+
+Field `spec.nonAdmin.enforceBSLSpec`, which mirrors the Velero BackupStorageLocation Spec, will be introduced in the
+OADP DPA object with the following exceptions:
+
+ - Fields marked as `required` in the Velero BSL Spec are treated as `optional` in the enforcement BSL Spec.
+   This allows admin users to enforce specific fields without requiring others.
+ - The `default` field is excluded from the enforcement BSL Spec, because it can not be enforced.
+
+With the above fields, admin users will be able to select for example which NonAdminBackup `spec.backupSpec` fields have custom default (and enforced) values.
 
 To avoid mistakes, not all fields will be able to be enforced, like `IncludedNamespaces`, that could break NAC usage.
 
@@ -65,8 +84,6 @@ spec:
     enable: true
     enforceBackupSpecs:
       snapshotVolumes: false
-  unsupportedOverrides:
-    tech-preview-ack: 'true'
 ```
 
 That means, that the 2 following NonAdminBackup will be accepted by NAC validation
@@ -123,48 +140,17 @@ Add `EnforceRestoreSpec` struct to OADP DPA `NonAdmin` struct
 type NonAdmin struct {
 	// which restore spec field values to enforce
 	// +optional
-	EnforceBackupSpec *velero.BackupSpec `json:"enforceBackupSpec,omitempty"`
+	EnforceRestoreSpec *velero.RestoreSpec `json:"enforceRestoreSpec,omitempty"`
 }
 ```
 
-Store previous `EnforceBackupSpec` and `EnforceRestoreSpec` value, so when admin user changes it, Deployment is also changed to trigger a Pod recreation
+Add `EnforceBSLSpec` struct to OADP DPA `NonAdmin` struct
 ```go
-const (
-	enforcedBackupSpecKey = "enforced-backup-spec"
-    enforcedRestoreSpecKey = "enforced-restore-spec"
-)
-
-var (
-	previousEnforcedBackupSpec    *velero.BackupSpec  = nil
-	dpaBackupSpecResourceVersion                      = ""
-	previousEnforcedRestoreSpec   *velero.RestoreSpec = nil
-	dpaRestoreSpecResourceVersion                     = ""
-)
-
-func ensureRequiredSpecs(deploymentObject *appsv1.Deployment, dpa *oadpv1alpha1.DataProtectionApplication, image string, imagePullPolicy corev1.PullPolicy) error {
-	if len(dpaBackupSpecResourceVersion) == 0 || !reflect.DeepEqual(dpa.Spec.NonAdmin.EnforceBackupSpec, previousEnforcedBackupSpec) {
-		dpaBackupSpecResourceVersion = dpa.GetResourceVersion()
-	}
-	previousEnforcedBackupSpec = dpa.Spec.NonAdmin.EnforceBackupSpec
-	if len(dpaRestoreSpecResourceVersion) == 0 || !reflect.DeepEqual(dpa.Spec.NonAdmin.EnforceRestoreSpec, previousEnforcedRestoreSpec) {
-		dpaRestoreSpecResourceVersion = dpa.GetResourceVersion()
-	}
-	previousEnforcedRestoreSpec = dpa.Spec.NonAdmin.EnforceRestoreSpec
-	enforcedSpecAnnotation := map[string]string{
-		enforcedBackupSpecKey: dpaBackupSpecResourceVersion,
-        enforcedRestoreSpecKey: dpaRestoreSpecResourceVersion,
-	}
-
-	templateObjectAnnotations := deploymentObject.Spec.Template.GetAnnotations()
-	if templateObjectAnnotations == nil {
-		deploymentObject.Spec.Template.SetAnnotations(enforcedSpecAnnotation)
-	} else {
-		templateObjectAnnotations[enforcedBackupSpecKey] = enforcedSpecAnnotation[enforcedBackupSpecKey]
-		templateObjectAnnotations[enforcedRestoreSpecKey] = enforcedSpecAnnotation[enforcedRestoreSpecKey]
-		deploymentObject.Spec.Template.SetAnnotations(templateObjectAnnotations)
-	}
+type NonAdmin struct {
+	// which backupstoragelocation spec field values to enforce
+	// +optional
+	EnforceBSLSpec *velero.BackupStorageLocationSpec `json:"enforceBSLSpec,omitempty"`
 }
-```
 
 During NAC startup, read OADP DPA, to be able to apply admin user enforcement
 ```go
@@ -220,7 +206,7 @@ Before creating NonAdminBackup's related Velero Backup, apply any missing fields
 		}
 ```
 
-Modify ValidateRestoreSpec function to use `EnforceRestoreSpec` and apply that to non admin users' NonAdminBackup request
+Modify ValidateRestoreSpec function to use `EnforceRestoreSpec` and apply that to non admin users' NonAdminRestore request
 ```go
 	enforcedSpec := reflect.ValueOf(enforcedRestoreSpec).Elem()
 	for index := range enforcedSpec.NumField() {
@@ -251,6 +237,37 @@ Before creating NonAdminRestore's related Velero Restore, apply any missing fiel
 		}
 ```
 
+Modify ValidateBSLSpec function to use `EnforceBSLSpec` and apply that to non admin users' NonAdminBackupStorageLocation request
+```go
+	enforcedSpec := reflect.ValueOf(enforcedBSLSpec).Elem()
+	for index := range enforcedSpec.NumField() {
+		enforcedField := enforcedSpec.Field(index)
+		enforcedFieldName := enforcedSpec.Type().Field(index).Name
+		currentField := reflect.ValueOf(nonAdminBsl.Spec.BackupStorageLocationSpec).Elem().FieldByName(enforcedFieldName)
+		if !enforcedField.IsZero() && !currentField.IsZero() && !reflect.DeepEqual(enforcedField.Interface(), currentField.Interface()) {
+			field, _ := reflect.TypeOf(nonAdminBsl.Spec.BackupStorageLocationSpec).Elem().FieldByName(enforcedFieldName)
+			tagName, _, _ := strings.Cut(field.Tag.Get("json"), ",")
+			return fmt.Errorf(
+				"NonAdminBackupStorageLocation spec.backupStorageLocationSpec.%v field value is enforced by admin user, can not override it",
+				tagName,
+			)
+		}
+	}
+```
+
+Before creating NonAdminBackupStorageLocation's related Velero BackupStorageLocation, apply any missing fields to it that admin user has enforced
+```go
+		enforcedSpec := reflect.ValueOf(r.EnforcedBSLSpec).Elem()
+		for index := range enforcedSpec.NumField() {
+			enforcedField := enforcedSpec.Field(index)
+			enforcedFieldName := enforcedSpec.Type().Field(index).Name
+			currentField := reflect.ValueOf(bslSpec).Elem().FieldByName(enforcedFieldName)
+			if !enforcedField.IsZero() && currentField.IsZero() {
+				currentField.Set(enforcedField)
+			}
+		}
+```
+
 For more details, check https://github.com/openshift/oadp-operator/pull/1584, https://github.com/migtools/oadp-non-admin/pull/110, https://github.com/openshift/oadp-operator/pull/1600 and https://github.com/migtools/oadp-non-admin/pull/122.
 
 ## Open Issues

docs/design/admin_control_over_spec.md

@@ -9,7 +9,7 @@ require (
 	github.com/google/uuid v1.6.0
 	github.com/onsi/ginkgo/v2 v2.19.0
 	github.com/onsi/gomega v1.33.1
-	github.com/openshift/oadp-operator v1.0.2-0.20250225171529-2e93d8609b8f
+	github.com/openshift/oadp-operator v1.0.2-0.20250227152435-9267da21ab64
 	github.com/sirupsen/logrus v1.9.3
 	github.com/stretchr/testify v1.9.0
 	github.com/vmware-tanzu/velero v1.14.0