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.

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

Conversation

mateusoliveira43
Copy link
Contributor

@mateusoliveira43 mateusoliveira43 commented Nov 7, 2024

Why the changes were made

Resolves #33

Related to: #37

Blocked by: openshift/oadp-operator#1584

This PR adds fetching of OADP's DPA during NAC set up, adding DPA spec.nonAdmin.enforceBackupSpec to NonAdminBackupReconciler and adds validation for NonAdminBackup spec respecting enforcement.

Note: admin user can not enforce falsy values.

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

Copy link

openshift-ci bot commented Nov 7, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

go.mod Outdated Show resolved Hide resolved
docs/design/admin_control_over_spec.md Outdated Show resolved Hide resolved
docs/design/admin_control_over_spec.md Outdated Show resolved Hide resolved
docs/design/admin_control_over_spec.md Outdated Show resolved Hide resolved
docs/design/admin_control_over_spec.md Show resolved Hide resolved
docs/design/admin_control_over_spec.md Show resolved Hide resolved
docs/design/admin_control_over_spec.md Show resolved Hide resolved
@mateusoliveira43 mateusoliveira43 force-pushed the fix/enforce-backup-spec-field-values branch from c4c3074 to 306351b Compare November 14, 2024 11:41
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?
Copy link
Contributor Author

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]>
@mateusoliveira43 mateusoliveira43 marked this pull request as ready for review November 14, 2024 11:52
@openshift-ci openshift-ci bot requested review from mpryc and mrnold November 14, 2024 11:52
@mateusoliveira43
Copy link
Contributor Author

/hold Blocked by openshift/oadp-operator#1584

cmd/main.go Outdated Show resolved Hide resolved
@mateusoliveira43
Copy link
Contributor Author

/unhold

Copy link
Member

@shubham-pampattiwar shubham-pampattiwar left a 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

Copy link
Collaborator

@mpryc mpryc left a comment

Choose a reason for hiding this comment

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

  1. 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
  1. 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
  1. Patch the NonAdminBackup with proper enforced flag
    Result: Proper Created state and condition

  2. 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]>
@mpryc
Copy link
Collaborator

mpryc commented Nov 21, 2024

@shubham-pampattiwar there was also small lint issue while running make lint on that PR, should be fixed now:

internal/common/function/function.go:85:21: cannot range over enforcedSpec.NumField() (value of type int) (typecheck)
	for index := range enforcedSpec.NumField() {
	                   ^
internal/common/function/function_test.go:363:22: cannot range over backupSpec.NumField() (value of type int) (typecheck)
		for index := range backupSpec.NumField() {
		                   ^
internal/controller/nonadminbackup_controller.go:610:22: cannot range over enforcedSpec.NumField() (value of type int) (typecheck)
		for index := range enforcedSpec.NumField() {
		                   ^
make: *** [Makefile:75: lint] Error 1



cmd/main.go:33: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/migtools/oadp-non-admin) (gci)

@mpryc
Copy link
Collaborator

mpryc commented Nov 21, 2024

/lgtm

Copy link

openshift-ci bot commented Nov 21, 2024

[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:
  • OWNERS [mateusoliveira43,mpryc,shubham-pampattiwar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@shubham-pampattiwar
Copy link
Member

/retest

@shubham-pampattiwar shubham-pampattiwar merged commit 9fdcb88 into migtools:master Nov 21, 2024
6 of 8 checks passed
@mpryc
Copy link
Collaborator

mpryc commented Nov 21, 2024

override snyk as it's not problematic atm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants