fix(backups): restore should not require existing target app#2239
fix(backups): restore should not require existing target app#2239androndo wants to merge 1 commit intomaincozystack/cozystack:mainfrom fix/restore-not-requires-target-appcozystack/cozystack:fix/restore-not-requires-target-appCopy head branch name to clipboard
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable sequence diagrams in the walkthrough.Disable the |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in the backup and restore functionality where in-place restores would fail if the target application was not already present in the cluster. The changes ensure that the restore process can successfully proceed by adjusting how the application's context is prepared for Velero, making it resilient to scenarios where the original application might be missing or when restoring to a new environment. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
The createVeleroRestore function in velerostrategy_controller.go was updated to simplify how the Application field is populated within the template context. Previously, the code would fetch the full target application object from the Kubernetes API server. This has been replaced by directly constructing a map containing only the application's name, namespace, and kind from the backup.Spec.ApplicationRef and backup.Namespace, thereby removing an unnecessary API call and streamlining the restore process.
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 `@internal/backupcontroller/velerostrategy_controller.go`:
- Around line 438-445: Do a best-effort fetch of the actual application object
and put the real object into the templateContext under the "Application" key
when available; if the GET (using
backup.Spec.ApplicationRef.Name/Kind/Namespace) fails, fall back to a minimal
synthetic map but only include metadata.name and include metadata.namespace only
if the application ref actually provided a namespace (do not unconditionally set
it to backup.Namespace), and ensure templates continue to receive the full
unstructured object (e.g. the unstructured.Unstructured.Object) when the
resource is found; update the code around templateContext and the application
lookup to use this fetch-then-fallback pattern so existing restore templates
that read .Application.spec or labels still work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af4e23da-8211-4f35-be25-0c9c24222998
📒 Files selected for processing (1)
internal/backupcontroller/velerostrategy_controller.go
| templateContext := map[string]interface{}{ | ||
| "Application": app.Object, | ||
| "Application": map[string]interface{}{ | ||
| "metadata": map[string]interface{}{ | ||
| "name": backup.Spec.ApplicationRef.Name, | ||
| "namespace": backup.Namespace, | ||
| }, | ||
| "kind": backup.Spec.ApplicationRef.Kind, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Preserve restore template compatibility while making app lookup optional
Line 439 now always supplies a reduced synthetic .Application, which can break existing restore templates that read other fields (for example .Application.spec.* or labels). Also, Line 442 always sets .Application.metadata.namespace = backup.Namespace, which is incorrect for cluster-scoped apps and can be wrong if app namespace differs from the Backup namespace.
Consider restoring a best-effort object fetch: use full object when available; fall back to synthetic metadata-only context only when the app is missing.
Proposed compatibility-safe approach
- templateContext := map[string]interface{}{
- "Application": map[string]interface{}{
- "metadata": map[string]interface{}{
- "name": backup.Spec.ApplicationRef.Name,
- "namespace": backup.Namespace,
- },
- "kind": backup.Spec.ApplicationRef.Kind,
- },
- "Parameters": map[string]string{},
- }
+ application := map[string]interface{}{
+ "metadata": map[string]interface{}{
+ "name": backup.Spec.ApplicationRef.Name,
+ },
+ "kind": backup.Spec.ApplicationRef.Kind,
+ }
+
+ // Best-effort enrichment: keep restore independent from target app existence.
+ if backup.Spec.ApplicationRef.APIGroup != nil {
+ if mapping, err := r.RESTMapping(schema.GroupKind{
+ Group: *backup.Spec.ApplicationRef.APIGroup,
+ Kind: backup.Spec.ApplicationRef.Kind,
+ }); err == nil {
+ ns := ""
+ if mapping.Scope.Name() == meta.RESTScopeNameNamespace {
+ ns = backup.Namespace
+ application["metadata"].(map[string]interface{})["namespace"] = ns
+ }
+ if app, err := r.Resource(mapping.Resource).Namespace(ns).Get(ctx, backup.Spec.ApplicationRef.Name, metav1.GetOptions{}); err == nil {
+ application = app.Object
+ }
+ }
+ }
+
+ templateContext := map[string]interface{}{
+ "Application": application,
+ "Parameters": map[string]string{},
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/backupcontroller/velerostrategy_controller.go` around lines 438 -
445, Do a best-effort fetch of the actual application object and put the real
object into the templateContext under the "Application" key when available; if
the GET (using backup.Spec.ApplicationRef.Name/Kind/Namespace) fails, fall back
to a minimal synthetic map but only include metadata.name and include
metadata.namespace only if the application ref actually provided a namespace (do
not unconditionally set it to backup.Namespace), and ensure templates continue
to receive the full unstructured object (e.g. the
unstructured.Unstructured.Object) when the resource is found; update the code
around templateContext and the application lookup to use this
fetch-then-fallback pattern so existing restore templates that read
.Application.spec or labels still work.
What this PR does
Release note
Summary by CodeRabbit