Fix OADP-7562: handle wildcard in scoped excluded cluster resources#333
Fix OADP-7562: handle wildcard in scoped excluded cluster resources#333mpryc wants to merge 1 commit intomigtools:oadp-devfrom
Conversation
|
@mpryc: This pull request references OADP-7562 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: mpryc 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 |
|
@mpryc: This pull request references OADP-7562 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. |
✅ 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. |
|
/cherry-pick oadp-1.6 |
📝 WalkthroughWalkthroughControllers now avoid appending controller-defined default exclusions when the exclusion list contains the wildcard Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
@mpryc: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
/hold until it's confirmed to fix issue. |
|
@mpryc: This pull request references OADP-7562 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)
internal/controller/nonadminbackup_controller_test.go (1)
1549-1586: Good test coverage for the wildcard bug fix.The test correctly validates that:
ExcludedClusterScopedResources: ["*"]is preserved without appending defaultsExcludedNamespaceScopedResources(without wildcard) still gets the default NAC resources appendedFor completeness, consider adding additional test entries:
ExcludedNamespaceScopedResources: ["*"]scenario (to verify the symmetric case)- Both exclusion lists containing
"*"(to verify no defaults are appended to either)💡 Additional test entry suggestion
ginkgo.Entry("Should not append default excluded resources when wildcard is used in excludedNamespaceScopedResources", nonAdminBackupFullReconcileScenario{ spec: nacv1alpha1.NonAdminBackupSpec{ BackupSpec: &velerov1.BackupSpec{ ExcludedNamespaceScopedResources: []string{"*"}, IncludedClusterScopedResources: []string{"persistentvolumes"}, }, }, status: nacv1alpha1.NonAdminBackupStatus{ Phase: nacv1alpha1.NonAdminPhaseCreated, VeleroBackup: &nacv1alpha1.VeleroBackup{ Namespace: oadpNamespace, Status: nil, Spec: &velerov1.BackupSpec{ ExcludedNamespaceScopedResources: []string{"*"}, IncludedClusterScopedResources: []string{"persistentvolumes"}, ExcludedClusterScopedResources: []string{ "securitycontextconstraints", "clusterroles", "clusterrolebindings", "priorityclasses", "customresourcedefinitions", "virtualmachineclusterinstancetypes", "virtualmachineclusterpreferences", }, }, }, Conditions: []metav1.Condition{ { Type: "Accepted", Status: metav1.ConditionTrue, Reason: "BackupAccepted", Message: "backup accepted", }, { Type: "Queued", Status: metav1.ConditionTrue, Reason: "BackupScheduled", Message: "Created Velero Backup object", }, }, }, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/nonadminbackup_controller_test.go` around lines 1549 - 1586, Add two additional ginkgo.Entry test cases to nonAdminbackup_controller_test.go using the same nonAdminBackupFullReconcileScenario pattern: one where BackupSpec.ExcludedNamespaceScopedResources == []string{"*"} to verify defaults are not appended to namespace-scoped exclusions, and another where both ExcludedNamespaceScopedResources and ExcludedClusterScopedResources are []string{"*"} to verify no defaults are appended to either; ensure each entry populates the top-level spec.BackupSpec and the nested status.VeleroBackup.Spec fields (referencing ExcludedNamespaceScopedResources and ExcludedClusterScopedResources) and includes the same Conditions and Phase as the existing entries so the test harness (nonAdminBackupFullReconcileScenario, VeleroBackup.Spec) validates the symmetric wildcard behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/controller/nonadminbackup_controller.go`:
- Around line 709-719: The fallback path unconditionally appends to old-style
ExcludedResources which breaks validation when a user has ExcludedResources:
["*"]; update the logic in the non-admin backup and restore controllers to check
for the wildcard before appending: in the backup flow, guard the two append
calls with if !slices.Contains(backupSpec.ExcludedResources, "*") (targeting the
block that currently appends
alwaysExcludedNamespacedResources/alwaysExcludedClusterResources when scoped
fields are absent), and in the restore flow guard the append with if
!slices.Contains(restoreSpec.ExcludedResources, "*") so nothing is appended when
the wildcard already excludes everything.
---
Nitpick comments:
In `@internal/controller/nonadminbackup_controller_test.go`:
- Around line 1549-1586: Add two additional ginkgo.Entry test cases to
nonAdminbackup_controller_test.go using the same
nonAdminBackupFullReconcileScenario pattern: one where
BackupSpec.ExcludedNamespaceScopedResources == []string{"*"} to verify defaults
are not appended to namespace-scoped exclusions, and another where both
ExcludedNamespaceScopedResources and ExcludedClusterScopedResources are
[]string{"*"} to verify no defaults are appended to either; ensure each entry
populates the top-level spec.BackupSpec and the nested status.VeleroBackup.Spec
fields (referencing ExcludedNamespaceScopedResources and
ExcludedClusterScopedResources) and includes the same Conditions and Phase as
the existing entries so the test harness (nonAdminBackupFullReconcileScenario,
VeleroBackup.Spec) validates the symmetric wildcard behavior.
🪄 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: c38373e3-5363-4f87-921c-73088b1d6efd
📒 Files selected for processing (2)
internal/controller/nonadminbackup_controller.gointernal/controller/nonadminbackup_controller_test.go
| // Skip appending when wildcard '*' is already present, as it already | ||
| // excludes everything and mixing '*' with specific items causes Velero | ||
| // FailedValidation. | ||
| if !slices.Contains(backupSpec.ExcludedNamespaceScopedResources, "*") { | ||
| backupSpec.ExcludedNamespaceScopedResources = append(backupSpec.ExcludedNamespaceScopedResources, | ||
| alwaysExcludedNamespacedResources...) | ||
| } | ||
| if !slices.Contains(backupSpec.ExcludedClusterScopedResources, "*") { | ||
| backupSpec.ExcludedClusterScopedResources = append(backupSpec.ExcludedClusterScopedResources, | ||
| alwaysExcludedClusterResources...) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if the old-style ExcludedResources field also supports wildcards in Velero
# and find all similar append patterns that might need the same fix
# Find other places where resources are appended to exclusion lists without wildcard check
rg -n -A3 -B3 'ExcludedResources\s*=\s*append' --type=go
# Check Velero documentation/validation for wildcard handling in ExcludedResources
rg -n 'ExcludedResources.*\*' --type=goRepository: migtools/oadp-non-admin
Length of output: 1363
🏁 Script executed:
# Get context around the fallback path in backup controller
sed -n '700,730p' internal/controller/nonadminbackup_controller.goRepository: migtools/oadp-non-admin
Length of output: 1556
🏁 Script executed:
# Get context around the restore controller append
sed -n '340,360p' internal/controller/nonadminrestore_controller.goRepository: migtools/oadp-non-admin
Length of output: 873
🏁 Script executed:
# Check if there are any tests or comments that mention ExcludedResources wildcard behavior
rg -i 'excludedresources.*wildcard|wildcard.*excludedresources' --type=go -C 3Repository: migtools/oadp-non-admin
Length of output: 49
🏁 Script executed:
# Search for ExcludedResources field definitions or usage patterns to understand its purpose
rg -n 'ExcludedResources' --type=go | head -20Repository: migtools/oadp-non-admin
Length of output: 1038
🏁 Script executed:
# Find the definition of alwaysExcludedNamespacedResources and alwaysExcludedClusterResources
rg -n 'alwaysExcluded(Namespaced|Cluster)Resources\s*[=:]' --type=go -A 10Repository: migtools/oadp-non-admin
Length of output: 1312
🏁 Script executed:
# Check if there are any constants or variables that might contain wildcards
rg -n 'alwaysExcluded' --type=goRepository: migtools/oadp-non-admin
Length of output: 637
🏁 Script executed:
# Look for Velero API types to understand ExcludedResources field
rg -n 'type.*Spec.*struct' --type=go | grep -i backup | head -5Repository: migtools/oadp-non-admin
Length of output: 359
🏁 Script executed:
# Check if there are any Velero imports or comments that explain ExcludedResources behavior
rg -n 'velerov1\.(Backup|Restore)' --type=go -B 2 -A 2 | head -40Repository: migtools/oadp-non-admin
Length of output: 3473
🏁 Script executed:
# Look for test cases that show ExcludedResources behavior with wildcards
rg -n 'ExcludedResources.*\*|"\*".*ExcludedResources' --type=go -B 2 -A 2Repository: migtools/oadp-non-admin
Length of output: 49
🏁 Script executed:
# Check if the code has any other wildcard validation patterns
rg -n 'slices.Contains.*\*' --type=go -B 2 -A 2Repository: migtools/oadp-non-admin
Length of output: 1078
🏁 Script executed:
# Verify the actual risk: check if user can set ExcludedResources to "*"
rg -n 'ExcludedResources\s*:' --type=go | grep -v '//'Repository: migtools/oadp-non-admin
Length of output: 254
🏁 Script executed:
# Check the actual BackupSpec and RestoreSpec type definitions to confirm ExcludedResources is user-provided
fd -t f -e go -x grep -l 'type.*BackupSpec\|type.*RestoreSpec' {} \; 2>/dev/null | head -10Repository: migtools/oadp-non-admin
Length of output: 197
🏁 Script executed:
# Verify if there are any tests that show user setting ExcludedResources with "*"
rg -n 'ExcludedResources.*=.*\[\]string' --type=go -A 3 | head -30Repository: migtools/oadp-non-admin
Length of output: 889
🏁 Script executed:
# Check NonAdminBackupSpec to see if it embeds BackupSpec
sed -n '27,100p' api/v1alpha1/nonadminbackup_types.goRepository: migtools/oadp-non-admin
Length of output: 2728
🏁 Script executed:
# Check if ExcludedResources is user-configurable by looking at how it's populated
rg -n 'backupSpec.ExcludedResources\s*=' --type=go -B 3 -A 1Repository: migtools/oadp-non-admin
Length of output: 723
🏁 Script executed:
# Look for where ExcludedResources might be set by users (in API or spec parsing)
rg -n 'ExcludedResources' --type=go | grep -v 'append\|test\|//'Repository: migtools/oadp-non-admin
Length of output: 49
🏁 Script executed:
# Check if there's any existing pattern for checking wildcards in ExcludedResources
rg -n 'Contains.*ExcludedResources' --type=go -B 2 -A 2Repository: migtools/oadp-non-admin
Length of output: 49
🏁 Script executed:
# Check the actual field definitions in Velero's BackupSpec to understand ExcludedResources
# Since we can't access Velero directly, look for any documentation or comments in this repo
rg -n 'ExcludedResources' --type=go -B 5 | grep -E 'type|struct|comment|//' | head -20Repository: migtools/oadp-non-admin
Length of output: 166
🏁 Script executed:
# Look at how the backup is created with ExcludedResources to understand the risk
sed -n '697,730p' internal/controller/nonadminbackup_controller.goRepository: migtools/oadp-non-admin
Length of output: 1645
🏁 Script executed:
# Check if there's any validation or comment about the wildcard behavior being specific to scoped fields
rg -n 'wildcard|Wildcard|\*' --type=go -B 2 -A 2 | grep -A 2 -B 2 'scoped\|Scoped' | head -30Repository: migtools/oadp-non-admin
Length of output: 2519
🏁 Script executed:
# Verify if the test actually shows the wildcard behavior being a Velero validation issue
sed -n '1549,1575p' internal/controller/nonadminbackup_controller_test.goRepository: migtools/oadp-non-admin
Length of output: 1092
🏁 Script executed:
# Check what validation errors Velero throws when mixing wildcards with specific items
# Look for any error handling or validation in the test files
rg -n 'FailedValidation' --type=go -B 3 -A 3Repository: migtools/oadp-non-admin
Length of output: 868
🏁 Script executed:
# Understand the user flow: can a user actually set ExcludedResources to "*" in the old-style path?
# Check if there are any examples in tests showing user setting this
rg -n 'BackupSpec.*{' --type=go -A 20 | grep -A 15 'ExcludedResources'Repository: migtools/oadp-non-admin
Length of output: 156
🏁 Script executed:
# Check if there's any documentation about when ExcludedResources vs scoped resources are used
sed -n '697,710p' internal/controller/nonadminbackup_controller.goRepository: migtools/oadp-non-admin
Length of output: 787
🏁 Script executed:
# Search for any comments or issues related to the old-style path wildcard handling
rg -n 'old-style|fallback' --type=go -B 3 -A 3 | grep -i 'wildcard\|exclude' -B 3 -A 3Repository: migtools/oadp-non-admin
Length of output: 724
🏁 Script executed:
# Check if there are tests for the fallback path with ExcludedResources containing wildcards
rg -n 'Should not append default excluded resources' --type=go -A 30 | grep -B 30 'ExcludedResources'Repository: migtools/oadp-non-admin
Length of output: 49
Add wildcard guard to old-style fallback path and restore controller.
The new-style scoped fields correctly prevent appending specific resources when "*" is present. However, the old-style fallback path (lines 722-725) unconditionally appends resources without checking for "*", which would violate Velero's validation if a user sets ExcludedResources: ["*"]. The same gap exists in nonadminrestore_controller.go (lines 351-352).
Add wildcard checks before appending in both locations:
- Fallback path (lines 722-725): Wrap both appends with
if !slices.Contains(backupSpec.ExcludedResources, "*") - Restore controller (lines 351-352): Wrap the append with
if !slices.Contains(restoreSpec.ExcludedResources, "*")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/nonadminbackup_controller.go` around lines 709 - 719, The
fallback path unconditionally appends to old-style ExcludedResources which
breaks validation when a user has ExcludedResources: ["*"]; update the logic in
the non-admin backup and restore controllers to check for the wildcard before
appending: in the backup flow, guard the two append calls with if
!slices.Contains(backupSpec.ExcludedResources, "*") (targeting the block that
currently appends
alwaysExcludedNamespacedResources/alwaysExcludedClusterResources when scoped
fields are absent), and in the restore flow guard the append with if
!slices.Contains(restoreSpec.ExcludedResources, "*") so nothing is appended when
the wildcard already excludes everything.
|
@mpryc: This pull request references OADP-7562 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/controller/nonadminbackup_controller_test.go (1)
1549-1586: Consider adding a sibling case for namespace wildcard handling.A second table entry for
ExcludedNamespaceScopedResources: []string{"*"}would cover the parallel controller guard and close the remaining branch-level regression gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/nonadminbackup_controller_test.go` around lines 1549 - 1586, Add a sibling ginkgo.Entry to the table-based tests (same nonAdminBackupFullReconcileScenario) that mirrors the existing case using ExcludedClusterScopedResources: []string{"*"} but instead sets the input spec's ExcludedNamespaceScopedResources: []string{"*"} and asserts the resulting VeleroBackup.Spec.ExcludedNamespaceScopedResources remains ["*"] (i.e., defaults are not appended); this will exercise the namespace-wildcard guard in the controller and close the branch-level regression gap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/nonadminbackup_controller_test.go`:
- Around line 1549-1586: Add a sibling ginkgo.Entry to the table-based tests
(same nonAdminBackupFullReconcileScenario) that mirrors the existing case using
ExcludedClusterScopedResources: []string{"*"} but instead sets the input spec's
ExcludedNamespaceScopedResources: []string{"*"} and asserts the resulting
VeleroBackup.Spec.ExcludedNamespaceScopedResources remains ["*"] (i.e., defaults
are not appended); this will exercise the namespace-wildcard guard in the
controller and close the branch-level regression gap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16f5d9fc-4ba2-4880-9887-9b41c8f6fb53
📒 Files selected for processing (3)
internal/controller/nonadminbackup_controller.gointernal/controller/nonadminbackup_controller_test.gointernal/controller/nonadminrestore_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/nonadminbackup_controller.go
|
@mpryc: This pull request references OADP-7562 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/controller/nonadminbackup_controller_test.go (1)
1549-1586: Add companion wildcard coverage for remaining exclusion paths.This entry covers the cluster-scoped wildcard well. Consider adding table entries for
ExcludedNamespaceScopedResources: []string{"*"}and old-styleExcludedResources: []string{"*"}to lock in all wildcard guards introduced in controller logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/nonadminbackup_controller_test.go` around lines 1549 - 1586, Add two new ginkgo.Entry test cases alongside the existing nonAdminBackupFullReconcileScenario entry: one where spec.BackupSpec.ExcludedNamespaceScopedResources = []string{"*"} and the expected status.VeleroBackup.Spec.ExcludedNamespaceScopedResources is also []string{"*"} (and include the same default ExcludedNamespaceScopedResources entries in the controller's created VeleroBackup when applicable), and another legacy-style case where spec.BackupSpec.ExcludedResources = []string{"*"} with the expected status.VeleroBackup.Spec.ExcludedResources = []string{"*"}; use the same nonAdminBackupFullReconcileScenario struct and mirror the Conditions/Phase fields from the existing entry so the test covers wildcard handling for both namespace-scoped and legacy exclusion fields.internal/controller/nonadminrestore_controller.go (1)
352-355: Wildcard guard is correct; add a restore wildcard test to prevent regressions.The guard is correct and aligns with the backup fix. Please add a
NonAdminRestoretest case forrestoreSpec.ExcludedResources = []string{"*"}to ensure"volumesnapshotclasses"is not appended in that path (current expectations aroundinternal/controller/nonadminrestore_controller_test.goLine 364 still only cover non-wildcard behavior).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/nonadminrestore_controller.go` around lines 352 - 355, Add a unit test that covers the wildcard exclusion path so we don't regress the behavior in nonadminrestore_controller.go: when restoreSpec.ExcludedResources is set to []string{"*"} the controller must NOT append "volumesnapshotclasses"; implement a new test case (or extend the existing NonAdminRestore test in nonadminrestore_controller_test) that sets restoreSpec.ExcludedResources = []string{"*"}, invokes the code path that runs the logic around slices.Contains and appending (the block that checks restoreSpec.ExcludedResources and may append "volumesnapshotclasses"), and assert that "volumesnapshotclasses" is not present in the final restoreSpec.ExcludedResources. Ensure the test fails if the append occurs to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/controller/nonadminbackup_controller_test.go`:
- Around line 1549-1586: Add two new ginkgo.Entry test cases alongside the
existing nonAdminBackupFullReconcileScenario entry: one where
spec.BackupSpec.ExcludedNamespaceScopedResources = []string{"*"} and the
expected status.VeleroBackup.Spec.ExcludedNamespaceScopedResources is also
[]string{"*"} (and include the same default ExcludedNamespaceScopedResources
entries in the controller's created VeleroBackup when applicable), and another
legacy-style case where spec.BackupSpec.ExcludedResources = []string{"*"} with
the expected status.VeleroBackup.Spec.ExcludedResources = []string{"*"}; use the
same nonAdminBackupFullReconcileScenario struct and mirror the Conditions/Phase
fields from the existing entry so the test covers wildcard handling for both
namespace-scoped and legacy exclusion fields.
In `@internal/controller/nonadminrestore_controller.go`:
- Around line 352-355: Add a unit test that covers the wildcard exclusion path
so we don't regress the behavior in nonadminrestore_controller.go: when
restoreSpec.ExcludedResources is set to []string{"*"} the controller must NOT
append "volumesnapshotclasses"; implement a new test case (or extend the
existing NonAdminRestore test in nonadminrestore_controller_test) that sets
restoreSpec.ExcludedResources = []string{"*"}, invokes the code path that runs
the logic around slices.Contains and appending (the block that checks
restoreSpec.ExcludedResources and may append "volumesnapshotclasses"), and
assert that "volumesnapshotclasses" is not present in the final
restoreSpec.ExcludedResources. Ensure the test fails if the append occurs to
prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb88cfda-ff88-4f9e-b763-10b6b6c8c842
📒 Files selected for processing (3)
internal/controller/nonadminbackup_controller.gointernal/controller/nonadminbackup_controller_test.gointernal/controller/nonadminrestore_controller.go
When a user sets excludedClusterScopedResources or excludedNamespaceScopedResources to '*', skip appending default resources to avoid Velero FailedValidation. add VolumeGroupSnapshotLabelKey to enforced backup spec tests to allow tests passing. fix lint issues. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Michal Pryc <mpryc@redhat.com>
|
@mpryc: This pull request references OADP-7562 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/controller/nonadminbackupstoragelocation_controller.go (1)
649-653: Silent failure on type assertion could mask bugs.Returning
falsewhen the type assertion fails silently completes without making updates. While this prevents panics, it could hide programming errors where the wrong object type is passed. Consider adding a log statement at debug level when the assertion fails.🔧 Suggestion to add logging on assertion failure
if updateErr := r.updateStatusWithRetry(ctx, logger, nabslRequest, func(obj client.Object) bool { req, ok := obj.(*nacv1alpha1.NonAdminBackupStorageLocationRequest) if !ok { + // This should never happen - log for debugging if it does + logger.V(1).Info("Unexpected: failed to assert object as NonAdminBackupStorageLocationRequest") return false } return updatePhaseIfNeeded(&req.Status.Phase, req.Spec.ApprovalDecision) }); updateErr != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/nonadminbackupstoragelocation_controller.go` around lines 649 - 653, The type assertion for obj to *nacv1alpha1.NonAdminBackupStorageLocationRequest currently returns false silently on failure; add a debug log when !ok to record the unexpected type (e.g., log.Debugf or controllerLogger.V(1).Info) including the actual object type via fmt.Sprintf("%T", obj) and any identifying info, then return false as before. Update the block around the assertion (the req, ok := obj.(*nacv1alpha1.NonAdminBackupStorageLocationRequest) check that then calls updatePhaseIfNeeded) to emit this debug log before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/common/function/function_test.go`:
- Around line 381-385: The test includes a case named
"VolumeGroupSnapshotLabelKey" that references
BackupSpec.VolumeGroupSnapshotLabelKey which does not exist in the vendored
Velero v1.14.0; remove that test case (or replace it with a field present in
v1.14.0) so the completeness check in function_test.go (the reflection-based
validation around lines 460-475) no longer expects the nonexistent field;
alternatively, if you intend to keep the test, upgrade the Velero dependency to
v1.17.0+ so BackupSpec contains VolumeGroupSnapshotLabelKey and the reflection
check will pass.
In `@internal/controller/nonadminrestore_controller.go`:
- Around line 352-355: Add a unit test that exercises the code path which checks
slices.Contains(restoreSpec.ExcludedResources, constant.WildcardString) so we
verify the wildcard case is preserved: create a Restore (or restoreSpec) with
ExcludedResources set to []string{constant.WildcardString}, invoke the
controller logic that runs the exclusion-normalization (the code in
nonadminrestore_controller.go — the Reconcile / processing function that
contains the slices.Contains check), and assert that ExcludedResources still
equals []string{constant.WildcardString} (i.e., "volumesnapshotclasses" is not
appended). Ensure the test uses the same symbols restoreSpec.ExcludedResources
and constant.WildcardString for clarity.
---
Nitpick comments:
In `@internal/controller/nonadminbackupstoragelocation_controller.go`:
- Around line 649-653: The type assertion for obj to
*nacv1alpha1.NonAdminBackupStorageLocationRequest currently returns false
silently on failure; add a debug log when !ok to record the unexpected type
(e.g., log.Debugf or controllerLogger.V(1).Info) including the actual object
type via fmt.Sprintf("%T", obj) and any identifying info, then return false as
before. Update the block around the assertion (the req, ok :=
obj.(*nacv1alpha1.NonAdminBackupStorageLocationRequest) check that then calls
updatePhaseIfNeeded) to emit this debug log before returning.
🪄 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: 26d8ffa8-a72c-4d48-8e49-cecc5e777810
📒 Files selected for processing (6)
internal/common/constant/constant.gointernal/common/function/function_test.gointernal/controller/nonadminbackup_controller.gointernal/controller/nonadminbackup_controller_test.gointernal/controller/nonadminbackupstoragelocation_controller.gointernal/controller/nonadminrestore_controller.go
✅ Files skipped from review due to trivial changes (1)
- internal/common/constant/constant.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/nonadminbackup_controller_test.go
| { | ||
| name: "VolumeGroupSnapshotLabelKey", | ||
| enforcedValue: "enforced-label-key", | ||
| overrideValue: "user-label-key", | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify VolumeGroupSnapshotLabelKey field exists in Velero BackupSpec
# Check go.mod for Velero version
echo "=== Velero version in go.mod ==="
rg -n 'github.com/vmware-tanzu/velero' go.mod
# Search for VolumeGroupSnapshotLabelKey usage in the codebase
echo -e "\n=== VolumeGroupSnapshotLabelKey references ==="
rg -n 'VolumeGroupSnapshotLabelKey'
# Verify the field exists in imported Velero types
echo -e "\n=== BackupSpec struct fields (if vendored) ==="
fd -t f 'backup_types.go' --exec grep -A 100 'type BackupSpec struct' {} \; 2>/dev/null | head -150Repository: migtools/oadp-non-admin
Length of output: 736
🏁 Script executed:
#!/bin/bash
# Check the BackupSpec struct definition in the project
echo "=== Looking for BackupSpec struct in project imports ==="
rg -n 'type BackupSpec struct' --type go
# Check the imports in function_test.go to see which Velero version is being used
echo -e "\n=== Imports in function_test.go ==="
head -50 internal/common/function/function_test.go | grep -A 20 'import'
# Check the completeness check logic at lines 460-475
echo -e "\n=== Completeness check logic ==="
sed -n '455,480p' internal/common/function/function_test.goRepository: migtools/oadp-non-admin
Length of output: 1531
🏁 Script executed:
#!/bin/bash
# Check when VolumeGroupSnapshotLabelKey was added to Velero
echo "=== Searching Velero upstream for VolumeGroupSnapshotLabelKey ==="
# Try to find any Velero types file that might be in vendor or cached
fd -t f -name '*velero*' -type f | grep -E '(types|backup)' | head -20
# Check if there's a vendor directory with Velero types
echo -e "\n=== Checking for vendored Velero types ==="
find . -path '*/velero/pkg/apis/velero/v1/*' -name '*.go' 2>/dev/null | head -10
# Look for any BackupSpec references that show actual field names
echo -e "\n=== Looking for BackupSpec field references in YAML/config ==="
rg -A 50 'type.*BackupSpec' --type go 2>/dev/null | head -100Repository: migtools/oadp-non-admin
Length of output: 4242
🌐 Web query:
Velero VolumeGroupSnapshotLabelKey release version when added
💡 Result:
The VolumeGroupSnapshotLabelKey feature, which allows customizing the label key used by Velero to group PVCs for VolumeGroupSnapshots (default: velero.io/volume-group), was added in Velero v1.17.0. It is configurable via the Backup spec field volumeGroupSnapshotLabelKey, the server flag --volume-group-snapshot-label-key, or defaults to velero.io/volume-group.
Citations:
- 1: https://velero.io/docs/v1.18/volume-group-snapshots
- 2: Implement VoulumeGroupSnapshot (VGS) support in Velero vmware-tanzu/velero#8865
- 3: https://github.com/vmware-tanzu/velero/releases
- 4: Add support for configuring VGS label key vmware-tanzu/velero#8938
CRITICAL: Test case references a non-existent BackupSpec field in current Velero version.
The VolumeGroupSnapshotLabelKey field was added in Velero v1.17.0, but this project uses v1.14.0. The test case at lines 381-385 will fail when the completeness check (lines 460-475) attempts to validate the field against the actual velerov1.BackupSpec struct via reflection. The field does not exist in v1.14.0's BackupSpec.
Either upgrade Velero to v1.17.0 or later, or remove this test case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/function/function_test.go` around lines 381 - 385, The test
includes a case named "VolumeGroupSnapshotLabelKey" that references
BackupSpec.VolumeGroupSnapshotLabelKey which does not exist in the vendored
Velero v1.14.0; remove that test case (or replace it with a field present in
v1.14.0) so the completeness check in function_test.go (the reflection-based
validation around lines 460-475) no longer expects the nonexistent field;
alternatively, if you intend to keep the test, upgrade the Velero dependency to
v1.17.0+ so BackupSpec contains VolumeGroupSnapshotLabelKey and the reflection
check will pass.
| if !slices.Contains(restoreSpec.ExcludedResources, constant.WildcardString) { | ||
| restoreSpec.ExcludedResources = append(restoreSpec.ExcludedResources, | ||
| "volumesnapshotclasses") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if there are tests covering the wildcard exclusion scenario for restores
# Search for test cases involving ExcludedResources with wildcard in restore controller tests
rg -n -C5 'ExcludedResources.*\*|"\*"' internal/controller/nonadminrestore_controller_test.go
# Also check if there's any wildcard-related test naming
rg -n 'wildcard|Wildcard' internal/controller/nonadminrestore_controller_test.goRepository: migtools/oadp-non-admin
Length of output: 49
🏁 Script executed:
# Check the implementation at lines 352-355
cat -n internal/controller/nonadminrestore_controller.go | sed -n '350,360p'Repository: migtools/oadp-non-admin
Length of output: 479
🏁 Script executed:
# Check the test file structure and the mentioned lines 369-370
wc -l internal/controller/nonadminrestore_controller_test.go
head -n 1 internal/controller/nonadminrestore_controller_test.goRepository: migtools/oadp-non-admin
Length of output: 128
🏁 Script executed:
# Look at the test file around lines 369-370 to see what's being tested
sed -n '365,380p' internal/controller/nonadminrestore_controller_test.goRepository: migtools/oadp-non-admin
Length of output: 779
🏁 Script executed:
# Search for any test setup with wildcard patterns in ExcludedResources across the entire test file
rg -n 'ExcludedResources' internal/controller/nonadminrestore_controller_test.goRepository: migtools/oadp-non-admin
Length of output: 143
🏁 Script executed:
# Look for scenario setup that might include wildcard configurations
rg -n 'scenario\.|WildcardString' internal/controller/nonadminrestore_controller_test.goRepository: migtools/oadp-non-admin
Length of output: 1682
🏁 Script executed:
# Check what WildcardString constant is defined as
rg -n 'WildcardString.*=' internal/pkg/constant/ | head -5Repository: migtools/oadp-non-admin
Length of output: 129
🏁 Script executed:
# Look for any scenario definitions in the test file to understand test structure
sed -n '1,150p' internal/controller/nonadminrestore_controller_test.go | grep -A2 'type.*scenario\|var.*scenario' | head -20Repository: migtools/oadp-non-admin
Length of output: 49
🏁 Script executed:
# Find where WildcardString is defined
rg -n 'WildcardString' --type go | head -10Repository: migtools/oadp-non-admin
Length of output: 821
🏁 Script executed:
# Look at the test scenarios structure - search for test case definitions
sed -n '80,200p' internal/controller/nonadminrestore_controller_test.goRepository: migtools/oadp-non-admin
Length of output: 5489
🏁 Script executed:
# Search for test scenario definitions in the test file
rg -n 'nonAdminRestoreFullReconcileScenario\{' internal/controller/nonadminrestore_controller_test.go -A 15 | head -100Repository: migtools/oadp-non-admin
Length of output: 2266
🏁 Script executed:
# Look for all test entry definitions with Entry() to see if any test the wildcard scenario
rg -n 'ginkgo.Entry' internal/controller/nonadminrestore_controller_test.go -B 2 -A 8 | head -150Repository: migtools/oadp-non-admin
Length of output: 1917
🏁 Script executed:
# Look at the test scenario structures to see if any include ExcludedResources with wildcard
sed -n '451,610p' internal/controller/nonadminrestore_controller_test.go | grep -C 10 'ExcludedResources'Repository: migtools/oadp-non-admin
Length of output: 49
🏁 Script executed:
# Search for all test scenario definitions to find if wildcard is tested
rg -n 'ginkgo.Entry.*"Should' internal/controller/nonadminrestore_controller_test.goRepository: migtools/oadp-non-admin
Length of output: 638
🏁 Script executed:
# Get more details on what test scenarios exist and their structure
sed -n '451,625p' internal/controller/nonadminrestore_controller_test.go | head -200Repository: migtools/oadp-non-admin
Length of output: 5120
🏁 Script executed:
# Check if there's any test that explicitly uses wildcard in ExcludedResources for restore tests
rg -n '\*' internal/controller/nonadminrestore_controller_test.go | grep -i 'exclude\|resource' | head -10Repository: migtools/oadp-non-admin
Length of output: 49
🏁 Script executed:
# Look at the backup controller to see if it has similar tests and if wildcard scenarios are tested there
rg -n 'wildcard\|WildcardString' internal/controller/nonadminbackup_controller_test.go -B 2 -A 2Repository: migtools/oadp-non-admin
Length of output: 49
Confirm test coverage gap: add test case for wildcard exclusion scenario.
The implementation correctly prevents appending volumesnapshotclasses when "*" is already in ExcludedResources, avoiding Velero validation failures. However, test coverage exists only for the normal case where volumesnapshotclasses is appended (line 369). A test case is missing to verify that when ExcludedResources = ["*"], it remains unchanged after processing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/controller/nonadminrestore_controller.go` around lines 352 - 355,
Add a unit test that exercises the code path which checks
slices.Contains(restoreSpec.ExcludedResources, constant.WildcardString) so we
verify the wildcard case is preserved: create a Restore (or restoreSpec) with
ExcludedResources set to []string{constant.WildcardString}, invoke the
controller logic that runs the exclusion-normalization (the code in
nonadminrestore_controller.go — the Reconcile / processing function that
contains the slices.Contains check), and assert that ExcludedResources still
equals []string{constant.WildcardString} (i.e., "volumesnapshotclasses" is not
appended). Ensure the test uses the same symbols restoreSpec.ExcludedResources
and constant.WildcardString for clarity.
|
/unhold |
|
/hold testing... |
4 participants
When a user sets excludedClusterScopedResources or excludedNamespaceScopedResources to '*', skip appending default resources to avoid Velero FailedValidation.
Why the changes were made
Fix: OADP-7562
How to test the changes made
Unit test was added to cover bug scenario.
Summary by CodeRabbit