fix: OCPBUGS-87162: Validate restore --hc-name matches backup namespaces#8670
fix: OCPBUGS-87162: Validate restore --hc-name matches backup namespaces#8670dpateriya wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-moderate
|
@dpateriya: This pull request references Jira Issue OCPBUGS-87162, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/cli
dpateriya
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dpateriya The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/retest |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds validation to the restore command to ensure the Velero backup includes the expected HCP control-plane namespace. The Sequence Diagram(s)sequenceDiagram
participant CLI
participant validateBackupExists
participant VeleroAPI
participant BackupSpec
CLI->>validateBackupExists: invoke restore validation (hc-name, hc-namespace, backup)
validateBackupExists->>VeleroAPI: GET Backup resource
VeleroAPI-->>validateBackupExists: Backup with spec.includedNamespaces
validateBackupExists->>BackupSpec: parse includedNamespaces
BackupSpec-->>validateBackupExists: namespaces list or parse error
validateBackupExists-->>CLI: error (non-render) or warning/log (render)
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (10 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 |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@dpateriya: This pull request references Jira Issue OCPBUGS-87162, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
This confirms everything. Now I have the complete picture for the report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe gitlint CI check failed because the commit message title does not follow the Conventional Commits format required by the repository. The PR title correctly uses Root CauseThe repository's The commit This starts with the JIRA issue key The author set the PR title correctly (with Recommendations
Evidence
|
![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)
cmd/oadp/restore_test.go (1)
358-406: ⚡ Quick winRename new table cases to the required
When ... it should ...style.These new case titles don’t follow the required
_test.gonaming convention.Example rename pattern
- name: "matching namespaces should pass", + name: "When backup includes expected control plane namespace it should pass", - name: "mismatched hc-name should fail", + name: "When hc-name does not match backup namespaces it should fail", - name: "mismatched hc-namespace should fail", + name: "When hc-namespace does not match backup namespaces it should fail", - name: "mismatched namespaces in render mode should warn but not fail", + name: "When namespaces mismatch in render mode it should warn without failing", - name: "backup without includedNamespaces should skip validation", + name: "When backup has no includedNamespaces it should skip namespace validation", - name: "matching namespaces with additional namespaces should pass", + name: "When expected namespace exists among additional namespaces it should pass",As per coding guidelines, "
**/*_test.go: Always use "When ... it should ..." format for describing test cases when creating unit tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/oadp/restore_test.go` around lines 358 - 406, Rename the test case "name" fields in the table that define the test scenarios so they follow the required `_test.go` convention: use "When ... it should ..." phrasing. Specifically update entries whose name values are "matching namespaces should pass", "mismatched hc-name should fail", "mismatched hc-namespace should fail", "mismatched namespaces in render mode should warn but not fail", "backup without includedNamespaces should skip validation", and "matching namespaces with additional namespaces should pass" to descriptive "When ... it should ..." sentences (e.g. "When backup namespaces match control plane it should pass", "When hc name mismatches it should fail with ...", etc.) so the test table entries in restore_test.go use the required naming style; keep the rest of the struct fields (hcName, hcNamespace, backupNamespaces, renderMode, shouldError, errorSubstring) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/oadp/restore.go`:
- Around line 295-309: The code currently ignores parsing errors from
unstructured.NestedStringSlice (backupNamespaces, nsFound, nsErr) inside
validateBackupExists; update the logic to handle nsErr when nsFound is true by
surfacing the error instead of silently proceeding: if nsErr != nil then in
renderMode call o.Log.Info or o.Log.Error with a clear message including
o.BackupName and nsErr, and when not in renderMode return a formatted error
(similar style to existing returns) explaining that spec.includedNamespaces is
malformed and include nsErr; keep the existing namespace-contains check
(slices.Contains) when nsErr == nil.
---
Nitpick comments:
In `@cmd/oadp/restore_test.go`:
- Around line 358-406: Rename the test case "name" fields in the table that
define the test scenarios so they follow the required `_test.go` convention: use
"When ... it should ..." phrasing. Specifically update entries whose name values
are "matching namespaces should pass", "mismatched hc-name should fail",
"mismatched hc-namespace should fail", "mismatched namespaces in render mode
should warn but not fail", "backup without includedNamespaces should skip
validation", and "matching namespaces with additional namespaces should pass" to
descriptive "When ... it should ..." sentences (e.g. "When backup namespaces
match control plane it should pass", "When hc name mismatches it should fail
with ...", etc.) so the test table entries in restore_test.go use the required
naming style; keep the rest of the struct fields (hcName, hcNamespace,
backupNamespaces, renderMode, shouldError, errorSubstring) unchanged.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0ed0a0ab-c39d-4b17-9dd9-4b070e48fb2d
📒 Files selected for processing (2)
cmd/oadp/restore.gocmd/oadp/restore_test.go
Test EvidenceTested the built binary ( Negative Test — Namespace mismatch detectedThe CLI now correctly blocks the restore and surfaces an actionable error when Positive Test — Matching names proceed normallyWhen |
|
/verified by me |
openshift-ci-robot
added
the
verified
|
@dpateriya: This PR has been marked as verified by 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. |
The oadp-restore command silently proceeds even when --hc-name and --hc-namespace do not match the backup's includedNamespaces, resulting in a partial restore that reports success with only guest-cluster resources while missing the entire control plane. Add namespace validation in validateBackupExists() that cross-checks the expected HCP namespace (<hc-namespace>-<hc-name>) against the backup's spec.includedNamespaces. In normal mode, a mismatch returns a clear error with the expected and actual namespaces. In render mode, it logs a warning so the user can inspect the generated manifest. Also update --hc-name and --hc-namespace flag descriptions to clarify they must match the original backed-up cluster values. Signed-off-by: Divyam pateriya <dpateriy@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
dpateriya
force-pushed
the
OCPBUGS-87162/restore-namespace-validation
branch
from
43ac4c1 to
422eb82
Compare
openshift-ci-robot
removed
the
verified
|
@dpateriya: This pull request references Jira Issue OCPBUGS-87162, which is valid. 3 validation(s) were run on this bug
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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8670 +/- ##
=======================================
Coverage 41.43% 41.43%
=======================================
Files 756 756
Lines 93658 93678 +20
=======================================
+ Hits 38807 38820 +13
- Misses 52128 52134 +6
- Partials 2723 2724 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@dpateriya: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
2 participants
Summary
--hc-name/--hc-namespaceprovided tohypershift create oadp-restorematch the cluster name stored in the backup'sspec.includedNamespaces, preventing silently partial restores when mismatched--hc-nameand--hc-namespaceflag descriptions to indicate they must match the original backed-up clusterProblem
When a user runs
hypershift create oadp-restore --hc-name restore-exampleagainst a backup of clusterexample, the CLI computesincludedNamespaces: [clusters, clusters-restore-example]. Sinceclusters-restore-exampledoesn't exist in the backup (it hasclusters-example), the Velero Restore completes successfully but only restores resources from the shared HC namespace — the entire control plane namespace is silently skipped.The restore reports
phase: CompletedwithitemsRestored: 19even though no HostedControlPlane, deployments, statefulsets, or other control plane resources were restored.Fix
After confirming the backup exists and is Completed, read its
spec.includedNamespacesand verify the expected HCP namespace ({hcNamespace}-{hcName}) is present. In non-render mode, return a hard error with a clear message showing the expected vs actual namespaces. In render mode, log a warning but proceed (consistent with existing render mode behavior).Test plan
TestValidateBackupNamespaceMatchcovering:--hc-namefails with clear error--hc-namespacefails with clear errorincludedNamespacesskips validation gracefullycmd/oadp/unit tests continue to passJira: https://redhat.atlassian.net/browse/OCPBUGS-87162
Made with Cursor
Summary by CodeRabbit
New Features
Improvements
--hc-nameand--hc-namespaceflag help text to state they must match the original backed-up cluster.Tests