fix: Enforce NonAdminRestore spec field values (migtools#122)

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

cmd/main.go

@@ -113,7 +113,7 @@ func main() {
 
 	restConfig := ctrl.GetConfigOrDie()
 
-	enforcedBackupSpec, err := getEnforcedSpec(restConfig, oadpNamespace)
+	enforcedBackupSpec, enforcedRestoreSpec, err := getEnforcedSpec(restConfig, oadpNamespace)
 	if err != nil {
 		setupLog.Error(err, "unable to get enforced spec")
 		os.Exit(1)
@@ -157,9 +157,10 @@ func main() {
 		os.Exit(1)
 	}
 	if err = (&controller.NonAdminRestoreReconciler{
-		Client:        mgr.GetClient(),
-		Scheme:        mgr.GetScheme(),
-		OADPNamespace: oadpNamespace,
+		Client:              mgr.GetClient(),
+		Scheme:              mgr.GetScheme(),
+		OADPNamespace:       oadpNamespace,
+		EnforcedRestoreSpec: enforcedRestoreSpec,
 	}).SetupWithManager(mgr); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "NonAdminRestore")
 		os.Exit(1)
@@ -182,28 +183,32 @@ func main() {
 	}
 }
 
-func getEnforcedSpec(restConfig *rest.Config, oadpNamespace string) (*velerov1.BackupSpec, error) {
+func getEnforcedSpec(restConfig *rest.Config, oadpNamespace string) (*velerov1.BackupSpec, *velerov1.RestoreSpec, error) {
 	dpaClientScheme := runtime.NewScheme()
 	utilruntime.Must(v1alpha1.AddToScheme(dpaClientScheme))
 	dpaClient, err := client.New(restConfig, client.Options{
 		Scheme: dpaClientScheme,
 	})
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	// TODO we could pass DPA name as env var and do a get call directly. Better?
 	dpaList := &v1alpha1.DataProtectionApplicationList{}
 	err = dpaClient.List(context.Background(), dpaList)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	enforcedBackupSpec := &velerov1.BackupSpec{}
+	enforcedRestoreSpec := &velerov1.RestoreSpec{}
 	for _, dpa := range dpaList.Items {
 		if dpa.Namespace == oadpNamespace {
 			if dpa.Spec.NonAdmin != nil && dpa.Spec.NonAdmin.EnforceBackupSpec != nil {
 				enforcedBackupSpec = dpa.Spec.NonAdmin.EnforceBackupSpec
 			}
+			if dpa.Spec.NonAdmin.EnforceRestoreSpec != nil {
+				enforcedRestoreSpec = dpa.Spec.NonAdmin.EnforceRestoreSpec
+			}
 		}
 	}
-	return enforcedBackupSpec, nil
+	return enforcedBackupSpec, enforcedRestoreSpec, nil
 }

docs/design/admin_control_over_spec.md

@@ -15,23 +15,25 @@ This design enables admin users to set custom default values for NonAdminBackup/
 
 Enable admin users to
 - set custom default values for NonAdminBackup spec.backupSpec fields, which can not be overridden
-- TODO restore
+- set custom default values for NonAdminRestore spec.restoreSpec 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/TODO NonAdminRestore custom default values to non admin users
+- 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
 - 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
 
 ## 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. If admin user changes any enforced field value, NAC Pod is recreated to always be up to date with admin user enforcements.
+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.
 
-TODO restore
+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.
+
+If admin user changes any enforced field value, NAC Pod is recreated to always be up to date with admin user enforcements.
 
 > **Note:** if there are on-going NAC operations prior to recreating NAC Pod, reconcile progress might get lost for NAC objects.
 
@@ -114,46 +116,50 @@ type NonAdmin struct {
 	EnforceBackupSpec *velero.BackupSpec `json:"enforceBackupSpec,omitempty"`
 }
 ```
-TODO restore
 
-Validate admin user did not enforce a field that can break NAC functionality
+Add `EnforceRestoreSpec` struct to OADP DPA `NonAdmin` struct
 ```go
-	if r.checkNonAdminEnabled() {
-		if r.dpa.Spec.NonAdmin.EnforceBackupSpec != nil {
-			if !reflect.ValueOf(r.dpa.Spec.NonAdmin.EnforceBackupSpec.IncludedNamespaces).IsZero() {
-				return false, errors.New("admin users can not set DPA spec.nonAdmin.enforceBackupSpecs.includedNamespaces field")
-			}
-		}
-	}
+type NonAdmin struct {
+	// which restore spec field values to enforce
+	// +optional
+	EnforceBackupSpec *velero.BackupSpec `json:"enforceBackupSpec,omitempty"`
+}
 ```
 
-Store previous `EnforceBackupSpec` value, so when admin user changes it, Deployment is also changed to trigger a Pod recreation
+Store previous `EnforceBackupSpec` and `EnforceRestoreSpec` value, so when admin user changes it, Deployment is also changed to trigger a Pod recreation
 ```go
 const (
 	enforcedBackupSpecKey = "enforced-backup-spec"
+    enforcedRestoreSpecKey = "enforced-restore-spec"
 )
 
 var (
-	previousEnforcedBackupSpec   *velero.BackupSpec = nil
-	dpaBackupSpecResourceVersion                    = ""
+	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
-	// TODO same thing for restore
+	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]
-		// TODO same thing for restore
+		templateObjectAnnotations[enforcedRestoreSpecKey] = enforcedSpecAnnotation[enforcedRestoreSpecKey]
 		deploymentObject.Spec.Template.SetAnnotations(templateObjectAnnotations)
 	}
 }
@@ -180,7 +186,7 @@ During NAC startup, read OADP DPA, to be able to apply admin user enforcement
 	}
 ```
 
-Modify ValidateSpec function to use `EnforceBackupSpec` and apply that to non admin users' NonAdminBackup request
+Modify ValidateBackupSpec function to use `EnforceBackupSpec` and apply that to non admin users' NonAdminBackup request
 ```go
 func ValidateBackupSpec(nonAdminBackup *nacv1alpha1.NonAdminBackup, enforcedBackupSpec *velerov1.BackupSpec) error {
 	enforcedSpec := reflect.ValueOf(enforcedBackupSpec).Elem()
@@ -213,9 +219,40 @@ Before creating NonAdminBackup's related Velero Backup, apply any missing fields
 		}
 ```
 
-For more details, check https://github.com/openshift/oadp-operator/pull/1584 and https://github.com/migtools/oadp-non-admin/pull/110.
+Modify ValidateRestoreSpec function to use `EnforceRestoreSpec` and apply that to non admin users' NonAdminBackup request
+```go
+	enforcedSpec := reflect.ValueOf(enforcedRestoreSpec).Elem()
+	for index := range enforcedSpec.NumField() {
+		enforcedField := enforcedSpec.Field(index)
+		enforcedFieldName := enforcedSpec.Type().Field(index).Name
+		currentField := reflect.ValueOf(nonAdminRestore.Spec.RestoreSpec).Elem().FieldByName(enforcedFieldName)
+		if !enforcedField.IsZero() && !currentField.IsZero() && !reflect.DeepEqual(enforcedField.Interface(), currentField.Interface()) {
+			field, _ := reflect.TypeOf(nonAdminRestore.Spec.RestoreSpec).Elem().FieldByName(enforcedFieldName)
+			tagName, _, _ := strings.Cut(field.Tag.Get("json"), ",")
+			return fmt.Errorf(
+				"NonAdminRestore spec.restoreSpec.%v field value is enforced by admin user, can not override it",
+				tagName,
+			)
+		}
+	}
+```
+
+Before creating NonAdminRestore's related Velero Restore, apply any missing fields to it that admin user has enforced
+```go
+		enforcedSpec := reflect.ValueOf(r.EnforcedRestoreSpec).Elem()
+		for index := range enforcedSpec.NumField() {
+			enforcedField := enforcedSpec.Field(index)
+			enforcedFieldName := enforcedSpec.Type().Field(index).Name
+			currentField := reflect.ValueOf(restoreSpec).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
 
-- Show NonAdminBackup spec.backupSpec fields/TODO NonAdminRestore custom default values to non admin users https://github.com/migtools/oadp-non-admin/issues/111
+- Show NonAdminBackup spec.backupSpec fields/NonAdminRestore spec.restoreSpec fields custom default values to non admin users https://github.com/migtools/oadp-non-admin/issues/111
 

go.mod

@@ -82,3 +82,5 @@ require (
 )
 
 replace github.com/vmware-tanzu/velero => github.com/openshift/velero v0.10.2-0.20240919150610-92244630d90b
+
+replace github.com/openshift/oadp-operator => github.com/mateusoliveira43/oadp-operator v0.0.0-20241128165459-cb4996d6a488

go.sum

@@ -67,6 +67,8 @@ github.com/kubernetes-csi/external-snapshotter/client/v7 v7.0.0 h1:j3YK74myEQRxR
 github.com/kubernetes-csi/external-snapshotter/client/v7 v7.0.0/go.mod h1:FlyYFe32mPxKEPaRXKNxfX576d1AoCzstYDoOOnyMA4=
 github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0=
 github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc=
+github.com/mateusoliveira43/oadp-operator v0.0.0-20241128165459-cb4996d6a488 h1:03HhsjqyLSdcKtGOtdQ//y5E1lzWmoDSqvEyLbcKSG8=
+github.com/mateusoliveira43/oadp-operator v0.0.0-20241128165459-cb4996d6a488/go.mod h1:ndXHIyjyavYVLFIi2EwfvpwUUSwPnjJo//CoyICO4aA=
 github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
 github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg=
 github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
@@ -78,8 +80,6 @@ github.com/onsi/ginkgo/v2 v2.19.0 h1:9Cnnf7UHo57Hy3k6/m5k3dRfGTMXGvxhHFvkDTCTpvA
 github.com/onsi/ginkgo/v2 v2.19.0/go.mod h1:rlwLi9PilAFJ8jCg9UE1QP6VBpd6/xj3SRC0d6TU0To=
 github.com/onsi/gomega v1.33.1 h1:dsYjIxxSR755MDmKVsaFQTE22ChNBcuuTWgkUDSubOk=
 github.com/onsi/gomega v1.33.1/go.mod h1:U4R44UsT+9eLIaYRB2a5qajjtQYn0hauxvRm16AVYg0=
-github.com/openshift/oadp-operator v1.0.2-0.20241119153315-6947e30c7ec5 h1:a+2WE+KDfBkZYNZqH+1e+T414rfX4e/oegsFRHPLtAM=
-github.com/openshift/oadp-operator v1.0.2-0.20241119153315-6947e30c7ec5/go.mod h1:ndXHIyjyavYVLFIi2EwfvpwUUSwPnjJo//CoyICO4aA=
 github.com/openshift/velero v0.10.2-0.20240919150610-92244630d90b h1:J8LV6NzonNemUxxsr76Lhl5+CnqBuQqojaf6Y7MwF24=
 github.com/openshift/velero v0.10.2-0.20240919150610-92244630d90b/go.mod h1:1Jk51qruLY/LCG8RMy6nVLVctIlWqJ9KBNXWroHzJZg=
 github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=

internal/common/function/function.go

@@ -115,7 +115,7 @@ func ValidateBackupSpec(nonAdminBackup *nacv1alpha1.NonAdminBackup, enforcedBack
 }
 
 // ValidateRestoreSpec return nil, if NonAdminRestore is valid; error otherwise
-func ValidateRestoreSpec(ctx context.Context, clientInstance client.Client, nonAdminRestore *nacv1alpha1.NonAdminRestore) error {
+func ValidateRestoreSpec(ctx context.Context, clientInstance client.Client, nonAdminRestore *nacv1alpha1.NonAdminRestore, enforcedRestoreSpec *velerov1.RestoreSpec) error {
 	if nonAdminRestore.Spec.RestoreSpec.BackupName == constant.EmptyString {
 		return fmt.Errorf("NonAdminRestore spec.restoreSpec.backupName is not set")
 	}
@@ -144,7 +144,20 @@ func ValidateRestoreSpec(ctx context.Context, clientInstance client.Client, nonA
 
 	// TODO nonAdminRestore.Spec.RestoreSpec.NamespaceMapping ?
 
-	// TODO enforce Restore Spec
+	enforcedSpec := reflect.ValueOf(enforcedRestoreSpec).Elem()
+	for index := range enforcedSpec.NumField() {
+		enforcedField := enforcedSpec.Field(index)
+		enforcedFieldName := enforcedSpec.Type().Field(index).Name
+		currentField := reflect.ValueOf(nonAdminRestore.Spec.RestoreSpec).Elem().FieldByName(enforcedFieldName)
+		if !enforcedField.IsZero() && !currentField.IsZero() && !reflect.DeepEqual(enforcedField.Interface(), currentField.Interface()) {
+			field, _ := reflect.TypeOf(nonAdminRestore.Spec.RestoreSpec).Elem().FieldByName(enforcedFieldName)
+			tagName, _, _ := strings.Cut(field.Tag.Get("json"), ",")
+			return fmt.Errorf(
+				"NonAdminRestore spec.restoreSpec.%v field value is enforced by admin user, can not override it",
+				tagName,
+			)
+		}
+	}
 
 	return nil
 }