feat(platform): migrate to etcd-operator v1alpha2 - #2859
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request completes the migration of the etcd-operator to v1alpha2. It involves updating API types, removing deprecated backup configurations in favor of the Cozystack BackupClass strategy, and adjusting the etcd chart to align with the new operator's requirements, including changes to TLS management and pod scheduling. Highlights
New Features🧠 You can now enable Memory (public preview) 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. Ignored Files
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 the 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 counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates etcd backup and restore flows to ChangesEtcd v1alpha2 migration
Sequence Diagram(s)sequenceDiagram
participant BackupJobReconciler
participant EtcdCluster as EtcdCluster
participant EtcdSnapshot as EtcdSnapshot
participant BackupStatus as BackupJob.status
BackupJobReconciler->>EtcdCluster: check Available
BackupJobReconciler->>EtcdSnapshot: ensure or reuse
EtcdSnapshot-->>BackupJobReconciler: phase and artifact status
BackupJobReconciler->>BackupStatus: write artifact or terminal failure
sequenceDiagram
participant Migration45 as migration 45
participant EtcdMigrate as etcd-migrate
participant Operator as etcd-operator deployment
participant CertManager as cert-manager
Migration45->>CertManager: patch wildcard SANs and wait for reissue
Migration45->>Migration45: resolve snapshot args and stage creds
Migration45->>Operator: scale to 0
Migration45->>EtcdMigrate: run --apply with backup-s3 args
Migration45->>Operator: scale to 1
Migration45->>Migration45: stamp version 46
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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 |
There was a problem hiding this comment.
Code Review
This pull request migrates the etcd-operator from etcd.aenix.io/v1alpha1 to etcd-operator.cozystack.io/v1alpha2, updating the backup strategy controllers, Helm charts, and tests accordingly, and introducing an in-place migration script to adopt legacy clusters. Feedback on the changes highlights a critical need to wrap the migration script in an EXIT trap to prevent leaving the operator scaled to zero on failure, and to add the --cluster flag to the defragmentation job to ensure it fans out to all members. Additionally, it is recommended to set the seccompProfile to RuntimeDefault for the operator deployment, and to update several stale comments and status messages to consistently reference Available and EtcdSnapshot instead of Ready and EtcdBackup.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| args: | ||
| - --endpoints={{ range $i, $e := until (int .Values.replicas) }}{{ if $i }},{{ end }}https://{{ $.Release.Name }}-{{ $i }}.{{ $.Release.Name }}-headless.{{ $.Release.Namespace }}.svc:2379{{ end }} | ||
| # The v1alpha2 operator names the headless Service after the | ||
| # cluster ({{ $.Release.Name }}) and does not use StatefulSet | ||
| # ordinal pod names. Point at the headless Service and let | ||
| # --cluster fan the defrag out to every member via member-list. | ||
| - --endpoints=https://{{ $.Release.Name }}.{{ $.Release.Namespace }}.svc:2379 |
There was a problem hiding this comment.
The defrag job only specifies a single endpoint (the headless service) but does not pass the --cluster flag. Without --cluster, etcdctl will only defrag a single resolved endpoint instead of fanning out the defragmentation to all members of the cluster. Since the old code explicitly listed all endpoints, adding --cluster is necessary to maintain the same behavior.
args:
# The v1alpha2 operator names the headless Service after the
# cluster ({{ $.Release.Name }}) and does not use StatefulSet
# ordinal pod names. Point at the headless Service and let
# --cluster fan the defrag out to every member via member-list.
- --endpoints=https://{{ $.Release.Name }}.{{ $.Release.Namespace }}.svc:2379
- --cluster| securityContext: | ||
| runAsNonRoot: true |
There was a problem hiding this comment.
For improved security, it is recommended to explicitly set the seccompProfile to RuntimeDefault in the pod's securityContext.
securityContext:
runAsNonRoot: true
seccompProfile:
type: RuntimeDefaultReferences
- Flag missing securityContext or containers running as root without an explicit reason. (link)
| apimeta.SetStatusCondition(&restoreJob.Status.Conditions, metav1.Condition{ | ||
| Type: "Ready", | ||
| Status: metav1.ConditionFalse, | ||
| Reason: "EtcdClusterBootstrapping", | ||
| Message: fmt.Sprintf("waiting for etcd.aenix.io/EtcdCluster %s/%s to reach Ready after bootstrap.restore", target.Namespace, etcdClusterName), | ||
| Message: fmt.Sprintf("waiting for etcd-operator.cozystack.io/EtcdCluster %s/%s to reach Ready after bootstrap.restore", target.Namespace, etcdClusterName), | ||
| }) |
There was a problem hiding this comment.
The status condition message still refers to the cluster reaching Ready after bootstrap.restore. Since the v1alpha2 operator uses the Available condition instead of Ready, these messages should be updated to refer to Available for accuracy.
| apimeta.SetStatusCondition(&restoreJob.Status.Conditions, metav1.Condition{ | |
| Type: "Ready", | |
| Status: metav1.ConditionFalse, | |
| Reason: "EtcdClusterBootstrapping", | |
| Message: fmt.Sprintf("waiting for etcd.aenix.io/EtcdCluster %s/%s to reach Ready after bootstrap.restore", target.Namespace, etcdClusterName), | |
| Message: fmt.Sprintf("waiting for etcd-operator.cozystack.io/EtcdCluster %s/%s to reach Ready after bootstrap.restore", target.Namespace, etcdClusterName), | |
| }) | |
| apimeta.SetStatusCondition(&restoreJob.Status.Conditions, metav1.Condition{ | |
| Type: "Ready", | |
| Status: metav1.ConditionFalse, | |
| Reason: "EtcdClusterBootstrapping", | |
| Message: fmt.Sprintf("waiting for etcd-operator.cozystack.io/EtcdCluster %s/%s to become Available after bootstrap.restore", target.Namespace, etcdClusterName), | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/backups/etcd/00-helpers.sh (1)
18-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast when
ETCD_NAMEis notetcd.Line 22 allows overriding
ETCD_NAME, but downstream scripts still hardcodeetcd(for example, waits/lookups), which leads to partial execution followed by opaque timeouts. Add a single validation here so misconfiguration fails immediately.Proposed patch
export ETCD_NAME="${ETCD_NAME:-etcd}" +if [[ "${ETCD_NAME}" != "etcd" ]]; then + log_error "ETCD_NAME must be 'etcd' (chart constraint in templates/check-release-name.yaml)" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/backups/etcd/00-helpers.sh` around lines 18 - 22, Add a fail-fast validation immediately after the ETCD_NAME export: check that the variable ETCD_NAME equals the literal "etcd", and if not print a clear error message to stderr (including the provided ETCD_NAME value) and exit with a non-zero status; reference the ETCD_NAME variable so downstream scripts relying on the hardcoded "etcd" behavior cannot proceed with a misconfigured name.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/backups/etcd/92-scenario-user-restore.md`:
- Around line 38-40: Update the wording in the restore verification step to use
a single readiness term: change the reference from "Ready=True" to
"Available=True" so it consistently refers to
EtcdCluster.status.conditions[Available]; ensure the step text (the sentence
that currently reads "Wait for `Ready=True`. The driver polls the new
`EtcdCluster.status.conditions[Available]`.") is rewritten to a single,
unambiguous phrase such as "Wait for `Available=True`." while keeping the
reference to `EtcdCluster.status.conditions[Available]` unchanged.
In `@internal/backupcontroller/etcdstrategy_controller.go`:
- Around line 237-245: Update all status/error messages that still say "Ready"
to "Available" to match the gating logic which uses etcdClusterReady
(Available). Specifically, in the block using etcdClusterReady(cluster) and
helper calls markBackupJobFailed and requeueBackupJobWithReason, replace "did
not become Available within ... (current Available condition: %s)"/waiting
messages so they consistently reference "Available" (use
etcdClusterReadyReason(cluster) for the condition text). Also search for the
same pattern in the other occurrences mentioned (the blocks around the other
occurrences that call etcdClusterReady, etcdClusterReadyReason,
markBackupJobFailed, requeueBackupJobWithReason) and make the same string
replacements so all user-visible status/reason text aligns with the Available
gate.
In `@packages/core/platform/images/migrations/Dockerfile`:
- Line 5: Replace the mutable tag in the Dockerfile FROM line that currently
reads "ghcr.io/cozystack/etcd-operator:v0.5.0 AS etcd-operator" with the
corresponding immutable image digest
(ghcr.io/cozystack/etcd-operator@sha256:...) so the migration image is pinned
and reproducible; update the FROM instruction to use the exact sha256 digest for
the v0.5.0 operator image.
In `@packages/core/platform/images/migrations/migrations/44`:
- Around line 75-94: The migration can leave the operator scaled to 0 because
errors from etcd-migrate or rollout status are masked and there's no guaranteed
restore; add a safe-guard: enable strict failure handling (set -euo pipefail)
for this block and install a trap on EXIT/ERR that always scales
"${ETCD_OPERATOR_NS}/${ETCD_OPERATOR_DEPLOY}" back to 1 (kubectl scale) so
replicas are restored on any failure, and remove the "|| true" that masks the
rollout status after scaling back to 1 so rollout failures surface; ensure this
applies to failures from the etcd-migrate --apply invocation and any kubectl
rollout status calls.
In `@packages/system/etcd-operator-crds/Makefile`:
- Around line 9-13: The update target in the Makefile uses the kubectl kustomize
remote shorthand (the update recipe invoking "kubectl kustomize
\"github.com/cozystack/etcd-operator/config/crd?ref=$(ETCD_OPERATOR_REF)\""),
which relies on kubectl embedding kustomize; either require/validate a minimum
kubectl version or make the remote path explicit to avoid ambiguity—update the
Makefile’s update target to use the explicit GitHub URL form
("https://github.com/...//config/crd?ref=$(ETCD_OPERATOR_REF)") or add a
preflight check/comment that CI/builds must use kubectl >=1.14 (and/or add a
simple version check using kubectl version --client) so the ETCD_OPERATOR_REF
variable remains used and generation remains robust.
In `@packages/system/etcd-operator-crds/templates/etcdsnapshots.yaml`:
- Around line 141-144: Add a Kubernetes version constraint to the Helm chart so
the CRD using the CEL validation rule (x-kubernetes-validations with rule:
has(self.s3) != has(self.pvc)) is not installed on clusters older than 1.25;
update the Chart.yaml to include kubeVersion: ">=1.25.0-0" (or document the
requirement) so Helm prevents installation on unsupported Kubernetes versions
and the CRD field won’t be rejected at apply time.
---
Outside diff comments:
In `@examples/backups/etcd/00-helpers.sh`:
- Around line 18-22: Add a fail-fast validation immediately after the ETCD_NAME
export: check that the variable ETCD_NAME equals the literal "etcd", and if not
print a clear error message to stderr (including the provided ETCD_NAME value)
and exit with a non-zero status; reference the ETCD_NAME variable so downstream
scripts relying on the hardcoded "etcd" behavior cannot proceed with a
misconfigured name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8cb5a201-547d-4af8-aec3-e4eecdbf2b1b
📒 Files selected for processing (72)
api/backups/strategy/v1alpha1/etcd_types.goexamples/backups/etcd/00-helpers.shexamples/backups/etcd/03-create-etcd-src.shexamples/backups/etcd/05-restore-in-place.shexamples/backups/etcd/90-scenario-admin-prepare.mdexamples/backups/etcd/91-scenario-user-backup.mdexamples/backups/etcd/92-scenario-user-restore.mdexamples/backups/etcd/README.mdhack/e2e-apps/etcd.batsinternal/backupcontroller/etcdapp/types.gointernal/backupcontroller/etcdstrategy_controller.gointernal/backupcontroller/etcdstrategy_controller_test.gointernal/backupcontroller/etcdtypes/types.gointernal/backupcontroller/etcdtypes/zz_generated.deepcopy.gopackages/core/platform/images/migrations/Dockerfilepackages/core/platform/images/migrations/migrations/44packages/core/platform/sources/etcd-operator.yamlpackages/core/platform/values.yamlpackages/extra/etcd/README.mdpackages/extra/etcd/templates/backup-secret.yamlpackages/extra/etcd/templates/etcd-backup-schedule.yamlpackages/extra/etcd/templates/etcd-cluster.yamlpackages/extra/etcd/templates/etcd-defrag.yamlpackages/extra/etcd/templates/vpa.yamlpackages/extra/etcd/tests/backup-secret_test.yamlpackages/extra/etcd/tests/etcd-backup-schedule_test.yamlpackages/extra/etcd/tests/topology-spread_test.yamlpackages/extra/etcd/values.schema.jsonpackages/extra/etcd/values.yamlpackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_etcds.yamlpackages/system/backupstrategy-controller/templates/rbac.yamlpackages/system/etcd-operator-crds/.helmignorepackages/system/etcd-operator-crds/Chart.yamlpackages/system/etcd-operator-crds/Makefilepackages/system/etcd-operator-crds/templates/etcdclusters.yamlpackages/system/etcd-operator-crds/templates/etcdmembers.yamlpackages/system/etcd-operator-crds/templates/etcdsnapshots.yamlpackages/system/etcd-operator/Chart.yamlpackages/system/etcd-operator/Makefilepackages/system/etcd-operator/charts/etcd-operator/.helmignorepackages/system/etcd-operator/charts/etcd-operator/Chart.yamlpackages/system/etcd-operator/charts/etcd-operator/README.mdpackages/system/etcd-operator/charts/etcd-operator/README.md.gotmplpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup-schedule.yamlpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup.yamlpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-cluster.yamlpackages/system/etcd-operator/charts/etcd-operator/templates/_helpers.tplpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/certificate.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/issuer.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/mutatingwebhookconfiguration.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/validatingwebhookconfiguration.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-manager-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-metrics-reader.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-proxy-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-manager-rolebinding.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-proxy-rolebinding.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/role-leader-election-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/rolebinding-leader-election-rolebinding.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/configmap-env.yamlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/deployment.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/service-controller-manager-metrics-service.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/service-webhook-service.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/serviceaccount.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/vpa.ymlpackages/system/etcd-operator/charts/etcd-operator/values.schema.jsonpackages/system/etcd-operator/charts/etcd-operator/values.yamlpackages/system/etcd-operator/templates/deployment.yamlpackages/system/etcd-operator/templates/metrics.yamlpackages/system/etcd-operator/templates/rbac.yamlpackages/system/etcd-operator/templates/vpa.yamlpackages/system/etcd-operator/values.yamlpackages/system/etcd-rd/cozyrds/etcd.yaml
💤 Files with no reviewable changes (31)
- packages/system/etcd-operator/charts/etcd-operator/README.md
- packages/extra/etcd/tests/backup-secret_test.yaml
- packages/system/etcd-operator/charts/etcd-operator/values.schema.json
- packages/system/etcd-operator/charts/etcd-operator/values.yaml
- packages/extra/etcd/tests/etcd-backup-schedule_test.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/service-controller-manager-metrics-service.yml
- packages/extra/etcd/templates/etcd-backup-schedule.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/_helpers.tpl
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-manager-role.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/rolebinding-leader-election-rolebinding.yml
- packages/system/etcd-operator/charts/etcd-operator/Chart.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/certificate.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/serviceaccount.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/service-webhook-service.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-proxy-rolebinding.yml
- packages/system/etcd-operator/charts/etcd-operator/.helmignore
- packages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup.yaml
- packages/extra/etcd/templates/backup-secret.yaml
- packages/system/etcd-operator/charts/etcd-operator/README.md.gotmpl
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-proxy-role.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/mutatingwebhookconfiguration.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/role-leader-election-role.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-metrics-reader.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/issuer.yml
- packages/system/etcd-operator/charts/etcd-operator/crds/etcd-cluster.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/deployment.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/configmap-env.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/vpa.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-manager-rolebinding.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/validatingwebhookconfiguration.yml
- packages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup-schedule.yaml
| update: | ||
| rm -rf templates | ||
| mkdir templates | ||
| kubectl kustomize "github.com/cozystack/etcd-operator/config/crd?ref=$(ETCD_OPERATOR_REF)" | \ | ||
| yq -s '"templates/" + (.metadata.name | sub("\..*$$"; "")) + ".yaml"' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify kubectl kustomize can fetch from GitHub URLs with ref parameter
# Check kubectl version
echo "=== kubectl version ==="
kubectl version --client --short 2>/dev/null || kubectl version --client
# Test the kustomize command (dry-run to avoid side effects)
echo -e "\n=== Testing kubectl kustomize GitHub URL ==="
kubectl kustomize "github.com/cozystack/etcd-operator/config/crd?ref=v0.5.0" >/dev/null 2>&1
if [ $? -eq 0 ]; then
echo "✓ kubectl kustomize GitHub URL pattern is supported"
else
echo "✗ kubectl kustomize GitHub URL pattern failed"
echo "This feature requires kubectl 1.14+ with kustomize 3.0+"
fiRepository: cozystack/cozystack
Length of output: 299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect Makefile snippet ==="
ls -la packages/system/etcd-operator-crds/Makefile
sed -n '1,120p' packages/system/etcd-operator-crds/Makefile
echo "=== Search for kubectl/kustomize version constraints in repo ==="
rg -n "KUBECTL_VERSION|kubectl.*version|kustomize.*version|KUSTOMIZE_VERSION|ETCD_OPERATOR_REF|ref=" -S . || true
echo "=== Search for other uses of kubectl kustomize GitHub URLs with ref= ==="
rg -n "kubectl kustomize .*github.com.*/.*\?ref=" -S . || trueRepository: cozystack/cozystack
Length of output: 26634
🌐 Web query:
kubectl kustomize GitHub URL ?ref support minimum version
💡 Result:
The kubectl kustomize command supports remote GitHub repository URLs with a?ref query parameter to specify a branch, tag, or commit hash [1][2]. Regarding the minimum version, the kustomize functionality was integrated into kubectl starting with version 1.14 [3]. While remote build support has been a long-standing feature, early versions of the integrated kustomize (specifically versions integrated prior to v3.5.5) contained bugs related to proper Git checkout behavior for specific refs or commit hashes [4]. To ensure reliable support for remote URL parameters like?ref, it is recommended to use a more recent version of kubectl (e.g., v1.21 or later), which updated the embedded kustomize to v4.0.5 and beyond [3]. Key usage details for remote URLs include: - URL Format: The path to the kustomization directory within the repository must be specified using a double slash (//) separator (e.g., https://github.com/org/repo.git//path/to/kustomization?ref=v1.0.6) [2]. - Supported Refs: You can use any git fetch-able reference, including branches, tags, or full commit hashes [2]. Short commit hashes are generally not supported [2].
Citations:
- 1: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_kustomize/
- 2: https://github.com/kubernetes-sigs/kustomize/blob/a7f4db7fb41e17b2c826a524f545e6174b4dc6ac/examples/remoteBuild.md
- 3: https://github.com/kubernetes-sigs/kustomize/blob/master/README.md
- 4: Kustomize unable to build remote base versioned with Git commit kubernetes-sigs/kustomize#2444
🌐 Web query:
kustomize remote base git ref parameter documentation ?ref=
💡 Result:
In Kustomize, the?ref= query parameter is used to specify a Git reference—such as a branch, tag, or full commit hash—when referencing a remote Git repository as a resource [1][2]. How it works: When adding a remote Git repository to your kustomization.yaml file (under resources), you can append this parameter to the repository URL to pin the configuration to a specific version [3]. Kustomize will clone the repository, check out the specified reference, and then build the resources from that state [1]. Syntax example: https://github.com/kubernetes-sigs/kustomize//path/to/directory/?ref=v1.0.6 [1][4] Key details: - Ref values: You can use branch names, tags, or full commit hashes [1][3]. Short commit hashes are generally not supported [1]. - Path separator: When specifying a subdirectory within the repository, use a double slash (//) to separate the repository URL from the path inside the repository [1][3]. - Additional parameters: The query string can also include other configurations, such as: - timeout: Specifies the duration for git operations (e.g.,?ref=v1.0.6&timeout=60s) [1][5][2]. - submodules: A boolean indicating whether to clone submodules (e.g.,?ref=main&submodules=false) [1][5][2]. It is recommended to use tags or full commit hashes for production environments to ensure configuration stability, while branches can be used for development workflows to automatically fetch the latest changes [3].
Citations:
- 1: https://github.com/kubernetes-sigs/kustomize/blob/a7f4db7fb41e17b2c826a524f545e6174b4dc6ac/examples/remoteBuild.md
- 2: https://deepwiki.com/kubernetes-sigs/kustomize/6.3-git-handling
- 3: https://oneuptime.com/blog/post/2026-02-09-kustomize-remote-bases-git/view
- 4: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_kustomize/
- 5: https://github.com/kubernetes-sigs/kustomize/blob/master/api/internal/git/repospec.go
🏁 Script executed:
rg -n "kubectl kustomize .*\\?ref=" -S packages hack internal || true
rg -n "\\?ref=.*//" -S packages hack internal || true
rg -n "kustomize build .*\\?ref=" -S packages hack internal || true
echo "=== Inspect hack/package.mk for kubectl tooling constraints (if any) ==="
ls -ლა hack/package.mk || true
sed -n '1,200p' hack/package.mk || trueRepository: cozystack/cozystack
Length of output: 2164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "kubectl kustomize .*\\?ref=" -S packages hack internal || true
rg -n "\\?ref=.*//" -S packages hack internal || true
rg -n "kustomize build .*\\?ref=" -S packages hack internal || true
echo "=== Inspect hack/package.mk for kubectl tooling constraints (if any) ==="
ls -la hack/package.mk || true
sed -n '1,220p' hack/package.mk || trueRepository: cozystack/cozystack
Length of output: 2147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "KUBECTL_VERSION|KUSTOMIZE_VERSION|kustomize version|kubectl version" -S . || trueRepository: cozystack/cozystack
Length of output: 413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for Dockerfiles/images that download kubectl
rg -n "dl.k8s.io/release/v\\$\\{KUBECTL_VERSION\\}|dl.k8s.io/release/v" -S packages hack . || trueRepository: cozystack/cozystack
Length of output: 431
🌐 Web query:
kustomize remote build GitHub URL syntax without double slash // before path ?ref=
💡 Result:
The double slash (//) is the recommended and standard convention in Kustomize for separating the remote Git repository URL from the subdirectory path containing the kustomization.yaml file [1][2][3]. While Kustomize has historically supported various URL parsing methods—including using the.git suffix or other markers to identify the repository root—the // delimiter is the explicit, non-ambiguous marker designed to separate the repository root from the kustomization path [4][5]. Using // ensures that Kustomize correctly identifies where the repository ends and the relative path to the target configuration begins [4]. If you omit the //, Kustomize may attempt to interpret the entire string differently, potentially causing errors or relying on older, less reliable parsing logic that attempts to "guess" the repository boundary [4][5]. Key points on the syntax: 1. Recommended format: https://github.com/org/repo//path/to/kustomization?ref=branch-or-tag [2][3]. 2. The // is a convention, not a real path element, and is used by Kustomize to split the repository root from the internal path [4]. 3. The?ref= parameter is used to specify the git reference (branch, tag, or commit) and is appended after the path (or after the repo URL if no path is specified) [1][2]. Using // is the most robust way to reference remote bases and is explicitly documented in Kustomize's remote build examples to avoid ambiguity [1][2][6].
Citations:
- 1: https://github.com/kubernetes-sigs/kustomize/blob/a7f4db7fb41e17b2c826a524f545e6174b4dc6ac/examples/remoteBuild.md
- 2: https://github.com/kubernetes-sigs/kustomize/blob/master/examples/remoteBuild.md
- 3: https://oneuptime.com/blog/post/2026-02-09-kustomize-remote-bases-git/view
- 4: https://github.com/kubernetes-sigs/kustomize/blob/master/api/internal/git/repospec.go
- 5: [Question] Kustomize URL formats kubernetes-sigs/kustomize#4454
- 6: https://oneuptime.com/blog/post/2026-03-05-remote-kustomize-bases-flux/view
🌐 Web query:
kubectl kustomize remote github.com URL without https and without // path ?ref
💡 Result:
Kustomize supports a simplified GitHub URL format that does not require the https:// prefix or the // path separator, though the behavior depends on the version and specific context [1][2]. The shorthand format github.com/owner/repo is a legacy style that Kustomize supports for backwards compatibility [1]. Modern Kustomize documentation generally recommends using full HTTPS or SSH Git clone URLs (e.g., https://github.com/owner/repo.git or git@github.com:owner/repo.git) to ensure reliability [1][3]. Regarding the // separator: The double slash (//) is used in Kustomize specifically to delimit the repository URL from the subdirectory path containing the kustomization.yaml file [1]. If your repository has a kustomization.yaml file at the root, you do not need the // path separator [1][2]. If you are referencing a subdirectory, the // is the standard convention to specify that path within the repository [1]. Important notes on URL handling: - Legacy vs. Modern: While shorthands like github.com/owner/repo work, they are not the preferred format for modern configurations [1]. - HTTPS vs. SSH: Kustomize supports both. SSH URLs (e.g., git@github.com:owner/repo) are frequently used for authentication-heavy environments [1]. - Automatic Detection: Kustomize attempts to determine if a string is a remote Git repository or a local file path [4][5]. If a URL format is ambiguous or missing expected indicators (like the scheme or common git delimiters), Kustomize may first attempt to load it as a local file, which can lead to confusing error messages if the path is not actually local [5][6]. For the most stable results, use the full Git URL format: https://github.com/owner/repo//path/to/kustomization?ref=branch-or-tag [1]
Citations:
- 1: https://github.com/kubernetes-sigs/kustomize/blob/a7f4db7fb41e17b2c826a524f545e6174b4dc6ac/examples/remoteBuild.md
- 2: kustomize build {repoUrl} kubernetes-sigs/kustomize#248
- 3: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_kustomize/
- 4: https://github.com/kubernetes-sigs/kustomize/blob/master/api/internal/git/repospec.go
- 5: [Question] Kustomize URL formats kubernetes-sigs/kustomize#4454
- 6: Refactor parseGitURL kubernetes-sigs/kustomize#4983
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "KUBECTL_VERSION|KUSTOMIZE_VERSION|kustomize version|kubectl version" -S . || trueRepository: cozystack/cozystack
Length of output: 413
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for Dockerfiles/images that download kubectl
rg -n "dl.k8s.io/release/v\\$\\{KUBECTL_VERSION\\}|dl.k8s.io/release/v" -S packages hack . || trueRepository: cozystack/cozystack
Length of output: 431
🌐 Web query:
kustomize remote build GitHub URL syntax without double slash // before path ?ref=
💡 Result:
Kustomize officially documents the use of a double slash (//) to explicitly delimit the repository root from the subdirectory containing the kustomization target [1]. This syntax helps Kustomize unambiguously identify the repository URL versus the internal path [2]. Official Syntax: https://github.com/org/repo//path/to/kustomization/root?ref=version Why the // exists: The // acts as an explicit marker [3]. While Kustomize may attempt to guess the repository root by detecting.git extensions or other conventions, these methods can be unreliable or ambiguous [3][2]. Using // ensures the tool correctly parses the repository base and the target subdirectory, particularly when paths are complex or branch names/tags are involved [1][3]. Can you use a single slash? You may find that single slashes sometimes work or have worked in specific versions of Kustomize (often due to underlying libraries like go-getter) [4][5]. However, this behavior is not the documented standard and can lead to inconsistent results, parsing errors, or bugs depending on the environment, the repository structure, or the Kustomize version [1][4]. It is strongly recommended to use the // syntax to ensure stability and compatibility [1][2]. Summary: Always prefer the // delimiter for remote builds to avoid ambiguity in how the repository and target path are split [1][2]. If your URL includes a?ref= query parameter, it should follow the path, for example: github.com/owner/repo//path/to/base?ref=main [1][5].
Citations:
- 1: https://github.com/kubernetes-sigs/kustomize/blob/a7f4db7fb41e17b2c826a524f545e6174b4dc6ac/examples/remoteBuild.md
- 2: [Question] Kustomize URL formats kubernetes-sigs/kustomize#4454
- 3: https://github.com/kubernetes-sigs/kustomize/blob/master/api/internal/git/repospec.go
- 4: kustomize 3.5.5 breaks installing ArgoCD from URL kubernetes-sigs/kustomize#2538
- 5: Kustomize unable to build remote base versioned with Git commit kubernetes-sigs/kustomize#2444
🌐 Web query:
kubectl kustomize remote github.com URL without https and without // path ?ref
💡 Result:
Kustomize supports a legacy URL format for remote GitHub repositories that omits the https:// scheme and the // path separator [1]. This format is maintained for backward compatibility [1]. When using this legacy format, you can specify a target in a GitHub repository as follows [1]: github.com/OWNER/REPO/PATH?ref=BRANCH_OR_COMMIT For example: kubectl kustomize github.com/kubernetes-sigs/kustomize/examples/helloWorld?ref=v1.0.6 Key details regarding this format: - Scheme Omission: You do not need to include https:// [1]. - Path Separator: Unlike the standard modern format (which requires // to separate the repository URL from the subdirectory path [1]), this legacy syntax uses a standard slash / to denote paths within the repository [1]. - Versioning: You specify the branch, tag, or commit hash using the?ref query parameter [1]. Note that while this legacy format remains supported, the modern Kustomize URL format (using https:// and //) is the recommended approach [1]. Using // explicitly in the URL helps distinguish remote repository targets from local directory paths, preventing potential ambiguity [1][2].
Citations:
- 1: https://github.com/kubernetes-sigs/kustomize/blob/a7f4db7fb41e17b2c826a524f545e6174b4dc6ac/examples/remoteBuild.md
- 2: [Question] Kustomize URL formats kubernetes-sigs/kustomize#4454
Update kubectl kustomize ?ref GitHub URL requirement (syntax is supported, but version isn’t pinned here)
The update target uses a valid kustomize remote-build shorthand (github.com/<org>/<repo>/<path>?ref=<git-ref>; no https:// or // required). The same pattern is already used in packages/system/gateway-api-crds/Makefile.
This Makefile doesn’t declare a minimum kubectl version for these generation targets—ensure build/CI uses kubectl with embedded kustomize support (kubectl 1.14+), or switch to the explicit https://github.com/...//config/crd?ref=... form for added robustness.
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 9-9: Required target "all" is missing from the Makefile.
(minphony)
[warning] 9-9: Required target "clean" is missing from the Makefile.
(minphony)
[warning] 9-9: Required target "test" is missing from the Makefile.
(minphony)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/etcd-operator-crds/Makefile` around lines 9 - 13, The update
target in the Makefile uses the kubectl kustomize remote shorthand (the update
recipe invoking "kubectl kustomize
\"github.com/cozystack/etcd-operator/config/crd?ref=$(ETCD_OPERATOR_REF)\""),
which relies on kubectl embedding kustomize; either require/validate a minimum
kubectl version or make the remote path explicit to avoid ambiguity—update the
Makefile’s update target to use the explicit GitHub URL form
("https://github.com/...//config/crd?ref=$(ETCD_OPERATOR_REF)") or add a
preflight check/comment that CI/builds must use kubectl >=1.14 (and/or add a
simple version check using kubectl version --client) so the ETCD_OPERATOR_REF
variable remains used and generation remains robust.
| x-kubernetes-validations: | ||
| - message: exactly one of destination.s3 or destination.pvc must be | ||
| set | ||
| rule: has(self.s3) != has(self.pvc) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check minimum Kubernetes version requirements in the repository
echo "=== Searching for Kubernetes version constraints ==="
# Check platform values, CI configs, documentation for K8s version
rg -n -C2 'kubernetes.*version|k8s.*version|kubeVersion' \
--iglob '*values*.yaml' \
--iglob '*Chart*.yaml' \
--iglob '*README*.md' \
--iglob '*.github/workflows/*'
# Check if other CRDs use x-kubernetes-validations
echo -e "\n=== Checking for existing CEL validation usage ==="
rg -l 'x-kubernetes-validations' --type yaml
# CEL validation requires K8s 1.25+
echo -e "\n=== Note: x-kubernetes-validations requires Kubernetes 1.25+ ==="Repository: cozystack/cozystack
Length of output: 37999
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Repo minimum Kubernetes version (README / docs / workflows / root charts) ==="
rg -n --hidden --no-ignore \
'minimum.*kubernetes|min.*kubernetes|supported.*kubernetes|kube.*version|Kubernetes v1\.|k8s v1\.' \
README*.md docs*.md .github/workflows/* Chart*.yaml charts/*.yaml 2>/dev/null || true
echo -e "\n=== Root Chart.yaml kubeVersion (if present) ==="
rg -n 'kubeVersion:' -S --glob '*/Chart.yaml' . 2>/dev/null | head -n 50Repository: cozystack/cozystack
Length of output: 349
🌐 Web query:
Kubernetes x-kubernetes-validations CEL CustomResourceDefinition minimum version 1.25
💡 Result:
The Kubernetes x-kubernetes-validations extension, which enables validation of CustomResourceDefinitions (CRDs) using the Common Expression Language (CEL), was introduced as a Beta feature in Kubernetes 1.25 [1][2][3]. Key milestones for this feature include: - Kubernetes 1.25: The feature graduated to Beta, allowing users to define validation rules directly within CRD schemas [1][3]. - Kubernetes 1.29: The feature graduated to Generally Available (GA) [4][5]. While basic validation rules became available in version 1.25, the feature set has continued to evolve; for instance, capabilities such as messageExpression and refined transition rule behaviors have been added in subsequent releases [2]. Therefore, Kubernetes 1.25 is the minimum version for utilizing the core x-kubernetes-validations functionality [1][2].
Citations:
- 1: https://kubernetes.website.cncfstack.com/blog/2022/09/23/crd-validation-rules-beta/
- 2: https://medium.com/@rameshavutu/kubernetes-cel-crd-validation-replace-webhooks-96ee347232b1
- 3: https://kubernetes.io/blog/2022/09/23/crd-validation-rules-beta/
- 4: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/
- 5: https://opensource.googleblog.com/2023/11/kubernetes-crd-validation-using-cel.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== etcd-operator-crds Chart.yaml kubeVersion ==="
fd -a "Chart.yaml" packages/system/etcd-operator-crds -t f -x echo {} | head -n 20
fd -t f "Chart.yaml" packages/system/etcd-operator-crds | xargs -I{} sh -c 'echo "--- {}"; rg -n "kubeVersion:" "{}" || true'
echo -e "\n=== Confirm x-kubernetes-validations usage in etcdsnapshots.yaml ==="
rg -n "x-kubernetes-validations" packages/system/etcd-operator-crds/templates/etcdsnapshots.yaml || true
echo -e "\n=== Show surrounding CEL rule lines ==="
rg -n -C3 "has\(self\.s3\) != has\(self\.pvc\)" packages/system/etcd-operator-crds/templates/etcdsnapshots.yaml || trueRepository: cozystack/cozystack
Length of output: 690
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Read packages/system/etcd-operator-crds/Chart.yaml ==="
cat -n packages/system/etcd-operator-crds/Chart.yaml | head -n 120
echo -e "\n=== Find any Chart.yaml kubeVersion constraints (top 50 files with matches) ==="
rg -n '^\s*kubeVersion:' -S --glob '**/Chart.yaml' . | head -n 50Repository: cozystack/cozystack
Length of output: 2714
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== File header (first 120 lines) ==="
cat -n packages/system/etcd-operator-crds/templates/etcdsnapshots.yaml | head -n 120
echo -e "\n=== Check whether Chart.yaml defines kubeVersion ==="
rg -n 'kubeVersion:' packages/system/etcd-operator-crds/Chart.yaml || trueRepository: cozystack/cozystack
Length of output: 6232
Set a Kubernetes >=1.25 requirement for the CRD’s x-kubernetes-validations CEL rule
The CRD at packages/system/etcd-operator-crds/templates/etcdsnapshots.yaml uses x-kubernetes-validations with a CEL rule:
x-kubernetes-validations:
- message: exactly one of destination.s3 or destination.pvc must be
set
rule: has(self.s3) != has(self.pvc)This mechanism requires Kubernetes 1.25+. However, packages/system/etcd-operator-crds/Chart.yaml has no kubeVersion constraint, so Helm could install it on older clusters where the CRD field may be rejected—add kubeVersion: ">=1.25.0-0" (and/or document the requirement).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/etcd-operator-crds/templates/etcdsnapshots.yaml` around lines
141 - 144, Add a Kubernetes version constraint to the Helm chart so the CRD
using the CEL validation rule (x-kubernetes-validations with rule: has(self.s3)
!= has(self.pvc)) is not installed on clusters older than 1.25; update the
Chart.yaml to include kubeVersion: ">=1.25.0-0" (or document the requirement) so
Helm prevents installation on unsupported Kubernetes versions and the CRD field
won’t be rejected at apply time.
9ee2d69 to
8b1c498
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core/platform/images/migrations/migrations/44 (1)
150-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOperator can be left scaled to 0 on failure.
If
etcd-migrate --applyfails (line 161-164), the script exits due toset -ebut the operator deployment remains at 0 replicas, leaving etcd clusters unmanaged until manual intervention. Additionally,|| trueon lines 153 and 168 masks rollout failures.🔧 Suggested fix: add trap to restore operator on any exit
+scaled_down=0 +restore_operator() { + if [ "$scaled_down" = "1" ]; then + echo "Restoring ${ETCD_OPERATOR_NS}/${ETCD_OPERATOR_DEPLOY} to 1 replica..." + kubectl -n "$ETCD_OPERATOR_NS" scale deploy "$ETCD_OPERATOR_DEPLOY" --replicas=1 || true + fi +} + if [ "$LEGACY_COUNT" -gt 0 ]; then + trap restore_operator EXIT echo "Scaling ${ETCD_OPERATOR_NS}/${ETCD_OPERATOR_DEPLOY} to 0 for adoption..." kubectl -n "$ETCD_OPERATOR_NS" scale deploy "$ETCD_OPERATOR_DEPLOY" --replicas=0 - kubectl -n "$ETCD_OPERATOR_NS" rollout status deploy "$ETCD_OPERATOR_DEPLOY" --timeout=120s || true + kubectl -n "$ETCD_OPERATOR_NS" rollout status deploy "$ETCD_OPERATOR_DEPLOY" --timeout=120s + scaled_down=1 ... echo "Scaling ${ETCD_OPERATOR_NS}/${ETCD_OPERATOR_DEPLOY} back to 1..." kubectl -n "$ETCD_OPERATOR_NS" scale deploy "$ETCD_OPERATOR_DEPLOY" --replicas=1 - kubectl -n "$ETCD_OPERATOR_NS" rollout status deploy "$ETCD_OPERATOR_DEPLOY" --timeout=180s || true + kubectl -n "$ETCD_OPERATOR_NS" rollout status deploy "$ETCD_OPERATOR_DEPLOY" --timeout=180s + scaled_down=0 + trap - EXIT fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/platform/images/migrations/migrations/44` around lines 150 - 169, The etcd-migrate operation can fail and cause the script to exit via set -e, leaving the ETCD operator deployment at 0 replicas and etcd clusters unmanaged. Add a trap handler at the beginning of the script that will automatically scale the operator deployment back to 1 replica on any script exit (success or failure). This trap should execute the kubectl scale command for the ETCD_OPERATOR_DEPLOY with --replicas=1 in the ETCD_OPERATOR_NS namespace. Additionally, reconsider the || true flags on the kubectl rollout status commands (lines 153 and 168) as they mask rollout failures and prevent visibility into potential deployment issues during the adoption process.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/system/etcd-operator/Makefile`:
- Around line 14-16: The `update` target in the Makefile for etcd-operator
currently only prints instructions instead of performing automated vendoring,
which violates the required system chart umbrella pattern. Replace the echo
statements in the `update` target with actual automation that vendors the
upstream charts into a `charts/` subdirectory following the established pattern
used by other packages in the `packages/system/**` directory. This should
include automated steps to pull the rbac.yaml from the upstream reference and
update the CRDs, making the vendoring process automatic rather than manual.
In `@packages/system/etcd-operator/templates/rbac.yaml`:
- Around line 15-50: The ClusterRole definition is overly permissive and
violates the principle of least privilege. Narrow the scope by removing
unnecessary verbs from sensitive core resources: for the secrets resource rule,
remove the list and watch verbs (keep only get if absolutely needed for the
operator's function), and consider restricting it to specific secret names
rather than cluster-wide access. For pods, persistentvolumeclaims, and services
resources, audit which verbs are actually required by the etcd-operator
controller and remove unnecessary ones (for example, evaluate if patch and
update are truly needed for pods). Where possible, replace cluster-wide
ClusterRole rules for core resources with Role rules scoped to the operator's
namespace to limit blast radius if the controller pod is compromised.
---
Duplicate comments:
In `@packages/core/platform/images/migrations/migrations/44`:
- Around line 150-169: The etcd-migrate operation can fail and cause the script
to exit via set -e, leaving the ETCD operator deployment at 0 replicas and etcd
clusters unmanaged. Add a trap handler at the beginning of the script that will
automatically scale the operator deployment back to 1 replica on any script exit
(success or failure). This trap should execute the kubectl scale command for the
ETCD_OPERATOR_DEPLOY with --replicas=1 in the ETCD_OPERATOR_NS namespace.
Additionally, reconsider the || true flags on the kubectl rollout status
commands (lines 153 and 168) as they mask rollout failures and prevent
visibility into potential deployment issues during the adoption process.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f990be4-b82d-407f-aad1-2d2d6e132048
📒 Files selected for processing (72)
api/backups/strategy/v1alpha1/etcd_types.goexamples/backups/etcd/00-helpers.shexamples/backups/etcd/03-create-etcd-src.shexamples/backups/etcd/05-restore-in-place.shexamples/backups/etcd/90-scenario-admin-prepare.mdexamples/backups/etcd/91-scenario-user-backup.mdexamples/backups/etcd/92-scenario-user-restore.mdexamples/backups/etcd/README.mdhack/e2e-apps/etcd.batsinternal/backupcontroller/etcdapp/types.gointernal/backupcontroller/etcdstrategy_controller.gointernal/backupcontroller/etcdstrategy_controller_test.gointernal/backupcontroller/etcdtypes/types.gointernal/backupcontroller/etcdtypes/zz_generated.deepcopy.gopackages/core/platform/images/migrations/Dockerfilepackages/core/platform/images/migrations/migrations/44packages/core/platform/sources/etcd-operator.yamlpackages/core/platform/values.yamlpackages/extra/etcd/README.mdpackages/extra/etcd/templates/backup-secret.yamlpackages/extra/etcd/templates/etcd-backup-schedule.yamlpackages/extra/etcd/templates/etcd-cluster.yamlpackages/extra/etcd/templates/etcd-defrag.yamlpackages/extra/etcd/templates/vpa.yamlpackages/extra/etcd/tests/backup-secret_test.yamlpackages/extra/etcd/tests/etcd-backup-schedule_test.yamlpackages/extra/etcd/tests/topology-spread_test.yamlpackages/extra/etcd/values.schema.jsonpackages/extra/etcd/values.yamlpackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_etcds.yamlpackages/system/backupstrategy-controller/templates/rbac.yamlpackages/system/etcd-operator-crds/.helmignorepackages/system/etcd-operator-crds/Chart.yamlpackages/system/etcd-operator-crds/Makefilepackages/system/etcd-operator-crds/templates/etcdclusters.yamlpackages/system/etcd-operator-crds/templates/etcdmembers.yamlpackages/system/etcd-operator-crds/templates/etcdsnapshots.yamlpackages/system/etcd-operator/Chart.yamlpackages/system/etcd-operator/Makefilepackages/system/etcd-operator/charts/etcd-operator/.helmignorepackages/system/etcd-operator/charts/etcd-operator/Chart.yamlpackages/system/etcd-operator/charts/etcd-operator/README.mdpackages/system/etcd-operator/charts/etcd-operator/README.md.gotmplpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup-schedule.yamlpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup.yamlpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-cluster.yamlpackages/system/etcd-operator/charts/etcd-operator/templates/_helpers.tplpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/certificate.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/issuer.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/mutatingwebhookconfiguration.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/validatingwebhookconfiguration.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-manager-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-metrics-reader.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-proxy-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-manager-rolebinding.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-proxy-rolebinding.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/role-leader-election-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/rolebinding-leader-election-rolebinding.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/configmap-env.yamlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/deployment.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/service-controller-manager-metrics-service.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/service-webhook-service.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/serviceaccount.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/vpa.ymlpackages/system/etcd-operator/charts/etcd-operator/values.schema.jsonpackages/system/etcd-operator/charts/etcd-operator/values.yamlpackages/system/etcd-operator/templates/deployment.yamlpackages/system/etcd-operator/templates/metrics.yamlpackages/system/etcd-operator/templates/rbac.yamlpackages/system/etcd-operator/templates/vpa.yamlpackages/system/etcd-operator/values.yamlpackages/system/etcd-rd/cozyrds/etcd.yaml
💤 Files with no reviewable changes (31)
- packages/system/etcd-operator/charts/etcd-operator/README.md.gotmpl
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/issuer.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/role-leader-election-role.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/vpa.yml
- packages/system/etcd-operator/charts/etcd-operator/values.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/service-controller-manager-metrics-service.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/validatingwebhookconfiguration.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/serviceaccount.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/service-webhook-service.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/_helpers.tpl
- packages/system/etcd-operator/charts/etcd-operator/.helmignore
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/configmap-env.yaml
- packages/extra/etcd/templates/etcd-backup-schedule.yaml
- packages/extra/etcd/tests/etcd-backup-schedule_test.yaml
- packages/system/etcd-operator/charts/etcd-operator/README.md
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/mutatingwebhookconfiguration.yml
- packages/system/etcd-operator/charts/etcd-operator/crds/etcd-cluster.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-proxy-rolebinding.yml
- packages/extra/etcd/tests/backup-secret_test.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-manager-rolebinding.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/deployment.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-proxy-role.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/certificate.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-metrics-reader.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-manager-role.yml
- packages/system/etcd-operator/charts/etcd-operator/values.schema.json
- packages/extra/etcd/templates/backup-secret.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/rolebinding-leader-election-rolebinding.yml
- packages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup.yaml
- packages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup-schedule.yaml
- packages/system/etcd-operator/charts/etcd-operator/Chart.yaml
✅ Files skipped from review due to trivial changes (11)
- packages/system/etcd-operator-crds/.helmignore
- packages/system/etcd-operator/values.yaml
- examples/backups/etcd/90-scenario-admin-prepare.md
- packages/system/etcd-operator-crds/Chart.yaml
- examples/backups/etcd/92-scenario-user-restore.md
- packages/system/etcd-operator/Chart.yaml
- api/backups/strategy/v1alpha1/etcd_types.go
- packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_etcds.yaml
- internal/backupcontroller/etcdapp/types.go
- examples/backups/etcd/README.md
- internal/backupcontroller/etcdtypes/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (21)
- packages/core/platform/values.yaml
- packages/system/etcd-operator/templates/metrics.yaml
- examples/backups/etcd/03-create-etcd-src.sh
- examples/backups/etcd/05-restore-in-place.sh
- examples/backups/etcd/91-scenario-user-backup.md
- packages/extra/etcd/templates/etcd-defrag.yaml
- hack/e2e-apps/etcd.bats
- packages/extra/etcd/tests/topology-spread_test.yaml
- packages/extra/etcd/templates/vpa.yaml
- examples/backups/etcd/00-helpers.sh
- packages/core/platform/sources/etcd-operator.yaml
- packages/system/etcd-rd/cozyrds/etcd.yaml
- packages/extra/etcd/values.yaml
- packages/system/etcd-operator-crds/templates/etcdsnapshots.yaml
- packages/system/backupstrategy-controller/templates/rbac.yaml
- packages/extra/etcd/values.schema.json
- internal/backupcontroller/etcdtypes/types.go
- packages/extra/etcd/README.md
- packages/extra/etcd/templates/etcd-cluster.yaml
- internal/backupcontroller/etcdstrategy_controller.go
- internal/backupcontroller/etcdstrategy_controller_test.go
| @echo "etcd-operator is a cozystack-authored chart; refresh templates/rbac.yaml" | ||
| @echo "from github.com/cozystack/etcd-operator config/rbac/role.yaml @ $(ETCD_OPERATOR_REF)" | ||
| @echo "and re-vendor CRDs via 'make -C ../etcd-operator-crds update'." |
There was a problem hiding this comment.
Manual update flow breaks the required system chart vendoring contract.
Line 14-Line 16 turns update into instructions-only, which makes upstream synchronization manual and bypasses the required packages/system/** vendored-chart umbrella pattern (charts/-based flow). Please restore an automated vendoring path consistent with that contract.
As per coding guidelines, packages/system/** must follow Helm Charts umbrella pattern with vendored upstream charts in charts/ subdirectory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/etcd-operator/Makefile` around lines 14 - 16, The `update`
target in the Makefile for etcd-operator currently only prints instructions
instead of performing automated vendoring, which violates the required system
chart umbrella pattern. Replace the echo statements in the `update` target with
actual automation that vendors the upstream charts into a `charts/` subdirectory
following the established pattern used by other packages in the
`packages/system/**` directory. This should include automated steps to pull the
rbac.yaml from the upstream reference and update the CRDs, making the vendoring
process automatic rather than manual.
Source: Coding guidelines
| - apiGroups: [""] | ||
| resources: [persistentvolumeclaims] | ||
| verbs: [create, delete, get, list, patch, update, watch] | ||
| - apiGroups: [""] | ||
| resources: [pods] | ||
| verbs: [create, delete, get, list, patch, watch] | ||
| - apiGroups: [""] | ||
| resources: [pods/log] | ||
| verbs: [get] | ||
| - apiGroups: [""] | ||
| resources: [secrets] | ||
| verbs: [get, list, watch] | ||
| - apiGroups: [""] | ||
| resources: [services] | ||
| verbs: [create, get, list, patch, update, watch] | ||
| - apiGroups: [batch] | ||
| resources: [jobs] | ||
| verbs: [create, delete, get, list, watch] | ||
| - apiGroups: [cert-manager.io] | ||
| resources: [certificates] | ||
| verbs: [create, get, list, patch, update, watch] | ||
| - apiGroups: [etcd-operator.cozystack.io] | ||
| resources: [etcdclusters] | ||
| verbs: [get, list, watch] | ||
| - apiGroups: [etcd-operator.cozystack.io] | ||
| resources: [etcdclusters/finalizers, etcdmembers/finalizers, etcdsnapshots/finalizers] | ||
| verbs: [update] | ||
| - apiGroups: [etcd-operator.cozystack.io] | ||
| resources: [etcdclusters/status, etcdmembers/status, etcdsnapshots/status] | ||
| verbs: [get, patch, update] | ||
| - apiGroups: [etcd-operator.cozystack.io] | ||
| resources: [etcdmembers, etcdsnapshots] | ||
| verbs: [create, delete, get, list, patch, update, watch] | ||
| - apiGroups: [policy] | ||
| resources: [poddisruptionbudgets] | ||
| verbs: [create, delete, get, list, patch, update, watch] |
There was a problem hiding this comment.
ClusterRole is overly broad for sensitive core resources.
Line 15-Line 50 grants cluster-wide access including secrets (get/list/watch) and mutable core resources (services, pods, persistentvolumeclaims). If the controller pod is compromised, this enables cluster-wide secret exfiltration and broad infrastructure manipulation. Please narrow to least privilege (scope core-resource access per managed namespace and drop unnecessary verbs, especially secret enumeration where possible).
🧰 Tools
🪛 Trivy (0.69.3)
[error] 19-21: Manage secrets
ClusterRole 'etcd-operator-manager-role' shouldn't have access to manage resource 'secrets'
Rule: KSV-0041
(IaC/Kubernetes)
[error] 22-24: Manage Kubernetes networking
ClusterRole 'etcd-operator-manager-role' should not have access to resources ["services", "endpoints", "endpointslices", "networkpolicies", "ingresses"] for verbs ["create", "update", "patch", "delete", "deletecollection", "impersonate", "*"]
Rule: KSV-0056
(IaC/Kubernetes)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/system/etcd-operator/templates/rbac.yaml` around lines 15 - 50, The
ClusterRole definition is overly permissive and violates the principle of least
privilege. Narrow the scope by removing unnecessary verbs from sensitive core
resources: for the secrets resource rule, remove the list and watch verbs (keep
only get if absolutely needed for the operator's function), and consider
restricting it to specific secret names rather than cluster-wide access. For
pods, persistentvolumeclaims, and services resources, audit which verbs are
actually required by the etcd-operator controller and remove unnecessary ones
(for example, evaluate if patch and update are truly needed for pods). Where
possible, replace cluster-wide ClusterRole rules for core resources with Role
rules scoped to the operator's namespace to limit blast radius if the controller
pod is compromised.
Source: Linters/SAST tools
8b1c498 to
2d6e68c
Compare
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)
packages/core/platform/images/migrations/Dockerfile (1)
8-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the migrations image as a non-root user.
The image still defaults to root, which increases blast radius if migration tooling/scripts are compromised at runtime. Add a dedicated unprivileged user before
ENTRYPOINT.Suggested patch
COPY migrations /migrations COPY run-migrations.sh /usr/bin/run-migrations.sh + +RUN addgroup -S cozy && adduser -S -G cozy -h /nonexistent cozy \ + && chown -R cozy:cozy /migrations /usr/bin/run-migrations.sh + +USER cozy WORKDIR /migrations🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/platform/images/migrations/Dockerfile` around lines 8 - 37, The Dockerfile currently runs as the root user by default, which increases security risk if the migration tooling is compromised. Before the ENTRYPOINT instruction at the end of the Dockerfile, add a RUN command to create a dedicated unprivileged user (for example, a user named "migrations") and then add a USER instruction to switch to that new user. Ensure the created user has the necessary permissions to read and execute the migration files in the /migrations directory and the /usr/bin/run-migrations.sh script.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/e2e-apps/etcd.bats`:
- Around line 84-96: The kubectl queries that extract the container name (around
the run command), metrics port (the mport variable assignment), and pod IP (the
pod_ip variable assignment) all use a broad selector app.kubernetes.io/name=etcd
and then dereference .items[0], which can select the wrong pod and make the test
flaky. Identify the strict member selector with running-phase filter that is
used earlier in this test file and apply that same selector to all three of
these kubectl commands to ensure you are consistently selecting the correct etcd
member pod before extracting the container name, metrics port, and pod IP
values.
---
Outside diff comments:
In `@packages/core/platform/images/migrations/Dockerfile`:
- Around line 8-37: The Dockerfile currently runs as the root user by default,
which increases security risk if the migration tooling is compromised. Before
the ENTRYPOINT instruction at the end of the Dockerfile, add a RUN command to
create a dedicated unprivileged user (for example, a user named "migrations")
and then add a USER instruction to switch to that new user. Ensure the created
user has the necessary permissions to read and execute the migration files in
the /migrations directory and the /usr/bin/run-migrations.sh script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47e12926-889c-47c9-aa94-798ca7803308
📒 Files selected for processing (75)
api/backups/strategy/v1alpha1/etcd_types.goexamples/backups/etcd/00-helpers.shexamples/backups/etcd/01-create-strategy.shexamples/backups/etcd/02-create-bucket.shexamples/backups/etcd/03-create-etcd-src.shexamples/backups/etcd/05-restore-in-place.shexamples/backups/etcd/90-scenario-admin-prepare.mdexamples/backups/etcd/91-scenario-user-backup.mdexamples/backups/etcd/92-scenario-user-restore.mdexamples/backups/etcd/README.mdhack/e2e-apps/etcd.batsinternal/backupcontroller/etcdapp/types.gointernal/backupcontroller/etcdstrategy_controller.gointernal/backupcontroller/etcdstrategy_controller_test.gointernal/backupcontroller/etcdtypes/types.gointernal/backupcontroller/etcdtypes/zz_generated.deepcopy.gopackages/core/platform/images/migrations/Dockerfilepackages/core/platform/images/migrations/migrations/44packages/core/platform/sources/etcd-operator.yamlpackages/core/platform/values.yamlpackages/extra/etcd/README.mdpackages/extra/etcd/templates/backup-secret.yamlpackages/extra/etcd/templates/etcd-backup-schedule.yamlpackages/extra/etcd/templates/etcd-cluster.yamlpackages/extra/etcd/templates/etcd-defrag.yamlpackages/extra/etcd/templates/vpa.yamlpackages/extra/etcd/tests/backup-secret_test.yamlpackages/extra/etcd/tests/backup-values-compat_test.yamlpackages/extra/etcd/tests/etcd-backup-schedule_test.yamlpackages/extra/etcd/tests/topology-spread_test.yamlpackages/extra/etcd/values.schema.jsonpackages/extra/etcd/values.yamlpackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_etcds.yamlpackages/system/backupstrategy-controller/templates/rbac.yamlpackages/system/etcd-operator-crds/.helmignorepackages/system/etcd-operator-crds/Chart.yamlpackages/system/etcd-operator-crds/Makefilepackages/system/etcd-operator-crds/templates/etcdclusters.yamlpackages/system/etcd-operator-crds/templates/etcdmembers.yamlpackages/system/etcd-operator-crds/templates/etcdsnapshots.yamlpackages/system/etcd-operator/Chart.yamlpackages/system/etcd-operator/Makefilepackages/system/etcd-operator/charts/etcd-operator/.helmignorepackages/system/etcd-operator/charts/etcd-operator/Chart.yamlpackages/system/etcd-operator/charts/etcd-operator/README.mdpackages/system/etcd-operator/charts/etcd-operator/README.md.gotmplpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup-schedule.yamlpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup.yamlpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-cluster.yamlpackages/system/etcd-operator/charts/etcd-operator/templates/_helpers.tplpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/certificate.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/issuer.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/mutatingwebhookconfiguration.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/validatingwebhookconfiguration.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-manager-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-metrics-reader.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-proxy-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-manager-rolebinding.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-proxy-rolebinding.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/role-leader-election-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/rolebinding-leader-election-rolebinding.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/configmap-env.yamlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/deployment.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/service-controller-manager-metrics-service.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/service-webhook-service.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/serviceaccount.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/vpa.ymlpackages/system/etcd-operator/charts/etcd-operator/values.schema.jsonpackages/system/etcd-operator/charts/etcd-operator/values.yamlpackages/system/etcd-operator/templates/deployment.yamlpackages/system/etcd-operator/templates/metrics.yamlpackages/system/etcd-operator/templates/rbac.yamlpackages/system/etcd-operator/templates/vpa.yamlpackages/system/etcd-operator/values.yamlpackages/system/etcd-rd/cozyrds/etcd.yaml
💤 Files with no reviewable changes (31)
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/serviceaccount.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-proxy-role.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/mutatingwebhookconfiguration.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/configmap-env.yaml
- packages/system/etcd-operator/charts/etcd-operator/.helmignore
- packages/extra/etcd/tests/etcd-backup-schedule_test.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/issuer.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/service-controller-manager-metrics-service.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-manager-role.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/certificate.yml
- packages/extra/etcd/templates/etcd-backup-schedule.yaml
- packages/system/etcd-operator/charts/etcd-operator/Chart.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/validatingwebhookconfiguration.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-proxy-rolebinding.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/_helpers.tpl
- packages/system/etcd-operator/charts/etcd-operator/README.md
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/vpa.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/deployment.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/rolebinding-leader-election-rolebinding.yml
- packages/extra/etcd/tests/backup-secret_test.yaml
- packages/system/etcd-operator/charts/etcd-operator/README.md.gotmpl
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-manager-rolebinding.yml
- packages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup-schedule.yaml
- packages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup.yaml
- packages/extra/etcd/templates/backup-secret.yaml
- packages/system/etcd-operator/charts/etcd-operator/values.schema.json
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/role-leader-election-role.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-metrics-reader.yml
- packages/system/etcd-operator/charts/etcd-operator/crds/etcd-cluster.yaml
- packages/system/etcd-operator/charts/etcd-operator/values.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/service-webhook-service.yml
✅ Files skipped from review due to trivial changes (14)
- packages/extra/etcd/tests/backup-values-compat_test.yaml
- examples/backups/etcd/02-create-bucket.sh
- examples/backups/etcd/90-scenario-admin-prepare.md
- packages/system/etcd-operator/values.yaml
- packages/system/etcd-operator-crds/Chart.yaml
- examples/backups/etcd/92-scenario-user-restore.md
- examples/backups/etcd/01-create-strategy.sh
- packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_etcds.yaml
- examples/backups/etcd/README.md
- examples/backups/etcd/91-scenario-user-backup.md
- internal/backupcontroller/etcdapp/types.go
- api/backups/strategy/v1alpha1/etcd_types.go
- internal/backupcontroller/etcdtypes/zz_generated.deepcopy.go
- packages/extra/etcd/values.yaml
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/extra/etcd/templates/vpa.yaml
- examples/backups/etcd/05-restore-in-place.sh
- packages/core/platform/sources/etcd-operator.yaml
- packages/system/etcd-operator/templates/metrics.yaml
- packages/extra/etcd/tests/topology-spread_test.yaml
- packages/core/platform/values.yaml
- packages/system/etcd-operator-crds/.helmignore
- examples/backups/etcd/03-create-etcd-src.sh
- packages/extra/etcd/README.md
- examples/backups/etcd/00-helpers.sh
- packages/system/etcd-operator-crds/templates/etcdsnapshots.yaml
- packages/extra/etcd/templates/etcd-cluster.yaml
- packages/extra/etcd/values.schema.json
- packages/core/platform/images/migrations/migrations/44
- packages/system/etcd-rd/cozyrds/etcd.yaml
- internal/backupcontroller/etcdstrategy_controller.go
2d6e68c to
c0552b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/platform/images/migrations/Dockerfile (1)
8-37: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRun the migration image as a non-root user.
The image currently defaults to root. Since this container carries cluster-admin migration credentials, drop privileges and make
/migrationswritable by the runtime user sorun-migrations.shcan stillchmodmigration files.Proposed hardening
COPY migrations /migrations COPY run-migrations.sh /usr/bin/run-migrations.sh +RUN addgroup -S migrations \ + && adduser -S -G migrations migrations \ + && chown -R migrations:migrations /migrations /usr/bin/run-migrations.sh \ + && chmod -R u+rx /migrations /usr/bin/run-migrations.sh +ENV HOME=/tmp +USER migrations WORKDIR /migrations🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/platform/images/migrations/Dockerfile` around lines 8 - 37, The migration image currently runs as root and should be hardened to use a non-root runtime user. Update the Dockerfile build flow to create and switch to a dedicated user before the final stage runtime, and adjust ownership/permissions on /migrations (and any writable paths used by run-migrations.sh) so the script can still chmod migration files without elevated privileges. Keep the changes localized around the existing WORKDIR and ENTRYPOINT setup in the Dockerfile.Source: Linters/SAST tools
🧹 Nitpick comments (3)
hack/migration-45-etcd-adopt.bats (2)
69-77: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert destructive steps execute exactly once.
Line 69/91 checks presence and ordering, but duplicate
SCALEorETCD-MIGRATE --applycalls would still pass. Addgrep -cassertions for exactly oneSCALE 0, oneETCD-MIGRATE --apply, and oneSCALE 1.Also applies to: 91-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/migration-45-etcd-adopt.bats` around lines 69 - 77, The migration test currently only checks that the expected SCALE and ETCD-MIGRATE --apply commands appear, so duplicate destructive steps could still pass. Update the assertions around the command log parsing in the migration test to use exact-count checks on the relevant entries, ensuring there is exactly one SCALE 0, exactly one ETCD-MIGRATE --apply, and exactly one SCALE 1 while keeping the existing flag checks on the apply command.
104-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a missing-credentials abort test case.
There is no test with
FAKE_CREDS=0. Add one that asserts migration fails and performs noSCALE 0/ETCD-MIGRATE --apply/STAMPwhencozy-backups-credsis unavailable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/migration-45-etcd-adopt.bats` around lines 104 - 166, Add a missing-credentials abort test in hack/migration-45-etcd-adopt.bats that sets FAKE_CREDS=0 and verifies the migration exits non-zero when cozy-backups-creds is unavailable. Use the existing test patterns around prep, bash "$MIG", and FAKE_CMDLOG to assert the failure path logs the missing-credentials case and performs no destructive actions, specifically no SCALE 0, no ETCD-MIGRATE --apply, and no STAMP.packages/system/etcd-operator/Makefile (1)
11-16: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPin
ETCD_OPERATOR_REFin one shared source.This ref is duplicated here and in
packages/system/etcd-operator-crds/Makefile. A future bump can leave the RBAC and CRDs vendored from different upstream revisions, which makes the two charts disagree on the operator contract. Move the pin into a shared include or derive one Makefile from the other.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/system/etcd-operator/Makefile` around lines 11 - 16, The operator ref is duplicated in both Makefiles, so keep the version pin in one shared place and have the other Makefile read from it. Update the etcd-operator and etcd-operator-crds build flow so both use the same source for ETCD_OPERATOR_REF, referencing the existing update target and shared vendoring steps to avoid RBAC/CRD drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/testdata/migration-45/kubectl`:
- Around line 46-50: The secret-read handling in kubectl is not honoring the
FAKE_CREDS contract: the JSONPath probe and the full secret read should both
reflect missing credentials when FAKE_CREDS=0. Update the secret-matching cases
in kubectl so the creds existence probe and the creds source path both fail or
return no data when FAKE_CREDS is disabled, while still succeeding only when
FAKE_CREDS is enabled; use the existing secret-read matchers for
cozy-backups-creds to keep the behavior consistent.
In `@packages/core/platform/images/migrations/Dockerfile`:
- Line 14: The Dockerfile build step currently fetches and runs the cozyhr
installer from the mutable main branch, which makes the migration image
non-reproducible and unsafe. Update the RUN step in the Dockerfile to use an
immutable ref for hack/install.sh (such as a tagged release or commit SHA) and
add checksum verification before executing it. Keep the change localized to the
image build logic and preserve the existing install.sh invocation path while
hardening the fetch-and-run flow.
In `@packages/core/platform/images/migrations/migrations/45`:
- Around line 162-163: The credential validation in the migration currently
checks only AWS_ACCESS_KEY_ID before proceeding, but the later adoption logic
also depends on AWS_SECRET_ACCESS_KEY. Update the pre-mutation guard in the
migration script to verify both secret keys exist in the secret using the same
kubectl/jsonpath pattern before any operator or cert mutations begin, so the
flow exits early if either key is missing.
- Around line 180-188: The Secret copy flow in the migration script is still
using client-side kubectl apply, which writes the full payload into
last-applied-configuration metadata. Update the kubectl invocation in the
secret-copy pipeline to use server-side apply instead, and keep the existing jq
cleanup in place so only the intended labels and metadata are preserved. Locate
the change around the secret migration command that ends with kubectl apply and
switch that apply step to server-side behavior for the copied Secret.
- Around line 289-304: The migration flow in the operator adoption script leaves
the deployment at 0 replicas if etcd-migrate --apply fails, and it also stamps
the version even when the rollout checks are not ready. Update the logic around
the etcd-migrate and kubectl rollout status steps so the operator is always
restored to 1 replica on failure, and make stamp_version run only after the
deployment has successfully rolled out and is ready; use the existing
ETCD_OPERATOR_NS, ETCD_OPERATOR_DEPLOY, etcd-migrate, and stamp_version symbols
to place the guard and cleanup correctly.
---
Outside diff comments:
In `@packages/core/platform/images/migrations/Dockerfile`:
- Around line 8-37: The migration image currently runs as root and should be
hardened to use a non-root runtime user. Update the Dockerfile build flow to
create and switch to a dedicated user before the final stage runtime, and adjust
ownership/permissions on /migrations (and any writable paths used by
run-migrations.sh) so the script can still chmod migration files without
elevated privileges. Keep the changes localized around the existing WORKDIR and
ENTRYPOINT setup in the Dockerfile.
---
Nitpick comments:
In `@hack/migration-45-etcd-adopt.bats`:
- Around line 69-77: The migration test currently only checks that the expected
SCALE and ETCD-MIGRATE --apply commands appear, so duplicate destructive steps
could still pass. Update the assertions around the command log parsing in the
migration test to use exact-count checks on the relevant entries, ensuring there
is exactly one SCALE 0, exactly one ETCD-MIGRATE --apply, and exactly one SCALE
1 while keeping the existing flag checks on the apply command.
- Around line 104-166: Add a missing-credentials abort test in
hack/migration-45-etcd-adopt.bats that sets FAKE_CREDS=0 and verifies the
migration exits non-zero when cozy-backups-creds is unavailable. Use the
existing test patterns around prep, bash "$MIG", and FAKE_CMDLOG to assert the
failure path logs the missing-credentials case and performs no destructive
actions, specifically no SCALE 0, no ETCD-MIGRATE --apply, and no STAMP.
In `@packages/system/etcd-operator/Makefile`:
- Around line 11-16: The operator ref is duplicated in both Makefiles, so keep
the version pin in one shared place and have the other Makefile read from it.
Update the etcd-operator and etcd-operator-crds build flow so both use the same
source for ETCD_OPERATOR_REF, referencing the existing update target and shared
vendoring steps to avoid RBAC/CRD drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3487d3ac-7286-4808-9ba4-91a06d263dd9
📒 Files selected for processing (78)
api/backups/strategy/v1alpha1/etcd_types.goexamples/backups/etcd/00-helpers.shexamples/backups/etcd/01-create-strategy.shexamples/backups/etcd/02-create-bucket.shexamples/backups/etcd/03-create-etcd-src.shexamples/backups/etcd/05-restore-in-place.shexamples/backups/etcd/90-scenario-admin-prepare.mdexamples/backups/etcd/91-scenario-user-backup.mdexamples/backups/etcd/92-scenario-user-restore.mdexamples/backups/etcd/README.mdhack/e2e-apps/etcd.batshack/migration-45-etcd-adopt.batshack/testdata/migration-45/etcd-migratehack/testdata/migration-45/kubectlinternal/backupcontroller/etcdapp/types.gointernal/backupcontroller/etcdstrategy_controller.gointernal/backupcontroller/etcdstrategy_controller_test.gointernal/backupcontroller/etcdtypes/types.gointernal/backupcontroller/etcdtypes/zz_generated.deepcopy.gopackages/core/platform/images/migrations/Dockerfilepackages/core/platform/images/migrations/migrations/45packages/core/platform/sources/etcd-operator.yamlpackages/core/platform/values.yamlpackages/extra/etcd/README.mdpackages/extra/etcd/templates/backup-secret.yamlpackages/extra/etcd/templates/etcd-backup-schedule.yamlpackages/extra/etcd/templates/etcd-cluster.yamlpackages/extra/etcd/templates/etcd-defrag.yamlpackages/extra/etcd/templates/vpa.yamlpackages/extra/etcd/tests/backup-secret_test.yamlpackages/extra/etcd/tests/backup-values-compat_test.yamlpackages/extra/etcd/tests/etcd-backup-schedule_test.yamlpackages/extra/etcd/tests/topology-spread_test.yamlpackages/extra/etcd/values.schema.jsonpackages/extra/etcd/values.yamlpackages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_etcds.yamlpackages/system/backupstrategy-controller/templates/rbac.yamlpackages/system/etcd-operator-crds/.helmignorepackages/system/etcd-operator-crds/Chart.yamlpackages/system/etcd-operator-crds/Makefilepackages/system/etcd-operator-crds/templates/etcdclusters.yamlpackages/system/etcd-operator-crds/templates/etcdmembers.yamlpackages/system/etcd-operator-crds/templates/etcdsnapshots.yamlpackages/system/etcd-operator/Chart.yamlpackages/system/etcd-operator/Makefilepackages/system/etcd-operator/charts/etcd-operator/.helmignorepackages/system/etcd-operator/charts/etcd-operator/Chart.yamlpackages/system/etcd-operator/charts/etcd-operator/README.mdpackages/system/etcd-operator/charts/etcd-operator/README.md.gotmplpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup-schedule.yamlpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup.yamlpackages/system/etcd-operator/charts/etcd-operator/crds/etcd-cluster.yamlpackages/system/etcd-operator/charts/etcd-operator/templates/_helpers.tplpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/certificate.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/issuer.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/mutatingwebhookconfiguration.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/validatingwebhookconfiguration.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-manager-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-metrics-reader.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-proxy-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-manager-rolebinding.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-proxy-rolebinding.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/role-leader-election-role.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/rbac/rolebinding-leader-election-rolebinding.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/configmap-env.yamlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/deployment.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/service-controller-manager-metrics-service.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/service-webhook-service.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/serviceaccount.ymlpackages/system/etcd-operator/charts/etcd-operator/templates/workload/vpa.ymlpackages/system/etcd-operator/charts/etcd-operator/values.schema.jsonpackages/system/etcd-operator/charts/etcd-operator/values.yamlpackages/system/etcd-operator/templates/deployment.yamlpackages/system/etcd-operator/templates/metrics.yamlpackages/system/etcd-operator/templates/rbac.yamlpackages/system/etcd-operator/templates/vpa.yamlpackages/system/etcd-operator/values.yamlpackages/system/etcd-rd/cozyrds/etcd.yaml
💤 Files with no reviewable changes (31)
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-manager-role.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/role-leader-election-role.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/serviceaccount.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/mutatingwebhookconfiguration.yml
- packages/extra/etcd/tests/backup-secret_test.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/_helpers.tpl
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-proxy-rolebinding.yml
- packages/system/etcd-operator/charts/etcd-operator/README.md
- packages/extra/etcd/templates/backup-secret.yaml
- packages/system/etcd-operator/charts/etcd-operator/README.md.gotmpl
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/vpa.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-proxy-role.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/validatingwebhookconfiguration.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/service-controller-manager-metrics-service.yml
- packages/system/etcd-operator/charts/etcd-operator/crds/etcd-cluster.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/certificate.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/cert-manager/issuer.yml
- packages/system/etcd-operator/charts/etcd-operator/.helmignore
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrole-metrics-reader.yml
- packages/extra/etcd/templates/etcd-backup-schedule.yaml
- packages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup-schedule.yaml
- packages/system/etcd-operator/charts/etcd-operator/Chart.yaml
- packages/system/etcd-operator/charts/etcd-operator/values.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/configmap-env.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/rolebinding-leader-election-rolebinding.yml
- packages/system/etcd-operator/charts/etcd-operator/crds/etcd-backup.yaml
- packages/system/etcd-operator/charts/etcd-operator/values.schema.json
- packages/extra/etcd/tests/etcd-backup-schedule_test.yaml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/service-webhook-service.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/rbac/clusterrolebinding-manager-rolebinding.yml
- packages/system/etcd-operator/charts/etcd-operator/templates/workload/deployment.yml
✅ Files skipped from review due to trivial changes (15)
- examples/backups/etcd/01-create-strategy.sh
- packages/system/etcd-operator-crds/.helmignore
- hack/testdata/migration-45/etcd-migrate
- examples/backups/etcd/02-create-bucket.sh
- packages/system/etcd-operator-crds/Chart.yaml
- packages/system/etcd-operator/templates/metrics.yaml
- packages/system/etcd-operator/Chart.yaml
- packages/extra/etcd/templates/vpa.yaml
- examples/backups/etcd/91-scenario-user-backup.md
- examples/backups/etcd/90-scenario-admin-prepare.md
- packages/extra/etcd/README.md
- api/backups/strategy/v1alpha1/etcd_types.go
- internal/backupcontroller/etcdapp/types.go
- packages/system/backupstrategy-controller/definitions/strategy.backups.cozystack.io_etcds.yaml
- examples/backups/etcd/README.md
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/extra/etcd/tests/backup-values-compat_test.yaml
- packages/system/etcd-operator/values.yaml
- examples/backups/etcd/05-restore-in-place.sh
- examples/backups/etcd/92-scenario-user-restore.md
- packages/core/platform/sources/etcd-operator.yaml
- packages/extra/etcd/templates/etcd-defrag.yaml
- packages/system/etcd-operator-crds/templates/etcdsnapshots.yaml
- examples/backups/etcd/03-create-etcd-src.sh
- examples/backups/etcd/00-helpers.sh
- packages/system/etcd-rd/cozyrds/etcd.yaml
- packages/system/backupstrategy-controller/templates/rbac.yaml
- packages/extra/etcd/values.yaml
- packages/extra/etcd/tests/topology-spread_test.yaml
- internal/backupcontroller/etcdtypes/zz_generated.deepcopy.go
- internal/backupcontroller/etcdtypes/types.go
- packages/extra/etcd/values.schema.json
- hack/e2e-apps/etcd.bats
- packages/extra/etcd/templates/etcd-cluster.yaml
- internal/backupcontroller/etcdstrategy_controller.go
- internal/backupcontroller/etcdstrategy_controller_test.go
| ARG TARGETARCH | ||
| ARG ETCD_OPERATOR_VERSION=v0.5.1 | ||
|
|
||
| RUN wget -O- https://github.com/cozystack/cozyhr/raw/refs/heads/main/hack/install.sh | sh -s -- -v 1.6.1 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin and verify the fetched installer script.
This executes hack/install.sh from the mutable main branch as root during the image build. Pin it to an immutable ref and verify the script checksum before execution to keep the migration image reproducible.
Proposed hardening
-RUN wget -O- https://github.com/cozystack/cozyhr/raw/refs/heads/main/hack/install.sh | sh -s -- -v 1.6.1
+ARG COZYHR_INSTALL_REF=<commit-sha>
+ARG COZYHR_INSTALL_SHA256=<sha256>
+RUN set -eux; \
+ curl -fsSL -o /tmp/cozyhr-install.sh \
+ "https://raw.githubusercontent.com/cozystack/cozyhr/${COZYHR_INSTALL_REF}/hack/install.sh"; \
+ echo "${COZYHR_INSTALL_SHA256} /tmp/cozyhr-install.sh" | sha256sum -c -; \
+ sh /tmp/cozyhr-install.sh -v 1.6.1; \
+ rm -f /tmp/cozyhr-install.sh🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/platform/images/migrations/Dockerfile` at line 14, The
Dockerfile build step currently fetches and runs the cozyhr installer from the
mutable main branch, which makes the migration image non-reproducible and
unsafe. Update the RUN step in the Dockerfile to use an immutable ref for
hack/install.sh (such as a tagged release or commit SHA) and add checksum
verification before executing it. Keep the change localized to the image build
logic and preserve the existing install.sh invocation path while hardening the
fetch-and-run flow.
c0552b4 to
827c852
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the in-place adoption path has three sequencing/availability defects that can leave the platform's etcd-operator down or adopted clusters permanently non-Ready.
Business context: migrate the platform's etcd integration from the legacy etcd.aenix.io/v1alpha1 operator (EtcdBackup, Ready condition) to etcd-operator.cozystack.io/v1alpha2 (EtcdSnapshot, Available condition), adopting existing clusters in place via the baked-in etcd-migrate tool.
Blockers
B1: adoption failure leaves the cluster-wide etcd-operator scaled to 0
File: packages/core/platform/images/migrations/migrations/45:290-300
Issue: the script scales the operator deployment to 0 (line 290), runs etcd-migrate --apply (line 294), then scales it back to 1 (line 300). With set -euo pipefail (line 17), any non-zero exit from etcd-migrate --apply aborts the script before the scale-back, leaving etcd-operator-controller-manager at 0 replicas.
Evidence: the migration runs as a Helm pre-upgrade,pre-install hook (packages/core/platform/templates/migration-hook.yaml:20); run-migrations.sh propagates the failure (exit 1) without restoring replicas. The operator is cluster-wide (it manages every tenant's etcd), so while it sits at 0 no etcd cluster anywhere is reconciled — no member replacement, no scaling. The Job then retries from the same broken state.
Impact: a transient or deterministic adoption failure during an unattended upgrade disables etcd reconciliation platform-wide until a human intervenes.
Fix: register a trap/cleanup that restores the operator replicas on any exit before propagating the error.
B2: adopted clusters get an immutable spec.tls mismatch
File: packages/extra/etcd/templates/etcd-cluster.yaml:41-54
Issue: the chart always renders spec.tls.client.certManager and spec.tls.peer.certManager, but adoption produces a secretRef-shaped spec.tls — see the script's own comment (migrations/45:84-96): "in secretRef TLS (what adoption produces) the operator never mints certs, it only references them". The v1alpha2 CRD makes the entire spec.tls subtree immutable.
Evidence: the etcdclusters CRD carries the CEL rule !has(self.tls) || !has(oldSelf.tls) || self.tls == oldSelf.tls (packages/system/etcd-operator-crds/templates/etcdclusters.yaml:1781), rejecting any post-create change to spec.tls. Once adoption creates a secretRef CR, the next HelmRelease reconcile renders the certManager shape and the API server rejects the update.
Impact: every adopted etcd HelmRelease stays non-Ready after migration — exactly the existing-cluster path this PR targets.
Fix: render the adopted secretRef shape for migrated clusters (or otherwise converge chart output with what etcd-migrate writes) so the reconcile is a no-op on spec.tls.
B3: adoption runs before the v1alpha2 CRDs are installed
File: packages/core/platform/images/migrations/migrations/45:293-297
Issue: etcd-migrate --apply creates etcd-operator.cozystack.io/v1alpha2 resources, but the v1alpha2 CRDs are introduced by this same PR as the new etcd-operator-crds component (packages/core/platform/sources/etcd-operator.yaml). That component installs via the PackageSource → HelmRelease pipeline, which only runs after the platform chart's resources are applied — i.e. after the pre-upgrade hook that runs this migration. The script documents a precondition it cannot guarantee (lines 7-11: "the etcd-operator HelmRelease has already been upgraded to the v1alpha2 operator by the time this runs").
Evidence: migration-hook.yaml:20 (helm.sh/hook: pre-upgrade,pre-install); sources/etcd-operator.yaml adds etcd-operator-crds as a new component this release; the script waits only for the legacy CRD (line 219) and never for etcdclusters.etcd-operator.cozystack.io.
Impact: on the upgrade that first introduces v1alpha2, a cluster with legacy etcd clusters runs adoption before the target CRDs/operator exist. A failed pre-upgrade hook aborts the entire platform upgrade, so the CRDs never get applied — a deadlock.
Fix: gate the migration on the v1alpha2 CRD/operator being present (wait/poll), or guarantee the CRDs are installed before the adoption hook runs.
Non-blocking follow-ups
- migration 45 validates only
AWS_ACCESS_KEY_ID(lines 162-163), but the snapshot also needsAWS_SECRET_ACCESS_KEY; a missing secret key aborts after operator/cert mutations have already begun. Validate both up front. stage_backup_credsuses client-sidekubectl apply -f -(line 188), which writes alast-applied-configurationannotation containing the full Secret payload. Use server-side apply.- Controller status text still references "Ready" where the gate is now "Available": the requeue reason
EtcdClusterNotReady(internal/backupcontroller/etcdstrategy_controller.go:232,243) paired with a "become Available" message, plus a few comments. Cosmetic, but worth aligning. - examples/backups/etcd/92-scenario-user-restore.md:38 still says "Wait for
Ready=True" while line 39 pollsstatus.conditions[Available].
The earlier missing---cluster defrag concern is already addressed — packages/extra/etcd/templates/etcd-defrag.yaml passes --cluster.
| kubectl -n "$ETCD_OPERATOR_NS" rollout status deploy "$ETCD_OPERATOR_DEPLOY" --timeout=120s || true | ||
|
|
||
| echo "=== etcd-migrate --apply (snapshot -> cozy-backups, then adopt) ===" | ||
| etcd-migrate --apply --yes \ |
There was a problem hiding this comment.
B1: with set -euo pipefail (line 17), a non-zero exit from etcd-migrate --apply here aborts the script before the scale-back on line 300, leaving the cluster-wide etcd-operator-controller-manager at 0 replicas. While the operator is at 0, no etcd cluster anywhere is reconciled, and the Job retries from that broken state. Restore the operator replicas via a trap/cleanup on exit before propagating the error.
| # cozystack stays cert-only (no etcd password auth). | ||
| tls: | ||
| client: | ||
| certManager: |
There was a problem hiding this comment.
B2: the chart renders certManager-mode spec.tls, but adoption produces secretRef-mode (see migrations/45:84-96), and spec.tls is immutable post-create (etcdclusters CRD CEL rule self.tls == oldSelf.tls, line 1781). After adoption creates a secretRef CR, the next HelmRelease reconcile renders this certManager shape and the API server rejects the update, so adopted clusters stay non-Ready. Render the adopted secretRef shape for migrated clusters.
98a9050 to
5f87e7d
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — the three sequencing/availability blockers from the prior review are resolved, and the version-stamp guardrail regression introduced in the rework is fixed (Unit & controller tests green).
Resolved since the prior review: B1 (operator stranded at 0 replicas on adoption failure) — trap restore_etcd_operator EXIT armed before the scale-down. B2 (immutable spec.tls mismatch) — the chart now renders the secretRef TLS shape that etcd-migrate adoption produces. B3 (adoption running before the v1alpha2 CRDs exist) — ensure_v1alpha2_crds applies the baked CRD copies first, and the operator component now dependsOn the new etcd-operator-crds package. Migration 47 now stamps via stamp_cozystack_version. Prior non-blocking items (both AWS keys validated up front, server-side apply for the staged creds, Ready→Available wording) are addressed.
Non-blocking follow-ups
- The migrations/47 header comment still claims "the etcd-operator HelmRelease has already been upgraded to the v1alpha2 operator by the time this runs", which contradicts the accurate
--agent-imagecomment ("the Deployment we just scaled to 0 is still the LEGACY operator"). The pre-upgrade hook runs before the etcd-operator HelmRelease reconciles — the header is misleading. hack/e2e-apps/etcd.bats: add an existence backstop beforekubectl wait workloadmonitor.cozystack.io/etcd, and drop the|| truefrom the teardown cleanup, per the e2e conventions.- The new first-party
packages/system/etcd-operatorchart (deployment/rbac/metrics/vpa) ships no helm-unittest tests, while the app chart gained new ones. packages/extra/etcd/README.mddoesn't spell out the "remove the legacybackup.*keys" step for existing clusters; scheduled backups stop with only a migration-log warning.
Checked, not blocking
- Member peer/client mTLS uses the short
<member>.<svc>.<ns>.svcdomain (operatorhelpers.gopeerURL/clientURLat v0.5.1), covered by the chart's*.etcd.<ns>.svcwildcard — the cluster-domain FQDN SANs the migration adds are belt-and-suspenders, not a chart requirement. - Restore recreates the EtcdCluster with the immutable
spec.bootstrap.restoreand resumes the HelmRelease whose chart omitsbootstrap; this relies on Helm's three-way merge preserving the out-of-band field, with the controller holding the HR suspended across the purge/recreate window. Worth a manual e2e run before merge since the e2e gate is skipped here.
…3122) ## What this PR does Tenant Kubernetes control planes get their etcd from the `extra/etcd` chart, which creates an `EtcdCluster` (`etcd.aenix.io/v1alpha1`) reconciled by etcd-operator v0.4.5. That operator hardcodes the etcd image to `quay.io/coreos/etcd:v3.5.12`, and the v1alpha1 CRD exposes no version field, so every tenant etcd runs 3.5.12. kube-apiserver only enables the `RequestWatchProgress` storage feature on etcd >= 3.5.13 (or >= 3.4.31). Kubernetes >= 1.31 enables `ConsistentListFromCache` — locked-to-true in 1.35 — which depends on it. On etcd 3.5.12 every consistent watch therefore fails inside the stream with HTTP 500 (`the required storage feature RequestWatchProgress is disabled`), which breaks watch-based clients of aggregated APIs in modern (k8s 1.35) tenants. This pins the etcd container image to a >= 3.5.13 patch release (v3.5.31) by setting `image` on the `etcd` container in the EtcdCluster's `spec.podTemplate.spec`. etcd-operator v0.4.5 strategically merges `podTemplate.spec` over its generated pod spec (matched by container name), so the override takes effect with no operator change — the same mechanism the chart already uses for the metrics port, probes, and resources. It restores consistent watches without waiting on the etcd-operator v1alpha2 migration (#2859), which is the longer-term fix. A helm-unittest suite asserts the etcd container carries the pinned image and is no longer left on the operator default v3.5.12. Addresses #3080. ### Upgrade impact On merge, etcd-operator reconciles every existing tenant `EtcdCluster` and rolls its StatefulSet from 3.5.12 to 3.5.31. This is a safe in-minor rolling patch upgrade (no etcd storage-format change, quorum preserved by the operator's rolling strategy), so expect a brief, sequential etcd pod restart per tenant and no data migration. ### Release note ```release-note fix(etcd): pin tenant etcd to v3.5.31 so kube-apiserver can enable RequestWatchProgress (consistent watches) on Kubernetes 1.35 tenants ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Pinned the bundled etcd image to `quay.io/coreos/etcd:v3.5.31` to improve compatibility with Kubernetes watch-related storage behavior. * Prevented deployments from reverting to the older default etcd image. * **Tests** * Added a new test suite to verify the template uses the expected pinned etcd image and blocks regressions. * **Chores** * Updated dependency automation to recognize and manage the pinned etcd image version. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — the three prior sequencing/availability blockers stay resolved and are now pinned by contract tests; the v0.5.2 rework (operator bump, seccomp, e2e refinements) checks out against the upstream API.
Verified against etcd-operator v0.5.2 source:
- The Ready→Available and Snapshot→Artifact rename in the backup driver matches the real v1alpha2 API (ClusterAvailable="Available", EtcdSnapshotStatus.Artifact{URI,SizeBytes,Checksum}, Phase Pending/Started/Complete/Failed).
- The vendored manager ClusterRole (
packages/system/etcd-operator/templates/rbac.yaml) is an exact match of the operator'smanager-role-rules.yamlat v0.5.2. etcd-migrateis SHA256-pinned per arch; the v1alpha2 CRDs are baked as pure YAML withhelm.sh/resource-policy: keep, so migration 49's rawkubectl applyand the lateretcd-operator-crdsHelmRelease co-own them without either deleting them.- The
backup.*schema removal is backward-compatible:values.schema.jsonhas noadditionalProperties: false, so leftoverbackup.*keys on an upgraded tenant are ignored, not rejected (pinned bytests/backup-values-compat_test.yaml).
Local run: backupcontroller Go tests pass, etcd helm-unittest 17/17, migration-49 bats 8/8, gopls clean.
Non-blocking follow-ups
migrations/49header (lines 7-11) still claims "the etcd-operator HelmRelease has already been upgraded to the v1alpha2 operator by the time this runs", which contradicts the accurate--agent-imagecomment (line 344-346: "the Deployment we just scaled to 0 is still the LEGACY operator"). The pre-upgrade hook runs before the etcd-operator HelmRelease reconciles — the header is misleading.- The new first-party
packages/system/etcd-operatorchart (deployment/rbac/metrics/vpa) ships no helm-unittest tests, while peer operator charts (postgres-operator, opensearch-operator, backup*-controller) carry them and the app chart gained new ones. Recommend a small suite (image/args, securityContext, RBAC rules present). hack/e2e-apps/etcd.bats: the inlinebackup_cleanupat the end of the last@testswallowscleanup.shfailures via|| true(line 84), so a stuck bucket/BackupClass cleanup does not fail the suite the way the inlineetcd_draindoes. Consider giving the inline backup path teeth.- Minor: migration 49's
ensure_wildcard_sansadds*.<name>.<ns>.svc.<cluster-domain>FQDN SANs to the cert-manager Certificates, but the chart template declares only the short*.etcd.<ns>.svcform, so the next HelmRelease reconcile strips the FQDN entries. Harmless (the operator dials members by the short.svcname —helpers.gopeerURL/clientURL— and only mints certs with the FQDN in certManager mode, which this chart does not use), but adoption and chart output do not converge to a fixed point on the cert SANs. Drop the FQDN SANs from the migration or add them to the chart.
myasnikovdaniil
left a comment
There was a problem hiding this comment.
Re-review (HEAD 144232c2b) — NOT LGTM: 1 blocker
Thanks for the updates. Both of my prior blockers are resolved:
- B1 — etcd.bats aborting the whole e2e suite under
cozytest.sh'sset -u(unboundBATS_TEST_DIRNAME): fixed via the repo-root-relativeETCD_EXAMPLES(inline note below). - B2 — migration hard-failing the upgrade when the backup target can't be resolved: substantially addressed (inline note below).
One hard blocker remains: E2E Tests is deterministically red, introduced by this PR.
CI status
E2E Tests— real, must fix. Fails on the newBackup and in-place restore round-trip (EtcdSnapshot driver)subtest. The snapshot-agent Job comes up fine but hangs for 20m uploading toS3_ENDPOINT=https://s3.example.org— the bucket's external ingress endpoint, which the e2e cluster can neither route to nor TLS-validate (cert-manager rejects*.example.org: "forbidden by policy"). The Job hits itsactiveDeadlineSeconds, theEtcdSnapshotnever leavesStarted, and theBackupJobsettlesFailed. Thesecret "etcd-etcd-backup-creds" not found/FailedMount etcd-*-tlsevents are cleanup-race red herrings (fired ~2s before the diagnostics dump, as teardown deleted the secrets while the controller spawned a retry pod). Fix direction inline.Require API owner review for sizeable API changes— not a code issue. Governance gate onapi/changes; needs an API-owner approval, nothing to fix in the diff.
Also: the branch is currently conflicting with main and needs a rebase.
| # reuses the cluster the previous @test created. | ||
| @test "Backup and in-place restore round-trip (EtcdSnapshot driver)" { | ||
| [ -x "${ETCD_EXAMPLES}/run-all.sh" ] || skip "etcd backup example scripts not found at ${ETCD_EXAMPLES}" | ||
| NAMESPACE=tenant-test "${ETCD_EXAMPLES}/run-all.sh" || { dump_diagnostics; false; } |
There was a problem hiding this comment.
Blocker: this subtest makes E2E Tests deterministically red.
run-all.sh → 02-create-bucket.sh reads the backup S3 endpoint straight from the BucketInfo secret (ETCD_ENDPOINT=$(jq -r '.spec.secretS3.endpoint' …), line 40) — the bucket's external ingress endpoint. In CI that is https://s3.example.org (placeholder e2e domain), so the in-cluster snapshot-agent can neither route to it nor validate its TLS (cert-manager rejects *.example.org). It stays Active/Ready retrying the upload for the full activeDeadlineSeconds (20m); the EtcdSnapshot never leaves Started, the BackupJob settles Failed, and this test fails (~2h wall-clock).
The sibling Create and Verify Seeweedfs Bucket test passes precisely because it sidesteps this — it kubectl port-forwards service/seaweedfs-s3 and talks to https://127.0.0.1:8333 --insecure. The etcd round-trip has no equivalent, so it can't pass in CI as written.
Fix direction: point the backup at the in-cluster SeaweedFS service (e.g. https://seaweedfs-s3.<ns>.svc:8333) with a skip-TLS-verify path for the snapshot-agent, or otherwise decouple the test from a publicly-resolvable / valid-TLS endpoint. Production is unaffected (real s3.<domain> resolves + has a real cert) — this is strictly an e2e-environment gap. Note the same external endpoint also feeds migration-49's mandatory safety snapshot (resolve_platform_backup_args), so it's worth confirming any future live-upgrade e2e wouldn't hit the same wall.
| # that unbound var would abort the whole suite at load time. cozytest's CWD is | ||
| # the repo root, so the plain relative path resolves — same convention | ||
| # hack/migration-49-etcd-adopt.bats uses with $PWD/... | ||
| ETCD_EXAMPLES="examples/backups/etcd" |
There was a problem hiding this comment.
✅ Resolves my prior B1. cozytest.sh sources this file under set -u without ever setting BATS_TEST_DIRNAME, so the old interpolation would abort the entire e2e suite before any test ran. The repo-root-relative path (plus the explaining comment above) fixes it — confirmed in the failed run, where the suite ran all the way through the contracts test.
| if ETCD_MIGRATE_BACKUP_ARGS=$(resolve_platform_backup_args); then | ||
| echo " -> safety snapshot: ${ETCD_MIGRATE_BACKUP_ARGS}" | ||
| else | ||
| echo "ERROR: refusing to adopt live etcd without a safety snapshot." >&2 |
There was a problem hiding this comment.
✅ Addresses my prior B2. This unconditional hard-fail is now acceptable because (a) it resolves the target from the always-present platform cozy-default-etcd strategy + projected cozy-backups-creds (not per-tenant backup config), and (b) there's a documented ETCD_ADOPT_SKIP_BACKUP=1 escape hatch (line 32) for clusters that intentionally have no backup storage. The EXIT-trap scale-back also keeps the cluster-wide operator from being stranded at 0 replicas. 👍
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Address review feedback from gemini-code-assist on packages/system/etcd-operator/templates/deployment.yaml: add an explicit seccompProfile (RuntimeDefault) to the controller-manager pod securityContext. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Address review feedback from coderabbitai on hack/e2e-apps/etcd.bats: read the container name, metrics port, and Pod IP with the strict member selector (name+instance+managed-by) plus a Running-phase filter instead of the broad app.kubernetes.io/name=etcd, so .items[0] cannot resolve to a non-member or not-yet-running Pod. Also assert a non-empty Pod IP before the metrics probe. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Andrey Kolkov <androndo@gmail.com>
The v1alpha2 operator sets Available=True early (reason ClusterDiscovered) as soon as the seed member forms the cluster, then scales the remaining members up one at a time. Gating the runtime-contracts test on Available raced the member-Pod count assertion and saw only 1/3 Pods, failing the run. Gate on status.readyMembers=3 instead — the count of members whose EtcdMember Ready condition is True (and the /scale status path), which only reaches 3 once every member Pod is Running and serving. Bump the wait timeout to 300s to cover sequential one-at-a-time rollout. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
The backup round-trip test could wedge for 20 min and then report a bare EtcdSnapshot phase=Started with no root cause: dump_diagnostics dumped the (healthy) EtcdCluster and the operator log, but the snapshot agent runs in its own Job Pod, so the etcd-read / S3-upload error lives only in that Pod's log and in the BackupJob/EtcdSnapshot status. - dump_diagnostics now dumps the whole backup chain: BackupJob/Backup and EtcdSnapshot describe, each snapshot Job's describe + Pod logs (selected by the operator's etcd-operator.cozystack.io/cluster label), and recent namespace events (the primary signal when a hung Pod logs nothing). - wait_for_field gains an optional terminal-failure value; steps 04/05 pass Failed so the BackupJob/RestoreJob waits return immediately on a terminal phase instead of polling to the timeout — which also keeps the snapshot Job Pod within its 600s TTL so its log is still readable at dump time. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
Signed-off-by: Andrey Kolkov <androndo@gmail.com>
144232c to
d0740ed
Compare
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — the migration and the v1alpha2 backup/restore rework are in good shape: all prior sequencing/availability blockers stay resolved, the API mapping checks out against etcd-operator v0.5.2, and the one open code finding is a minor diagnostic-message bug. The two caveats below are about CI coverage, not code correctness.
Verified resolved / sound:
- Migration 50:
restore_etcd_operatorEXIT trap (operator never stranded at 0 replicas),ensure_v1alpha2_crdsapplies + waits the baked CRDs before adoption, mandatory pre-adoption snapshot with a documented escape hatch, dry-run → apply, version-stamp gate. The header comment now correctly states it runs pre-HR-reconcile against the still-legacy operator. - Restore → resume-HR immutability: the chart omits
spec.bootstrap, and the app HelmRelease does not force upgrades, so helm-controller's three-way merge preserves the out-of-bandspec.bootstrap.restoreon the recreatedEtcdCluster— the resume reconcile is a no-op on that immutable subtree, not a rejected removal. Sound, but see caveat 1. - Open bot threads are stale:
etcd-defrag.yamldoes pass--cluster(plus--endpoints), and the controller pod carriesseccompProfile: RuntimeDefault+runAsNonRoot.
Non-blocking follow-ups
- CI no longer exercises the backup/restore round-trip. The
EtcdSnapshotS3 round-trip is gated out of CI (ETCD_E2E_S3_ROUNDTRIP) because the snapshot agent can't trust an in-cluster self-signed S3 (no CA-mount / InsecureSkipVerify surface on the CRD) and the external ingress endpoint is unroutable in kind — a genuine upstream limitation, well-documented in the test. But it means the headline feature, and the three-way-merge restore path above, are unverified in CI; run a full round-trip on a real cluster before relying on restore. (hack/e2e-apps/etcd.bats) latestEtcdSnapshotConditionMessagechecks the wrong condition type for snapshots (internal/backupcontroller/etcdstrategy_controller.go:402-405). The failure-shaped clause testsc.Type == ClusterConditionAvailable("Available"), but anEtcdSnapshot's lifecycle condition isReady(upstreamSnapshotReady = "Ready") — the comment itself says "a Ready=False entry." A snapshot that fails withReady=Falseand a reason not prefixedFailedfalls through to the latest-by-transition message, which the function's own header warns can shadow the real cause. Diagnostic-only (phase=Failedis detected separately), but the code contradicts its comment.internal/backupcontroller/etcdstrategy_controller.go:874— a restore status message still says the cluster reaches "Ready" afterbootstrap.restore; the gate is now "Available". Cosmetic wording.- The two
Makefilebot threads (etcd-operator,etcd-operator-crds) concern the first-party vendoring / kustomize-fetch flow; theetcd-operatorchart is now deliberately cozystack-authored (documented in its Makefile), somake updatere-vendors only RBAC + CRDs by hand — worth confirming that is the intended contract.
CI: E2E Tests is still running on the current head (the round-trip-gating commit should clear the prior red). Require API owner review for sizeable API changes is a governance gate on the api/ change, not a code fix — it needs an API-owner approval.
…acts The suite was ported from the pre-#2859 etcd.bats and asserted etcd.aenix.io/v1alpha1 objects that no longer exist on main — every Test hard-failed at the first assert on an unknown GVK. - drop the etcd-empty-backup and etcd-backup-schedule scenarios with their fixtures: the chart no longer renders EtcdBackupSchedule or etcd-s3-creds (backups moved to the generic backups API), and legacy-values tolerance is pinned by helm-unittest, not e2e - rewrite the remaining scenario into etcd-1-contracts, mirroring main's contracts test: gate on status.readyMembers=3 (Available=True flips at seed-member discovery and races replica-count contracts), assert the operator-owned member Pod shape (strict selector, container name, named metrics port), WorkloadMonitor operational/availableReplicas, and script the three contracts that need imperative probes: the /scale subresource, the plaintext-HTTP metrics probe against the Pod IP, and the on-demand defrag Job - add etcd-2-backup-roundtrip driving examples/backups/etcd/run-all.sh, gated on ETCD_E2E_S3_ROUNDTRIP=1 exactly like main (kind CI has no publicly-trusted S3 endpoint the snapshot agent could verify) - upgrade catch blocks to the v1alpha2 GVKs plus EtcdMember/ EtcdSnapshot/BackupJob describes and the snapshot-Job Pod logs - strengthen _lib/etcd-cleanup.sh with main's etcd_drain deltas: probe the Etcd CR itself (it can linger Terminating and block the next singleton apply) and use the err-sentinel pattern so a transient API error is never misread as "deleted"; add etcd_backup_cleanup wrapping examples/backups/etcd/cleanup.sh for the round-trip's teardown Assisted-By: Claude <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
…OM floor) Two defects that break the etcd-operator on an in-cluster 1.5 -> 1.6 upgrade (#2859 swapped the upstream chart for the cozystack-authored one): - #3242: the controller Deployment's spec.selector.matchLabels changed ({instance,name} -> {name,control-plane}). spec.selector is immutable, so `helm upgrade` cannot patch the existing Deployment and the whole HelmRelease upgrade fails ("field is immutable") -- the operator and the v1alpha2 CRDs it serves never come up, which blocks the etcd v1alpha2 adoption. Fresh installs use the new selector directly, so fresh-install CI does not catch it. Add a pre-upgrade hook that deletes the Deployment ONLY when its live selector is the pre-1.6 one (lacks control-plane=controller-manager), so Helm recreates it cleanly. No-op when the selector already matches (rc.1 -> later) and never runs on a fresh install (pre-upgrade only). Keeping the selector stable in the chart is not an option: rc.1 already shipped the new selector, so aligning it back would merely move the immutable break to rc.1 -> next. - Raise the manager's cold-start memory limit floor 128Mi -> 256Mi (and the VPA minAllowed to match). The steady-state working set is ~250Mi (the VPA's own recommendation); at 128Mi a Pod that starts before the VPA admission webhook rewrites it (e.g. a Deployment recreated out of band, or the webhook briefly unavailable during upgrade) OOMKills into a crash loop. Defense in depth so the operator never depends on VPA timing merely to avoid crashing; the VPA still scales it further under load up to maxAllowed. Verified live on a 1.5 -> 1.6 adoption: the delete-Deployment step plus the VPA re-applying 256Mi+ let the operator come up and the adoption complete. Refs: #3242, #3243 Signed-off-by: Andrey Kolkov <androndo@gmail.com>
…2826) ## What this PR does Migrates the E2E **app** test suite from BATS to [Kyverno Chainsaw](https://github.com/kyverno/chainsaw) — a declarative, Kubernetes-native E2E framework (CNCF, part of the Kyverno project; successor to KUTTL) — and wires it into CI as the replacement for the per-app BATS loop. This grows the original pilot (postgres + bucket) into the full suite. All 22 app suites are ported under `hack/e2e-chainsaw/` and the `hack/e2e-apps/*.bats` files are removed: `postgres`, `bucket`, `mariadb`, `mongodb`, `redis`, `qdrant`, `clickhouse`, `kafka`, `etcd`, `openbao`, `harbor`, `foundationdb`, `external-dns`, `kuberture`, `vminstance`, `gateway`, `kubernetes-latest`, `kubernetes-previous`, `kubernetes-oidc-system`, `kubernetes-oidc-customconfig`, `securitygroup`, `serviceexposure`. **Approach** - **Declarative** suites assert on `status.conditions` and concrete fields. The `timeout N sh -ec "until kubectl get ..."` + `kubectl wait` pair that appeared ~100 times collapses into a single `assert` that polls existence and state together, with a structured diff on failure and automatic `events`/`describe`/`podLogs` capture via `catch` (previously only `harbor.bats` did this, by hand). - **Imperative** suites keep their logic in `script` steps: `openbao` init/unseal, `kuberture` external-dns split-horizon probes, `vminstance`, the `gateway` admission/impersonation cases, and `kubernetes-latest`/`previous`, which wrap the relocated `hack/e2e-chainsaw/_lib/run-kubernetes.sh` verbatim (Kamaji bring-up, LB/NFS/ouroboros checks). - `gateway` tests derive the tenant apex from the namespace `namespace.cozystack.io/host` label at runtime, so they are host-independent. **CI** - The `e2e` job now runs `chainsaw test hack/e2e-chainsaw/` via a new `test-chainsaw` target and uploads the JUnit `chainsaw-report.xml`. - The `chainsaw` binary is added to the e2e-sandbox image. - `install-cozystack` and `test-openapi` stay BATS (cluster bootstrap + OpenAPI checks). - The per-app 3-retry loop is dropped — assertion polling replaces the fixed-timeout flakiness it papered over. **Validation** Ran against a development cluster: the DB/app, storage, and VM suites pass; the three suites that depend on platform features not present on that cluster (`gateway`, `kuberture`, `external-dns`) and the two heavyweight `kubernetes-*` suites are exercised by this PR's CI run on a freshly installed platform. Note for reviewers: service-port asserts use the `(ports[*].port)` projection form rather than a number-literal filter (`` ports[?port == `N`] ``), because Chainsaw v0.2.15 mis-evaluates JMESPath number-literal comparisons. **2026-07-09: reconciled with main (~520 commits of drift)** - etcd suite re-ported to the v1alpha2 operator contracts (#2859): `readyMembers=3` gate, pod-label/`/scale`/WorkloadMonitor/metrics/defrag contracts, plus the `ETCD_E2E_S3_ROUNDTRIP`-gated backup round-trip driving `examples/backups/etcd` - 12 new `ingress-hostname-policy` gateway cases ported (apex derived at runtime from the tenant-root namespace label) - post-branch bats drift folded in: `mariadb-single` webhook guard, kafka `c1.small` presets, bucket/harbor 2m BucketClaim fail-fast, bucket readonly-denial promoted to a hard fail (cosi-driver v0.3.1), qdrant PVC-reclaim guard (#3059), kuberture fail-loud negations, SC-fallback-default test (#2872 B1) - 4 suites that only exist on main since the branch was cut are ported: `kubernetes-oidc-system`, `kubernetes-oidc-customconfig`, `securitygroup`, `serviceexposure` - CI seams: `nightly.yaml` converted to the chainsaw invocation, `run-kubernetes.sh` reconciled (Talos/CABPT waits, tenant drain, LINSTOR pool wait, talos-image-cache under `_lib/`), `e2e-capture-dataplane.sh` replicated in the Chainsaw global catch ### Release note ```release-note fix(kubernetes): tenant Kubernetes teardown no longer hangs when the cluster has no working nodes — the pre-delete hook now bounds its wait for the in-tenant HelmReleases and force-clears their Flux finalizers on timeout (#3271) ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * End-to-end testing is now driven by a unified Chainsaw suite (platform apps, Kubernetes, networking, security groups, service exposure). * Test Impact Analysis selects affected suites, with an option to run the full suite. * CI produces a JUnit report artifact and collects suite-scoped diagnostics on failure. * **Bug Fixes** * CI E2E now fails fast and emits clearer Kubernetes/HelmRelease and sandbox bucket state diagnostics. * Improved teardown reliability with bounded wait + finalizer handling to avoid hangs. * **Documentation** * Updated E2E testing docs and local guidance to reflect the new Chainsaw-based workflow and suite paths. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…OM floor) Two defects that break the etcd-operator on an in-cluster 1.5 -> 1.6 upgrade (#2859 swapped the upstream chart for the cozystack-authored one): - #3242: the controller Deployment's spec.selector.matchLabels changed ({instance,name} -> {name,control-plane}). spec.selector is immutable, so `helm upgrade` cannot patch the existing Deployment and the whole HelmRelease upgrade fails ("field is immutable") -- the operator and the v1alpha2 CRDs it serves never come up, which blocks the etcd v1alpha2 adoption. Fresh installs use the new selector directly, so fresh-install CI does not catch it. Add a pre-upgrade hook that deletes the Deployment ONLY when its live selector is the pre-1.6 one (lacks control-plane=controller-manager), so Helm recreates it cleanly. No-op when the selector already matches (rc.1 -> later) and never runs on a fresh install (pre-upgrade only). Keeping the selector stable in the chart is not an option: rc.1 already shipped the new selector, so aligning it back would merely move the immutable break to rc.1 -> next. - Raise the manager's cold-start memory limit floor 128Mi -> 256Mi (and the VPA minAllowed to match). The steady-state working set is ~250Mi (the VPA's own recommendation); at 128Mi a Pod that starts before the VPA admission webhook rewrites it (e.g. a Deployment recreated out of band, or the webhook briefly unavailable during upgrade) OOMKills into a crash loop. Defense in depth so the operator never depends on VPA timing merely to avoid crashing; the VPA still scales it further under load up to maxAllowed. Verified live on a 1.5 -> 1.6 adoption: the delete-Deployment step plus the VPA re-applying 256Mi+ let the operator come up and the adoption complete. Refs: #3242, #3243 Signed-off-by: Andrey Kolkov <androndo@gmail.com>
…es (#3270) Consolidates the etcd `v1alpha2` transition fix for **in-cluster 1.5 → 1.6 upgrades** into a single PR, rebased on current `main`. Supersedes #3265 and #3261 — their commits are carried here (authorship preserved), so those PRs can be closed once this lands. Fresh installs use the new shapes directly and are unaffected, so fresh-install CI does not catch these; the 1.5 → 1.6 e2e upgrade path is the authoritative regression guard. ## What this PR does ### 1. Keep the legacy `etcd-headless` Service alive during adoption — `packages/extra/etcd` `etcd-migrate` adopts legacy clusters **in place**: the Pods keep their original `spec.subdomain: etcd-headless` and are dialed at `etcd-<i>.etcd-headless.<ns>.svc` until they roll onto the operator's native `<member>.etcd.<ns>.svc` domain. The v1alpha2 operator only creates the native `etcd` Service and the legacy `etcd-headless` Service is pruned, so those per-pod names stop resolving (`no such host`), `MemberList` fails, and `status.readyMembers` never populates — the `EtcdCluster` never goes `Ready` even though etcd is healthy and in quorum. We ship a chart-managed transitional headless `etcd-headless` Service (selector mirrors the operator's native `etcd` Service via `etcd-operator.cozystack.io/cluster`, `publishNotReadyAddresses: true`) — the DNS counterpart of the legacy `*.etcd-headless.<ns>.svc` SAN already kept for this window. Removable together with that SAN once members roll onto the native subdomain. ### 2. Survive the immutable controller-Deployment selector on upgrade — `packages/system/etcd-operator` (#3242) #2859 replaced the upstream etcd-operator chart with the cozystack-authored one, changing `Deployment.spec.selector.matchLabels`. `spec.selector` is immutable, so `helm upgrade` cannot patch the existing Deployment and the whole HelmRelease upgrade fails (`field is immutable`). A **pre-upgrade hook** (`templates/pre-upgrade-selector-fix.yaml`: ServiceAccount + Role + RoleBinding + Job) deletes the Deployment **only** when its live selector is the pre-1.6 one, so Helm recreates it cleanly. No-op when the selector already matches, never runs on fresh install. ### 3. Raise the operator's memory cold-start floor — `packages/system/etcd-operator` Steady-state working set is ~250Mi; the static `limits.memory: 128Mi` OOMKills a Pod that starts before the VPA admission webhook rewrites it. Raise the floor to `256Mi` (and VPA `minAllowed` to match) as defense in depth so the operator never depends on VPA timing to avoid crashing. ### 4. Make migration 50 (etcd adoption) robust in-cluster — `packages/core/platform/images/migrations/migrations/50` (was #3261) Exact server/peer cert-SAN match, Secret-gated wait on the adoption Secret, and in-cluster (IPv6-safe) kubeconfig handling, so the migration script drives the adoption reliably from inside the cluster. Covered by `hack/migration-50-etcd-adopt.bats`. ### 5. Hardening / review fixes (this PR's original scope) - **Hook `runAsUser: 65532`.** `pre-upgrade-selector-fix.yaml` set `runAsNonRoot: true` but no numeric `runAsUser`; `clastix/kubectl`'s image user is the non-numeric name `nonroot`, which the kubelet cannot verify against `runAsNonRoot` — so the hook Pod fails admission and silently blocks the very upgrade it exists to unblock. Adds `runAsUser: 65532`, matching the postgres-operator webhook-ready hook that runs the same image. - **Digest-pin the kubectl image.** The values comment claimed digest-pinning but shipped a floating `v1.32` tag. Reuses the digest postgres-operator vendors, adds the `renovate` annotation, and templates `repo:tag@digest`. - **Tests.** `etcd-operator/tests/selector-fix-hook_test.yaml` (hook wiring, weight ordering, namespaced least-privilege RBAC, numeric-non-root security context, digest-pinned image), `etcd-operator/tests/deployment_test.yaml` (256Mi cold-start floor), `extra/etcd/tests/etcd-cluster_test.yaml` (transitional `etcd-headless` Service). Each assertion was mutation-tested. ### 6. Derive the default S3 endpoint from the provisioned bucket — `packages/system/backupstrategy-controller` Derive the default S3 endpoint (and per-driver scheme / TLS / `secure_connection`) from the provisioned bucket Secret instead of requiring it to be hand-set, so etcd (and other) backup strategies get a working endpoint by default. Docs in `docs/operations/backup-classes.md`; covered by `tests/endpoint_form_test.yaml`. ### Verification - `helm unittest` green on current `main`: etcd-operator **18/18**, extra/etcd **18/18**, backupstrategy-controller **11/11**. - Behaviours 1 & 2 were reproduced and confirmed live on a 1.5.2 → 1.6.0-rc.1 adoption (3-node cluster) — recreating the `etcd-headless` Service took the adopted cluster to `readyMembers=3 / Available=True`. ```release-note fix(etcd): complete the v1alpha2 transition on in-cluster 1.5→1.6 upgrades — keep the legacy etcd-headless Service alive so adopted members stay resolvable, delete the pre-1.6 operator Deployment via a pre-upgrade hook to get past the immutable selector, raise the operator's memory floor so it does not OOM before the VPA scales it, and make the etcd adoption migration robust in-cluster. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved backup storage endpoint handling across supported backup drivers, including provisioned and external S3 storage. * Added compatibility support for legacy etcd pod discovery during migration. * Added an automated upgrade safeguard for etcd operator deployments. * **Bug Fixes** * Improved certificate SAN detection and etcd migration authentication. * Increased the etcd operator’s minimum startup memory to prevent early restarts. * **Documentation** * Clarified backup endpoint, TLS, and driver-specific behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Screenshots
Release note
Summary by CodeRabbit