OCPBUGS-83667: inject restoreSnapshotURL into HostedControlPlane during restore - #244
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@jparrill: This pull request references Jira Issue OCPBUGS-83667, 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. |
WalkthroughThe changes extend etcd snapshot URL handling from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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
![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
🧹 Nitpick comments (1)
pkg/core/restore.go (1)
168-193: Extract the snapshot-restore injection flow into a shared helper.This block now duplicates the HostedCluster path for annotation lookup, optional presigning, and
RestoreSnapshotURLmutation. Since this regression came from HC/HCP behavior drifting, centralizing this logic would make the next change much less likely to miss one resource type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/core/restore.go` around lines 168 - 193, Extract the duplicated snapshot-restore injection logic into a single helper (e.g., injectSnapshotRestoreURL or similar) and call it from both HostedCluster and HostedControlPlane flows: the helper should accept context, the annotations map or metav1.Object (used with meta.Accessor), the original snapshotURL string, the backup and logger (to call p.presignS3URL and p.log), and a target object mutator that sets Spec.Etcd.Managed.Storage.RestoreSnapshotURL on the resource and writes back via runtime.DefaultUnstructuredConverter.ToUnstructured plus input.Item.SetUnstructuredContent; replace the inline code in restore.go that uses meta.Accessor, strings.HasPrefix(s3://), p.presignS3URL, setting hcp.Spec.Etcd.Managed.Storage.RestoreSnapshotURL and converting to unstructured with a call to this helper so both HostedCluster and HostedControlPlane share the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/core/backup.go`:
- Around line 167-173: The log call currently prints p.etcdSnapshotURL verbatim
which can leak sensitive presigned URLs; update the code around where
p.etcdSnapshotURL is used (the block that calls meta.Accessor(item),
common.AddAnnotation(metadata, common.EtcdSnapshotURLAnnotation,
p.etcdSnapshotURL) and p.log.Infof) to avoid logging the full URL — either log
only the HostedControlPlane name via metadata.GetName() or a redacted form
(e.g., replace everything after the host/path with "<REDACTED>" or show only the
scheme+host), and keep the annotation write unchanged; modify the p.log.Infof
invocation to use the redacted value or just the HCP name instead of
p.etcdSnapshotURL.
---
Nitpick comments:
In `@pkg/core/restore.go`:
- Around line 168-193: Extract the duplicated snapshot-restore injection logic
into a single helper (e.g., injectSnapshotRestoreURL or similar) and call it
from both HostedCluster and HostedControlPlane flows: the helper should accept
context, the annotations map or metav1.Object (used with meta.Accessor), the
original snapshotURL string, the backup and logger (to call p.presignS3URL and
p.log), and a target object mutator that sets
Spec.Etcd.Managed.Storage.RestoreSnapshotURL on the resource and writes back via
runtime.DefaultUnstructuredConverter.ToUnstructured plus
input.Item.SetUnstructuredContent; replace the inline code in restore.go that
uses meta.Accessor, strings.HasPrefix(s3://), p.presignS3URL, setting
hcp.Spec.Etcd.Managed.Storage.RestoreSnapshotURL and converting to unstructured
with a call to this helper so both HostedCluster and HostedControlPlane share
the same logic.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a56902ec-3350-4399-b333-8fcfee1b6ec9
📒 Files selected for processing (4)
pkg/core/backup.gopkg/core/backup_test.gopkg/core/restore.gopkg/core/restore_test.go
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jparrill: This pull request references Jira Issue OCPBUGS-83667, 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. |
…g OADP restore The restore plugin was only injecting restoreSnapshotURL into the HostedCluster but not into the HostedControlPlane. When the HyperShift operator reconciled the HC and tried to propagate restoreSnapshotURL to the already-created HCP, the immutability webhook rejected the mutation with "restoreSnapshotURL cannot be added or removed after creation". This fix ensures both the HC and HCP carry the etcd snapshot URL annotation during backup and have restoreSnapshotURL injected into their specs during restore, before Velero submits them to the API server. Fixes: OCPBUGS-83667 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
|
@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: This pull request references Jira Issue OCPBUGS-83667, 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/core/restore.go (1)
168-195: Extract shared snapshot URL handling to reduce HC/HCP drift risk.The presign+inject logic here duplicates the HostedCluster path (Line 211 onward). Consider moving this into a shared helper (e.g., resolve snapshot URL + resource-specific setter) so future fixes don’t diverge across the two flows.
♻️ Refactor sketch
+func (p *RestorePlugin) resolveSnapshotURL(ctx context.Context, backup *velerov1api.Backup, metadata metav1.Object) (string, error) { + annotations := metadata.GetAnnotations() + snapshotURL := annotations[common.EtcdSnapshotURLAnnotation] + if snapshotURL == "" { + return "", nil + } + if strings.HasPrefix(snapshotURL, "s3://") { + presigned, err := p.presignS3URL(ctx, backup, snapshotURL) + if err != nil { + return "", fmt.Errorf("error generating pre-signed URL for etcd snapshot: %w", err) + } + return presigned, nil + } + return snapshotURL, nil +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/core/restore.go` around lines 168 - 195, The presign+inject logic in restore.go is duplicated for HostedControlPlane and HostedCluster; extract the shared URL resolution into a helper (e.g., ResolveSnapshotURL(ctx, backup, snapshotURL) that calls p.presignS3URL when needed and returns the final URL or error) and create small resource-specific setter functions (e.g., InjectRestoreSnapshotURLIntoHCP(hcp, url) and the existing HC setter) that mutate the object/unstructured content; replace the duplicated block around p.presignS3URL and hcp.Spec.Etcd.Managed.Storage.RestoreSnapshotURL with a call to the resolver and the HCP injector so both flows reuse the same URL logic and only differ in the setter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/core/restore.go`:
- Around line 168-195: The presign+inject logic in restore.go is duplicated for
HostedControlPlane and HostedCluster; extract the shared URL resolution into a
helper (e.g., ResolveSnapshotURL(ctx, backup, snapshotURL) that calls
p.presignS3URL when needed and returns the final URL or error) and create small
resource-specific setter functions (e.g., InjectRestoreSnapshotURLIntoHCP(hcp,
url) and the existing HC setter) that mutate the object/unstructured content;
replace the duplicated block around p.presignS3URL and
hcp.Spec.Etcd.Managed.Storage.RestoreSnapshotURL with a call to the resolver and
the HCP injector so both flows reuse the same URL logic and only differ in the
setter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 021fec8b-e60b-4c16-b2bf-53c68f4c81ac
📒 Files selected for processing (4)
pkg/core/backup.gopkg/core/backup_test.gopkg/core/restore.gopkg/core/restore_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/core/backup_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/core/backup.go
- pkg/core/restore_test.go
|
/lgtm |
|
@jparrill: Jira Issue OCPBUGS-83667: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-83667 has been moved to the MODIFIED state. 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 backport oadp-1.6 |
|
@jparrill: The following backport issues have been created: Queuing cherrypicks to the requested branches to be created after this PR merges: 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. |
|
@openshift-ci-robot: #244 failed to apply on top of branch "oadp-1.6": 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 kubernetes-sigs/prow repository. |
|
I have verified this part by running backup/resetore tests from this branch: https://github.com/mgencur/hypershift/tree/CNTRLPLANE-2834_extend |
5 participants
Summary
etcd-snapshot-urlannotation on the HostedControlPlane item (previously only set on HostedCluster)restoreSnapshotURLinto the HCP spec before Velero submits it to the API server, mirroring the existing HC logic%wfor error wrapping in presign callers to preserve the error chainProblem
The restore plugin injected
restoreSnapshotURLinto the HostedCluster but not into the HostedControlPlane. When the HyperShift operator reconciled and tried to propagate the field to the already-created HCP, the immutability webhook rejected it:Root Cause
The Velero
RestoreItemAction.Execute()contract mutates items in-memory before API submission. The plugin handledHostedClustercorrectly but only validated platform config forHostedControlPlanewithout injecting the snapshot URL.Test plan
TestRestoreExecuteHCPSnapshotURL— s3 presign, https passthrough, no annotation, presign error (missing BSL)TestExecute— HCP annotation injection during backupgo test ./...)Fixes: https://issues.redhat.com/browse/OCPBUGS-83667
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests