packages/system/backup-controller/templates/rbac.yaml (2)

24-26: Consider scoping secrets permissions.

The secrets permissions are quite broad (create/get/list/watch/update/patch across all namespaces). The controller creates secrets in the cozy-velero namespace. Consider if this could be scoped more narrowly using resourceNames or if there's a need for such broad access.

---

30-32: Consider adding delete permission for Velero resources.

The controller creates BackupStorageLocations, VolumeSnapshotLocations, and Secrets per BackupJob, but there's no cleanup mechanism visible. Consider whether delete permission should be added for garbage collection when BackupJobs are deleted, or document that these resources are intentionally retained.

internal/backupcontroller/velerostrategy_controller.go (3)

214-216: Use constant for requeue duration.

Line 216 uses a hardcoded 5 * time.Second while the rest of the file uses defaultRequeueAfter or defaultActiveJobPollingInterval constants.

🔎 Proposed fix

 	// Still in progress (InProgress, New, etc.)
-	return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
+	return ctrl.Result{RequeueAfter: defaultActiveJobPollingInterval}, nil
 }

---

381-396: Avoid blocking sleep in reconciler.

Using time.Sleep in a reconciler blocks the worker goroutine. For conflict retries, consider returning with requeue instead, or use a client with built-in retry. The same pattern appears in createVolumeSnapshotLocation (lines 424-439).

Alternatively, use a server-side apply or patch approach to avoid conflicts entirely:

// Example using Patch instead of Update to reduce conflicts
patch := client.MergeFrom(foundBSL.DeepCopy())
foundBSL.Spec = bsl.Spec
if err := r.Patch(ctx, foundBSL, patch); err != nil {
    return fmt.Errorf("failed to patch BackupStorageLocation: %w", err)
}

---

450-477: Consider deduplicating conditions.

The markBackupJobFailed function appends to conditions without checking for duplicates. If the reconciler retries after a transient error during status update, it could append multiple "Ready=False" conditions. Consider using a helper like meta.SetStatusCondition from k8s.io/apimachinery/pkg/api/meta.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1161
File: packages/apps/virtual-machine/templates/dashboard-resourcemap.yaml:6-12
Timestamp: 2025-07-10T12:14:27.197Z
Learning: Kubernetes RBAC rules with resourceNames work correctly for list/watch verbs. When resourceNames is specified in an RBAC rule, it properly restricts access to only those named resources, even for list and watch operations. Examples: `kubectl get resource resourcename -w` watches for changes on a single resource, and `kubectl get resource --field-selector .metadata.name=resourcename` lists a specific resource. The Kubernetes API server correctly distinguishes such requests from their less specific counterparts.

Learnt from: kvaps
Repo: cozystack/cozystack PR: 1760
File: packages/core/platform/sources/velero.yaml:20-23
Timestamp: 2025-12-25T15:24:47.730Z
Learning: In Velero PackageSource configurations, the privileged: true setting is required for node-agents to function properly with hostPath access for backup operations.

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1239
File: packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml:3-15
Timestamp: 2025-07-24T13:21:17.388Z
Learning: In COSI (Container Object Storage Interface), BucketAccessClass resources have fields like driverName and authenticationType at the top level, not under a spec block. Only BucketClass resources require a spec block for fields like driverName and deletionPolicy.

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to internal/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in internal directory

internal/backupcontroller/velerostrategy_controller.go (2)

204-205: Use the defined constant for consistency.

Line 205 hardcodes 5 * time.Second instead of using the defaultActiveJobPollingInterval constant defined at line 48.

🔎 Proposed fix

 	// Still in progress (InProgress, New, etc.)
-	return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
+	return ctrl.Result{RequeueAfter: defaultActiveJobPollingInterval}, nil

---

399-440: Consider extracting the retry logic to reduce duplication.

The retry logic in lines 416-431 is identical to lines 373-388 in createBackupStorageLocation. Consider extracting this into a generic helper function to improve maintainability.

💡 Example refactor

func (r *BackupJobReconciler) updateWithRetry(ctx context.Context, obj client.Object, updateSpec func() error) error {
	logger := log.FromContext(ctx)
	for i := 0; i < 3; i++ {
		key := client.ObjectKeyFromObject(obj)
		if err := r.Get(ctx, key, obj); err != nil {
			return fmt.Errorf("failed to get object for update: %w", err)
		}
		if err := updateSpec(); err != nil {
			if errors.IsConflict(err) && i < 2 {
				logger.V(1).Info("conflict updating object, retrying", "attempt", i+1)
				time.Sleep(100 * time.Millisecond)
				continue
			}
			return err
		}
		return nil
	}
	return fmt.Errorf("failed after 3 retry attempts")
}

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: kvaps
Repo: cozystack/cozystack PR: 1760
File: packages/core/platform/sources/velero.yaml:20-23
Timestamp: 2025-12-25T15:24:47.730Z
Learning: In Velero PackageSource configurations, the privileged: true setting is required for node-agents to function properly with hostPath access for backup operations.

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1239
File: packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml:3-15
Timestamp: 2025-07-24T13:21:17.388Z
Learning: In COSI (Container Object Storage Interface), BucketAccessClass resources have fields like driverName and authenticationType at the top level, not under a spec block. Only BucketClass resources require a spec block for fields like driverName and deletionPolicy.

internal/backupcontroller/velerostrategy_controller.go (1)

205-205: Use the defined constant for consistency.

This line uses a hardcoded 5 * time.Second while the rest of the file uses defaultRequeueAfter or defaultActiveJobPollingInterval constants. Use the constant for consistency and maintainability.

🔎 Proposed fix

-	return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
+	return ctrl.Result{RequeueAfter: defaultActiveJobPollingInterval}, nil

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: kvaps
Repo: cozystack/cozystack PR: 1760
File: packages/core/platform/sources/velero.yaml:20-23
Timestamp: 2025-12-25T15:24:47.730Z
Learning: In Velero PackageSource configurations, the privileged: true setting is required for node-agents to function properly with hostPath access for backup operations.

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1239
File: packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml:3-15
Timestamp: 2025-07-24T13:21:17.388Z
Learning: In COSI (Container Object Storage Interface), BucketAccessClass resources have fields like driverName and authenticationType at the top level, not under a spec block. Only BucketClass resources require a spec block for fields like driverName and deletionPolicy.

internal/backupcontroller/velerostrategy_controller.go (1)

633-637: Verify label selector matches deployed VirtualMachine instances.

The label selector assumes app.kubernetes.io/instance equals virtual-machine-{ApplicationRef.Name}. As flagged in previous reviews, if VirtualMachine Helm releases use {{ .Release.Name }} for this label, backups may miss VMs deployed with different release names.

Consider one of these approaches:

1. Enforce a naming convention that guarantees release name matches the pattern
2. Query the actual VirtualMachine resource to determine its labels at runtime
3. Use a more stable selector like app.kubernetes.io/name: "virtual-machine" combined with namespace filtering

Run the following script to verify the label pattern on deployed VirtualMachine resources:

#!/bin/bash
# Check if VirtualMachine resources in the cluster have the expected label pattern

echo "=== Checking VirtualMachine resources for label patterns ==="
kubectl get virtualmachines.kubevirt.io --all-namespaces -o json | \
  jq -r '.items[] | "\(.metadata.namespace)/\(.metadata.name) labels: \(.metadata.labels)"'

echo ""
echo "=== Checking for app.kubernetes.io/instance label values ==="
kubectl get virtualmachines.kubevirt.io --all-namespaces -o json | \
  jq -r '.items[] | select(.metadata.labels."app.kubernetes.io/instance" != null) | "\(.metadata.namespace)/\(.metadata.name): \(.metadata.labels."app.kubernetes.io/instance")"'

internal/backupcontroller/velerostrategy_controller.go (1)

233-233: Use the defined constant for consistency.

Line 233 uses a hardcoded 5 * time.Second for requeuing, but the constant defaultRequeueAfter is defined at the top of the file for this purpose. Use the constant for maintainability.

🔎 Proposed fix

-	return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
+	return ctrl.Result{RequeueAfter: defaultRequeueAfter}, nil

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: kvaps
Repo: cozystack/cozystack PR: 1760
File: packages/core/platform/sources/velero.yaml:20-23
Timestamp: 2025-12-25T15:24:47.730Z
Learning: In Velero PackageSource configurations, the privileged: true setting is required for node-agents to function properly with hostPath access for backup operations.

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1239
File: packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml:3-15
Timestamp: 2025-07-24T13:21:17.388Z
Learning: In COSI (Container Object Storage Interface), BucketAccessClass resources have fields like driverName and authenticationType at the top level, not under a spec block. Only BucketClass resources require a spec block for fields like driverName and deletionPolicy.

hack/e2e-apps/backup.bats (3)

26-30: Consider using mktemp for temporary credential files.

Writing to a fixed path /tmp/bucket-backup-credentials.json can cause race conditions in parallel test runs and leaves credentials in a predictable location.

🔎 Proposed fix

+  CRED_FILE=$(mktemp /tmp/bucket-backup-credentials.XXXXXX.json)
+  trap "rm -f $CRED_FILE" EXIT
+
   # Get bucket credentials for later S3 verification
-  kubectl -n ${namespace} get secret bucket-${bucket_name} -ojsonpath='{.data.BucketInfo}' | base64 -d > /tmp/bucket-backup-credentials.json
-  ACCESS_KEY=$(jq -r '.spec.secretS3.accessKeyID' /tmp/bucket-backup-credentials.json)
-  SECRET_KEY=$(jq -r '.spec.secretS3.accessSecretKey' /tmp/bucket-backup-credentials.json)
-  BUCKET_NAME=$(jq -r '.spec.bucketName' /tmp/bucket-backup-credentials.json)
-  ENDPOINT=$(jq -r '.spec.secretS3.endpoint' /tmp/bucket-backup-credentials.json)
+  kubectl -n ${namespace} get secret bucket-${bucket_name} -ojsonpath='{.data.BucketInfo}' | base64 -d > "$CRED_FILE"
+  ACCESS_KEY=$(jq -r '.spec.secretS3.accessKeyID' "$CRED_FILE")
+  SECRET_KEY=$(jq -r '.spec.secretS3.accessSecretKey' "$CRED_FILE")
+  BUCKET_NAME=$(jq -r '.spec.bucketName' "$CRED_FILE")
+  ENDPOINT=$(jq -r '.spec.secretS3.endpoint' "$CRED_FILE")

---

121-127: Consider using jq for more robust JSON parsing.

Word splitting on kubectl jsonpath output works for K8s resource names but is fragile. Using jq with a proper array iteration would be more robust and handle edge cases better.

🔎 Proposed fix

   # Find backup by pattern matching namespace-backupjob
-  for backup in $(kubectl -n cozy-velero get backups.velero.io -o jsonpath='{.items[*].metadata.name}' 2>/dev/null); do
-    if echo "$backup" | grep -q "^${namespace}-${backupjob_name}-"; then
-      VELERO_BACKUP_NAME=$backup
-      VELERO_BACKUP_PHASE=$(kubectl -n cozy-velero get backups.velero.io $backup -o jsonpath='{.status.phase}' 2>/dev/null || echo "")
-      break
-    fi
-  done
+  VELERO_BACKUP_NAME=$(kubectl -n cozy-velero get backups.velero.io -o json 2>/dev/null | \
+    jq -r --arg pattern "^${namespace}-${backupjob_name}-" \
+    '.items[].metadata.name | select(test($pattern))' | head -1)

---

133-141: Consider adding explicit failure handling for better test diagnostics.

The timeout correctly waits for either Completed or Failed, but if the backup fails, the test will fail silently at the assertion. Adding a check for the Failed state with a diagnostic message would help debug issues.

🔎 Proposed improvement

   # Verify Velero Backup is Completed
   VELERO_BACKUP_PHASE=$(kubectl -n cozy-velero get backups.velero.io ${VELERO_BACKUP_NAME} -o jsonpath='{.status.phase}')
+  if [ "$VELERO_BACKUP_PHASE" = "Failed" ]; then
+    echo "Velero backup failed. Failure reason:"
+    kubectl -n cozy-velero get backups.velero.io ${VELERO_BACKUP_NAME} -o jsonpath='{.status.failureReason}'
+    false
+  fi
   [ "$VELERO_BACKUP_PHASE" = "Completed" ]

Configuration used: defaults

Review profile: CHILL

Plan: Pro

internal/backupcontroller/velerostrategy_controller.go (1)

633-637: Verify the label selector matches VirtualMachine deployments.

The label selector assumes VMs have app.kubernetes.io/instance: virtual-machine-{ApplicationRef.Name}. This works for Cozystack's Helm-deployed VMs but may miss VMs deployed differently.

This was flagged in a previous review. The current implementation follows Cozystack's Helm naming conventions, but consider documenting this assumption or adding validation that the target VM exists with the expected label.

internal/backupcontroller/velerostrategy_controller.go (5)

232-234: Use the defined constant for requeue duration.

Line 233 uses a hardcoded 5 * time.Second instead of the already-defined defaultActiveJobPollingInterval constant.

🔎 Proposed fix

 	// Still in progress (InProgress, New, etc.)
-	return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
+	return ctrl.Result{RequeueAfter: defaultActiveJobPollingInterval}, nil
 }

---

247-251: Consider extracting the hardcoded API version.

The version "v1alpha1" is hardcoded. If the Bucket API evolves, this could become a maintenance burden.

---

381-387: Consider using bytes.Equal instead of reflect.DeepEqual for byte slices.

For comparing byte slices, bytes.Equal is more idiomatic and slightly more efficient than reflect.DeepEqual.

🔎 Proposed fix

