OCPBUGS-82166: fix etcd snapshot restore for etcd 3.6 and enforce restoreSnapshotURL immutability#8186
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughThe etcd init script now prefers Sequence Diagram(s)sequenceDiagram
participant InitScript as Init Script
participant ObjStorage as Object Storage (snapshot URL)
participant FS as Filesystem
participant Tool as Tool Selector
participant EtcdUtl as /usr/bin/etcdutl
participant EtcdCtl as etcdctl
InitScript->>FS: check /var/lib/data
alt data non-empty
InitScript->>InitScript: log warning & list contents
end
InitScript->>ObjStorage: curl "snapshot-url" -> /tmp/snapshot
ObjStorage-->>InitScript: snapshot bytes
InitScript->>FS: read first 5 bytes of /tmp/snapshot
alt header == "<?xml"
InitScript->>InitScript: log error, output snapshot contents
InitScript-->>InitScript: exit 1
else header != "<?xml"
InitScript->>FS: create /var/lib/restore
InitScript->>Tool: test -x /usr/bin/etcdutl
alt etcdutl exists
Tool->>EtcdUtl: INFO + etcdutl snapshot status -w table /tmp/snapshot
EtcdUtl-->>InitScript: status
Tool->>EtcdUtl: etcdutl snapshot restore --data-dir=/var/lib/restore ...
EtcdUtl-->>FS: write /var/lib/restore
else
Tool->>Tool: test for etcdctl
alt etcdctl exists
Tool->>EtcdCtl: INFO + ETCDCTL_API=3 etcdctl snapshot status /tmp/snapshot
EtcdCtl-->>InitScript: status
Tool->>EtcdCtl: ETCDCTL_API=3 etcdctl snapshot restore --data-dir=/var/lib/restore ...
EtcdCtl-->>FS: write /var/lib/restore
else neither exists
Tool->>InitScript: ERROR "no etcdutl or etcdctl available"
InitScript-->>InitScript: exit 1
end
end
InitScript->>FS: remove /var/lib/data
InitScript->>FS: move /var/lib/restore -> /var/lib/data
end
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
area/control-plane-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@control-plane-operator/controllers/hostedcontrolplane/v2/etcd/etcd-init.sh`:
- Line 12: The curl invocation in etcd-init.sh uses an unquoted variable
${RESTORE_URL}, which can be subject to word splitting or globbing; update the
curl command to quote the RESTORE_URL variable (use "${RESTORE_URL}") so the
pre-signed S3 URL is passed as a single argument to curl and special characters
are preserved.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e712833b-5890-4272-b160-6c2d74e1ff25
📒 Files selected for processing (1)
control-plane-operator/controllers/hostedcontrolplane/v2/etcd/etcd-init.sh
jparrill
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@jparrill: This pull request references Jira Issue OCPBUGS-82166, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jparrill: This pull request references Jira Issue OCPBUGS-82166, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@jparrill: This pull request references Jira Issue OCPBUGS-82166, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8186 +/- ##
==========================================
+ Coverage 33.11% 33.16% +0.04%
==========================================
Files 768 768
Lines 93116 93163 +47
==========================================
+ Hits 30840 30902 +62
+ Misses 59665 59646 -19
- Partials 2611 2615 +4 🚀 New features to boost your workflow:
|
|
Scheduling tests matching the |
Test Resultse2e-aws
e2e-aks
|
|
/retest-required |
1 similar comment
|
/retest-required |
jparrill
changed the title
openshift-ci-robot
removed
jira/valid-reference
| route: {} | ||
| expectedError: "restoreSnapshotURL is immutable" | ||
|
|
||
| - name: When restoreSnapshotURL is added after creation it should pass |
There was a problem hiding this comment.
can we also have a case for when unset after set it should fail
| // storage specifies how etcd data is persisted. | ||
| // +required | ||
| // +kubebuilder:validation:XValidation:rule="(has(self.restoreSnapshotURL) == has(oldSelf.restoreSnapshotURL)) && (!has(self.restoreSnapshotURL) || self.restoreSnapshotURL == oldSelf.restoreSnapshotURL)",message="restoreSnapshotURL is immutable" | ||
| Storage ManagedEtcdStorageSpec `json:"storage"` |
There was a problem hiding this comment.
This is difficult to read and reason about. Here I would just add:
// +kubebuilder:validation:XValidation:rule="has(self.restoreSnapshotURL) == has(oldSelf.restoreSnapshotURL)",message="restoreSnapshotURL cannot be added or removed after creation"
Storage ManagedEtcdStorageSpec `json:"storage"`and add the equality check back ontop of restoreSnapshotUR field directly
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="restoreSnapshotURL is immutable"
RestoreSnapshotURL []string `json:"restoreSnapshotURL,omitempty"`
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, enxebre, jparrill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Add two CEL rules to enforce restoreSnapshotURL immutability: - Parent-level has() check on storage to prevent adding or removing the field after creation (covers absent-to-present transitions). - Field-level self == oldSelf on restoreSnapshotURL to prevent changing the value once set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…bility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…alidation Add CRD validation tests covering restoreSnapshotURL field constraints: - MaxItems enforcement (at most 1 entry) - Immutability on update (change and removal are rejected) - Post-creation addition is rejected - Allowed transitions (unchanged value, unrelated field updates) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
|
/lgtm |
|
Scheduling tests matching the |
| // +kubebuilder:validation:MaxItems=1 | ||
| // +kubebuilder:validation:items:MaxLength=1024 | ||
| // +kubebuilder:validation:XValidation:rule="self.size() <= 1", message="RestoreSnapshotURL shouldn't contain more than 1 entry" | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="restoreSnapshotURL is immutable" |
There was a problem hiding this comment.
can we please update the gdoc to reflect clearly this is only settable on creation
|
/retest-required |
|
/verified bypass More info in here: https://redhat-internal.slack.com/archives/G01QS0P2F6W/p1775814810815969?thread_ts=1775808691.857879&cid=G01QS0P2F6W |
openshift-ci-robot
added
the
verified
|
@jparrill: The DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
|
/test images |
|
@jparrill: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@jparrill: Jira Issue Verification Checks: Jira Issue OCPBUGS-82166 Jira Issue OCPBUGS-82166 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
…toreSnapshotURL immutability (openshift#8186) * fix(OCPBUGS-82166): add etcd 3.6 compatibility for snapshot restore etcd 3.6 (OCP 4.21+) removed `etcdctl snapshot restore/status` in favor of `etcdutl`. Add fallback logic to use the available tool. Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(CNTRLPLANE-2678): enforce restoreSnapshotURL immutability via CEL Add two CEL rules to enforce restoreSnapshotURL immutability: - Parent-level has() check on storage to prevent adding or removing the field after creation (covers absent-to-present transitions). - Field-level self == oldSelf on restoreSnapshotURL to prevent changing the value once set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * chore(CNTRLPLANE-2678): regenerate CRDs for restoreSnapshotURL immutability Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * test(CNTRLPLANE-2678): add envtest suite for restoreSnapshotURL CEL validation Add CRD validation tests covering restoreSnapshotURL field constraints: - MaxItems enforcement (at most 1 entry) - Immutability on update (change and removal are rejected) - Post-creation addition is rejected - Allowed transitions (unchanged value, unrelated field updates) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> --------- Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…toreSnapshotURL immutability (openshift#8186) * fix(OCPBUGS-82166): add etcd 3.6 compatibility for snapshot restore etcd 3.6 (OCP 4.21+) removed `etcdctl snapshot restore/status` in favor of `etcdutl`. Add fallback logic to use the available tool. Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(CNTRLPLANE-2678): enforce restoreSnapshotURL immutability via CEL Add two CEL rules to enforce restoreSnapshotURL immutability: - Parent-level has() check on storage to prevent adding or removing the field after creation (covers absent-to-present transitions). - Field-level self == oldSelf on restoreSnapshotURL to prevent changing the value once set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * chore(CNTRLPLANE-2678): regenerate CRDs for restoreSnapshotURL immutability Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * test(CNTRLPLANE-2678): add envtest suite for restoreSnapshotURL CEL validation Add CRD validation tests covering restoreSnapshotURL field constraints: - MaxItems enforcement (at most 1 entry) - Immutability on update (change and removal are rejected) - Post-creation addition is rejected - Allowed transitions (unchanged value, unrelated field updates) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> --------- Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…toreSnapshotURL immutability (openshift#8186) * fix(OCPBUGS-82166): add etcd 3.6 compatibility for snapshot restore etcd 3.6 (OCP 4.21+) removed `etcdctl snapshot restore/status` in favor of `etcdutl`. Add fallback logic to use the available tool. Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(CNTRLPLANE-2678): enforce restoreSnapshotURL immutability via CEL Add two CEL rules to enforce restoreSnapshotURL immutability: - Parent-level has() check on storage to prevent adding or removing the field after creation (covers absent-to-present transitions). - Field-level self == oldSelf on restoreSnapshotURL to prevent changing the value once set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * chore(CNTRLPLANE-2678): regenerate CRDs for restoreSnapshotURL immutability Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> * test(CNTRLPLANE-2678): add envtest suite for restoreSnapshotURL CEL validation Add CRD validation tests covering restoreSnapshotURL field constraints: - MaxItems enforcement (at most 1 entry) - Immutability on update (change and removal are rejected) - Post-creation addition is rejected - Allowed transitions (unchanged value, unrelated field updates) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> --------- Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
7 participants
Summary
etcdutlfor snapshot restore/status on etcd 3.6+ (OCP 4.21+), falling back toetcdctlfor older versionsself == oldSelf) to enforcerestoreSnapshotURLimmutability at admission time, aligning with the existing API contract ("only when the etcd PV is empty")restoreSnapshotURLfield constraintsChanges
restoreSnapshotURL+ regenerated CRDsTest plan
restoreSnapshotURLis rejected by CEL validationrestoreSnapshotURLCEL validation (make test-envtest-kube)Bug
https://redhat.atlassian.net/browse/OCPBUGS-82166
🤖 Generated with Claude Code