diff --git a/cmd/main.go b/cmd/main.go index f364ca3..2050685 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -18,11 +18,14 @@ limitations under the License. package main import ( + "context" "crypto/tls" "flag" "fmt" "os" + // TODO when to update oadp-operator version in go.mod? + "github.com/openshift/oadp-operator/api/v1alpha1" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -30,7 +33,9 @@ import ( // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. _ "k8s.io/client-go/plugin/pkg/client/auth" + "k8s.io/client-go/rest" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" @@ -55,6 +60,8 @@ func init() { // +kubebuilder:scaffold:scheme } +// +kubebuilder:rbac:groups=oadp.openshift.io,resources=dataprotectionapplications,verbs=list + func main() { var metricsAddr string var enableLeaderElection bool @@ -104,7 +111,15 @@ func main() { os.Exit(1) } - mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ + restConfig := ctrl.GetConfigOrDie() + + enforcedBackupSpec, err := getEnforcedSpec(restConfig, oadpNamespace) + if err != nil { + setupLog.Error(err, "unable to get enforced spec") + os.Exit(1) + } + + mgr, err := ctrl.NewManager(restConfig, ctrl.Options{ Scheme: scheme, Metrics: metricsserver.Options{ BindAddress: metricsAddr, @@ -133,9 +148,10 @@ func main() { } if err = (&controller.NonAdminBackupReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - OADPNamespace: oadpNamespace, + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + OADPNamespace: oadpNamespace, + EnforcedBackupSpec: enforcedBackupSpec, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "NonAdminBackup") os.Exit(1) @@ -157,3 +173,29 @@ func main() { os.Exit(1) } } + +func getEnforcedSpec(restConfig *rest.Config, oadpNamespace string) (*velerov1.BackupSpec, error) { + dpaClientScheme := runtime.NewScheme() + utilruntime.Must(v1alpha1.AddToScheme(dpaClientScheme)) + dpaClient, err := client.New(restConfig, client.Options{ + Scheme: dpaClientScheme, + }) + if err != nil { + return 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 + } + enforcedBackupSpec := &velerov1.BackupSpec{} + for _, dpa := range dpaList.Items { + if dpa.Namespace == oadpNamespace { + if dpa.Spec.NonAdmin != nil && dpa.Spec.NonAdmin.EnforceBackupSpec != nil { + enforcedBackupSpec = dpa.Spec.NonAdmin.EnforceBackupSpec + } + } + } + return enforcedBackupSpec, nil +} diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 3471f6a..7a9ad1d 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -4,6 +4,12 @@ kind: ClusterRole metadata: name: non-admin-controller-role rules: +- apiGroups: + - oadp.openshift.io + resources: + - dataprotectionapplications + verbs: + - list - apiGroups: - oadp.openshift.io resources: diff --git a/docs/design/admin_control_over_spec.md b/docs/design/admin_control_over_spec.md new file mode 100644 index 0000000..3d3b17f --- /dev/null +++ b/docs/design/admin_control_over_spec.md @@ -0,0 +1,221 @@ +# Admin users control over non admin objects' spec + +## 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. + +## 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. + +## Goals + +Enable admin users to +- set custom default values for NonAdminBackup spec.backupSpec fields, which can not be overridden +- TODO restore + +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 +- 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. + +TODO restore + +> **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. + +To avoid mistakes, not all fields will be able to be enforced, like `IncludedNamespaces`, that 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. + +If a NonAdminBackup is created with fields overriding any enforced values, it will fail validation prior to creating an associated Velero Backup. Validation error is shown in NonAdminBackup status and NAC logs. + +If NonAdminBackup respects enforcement, the created associated Velero Backup will have the enforced spec field values. + +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. + +### Example workflows + +In this example, admin user has configured NAC with the following OADP DPA options +```yaml +... +spec: + ... + nonAdmin: + enable: true + enforceBackupSpecs: + snapshotVolumes: false + unsupportedOverrides: + tech-preview-ack: 'true' +``` + +That means, that the 2 following NonAdminBackup will be accepted by NAC validation +```yaml +... +spec: + backupSpec: + snapshotVolumes: false +``` + +```yaml +... +spec: + backupSpec: + ttl: 3h +``` +> **Note:** the related Velero Backup for this NonAdminBackup will have `spec.snapshotVolumes` set to false + +But this NonAdminBackup will not be accepted by NAC validation +```yaml +... +spec: + backupSpec: + snapshotVolumes: true +``` +NonAdminBackup status and NAC log will have the following message: +> spec.backupSpec.snapshotVolumes field value is enforced by admin user, can not override it + +## Alternatives Considered + +Instead of using a DPA field, using a ConfigMap was considered. Since users would not have type assertion when creating those ConfigMap and parsing it would be harder in NAC side, it was discarded. + +## Security Considerations + +No security considerations. + +## Compatibility + +No compatibility issues. + +## Implementation + +Add `EnforceBackupSpec` struct to OADP DPA `NonAdmin` struct +```go +type NonAdmin struct { + // which bakup spec field values to enforce + // +optional + EnforceBackupSpec *velero.BackupSpec `json:"enforceBackupSpec,omitempty"` +} +``` +TODO restore + +Validate admin user did not enforce a field that can break NAC functionality +```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") + } + } + } +``` + +Store previous `EnforceBackupSpec` value, so when admin user changes it, Deployment is also changed to trigger a Pod recreation +```go +const ( + enforcedBackupSpecKey = "enforced-backup-spec" +) + +var ( + previousEnforcedBackupSpec *velero.BackupSpec = nil + dpaBackupSpecResourceVersion = "" +) + +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 + enforcedSpecAnnotation := map[string]string{ + enforcedBackupSpecKey: dpaBackupSpecResourceVersion, + } + + templateObjectAnnotations := deploymentObject.Spec.Template.GetAnnotations() + if templateObjectAnnotations == nil { + deploymentObject.Spec.Template.SetAnnotations(enforcedSpecAnnotation) + } else { + templateObjectAnnotations[enforcedBackupSpecKey] = enforcedSpecAnnotation[enforcedBackupSpecKey] + // TODO same thing for restore + deploymentObject.Spec.Template.SetAnnotations(templateObjectAnnotations) + } +} +``` + +During NAC startup, read OADP DPA, to be able to apply admin user enforcement +```go + restConfig := ctrl.GetConfigOrDie() + + dpaClientScheme := runtime.NewScheme() + utilruntime.Must(v1alpha1.AddToScheme(dpaClientScheme)) + dpaClient, err := client.New(restConfig, client.Options{ + Scheme: dpaClientScheme, + }) + if err != nil { + setupLog.Error(err, "unable to create Kubernetes client") + os.Exit(1) + } + dpaList := &v1alpha1.DataProtectionApplicationList{} + err = dpaClient.List(context.Background(), dpaList) + if err != nil { + setupLog.Error(err, "unable to list DPAs") + os.Exit(1) + } +``` + +Modify ValidateSpec 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() + 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) + } + } +``` + +For more details, check https://github.com/openshift/oadp-operator/pull/1584 and https://github.com/migtools/oadp-non-admin/pull/110. + +## 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 + diff --git a/go.mod b/go.mod index c5fdf94..80aec0b 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/migtools/oadp-non-admin -go 1.22.0 +go 1.22.2 toolchain go1.22.6 @@ -9,11 +9,13 @@ 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.20241119153315-6947e30c7ec5 github.com/stretchr/testify v1.9.0 - github.com/vmware-tanzu/velero v1.12.0 + github.com/vmware-tanzu/velero v1.14.0 k8s.io/api v0.29.0 k8s.io/apimachinery v0.29.0 k8s.io/client-go v0.29.0 + k8s.io/utils v0.0.0-20230726121419-3b25d923346b sigs.k8s.io/controller-runtime v0.17.2 ) @@ -58,11 +60,11 @@ require ( go.uber.org/multierr v1.11.0 // indirect go.uber.org/zap v1.27.0 // indirect golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 // indirect - golang.org/x/net v0.26.0 // indirect + golang.org/x/net v0.29.0 // indirect golang.org/x/oauth2 v0.19.0 // indirect - golang.org/x/sys v0.21.0 // indirect - golang.org/x/term v0.21.0 // indirect - golang.org/x/text v0.16.0 // indirect + golang.org/x/sys v0.25.0 // indirect + golang.org/x/term v0.24.0 // indirect + golang.org/x/text v0.18.0 // indirect golang.org/x/time v0.5.0 // indirect golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d // indirect gomodules.xyz/jsonpatch/v2 v2.4.0 // indirect @@ -74,7 +76,6 @@ require ( k8s.io/component-base v0.29.0 // indirect k8s.io/klog/v2 v2.110.1 // indirect k8s.io/kube-openapi v0.0.0-20231010175941-2dd684a91f00 // indirect - k8s.io/utils v0.0.0-20230726121419-3b25d923346b // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect sigs.k8s.io/yaml v1.4.0 // indirect diff --git a/go.sum b/go.sum index 89000a1..c18fd53 100644 --- a/go.sum +++ b/go.sum @@ -78,6 +78,8 @@ 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= @@ -92,8 +94,8 @@ github.com/prometheus/common v0.52.3 h1:5f8uj6ZwHSscOGNdIQg6OiZv/ybiK2CO2q2drVZA github.com/prometheus/common v0.52.3/go.mod h1:BrxBKv3FWBIGXw89Mg1AeBq7FSyRzXWI3l3e7W3RN5U= github.com/prometheus/procfs v0.12.0 h1:jluTpSng7V9hY0O2R9DzzJHYb2xULk9VTR1V1R/k6Bo= github.com/prometheus/procfs v0.12.0/go.mod h1:pcuDEFsWDnvcgNzo4EEweacyhjeA9Zk3cnaOZAZEfOo= -github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ= -github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog= +github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= +github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/sirupsen/logrus v1.9.3 h1:dueUQJ1C2q9oE3F7wvmSGAaVtTmUizReu6fjN8uqzbQ= github.com/sirupsen/logrus v1.9.3/go.mod h1:naHLuLoDiP4jHNo9R0sCBMtWGeIprob74mVsIT4qYEQ= @@ -134,8 +136,8 @@ golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= -golang.org/x/net v0.26.0 h1:soB7SVo0PWrY4vPW/+ay0jKDNScG2X9wFeYlXIvJsOQ= -golang.org/x/net v0.26.0/go.mod h1:5YKkiSynbBIh3p6iOc/vibscux0x38BZDkn8sCUPxHE= +golang.org/x/net v0.29.0 h1:5ORfpBpCs4HzDYoodCDBbwHzdR5UrLBZ3sOnUJmFoHo= +golang.org/x/net v0.29.0/go.mod h1:gLkgy8jTGERgjzMic6DS9+SP0ajcu6Xu3Orq/SpETg0= golang.org/x/oauth2 v0.19.0 h1:9+E/EZBCbTLNrbN35fHv/a/d/mOBatymz1zbtQrXpIg= golang.org/x/oauth2 v0.19.0/go.mod h1:vYi7skDa1x015PmRRYZ7+s1cWyPgrPiSYRe4rnsexc8= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= @@ -145,14 +147,14 @@ golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5h golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.21.0 h1:rF+pYz3DAGSQAxAu1CbC7catZg4ebC4UIeIhKxBZvws= -golang.org/x/sys v0.21.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -golang.org/x/term v0.21.0 h1:WVXCp+/EBEHOj53Rvu+7KiT/iElMrO8ACK16SMZ3jaA= -golang.org/x/term v0.21.0/go.mod h1:ooXLefLobQVslOqselCNF4SxFAaoS6KujMbsGzSDmX0= +golang.org/x/sys v0.25.0 h1:r+8e+loiHxRqhXVl6ML1nO3l1+oFoWbnlu2Ehimmi34= +golang.org/x/sys v0.25.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +golang.org/x/term v0.24.0 h1:Mh5cbb+Zk2hqqXNO7S1iTjEphVL+jb8ZWaqh/g+JWkM= +golang.org/x/term v0.24.0/go.mod h1:lOBK/LVxemqiMij05LGJ0tzNr8xlmwBRJ81PX6wVLH8= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.16.0 h1:a94ExnEXNtEwYLGJSIUxnWoxoRz/ZcCsV63ROupILh4= -golang.org/x/text v0.16.0/go.mod h1:GhwF1Be+LQoKShO3cGOHzqOgRrGaYc9AvblQOmPVHnI= +golang.org/x/text v0.18.0 h1:XvMDiNzPAl0jr17s6W9lcaIhGUfUORdGCNsuLmPG224= +golang.org/x/text v0.18.0/go.mod h1:BuEKDfySbSR4drPmRPG/7iBdf8hvFMuRexcpahXilzY= golang.org/x/time v0.5.0 h1:o7cqy6amK/52YcAKIPlM3a+Fpj35zvRj2TP+e1xFSfk= golang.org/x/time v0.5.0/go.mod h1:3BpzKBy/shNhVucY/MWOyx10tF3SFh9QdLuxbVysPQM= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= diff --git a/internal/common/function/function.go b/internal/common/function/function.go index 4b0176c..40b03ea 100644 --- a/internal/common/function/function.go +++ b/internal/common/function/function.go @@ -20,6 +20,8 @@ package function import ( "context" "fmt" + "reflect" + "strings" "github.com/go-logr/logr" "github.com/google/uuid" @@ -62,7 +64,7 @@ func containsOnlyNamespace(namespaces []string, namespace string) bool { } // ValidateBackupSpec return nil, if NonAdminBackup is valid; error otherwise -func ValidateBackupSpec(nonAdminBackup *nacv1alpha1.NonAdminBackup) error { +func ValidateBackupSpec(nonAdminBackup *nacv1alpha1.NonAdminBackup, enforcedBackupSpec *velerov1.BackupSpec) error { // this should be Kubernetes API validation if nonAdminBackup.Spec.BackupSpec == nil { return fmt.Errorf("BackupSpec is not defined") @@ -70,7 +72,27 @@ func ValidateBackupSpec(nonAdminBackup *nacv1alpha1.NonAdminBackup) error { if nonAdminBackup.Spec.BackupSpec.IncludedNamespaces != nil { if !containsOnlyNamespace(nonAdminBackup.Spec.BackupSpec.IncludedNamespaces, nonAdminBackup.Namespace) { - return fmt.Errorf("spec.backupSpec.IncludedNamespaces can not contain namespaces other than: %s", nonAdminBackup.Namespace) + return fmt.Errorf("NonAdminBackup spec.backupSpec.includedNamespaces can not contain namespaces other than: %s", nonAdminBackup.Namespace) + } + } + if enforcedBackupSpec.IncludedNamespaces != nil { + if !containsOnlyNamespace(enforcedBackupSpec.IncludedNamespaces, nonAdminBackup.Namespace) { + return fmt.Errorf("NonAdminBackup spec.backupSpec.includedNamespaces enforced value by admin user violates NAC usage") + } + } + + enforcedSpec := reflect.ValueOf(enforcedBackupSpec).Elem() + for index := 0; index < enforcedSpec.NumField(); index++ { + 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, + ) } } diff --git a/internal/common/function/function_test.go b/internal/common/function/function_test.go index 26fb977..64b33de 100644 --- a/internal/common/function/function_test.go +++ b/internal/common/function/function_test.go @@ -19,17 +19,22 @@ package function import ( "context" "errors" + "reflect" + "slices" "strings" "testing" + "time" "github.com/google/uuid" "github.com/onsi/ginkgo/v2" "github.com/stretchr/testify/assert" velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -92,7 +97,7 @@ func TestValidateBackupSpec(t *testing.T) { spec: &velerov1.BackupSpec{ IncludedNamespaces: []string{"namespace1", "namespace2", "namespace3"}, }, - errMessage: "spec.backupSpec.IncludedNamespaces can not contain namespaces other than: non-admin-backup-namespace", + errMessage: "NonAdminBackup spec.backupSpec.includedNamespaces can not contain namespaces other than: non-admin-backup-namespace", }, { name: "valid spec", @@ -111,7 +116,7 @@ func TestValidateBackupSpec(t *testing.T) { BackupSpec: test.spec, }, } - err := ValidateBackupSpec(nonAdminBackup) + err := ValidateBackupSpec(nonAdminBackup, &velerov1.BackupSpec{}) if len(test.errMessage) == 0 { assert.NoError(t, err) } else { @@ -122,6 +127,250 @@ func TestValidateBackupSpec(t *testing.T) { } } +func TestValidateBackupSpecEnforcedFields(t *testing.T) { + all := "*" + + tests := []struct { + enforcedValue any + overrideValue any + name string + }{ + { + name: "Metadata", + enforcedValue: velerov1.Metadata{ + Labels: map[string]string{ + constant.OadpLabel: constant.TrueString, + }, + }, + overrideValue: velerov1.Metadata{ + Labels: map[string]string{ + "openshift/banana": "false", + constant.OadpLabel: constant.TrueString, + }, + }, + }, + { + name: "IncludedNamespaces", + enforcedValue: []string{"self-service-namespace"}, + overrideValue: []string{"openshift-adp"}, + }, + { + name: "ExcludedNamespaces", + enforcedValue: []string{"openshift-adp"}, + overrideValue: []string{"cherry"}, + }, + { + name: "IncludedResources", + enforcedValue: []string{"pods"}, + overrideValue: []string{"secrets"}, + }, + { + name: "ExcludedResources", + enforcedValue: []string{"nonadminbackups.nac.oadp.openshift.io"}, + overrideValue: []string{}, + }, + { + name: "IncludedClusterScopedResources", + enforcedValue: []string{}, + overrideValue: []string{all}, + }, + { + name: "ExcludedClusterScopedResources", + enforcedValue: []string{all}, + overrideValue: []string{}, + }, + { + name: "IncludedNamespaceScopedResources", + enforcedValue: []string{}, + overrideValue: []string{all}, + }, + { + name: "ExcludedNamespaceScopedResources", + enforcedValue: []string{all}, + overrideValue: []string{}, + }, + { + name: "LabelSelector", + enforcedValue: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "grapes": constant.TrueString, + }, + }, + overrideValue: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + constant.OadpLabel: constant.TrueString, + }, + }, + }, + { + name: "OrLabelSelectors", + enforcedValue: []*metav1.LabelSelector{ + { + MatchLabels: map[string]string{ + "kiwi": constant.TrueString, + }, + }, + { + MatchLabels: map[string]string{ + "green": "false", + }, + }, + }, + overrideValue: []*metav1.LabelSelector{ + { + MatchLabels: map[string]string{ + constant.OadpLabel: constant.TrueString, + }, + }, + }, + }, + { + name: "SnapshotVolumes", + enforcedValue: ptr.To(false), + overrideValue: ptr.To(true), + }, + { + name: "TTL", + enforcedValue: metav1.Duration{Duration: 12 * time.Hour}, //nolint:revive // just test + overrideValue: metav1.Duration{Duration: 30 * 24 * time.Hour}, //nolint:revive // just test + }, + { + name: "IncludeClusterResources", + enforcedValue: ptr.To(true), + overrideValue: ptr.To(false), + }, + { + name: "Hooks", + enforcedValue: velerov1.BackupHooks{ + Resources: []velerov1.BackupResourceHookSpec{ + { + Name: "test", + }, + }, + }, + overrideValue: velerov1.BackupHooks{ + Resources: []velerov1.BackupResourceHookSpec{ + { + Name: "another", + }, + }, + }, + }, + { + name: "StorageLocation", + enforcedValue: "default", + overrideValue: "lemon", + }, + { + name: "VolumeSnapshotLocations", + enforcedValue: []string{"aws"}, + overrideValue: []string{"gcp"}, + }, + { + name: "DefaultVolumesToRestic", + enforcedValue: ptr.To(true), + overrideValue: ptr.To(false), + }, + { + name: "DefaultVolumesToFsBackup", + enforcedValue: ptr.To(false), + overrideValue: ptr.To(true), + }, + { + name: "OrderedResources", + enforcedValue: map[string]string{ + "pods": "ns1/pod1,ns2/pod2", + }, + overrideValue: map[string]string{}, + }, + { + name: "CSISnapshotTimeout", + enforcedValue: metav1.Duration{Duration: 3 * time.Minute}, //nolint:revive // just test + overrideValue: metav1.Duration{Duration: 30 * time.Minute}, //nolint:revive // just test + }, + { + name: "ItemOperationTimeout", + enforcedValue: metav1.Duration{Duration: 30 * time.Minute}, //nolint:revive // just test + overrideValue: metav1.Duration{Duration: time.Hour}, + }, + { + name: "ResourcePolicy", + enforcedValue: &corev1.TypedLocalObjectReference{ + Kind: "test", + Name: "example", + }, + overrideValue: &corev1.TypedLocalObjectReference{}, + }, + { + name: "SnapshotMoveData", + enforcedValue: ptr.To(false), + overrideValue: ptr.To(true), + }, + { + name: "DataMover", + enforcedValue: "OADP", + overrideValue: "third-party", + }, + { + name: "UploaderConfig", + enforcedValue: &velerov1.UploaderConfigForBackup{ + ParallelFilesUpload: 2, //nolint:revive // just test + }, + overrideValue: &velerov1.UploaderConfigForBackup{ + ParallelFilesUpload: 32, //nolint:revive // just test + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + enforcedSpec := &velerov1.BackupSpec{} + reflect.ValueOf(enforcedSpec).Elem().FieldByName(test.name).Set(reflect.ValueOf(test.enforcedValue)) + + userNonAdminBackup := &nacv1alpha1.NonAdminBackup{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "self-service-namespace", + }, + Spec: nacv1alpha1.NonAdminBackupSpec{ + BackupSpec: &velerov1.BackupSpec{}, + }, + } + err := ValidateBackupSpec(userNonAdminBackup, enforcedSpec) + if err != nil { + t.Errorf("not setting backup spec field '%v' test failed: %v", test.name, err) + } + + reflect.ValueOf(userNonAdminBackup.Spec.BackupSpec).Elem().FieldByName(test.name).Set(reflect.ValueOf(test.enforcedValue)) + err = ValidateBackupSpec(userNonAdminBackup, enforcedSpec) + if err != nil { + t.Errorf("setting backup spec field '%v' with value respecting enforcement test failed: %v", test.name, err) + } + + reflect.ValueOf(userNonAdminBackup.Spec.BackupSpec).Elem().FieldByName(test.name).Set(reflect.ValueOf(test.overrideValue)) + err = ValidateBackupSpec(userNonAdminBackup, enforcedSpec) + if err == nil { + t.Errorf("setting backup spec field '%v' with value overriding enforcement test failed: %v", test.name, err) + } + }) + } + + t.Run("Ensure all backup spec fields were tested", func(t *testing.T) { + backupSpecFields := []string{} + for _, test := range tests { + backupSpecFields = append(backupSpecFields, test.name) + } + backupSpec := reflect.ValueOf(&velerov1.BackupSpec{}).Elem() + + for index := 0; index < backupSpec.NumField(); index++ { + if !slices.Contains(backupSpecFields, backupSpec.Type().Field(index).Name) { + t.Errorf("backup spec field '%v' is not tested", backupSpec.Type().Field(index).Name) + } + } + if backupSpec.NumField() != len(tests) { + t.Errorf("list of tests have different number of elements") + } + }) +} + func TestGenerateNacObjectNameWithUUID(t *testing.T) { tests := []struct { name string diff --git a/internal/controller/nonadminbackup_controller.go b/internal/controller/nonadminbackup_controller.go index 1ffbdeb..00231d3 100644 --- a/internal/controller/nonadminbackup_controller.go +++ b/internal/controller/nonadminbackup_controller.go @@ -48,8 +48,9 @@ import ( // NonAdminBackupReconciler reconciles a NonAdminBackup object type NonAdminBackupReconciler struct { client.Client - Scheme *runtime.Scheme - OADPNamespace string + Scheme *runtime.Scheme + EnforcedBackupSpec *velerov1.BackupSpec + OADPNamespace string } type reconcileStepFunction func(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) @@ -477,7 +478,7 @@ func (r *NonAdminBackupReconciler) initNabCreate(ctx context.Context, logger log // If the BackupSpec is invalid, the function sets the NonAdminBackup condition Accepted to "False". // If the BackupSpec is valid, the function sets the NonAdminBackup condition Accepted to "True". func (r *NonAdminBackupReconciler) validateSpec(ctx context.Context, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup) (bool, error) { - err := function.ValidateBackupSpec(nab) + err := function.ValidateBackupSpec(nab, r.EnforcedBackupSpec) if err != nil { updatedPhase := updateNonAdminPhase(&nab.Status.Phase, nacv1alpha1.NonAdminBackupPhaseBackingOff) updatedCondition := meta.SetStatusCondition(&nab.Status.Conditions, @@ -605,6 +606,16 @@ func (r *NonAdminBackupReconciler) createVeleroBackupAndSyncWithNonAdminBackup(c backupSpec := nab.Spec.BackupSpec.DeepCopy() backupSpec.IncludedNamespaces = []string{nab.Namespace} + enforcedSpec := reflect.ValueOf(r.EnforcedBackupSpec).Elem() + for index := 0; index < enforcedSpec.NumField(); index++ { + 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) + } + } + veleroBackup := velerov1.Backup{ ObjectMeta: metav1.ObjectMeta{ Name: veleroBackupNACUUID, diff --git a/internal/controller/nonadminbackup_controller_test.go b/internal/controller/nonadminbackup_controller_test.go index feb7f96..495059f 100644 --- a/internal/controller/nonadminbackup_controller_test.go +++ b/internal/controller/nonadminbackup_controller_test.go @@ -31,6 +31,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -55,8 +56,9 @@ type nonAdminBackupSingleReconcileScenario struct { } type nonAdminBackupFullReconcileScenario struct { - spec nacv1alpha1.NonAdminBackupSpec - status nacv1alpha1.NonAdminBackupStatus + enforcedBackupSpec *velerov1.BackupSpec + spec nacv1alpha1.NonAdminBackupSpec + status nacv1alpha1.NonAdminBackupStatus } func buildTestNonAdminBackup(nonAdminNamespace string, nonAdminName string, spec nacv1alpha1.NonAdminBackupSpec) *nacv1alpha1.NonAdminBackup { @@ -261,9 +263,10 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func // priorResourceVersion, err := strconv.Atoi(nonAdminBackup.ResourceVersion) // gomega.Expect(err).To(gomega.Not(gomega.HaveOccurred())) result, err := (&NonAdminBackupReconciler{ - Client: k8sClient, - Scheme: testEnv.Scheme, - OADPNamespace: oadpNamespace, + Client: k8sClient, + Scheme: testEnv.Scheme, + OADPNamespace: oadpNamespace, + EnforcedBackupSpec: &velerov1.BackupSpec{}, }).Reconcile( context.Background(), reconcile.Request{NamespacedName: types.NamespacedName{ @@ -734,11 +737,11 @@ var _ = ginkgo.Describe("Test single reconciles of NonAdminBackup Reconcile func Type: "Accepted", Status: metav1.ConditionFalse, Reason: "InvalidBackupSpec", - Message: "spec.backupSpec.IncludedNamespaces can not contain namespaces other than:", + Message: "NonAdminBackup spec.backupSpec.includedNamespaces can not contain namespaces other than:", }, }, }, - resultError: reconcile.TerminalError(fmt.Errorf("spec.backupSpec.IncludedNamespaces can not contain namespaces other than: ")), + resultError: reconcile.TerminalError(fmt.Errorf("NonAdminBackup spec.backupSpec.includedNamespaces can not contain namespaces other than: ")), })) }) @@ -778,10 +781,15 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", }) gomega.Expect(err).ToNot(gomega.HaveOccurred()) + enforcedBackupSpec := &velerov1.BackupSpec{} + if scenario.enforcedBackupSpec != nil { + enforcedBackupSpec = scenario.enforcedBackupSpec + } err = (&NonAdminBackupReconciler{ - Client: k8sManager.GetClient(), - Scheme: k8sManager.GetScheme(), - OADPNamespace: oadpNamespace, + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + OADPNamespace: oadpNamespace, + EnforcedBackupSpec: enforcedBackupSpec, }).SetupWithManager(k8sManager) gomega.Expect(err).ToNot(gomega.HaveOccurred()) @@ -835,8 +843,6 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", scenario.spec, )).To(gomega.BeTrue()) - ginkgo.By("Simulating VeleroBackup update to finished state") - veleroBackup := &velerov1.Backup{} gomega.Expect(k8sClient.Get( ctxTimeout, @@ -847,11 +853,20 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", veleroBackup, )).To(gomega.Succeed()) - // can not call .Status().Update() for veleroBackup object https://github.com/vmware-tanzu/velero/issues/8285 + if scenario.enforcedBackupSpec != nil { + ginkgo.By("Validating Velero Backup Spec") + expectedSpec := scenario.enforcedBackupSpec.DeepCopy() + expectedSpec.IncludedNamespaces = []string{nonAdminObjectNamespace} + gomega.Expect(reflect.DeepEqual(veleroBackup.Spec, *expectedSpec)).To(gomega.BeTrue()) + } + + ginkgo.By("Simulating VeleroBackup update to finished state") + veleroBackup.Status = velerov1.BackupStatus{ Phase: velerov1.BackupPhaseCompleted, } + // can not call .Status().Update() for veleroBackup object https://github.com/vmware-tanzu/velero/issues/8285 gomega.Expect(k8sClient.Update(ctxTimeout, veleroBackup)).To(gomega.Succeed()) ginkgo.By("VeleroBackup updated") @@ -906,11 +921,18 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", }, }, }, + enforcedBackupSpec: &velerov1.BackupSpec{ + SnapshotVolumes: ptr.To(false), + TTL: metav1.Duration{ + Duration: 36 * time.Hour, + }, + DefaultVolumesToFsBackup: ptr.To(true), + }, }), ginkgo.Entry("Should update NonAdminBackup until it invalidates and then delete it", nonAdminBackupFullReconcileScenario{ spec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{ - IncludedNamespaces: []string{"not-valid"}, + SnapshotVolumes: ptr.To(true), }, }, status: nacv1alpha1.NonAdminBackupStatus{ @@ -920,10 +942,13 @@ var _ = ginkgo.Describe("Test full reconcile loop of NonAdminBackup Controller", Type: "Accepted", Status: metav1.ConditionFalse, Reason: "InvalidBackupSpec", - Message: "spec.backupSpec.IncludedNamespaces can not contain namespaces other than:", + Message: "spec.backupSpec.snapshotVolumes field value is enforced by admin user", }, }, }, + enforcedBackupSpec: &velerov1.BackupSpec{ + SnapshotVolumes: ptr.To(false), + }, }), ) }) diff --git a/internal/predicate/nonadminbackup_predicate.go b/internal/predicate/nonadminbackup_predicate.go index c4d001c..1657d2c 100644 --- a/internal/predicate/nonadminbackup_predicate.go +++ b/internal/predicate/nonadminbackup_predicate.go @@ -40,7 +40,6 @@ func (NonAdminBackupPredicate) Create(ctx context.Context, evt event.CreateEvent func (NonAdminBackupPredicate) Update(ctx context.Context, evt event.UpdateEvent) bool { logger := function.GetLogger(ctx, evt.ObjectNew, nonAdminBackupPredicateKey) - // spec change if evt.ObjectNew.GetGeneration() != evt.ObjectOld.GetGeneration() { logger.V(1).Info("Accepted Update event") return true