Repository navigation
feat(backups): implement Job backup strategy - #1721
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughImplements Job strategy reconciliation for backups and restores: documents template rendering context, adds controller logic to render templates and manage deterministic batch Jobs, persists Backup artifacts with driver-parameter round-trip, and adds comprehensive unit tests plus NATS example/demo scripts and docs. ChangesJob Strategy Controller Implementation
Sequence Diagram(s)sequenceDiagram
participant BackupJobReconciler
participant JobStrategy as JobStrategy_CR
participant App as Target_Application
participant BatchJob as batch/v1.Job
participant Backup as Backup_Artifact
BackupJobReconciler->>BackupJobReconciler: phase short-circuit check
BackupJobReconciler->>BackupJobReconciler: set StartedAt (refetch + patch)
BackupJobReconciler->>JobStrategy: fetch strategy CR
BackupJobReconciler->>App: fetch target application
BackupJobReconciler->>BackupJobReconciler: render template (.Application, .Mode, .Parameters)
BackupJobReconciler->>BatchJob: ensure deterministic owned batch/v1.Job (mode/owner labels)
BatchJob-->>BackupJobReconciler: Job status (Running/Failed/Succeeded)
BackupJobReconciler->>Backup: on success create Backup CR with StrategyRef + driver metadata
sequenceDiagram
participant RestoreJobReconciler
participant SourceBackup as Source_Backup
participant JobStrategy as JobStrategy_CR
participant TargetApp as Target_Application
participant BatchJob as batch/v1.Job
RestoreJobReconciler->>RestoreJobReconciler: phase short-circuit check
RestoreJobReconciler->>SourceBackup: extract application ref and parameters from DriverMetadata
RestoreJobReconciler->>RestoreJobReconciler: resolve effective target (targetApplicationRef override)
RestoreJobReconciler->>RestoreJobReconciler: set StartedAt (refetch + patch)
RestoreJobReconciler->>JobStrategy: fetch strategy CR
RestoreJobReconciler->>TargetApp: fetch resolved target application
RestoreJobReconciler->>RestoreJobReconciler: render restore-mode template (.Application, .Mode, .Parameters, .Backup)
RestoreJobReconciler->>BatchJob: ensure deterministic owned batch/v1.Job in restore namespace
BatchJob-->>RestoreJobReconciler: Job status (Running/Failed/Succeeded)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested labels
Suggested reviewers
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 docstrings
🧪 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 |
Contributor
Summary of ChangesHello Timofei Larkin (@lllamnyp), 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 lays the foundational groundwork for a robust backup system by implementing the Highlights
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 (@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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new JobStrategy implementation for BackupJob resources, allowing the creation of Kubernetes Jobs based on templated PodTemplateSpec definitions. It also includes a generic templating utility to process Go template expressions within Kubernetes objects. The changes are a good step towards a flexible backup strategy. However, there are a few areas that could be improved for robustness and error handling, particularly regarding the templating logic and handling of API group references.
Comment on lines
+19
to
+25
| templateFunc := func(in string) string { | ||
| out, err := template(in, templateContext) | ||
| if err != nil { | ||
| return in | ||
| } | ||
| return out | ||
| } |
Contributor
There was a problem hiding this comment.
The templateFunc silently swallows errors from template(in, templateContext), returning the original string in instead. This can lead to subtle bugs where template expressions are malformed or refer to non-existent fields, but no error is propagated, and the resulting object contains untemplated strings. It would be more robust to propagate these errors or at least log them with a higher severity to make debugging easier.
Suggested change
| templateFunc := func(in string) string { | |
| out, err := template(in, templateContext) | |
| if err != nil { | |
| return in | |
| } | |
| return out | |
| } | |
| templateFunc := func(in string) string { | |
| out, err := template(in, templateContext) | |
| if err != nil { | |
| // TODO: Log this error or return it from Template function for better error visibility | |
| return in | |
| } | |
| return out | |
| } |
Comment on lines
+56
to
+64
| if j.Spec.ApplicationRef.APIGroup != nil { | ||
| applicationRefAPIGroup = *j.Spec.ApplicationRef.APIGroup | ||
| } | ||
| if j.Spec.StrategyRef.APIGroup != nil { | ||
| strategyRefAPIGroup = *j.Spec.StrategyRef.APIGroup | ||
| } | ||
| if j.Spec.StorageRef.APIGroup != nil { | ||
| storageRefAPIGroup = *j.Spec.StorageRef.APIGroup | ||
| } |
Contributor
There was a problem hiding this comment.
The APIGroup fields in TypedLocalObjectReference are pointers to strings. If they are nil, the current logic initializes applicationRefAPIGroup, strategyRefAPIGroup, and storageRefAPIGroup to empty strings. While an empty string for APIGroup typically refers to the core Kubernetes API group, this behavior is implicit and not explicitly handled or documented. It might be clearer to explicitly check for nil and assign "" if that's the intended behavior for core API group resources, or handle it as an error if APIGroup is always expected to be set for non-core resources.
var applicationRefAPIGroup string
var strategyRefAPIGroup string
var storageRefAPIGroup string
if j.Spec.ApplicationRef.APIGroup != nil {
applicationRefAPIGroup = *j.Spec.ApplicationRef.APIGroup
} else {
// Default to core API group if not specified
applicationRefAPIGroup = ""
}
if j.Spec.StrategyRef.APIGroup != nil {
strategyRefAPIGroup = *j.Spec.StrategyRef.APIGroup
} else {
strategyRefAPIGroup = ""
}
if j.Spec.StorageRef.APIGroup != nil {
storageRefAPIGroup = *j.Spec.StorageRef.APIGroup
} else {
storageRefAPIGroup = ""
}
Comment on lines
+105
to
+108
| values, ok := app.Object["spec"].(map[string]any) | ||
| if !ok { | ||
| values = map[string]any{} | ||
| } |
Contributor
There was a problem hiding this comment.
The type assertion app.Object["spec"].(map[string]any) and subsequent fallback to an empty map if the assertion fails could mask potential issues. If the application object's spec is expected to always be a map[string]any, a failure here might indicate a malformed resource or an unexpected API version. It would be safer to log a warning or return an error in such cases, rather than silently proceeding with an empty map, which could lead to incorrect template rendering.
values, ok := app.Object["spec"].(map[string]any)
if !ok {
log.Info("Application object 'spec' field is not a map[string]any, using empty values", "application", client.ObjectKeyFromObject(app))
values = map[string]any{}
}
6 of 21 tasks
github-actions
Bot
added
do-not-merge/work-in-progress
Timofei Larkin (lllamnyp)
force-pushed
the
feat/jobdriver-implementation
branch
from
May 22, 2026 08:38
15ce380 to
89c72b0
Compare
github-actions
Bot
added
area/uncategorized
Timofei Larkin (lllamnyp)
changed the title
github-actions
Bot
added
area/storage
Complete the strategy.backups.cozystack.io/Job driver as the generic, app-agnostic counterpart to the Altinity driver: render the strategy PodTemplateSpec, run it as a one-shot batch/v1.Job, poll its terminal condition, and emit a Backup. Wire it into the existing BackupClass dispatch via reconcileJob/reconcileJobRestore rather than a standalone reconciler, matching the post-BackupClass backups API (the old draft read BackupJob.Spec.StrategyRef/StorageRef, which no longer exist). Template context follows the in-tree convention (.Application, .Release, .Mode, .Parameters, .Backup); the JobSpec.Template doc and regenerated CRD are updated to match. Restore re-renders the same template with .Mode=restore; cross-namespace restore is unsupported, mirroring Altinity. Signed-off-by: Timofei Larkin <[email protected]>
Timofei Larkin (lllamnyp)
force-pushed
the
feat/jobdriver-implementation
branch
from
May 22, 2026 13:46
89c72b0 to
0291149
Compare
Timofei Larkin (lllamnyp)
marked this pull request as ready for review
May 24, 2026 07:04
dosubot
Bot
added
the
kind/api-change
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements the generic Job strategy for backups and restores, enabling app-agnostic backup tools to be executed as Kubernetes batch jobs. The implementation includes controller logic for template rendering, job lifecycle management, and backup artifact creation, along with comprehensive unit tests and updated CRD documentation. Feedback identifies opportunities to normalize application references to avoid potential lookup failures and suggests making the hardcoded job backoff limit configurable for improved flexibility.
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| app, err := r.getApplicationUnstructured(ctx, j.Namespace, j.Spec.ApplicationRef) |
Contributor
There was a problem hiding this comment.
The ApplicationRef should be normalized to ensure the default API group (apps.cozystack.io) is used if it's missing in the spec. While the caller in backupjob_controller.go performs normalization for resolving the backup class, it passes the original unnormalized object j to this method. This could lead to NotFound errors if getApplicationUnstructured expects a fully qualified group.
Suggested change
| app, err := r.getApplicationUnstructured(ctx, j.Namespace, j.Spec.ApplicationRef) | |
| app, err := r.getApplicationUnstructured(ctx, j.Namespace, backupsv1alpha1.NormalizeApplicationRef(j.Spec.ApplicationRef)) |
Comment on lines
+362
to
+366
| targetRef := corev1.TypedLocalObjectReference{ | ||
| APIGroup: stringPtr(targetAPIGroup), | ||
| Kind: targetAppKind, | ||
| Name: targetAppName, | ||
| } |
Contributor
There was a problem hiding this comment.
Similar to the backup path, the targetRef should be normalized to ensure the default API group is correctly populated if missing. This ensures that getApplicationUnstructured and the subsequent template rendering use the correct fully qualified group.
Suggested change
| targetRef := corev1.TypedLocalObjectReference{ | |
| APIGroup: stringPtr(targetAPIGroup), | |
| Kind: targetAppKind, | |
| Name: targetAppName, | |
| } | |
| targetRef := backupsv1alpha1.NormalizeApplicationRef(corev1.TypedLocalObjectReference{ | |
| APIGroup: stringPtr(targetAPIGroup), | |
| Kind: targetAppKind, | |
| Name: targetAppName, | |
| }) |
| for k, v := range labels { | ||
| pod.Labels[k] = v | ||
| } | ||
| backoffLimit := int32(2) |
Contributor
There was a problem hiding this comment.
The backoffLimit is hardcoded to 2. While this matches the Altinity driver, for a generic Job strategy, it might be beneficial to allow this to be configurable via BackupClass parameters or the Job spec in the future, as different backup tools might have different retry requirements.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/backupcontroller/jobstrategy_controller_test.go`:
- Around line 189-190: The test assertion prints entire []string slices with %q;
change the t.Errorf calls that compare containerArgs and args to report the
specific element values (use containerArgs[i] and args[i]) and guard against
out-of-range by substituting "<missing>" when an index is absent; update both
the backup check (using variables containerArgs and args around the existing
t.Errorf) and the similar restore-args check near the other assertion (~line
436) so failures show the specific element instead of the whole slice.
- Around line 62-64: The test setup currently ignores errors returned by
scheme.AddToScheme, backupsv1alpha1.AddToScheme, and
strategyv1alpha1.AddToScheme—update the test initialization to check each error
and call t.Fatalf with the error if non-nil (e.g., if err :=
scheme.AddToScheme(testScheme); err != nil { t.Fatalf("...: %v", err) }) so
failures fail fast; additionally, replace use of %q for slice values in t.Errorf
calls that log containerArgs and args (at the assertions around
containerArgs/args) with %v or use strings.Join(slice, " ") so the output is
readable (update the t.Errorf format strings that reference containerArgs and
args accordingly).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81d3c182-bade-4de5-abef-b3cca7432007
📒 Files selected for processing (4)
api/backups/strategy/v1alpha1/job_types.gointernal/backupcontroller/jobstrategy_controller.gointernal/backupcontroller/jobstrategy_controller_test.gopackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_jobs.yaml
Comment on lines
+62
to
+64
| _ = scheme.AddToScheme(testScheme) | ||
| _ = backupsv1alpha1.AddToScheme(testScheme) | ||
| _ = strategyv1alpha1.AddToScheme(testScheme) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ignored AddToScheme errors in this test file
rg -n '^\s*_\s*=\s*(scheme\.AddToScheme|backupsv1alpha1\.AddToScheme|strategyv1alpha1\.AddToScheme)\(' internal/backupcontroller/jobstrategy_controller_test.goRepository: cozystack/cozystack
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/backupcontroller/jobstrategy_controller_test.go"
# Show the scheme setup region (context around 62-64)
sed -n '45,90p' "$FILE"
# Locate t.Errorf usages with %q in the file
rg -n 't\.Errorf\([^\)]*%q' "$FILE"
# Show surrounding context for the specific line numbers mentioned in the review comment (best-effort)
sed -n '170,220p' "$FILE"
sed -n '410,470p' "$FILE"Repository: cozystack/cozystack
Length of output: 7307
Fail fast on scheme registration errors in test setup
- internal/backupcontroller/jobstrategy_controller_test.go:62-64 currently ignores returned errors from
scheme.AddToScheme,backupsv1alpha1.AddToScheme, andstrategyv1alpha1.AddToScheme; uset.Fatalfon any error.
Suggested patch
testScheme := runtime.NewScheme()
- _ = scheme.AddToScheme(testScheme)
- _ = backupsv1alpha1.AddToScheme(testScheme)
- _ = strategyv1alpha1.AddToScheme(testScheme)
+ if err := scheme.AddToScheme(testScheme); err != nil {
+ t.Fatalf("add core scheme: %v", err)
+ }
+ if err := backupsv1alpha1.AddToScheme(testScheme); err != nil {
+ t.Fatalf("add backups v1alpha1 scheme: %v", err)
+ }
+ if err := strategyv1alpha1.AddToScheme(testScheme); err != nil {
+ t.Fatalf("add strategy v1alpha1 scheme: %v", err)
+ }- Around internal/backupcontroller/jobstrategy_controller_test.go:189 and ~436,
t.Errorfuses%qfor slice values (containerArgs/argsof type[]string); switch to%v(orstrings.Join) so failure output is readable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _ = scheme.AddToScheme(testScheme) | |
| _ = backupsv1alpha1.AddToScheme(testScheme) | |
| _ = strategyv1alpha1.AddToScheme(testScheme) | |
| testScheme := runtime.NewScheme() | |
| if err := scheme.AddToScheme(testScheme); err != nil { | |
| t.Fatalf("add core scheme: %v", err) | |
| } | |
| if err := backupsv1alpha1.AddToScheme(testScheme); err != nil { | |
| t.Fatalf("add backups v1alpha1 scheme: %v", err) | |
| } | |
| if err := strategyv1alpha1.AddToScheme(testScheme); err != nil { | |
| t.Fatalf("add strategy v1alpha1 scheme: %v", err) | |
| } |
🤖 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 `@internal/backupcontroller/jobstrategy_controller_test.go` around lines 62 -
64, The test setup currently ignores errors returned by scheme.AddToScheme,
backupsv1alpha1.AddToScheme, and strategyv1alpha1.AddToScheme—update the test
initialization to check each error and call t.Fatalf with the error if non-nil
(e.g., if err := scheme.AddToScheme(testScheme); err != nil { t.Fatalf("...:
%v", err) }) so failures fail fast; additionally, replace use of %q for slice
values in t.Errorf calls that log containerArgs and args (at the assertions
around containerArgs/args) with %v or use strings.Join(slice, " ") so the output
is readable (update the t.Errorf format strings that reference containerArgs and
args accordingly).
Comment on lines
+189
to
+190
| t.Errorf("rendered args[%d]: want %q, got %q", i, want, containerArgs) | ||
| break |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify potentially incorrect %q usage with []string values in test error formatting
rg -nP 't\.Errorf\(".*got %q".*,\s*(containerArgs|args)\s*\)' internal/backupcontroller/jobstrategy_controller_test.goRepository: cozystack/cozystack
Length of output: 215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '160,215p' internal/backupcontroller/jobstrategy_controller_test.go | nl -ba -w2 -s':'
echo '---'
sed -n '410,465p' internal/backupcontroller/jobstrategy_controller_test.go | nl -ba -w2 -s':'Repository: cozystack/cozystack
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '160,215p' internal/backupcontroller/jobstrategy_controller_test.go | cat -n -w2 -s':'
echo '---'
sed -n '410,465p' internal/backupcontroller/jobstrategy_controller_test.go | cat -n -w2 -s':'Repository: cozystack/cozystack
Length of output: 129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== section 160-215 =="
sed -n '160,215p' internal/backupcontroller/jobstrategy_controller_test.go | cat -n
echo "== section 410-465 =="
sed -n '410,465p' internal/backupcontroller/jobstrategy_controller_test.go | cat -nRepository: cozystack/cozystack
Length of output: 5311
Fix %q formatting of slice values in assertion failures.
containerArgs and args are []string, but the failure message currently passes the whole slice to %q (noisy/less actionable). Report the specific element containerArgs[i] / args[i] (or "<missing>" when out of range) instead.
File: internal/backupcontroller/jobstrategy_controller_test.go
Lines: 189-190 (also applies to the restore args check at line ~436)
Suggested patch
for i, want := range wantArgs {
if i >= len(containerArgs) || containerArgs[i] != want {
- t.Errorf("rendered args[%d]: want %q, got %q", i, want, containerArgs)
+ got := "<missing>"
+ if i < len(containerArgs) {
+ got = containerArgs[i]
+ }
+ t.Errorf("rendered args[%d]: want %q, got %q", i, want, got)
break
}
}
@@
for i, w := range want {
if i >= len(args) || args[i] != w {
- t.Errorf("rendered restore args[%d]: want %q, got %q", i, w, args)
+ got := "<missing>"
+ if i < len(args) {
+ got = args[i]
+ }
+ t.Errorf("rendered restore args[%d]: want %q, got %q", i, w, got)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| t.Errorf("rendered args[%d]: want %q, got %q", i, want, containerArgs) | |
| break | |
| got := "<missing>" | |
| if i < len(containerArgs) { | |
| got = containerArgs[i] | |
| } | |
| t.Errorf("rendered args[%d]: want %q, got %q", i, want, got) | |
| break |
🤖 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 `@internal/backupcontroller/jobstrategy_controller_test.go` around lines 189 -
190, The test assertion prints entire []string slices with %q; change the
t.Errorf calls that compare containerArgs and args to report the specific
element values (use containerArgs[i] and args[i]) and guard against out-of-range
by substituting "<missing>" when an index is absent; update both the backup
check (using variables containerArgs and args around the existing t.Errorf) and
the similar restore-args check near the other assertion (~line 436) so failures
show the specific element instead of the whole slice.
Aleksei Sviridkin (lexfrei)
removed
the
area/uncategorized
Contributor
|
Address review feedback on the Job backup strategy: - Assert .Application is templated: the rendered backup Job now reads .Application.spec.replicas, the load-bearing context key that lets a generic backup tool read live app spec (previously only .Release, .Mode, .Parameters and .Backup had coverage). - Pin failure-path messages so a reordered error path can't pass on the wrong branch: the missing-strategy test asserts the strategy name, and the unmappable-kind tests assert "kind not registered". - Document why the full Status().Update in the success branches is safe (the object is freshly Get() each pass; the StartedAt write requeues). - Note the fixed batch/v1.Job backoffLimit=2 and link the follow-up to make it configurable (#2742). Signed-off-by: Timofei Larkin <[email protected]>
Add examples/backups/nats/, the first worked example of the generic Job
backup strategy. It backs up a NATS JetStream stream that has no dedicated
driver, using a stock natsio/nats-box image: 'nats stream backup' -> tar ->
curl --aws-sigv4 PUT to the COSI/seaweedfs S3 endpoint, and the reverse for
restore. One PodTemplateSpec serves both directions, branching at runtime on
{{ .Mode }} (the engine templates each string leaf independently and cannot
restructure the Pod per mode).
Mirrors the clickhouse example's structure: numbered scripts (bucket -> NATS
src + seeded stream -> strategy -> BackupClass -> BackupJob -> in-place and
to-copy RestoreJob), shared helpers, run-all.sh, README, and scenario docs.
Like every examples/backups/* dir it is a runnable demo, not CI-gated.
Signed-off-by: Timofei Larkin <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@examples/backups/nats/03-create-bucket.sh`:
- Around line 60-66: The current here-doc writes raw S3_* variables into
.bucket-info.env which can allow command injection when that file is sourced;
instead, escape or quote each value before writing: replace the heredoc that
creates "$SCRIPT_DIR/.bucket-info.env" with per-variable safe writes that use
shell-safe quoting (e.g. printf with %q or otherwise shell-escape each of
S3_ACCESS_KEY, S3_SECRET_KEY, S3_ENDPOINT, S3_REGION, S3_BUCKET) so the
resulting file contains exported, safely-escaped assignments (refer to the
variables S3_ACCESS_KEY, S3_SECRET_KEY, S3_ENDPOINT, S3_REGION, S3_BUCKET and
the target file .bucket-info.env).
In `@examples/backups/nats/06-restore-in-place.sh`:
- Around line 23-24: The restore YAML is using the BackupJob name variable
`${BACKUPJOB_NAME}` but must reference the actual Backup object name at
`BackupJob.status.backupRef.name`; update the script so it queries the BackupJob
status (e.g., `kubectl get backupjob $BACKUPJOB_NAME -o
jsonpath='{.status.backupRef.name}'` or equivalent) stores that value into a
variable like `BACKUP_NAME`, and then set `RestoreJob.spec.backupRef.name` to
that resolved `BACKUP_NAME` instead of `${BACKUPJOB_NAME}` so the RestoreJob
points to the real Backup resource.
In `@examples/backups/nats/cleanup.sh`:
- Around line 4-24: The script currently uses set -uo pipefail (missing -e) so
failures won’t abort and log_success at the end can report success even if
deletes failed; change the top options to include -e (set -euo pipefail) so the
script exits on any command error, and ensure critical cleanup commands (the
kubectl delete invocations that operate on
restorejob/backupjob/backup/nats/bucket/backupclass/job.strategy.backups.cozystack.io
and the rm) will cause the script to fail rather than reach
print_header/log_success; keep --ignore-not-found where intentional but rely on
-e for real failures so print_header and log_success reflect actual success.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7463f7c4-a5a6-4438-8d60-3ceb77259301
📒 Files selected for processing (14)
examples/backups/nats/00-helpers.shexamples/backups/nats/01-create-strategy.shexamples/backups/nats/02-create-backupclass.shexamples/backups/nats/03-create-bucket.shexamples/backups/nats/04-create-nats.shexamples/backups/nats/05-create-backupjob.shexamples/backups/nats/06-restore-in-place.shexamples/backups/nats/07-restore-to-copy.shexamples/backups/nats/90-scenario-admin-prepare.mdexamples/backups/nats/91-scenario-user-backup.mdexamples/backups/nats/92-scenario-user-restore.mdexamples/backups/nats/README.mdexamples/backups/nats/cleanup.shexamples/backups/nats/run-all.sh
✅ Files skipped from review due to trivial changes (4)
- examples/backups/nats/91-scenario-user-backup.md
- examples/backups/nats/90-scenario-admin-prepare.md
- examples/backups/nats/92-scenario-user-restore.md
- examples/backups/nats/README.md
Comment on lines
+60
to
+66
| cat > "$SCRIPT_DIR/.bucket-info.env" <<ENV | ||
| export S3_ACCESS_KEY=${S3_ACCESS_KEY} | ||
| export S3_SECRET_KEY=${S3_SECRET_KEY} | ||
| export S3_ENDPOINT=${S3_ENDPOINT} | ||
| export S3_REGION=${S3_REGION} | ||
| export S3_BUCKET=${S3_BUCKET} | ||
| ENV |
Contributor
There was a problem hiding this comment.
Escape values before writing .bucket-info.env to avoid command injection
Line 60-Line 66 write unescaped values into a file that is later sourced. If any field contains $(...), backticks, or other shell syntax, it can execute on the operator host.
Proposed fix
umask 077
-cat > "$SCRIPT_DIR/.bucket-info.env" <<ENV
-export S3_ACCESS_KEY=${S3_ACCESS_KEY}
-export S3_SECRET_KEY=${S3_SECRET_KEY}
-export S3_ENDPOINT=${S3_ENDPOINT}
-export S3_REGION=${S3_REGION}
-export S3_BUCKET=${S3_BUCKET}
-ENV
+{
+ printf 'export S3_ACCESS_KEY=%q\n' "$S3_ACCESS_KEY"
+ printf 'export S3_SECRET_KEY=%q\n' "$S3_SECRET_KEY"
+ printf 'export S3_ENDPOINT=%q\n' "$S3_ENDPOINT"
+ printf 'export S3_REGION=%q\n' "$S3_REGION"
+ printf 'export S3_BUCKET=%q\n' "$S3_BUCKET"
+} > "$SCRIPT_DIR/.bucket-info.env"
chmod 600 "$SCRIPT_DIR/.bucket-info.env"🤖 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 `@examples/backups/nats/03-create-bucket.sh` around lines 60 - 66, The current
here-doc writes raw S3_* variables into .bucket-info.env which can allow command
injection when that file is sourced; instead, escape or quote each value before
writing: replace the heredoc that creates "$SCRIPT_DIR/.bucket-info.env" with
per-variable safe writes that use shell-safe quoting (e.g. printf with %q or
otherwise shell-escape each of S3_ACCESS_KEY, S3_SECRET_KEY, S3_ENDPOINT,
S3_REGION, S3_BUCKET) so the resulting file contains exported, safely-escaped
assignments (refer to the variables S3_ACCESS_KEY, S3_SECRET_KEY, S3_ENDPOINT,
S3_REGION, S3_BUCKET and the target file .bucket-info.env).
Comment on lines
+23
to
+24
| backupRef: | ||
| name: ${BACKUPJOB_NAME} |
Contributor
There was a problem hiding this comment.
Use the resolved Backup name in RestoreJob.spec.backupRef
Line 24 uses ${BACKUPJOB_NAME}, but backupRef.name is expected to be the Backup object name. Resolve it from BackupJob.status.backupRef.name before applying the RestoreJob.
Proposed fix
+backup_ref=$(kubectl -n "$NAMESPACE" get backupjob "$BACKUPJOB_NAME" -o jsonpath='{.status.backupRef.name}')
+[[ -n "$backup_ref" ]] || { log_error "BackupRef is empty on BackupJob '${BACKUPJOB_NAME}'"; exit 1; }
+
kubectl apply -f - <<EOF
apiVersion: backups.cozystack.io/v1alpha1
kind: RestoreJob
metadata:
name: ${RESTOREJOB_INPLACE_NAME}
namespace: ${NAMESPACE}
spec:
backupRef:
- name: ${BACKUPJOB_NAME}
+ name: ${backup_ref}
EOF🤖 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 `@examples/backups/nats/06-restore-in-place.sh` around lines 23 - 24, The
restore YAML is using the BackupJob name variable `${BACKUPJOB_NAME}` but must
reference the actual Backup object name at `BackupJob.status.backupRef.name`;
update the script so it queries the BackupJob status (e.g., `kubectl get
backupjob $BACKUPJOB_NAME -o jsonpath='{.status.backupRef.name}'` or equivalent)
stores that value into a variable like `BACKUP_NAME`, and then set
`RestoreJob.spec.backupRef.name` to that resolved `BACKUP_NAME` instead of
`${BACKUPJOB_NAME}` so the RestoreJob points to the real Backup resource.
Comment on lines
+4
to
+24
| set -uo pipefail | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| source "$SCRIPT_DIR/00-helpers.sh" | ||
|
|
||
| print_header "Cleanup NATS backup demo" | ||
|
|
||
| kubectl -n "$NAMESPACE" delete restorejob "$RESTOREJOB_TOCOPY_NAME" --ignore-not-found | ||
| kubectl -n "$NAMESPACE" delete restorejob "$RESTOREJOB_INPLACE_NAME" --ignore-not-found | ||
| kubectl -n "$NAMESPACE" delete backupjob "$BACKUPJOB_NAME" --ignore-not-found | ||
| kubectl -n "$NAMESPACE" delete backup "$BACKUPJOB_NAME" --ignore-not-found | ||
| kubectl -n "$NAMESPACE" delete secret "${NATS_RESTORE_NAME}-backup-s3" --ignore-not-found | ||
| kubectl -n "$NAMESPACE" delete secret "${NATS_NAME}-backup-s3" --ignore-not-found | ||
| kubectl -n "$NAMESPACE" delete nats "$NATS_RESTORE_NAME" --ignore-not-found | ||
| kubectl -n "$NAMESPACE" delete nats "$NATS_NAME" --ignore-not-found | ||
| kubectl -n "$NAMESPACE" delete bucket "$BUCKET_NAME" --ignore-not-found | ||
| rm -f "$SCRIPT_DIR/.bucket-info.env" | ||
| kubectl delete backupclass "$BACKUPCLASS_NAME" --ignore-not-found | ||
| kubectl delete job.strategy.backups.cozystack.io "$STRATEGY_NAME" --ignore-not-found | ||
|
|
||
| log_success "Cleanup complete." |
Contributor
There was a problem hiding this comment.
Cleanup can silently fail but still report success.
On Line [4], missing -e means command failures won’t abort; then Line [24] always prints success. This can leave resources undeleted while reporting “Cleanup complete.”
Suggested fix
- set -uo pipefail
+ set -euo pipefail📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| set -uo pipefail | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| source "$SCRIPT_DIR/00-helpers.sh" | |
| print_header "Cleanup NATS backup demo" | |
| kubectl -n "$NAMESPACE" delete restorejob "$RESTOREJOB_TOCOPY_NAME" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete restorejob "$RESTOREJOB_INPLACE_NAME" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete backupjob "$BACKUPJOB_NAME" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete backup "$BACKUPJOB_NAME" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete secret "${NATS_RESTORE_NAME}-backup-s3" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete secret "${NATS_NAME}-backup-s3" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete nats "$NATS_RESTORE_NAME" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete nats "$NATS_NAME" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete bucket "$BUCKET_NAME" --ignore-not-found | |
| rm -f "$SCRIPT_DIR/.bucket-info.env" | |
| kubectl delete backupclass "$BACKUPCLASS_NAME" --ignore-not-found | |
| kubectl delete job.strategy.backups.cozystack.io "$STRATEGY_NAME" --ignore-not-found | |
| log_success "Cleanup complete." | |
| set -euo pipefail | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| source "$SCRIPT_DIR/00-helpers.sh" | |
| print_header "Cleanup NATS backup demo" | |
| kubectl -n "$NAMESPACE" delete restorejob "$RESTOREJOB_TOCOPY_NAME" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete restorejob "$RESTOREJOB_INPLACE_NAME" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete backupjob "$BACKUPJOB_NAME" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete backup "$BACKUPJOB_NAME" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete secret "${NATS_RESTORE_NAME}-backup-s3" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete secret "${NATS_NAME}-backup-s3" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete nats "$NATS_RESTORE_NAME" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete nats "$NATS_NAME" --ignore-not-found | |
| kubectl -n "$NAMESPACE" delete bucket "$BUCKET_NAME" --ignore-not-found | |
| rm -f "$SCRIPT_DIR/.bucket-info.env" | |
| kubectl delete backupclass "$BACKUPCLASS_NAME" --ignore-not-found | |
| kubectl delete job.strategy.backups.cozystack.io "$STRATEGY_NAME" --ignore-not-found | |
| log_success "Cleanup complete." |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 7-7: Not following: ./00-helpers.sh was not specified as input (see shellcheck -x).
(SC1091)
🤖 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 `@examples/backups/nats/cleanup.sh` around lines 4 - 24, The script currently
uses set -uo pipefail (missing -e) so failures won’t abort and log_success at
the end can report success even if deletes failed; change the top options to
include -e (set -euo pipefail) so the script exits on any command error, and
ensure critical cleanup commands (the kubectl delete invocations that operate on
restorejob/backupjob/backup/nats/bucket/backupclass/job.strategy.backups.cozystack.io
and the rm) will cause the script to fail rather than reach
print_header/log_success; keep --ignore-not-found where intentional but rely on
-e for real failures so print_header and log_success reflect actual success.
Contributor
|
Andrey Kolkov (androndo)
approved these changes
May 27, 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
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.
What
Implements the
strategy.backups.cozystack.io/Jobbackup strategy — a generic, application-agnostic driver that runs a user-supplied Kubernetes Job to create (and restore) a backup.Background
This PR began as a draft written against an earlier shape of the backups API. The API has since moved to a
BackupClass-based dispatch model, which broke the original draft on three counts:BackupJob.Spec.StrategyRefandBackupJob.Spec.StorageRefwere removed; the strategy and its parameters are now resolved from aBackupClassinto aResolvedBackupConfig.BackupJobReconciler.Reconcile/RestoreJobReconciler.Reconcileresolve the strategy and switch into per-strategy methods (reconcileVelero,reconcileCNPG,reconcileAltinity, …)..Values,.Storage) diverged from the convention the other strategies adopted, and its.Storagehalf targeted a storage API that does not exist.What changed
reconcileJob/reconcileJobRestore(internal/backupcontroller/jobstrategy_controller.go): replaces the standaloneBackupJobStrategyReconcilerwith the two dispatch methods theBackupJob/RestoreJobreconcilers call. The Job strategy is the generic counterpart of the Altinity driver — same lifecycle (renderPodTemplateSpec→ run a one-shotbatch/v1.Job→ poll its terminal condition → emit aBackup), minus the ClickHouse-specificapplicationRefgate, so it runs for any application. Shared package helpers (renderJobTemplate,jobConditionState,getApplicationUnstructured, status helpers, deterministic Job naming, controller refs) are reused..Application(live application object),.Release.Name/.Release.Namespace,.Mode(backup/restore),.Parameters(resolvedBackupClassparameters), and.Backup(restore runs only). Strategy parameters round-trip through theBackup'sdriverMetadataso a later restore re-renders against the same values..Mode == "restore". Cross-namespace restore is unsupported (the RestoreJob target reference is namespace-local), matching the Altinity driver.JobSpec.Templatedoc + regenerated CRD: the field doc previously advertised.Values; it now documents the real context, andstrategy.backups.cozystack.io_jobs.yamlis regenerated to match (the change is limited to that field's description).jobstrategy_controller_test.go): backup-Job creation / args / labels / status, completion →Backupartifact with parameter round-trip, Job-failure surfacing, missing-strategy terminal failure, restore into the target namespace, restore completion, and the parameter-prefix unit. A deliberately generic application kind pins that the driver never gates onapplicationRef.kind.No RBAC changes are required: the controller already holds
strategy.backups.cozystack.io/*,batch/jobs,apps.cozystack.io/*, andbackupspermissions.Notes for reviewers
PodTemplateSpecmust branch on.Modeto do anything restore-specific. This matches the existing convention; worth confirming it's the intended contract for the Job strategy's docs/examples.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests