-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
fix: Enforce NonAdminBackup spec field values #110
fix: Enforce NonAdminBackup spec field values #110
Conversation
Skipping CI for Draft Pull Request. |
187da0e
to
dba046c
Compare
Signed-off-by: Mateus Oliveira <[email protected]>
c4c3074
to
306351b
Compare
cmd/main.go
Outdated
setupLog.Error(err, "unable to create Kubernetes client") | ||
os.Exit(1) | ||
} | ||
// TODO we could pass DPA name as env var and do a get call directly. Better? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
run make manifests Signed-off-by: Mateus Oliveira <[email protected]>
/hold Blocked by openshift/oadp-operator#1584 |
Signed-off-by: Mateus Oliveira <[email protected]>
Signed-off-by: Mateus Oliveira <[email protected]>
encapsulate logic in function Signed-off-by: Mateus Oliveira <[email protected]>
/unhold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes worked fine for me:
spec:
backupSpec:
includedNamespaces:
- nacuser
snapshotVolumes: true
status:
conditions:
- lastTransitionTime: "2024-11-20T04:42:43Z"
message: NonAdminBackup spec.backupSpec.snapshotVolumes field value is enforced
by admin user, can not override it
reason: InvalidBackupSpec
status: "False"
type: Accepted
phase: BackingOff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Crash when no dpa.Spec.NonAdmin.EnforceBackupSpec is specified:
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x15d50bb]
goroutine 1 [running]:
main.getEnforcedSpec(0xc00042a908, {0xc00004c0f0, 0xd})
cmd/main.go:195 +0x1db
main.main()
cmd/main.go:116 +0x4fa
Fix:
diff --git a/cmd/main.go b/cmd/main.go
index 0d32e48..b8b50be 100644
--- a/cmd/main.go
+++ b/cmd/main.go
@@ -30,6 +30,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+
// 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"
@@ -192,7 +193,9 @@ func getEnforcedSpec(restConfig *rest.Config, oadpNamespace string) (*velerov1.B
enforcedBackupSpec := &velerov1.BackupSpec{}
for _, dpa := range dpaList.Items {
if dpa.Namespace == oadpNamespace {
- enforcedBackupSpec = dpa.Spec.NonAdmin.EnforceBackupSpec
+ if dpa.Spec.NonAdmin != nil && dpa.Spec.NonAdmin.EnforceBackupSpec != nil {
+ enforcedBackupSpec = dpa.Spec.NonAdmin.EnforceBackupSpec
+ }
}
}
return enforcedBackupSpec, nil
- Create the NonAdminBackup with different then enforced flag
Result: Proper BackingOff state and condition
First enforce failure:
status:
conditions:
- lastTransitionTime: '2024-11-21T11:19:58Z'
message: 'NonAdminBackup spec.backupSpec.snapshotVolumes field value is enforced by admin user, can not override it'
reason: InvalidBackupSpec
status: 'False'
type: Accepted
phase: BackingOff
-
Patch the NonAdminBackup with proper enforced flag
Result: Proper Created state and condition -
Patch again the NonAdminBackup with different then enforced flag
Result: roper BackingOff state and condition, veleroBackup reference is also proper as it was from the previous patch.
status:
conditions:
- lastTransitionTime: '2024-11-21T11:23:03Z'
message: 'NonAdminBackup spec.backupSpec.snapshotVolumes field value is enforced by admin user, can not override it'
reason: InvalidBackupSpec
status: 'False'
type: Accepted
- lastTransitionTime: '2024-11-21T11:21:05Z'
message: Created Velero Backup object
reason: BackupScheduled
status: 'True'
type: Queued
phase: BackingOff
veleroBackup:
nacuuid: default-exampleabc-8a10e3ec-0610-429a-86cf-421d5da151b2
name: default-exampleabc-8a10e3ec-0610-429a-86cf-421d5da151b2
namespace: openshift-adp
status: {}
Small fix to ensure EnforceBackupSpec is not causing panic. Signed-off-by: Michal Pryc <[email protected]>
Signed-off-by: Michal Pryc <[email protected]>
@shubham-pampattiwar there was also small lint issue while running
|
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mateusoliveira43, mpryc, shubham-pampattiwar The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
override snyk as it's not problematic atm |
Why the changes were made
Resolves #33
Related to: #37
Blocked by: openshift/oadp-operator#1584This PR adds fetching of OADP's DPA during NAC set up, adding DPA
spec.nonAdmin.enforceBackupSpec
toNonAdminBackupReconciler
and adds validation for NonAdminBackup spec respecting enforcement.How to test the changes made
Read design.
Follow install-from-source testing documentation, pointing this branch and my OADP fork branch
fix/enforce-non-admin-bckup-spec-field-values