OADP-7660: Document NonAdminBSL requires long-term credentials#334
OADP-7660: Document NonAdminBSL requires long-term credentials#334kaovilai wants to merge 1 commit intomigtools:oadp-devfrom
Conversation
Document that NonAdminBSL must use long-term cloud credentials stored in the non-admin user's namespace. Explain why short-lived credentials (AWS STS, GCP WIF, Azure WI) are not supported: the projected SA token belongs to the admin Velero identity, there is no per-namespace cloud identity scoping, and credential files are copied without format validation. Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: Happy <yesreply@happy.engineering> Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
|
Skipping CI for Draft Pull Request. |
|
@kaovilai: This pull request references OADP-7660 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughA new documentation page is added that specifies credential requirements for NonAdminBackupStorageLocation (NonAdminBSL), explaining why short-lived credential mechanisms are unsupported, detailing the credential flow, required formats for AWS/GCP/Azure, and providing security recommendations. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Pull request overview
Adds explicit documentation clarifying that NonAdminBackupStorageLocation (NonAdminBSL) currently requires long-term, per-namespace cloud credentials and that short-lived identity mechanisms are unsupported due to how NAC syncs credentials into Velero.
Changes:
- Introduces a new doc describing current NonAdminBSL credential flow (Secret sync + Velero credential file usage).
- Documents why AWS STS / GCP WIF / Azure Workload Identity are not supported for NonAdminBSL today.
- Provides example Secret formats for AWS/GCP/Azure and admin security recommendations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ### NonAdminBSL referencing the Secret | ||
|
|
||
| ```yaml | ||
| apiVersion: nac.oadp.openshift.io/v1alpha1 |
There was a problem hiding this comment.
The apiVersion in the NonAdminBackupStorageLocation example is incorrect for this repo. CRDs and samples use the oadp.openshift.io/v1alpha1 API group (not nac.oadp.openshift.io/v1alpha1), so the manifest as written won’t apply.
| apiVersion: nac.oadp.openshift.io/v1alpha1 | |
| apiVersion: oadp.openshift.io/v1alpha1 |
| # NonAdminBSL Credential Requirements: Long-Term Credentials Only | ||
|
|
||
| _Jira: [OADP-7660](https://redhat.atlassian.net/browse/OADP-7660)_ | ||
|
|
||
| ## Overview | ||
|
|
||
| NonAdminBackupStorageLocation (NonAdminBSL) requires non-admin users to provide long-term cloud credentials (access keys, service account JSON keys, client secrets) stored in a Secret in their own namespace. | ||
| Short-lived, cloud-native credential mechanisms (AWS STS, GCP Workload Identity Federation, Azure Workload Identity) are **not supported** for NonAdminBSL and **must not be used**. | ||
|
|
||
| ## How NonAdminBSL Credentials Work Today |
There was a problem hiding this comment.
This doc consistently uses the acronym “NonAdminBSL”, but the rest of the repo’s docs and user-facing naming tends to use “NABSL” (matching the CRD shortName nabsl). Consider standardizing on “NABSL”, or explicitly stating both (e.g., “Non-Admin BackupStorageLocation (NABSL, also referred to as NonAdminBSL in code)”) to avoid confusing readers.
| _Jira: [OADP-7660](https://redhat.atlassian.net/browse/OADP-7660)_ | ||
|
|
||
| ## Overview | ||
|
|
There was a problem hiding this comment.
This new doc doesn’t appear to be linked from README.md or other docs pages, so it’s easy to miss. Consider adding a cross-link from an existing entry point (e.g., README “Using NAC” / NABSL section or docs/non_admin_user.md) so users can discover these credential constraints.
| For general non-admin usage and workflow, see the [Non-admin user guide](./non_admin_user.md). This page focuses specifically on credential constraints for NonAdminBSL. |
|
@kaovilai: This pull request references OADP-7660 which is a valid jira issue. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
![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
🧹 Nitpick comments (1)
docs/nonadminbsl_credential_requirements.md (1)
84-103: Consider noting that the GCP example shows core fields only.The GCP service account JSON example includes the essential fields but omits some optional fields commonly present in full service account key files (e.g.,
auth_provider_x509_cert_url,client_x509_cert_url). While the example is sufficient for demonstration purposes, you might consider adding a note that actual GCP service account JSON files may contain additional fields.📝 Optional clarification
Add a note after line 103:
"token_uri": "https://oauth2.googleapis.com/token" } + +_Note: Actual GCP service account JSON files may include additional fields such as `auth_provider_x509_cert_url` and `client_x509_cert_url`._🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/nonadminbsl_credential_requirements.md` around lines 84 - 103, The GCP service account JSON example under the YAML block (the stringData: cloud secret example) shows only core fields; add a brief note immediately after that snippet stating this is a minimal example and actual GCP service account key JSON files often include additional fields (e.g., auth_provider_x509_cert_url, client_x509_cert_url and other metadata) and that those extra fields are acceptable when using the secret.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/nonadminbsl_credential_requirements.md`:
- Around line 10-15: Update the GitHub permalink range in
docs/nonadminbsl_credential_requirements.md for the NonAdminBSL controller
reference: replace the existing `#L736-L841` range with `#L736-L849` so the link
covers the full syncSecrets function in the NonAdminBSL controller (syncSecrets)
reference; leave the Velero FileStore.Path() reference unchanged.
---
Nitpick comments:
In `@docs/nonadminbsl_credential_requirements.md`:
- Around line 84-103: The GCP service account JSON example under the YAML block
(the stringData: cloud secret example) shows only core fields; add a brief note
immediately after that snippet stating this is a minimal example and actual GCP
service account key JSON files often include additional fields (e.g.,
auth_provider_x509_cert_url, client_x509_cert_url and other metadata) and that
those extra fields are acceptable when using the secret.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd3c6e79-ec05-4518-9f5f-f3eb7efb72ee
📒 Files selected for processing (1)
docs/nonadminbsl_credential_requirements.md
| ## How NonAdminBSL Credentials Work Today | ||
|
|
||
| The non-admin user creates a Secret in their namespace containing cloud credentials. | ||
| The [NonAdminBSL controller](https://github.com/migtools/oadp-non-admin/blob/31a3bfc20310898fae5a438005d0d242240d0fbb/internal/controller/nonadminbackupstoragelocation_controller.go#L736-L841) copies this Secret into the OADP namespace and references the copy in the Velero BSL's `spec.credential` field. | ||
| Velero's [`FileStore.Path()`](https://github.com/vmware-tanzu/velero/blob/6e91e72e655568dd6944ca7bb3cf00b6c7fbb3c8/internal/credentials/file_store.go#L66-L88) writes the secret data to a temporary file on disk and passes the file path to the cloud plugin via `config["credentialsFile"]`. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify GitHub permalink URLs return 200 and check if line ranges are valid
echo "Checking GitHub URL accessibility..."
# Check NonAdminBSL controller syncSecrets reference
url1="https://github.com/migtools/oadp-non-admin/blob/31a3bfc20310898fae5a438005d0d242240d0fbb/internal/controller/nonadminbackupstoragelocation_controller.go#L736-L841"
echo "URL 1: $url1"
curl -s -o /dev/null -w "HTTP Status: %{http_code}\n" -L "$url1"
# Check Velero FileStore.Path() reference
url2="https://github.com/vmware-tanzu/velero/blob/6e91e72e655568dd6944ca7bb3cf00b6c7fbb3c8/internal/credentials/file_store.go#L66-L88"
echo "URL 2: $url2"
curl -s -o /dev/null -w "HTTP Status: %{http_code}\n" -L "$url2"
# Check Azure credential reference
url3="https://github.com/vmware-tanzu/velero/blob/6e91e72e655568dd6944ca7bb3cf00b6c7fbb3c8/pkg/util/azure/credential.go#L48-L54"
echo "URL 3: $url3"
curl -s -o /dev/null -w "HTTP Status: %{http_code}\n" -L "$url3"Repository: migtools/oadp-non-admin
Length of output: 588
🏁 Script executed:
#!/bin/bash
# Verify line ranges are correct by fetching raw file content
echo "=== Checking NonAdminBSL controller.go ==="
raw_url1="https://raw.githubusercontent.com/migtools/oadp-non-admin/31a3bfc20310898fae5a438005d0d242240d0fbb/internal/controller/nonadminbackupstoragelocation_controller.go"
curl -s "$raw_url1" | sed -n '735,850p' | head -20
echo ""
echo "=== Checking Velero FileStore.Path() ==="
raw_url2="https://raw.githubusercontent.com/vmware-tanzu/velero/6e91e72e655568dd6944ca7bb3cf00b6c7fbb3c8/internal/credentials/file_store.go"
curl -s "$raw_url2" | sed -n '60,95p'
echo ""
echo "=== Line count check for controller.go ==="
curl -s "$raw_url1" | wc -l
echo ""
echo "=== Checking context around line 736-841 in controller.go ==="
curl -s "$raw_url1" | sed -n '735p;736p;737p;840p;841p;842p'Repository: migtools/oadp-non-admin
Length of output: 2711
🏁 Script executed:
#!/bin/bash
# Find the actual end of syncSecrets function
raw_url="https://raw.githubusercontent.com/migtools/oadp-non-admin/31a3bfc20310898fae5a438005d0d242240d0fbb/internal/controller/nonadminbackupstoragelocation_controller.go"
# Get the file and find where syncSecrets starts
echo "=== Finding syncSecrets function boundaries ==="
curl -s "$raw_url" | awk '
/^func \(r \*NonAdminBackupStorageLocationReconciler\) syncSecrets/ {
start = NR
depth = 0
}
start && NR >= start {
for (i = 1; i <= length($0); i++) {
c = substr($0, i, 1)
if (c == "{") depth++
if (c == "}") depth--
}
if (depth == 0 && NR > start) {
print "Function starts at line: " start
print "Function ends at line: " NR
exit
}
}
'
echo ""
echo "=== Lines around 841 ==="
curl -s "$raw_url" | sed -n '838,845p' | cat -n
echo ""
echo "=== Lines around 850 ==="
curl -s "$raw_url" | sed -n '847,856p' | cat -nRepository: migtools/oadp-non-admin
Length of output: 1041
Correct the GitHub line range for the NonAdminBSL controller reference.
The syncSecrets function in the NonAdminBSL controller actually extends to line 849, not line 841. Update the GitHub permalink from #L736-L841 to #L736-L849 to capture the complete function. The Velero FileStore.Path() reference is accurate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/nonadminbsl_credential_requirements.md` around lines 10 - 15, Update the
GitHub permalink range in docs/nonadminbsl_credential_requirements.md for the
NonAdminBSL controller reference: replace the existing `#L736-L841` range with
`#L736-L849` so the link covers the full syncSecrets function in the NonAdminBSL
controller (syncSecrets) reference; leave the Velero FileStore.Path() reference
unchanged.
Summary
Related
Note
Responses generated with Claude
Generated with Claude Code
via Happy
Summary by CodeRabbit
Release Notes