Add oadp setup command with RBAC detection and refactor builders#147
Merged
Joeavaikath merged 6 commits intoMar 6, 2026
Merged
Conversation
Implements issue migtools#141 by adding an `oadp setup` command that automatically detects whether the user has cluster-wide admin permissions and configures the CLI accordingly. Detection strategy: - Attempts to list deployments across all namespaces - If successful and finds openshift-adp-controller-manager → admin mode - If permission denied/forbidden → non-admin mode - If unauthorized (not logged in) → clear error with instructions Key features: - Auto-detects and configures nonadmin setting - Preserves existing config fields (default-nabsl, etc) - Available in both admin and non-admin modes - --force flag to re-run detection - Clear user feedback with next steps Files added: - cmd/setup/setup.go - Main setup command implementation - cmd/setup/detector.go - Permission detection logic Files modified: - cmd/shared/factories.go - Added WriteVeleroClientConfig() and OADPNamespace field - cmd/root.go - Registered setup command, added to allowed commands Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Joseph <jvaikath@redhat.com>
Renamed builder constructors from For* pattern to New*Builder pattern to align with Go best practices where constructors should start with "New". Changes: - ForNonAdminBackup → NewNonAdminBackupBuilder - ForNonAdminRestore → NewNonAdminRestoreBuilder - Renamed parameter ns → namespace for clarity Updated all usages in create.go files and example documentation. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Joseph <jvaikath@redhat.com>
Mention that the OADP controller deployment is not accessible for non-admin users. Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as "velero setup"
participant OC as "oc auth can-i"
participant Config as "client config file"
User->>CLI: run `velero setup` (optional --force)
CLI->>OC: execute `oc auth can-i create backups.velero.io --all-namespaces`
alt OC returns "yes"
OC-->>CLI: "yes"
CLI->>CLI: DetectionResult{IsAdmin:true}
else OC returns "no"
OC-->>CLI: "no"
CLI->>CLI: DetectionResult{IsAdmin:false}
else OC errors (not logged in / forbidden)
OC-->>CLI: error
CLI->>CLI: DetectionResult{Error:...}
end
CLI->>Config: Read existing client config
Config-->>CLI: existing config map
CLI->>CLI: merge detection into config
CLI->>Config: Write updated config (WriteVeleroClientConfig)
Config-->>CLI: save confirmation
CLI-->>User: print setup summary and guidance
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/non-admin/backup/nonadminbackup_builder.go (1)
29-29:⚠️ Potential issue | 🟡 MinorExample comment references old constructor name.
The example usage in the comment block still uses
builder.ForNonAdminBackupbut the constructor was renamed toNewNonAdminBackupBuilder.📝 Proposed fix
-var nonAdminBackup = builder.ForNonAdminBackup("user-namespace", "backup-1"). +var nonAdminBackup = NewNonAdminBackupBuilder("user-namespace", "backup-1").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/non-admin/backup/nonadminbackup_builder.go` at line 29, The example usage still calls the old constructor name builder.ForNonAdminBackup; update it to the new constructor NewNonAdminBackupBuilder (e.g. replace builder.ForNonAdminBackup("user-namespace", "backup-1") with builder.NewNonAdminBackupBuilder("user-namespace", "backup-1")) and search for any other references to ForNonAdminBackup in nonadminbackup_builder.go to rename them to NewNonAdminBackupBuilder accordingly so the example matches the current API.
🧹 Nitpick comments (2)
cmd/shared/factories.go (1)
138-138: Consider using more restrictive file permissions.The config file is written with
0644permissions, making it world-readable. Since this is a user-specific configuration file that may contain cluster context information, consider using0600to restrict access to the owner only.🔒 Proposed fix
- if err := os.WriteFile(configPath, data, 0644); err != nil { + if err := os.WriteFile(configPath, data, 0600); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/shared/factories.go` at line 138, The config file is created with world-readable mode 0644; update the write in cmd/shared/factories.go (the os.WriteFile call that writes to configPath) to use a more restrictive permission (0600) so only the owner can read/write the file; ensure any tests or callers that expect 0644 are adjusted and that this change is applied where configPath is written (the os.WriteFile(..., data, 0644) invocation).cmd/setup/detector.go (1)
61-61: Consider extracting the deployment name as a constant.The hardcoded string
"openshift-adp-controller-manager"is used to identify the OADP controller. Consider extracting this to a package-level constant for maintainability.♻️ Proposed refactor
+const oadpControllerDeploymentName = "openshift-adp-controller-manager" + // detectUserMode detects whether the user has admin permissions and finds the OADP namespace. ... for _, deployment := range deployments.Items { - if deployment.Name == "openshift-adp-controller-manager" { + if deployment.Name == oadpControllerDeploymentName {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/setup/detector.go` at line 61, Extract the hardcoded deployment name "openshift-adp-controller-manager" to a package-level constant (e.g., const oadpControllerDeployment = "openshift-adp-controller-manager") and replace the literal comparison in the code that checks deployment.Name (the if statement in detector.go) with a comparison to that constant; add the constant near the top of the file so any other uses in the package can reference it for maintainability and update tests/usages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/setup/setup.go`:
- Around line 163-164: Capture and handle the error returned by os.UserHomeDir()
instead of ignoring it: change the assignment to homeDir, err :=
os.UserHomeDir() and if err != nil set a safe fallback (e.g. homeDir = "~" or ""
), and optionally log or return the error; then build configPath using the
validated homeDir variable. This touches the homeDir and configPath usage in
cmd/setup/setup.go.
---
Outside diff comments:
In `@cmd/non-admin/backup/nonadminbackup_builder.go`:
- Line 29: The example usage still calls the old constructor name
builder.ForNonAdminBackup; update it to the new constructor
NewNonAdminBackupBuilder (e.g. replace
builder.ForNonAdminBackup("user-namespace", "backup-1") with
builder.NewNonAdminBackupBuilder("user-namespace", "backup-1")) and search for
any other references to ForNonAdminBackup in nonadminbackup_builder.go to rename
them to NewNonAdminBackupBuilder accordingly so the example matches the current
API.
---
Nitpick comments:
In `@cmd/setup/detector.go`:
- Line 61: Extract the hardcoded deployment name
"openshift-adp-controller-manager" to a package-level constant (e.g., const
oadpControllerDeployment = "openshift-adp-controller-manager") and replace the
literal comparison in the code that checks deployment.Name (the if statement in
detector.go) with a comparison to that constant; add the constant near the top
of the file so any other uses in the package can reference it for
maintainability and update tests/usages accordingly.
In `@cmd/shared/factories.go`:
- Line 138: The config file is created with world-readable mode 0644; update the
write in cmd/shared/factories.go (the os.WriteFile call that writes to
configPath) to use a more restrictive permission (0600) so only the owner can
read/write the file; ensure any tests or callers that expect 0644 are adjusted
and that this change is applied where configPath is written (the
os.WriteFile(..., data, 0644) invocation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d73688be-c7df-4cfe-a7af-9da7422ada53
📒 Files selected for processing (8)
cmd/non-admin/backup/create.gocmd/non-admin/backup/nonadminbackup_builder.gocmd/non-admin/restore/create.gocmd/non-admin/restore/nonadminrestore_builder.gocmd/root.gocmd/setup/detector.gocmd/setup/setup.gocmd/shared/factories.go
Contributor
non-admin check |
kaovilai
reviewed
Mar 5, 2026
Detect if user has admin permissions by checking if they can create Velero Backup resources across all namespaces. Signed-off-by: Joseph <jvaikath@redhat.com>
Joeavaikath
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cmd/setup/setup.go (1)
130-146:⚠️ Potential issue | 🟡 MinorHandle
os.UserHomeDir()failures in both output helpers.Both helpers ignore the error and can print an empty or misleading config path. Please use a safe fallback like
~/.config/velero/config.jsonor return the error from the caller.📝 Suggested fix
+func configFileDisplayPath() string { + homeDir, err := os.UserHomeDir() + if err != nil { + return "~/.config/velero/config.json" + } + return filepath.Join(homeDir, ".config", "velero", "config.json") +} + // printCurrentConfig prints the current configuration func (o *SetupOptions) printCurrentConfig(config *shared.ClientConfig) { - homeDir, _ := os.UserHomeDir() - configPath := filepath.Join(homeDir, ".config", "velero", "config.json") + configPath := configFileDisplayPath() if config.IsNonAdmin() { fmt.Println("Current mode: non-admin") @@ // printSetupSuccess prints a success message after setup func (o *SetupOptions) printSetupSuccess() { - homeDir, _ := os.UserHomeDir() - configPath := filepath.Join(homeDir, ".config", "velero", "config.json") + configPath := configFileDisplayPath() if o.detectionResult.IsAdmin { fmt.Println("✓ Admin mode enabled")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/setup/setup.go` around lines 130 - 146, Both printCurrentConfig and printSetupSuccess call os.UserHomeDir() and ignore its error, which can lead to an empty/misleading config path; update these functions (printCurrentConfig and printSetupSuccess) to check the error return from os.UserHomeDir() and on failure use a safe fallback path ("~/.config/velero/config.json") or propagate the error to the caller (choose consistent behavior across both helpers), then build configPath using the resolved home (or the fallback) before printing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/setup/setup.go`:
- Around line 112-120: The code updates only config.NonAdmin but never persists
the detected namespace; copy the detected namespace from o.detectionResult (e.g.
o.detectionResult.Namespace) into the config appropriate field (e.g.
config.Namespace or config.OadpNamespace) before calling
shared.WriteVeleroClientConfig(config), and only assign it when the detected
namespace is non-empty so the CLI points to the controller namespace discovered
by detectUserMode().
In `@cmd/shared/factories.go`:
- Around line 116-141: WriteVeleroClientConfig currently overwrites
~/.config/velero/config.json and discards other Velero keys; change it to read
the existing config (os.ReadFile + json.Unmarshal into a
map[string]interface{}), merge only the keys your command owns (map keys
"namespace", "nonadmin", "default-nabsl" populated from the ClientConfig
instance) into that map, and then json.MarshalIndent and write the merged map
back to configPath (creating dirs if needed). Keep existing file
permissions/formatting behavior, handle the case where the file does not exist
by starting from an empty map, and preserve error handling for os.UserHomeDir,
os.MkdirAll, json marshal/unmarshal, and os.WriteFile; update function
references ClientConfig, WriteVeleroClientConfig, and configPath accordingly.
---
Duplicate comments:
In `@cmd/setup/setup.go`:
- Around line 130-146: Both printCurrentConfig and printSetupSuccess call
os.UserHomeDir() and ignore its error, which can lead to an empty/misleading
config path; update these functions (printCurrentConfig and printSetupSuccess)
to check the error return from os.UserHomeDir() and on failure use a safe
fallback path ("~/.config/velero/config.json") or propagate the error to the
caller (choose consistent behavior across both helpers), then build configPath
using the resolved home (or the fallback) before printing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c2ab871-1a41-4531-b22f-fd81d488b017
📒 Files selected for processing (3)
cmd/setup/detector.gocmd/setup/setup.gocmd/shared/factories.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/setup/detector.go
kaovilai
reviewed
Mar 6, 2026
kaovilai
reviewed
Mar 6, 2026
Signed-off-by: Joseph <jvaikath@redhat.com>
kaovilai
previously approved these changes
Mar 6, 2026
Member
kaovilai
left a comment
kaovilai
left a comment
There was a problem hiding this comment.
left some thoughts but works as is.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/shared/factories.go`:
- Around line 144-159: The mergeClientConfig function currently unconditionally
sets configMap["namespace"] = config.Namespace which can overwrite an existing
namespace with an empty value; update mergeClientConfig to handle "namespace"
consistently with "nonadmin" and "default-nabsl": check config.Namespace (via
ClientConfig.Namespace) and only set configMap["namespace"] when it is
non-empty, otherwise delete(configMap, "namespace") so the existing namespace is
preserved when the input is empty.
kaovilai
approved these changes
Mar 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why the changes were made
Implements issue #141 by adding an
oadp setupcommand that auto-detects whether users have cluster-wide admin permissions and configures the CLI accordingly. Previously, users had to manually runoc oadp client config set nonadmin=true, which was confusing and error-prone.The detection uses RBAC permission checks (
oc auth can-i create backups.velero.io --all-namespaces) to determine mode. Admin users can create Velero Backup resources cluster-wide, while non-admin users can only create NonAdminBackup resources in their namespace. This directly tests the actual permission difference rather than checking infrastructure visibility.Also includes builder constructor refactoring to follow Go naming conventions (
ForNonAdminBackup→NewNonAdminBackupBuilder,ForNonAdminRestore→NewNonAdminRestoreBuilder) and adds default storage location configuration support for non-admin backups.How to test the changes made
Build:
Test setup command with admin permissions:
Test with non-admin permissions:
Test when not logged in:
Test config preservation:
Verify builder refactoring:
Summary by CodeRabbit
Release Notes
setupcommand that automatically detects admin vs non-admin user mode.--forceflag option to override existing settings.