[backups] Build and deploy backup controller #1685
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. WalkthroughA new backup-controller component is introduced with a complete Kubebuilder-based Go controller, Helm chart configuration, Docker image definition, platform bundle integration, and minimal logic updates to handle cron expression validation state. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @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 establishes the foundational infrastructure for a new backup controller within the system. It enables the building and containerization of the controller's Go application and integrates its deployment into the existing PaaS bundles via a new Helm chart. This ensures the backup controller can be consistently deployed and managed across different PaaS environments. 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-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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new backup controller, including its build process and deployment via a Helm chart. The changes are a good step forward, but there are a few critical issues in the Kubernetes manifests that will prevent the controller from working correctly. Specifically, there's an incorrect dependency in one of the bundles and a misconfigured RBAC role binding. Additionally, the deployment manifest is missing crucial configurations like health probes and resource limits, and the RBAC permissions are overly permissive. I've provided detailed suggestions to fix these issues to ensure the controller is robust and secure.
| releaseName: backup-controller | ||
| chart: cozy-backup-controller | ||
| namespace: cozy-backup-controller | ||
| dependsOn: [cilium,kubeovn] |
There was a problem hiding this comment.
The dependsOn field for the backup-controller release lists cilium and kubeovn. However, these releases are not part of the paas-hosted.yaml bundle, which will cause the deployment to fail. The dependency should be removed or changed to a release that exists in this bundle.
dependsOn: []| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: backup-controller |
There was a problem hiding this comment.
The roleRef.name in this ClusterRoleBinding is backup-controller, but the ClusterRole defined in rbac.yaml is named backups.cozystack.io:core-controller. This mismatch will prevent the controller's service account from being granted any permissions, causing the controller to fail. The roleRef.name must match the metadata.name of the ClusterRole.
name: backups.cozystack.io:core-controller| containers: | ||
| - name: backup-controller | ||
| image: "{{ .Values.backupController.image }}" | ||
| args: | ||
| - --leader-elect | ||
| {{- if .Values.backupController.debug }} | ||
| - --zap-log-level=debug | ||
| {{- else }} | ||
| - --zap-log-level=info | ||
| {{- end }} |
There was a problem hiding this comment.
The deployment configuration for the backup controller is missing several important settings for production-readiness:
- Health Probes:
livenessProbeandreadinessProbeare not defined. These are essential for Kubernetes to manage the pod's lifecycle correctly. The controller exposes/healthzand/readyzendpoints on port 8081 for this purpose. - Resource Management:
resources.requestsandresources.limitsfor CPU and memory are missing. Setting these helps ensure predictable performance and scheduling. - Metrics: Controller metrics are currently disabled by default. They should be enabled by passing the
--metrics-bind-addressflag to expose the metrics endpoint, which is crucial for monitoring.
I've suggested adding these configurations.
containers:
- name: backup-controller
image: "{{ .Values.backupController.image }}"
args:
- --leader-elect
- --metrics-bind-address=:8443
{{- if .Values.backupController.debug }}
- --zap-log-level=debug
{{- else }}
- --zap-log-level=info
{{- end }}
ports:
- name: metrics
containerPort: 8443
- name: health
containerPort: 8081
readinessProbe:
httpGet:
path: /readyz
port: health
initialDelaySeconds: 5
periodSeconds: 10
livenessProbe:
httpGet:
path: /healthz
port: health
initialDelaySeconds: 15
periodSeconds: 20
resources:
requests:
cpu: 10m
memory: 64Mi
limits:
cpu: 500m
memory: 128Mi| // TODO(user): If CertDir, CertName, and KeyName are not specified, controller-runtime will automatically | ||
| // generate self-signed certificates for the metrics server. While convenient for development and testing, | ||
| // this setup is not recommended for production. |
There was a problem hiding this comment.
This TODO comment highlights a potential issue for production environments regarding self-signed certificates for the metrics server. For production readiness, this should be addressed by providing a mechanism to supply proper certificates. Please consider creating a follow-up task to handle certificate management for the metrics endpoint.
| rules: | ||
| - apiGroups: ["backups.cozystack.io"] | ||
| resources: ["*"] | ||
| verbs: ["get", "list", "watch"] | ||
| - apiGroups: ["backups.cozystack.io"] | ||
| resources: | ||
| - backupjobs | ||
| verbs: ["*"] |
There was a problem hiding this comment.
The permissions defined in this ClusterRole are overly permissive. It's a security best practice to follow the principle of least privilege by granting only the permissions that are strictly necessary.
- The first rule grants
get,list,watchon all (*) resources within thebackups.cozystack.ioAPI group. This should be scoped down to just theplansresource. - The second rule grants all (
*) verbs onbackupjobs. Based on the controller's logic, it only needs thecreateverb.
I recommend tightening these permissions as suggested.
rules:
- apiGroups: ["backups.cozystack.io"]
resources: ["plans"]
verbs: ["get", "list", "watch"]
- apiGroups: ["backups.cozystack.io"]
resources: ["backupjobs"]
verbs: ["create"]cb24e7f to
47c4655
Compare
ff15585 to
9a0e632
Compare
47c4655 to
a7b4239
Compare
5c9f64d to
fc9f355
Compare
## What this PR does This patch adds compilation and docker build steps for the backup controller as well as adding a Helm chart to deploy it as part of the PaaS bundles. ### Release note ```release-note [backups] Build and deploy backup controller ``` Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
fc9f355 to
8928552
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/backup-controller/main.go (1)
109-119: Production certificate management remains unaddressed.The TODO comment about self-signed certificates for the metrics server was already flagged in previous reviews. This should be tracked for production readiness.
🧹 Nitpick comments (1)
packages/system/backup-controller/Makefile (1)
7-18: Declare targets as .PHONY.The
imageandimage-backup-controllertargets should be declared as.PHONYsince they don't produce files with those names.Apply this diff:
include ../../../scripts/common-envs.mk include ../../../scripts/package.mk +.PHONY: image image-backup-controller + image: image-backup-controller
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
Makefile(1 hunks)cmd/backup-controller/main.go(1 hunks)internal/backupcontroller/plan_controller.go(1 hunks)packages/core/platform/bundles/paas-full.yaml(1 hunks)packages/core/platform/bundles/paas-hosted.yaml(1 hunks)packages/system/backup-controller/Chart.yaml(1 hunks)packages/system/backup-controller/Makefile(1 hunks)packages/system/backup-controller/images/backup-controller/Dockerfile(1 hunks)packages/system/backup-controller/templates/crds.yaml(1 hunks)packages/system/backup-controller/templates/deployment.yaml(1 hunks)packages/system/backup-controller/templates/rbac-bind.yaml(1 hunks)packages/system/backup-controller/templates/rbac.yaml(1 hunks)packages/system/backup-controller/templates/sa.yaml(1 hunks)packages/system/backup-controller/values.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Controller-runtime patterns and kubebuilder style for Go code
Files:
internal/backupcontroller/plan_controller.gocmd/backup-controller/main.go
**/Chart.yaml
📄 CodeRabbit inference engine (AGENTS.md)
Use Helm Charts with the umbrella pattern and vendor upstream charts in
charts/directory
Files:
packages/system/backup-controller/Chart.yaml
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to **/*.go : Use Controller-runtime patterns and kubebuilder style for Go code
📚 Learning: 2025-07-10T12:14:27.197Z
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.
Applied to files:
packages/system/backup-controller/templates/rbac.yaml
📚 Learning: 2025-11-27T11:21:45.808Z
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to **/Chart.yaml : Use Helm Charts with the umbrella pattern and vendor upstream charts in `charts/` directory
Applied to files:
packages/system/backup-controller/templates/crds.yamlpackages/core/platform/bundles/paas-hosted.yamlpackages/system/backup-controller/Chart.yamlpackages/core/platform/bundles/paas-full.yaml
📚 Learning: 2025-11-27T11:21:45.808Z
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to **/*.go : Use Controller-runtime patterns and kubebuilder style for Go code
Applied to files:
cmd/backup-controller/main.go
🧬 Code graph analysis (2)
internal/backupcontroller/plan_controller.go (1)
api/backups/v1alpha1/plan_types.go (1)
PlanConditionError(22-22)
cmd/backup-controller/main.go (4)
internal/sse/server.go (1)
Options(17-44)internal/controller/dashboard/manager.go (1)
NewManager(51-54)pkg/apiserver/apiserver.go (1)
Scheme(53-53)internal/backupcontroller/plan_controller.go (1)
PlanReconciler(28-31)
🪛 checkmake (0.2.2)
packages/system/backup-controller/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 7-7: Target "image" should be declared PHONY.
(phonydeclared)
🪛 YAMLlint (1.37.1)
packages/system/backup-controller/templates/crds.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/backup-controller/templates/deployment.yaml
[error] 31-31: syntax error: could not find expected ':'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (17)
Makefile (1)
18-18: LGTM!The backup-controller image build step is properly integrated into the build sequence and follows the established pattern.
internal/backupcontroller/plan_controller.go (1)
65-76: LGTM!The error condition reset logic correctly clears stale error states when cron parsing succeeds, following controller-runtime best practices. The implementation properly checks for existing conditions and handles status updates with appropriate error handling.
Based on learnings, this follows Controller-runtime patterns and kubebuilder style appropriately.
packages/system/backup-controller/templates/sa.yaml (1)
1-4: LGTM!The ServiceAccount definition is correct and appropriately minimal for a Helm chart template.
packages/system/backup-controller/Chart.yaml (1)
1-3: LGTM!The Chart descriptor is appropriately structured for a custom controller. The umbrella pattern guidance applies to charts that vendor upstream components, whereas this is a custom backup-controller implementation.
As per coding guidelines, this follows the Helm chart structure appropriately for a custom controller.
packages/system/backup-controller/images/backup-controller/Dockerfile (2)
3-23: LGTM!The multi-stage build follows best practices for containerizing Go applications:
- Cross-platform build support with TARGETOS/TARGETARCH
- Static linking with CGO disabled for scratch base
- Minimal final image with only binary and CA certificates
1-1: Go 1.24 is available and valid —golang:1.24-alpineis a legitimate Docker image tag. Go 1.24 was released before December 2025, so the build will not fail due to version unavailability. Consider updating to Go 1.25.5 (the current stable release as of December 2025) for access to the latest features and security patches.Likely an incorrect or invalid review comment.
packages/system/backup-controller/templates/crds.yaml (1)
1-4: LGTM! Static analysis error is a false positive.The Helm template correctly aggregates CRD definitions from the
definitions/*directory. The YAMLlint syntax error is expected since static analysis tools don't understand Go template syntax.packages/system/backup-controller/templates/rbac-bind.yaml (1)
1-12: ClusterRoleBinding is correctly configured with valid reference.The ClusterRoleBinding properly references the ClusterRole
backups.cozystack.io:core-controllerwhich exists inrbac.yamlwith matching metadata. The binding subject correctly points to thebackup-controllerServiceAccount, and the namespace uses the appropriate template variable.packages/system/backup-controller/templates/rbac.yaml (1)
5-11: LGTM! ClusterRole permissions are properly scoped.The RBAC permissions are well-defined and follow the principle of least privilege. The controller has read access to Plans and appropriate access to manage BackupJobs.
packages/system/backup-controller/values.yaml (1)
1-14: LGTM! Default values are well-configured.The default configuration is production-ready with:
- High availability (2 replicas)
- Metrics enabled for observability
- Conservative resource limits
- Secure metrics endpoint (HTTPS on :8443)
cmd/backup-controller/main.go (4)
48-53: LGTM! Scheme initialization follows kubebuilder patterns.The scheme registration correctly includes both the core Kubernetes types and the custom backups v1alpha1 API types, following controller-runtime best practices.
As per coding guidelines, this follows the expected controller-runtime patterns and kubebuilder style.
80-93: LGTM! HTTP/2 hardening improves security posture.The conditional disabling of HTTP/2 by default protects against known CVEs (GHSA-qppj-fm5r-hxr3, GHSA-4374-p667-p6c8). The implementation correctly applies TLS options to both webhook and metrics servers.
150-156: LGTM! PlanReconciler setup follows controller-runtime patterns.The reconciler is correctly initialized with the manager's client and scheme, following standard kubebuilder conventions.
As per coding guidelines, this follows the expected controller-runtime patterns and kubebuilder style.
121-124: Verify the need for increased rate limits.The Kubernetes client QPS and Burst have been increased significantly (QPS: 5.0 → 50.0, Burst: 10 → 100). This pattern is applied consistently across three controllers (backup-controller, lineage-controller-webhook, and cozystack-controller), yet the PlanReconciler's workload is cron-based with 5-minute minimum intervals, not high-frequency operations. No documentation or load testing results justify these aggressive limits.
Please confirm:
- What is the expected reconciliation workload that requires these high limits?
- Have you load-tested the controller to verify these values are appropriate?
- Could these limits impact API server performance under load?
packages/core/platform/bundles/paas-full.yaml (1)
115-120: LGTM! Backup controller integration follows bundle conventions.The backup-controller release is properly configured with:
- Correct chart reference (
cozy-backup-controller)- Appropriate dependencies on core networking (cilium, kubeovn)
- Dedicated namespace (
cozy-backup-controller)- Standard naming conventions
packages/system/backup-controller/templates/deployment.yaml (2)
25-57: LGTM! Deployment is production-ready with comprehensive configuration.The deployment now includes all essential production settings that were previously flagged:
- Health probes for both liveness and readiness
- Conditional metrics endpoint configuration
- Resource requests and limits
- Debug logging control
38-40: Potential issue with port extraction from bindAddress.Line 40 uses
mustLastto extract the port frombindAddress, which assumes the value contains a colon. IfbindAddressis set to a port-only value like"8443"instead of":8443", this will fail during template rendering.Consider adding validation or using a more robust extraction method:
ports: - name: metrics - containerPort: {{ split ":" .Values.backupController.metrics.bindAddress | mustLast }} + containerPort: {{ trimPrefix ":" .Values.backupController.metrics.bindAddress }} - name: health containerPort: 8081Alternatively, if the format is guaranteed to be
":port", document this requirement in the values.yaml comments.Likely an incorrect or invalid review comment.
| - name: backup-controller | ||
| releaseName: backup-controller | ||
| chart: cozy-backup-controller | ||
| namespace: cozy-backup-controller | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, check if the backup-controller directory exists and explore its structure
find . -type d -name "backup-controller" | head -20Repository: cozystack/cozystack
Length of output: 183
🏁 Script executed:
# Check the paas-hosted.yaml file to see how other releases define dependsOn
cat -n packages/core/platform/bundles/paas-hosted.yaml | head -80Repository: cozystack/cozystack
Length of output: 2954
🏁 Script executed:
# Search for backup-controller related files to understand dependencies
fd backup-controller --type f | head -30Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check if backup-controller templates exist and what they contain
find ./packages/system/backup-controller -type f -name "*.yaml" -o -name "*.yml" | head -20Repository: cozystack/cozystack
Length of output: 779
🏁 Script executed:
# Search for webhook configurations in backup-controller
rg -l 'WebhookConfiguration' ./packages/system/backup-controller/ 2>/dev/null || echo "No WebhookConfiguration found"Repository: cozystack/cozystack
Length of output: 92
🏁 Script executed:
# Check for cert-manager references in backup-controller
rg 'cert-manager' ./packages/system/backup-controller/ 2>/dev/null || echo "No cert-manager references found"Repository: cozystack/cozystack
Length of output: 95
🏁 Script executed:
# Check the main chart files for dependencies
cat ./packages/system/backup-controller/Chart.yaml 2>/dev/null || echo "Chart.yaml not found"Repository: cozystack/cozystack
Length of output: 206
🏁 Script executed:
# Check if backup-controller references cozystack-controller or cozystack-api
rg 'cozystack-controller|cozystack-api' ./packages/system/backup-controller/Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# Check the deployment to understand what it needs
cat -n ./packages/system/backup-controller/templates/deployment.yaml | head -60Repository: cozystack/cozystack
Length of output: 2121
Add explicit dependsOn field for consistency.
The backup-controller release definition is missing the dependsOn field, making it inconsistent with all other releases in the bundle. Every other release explicitly declares its dependencies (either empty or with listed dependencies).
- name: backup-controller
releaseName: backup-controller
chart: cozy-backup-controller
namespace: cozy-backup-controller
+ dependsOn: []Backup-controller has no external dependencies (no webhooks, cert-manager references, or coupling to cozystack-controller/cozystack-api), so dependsOn: [] is appropriate.
📝 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: backup-controller | |
| releaseName: backup-controller | |
| chart: cozy-backup-controller | |
| namespace: cozy-backup-controller | |
| - name: backup-controller | |
| releaseName: backup-controller | |
| chart: cozy-backup-controller | |
| namespace: cozy-backup-controller | |
| dependsOn: [] |
🤖 Prompt for AI Agents
In packages/core/platform/bundles/paas-hosted.yaml around lines 59 to 63, the
backup-controller release block is missing the explicit dependsOn field used by
all other releases; add a dependsOn: [] entry under the backup-controller
release (keeping indentation consistent with other releases) so the block reads
with an explicit empty dependency list.
## What this PR does This patch adds compilation and docker build steps for the backup controller as well as adding a Helm chart to deploy it as part of the PaaS bundles. ### Release note ```release-note [backups] Build and deploy backup controller ``` (cherry picked from commit 8989791) Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
…d backup system (#1867) ## What this PR does Update changelog for v1.0.0-alpha.1 to include missing features: - **Cozystack Operator**: New operator for Package and PackageSource management (#1740, #1741, #1755, #1756, #1760, #1761) - **Backup System**: Comprehensive backup functionality with Velero integration (#1640, #1685, #1687, #1708, #1719, #1720, #1737, #1762) - Add @androndo to contributors - Update Full Changelog link to v0.38.0...v1.0.0-alpha.1 ### Release note ```release-note [docs] Update changelog for v1.0.0-alpha.1: add cozystack-operator and backup system ```
What this PR does
This patch adds compilation and docker build steps for the backup controller as well as adding a Helm chart to deploy it as part of the PaaS bundles.
Release note
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.