+import "bytes"
+
 			for k, newVal := range newData {
 				existingVal, exists := existingData[k]
-				if !exists || !reflect.DeepEqual(existingVal, newVal) {
+				if !exists || !bytes.Equal(existingVal, newVal) {
 					dataChanged = true
 					break
 				}

---

506-512: Consider using meta.SetStatusCondition for idempotent condition updates.

Appending conditions directly could lead to duplicates if this function is called multiple times before the status update succeeds. Using meta.SetStatusCondition from k8s.io/apimachinery/pkg/api/meta ensures idempotent updates.

🔎 Proposed fix

+import "k8s.io/apimachinery/pkg/api/meta"
+
 	// Add condition
-	backupJob.Status.Conditions = append(backupJob.Status.Conditions, metav1.Condition{
+	meta.SetStatusCondition(&backupJob.Status.Conditions, metav1.Condition{
 		Type:               "Ready",
 		Status:             metav1.ConditionFalse,
 		Reason:             "BackupFailed",
 		Message:            message,
 		LastTransitionTime: now,
 	})

---

706-708: Remove redundant nil check.

The artifact variable is unconditionally assigned on line 658, so this nil check will always be true.

🔎 Proposed fix

 	if backupJob.Spec.PlanRef != nil {
 		backup.Spec.PlanRef = backupJob.Spec.PlanRef
 	}

-	if artifact != nil {
-		backup.Status.Artifact = artifact
-	}
+	backup.Status.Artifact = artifact

 	if err := r.Create(ctx, backup); err != nil {

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to go.mod : Do NOT modify go.mod manually; use `go get` command instead

Learnt from: kvaps
Repo: cozystack/cozystack PR: 1760
File: packages/core/platform/sources/velero.yaml:20-23
Timestamp: 2025-12-25T15:24:47.730Z
Learning: In Velero PackageSource configurations, the privileged: true setting is required for node-agents to function properly with hostPath access for backup operations.

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1239
File: packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml:3-15
Timestamp: 2025-07-24T13:21:17.388Z
Learning: In COSI (Container Object Storage Interface), BucketAccessClass resources have fields like driverName and authenticationType at the top level, not under a spec block. Only BucketClass resources require a spec block for fields like driverName and deletionPolicy.

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1161
File: packages/apps/virtual-machine/templates/dashboard-resourcemap.yaml:6-12
Timestamp: 2025-07-10T12:14:27.197Z
Learning: Kubernetes RBAC rules with resourceNames work correctly for list/watch verbs. When resourceNames is specified in an RBAC rule, it properly restricts access to only those named resources, even for list and watch operations. Examples: `kubectl get resource resourcename -w` watches for changes on a single resource, and `kubectl get resource --field-selector .metadata.name=resourcename` lists a specific resource. The Kubernetes API server correctly distinguishes such requests from their less specific counterparts.

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to internal/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in internal directory

hack/e2e-apps/backup.bats (2)

17-17: pkill pattern may terminate unrelated port-forwards.

The pattern kubectl.*port-forward.*seaweedfs-s3 could inadvertently kill port-forwards from other tests or sessions. Consider storing the PID when the port-forward starts (line 207) and using it here for precise cleanup.

🔎 Proposed fix

+# Track port-forward PID at file scope for teardown
+PORT_FORWARD_PID=""
+
 teardown() {
   # Clean up resources (runs even if test fails)
   # ...
   
-  # Clean up port-forward if still running
-  pkill -f "kubectl.*port-forward.*seaweedfs-s3" 2>/dev/null || true
+  # Clean up port-forward if PID is known
+  if [ -n "$PORT_FORWARD_PID" ]; then
+    kill $PORT_FORWARD_PID 2>/dev/null || true
+    wait $PORT_FORWARD_PID 2>/dev/null || true
+  fi

Then at line 206-207, capture the PID:

 kubectl -n tenant-root port-forward service/seaweedfs-s3 8333:8333 > /dev/null 2>&1 &
-PORT_FORWARD_PID=$!
+PORT_FORWARD_PID=$!  # Now accessible in teardown

---

194-202: Timeout loop succeeds on "Failed" status.

The grep pattern 'Completed\|Failed' at line 195 will exit the loop successfully even if the backup reaches "Failed" status. While line 201 will correctly fail the test, the timeout message could be misleading.

🔎 Proposed refinement

   # Wait for Velero Backup to complete (with timeout)
   timeout 300 sh -ec "
-    until kubectl -n cozy-velero get backups.velero.io ${VELERO_BACKUP_NAME} -o jsonpath='{.status.phase}' | grep -q 'Completed\|Failed'; do
+    until kubectl -n cozy-velero get backups.velero.io ${VELERO_BACKUP_NAME} -o jsonpath='{.status.phase}' | grep -q 'Completed'; do
+      # Exit early if backup failed
+      if kubectl -n cozy-velero get backups.velero.io ${VELERO_BACKUP_NAME} -o jsonpath='{.status.phase}' | grep -q 'Failed'; then
+        echo 'Backup failed' >&2
+        exit 1
+      fi
       sleep 5
     done
   "

Configuration used: defaults

Review profile: CHILL

Plan: Pro

hack/e2e-apps/backup.bats (1)

213-215: Fix S3 endpoint protocol: use HTTP instead of HTTPS.

Line 215 configures the MinIO client with https://localhost:8333, but the SeaweedFS S3 service listens only on HTTP (port 8333 with httpsPort: 0 disabled in configuration). The MC_INSECURE=1 flag only skips certificate validation for HTTPS; it cannot resolve a protocol mismatch. Change line 215 to use http://:

Suggested change

   # Set up MinIO client with insecure flag (use environment variable for all commands)
   export MC_INSECURE=1
-  mc alias set local https://localhost:8333 $ACCESS_KEY $SECRET_KEY
+  mc alias set local http://localhost:8333 $ACCESS_KEY $SECRET_KEY

hack/e2e-apps/backup.bats (1)

177-177: Consider replacing arbitrary sleeps with polling loops for more reliable tests.

Lines 177 and 211 use fixed 30-second sleeps to wait for asynchronous operations. These arbitrary delays can make tests unnecessarily slow (if the operation completes faster) or flaky (if the operation takes longer than 30 seconds). Polling loops with timeouts provide more reliable and efficient waiting.

🔎 Example polling pattern for line 177

-  print_log "Wait a bit for the backup to be created and appear in the API"
-  sleep 30
+  print_log "Wait for backup to appear in API"
+  timeout 60 sh -ec "until kubectl -n cozy-velero get backups.velero.io 2>/dev/null | grep -q '^${namespace}-${backupjob_name}-'; do sleep 2; done"

For line 211, consider polling the S3 backup path instead:

-  # Wait a bit for backup data to be written to S3
-  sleep 30
+  # Poll for backup data in S3
+  timeout 60 sh -ec "until mc ls local/${BUCKET_NAME}/backups/ 2>/dev/null | grep -q '${VELERO_BACKUP_NAME}'; do sleep 5; done"

Also applies to: 211-211

Configuration used: defaults

Review profile: CHILL

Plan: Pro

internal/template/template.go (2)

39-56: LGTM! Helper correctly traverses JSON structures.

The mapAtStrings function correctly handles the recursive traversal of maps, slices, and strings. Non-string types (numbers, booleans, null) appropriately pass through unchanged.

Note: The function mutates the input structures in place, which is efficient but could be surprising. Consider documenting this behavior if the function becomes public.

---

58-68: LGTM! Standard text/template usage.

The template function correctly uses Go's text/template package for string templating. Error handling is appropriate.

If additional template functions are needed in the future (e.g., {{ lower .Name }}), consider using .Funcs(template.FuncMap{...}) before parsing.

internal/template/template_test.go (2)

65-67: Use structured comparison instead of JSON string comparison.

Comparing JSON-marshaled strings (lines 65-66) is fragile—field ordering or whitespace differences could cause false failures even when the objects are semantically equal.

🔎 Proposed fix using reflect.DeepEqual or cmp.Diff

-	if string(b1) != string(b2) {
-		t.Fatalf("transformed value not equal to reference value, expected: %s, got: %s", string(b1), string(b2))
+	if !reflect.DeepEqual(reference, *got) {
+		t.Fatalf("transformed value not equal to reference value\nexpected: %+v\ngot: %+v", reference, *got)
 	}

Or use github.com/google/go-cmp/cmp:

+	if diff := cmp.Diff(reference, *got); diff != "" {
+		t.Fatalf("transformed value mismatch (-want +got):\n%s", diff)
+	}

---

11-68: Expand test coverage to include error cases and edge cases.

The test only covers the happy path. Consider adding tests for:

• Template parsing errors (malformed syntax: {{ .Missing)
• Template execution errors (missing keys: {{ .Undefined }})
• Empty input objects
• Nil template context
• Non-templated strings (pass-through behavior)

internal/backupcontroller/velerostrategy_controller.go (2)

218-218: Use constant instead of hardcoded duration.

Line 218 uses a hardcoded 5 * time.Second instead of the defined constant defaultRequeueAfter or defaultActiveJobPollingInterval. Use the constant for consistency with the rest of the file.

🔎 Proposed fix

 	// Still in progress (InProgress, New, etc.)
-	return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
+	return ctrl.Result{RequeueAfter: defaultActiveJobPollingInterval}, nil

---

140-166: Consider handling multiple backups in list.

While the labels should ensure only one Velero Backup exists per BackupJob, there's no handling or logging if len(veleroBackupList.Items) > 1. Consider adding a debug or warning log to catch unexpected cases where multiple backups share the same labels (e.g., due to bugs or manual intervention).

🔎 Proposed enhancement

 	if len(veleroBackupList.Items) == 0 {
 		// Create Velero Backup
 		...
 	} else {
+		if len(veleroBackupList.Items) > 1 {
+			logger.V(1).Info("found multiple Velero Backups for BackupJob, using first one", 
+				"count", len(veleroBackupList.Items))
+		}
 		// Assign the first backup from the list
 		veleroBackup = &veleroBackupList.Items[0]
 	}

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: kvaps
Repo: cozystack/cozystack PR: 1760
File: packages/core/platform/sources/velero.yaml:20-23
Timestamp: 2025-12-25T15:24:47.730Z
Learning: In Velero PackageSource configurations, the privileged: true setting is required for node-agents to function properly with hostPath access for backup operations.

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1239
File: packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml:3-15
Timestamp: 2025-07-24T13:21:17.388Z
Learning: In COSI (Container Object Storage Interface), BucketAccessClass resources have fields like driverName and authenticationType at the top level, not under a spec block. Only BucketClass resources require a spec block for fields like driverName and deletionPolicy.

hack/e2e-apps/backup.bats (3)

213-215: Verify HTTPS/HTTP compatibility with SeaweedFS S3.

Using https://localhost:8333 assumes SeaweedFS S3 serves HTTPS. If it serves HTTP, the connection will fail even with MC_INSECURE=1 (which only skips certificate validation, not protocol mismatch).

#!/bin/bash
# Check what protocol SeaweedFS S3 service uses
kubectl get svc seaweedfs-s3 -n tenant-root -o yaml | grep -A10 'ports:'
# Check if there's TLS configuration
kubectl get svc seaweedfs-s3 -n tenant-root -o jsonpath='{.spec.ports[*].name}'

---

157-161: Race condition: waiting for transient Running phase may cause flaky failures.

If the BackupJob transitions quickly from creation to Succeeded (or skips Running entirely), the wait on line 158 will timeout even though the backup succeeded. The Succeeded wait at line 161 is sufficient.

🔎 Proposed fix

-  print_log "Wait for BackupJob to start"
-  kubectl -n ${namespace} wait backupjob ${backupjob_name} --timeout=60s --for=jsonpath='{.status.phase}'=Running
-
   print_log "Wait for BackupJob to complete"
   kubectl -n ${namespace} wait backupjob ${backupjob_name} --timeout=300s --for=jsonpath='{.status.phase}'=Succeeded

---

191-197: Redundant wait logic with potential infinite loop.

Lines 191-194 contain an unbounded until loop, but line 197 duplicates the wait with a proper timeout. Either remove the unbounded loop or add a timeout to it.

🔎 Proposed fix — remove redundant unbounded loop

   print_log "Verify Velero Backup was found"
   [ -n "$VELERO_BACKUP_NAME" ]
   
-  echo '# Wait for Velero Backup to complete' >&3
-  until kubectl -n cozy-velero get backups.velero.io ${VELERO_BACKUP_NAME} -o jsonpath='{.status.phase}' | grep -q 'Completed\|Failed'; do
-    sleep 5
-  done
-
   print_log "Verify Velero Backup is Completed"
   timeout 90 sh -ec "until [ \"\$(kubectl -n cozy-velero get backups.velero.io ${VELERO_BACKUP_NAME} -o jsonpath='{.status.phase}' 2>/dev/null)\" = \"Completed\" ]; do sleep 30; done"

internal/backupcontroller/velerostrategy_controller.go (2)

145-166: Critical bug: veleroBackup is never assigned from the list when backups exist.

When len(veleroBackupList.Items) > 0, the code proceeds to use veleroBackup (lines 166, 180, 190, 211) but it remains an uninitialized empty struct from line 130. This causes incorrect phase checks and invalid data passed to createBackupResource.

🔎 Proposed fix

 	if len(veleroBackupList.Items) == 0 {
 		// Create Velero Backup
 		logger.Debug("Velero Backup not found, creating new one")
 		if err := r.createVeleroBackup(ctx, j, veleroStrategy); err != nil {
 			logger.Error(err, "failed to create Velero Backup")
 			return r.markBackupJobFailed(ctx, j, fmt.Sprintf("failed to create Velero Backup: %v", err))
 		}
 		// After successful Velero backup creation, set phase to Running
 		if j.Status.Phase != backupsv1alpha1.BackupJobPhaseRunning {
 			logger.Debug("setting BackupJob phase to Running after successful Velero backup creation")
 			j.Status.Phase = backupsv1alpha1.BackupJobPhaseRunning
 			if err := r.Status().Update(ctx, j); err != nil {
 				logger.Error(err, "failed to update BackupJob phase to Running")
 				return ctrl.Result{}, err
 			}
 		}
 		logger.Debug("created Velero Backup, requeuing", "veleroBackupName", veleroBackupName)
 		// Requeue to check status
 		return ctrl.Result{RequeueAfter: defaultRequeueAfter}, nil
 	}
+
+	// Assign the backup from the list
+	veleroBackup = &veleroBackupList.Items[0]
 
 	logger.Debug("found existing Velero Backup", "veleroBackupName", veleroBackupName, "phase", veleroBackup.Status.Phase)

---

519-524: Missing error check after fetching application resource.

The error from r.Resource(...).Get() at line 519 is not checked before using app at line 521. If the application doesn't exist, app will be nil and the template call will fail with an unclear error.

🔎 Proposed fix

 	app, err := r.Resource(mapping.Resource).Namespace(ns).Get(ctx, backupJob.Spec.ApplicationRef.Name, metav1.GetOptions{})
+	if err != nil {
+		return fmt.Errorf("failed to get application resource %s/%s: %w", backupJob.Spec.ApplicationRef.Kind, backupJob.Spec.ApplicationRef.Name, err)
+	}
 
 	veleroBackupSpec, err := template.Template(&strategy.Spec.Template.Spec, app.Object)

packages/system/backup-controller/templates/rbac.yaml (1)

24-26: Missing delete permission for secrets cleanup.

The controller creates secrets in the cozy-velero namespace (via createS3CredsForVelero), but without delete permission, orphaned secrets will accumulate when BackupJobs are deleted. Cross-namespace owner references cannot be used for automatic garbage collection.

🔎 Proposed fix

 - apiGroups: [""]
   resources: ["secrets"]
-  verbs: ["create", "get", "list", "watch", "update", "patch"]
+  verbs: ["create", "get", "list", "watch", "update", "patch", "delete"]

internal/template/template_test.go (1)

11-68: Consider adding edge case coverage and error handling tests.

The test validates basic templating functionality correctly, but consider adding:

• Tests for malformed templates (e.g., {{ .NonExistent }})
• Tests for empty strings and nil values
• Tests for special characters in template values
• Table-driven tests to cover multiple scenarios efficiently

The JSON string comparison approach is acceptable but be aware it relies on deterministic field ordering.

api/backups/v1alpha1/backupjob_types.go (1)

24-27: Consider reviewing the label naming convention.

The label keys use an unconventional format with CamelCase components:

• backups.cozystack.io/owned-by.BackupJobName
• backups.cozystack.io/owned-by.BackupJobNamespace

Kubernetes label keys typically use lowercase with hyphens. Consider whether this format:

1. Follows existing conventions in your codebase
2. Should use a format like backups.cozystack.io/owned-by-backupjob-name instead
3. Might create confusion between the "owned-by" and "BackupJobName" portions

If this format is intentional and consistent with your project's conventions, this comment can be ignored.

internal/backupcontroller/velerostrategy_controller.go (4)

217-218: Use constant instead of hardcoded value.

Line 218 uses 5 * time.Second directly, but defaultActiveJobPollingInterval is defined for this purpose.

🔎 Proposed fix

 	// Still in progress (InProgress, New, etc.)
-	return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
+	return ctrl.Result{RequeueAfter: defaultActiveJobPollingInterval}, nil

---

575-578: Simplify unnecessary fmt.Sprintf.

fmt.Sprintf("%s", backupJob.Name) is redundant—use backupJob.Name directly.

🔎 Proposed fix

 	backup := &backupsv1alpha1.Backup{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      fmt.Sprintf("%s", backupJob.Name),
+			Name:      backupJob.Name,
 			Namespace: backupJob.Namespace,

---

605-607: Remove unnecessary nil check.

artifact is always non-nil since it's unconditionally assigned at line 557. This check is dead code.

🔎 Proposed fix

-	if artifact != nil {
-		backup.Status.Artifact = artifact
-	}
+	backup.Status.Artifact = artifact

---

396-438: Good retry pattern for conflict handling.

The retry loops with conflict detection for BackupStorageLocation and VolumeSnapshotLocation updates handle concurrent status updates from Velero correctly. Consider extracting the common retry logic into a helper function to reduce duplication.

Also applies to: 440-481

packages/system/backup-controller/templates/rbac.yaml (1)

30-32: Consider adding delete permission for Velero resources.

For cleanup consistency, you may want delete on Velero resources if the controller should clean up orphaned BackupStorageLocations, VolumeSnapshotLocations, or Velero Backups when BackupJobs are removed.

🔎 Proposed fix

 - apiGroups: ["velero.io"]
   resources: ["backups", "backupstoragelocations", "volumesnapshotlocations", "restores"]
-  verbs: ["create", "get", "list", "watch", "update", "patch"]
+  verbs: ["create", "get", "list", "watch", "update", "patch", "delete"]

hack/e2e-apps/backup.bats (1)

127-131: Consider removing the arbitrary sleep 5.

The sleep 5 on line 127 before waiting for resources is likely unnecessary if the subsequent wait commands have proper timeouts. If needed for HelmRelease propagation, add a comment explaining the reason.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: kvaps
Repo: cozystack/cozystack PR: 1760
File: packages/core/platform/sources/velero.yaml:20-23
Timestamp: 2025-12-25T15:24:47.730Z
Learning: In Velero PackageSource configurations, the privileged: true setting is required for node-agents to function properly with hostPath access for backup operations.

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1239
File: packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml:3-15
Timestamp: 2025-07-24T13:21:17.388Z
Learning: In COSI (Container Object Storage Interface), BucketAccessClass resources have fields like driverName and authenticationType at the top level, not under a spec block. Only BucketClass resources require a spec block for fields like driverName and deletionPolicy.

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1161
File: packages/apps/virtual-machine/templates/dashboard-resourcemap.yaml:6-12
Timestamp: 2025-07-10T12:14:27.197Z
Learning: Kubernetes RBAC rules with resourceNames work correctly for list/watch verbs. When resourceNames is specified in an RBAC rule, it properly restricts access to only those named resources, even for list and watch operations. Examples: `kubectl get resource resourcename -w` watches for changes on a single resource, and `kubectl get resource --field-selector .metadata.name=resourcename` lists a specific resource. The Kubernetes API server correctly distinguishes such requests from their less specific counterparts.

hack/e2e-apps/backup.bats.disabled (1)

157-161: Race condition: waiting for transient Running phase may cause flaky test failures.

The test waits for phase=Running (line 158) before waiting for phase=Succeeded (line 161). If the BackupJob transitions quickly or directly to Succeeded (faster than the polling interval), the Running wait will timeout even though the backup succeeded.

🔎 Proposed fix

-  print_log "Wait for BackupJob to start"
-  kubectl -n ${namespace} wait backupjob ${backupjob_name} --timeout=60s --for=jsonpath='{.status.phase}'=Running
-
   print_log "Wait for BackupJob to complete"
   kubectl -n ${namespace} wait backupjob ${backupjob_name} --timeout=300s --for=jsonpath='{.status.phase}'=Succeeded

internal/template/template.go (1)

19-25: Template errors are silently swallowed.

The templateFunc closure returns the original input string when template() fails (line 22), hiding template parsing or execution errors from the caller. Users will see unexpanded template syntax (e.g., {{ .Missing }}) without any indication that an error occurred, making debugging extremely difficult.

🔎 Proposed fix to propagate template errors

Collect errors during traversal and return them:

+	var templateErrors []error
 	templateFunc := func(in string) string {
 		out, err := template(in, templateContext)
 		if err != nil {
+			templateErrors = append(templateErrors, fmt.Errorf("template error in %q: %w", in, err))
 			return in
 		}
 		return out
 	}
 	unstructured = mapAtStrings(unstructured, templateFunc)
+	if len(templateErrors) > 0 {
+		return nil, fmt.Errorf("template errors: %v", templateErrors)
+	}
 	b, err = json.Marshal(unstructured)

Note: Requires importing "fmt" at the top of the file.

hack/e2e-apps/backup.bats.disabled (2)

191-201: Redundant wait logic creates confusion.

Lines 191-194 implement an until loop without an explicit timeout, while line 197 adds a separate timeout wrapper that also waits for Completed. This creates redundant and confusing wait logic.

🔎 Proposed fix to consolidate wait logic

-  echo '# Wait for Velero Backup to complete' >&3
-  until kubectl -n cozy-velero get backups.velero.io ${VELERO_BACKUP_NAME} -o jsonpath='{.status.phase}' | grep -q 'Completed\|Failed'; do
-    sleep 5
-  done
-
   print_log "Verify Velero Backup is Completed"
   timeout 90 sh -ec "until [ \"\$(kubectl -n cozy-velero get backups.velero.io ${VELERO_BACKUP_NAME} -o jsonpath='{.status.phase}' 2>/dev/null)\" = \"Completed\" ]; do sleep 30; done"

---

204-208: Port-forward background process may leak.

The timeout 100s wrapper spawns the port-forward in the background but relies on teardown's pkill (line 17) to clean it up. If the test script exits abnormally, the process may leak.

🔎 Proposed improvement using a subshell trap

-  bash -c 'timeout 100s kubectl port-forward service/seaweedfs-s3 -n tenant-root 8333:8333 > /dev/null 2>&1 &'
+  (trap 'kill $PORT_FWD_PID 2>/dev/null || true' EXIT
+   kubectl port-forward service/seaweedfs-s3 -n tenant-root 8333:8333 > /dev/null 2>&1 &
+   PORT_FWD_PID=$!
+   sleep 100
+  ) &

Alternatively, store the PID and ensure cleanup in teardown.

internal/template/template.go (1)

9-37: Consider adding documentation for the Template function.

The generic Template function performs complex operations (JSON marshaling, recursive string transformation, unmarshaling). Adding a doc comment would help users understand its behavior, especially regarding error handling and the template context format.

Example documentation

// Template applies text/template transformations to all string fields in obj using templateContext.
// It marshals obj to JSON, recursively applies template substitutions to string values,
// and unmarshals back to type T. Template syntax errors in individual strings currently
// result in the original string being preserved (see known limitation).
//
// Parameters:
//   - obj: pointer to the object to template
//   - templateContext: variables available to templates (e.g., map[string]any{"name": "foo"})
//
// Returns a new templated object of type T, or an error if marshaling/unmarshaling fails.
func Template[T any](obj *T, templateContext map[string]any) (*T, error) {

internal/backupcontroller/backupjob_controller.go (1)

25-31: Consider naming the embedded interfaces for clarity.

Embedding dynamic.Interface and meta.RESTMapper directly works but can be confusing for readers. Named fields would improve clarity and make the struct self-documenting.

🔎 Proposed refactor

 type BackupJobReconciler struct {
 	client.Client
-	dynamic.Interface
-	meta.RESTMapper
+	DynamicClient dynamic.Interface
+	RESTMapper    meta.RESTMapper
 	Scheme   *runtime.Scheme
 	Recorder record.EventRecorder
 }

internal/backupcontroller/velerostrategy_controller.go (4)

226-226: Use the defined constant for requeue duration.

Line 226 uses a hardcoded 5 * time.Second instead of the defaultActiveJobPollingInterval constant defined at line 67. This was noted in past reviews.

🔎 Proposed fix

 	// Still in progress (InProgress, New, etc.)
-	return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
+	return ctrl.Result{RequeueAfter: defaultActiveJobPollingInterval}, nil
 }

---

498-505: Consider using meta.SetStatusCondition for idempotent condition updates.

Appending to Conditions directly could create duplicates if this function is called multiple times due to update failures. Using meta.SetStatusCondition from k8s.io/apimachinery/pkg/api/meta ensures idempotent updates.

🔎 Proposed fix

+	apimeta "k8s.io/apimachinery/pkg/api/meta"
 ...
 	// Add condition
-	backupJob.Status.Conditions = append(backupJob.Status.Conditions, metav1.Condition{
+	apimeta.SetStatusCondition(&backupJob.Status.Conditions, metav1.Condition{
 		Type:               "Ready",
 		Status:             metav1.ConditionFalse,
 		Reason:             "BackupFailed",
 		Message:            message,
 		LastTransitionTime: now,
 	})

---

586-589: Simplify redundant string formatting.

fmt.Sprintf("%s", backupJob.Name) is equivalent to just backupJob.Name.

🔎 Proposed fix

 	backup := &backupsv1alpha1.Backup{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      fmt.Sprintf("%s", backupJob.Name),
+			Name:      backupJob.Name,
 			Namespace: backupJob.Namespace,

---

616-618: Remove unnecessary nil check.

The artifact variable is always non-nil as it's assigned at line 568. This check is redundant.

🔎 Proposed fix

 	if backupJob.Spec.PlanRef != nil {
 		backup.Spec.PlanRef = backupJob.Spec.PlanRef
 	}
 
-	if artifact != nil {
-		backup.Status.Artifact = artifact
-	}
+	backup.Status.Artifact = artifact
 
 	if err := r.Create(ctx, backup); err != nil {

Configuration used: defaults

Review profile: CHILL

Plan: Pro