Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated enforcement design to align with the issue 151 #156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Updated enforcement design to align with the issue 151 #156

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
213 changes: 126 additions & 87 deletions docs/design/admin_control_over_spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,49 +2,63 @@

## 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 relevant 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.
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.
The Non Admin Controller (NAC) restricts how non-admin users interact with the OADP operator. It allows them to create backups, restores, and backup storage locations only within their own namespaces using `NonAdminBackup`, `NonAdminRestore`, and `NonAdminBackupStorageLocation`, while enforcing additional restrictions and policies.

The NAC Controller enforces two types of controls: **restrictions** and **enforcements**.

- **Controller Restrictions** define policies that neither users nor cluster administrators can override.
- **Controller Enforcements** define policies that administrators can override if needed.

This design allows system administrators to manage **Controller Enforcements**, enabling them to set custom default values for the `NonAdminBackup`, `NonAdminRestore`, and `NonAdminBackupStorageLocation` spec fields, which non-admin users cannot override.

For example, administrators may want to enforce specific rules on non-admin user operations, such as:

- Enforcing a specific time to live (TTL) for Velero Backups associated with `NonAdminBackup`.
- Limiting the region that users can select within Velero `BackupStorageLocation`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What benefit does limiting region bring to admins exactly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • AWS pricing varies by region. Admins can enforce the use of cost-effective regions and avoid expensive cross-region data transfer fees.
  • Performance optimization to ensure s3 region is closer to the cluster
  • Simplified management by ensuring everything is in one regions so admins can review users usage based on one region, they can better monitor access logs, apply regional security policies as everything is bound to one region
  • I may also think that GDPR or HIPAA may take place where some data must be stored in a specific locations, e.g. in Poland you can not store any medical data outside of physical location data centers which must be based in the EEA (EU countries + Norway, Iceland, Liechtenstein):
GDPR (General Data Protection Regulation)
[...]
Article 44-49: Transfers of personal data outside the European Economic Area (EEA) (EU + Norway, Iceland, Liechtenstein) are restricted unless specific conditions are met.

So above in AWS, Azure, Google Cloud - you must ensure data is stored in an EEA-based region (e.g., AWS Frankfurt, AWS Stockholm).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we are assuming non admins who brought their own creds are actually sharing admin budgets constraints..

I guess up to admin to decide.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance optimization to ensure s3 region is closer to the cluster

enforcement error/warn should say what is allowed then

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the line 32 for this?

https://github.com/migtools/oadp-non-admin/pull/156/files#diff-788dfa1caf04072e867eaa6c490201f32d2e8bdf4cda035ba397d4d974737850R32

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Just the background was unclear to me. Perhaps adding above bullet points to background would be great.

- Controlling which resource types can be restored in a `NonAdminRestore` request.

## 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 enforced values for NonAdminBackup spec.backupSpec fields, which can not be overridden
- set custom enforced values for NonAdminRestore spec.restoreSpec fields, which can not be overridden
- set custom enforced 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
- Show custom enforced values validation errors in NAC object statuses and in NAC logs
- Each enforced value is associated with a specific spec field. If a spec field is a map or a list, enforcing one key/value pair will not override the whole map/list
- Custom enforced values are overriding the Spec default values
- Non Admin users must define all fields that are within the enforced spec field

## 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
- 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`)
- Prevent non admin users to create NonAdminBackup/NonAdminRestore/NonAdminBackupStorageLocation objects with overridden defaults. During reconciliation, NAC will check if the object has different values than enforced ones and will error if it does.
- Show NonAdminBackup spec.backupSpec fields/NonAdminRestore spec.restoreSpec fields/NonAdminBackupStorageLocation spec.backupStorageLocationSpec fields enforced values to the non admin users
- 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.
Additional fields will be added to the OADP DPA object:
- `spec.nonAdmin.enforceBackupSpec`
- `spec.nonAdmin.enforceRestoreSpec`
- `spec.nonAdmin.enforceBackupStorageLocationSpec`

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.
These fields allow admin users to define custom enforced values for the `spec.backupSpec`, `spec.restoreSpec`, and `spec.backupStorageLocationSpec` fields within NonAdminBackup, NonAdminRestore, and NonAdminBackupStorageLocation objects, respectively. The Non Admin Controller (NAC) will ensure compliance with these values, and any NonAdmin object that attempts to override them will fail validation before the creation of the associated Velero object, whether it be a Backup, Restore, or BackupStorageLocation. Furthermore, if an admin user updates any enforced field value, the NAC Pod will be automatically recreated to reflect the latest administrative settings, ensuring that the system consistently enforces the specified policies and maintains operational integrity within the OADP operator.

> **Note:** if there are on-going NAC operations prior to recreating NAC Pod, reconcile progress might get lost for NAC objects.

## Detailed Design

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.
With it, admin users will be able to select which NonAdminBackup `spec.backupSpec` fields have custom enforced (new default) values.

To avoid mistakes, not all fields will be able to be enforced, like `IncludedNamespaces`, that could break NAC usage.
To avoid mistakes, not all fields will be able to be enforced, like `IncludedNamespaces` which is within **Controller Restrictions** fields and admin users should not enforce it, which could break NAC usage.

NAC will respect the set values by reading DPA field during startup.If admin user changes any enforced field value, NAC Pod is recreated (and only NAC Pod) to always be up to date with admin user enforcements.

Expand All @@ -54,8 +68,14 @@ If NonAdminBackup respects enforcement, the created associated Velero Backup wil

Enforcement is done dynamically. If new field is added to Velero Backup Spec, it will be presented to user without code changes. If a field changes type/or default value, tests will warn us.

Similar enforcement is done for NonAdminRestore and NonAdminBackupStorageLocation.

It is important to note that the enforcement of one field will not override the whole map/list.

### Example workflows

#### Admin user configures NAC with enforced spec fields

In this example, admin user has configured NAC with the following OADP DPA options
```yaml
...
Expand All @@ -65,35 +85,84 @@ spec:
enable: true
enforceBackupSpecs:
snapshotVolumes: false
unsupportedOverrides:
tech-preview-ack: 'true'
```

That means, that the 2 following NonAdminBackup will be accepted by NAC validation
That means, that the following NonAdminBackup will be accepted by NAC validation
```yaml
...
spec:
backupSpec:
snapshotVolumes: false
ttl: 3h
```

> **Note:** the related Velero Backup for this NonAdminBackup will have `spec.snapshotVolumes` set to false, ttl set to 3h and other fields set to their default values.

But this NonAdminBackup will not be accepted by NAC validation
```yaml
...
spec:
backupSpec:
ttl: 3h
snapshotVolumes: true
```
> **Note:** the related Velero Backup for this NonAdminBackup will have `spec.snapshotVolumes` set to false
NonAdminBackup status and NAC log will have the following message:
> spec.backupSpec.snapshotVolumes field value is enforced by admin user, can not override it

But this NonAdminBackup will not be accepted by NAC validation
Also the following NonAdminBackup will be rejected, because user did not define all fields that are within the enforced spec field:
```yaml
...
spec:
backupSpec:
snapshotVolumes: true
ttl: 3h
```

NonAdminBackup status and NAC log will have the following message:
> spec.backupSpec.snapshotVolumes field value is enforced by admin user, can not override it
> spec.backupSpec.ttl field value is enforced by admin user, can not override it

#### Admin user configures NAC with enforced spec map/list fields

In this example, admin user has configured NAC with the following OADP DPA options, from which only `config.region` and `objectStorage.prefix` are enforced and the `config` is a map
```yaml
...
spec:
...
nonAdmin:
enable: true
enforceBackupStorageLocationSpec:
config:
region: eu-central-1
objectStorage:
prefix: non-admin-buckets
provider: aws
```

That means, that the following NonAdminBackupStorageLocation will be accepted by NAC validation
```yaml
...
spec:
backupStorageLocationSpec:
config:
serverSideEncryption: AES256
region: eu-central-1
objectStorage:
prefix: non-admin-buckets
provider: aws
```

The following NonAdminBackupStorageLocation will be rejected, because user did not define all fields that are within the enforced spec field:
```yaml
...
spec:
backupStorageLocationSpec:
config:
serverSideEncryption: AES256
objectStorage:
prefix: non-admin-buckets
provider: aws
```

NonAdminBackupStorageLocation status and NAC log will have the following message:
> spec.backupStorageLocationSpec.config.region field value is enforced by admin user, can not override it

## Alternatives Considered

Expand Down Expand Up @@ -127,18 +196,30 @@ type NonAdmin struct {
}
```

Store previous `EnforceBackupSpec` and `EnforceRestoreSpec` value, so when admin user changes it, Deployment is also changed to trigger a Pod recreation
Add `EnforceBackupStorageLocationSpec` struct to OADP DPA `NonAdmin` struct
```go
type NonAdmin struct {
// which backupStorageLocation spec field values to enforce
// +optional
EnforceBackupStorageLocationSpec *velero.BackupStorageLocationSpec `json:"enforceBackupStorageLocationSpec,omitempty"`
}
```

Store previous `EnforceBackupSpec`, `EnforceRestoreSpec` and `EnforceBackupStorageLocationSpec` 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"
enforcedBackupStorageLocationSpecKey = "enforced-backup-storage-location-spec"
)

var (
previousEnforcedBackupSpec *velero.BackupSpec = nil
dpaBackupSpecResourceVersion = ""
previousEnforcedRestoreSpec *velero.RestoreSpec = nil
dpaRestoreSpecResourceVersion = ""
previousEnforcedBackupStorageLocationSpec *velero.BackupStorageLocationSpec = nil
dpaBackupStorageLocationSpecResourceVersion = ""
)

