Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a cluster-scoped BackupClass API; removes inline StorageRef/StrategyRef from Plan/Backup/BackupJob and replaces them with backupClassName; normalizes ApplicationRef API group; adds a resolver returning ResolvedBackupConfig (StrategyRef + Parameters) that is threaded through reconciliation and Velero template rendering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant JobCtrl as BackupJob Controller
participant Resolver as BackupClass Resolver
participant API as Kubernetes API
participant Strategy as Strategy Driver/Controller
JobCtrl->>Resolver: ResolveBackupClass(backupClassName, applicationRef)
Resolver->>Resolver: NormalizeApplicationRef(applicationRef)
Resolver->>API: Get BackupClass(backupClassName)
API-->>Resolver: BackupClass{spec.strategies}
Resolver->>Resolver: Match strategy by APIGroup + Kind
Resolver-->>JobCtrl: ResolvedBackupConfig{StrategyRef, Parameters}
JobCtrl->>Strategy: reconcile(job, resolved)
Strategy->>Strategy: Render templates using Application + Parameters
Strategy-->>JobCtrl: Result / Created resources / Error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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
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 |
Summary of ChangesHello @androndo, 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 significantly refactors the backup API by introducing a 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. 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}
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed refactoring of the backup API. The introduction of the BackupClass CRD is a great design choice that decouples backup strategies and storage configuration from the BackupJob and Plan resources, making the system more flexible and maintainable. The changes are consistent across the API types, controllers, and new tests have been added to cover the new logic. I've identified a few areas for improvement, mainly concerning code duplication and clarity, which should be straightforward to address.
androndo
force-pushed
the
feat/backups-rework
branch
from
754ddfb to
6c11dff
Compare
dosubot
bot
added
size:XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/backups/v1alpha1/plan_types.go (1)
31-34: Typo in comment: "Condtions" should be "Conditions".Proposed fix
-// Condtions +// Conditions const ( PlanConditionError = "Error" )internal/backupcontroller/plan_controller.go (1)
53-62: ConditionReasonshould be CamelCase without spaces.Per Kubernetes API conventions, the
Reasonfield should be a single CamelCase word. "Failed to parse cron spec" contains spaces and should be reformatted.Proposed fix
meta.SetStatusCondition(&p.Status.Conditions, metav1.Condition{ Type: backupsv1alpha1.PlanConditionError, Status: metav1.ConditionTrue, - Reason: "Failed to parse cron spec", + Reason: "InvalidCronSpec", Message: errWrapped.Error(), })internal/backupcontroller/velerostrategy_controller.go (1)
299-327: Artifact URI should use the Velero backup's namespace, not the BackupJob's namespace.Velero backups are created in the
cozy-veleronamespace (seeveleroNamespaceconstant at line 47 and thecreateVeleroBackupfunction), but the artifact URI at line 303 usesbackupJob.Namespace. This causes the URI to point to a namespace that will never contain the Velero Backup. ThedriverMetadataalready correctly capturesveleroBackup.Namespace, so the URI should do the same.Proposed fix
- artifact := &backupsv1alpha1.BackupArtifact{ - URI: fmt.Sprintf("velero://%s/%s", backupJob.Namespace, veleroBackup.Name), - } + artifact := &backupsv1alpha1.BackupArtifact{ + URI: fmt.Sprintf("velero://%s/%s", veleroBackup.Namespace, veleroBackup.Name), + }
♻️ Duplicate comments (2)
internal/backupcontroller/factory/backupjob.go (1)
12-26: Consolidate ApplicationRef normalization to avoid drift.This duplicates the default API group and normalization logic from the backupcontroller package. Consider moving the shared constant/function into a small common package (or API package) so both callers use a single source of truth without introducing cycles.
internal/backupcontroller/backupclass_resolver_test.go (1)
334-337: Strengthen StrategyRef assertions (Kind/Name/APIGroup).Right now the test only checks
Kind. Adding expectedNameandAPIGroupassertions would make the resolution tests more robust and prevent false positives.
🧹 Nitpick comments (4)
api/backups/v1alpha1/backupclass_types.go (1)
69-78: Consider adding validation for theKindfield.The
Kindfield is required but has no validation to prevent empty strings. Consider adding a minimum length validation.Proposed enhancement
// ApplicationSelector specifies which application types a strategy applies to. type ApplicationSelector struct { // APIGroup is the API group of the application. // If not specified, defaults to "apps.cozystack.io". // +optional APIGroup *string `json:"apiGroup,omitempty"` // Kind is the kind of the application (e.g., VirtualMachine, MySQL). + // +kubebuilder:validation:MinLength=1 Kind string `json:"kind"` }internal/backupcontroller/backupclass_resolver.go (1)
54-58: Redundant APIGroup extraction after normalization.Since
applicationRefis already normalized at line 46 (which guaranteesAPIGroupis non-nil and non-empty), this block will always take the branch at line 57. The initial assignment and nil check are unnecessary.Proposed simplification
- // Determine application API group (already normalized, but extract for matching) - appAPIGroup := DefaultApplicationAPIGroup - if applicationRef.APIGroup != nil { - appAPIGroup = *applicationRef.APIGroup - } + // Extract normalized API group for matching + appAPIGroup := *applicationRef.APIGroup // Find matching strategyinternal/backupcontroller/plan_controller.go (1)
66-76: Good practice: clearing error condition on success.However, the
Reasonfield here should also follow CamelCase convention.Proposed fix
meta.SetStatusCondition(&p.Status.Conditions, metav1.Condition{ Type: backupsv1alpha1.PlanConditionError, Status: metav1.ConditionFalse, - Reason: "Cron spec is valid", + Reason: "CronSpecValid", Message: "The cron schedule has been successfully parsed", })internal/backupcontroller/factory/backupjob_test.go (1)
123-150: Strengthen the job-name assertion to prevent false positives.Right now the test only checks length. It would be more robust to assert the exact expected name derived from the plan and scheduled timestamp.
Proposed test hardening
import ( + "fmt" "testing" "time" @@ validate: func(t *testing.T, job *backupsv1alpha1.BackupJob) { if job.Name == "" { t.Error("BackupJob name should be generated") } - // Name should start with plan name - if len(job.Name) < len("test-plan") { - t.Errorf("BackupJob name = %v, should start with test-plan", job.Name) - } + expected := fmt.Sprintf("test-plan-%d", tt.scheduled.Unix()/60) + if job.Name != expected { + t.Errorf("BackupJob name = %v, want %v", job.Name, expected) + } },
lllamnyp
left a comment
lllamnyp
left a comment
There was a problem hiding this comment.
Add parameters to templating
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/backupcontroller/velerostrategy_controller.go (1)
61-95: Guard against empty resolved.StrategyRef.Name before lookup.The reconciliation flow validates
resolved.StrategyRef.APIGroupandKindbefore callingreconcileVelero, but does not validate thatNameis non-empty. If a BackupClass is created with an emptyStrategyRef.Name, the code will attempt a lookup with an empty name, resulting in a "not found" error that masks the actual issue.Add an explicit check to catch this early:
🛡️ Suggested guard
logger.Debug("fetching Velero strategy", "strategyName", resolved.StrategyRef.Name) + if resolved.StrategyRef.Name == "" { + return r.markBackupJobFailed(ctx, j, "resolved strategy ref has empty name") + } veleroStrategy := &strategyv1alpha1.Velero{}
🤖 Fix all issues with AI agents
In `@internal/backupcontroller/velerostrategy_controller_test.go`:
- Around line 191-205: The test currently skips assertions if
veleroBackup.Spec.LabelSelector is nil, allowing a missing template expansion to
pass silently; update the test in velerostrategy_controller_test.go to check
veleroBackup.Spec.LabelSelector for nil and call t.Fatal(...) (not t.Error)
immediately if it is nil so the test fails fast when the LabelSelector is
missing, then proceed to assert the "app" label and storage location as before
(refer to veleroBackup.Spec.LabelSelector, MatchLabels and
veleroBackup.Spec.StorageLocation to locate the checks).
In `@internal/backupcontroller/velerostrategy_controller.go`:
- Around line 304-307: The artifact URI is using backupJob.Namespace instead of
the Velero backup's namespace; update the BackupArtifact URI construction (the
URI field on the BackupArtifact created in this block) to use
veleroBackup.Namespace (not backupJob.Namespace) so the URI becomes
velero://<velero-namespace>/<veleroBackup.Name>, matching the metadata which
already records veleroBackup.Namespace and ensuring consumers can locate the
Velero backup correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/backups/v1alpha1/DESIGN.md (1)
469-475: Summary seems outdated after BackupClass refactor.Line 472 still says “Plan + Storage – core-owned,” but storage selection now lives in BackupClass and is resolved per BackupJob/Plan. Please update the summary wording to reflect BackupClass as the central configuration unit.
🤖 Fix all issues with AI agents
In `@api/backups/v1alpha1/DESIGN.md`:
- Line 324: Update the typo in the DESIGN documentation sentence that reads
"Parameters are no stored directly in `Backup`" to "Parameters are not stored
directly in `Backup`"; edit the sentence referencing `Backup` and `BackupClass`
so it reads correctly (e.g., "Parameters are not stored directly in `Backup`.
Instead, they are resolved from `BackupClass` parameters when the backup was
created.") to preserve the existing meaning.
🧹 Nitpick comments (1)
api/backups/v1alpha1/DESIGN.md (1)
170-216: Convert emphasized labels to headings to satisfy MD036.Static analysis flags emphasis used as headings. Please convert these emphasized labels (e.g., “Key fields (spec)”, “BackupClass resolution”, “Parameters”) into proper Markdown headings for lint compliance.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@api/backups/v1alpha1/DESIGN.md`:
- Line 170: Replace bold-emphasized section titles like "**Key fields (spec)**"
(and the other similar instances noted) with proper Markdown heading syntax
(e.g., use one or more leading '#' characters appropriate to the document
structure such as "## Key fields (spec)"); update all occurrences flagged (the
lines containing the bolded headings) to headings to satisfy MD036 and keep
heading levels consistent across the document.
| **Purpose** | ||
| Define a class of backup configurations that encapsulate strategy and parameters per application type. `BackupClass` is a cluster-scoped resource that allows admins to configure backup strategies and parameters in a reusable way. | ||
|
|
||
| **Key fields (spec)** |
There was a problem hiding this comment.
Use headings instead of bold text for section titles (MD036).
markdownlint flags these as emphasis used for headings. Consider switching to proper heading levels.
📝 Suggested fix
-**Key fields (spec)**
+#### Key fields (spec)
@@
-**BackupClass resolution**
+#### BackupClass resolution
@@
-**Parameters**
+#### ParametersAlso applies to: 204-204, 216-216
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
170-170: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In `@api/backups/v1alpha1/DESIGN.md` at line 170, Replace bold-emphasized section
titles like "**Key fields (spec)**" (and the other similar instances noted) with
proper Markdown heading syntax (e.g., use one or more leading '#' characters
appropriate to the document structure such as "## Key fields (spec)"); update
all occurrences flagged (the lines containing the bolded headings) to headings
to satisfy MD036 and keep heading levels consistent across the document.
lllamnyp
left a comment
lllamnyp
left a comment
There was a problem hiding this comment.
LGTM.
Update the CRDs (use the ./hack/update-codegen.sh script) and update cmd/backup-controller/main.go and cmd/backupstrategy-controller/main.go so that there is a watch for BackupClass.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/backupcontroller/backupjob_controller.go`:
- Around line 119-140: In the handler created by
handler.TypedEnqueueRequestsFromMapFunc (the block that calls r.List with a
backupsv1alpha1.BackupJobList filtered by "spec.backupClassName": bc.Name),
don't silently return nil on error; log the failure before returning. Update the
anonymous func so that when r.List(ctx, &jobs,
client.MatchingFields{"spec.backupClassName": bc.Name}) returns an error you
call the controller logger (e.g., r.Log or the controller's logger field) to
emit a descriptive message including bc.Name and the error, then return nil as
before.
🧹 Nitpick comments (1)
packages/system/backup-controller/definitions/backups.cozystack.io_backupclasses.yaml (1)
167-171: Consider adding printer columns for better UX.The CRD lacks
additionalPrinterColumns. Consider adding columns to show useful info when runningkubectl get backupclasses, such as the number of strategies or age.♻️ Optional: Add printer columns
served: true storage: true + additionalPrinterColumns: + - jsonPath: .metadata.creationTimestamp + name: Age + type: date subresources: status: {}
androndo
force-pushed
the
feat/backups-rework
branch
from
39893c4 to
b9d8deb
Compare
…oduction Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
androndo
force-pushed
the
feat/backups-rework
branch
from
b9d8deb to
f6641c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@api/backups/v1alpha1/backupclass_types.go`:
- Around line 56-61: The StrategyRef field on BackupClass currently uses
corev1.TypedLocalObjectReference which is namespaced; because BackupClass is
cluster-scoped you must replace StrategyRef with a cluster-scoped reference
struct (e.g., a new BackupStrategyRef or ClusterTypedObjectReference) that only
contains apiGroup, kind and name (no namespace) and update all usages and JSON
tags to the new type; ensure you add the new struct definition in the same
package (with deepcopy+openapi markers as needed) and update any import
references (remove corev1 usage for this field) so CRD generation and any code
referencing BackupClass.StrategyRef compile against the cluster-scoped reference
type.
In `@api/backups/v1alpha1/DESIGN.md`:
- Around line 362-363: The document is unclear how restore-time storage is
deterministically resolved since BackupJob only references Backup and Backup
doesn’t include BackupClass; update the DESIGN text (including the other
mentioned section around lines 411-418) to require drivers to persist the
necessary storage identifier (for example the BackupClass name, storage location
name, or equivalent id) into Backup.driverMetadata when creating the Backup so
that the driver can deterministically resolve the storage location at restore
time; explicitly state that restore logic must read Backup.driverMetadata to
locate the original storage and fall back to driver-internal metadata only if
that field is absent.
♻️ Duplicate comments (1)
internal/backupcontroller/backupjob_controller.go (1)
119-140: Log errors from List operation instead of silently returning nil.When
r.Listfails (line 125), the handler returnsnilwhich silently drops the event. This could hide transient errors or misconfigurations that would be difficult to debug in production.🐛 Proposed fix
WatchesRawSource(source.Kind( mgr.GetCache(), &backupsv1alpha1.BackupClass{}, handler.TypedEnqueueRequestsFromMapFunc(func(ctx context.Context, bc *backupsv1alpha1.BackupClass) []reconcile.Request { + logger := log.FromContext(ctx) var jobs backupsv1alpha1.BackupJobList if err := r.List(ctx, &jobs, client.MatchingFields{"spec.backupClassName": bc.Name}); err != nil { + logger.Error(err, "failed to list BackupJobs for BackupClass", "backupClassName", bc.Name) return nil }
🧹 Nitpick comments (1)
internal/backupcontroller/backupclass_resolver.go (1)
68-72: Avoid leaking mutable spec maps to callers.
Parametersis returned directly from the BackupClass spec; downstream mutation would affect the cached object. Consider copying the map before returning.♻️ Suggested defensive copy
- if strategyAPIGroup == appAPIGroup && strategy.Application.Kind == applicationRef.Kind { + if strategyAPIGroup == appAPIGroup && strategy.Application.Kind == applicationRef.Kind { + var params map[string]string + if strategy.Parameters != nil { + params = make(map[string]string, len(strategy.Parameters)) + for k, v := range strategy.Parameters { + params[k] = v + } + } return &ResolvedBackupConfig{ StrategyRef: strategy.StrategyRef, - Parameters: strategy.Parameters, + Parameters: params, }, nil }
| **Note:** Parameters are resolved from `BackupClass` when the `BackupJob` is created. The driver uses these parameters to determine where to store backups. The storage location itself is managed by the driver (e.g., Velero's `BackupStorageLocation` CRD) and is not directly referenced in the `Backup` resource. When restoring, the driver resolves the storage location from the original `BackupClass` parameters or from the driver's own metadata. | ||
|
|
There was a problem hiding this comment.
Clarify how restore-time storage is deterministically resolved.
RestoreJob only references Backup, and Backup doesn’t include backupClassName. If the intent is to resolve storage from the original BackupClass, consider explicitly stating that the driver must persist the needed identifier (e.g., storage location name or BackupClass name) in Backup.driverMetadata so restore can be deterministic.
Also applies to: 411-418
🤖 Prompt for AI Agents
In `@api/backups/v1alpha1/DESIGN.md` around lines 362 - 363, The document is
unclear how restore-time storage is deterministically resolved since BackupJob
only references Backup and Backup doesn’t include BackupClass; update the DESIGN
text (including the other mentioned section around lines 411-418) to require
drivers to persist the necessary storage identifier (for example the BackupClass
name, storage location name, or equivalent id) into Backup.driverMetadata when
creating the Backup so that the driver can deterministically resolve the storage
location at restore time; explicitly state that restore logic must read
Backup.driverMetadata to locate the original storage and fall back to
driver-internal metadata only if that field is absent.
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@api/backups/v1alpha1/backupjob_webhook.go`:
- Around line 27-30: The BackupJob defaulter is currently a no-op; implement
defaulting so that when a BackupJob has a non-nil applicationRef with an empty
apiGroup you set it to "apps.cozystack.io". In BackupJob.Default(), check the
ApplicationRef field (e.g. j.Spec.ApplicationRef or j.ApplicationRef depending
on struct) for nil and for an empty ApiGroup string, and assign
"apps.cozystack.io" when missing; keep nil-safety and only modify the field when
it is unset so existing explicit values remain unchanged.
🧹 Nitpick comments (1)
api/backups/v1alpha1/backupjob_types.go (1)
47-57: Add schema-level non-empty validation forbackupClassName.Webhook validation covers empty values, but a CRD MinLength avoids round-trips and gives earlier feedback.
♻️ Suggested schema tweak
// BackupClassName references a BackupClass that contains strategy and storage configuration. // The BackupClass will be resolved to determine the appropriate strategy and storage // based on the ApplicationRef. // This field is immutable once the BackupJob is created. // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="backupClassName is immutable" +// +kubebuilder:validation:MinLength=1 BackupClassName string `json:"backupClassName"`
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@api/backups/v1alpha1/backupjob_webhook_test.go`:
- Around line 55-70: The test currently expects a whitespace-only
BackupJob.Spec.BackupClassName to be accepted, but we should reject it; update
the test case in backupjob_webhook_test.go to set wantErr: true for the case
with BackupClassName: " " and then update the BackupJob webhook validation
(the BackupJob's admission/validation logic—e.g., the
ValidateCreate/ValidateUpdate handler that checks BackupJobSpec.BackupClassName)
to trim whitespace (strings.TrimSpace) and treat an empty result as invalid,
returning an appropriate validation error when BackupClassName is empty after
trimming.
- Around line 173-201: The test case "update when old backupClassName is empty
should allow setting it" contradicts the BackupJobSpec immutability rule for
BackupClassName: since BackupJobSpec declares BackupClassName as immutable (self
== oldSelf), updating from empty to non-empty should be rejected; update the
test to expect an error (set wantErr: true) or alternatively relax the
immutability in the BackupJobSpec schema/validation where BackupClassName is
enforced. Locate the case in backupjob_webhook_test.go and either change the
test's wantErr for that table entry or modify the validation/field immutability
logic that enforces BackupClassName in BackupJobSpec.
In `@api/backups/v1alpha1/backupjob_webhook.go`:
- Around line 54-56: The current immutability check in backupjob_webhook.go only
rejects changes when oldJob.Spec.BackupClassName is non-empty; update the check
to reject any change to BackupClassName regardless of emptiness by comparing
oldJob.Spec.BackupClassName directly to j.Spec.BackupClassName (i.e., remove the
oldJob.Spec.BackupClassName != "" guard) and keep returning the same formatted
error when they differ so empty→non-empty transitions are also rejected.
- Around line 36-38: The validation currently only checks j.Spec.BackupClassName
== "" and accepts whitespace-only values; change the check in the webhook
validation (where j.Spec.BackupClassName is validated) to treat whitespace-only
strings as empty by using strings.TrimSpace(j.Spec.BackupClassName) and return
the same fmt.Errorf when the trimmed value is empty. Also update the unit test
that sets BackupClassName to a whitespace-only string (the test case in
backupjob_webhook_test that currently uses BackupClassName: " ") to expect an
error instead of no error.
♻️ Duplicate comments (1)
api/backups/v1alpha1/backupjob_webhook.go (1)
24-27: Default() should setapplicationRef.apiGroupwhen empty.Spec explicitly documents the default (
apps.cozystack.io), but the defaulter is still a no-op. That leaves user-created objects without the promised default and can break downstream assumptions.✅ Proposed fix
func (j *BackupJob) Default() { - // No defaults needed for BackupJob currently + if j.Spec.ApplicationRef.APIGroup == nil || (j.Spec.ApplicationRef.APIGroup != nil && *j.Spec.ApplicationRef.APIGroup == "") { + defaultGroup := "apps.cozystack.io" + j.Spec.ApplicationRef.APIGroup = &defaultGroup + } }
| name: "BackupJob with whitespace-only backupClassName should be rejected", | ||
| job: &BackupJob{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-job", | ||
| Namespace: "default", | ||
| }, | ||
| Spec: BackupJobSpec{ | ||
| ApplicationRef: corev1.TypedLocalObjectReference{ | ||
| Kind: "VirtualMachine", | ||
| Name: "vm1", | ||
| }, | ||
| BackupClassName: " ", | ||
| }, | ||
| }, | ||
| wantErr: false, // Whitespace is technically not empty, but this is acceptable | ||
| }, |
There was a problem hiding this comment.
Reject whitespace-only backupClassName.
A whitespace-only value can’t resolve to any real BackupClass, so it should be treated as empty and rejected to fail fast. Please align the test expectation with that behavior (and update the webhook validation accordingly).
💡 Proposed test adjustment
{
name: "BackupJob with whitespace-only backupClassName should be rejected",
job: &BackupJob{
@@
BackupClassName: " ",
},
},
- wantErr: false, // Whitespace is technically not empty, but this is acceptable
+ wantErr: true,
+ errMsg: "backupClassName is required and cannot be empty",
},📝 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.
| name: "BackupJob with whitespace-only backupClassName should be rejected", | |
| job: &BackupJob{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "test-job", | |
| Namespace: "default", | |
| }, | |
| Spec: BackupJobSpec{ | |
| ApplicationRef: corev1.TypedLocalObjectReference{ | |
| Kind: "VirtualMachine", | |
| Name: "vm1", | |
| }, | |
| BackupClassName: " ", | |
| }, | |
| }, | |
| wantErr: false, // Whitespace is technically not empty, but this is acceptable | |
| }, | |
| name: "BackupJob with whitespace-only backupClassName should be rejected", | |
| job: &BackupJob{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "test-job", | |
| Namespace: "default", | |
| }, | |
| Spec: BackupJobSpec{ | |
| ApplicationRef: corev1.TypedLocalObjectReference{ | |
| Kind: "VirtualMachine", | |
| Name: "vm1", | |
| }, | |
| BackupClassName: " ", | |
| }, | |
| }, | |
| wantErr: true, | |
| errMsg: "backupClassName is required and cannot be empty", | |
| }, |
🤖 Prompt for AI Agents
In `@api/backups/v1alpha1/backupjob_webhook_test.go` around lines 55 - 70, The
test currently expects a whitespace-only BackupJob.Spec.BackupClassName to be
accepted, but we should reject it; update the test case in
backupjob_webhook_test.go to set wantErr: true for the case with
BackupClassName: " " and then update the BackupJob webhook validation (the
BackupJob's admission/validation logic—e.g., the ValidateCreate/ValidateUpdate
handler that checks BackupJobSpec.BackupClassName) to trim whitespace
(strings.TrimSpace) and treat an empty result as invalid, returning an
appropriate validation error when BackupClassName is empty after trimming.
| // This references a cluster-scoped resource, so it does not include a namespace. | ||
| StrategyRef TypedClusterObjectReference `json:"strategyRef"` |
There was a problem hiding this comment.
Revert this. A local object reference does not have a namespace. Neither does a cluster-scoped object. TypedLocalObjectReference fits the use-case perfectly.
| if oldJob.Spec.BackupClassName != j.Spec.BackupClassName { | ||
| return nil, fmt.Errorf("backupClassName is immutable and cannot be changed from %q to %q", oldJob.Spec.BackupClassName, j.Spec.BackupClassName) |
There was a problem hiding this comment.
K8s CRDs support validation for update operations. Just google "kubebuilder immutable field", find the right annotation and add regenerate the CRDs. You don't need to implement such checks in a custom webhook.
This reverts commit 73d6e30. <!-- Thank you for making a contribution! Here are some tips for you: - Start the PR title with the [label] of Cozystack component: - For system components: [platform], [system], [linstor], [cilium], [kube-ovn], [dashboard], [cluster-api], etc. - For managed apps: [apps], [tenant], [kubernetes], [postgres], [virtual-machine] etc. - For development and maintenance: [tests], [ci], [docs], [maintenance]. - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does Revert wrong type change according review [request](#1873 (comment)) ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same [label] as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note [fix strategyRef type in backups] ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Breaking Changes** * Backup strategy references have been updated from cluster-scoped to namespace-scoped references. This change enables better namespace isolation and control. Update your backup configurations to reference strategies within the appropriate namespace. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
What this PR does
Release note
Closes #1880
The following functions related StorageRef were removed as they are no longer used after migrating to BackupClass API:
// - resolveBucketStorageRef: Previously resolved S3 credentials from Bucket storageRef
// - createS3CredsForVelero: Previously created Velero S3 credentials secrets
// - createBackupStorageLocation: Previously created Velero BackupStorageLocation resources
// - createVolumeSnapshotLocation: Previously created Velero VolumeSnapshotLocation resources
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.