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

Allow Non-Admin user to create Backup Storage Locations - fixes #36 #115

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mpryc
Copy link
Collaborator

@mpryc mpryc commented Nov 18, 2024

Why the changes were made

Fix #36, also #57

How to test the changes made

  1. Create as usual OADP with NAC.
  2. Create user secret in the user's namespace similarly to the S3 secret for DPA:
$ oc create secret generic nac-credentials -n <USERS_NAMESPACE> --from-file cloud=credentials-velero
  1. Create Non Admin BSL pointing to the above credential in the user's namespace, example spec part:
apiVersion: oadp.openshift.io/v1alpha1
kind: NonAdminBackupStorageLocation
metadata:
  name: example
  namespace: nactest
spec:
  config:
    region: eu-central-1
  credential:
    key: cloud
    name: nac-credentials
  objectStorage:
    bucket: my-bucket
    prefix: some-prefix
  provider: aws
  1. Check the NAC logs, the NaBSL object (if it was correctly reconciled and updated with status from Velero BSL)

5...6...7... Do break things, such as improper Secret data, non-existing secret, removal of the secret from the OADP namespace or the user namespace, removal of the BSL from the OADP namespace and see if everything reconciles as expected.

Read design

Copy link

openshift-ci bot commented Nov 18, 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

Copy link

openshift-ci bot commented Nov 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mpryc

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mpryc
Copy link
Collaborator Author

mpryc commented Nov 18, 2024

@mateusoliveira43 @shubham-pampattiwar @weshayutin enjoy playing with it, comments welcome.
Note that tests will be implemented after we agree on the current implementation. This part is for creation/deletion of BSL. Actual use of the BSL from the Non Admin Backup should be in separate PR as it will require some changes in the non-admin Backup reconcile to ensure we are safe-guarding against usage from BSL's which are not allowed for the user.

mpryc added 4 commits December 3, 2024 13:13
The scaffold was generated using operator-sdk version 1.37.0:

$ operator-sdk create api --version v1alpha1 --kind NonAdminBackupStorageLocation  --group oadp

Signed-off-by: Michal Pryc <[email protected]>
Second step to generate scaffold folder for the NABSL
controller with the $ make manifests command

Used controller-gen-v0.14.0 for the above.

Signed-off-by: Michal Pryc <[email protected]>
Signed-off-by: Michal Pryc <[email protected]>
@mpryc mpryc force-pushed the nonadminstoragelocation_controller branch from 5464b98 to 306fe37 Compare December 3, 2024 12:49
@shawn-hurley
Copy link

What happens if the BSL already exists in the velero namespace?

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!
Copy link
Contributor

Choose a reason for hiding this comment

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

remove scaffolded comments

@@ -30,8 +30,6 @@ 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.)
Copy link
Contributor

Choose a reason for hiding this comment

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

undo

@@ -16,3 +16,9 @@ resources:
# - auth_proxy_role.yaml
# - auth_proxy_role_binding.yaml
# - auth_proxy_client_clusterrole.yaml
# For each CRD, "Editor" and "Viewer" roles are scaffolded by
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The scaffold was using operator-sdk with the following versions:

# operator-sdk version
operator-sdk version: "v1.37.0", commit: "819984d4c1a51c8ff2ef6c23944554148ace0752", kubernetes version: "1.29.0", go version: "go1.21.13", GOOS: "linux", GOARCH: "amd64"

# kubebuilder version
Version: main.version{KubeBuilderVersion:"4.2.0", KubernetesVendor:"1.31.0", GitCommit:"c7cde5172dc8271267dbf2899e65ef6f9d30f91e", BuildDate:"2024-08-17T09:41:45Z", GoOs:"linux", GoArch:"amd64"}

See the commit message:
6d9db57

Copy link
Contributor

Choose a reason for hiding this comment

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

this project does not use operator-sdk

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The generated api is same as under the hood operator-sdk calls kubebuilder, question is if I should use older kubebuilder version instead of newer one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am currently rebasing this on top of kubebuilder generated scaffold with the same version 1.34.0. Maybe it makes sense to use same version, so later we can upgrade to newer one from same base.

Copy link
Contributor

@mateusoliveira43 mateusoliveira43 left a comment

Choose a reason for hiding this comment

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

@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

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.

Ability for non-admin users to create BackupStorageLocations(BSL)
4 participants