func ensureRequiredSpecs(deploymentObject *appsv1.Deployment, dpa *oadpv1alpha1.DataProtectionApplication, image string, imagePullPolicy corev1.PullPolicy) error {
Expand All @@ -150,9 +231,14 @@ func ensureRequiredSpecs(deploymentObject *appsv1.Deployment, dpa *oadpv1alpha1.
dpaRestoreSpecResourceVersion = dpa.GetResourceVersion()
}
previousEnforcedRestoreSpec = dpa.Spec.NonAdmin.EnforceRestoreSpec
if len(dpaBackupStorageLocationSpecResourceVersion) == 0 || !reflect.DeepEqual(dpa.Spec.NonAdmin.EnforceBackupStorageLocationSpec, previousEnforcedBackupStorageLocationSpec) {
dpaBackupStorageLocationSpecResourceVersion = dpa.GetResourceVersion()
}
previousEnforcedBackupStorageLocationSpec = dpa.Spec.NonAdmin.EnforceBackupStorageLocationSpec
enforcedSpecAnnotation := map[string]string{
enforcedBackupSpecKey: dpaBackupSpecResourceVersion,
enforcedRestoreSpecKey: dpaRestoreSpecResourceVersion,
enforcedBackupStorageLocationSpecKey: dpaBackupStorageLocationSpecResourceVersion,
}

templateObjectAnnotations := deploymentObject.Spec.Template.GetAnnotations()
Expand All @@ -161,6 +247,7 @@ func ensureRequiredSpecs(deploymentObject *appsv1.Deployment, dpa *oadpv1alpha1.
} else {
templateObjectAnnotations[enforcedBackupSpecKey] = enforcedSpecAnnotation[enforcedBackupSpecKey]
templateObjectAnnotations[enforcedRestoreSpecKey] = enforcedSpecAnnotation[enforcedRestoreSpecKey]
templateObjectAnnotations[enforcedBackupStorageLocationSpecKey] = enforcedSpecAnnotation[enforcedBackupStorageLocationSpecKey]
deploymentObject.Spec.Template.SetAnnotations(templateObjectAnnotations)
}
}
Expand All @@ -187,71 +274,23 @@ During NAC startup, read OADP DPA, to be able to apply admin user enforcement
}
```

Modify ValidateBackupSpec function to use `EnforceBackupSpec` and apply that to non admin users' NonAdminBackup request
Implement `EnforceNacSpec` function to validate NonAdminBackup/NonAdminRestore/NonAdminBackupStorageLocation spec from Validation functions:
```go
func ValidateBackupSpec(nonAdminBackup *nacv1alpha1.NonAdminBackup, enforcedBackupSpec *velerov1.BackupSpec) error {
enforcedSpec := reflect.ValueOf(enforcedBackupSpec).Elem()
for index := range enforcedSpec.NumField() {
enforcedField := enforcedSpec.Field(index)
enforcedFieldName := enforcedSpec.Type().Field(index).Name
currentField := reflect.ValueOf(nonAdminBackup.Spec.BackupSpec).Elem().FieldByName(enforcedFieldName)
if !enforcedField.IsZero() && !currentField.IsZero() && !reflect.DeepEqual(enforcedField.Interface(), currentField.Interface()) {
field, _ := reflect.TypeOf(nonAdminBackup.Spec.BackupSpec).Elem().FieldByName(enforcedFieldName)
tagName, _, _ := strings.Cut(field.Tag.Get("json"), ",")
return fmt.Errorf(
"NonAdminBackup spec.backupSpec.%v field value is enforced by admin user, can not override it",
tagName,
)
}
}
}
```

Before creating NonAdminBackup's related Velero Backup, apply any missing fields to it that admin user has enforced
```go
enforcedSpec := reflect.ValueOf(r.EnforcedBackupSpec).Elem()
for index := range enforcedSpec.NumField() {
enforcedField := enforcedSpec.Field(index)
enforcedFieldName := enforcedSpec.Type().Field(index).Name
currentField := reflect.ValueOf(backupSpec).Elem().FieldByName(enforcedFieldName)
if !enforcedField.IsZero() && currentField.IsZero() {
currentField.Set(enforcedField)
}
}
```
func EnforceNacSpec(currentSpec, enforcedSpec any) error {
// Implementation not part of the design
}

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,
)
}
}
```
// Validate NonAdminBackup/NonAdminRestore/NonAdminBackupStorageLocation spec from ValidateBackupSpec/ValidateRestoreSpec/ValidateBackupStorageLocationSpec functions. Example on the RestoreSpec validation:
err = EnforceNacSpec(nonAdminRestore.Spec.RestoreSpec, enforcedRestoreSpec)
if err != nil {
return err
}

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)
}
}
return nil
```

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.
For more details, check [Issue 151](https://github.com/migtools/oadp-non-admin/issues/151) and [PR 1584](https://github.com/openshift/oadp-operator/pull/1584), [PR 110](https://github.com/migtools/oadp-non-admin/pull/110), [PR 1600](https://github.com/openshift/oadp-operator/pull/1600) and [PR 122](https://github.com/migtools/oadp-non-admin/pull/122).

## Open Issues

Expand Down