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

Validate namespaces, so non admin can not create backups outside of ns #56

Merged
merged 1 commit into from
May 8, 2024

Conversation

mpryc
Copy link
Collaborator

@mpryc mpryc commented Apr 30, 2024

Validate namespaces, so non admin can not create backups outside of ns

Validation ensures the namespace in the VeleroBackup object are the same as the namespace for which NAB resides.

Note this PR is on top of #54 which should be merged first.

How to test

  1. Setup non-admin user as per instructions: https://github.com/migtools/oadp-non-admin/blob/master/docs/non_admin_user.md
  2. Create two NonAdminBackup objects in the namespace for which the non-admin user have namespace admin rights. After each creation see if at the end of reconcile the NonAdminBackup is correctly updated with its status and if in some cases VeleroBackup object was created in the openshift-adp namespace (as an admin) or not created if there was issue with namespace.

All of the below are in the non-admin namespace nacproject:

a) Case where there is backupSpec, but empty, should create Velero Backup with nacproject namespace in the includedNamespaces (below is not full spec, removed managedFields from it):

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  name: empty-backupspec
  namespace: nacproject
spec: 
  backupSpec: {}

Result should be created Velero Backup with includedNamespaces same as the origin NAB namespace and updated NonAdminBackup (below is not full spec, removed managedFields from it):

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  creationTimestamp: '2024-05-02T08:27:46Z'
  name: empty-backupspec
  namespace: nacproject
  resourceVersion: '71261871'
  uid: 624c270e-2711-490d-923e-aaebfcc2a420
spec:
  backupSpec:
    volumeSnapshotLocations:
      - velero-sample-1
    defaultVolumesToFsBackup: false
    csiSnapshotTimeout: 10m0s
    ttl: 720h0m0s
    itemOperationTimeout: 4h0m0s
    metadata: {}
    storageLocation: velero-sample-1
    hooks: {}
    includedNamespaces:
      - nacproject
    snapshotMoveData: false
status:
  conditions:
    - lastTransitionTime: '2024-05-02T08:27:47Z'
      message: backup accepted
      reason: BackupAccepted
      status: 'True'
      type: Accepted
    - lastTransitionTime: '2024-05-02T08:27:57Z'
      message: Created Velero Backup object
      reason: BackupScheduled
      status: 'True'
      type: Queued
  phase: Created
  veleroBackupName: nab-nacproject-d0b80d478b6a26
  veleroBackupNamespace: openshift-adp
  veleroBackupStatus:
    expiration: '2024-06-01T08:27:57Z'
    failureReason: >-
      unable to get credentials: unable to get key for secret: Secret
      "cloud-credentials" not found
    formatVersion: 1.1.0
    phase: Failed
    startTimestamp: '2024-05-02T08:27:57Z'
    version: 1

b) Create NonAdminBackup with includedNamespaces that does not match the namespace from the NonAdminBackup. It may include namespace of the NAB object, but on top of that it should have also other namespaces.

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  name: example-wrongnamespace
  namespace: nacproject
spec:
   backupSpec:
     includedNamespaces:
       - othernamespace
       - yetanothernamespace
       - nacproject

Result should be NAB in backingOff phase with spec.backupSpec.IncludedNamespaces can not contain namespaces other then: nacproject condition message:

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  creationTimestamp: '2024-05-02T08:33:35Z'
  generation: 1
  name: example-wrongnamespace
  namespace: nacproject
  resourceVersion: '71264312'
  uid: 2716a72d-3e44-466d-8126-5bcdeed9413b
spec:
  backupSpec:
    includedNamespaces:
       - othernamespace
       - yetanothernamespace
       - nacproject
status:
  conditions:
    - lastTransitionTime: '2024-05-02T08:33:45Z'
      message: >-
        spec.backupSpec.IncludedNamespaces can not contain namespaces other
        then: nacproject
      reason: InvalidBackupSpec
      status: 'False'
      type: Accepted
  phase: BackingOff

@openshift-ci openshift-ci bot requested a review from kaovilai April 30, 2024 10:37
@mpryc mpryc requested review from weshayutin and shubham-pampattiwar and removed request for kaovilai April 30, 2024 10:37
@mpryc
Copy link
Collaborator Author

mpryc commented Apr 30, 2024

@mateusoliveira43 @shubham-pampattiwar @weshayutin This PR is really all about that commit only, because other commits are covered in #54:

152d1c7ba3466c37a66fff2304f111136182e351

@mpryc mpryc self-assigned this Apr 30, 2024
@mpryc
Copy link
Collaborator Author

mpryc commented Apr 30, 2024

Fixes #49

@mpryc mpryc mentioned this pull request May 2, 2024
Validation ensures the namespace in the VeleroBackup object are the same
as the namespace for which NAB resides.

Signed-off-by: Michal Pryc <[email protected]>
@mpryc
Copy link
Collaborator Author

mpryc commented May 8, 2024

Rebased validation of namespaces. @mateusoliveira43 @shubham-pampattiwar please review as it's on top of previous work with merged reconcile functions.

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.

/lgtm

Copy link

openshift-ci bot commented May 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 [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

@openshift-merge-bot openshift-merge-bot bot merged commit 537dc28 into migtools:master May 8, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Merged / Ready for build
Development

Successfully merging this pull request may close these issues.

3 participants