TRT-2702: Revert "STOR-3004: Rebase external-snapshotter to v8.6.0"#220
Merged
openshift-merge-bot[bot] merged 1 commit intoJun 11, 2026
Merged
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@redhat-chai-bot: This pull request references STOR-3004 which is a valid jira issue. 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. |
|
Warning Review limit reached
More reviews will be available in 3 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (197)
📒 Files selected for processing (103)
WalkthroughThe PR removes the v1 VolumeGroupSnapshot API surface, regenerates v1beta1/v1beta2 clients and informer/lister code, updates controller and utility code to use v1beta2 types and clients, and refreshes related CRDs, tests, manifests, and build tooling. ChangesGroup snapshot API migration
Sequence Diagram(s)sequenceDiagram
participant Controller
participant VersionedClientset
participant RESTClient
participant APIserver
Controller->>VersionedClientset: GroupsnapshotV1beta2()
VersionedClientset->>RESTClient: build typed request
RESTClient->>APIserver: Get/List/Watch/Create/Update/Delete/Patch
APIserver-->>RESTClient: typed response
RESTClient-->>Controller: VolumeGroupSnapshot* objects
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @redhat-chai-bot. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
pkg/utils/vgs.go (1)
39-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve owner-reference compatibility for existing
v1objects.Line 46 now accepts only
groupsnapshot.storage.k8s.io/v1beta2. Existing snapshots withVolumeGroupSnapshotowner refs stamped as.../v1will no longer be recognized as members, which can break upgrade behavior.Suggested fix
func getVolumeGroupSnapshotParentObjectName(snapshot *crdv1.VolumeSnapshot) string { @@ - apiVersion := fmt.Sprintf( - "%s/%s", - crdv1beta2.SchemeGroupVersion.Group, - crdv1beta2.SchemeGroupVersion.Version, - ) + v1beta2APIVersion := fmt.Sprintf("%s/%s", crdv1beta2.SchemeGroupVersion.Group, crdv1beta2.SchemeGroupVersion.Version) + v1APIVersion := fmt.Sprintf("%s/%s", crdv1beta2.SchemeGroupVersion.Group, "v1") @@ - if owner.Kind == "VolumeGroupSnapshot" && owner.APIVersion == apiVersion { + if owner.Kind == "VolumeGroupSnapshot" && + (owner.APIVersion == v1beta2APIVersion || owner.APIVersion == v1APIVersion) { return owner.Name } }Based on learnings: “Changes must be backward compatible; old code should still compile and runtime behavior cannot change in breaking ways.”
🤖 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 `@pkg/utils/vgs.go` around lines 39 - 47, The owner-ref check currently only matches crdv1beta2.SchemeGroupVersion (groupsnapshot.storage.k8s.io/v1beta2) which breaks recognition of older owner refs stamped as .../v1; update the logic that iterates snapshot.ObjectMeta.OwnerReferences to also accept the same group with version "v1" (e.g. build a second apiVersionV1 string from crdv1beta2.SchemeGroupVersion.Group and "v1") and change the conditional that tests owner.APIVersion to accept either apiVersion or apiVersionV1 when owner.Kind == "VolumeGroupSnapshot", returning owner.Name in either case.Source: Learnings
pkg/sidecar-controller/groupsnapshot_helper.go (1)
542-542:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStore the patched object in the group-snapshot cache, not
contentStore.After patching the annotation, this writes the
VolumeGroupSnapshotContentthroughctrl.storeContentUpdate(...). That updates the volume-snapshot-content cache with the wrong object type and leavesgroupSnapshotContentStorestale until an informer round-trip.Suggested fix
- _, err = ctrl.storeContentUpdate(groupSnapshotContent) + _, err = ctrl.storeGroupSnapshotContentUpdate(groupSnapshotContent)🤖 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 `@pkg/sidecar-controller/groupsnapshot_helper.go` at line 542, The code is calling ctrl.storeContentUpdate(...) with a VolumeGroupSnapshotContent (groupSnapshotContent), which writes to the generic contentStore and leaves groupSnapshotContentStore stale; replace that call so the patched object is stored in the group-snapshot cache (use the controller helper that updates groupSnapshotContentStore or directly update groupSnapshotContentStore with groupSnapshotContent) instead of calling ctrl.storeContentUpdate; update the call site that currently references ctrl.storeContentUpdate to call the group-snapshot-specific store update (or the existing method that targets groupSnapshotContentStore) so groupSnapshotContent is cached correctly.release-tools/prow.sh (1)
843-856:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInjection risk: sed substitution uses unescaped variable.
The sed command on line 856 uses
$imagedirectly without escaping, which could fail or produce unexpected results if the image name contains sed metacharacters (e.g.,/,&,\). While the risk is lower since$imageis parsed from YAML with a controlled structure, defensive escaping should still be applied.🛡️ Recommended fix: escape the image variable
image=$(echo "$nocomments" | sed -e 's;.*image:[[:space:]]*;;') name=$(echo "$image" | sed -e 's;.*/\([^:]*\).*;\1;') tag=$(echo "$image" | sed -e 's;.*:;;') # Now replace registry and/or tag NEW_TAG="csiprow" - line="$(echo "$nocomments" | sed -e "s;$image;${name}:${NEW_TAG};")" + escaped_image=$(printf '%s\n' "$image" | sed 's/[[\.*^$/]/\\&/g') + line="$(echo "$nocomments" | sed -e "s;$escaped_image;${name}:${NEW_TAG};")"As per coding guidelines: "Injection prevention: Validate at trust boundaries with allow-lists, not deny-lists" and "Command: no shell=True, os.system, or backtick exec with user input".
🤖 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 `@release-tools/prow.sh` around lines 843 - 856, The sed substitution uses the raw variable image (in the block handling nocomments -> image -> name -> tag and building line with NEW_TAG), which can break or allow injection if image contains sed metacharacters; before using image in the sed command that builds line (the sed -e "s;$image;${name}:${NEW_TAG};"), compute an escaped version (e.g., escape / and & and backslashes or choose a safe delimiter) and use that escaped variable in the sed expression instead of $image so the substitution is robust and safe.Source: Coding guidelines
client/clientset/versioned/clientset.go (1)
33-38:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPublic clientset contract break: removed
GroupsnapshotV1()accessor.Removing this exported method from
Interface/Clientsetis a compile-time break for existing consumers using the v1 entrypoint (Line 35-37, Line 49-56), which violates backward-compat expectations for this repo line.Based on learnings: "Changes must be backward compatible; old code should still compile and runtime behavior cannot change in breaking ways."
Also applies to: 49-56
🤖 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 `@client/clientset/versioned/clientset.go` around lines 33 - 38, The public Interface removed the backward-compatible accessor GroupsnapshotV1(); restore the GroupsnapshotV1() method on the Interface and implement a corresponding receiver method on Clientset that returns the same concrete subclient as the existing GroupsnapshotV1beta1/GroupsnapshotV1beta2 accessors to preserve runtime behavior. Specifically, add GroupsnapshotV1() to the Interface and implement func (c *Clientset) GroupsnapshotV1() groupsnapshotv1.GroupsnapshotV1Interface { return c.GroupsnapshotV1beta1() } (or delegate to the beta2 accessor if that matches previous behavior) so existing callers compile without changing other constructors or fields such as the Clientset subclient fields used by GroupsnapshotV1beta1/GroupsnapshotV1beta2/SnapshotV1.Source: Learnings
client/clientset/versioned/scheme/register.go (1)
35-39:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep v1 scheme registration for compatibility in v8.
Line 35-Line 39 no longer register
groupsnapshotv1.AddToScheme. This drops decode/encode support forgroupsnapshot.storage.k8s.io/v1objects in composed schemes and can break existing consumers still handling persisted/legacy v1 objects. Keep v1 registration as compatibility plumbing until a planned major-version break.Based on learnings, “Changes must be backward compatible; old code should still compile and runtime behavior cannot change in breaking ways”.
🤖 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 `@client/clientset/versioned/scheme/register.go` around lines 35 - 39, The removal of groupsnapshotv1.AddToScheme from the runtime.SchemeBuilder (localSchemeBuilder) drops support for groupsnapshot.storage.k8s.io/v1 objects; restore backward compatibility by re-adding groupsnapshotv1.AddToScheme to the localSchemeBuilder list so that the composed scheme still registers the v1 API, ensuring functions that rely on AddToScheme (localSchemeBuilder) continue to decode/encode legacy v1 objects until the planned major-version break.Source: Learnings
pkg/common-controller/framework_test.go (2)
301-361:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGroup snapshot content paths are reading the wrong backing map.
In the
volumegroupsnapshotcontentscreate/update branches, existence/version checks readr.contents(volume snapshot content map) instead ofr.groupContents. That can produce incorrect “already exists/not found/version conflict” behavior for group-content tests.Suggested fix
- _, found := r.contents[content.Name] + _, found := r.groupContents[content.Name] ... - storedVolume, found := r.contents[content.Name] + storedGroupContent, found := r.groupContents[content.Name] if found { - storedVer, _ := strconv.Atoi(storedVolume.ResourceVersion) + storedVer, _ := strconv.Atoi(storedGroupContent.ResourceVersion)🤖 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 `@pkg/common-controller/framework_test.go` around lines 301 - 361, The volumegroupsnapshotcontents branches are incorrectly reading/writing the volume snapshot map (r.contents) instead of the group snapshot map (r.groupContents); in the action.Matches("create", "volumegroupsnapshotcontents") and action.Matches("update", "volumegroupsnapshotcontents") cases, replace lookups like r.contents[content.Name] and assignments r.contents[content.Name] with r.groupContents[content.Name] (and ensure storedVolume typed to the group-content element), so existence checks, version bumps (using storedVer/requestedVer and errVersionConflict) and final storage use r.groupContents consistently while keeping the rest of the logic (DeepCopy, ResourceVersion increment, changedObjects appends) unchanged.
284-304:⚠️ Potential issue | 🟠 MajorFix snapshotReactor create action casts and group-content map lookups
- create reactors for both
volumesnapshotcontentsandvolumegroupsnapshotcontentscast the"create"action tocore.UpdateAction, which can panic; usecore.CreateActioninstead.volumegroupsnapshotcontentscreate/update existence/version checks read fromr.contentsbut results are stored inr.groupContents; user.groupContentsfor the lookups.Suggested fix
- obj := action.(core.UpdateAction).GetObject() + obj := action.(core.CreateAction).GetObject() ... - obj := action.(core.UpdateAction).GetObject() + obj := action.(core.CreateAction).GetObject()🤖 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 `@pkg/common-controller/framework_test.go` around lines 284 - 304, The create reactors for "volumesnapshotcontents" and "volumegroupsnapshotcontents" are incorrectly casting the action to core.UpdateAction and the group-content branch is checking r.contents instead of r.groupContents; change the cast to core.CreateAction when handling the create action (where you call action.(...) to get the object) and in the "volumegroupsnapshotcontents" create/update existence/version checks switch lookups from r.contents to r.groupContents so you read and store against the correct map (refer to the action.Matches cases, the core.CreateAction cast, r.contents, r.groupContents, and the types crdv1.VolumeSnapshotContent / crdv1beta2.VolumeGroupSnapshotContent).pkg/common-controller/snapshot_controller.go (1)
1012-1023:⚠️ Potential issue | 🟠 MajorFix conflict retry in
removePVCFinalizerby preserving the original update error
removePVCFinalizerreturnsnewControllerUpdateError(newPvc.Name, err.Error())from insideretry.RetryOnConflict.newControllerUpdateErrorcreates acontrollerUpdateErrorthat only stores a message string (noUnwrap/ no underlying error), sok8s.io/apimachinery/pkg/api/errors.IsConflictcan’t detect the originalapierrors.StatusErrorviaerrors.As. As a result,RetryOnConflictwon’t retry on conflicts.Suggested fix
-func (ctrl *csiSnapshotCommonController) removePVCFinalizer(pvc *v1.PersistentVolumeClaim) error { - return retry.RetryOnConflict(retry.DefaultRetry, func() error { +func (ctrl *csiSnapshotCommonController) removePVCFinalizer(pvc *v1.PersistentVolumeClaim) error { + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { // Get snapshot source which is a PVC newPvc, err := ctrl.client.CoreV1().PersistentVolumeClaims(pvc.Namespace).Get(context.TODO(), pvc.Name, metav1.GetOptions{}) if err != nil { return err } newPvc = newPvc.DeepCopy() newPvc.ObjectMeta.Finalizers = utils.RemoveString(newPvc.ObjectMeta.Finalizers, utils.PVCFinalizer) _, err = ctrl.client.CoreV1().PersistentVolumeClaims(newPvc.Namespace).Update(context.TODO(), newPvc, metav1.UpdateOptions{}) if err != nil { - return newControllerUpdateError(newPvc.Name, err.Error()) + return err } klog.V(5).Infof("Removed protection finalizer from persistent volume claim %s", pvc.Name) return nil }) + if err != nil { + return newControllerUpdateError(pvc.Name, err.Error()) + } + return nil }🤖 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 `@pkg/common-controller/snapshot_controller.go` around lines 1012 - 1023, The retry closure in removePVCFinalizer currently returns newControllerUpdateError(newPvc.Name, err.Error()) which discards the original error type and prevents errors.IsConflict/RetryOnConflict from detecting apierrors.StatusError; fix by preserving/wrapping the original error instead (either change newControllerUpdateError to wrap the original err using %w or replace the return with a wrapped error like fmt.Errorf("update pvc %s: %w", newPvc.Name, err)) so the underlying apierrors.StatusError is retained for errors.As/IsConflict to work correctly.
🧹 Nitpick comments (1)
deploy/kubernetes/csi-snapshotter/setup-csi-snapshotter.yaml (1)
74-121: 💤 Low valueConsider adding resource limits and security context for production use.
While this appears to be a test/demo manifest (based on comments), consider documenting that production deployments should add:
resources.limitsandresources.requestsfor CPU and memory on all containerssecurityContextwithrunAsNonRoot,readOnlyRootFilesystem, andallowPrivilegeEscalation: falsewhere appropriate (noting that the hostpath container legitimately requiresprivileged: truefor storage operations)This recommendation aligns with Kubernetes security best practices, though it's acceptable to defer for test manifests.
🤖 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 `@deploy/kubernetes/csi-snapshotter/setup-csi-snapshotter.yaml` around lines 74 - 121, Add guidance and defaults for production: update the containers csi-provisioner and csi-snapshotter (and any other non-privileged containers) to include resources.requests and resources.limits for CPU and memory, and add a securityContext with runAsNonRoot: true, readOnlyRootFilesystem: true, and allowPrivilegeEscalation: false; keep the hostpath container's existing securityContext privileged: true and document that it requires privileged mode, and call out the socket-dir volume usage so reviewers know where these settings apply.
🤖 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
`@client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshotcontents.yaml`:
- Around line 20-28: The CRD currently hard-codes the conversion webhook service
namespace (clientConfig.service.namespace: default) pointing to
snapshot-conversion-webhook-service which breaks installs that run the webhook
in another namespace; change the CRD to not hard-code "default" but instead
reference a templated/configurable namespace for the conversion webhook (or
remove the fixed namespace and populate it at install time) so the
conversion.clientConfig.service.namespace is provided from the chart/installer
(e.g., via a Helm value or template variable) and keep the service name
snapshot-conversion-webhook-service and path /convert as-is to maintain
compatibility.
In `@client/informers/externalversions/factory.go`:
- Around line 208-209: The example incorrectly assigns ctx, cancel from
context.Background(); change the example to call context.WithCancel to obtain
both values so defer cancel() is valid—replace the use of context.Background()
with context.WithCancel(context.Background()) so ctx and cancel are correctly
declared (symbols to update: ctx, cancel, context.Background(),
context.WithCancel(), and the existing defer cancel()).
In
`@client/informers/externalversions/volumegroupsnapshot/v1beta2/volumegroupsnapshot.go`:
- Around line 58-72: The informer uses context.TODO() in
NewFilteredVolumeGroupSnapshotInformer's cache.ListWatch ListFunc/WatchFunc
which prevents cancellation on shutdown; change the ListFunc/WatchFunc to
ListWithContextFunc and WatchFuncWithContext and forward the provided ctx into
the typed client calls (call
client.GroupsnapshotV1beta2().VolumeGroupSnapshots(namespace).List(ctx, options)
and .Watch(ctx, options)), preserving the tweakListOptions(&options) behavior
before invoking the client so in-flight list/watch requests are cancellable by
the informer context.
In `@cmd/snapshot-controller/main.go`:
- Around line 118-134: The current startup treats missing VolumeGroupSnapshot
CRDs as a fatal preflight (in the enableVolumeGroupSnapshots check calling
client.GroupsnapshotV1beta2().VolumeGroupSnapshots().List /
VolumeGroupSnapshotClasses().List / VolumeGroupSnapshotContents().List) and
returns/exit on error; change this to non-fatal behavior by logging a warning
(klog.Warningf) when any of those List calls fail, not returning false or
exiting, and defer activation: either disable volume-group-snapshot features
until CRDs are detected or spawn a background goroutine that retries those List
calls with backoff and enables the feature once successful; also remove/replace
any direct process exit at the exit path referenced (the code around the earlier
timeout exit) so the controller continues running and can recover when CRDs are
installed.
In `@deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml`:
- Line 37: Verify the snapshot-controller image tag
registry.k8s.io/sig-storage/snapshot-controller:v8.4.0 exists upstream and
document CVE/patch differences vs v8.5.0 (ensure the
HyperShift/VolumeGroupSnapshot v1beta2 fix remains); then harden the pod spec
that runs the snapshot controller by adding container-level resources, a
securityContext (runAsNonRoot: true, readOnlyRootFilesystem: true,
dropCapabilities: [ALL], allowPrivilegeEscalation: false), livenessProbe and
readinessProbe entries, and set automountServiceAccountToken: false on the
Pod/Deployment spec; preserve minReadySeconds: 35 as noted. Include these
changes around the container declaration that references the image line so
reviewers can find the updated pod/deployment spec.
In `@go.mod`:
- Around line 86-92: Bump the vulnerable OpenTelemetry module versions in
go.mod: update go.opentelemetry.io/otel and go.opentelemetry.io/otel/sdk to
their patched releases (replace v1.40.0 with the fixed versions), then
re-resolve the module graph by running go mod tidy (and commit the updated
go.sum); ensure any related indirect modules (e.g., otlptrace, otlptracegrpc,
metric, trace) are compatible and updated if needed so the build and tests pass.
In `@pkg/common-controller/snapshot_controller.go`:
- Around line 285-292: The code in
checkandRemoveSnapshotFinalizersAndCheckandDeleteContent returns nil when
isVolumeBeingCreatedFromSnapshot(snapshot) is true, which prevents immediate
requeue and can leave the Snapshot stuck; change this to trigger a requeue
instead of treating it as handled — e.g., return a requeue signal/error so the
controller retries later (for example, return a small requeue error like
fmt.Errorf("snapshot in use for PVC restore, requeue") or use the controller's
existing requeue helper if one exists), keeping the check using
isVolumeBeingCreatedFromSnapshot and preserving the eventRecorder log.
In `@pkg/metrics/metrics.go`:
- Around line 299-311: The goroutine in
operationMetricsManager.scheduleOpsInFlightMetric creates a new time.Ticker in a
for-range and never stops it, and ctx.Done() is only checked outside that inner
loop; fix by creating a ticker once (ticker :=
time.NewTicker(inFlightCheckInterval)) and using a single select loop like: for
{ select { case <-ctx.Done(): ticker.Stop(); return; case <-ticker.C:
opMgr.mu.Lock(); opMgr.opInFlight.Set(float64(len(opMgr.cache)));
opMgr.mu.Unlock() } } so the ticker is stopped on cancellation and the goroutine
exits promptly.
In `@pkg/utils/util.go`:
- Around line 405-406: The exported helper GetGroupSnapshotSecretReference was
changed to accept v1beta2 types, which breaks downstream callers; revert the
public signature to accept the original v1 types and restore its original
behavior, and add a new v1beta2-specific entry point (e.g.,
GetGroupSnapshotSecretReferenceV1Beta2 or a wrapper function) that converts
v1beta2 inputs to v1 or delegates to shared internal logic; apply the same
pattern for the other exported helpers you modified (the additional helpers
flagged in the comment) by keeping their original v1 signatures and adding
separate v1beta2 wrappers/shims that perform any necessary type conversion or
call into common unexported functions.
In `@pkg/webhook/webhook.go`:
- Around line 45-48: The http.Server instance assigned to srv omits
ReadTimeout/WriteTimeout/IdleTimeout, exposing the server to connection-hold
DoS; update the http.Server initialization (where srv is constructed) to set
sensible ReadTimeout, WriteTimeout, and IdleTimeout values (e.g.,
seconds/minutes appropriate for your handlers) and ensure any long-running
operations respect context cancellation (use the request context.Context in
handler functions) so that connections/timeouts are enforced and resources are
not held indefinitely.
In `@release-tools/.github/workflows/codespell.yml`:
- Around line 11-12: The workflow uses floating action references
(actions/checkout@v6 and codespell-project/actions-codespell@master); replace
these with full commit SHAs to pin dependencies and update any other workflows
(e.g., trivy.yaml) that also use unpinned refs. Locate the two uses in the
workflow (the lines referencing actions/checkout and
codespell-project/actions-codespell) and change them to their corresponding full
commit SHA references (e.g., actions/checkout@<commit-sha> and
codespell-project/actions-codespell@<commit-sha>), ensuring you fetch the exact
commit SHAs from the actions' repositories and use those pinned SHAs
consistently across all workflows.
---
Outside diff comments:
In `@client/clientset/versioned/clientset.go`:
- Around line 33-38: The public Interface removed the backward-compatible
accessor GroupsnapshotV1(); restore the GroupsnapshotV1() method on the
Interface and implement a corresponding receiver method on Clientset that
returns the same concrete subclient as the existing
GroupsnapshotV1beta1/GroupsnapshotV1beta2 accessors to preserve runtime
behavior. Specifically, add GroupsnapshotV1() to the Interface and implement
func (c *Clientset) GroupsnapshotV1() groupsnapshotv1.GroupsnapshotV1Interface {
return c.GroupsnapshotV1beta1() } (or delegate to the beta2 accessor if that
matches previous behavior) so existing callers compile without changing other
constructors or fields such as the Clientset subclient fields used by
GroupsnapshotV1beta1/GroupsnapshotV1beta2/SnapshotV1.
In `@client/clientset/versioned/scheme/register.go`:
- Around line 35-39: The removal of groupsnapshotv1.AddToScheme from the
runtime.SchemeBuilder (localSchemeBuilder) drops support for
groupsnapshot.storage.k8s.io/v1 objects; restore backward compatibility by
re-adding groupsnapshotv1.AddToScheme to the localSchemeBuilder list so that the
composed scheme still registers the v1 API, ensuring functions that rely on
AddToScheme (localSchemeBuilder) continue to decode/encode legacy v1 objects
until the planned major-version break.
In `@pkg/common-controller/framework_test.go`:
- Around line 301-361: The volumegroupsnapshotcontents branches are incorrectly
reading/writing the volume snapshot map (r.contents) instead of the group
snapshot map (r.groupContents); in the action.Matches("create",
"volumegroupsnapshotcontents") and action.Matches("update",
"volumegroupsnapshotcontents") cases, replace lookups like
r.contents[content.Name] and assignments r.contents[content.Name] with
r.groupContents[content.Name] (and ensure storedVolume typed to the
group-content element), so existence checks, version bumps (using
storedVer/requestedVer and errVersionConflict) and final storage use
r.groupContents consistently while keeping the rest of the logic (DeepCopy,
ResourceVersion increment, changedObjects appends) unchanged.
- Around line 284-304: The create reactors for "volumesnapshotcontents" and
"volumegroupsnapshotcontents" are incorrectly casting the action to
core.UpdateAction and the group-content branch is checking r.contents instead of
r.groupContents; change the cast to core.CreateAction when handling the create
action (where you call action.(...) to get the object) and in the
"volumegroupsnapshotcontents" create/update existence/version checks switch
lookups from r.contents to r.groupContents so you read and store against the
correct map (refer to the action.Matches cases, the core.CreateAction cast,
r.contents, r.groupContents, and the types crdv1.VolumeSnapshotContent /
crdv1beta2.VolumeGroupSnapshotContent).
In `@pkg/common-controller/snapshot_controller.go`:
- Around line 1012-1023: The retry closure in removePVCFinalizer currently
returns newControllerUpdateError(newPvc.Name, err.Error()) which discards the
original error type and prevents errors.IsConflict/RetryOnConflict from
detecting apierrors.StatusError; fix by preserving/wrapping the original error
instead (either change newControllerUpdateError to wrap the original err using
%w or replace the return with a wrapped error like fmt.Errorf("update pvc %s:
%w", newPvc.Name, err)) so the underlying apierrors.StatusError is retained for
errors.As/IsConflict to work correctly.
In `@pkg/sidecar-controller/groupsnapshot_helper.go`:
- Line 542: The code is calling ctrl.storeContentUpdate(...) with a
VolumeGroupSnapshotContent (groupSnapshotContent), which writes to the generic
contentStore and leaves groupSnapshotContentStore stale; replace that call so
the patched object is stored in the group-snapshot cache (use the controller
helper that updates groupSnapshotContentStore or directly update
groupSnapshotContentStore with groupSnapshotContent) instead of calling
ctrl.storeContentUpdate; update the call site that currently references
ctrl.storeContentUpdate to call the group-snapshot-specific store update (or the
existing method that targets groupSnapshotContentStore) so groupSnapshotContent
is cached correctly.
In `@pkg/utils/vgs.go`:
- Around line 39-47: The owner-ref check currently only matches
crdv1beta2.SchemeGroupVersion (groupsnapshot.storage.k8s.io/v1beta2) which
breaks recognition of older owner refs stamped as .../v1; update the logic that
iterates snapshot.ObjectMeta.OwnerReferences to also accept the same group with
version "v1" (e.g. build a second apiVersionV1 string from
crdv1beta2.SchemeGroupVersion.Group and "v1") and change the conditional that
tests owner.APIVersion to accept either apiVersion or apiVersionV1 when
owner.Kind == "VolumeGroupSnapshot", returning owner.Name in either case.
In `@release-tools/prow.sh`:
- Around line 843-856: The sed substitution uses the raw variable image (in the
block handling nocomments -> image -> name -> tag and building line with
NEW_TAG), which can break or allow injection if image contains sed
metacharacters; before using image in the sed command that builds line (the sed
-e "s;$image;${name}:${NEW_TAG};"), compute an escaped version (e.g., escape /
and & and backslashes or choose a safe delimiter) and use that escaped variable
in the sed expression instead of $image so the substitution is robust and safe.
---
Nitpick comments:
In `@deploy/kubernetes/csi-snapshotter/setup-csi-snapshotter.yaml`:
- Around line 74-121: Add guidance and defaults for production: update the
containers csi-provisioner and csi-snapshotter (and any other non-privileged
containers) to include resources.requests and resources.limits for CPU and
memory, and add a securityContext with runAsNonRoot: true,
readOnlyRootFilesystem: true, and allowPrivilegeEscalation: false; keep the
hostpath container's existing securityContext privileged: true and document that
it requires privileged mode, and call out the socket-dir volume usage so
reviewers know where these settings apply.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+20
to
+28
| conversion: | ||
| strategy: Webhook | ||
| webhook: | ||
| conversionReviewVersions: ["v1"] | ||
| clientConfig: | ||
| service: | ||
| namespace: default | ||
| name: snapshot-conversion-webhook-service | ||
| path: /convert |
There was a problem hiding this comment.
Don’t hard-code the conversion webhook into default.
This CRD is cluster-scoped, but the conversion endpoint is now pinned to snapshot-conversion-webhook-service.default. Any install that runs the webhook in another namespace will fail v1beta1↔v1beta2 conversions, which turns requests against the non-storage version into API errors. Please make this follow the actual deployment namespace or template it at install time. Based on learnings, changes must be backward compatible; hard-coding a namespace-specific conversion endpoint changes runtime requirements for existing installs.
🤖 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
`@client/config/crd/groupsnapshot.storage.k8s.io_volumegroupsnapshotcontents.yaml`
around lines 20 - 28, The CRD currently hard-codes the conversion webhook
service namespace (clientConfig.service.namespace: default) pointing to
snapshot-conversion-webhook-service which breaks installs that run the webhook
in another namespace; change the CRD to not hard-code "default" but instead
reference a templated/configurable namespace for the conversion webhook (or
remove the fixed namespace and populate it at install time) so the
conversion.clientConfig.service.namespace is provided from the chart/installer
(e.g., via a Helm value or template variable) and keep the service name
snapshot-conversion-webhook-service and path /convert as-is to maintain
compatibility.
Source: Learnings
Comment on lines
+208
to
209
| // ctx, cancel := context.Background() | ||
| // defer cancel() |
There was a problem hiding this comment.
Fix broken context example in documentation.
The example code has invalid Go syntax:
ctx, cancel := context.Background()
defer cancel()context.Background() returns only one value (a Context), so this multiple assignment will fail. The code should be either:
ctx := context.Background()(no cancel), orctx, cancel := context.WithCancel(context.Background())(with cancel)
Since defer cancel() appears on line 209, the intended pattern is likely the second option.
📝 Suggested fix
- ctx, cancel := context.Background()
+ ctx, cancel := context.WithCancel(context.Background())
defer cancel()🤖 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 `@client/informers/externalversions/factory.go` around lines 208 - 209, The
example incorrectly assigns ctx, cancel from context.Background(); change the
example to call context.WithCancel to obtain both values so defer cancel() is
valid—replace the use of context.Background() with
context.WithCancel(context.Background()) so ctx and cancel are correctly
declared (symbols to update: ctx, cancel, context.Background(),
context.WithCancel(), and the existing defer cancel()).
Comment on lines
58
to
72
| func NewFilteredVolumeGroupSnapshotInformer(client versioned.Interface, namespace string, resyncPeriod time.Duration, indexers cache.Indexers, tweakListOptions internalinterfaces.TweakListOptionsFunc) cache.SharedIndexInformer { | ||
| return cache.NewSharedIndexInformer( | ||
| cache.ToListWatcherWithWatchListSemantics(&cache.ListWatch{ | ||
| &cache.ListWatch{ | ||
| ListFunc: func(options v1.ListOptions) (runtime.Object, error) { | ||
| if tweakListOptions != nil { | ||
| tweakListOptions(&options) | ||
| } | ||
| return client.GroupsnapshotV1beta2().VolumeGroupSnapshots(namespace).List(context.Background(), options) | ||
| return client.GroupsnapshotV1beta2().VolumeGroupSnapshots(namespace).List(context.TODO(), options) | ||
| }, | ||
| WatchFunc: func(options v1.ListOptions) (watch.Interface, error) { | ||
| if tweakListOptions != nil { | ||
| tweakListOptions(&options) | ||
| } | ||
| return client.GroupsnapshotV1beta2().VolumeGroupSnapshots(namespace).Watch(context.Background(), options) | ||
| return client.GroupsnapshotV1beta2().VolumeGroupSnapshots(namespace).Watch(context.TODO(), options) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Check this informer's list/watch context usage"
rg -n -C2 'context\.TODO\(\)|ListWithContextFunc|WatchFunc|ToListWatcherWithWatchListSemantics' client/informers/externalversions/volumegroupsnapshot/v1beta2/volumegroupsnapshot.go
echo
echo "2) Compare with other generated informers for context-aware patterns"
rg -n -C2 'ListWithContextFunc|ToListWatcherWithWatchListSemantics|context\.TODO\(\)' client/informers/externalversions --type go
echo
echo "3) Confirm client-go version(s) that generator behavior is tied to"
rg -n 'k8s\.io/client-go' go.mod client/go.mod || trueRepository: openshift/csi-external-snapshotter
Length of output: 11884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo vendor presence (if any):"
ls -la | sed -n '1,120p'
echo
echo "Search for context-aware ListWatch callback fields in the repo:"
rg -n --hidden "ListWithContextFunc|WatchWithContextFunc" . || true
echo
echo "If module cache is available, search for ListWatch definition in k8s.io/client-go:"
if command -v go >/dev/null 2>&1; then
GOPATH="$(go env GOPATH 2>/dev/null || true)"
if [ -n "${GOPATH}" ] && [ -d "$GOPATH/pkg/mod" ]; then
rg -n "type ListWatch struct" "$GOPATH/pkg/mod" -S -g '*client-go*' | head -n 50
echo
rg -n "ListWithContextFunc|WatchWithContextFunc" "$GOPATH/pkg/mod" -S -g '*client-go*' | head -n 50
else
echo "No GOPATH/pkg/mod directory found in sandbox."
fi
else
echo "go command not found in sandbox."
fiRepository: openshift/csi-external-snapshotter
Length of output: 27388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect ListWatch context semantics:"
sed -n '1,220p' vendor/k8s.io/client-go/tools/cache/listwatch.go | cat -n
echo
sed -n '220,360p' vendor/k8s.io/client-go/tools/cache/listwatch.go | cat -n
echo
echo "Search for NewSharedIndexInformer and Reflector run loop:"
rg -n "func NewSharedIndexInformer|type Reflector|func \\(.*\\) Run\\(|ListWatch\\." vendor/k8s.io/client-go/tools/cache -S
echo
echo "Inspect reflector where list/watch are invoked:"
rg -n "lw\\.List|lw\\.Watch|ListWithContextFunc|WatchWithContextFunc" vendor/k8s.io/client-go/tools/cache/reflector.go -n
echo
sed -n '1,260p' vendor/k8s.io/client-go/tools/cache/reflector.go | cat -n
echo
sed -n '260,520p' vendor/k8s.io/client-go/tools/cache/reflector.go | cat -nRepository: openshift/csi-external-snapshotter
Length of output: 15054
🏁 Script executed:
#!/bin/bash
set -u
echo "Find where Reflector calls List/Watch (with context) in reflector.go"
rg -n "ListWithContext\\(|WatchWithContext\\(|List\\(|Watch\\(|ToListerWatcherWithContext|context\\.With" vendor/k8s.io/client-go/tools/cache/reflector.go
echo
echo "Show Reflector.Run and nearby code paths"
sed -n '320,520p' vendor/k8s.io/client-go/tools/cache/reflector.go | cat -n
echo
echo "Show Reflector.list / Reflector.watch (likely helper methods)"
rg -n "func \\(r \\*Reflector\\) (list|watch|List|Watch)" vendor/k8s.io/client-go/tools/cache/reflector.go || true
sed -n '520,820p' vendor/k8s.io/client-go/tools/cache/reflector.go | cat -n
echo
echo "Inspect shared informer wiring to Reflector (stopCh/context)"
sed -n '240,420p' vendor/k8s.io/client-go/tools/cache/shared_informer.go | cat -nRepository: openshift/csi-external-snapshotter
Length of output: 34182
Replace context.TODO() with the reflector/informer context in List/Watch
client/informers/externalversions/volumegroupsnapshot/v1beta2/volumegroupsnapshot.go hardcodes context.TODO() in the cache.ListWatch ListFunc/WatchFunc calls (lines 65, 71). Reflector uses ListWithContext(ctx)/WatchWithContext(ctx), but because ListWithContextFunc/WatchFuncWithContext are unset, those calls fall back to ListFunc/WatchFunc, so shutdown cancellation won’t stop in-flight list/watch requests. Switch to ListWithContextFunc/WatchFuncWithContext and pass through the provided ctx to the typed client List/Watch.
🤖 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
`@client/informers/externalversions/volumegroupsnapshot/v1beta2/volumegroupsnapshot.go`
around lines 58 - 72, The informer uses context.TODO() in
NewFilteredVolumeGroupSnapshotInformer's cache.ListWatch ListFunc/WatchFunc
which prevents cancellation on shutdown; change the ListFunc/WatchFunc to
ListWithContextFunc and WatchFuncWithContext and forward the provided ctx into
the typed client calls (call
client.GroupsnapshotV1beta2().VolumeGroupSnapshots(namespace).List(ctx, options)
and .Watch(ctx, options)), preserving the tweakListOptions(&options) behavior
before invoking the client so in-flight list/watch requests are cancellable by
the informer context.
Source: Coding guidelines
Comment on lines
118
to
134
| if enableVolumeGroupSnapshots { | ||
| _, err = client.GroupsnapshotV1().VolumeGroupSnapshots("").List(ctx, listOptions) | ||
| _, err = client.GroupsnapshotV1beta2().VolumeGroupSnapshots("").List(ctx, listOptions) | ||
| if err != nil { | ||
| klog.Errorf("Failed to list v1 volumegroupsnapshots with error=%+v", err) | ||
| klog.Errorf("Failed to list v1beta2 volumegroupsnapshots with error=%+v", err) | ||
| return false, nil | ||
| } | ||
|
|
||
| _, err = client.GroupsnapshotV1().VolumeGroupSnapshotClasses().List(ctx, listOptions) | ||
| _, err = client.GroupsnapshotV1beta2().VolumeGroupSnapshotClasses().List(ctx, listOptions) | ||
| if err != nil { | ||
| klog.Errorf("Failed to list v1 volumegroupsnapshotclasses with error=%+v", err) | ||
| klog.Errorf("Failed to list v1beta2 volumegroupsnapshotclasses with error=%+v", err) | ||
| return false, nil | ||
| } | ||
| _, err = client.GroupsnapshotV1().VolumeGroupSnapshotContents().List(ctx, listOptions) | ||
| _, err = client.GroupsnapshotV1beta2().VolumeGroupSnapshotContents().List(ctx, listOptions) | ||
| if err != nil { | ||
| klog.Errorf("Failed to list v1 volumegroupsnapshotcontents with error=%+v", err) | ||
| klog.Errorf("Failed to list v1beta2 volumegroupsnapshotcontents with error=%+v", err) | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
Startup still hard-fails on missing VolumeGroupSnapshot CRDs.
Line 118-Line 134 make group-snapshot CRD presence a blocking preflight, and Line 258-Line 260 exit the process when it times out. In environments where CRDs are installed asynchronously, this preserves crash-loop startup behavior. Make group-snapshot CRD readiness non-fatal at startup (warn + background retry / delayed activation) so the controller can stay up and recover when CRDs appear.
Also applies to: 258-260
🤖 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 `@cmd/snapshot-controller/main.go` around lines 118 - 134, The current startup
treats missing VolumeGroupSnapshot CRDs as a fatal preflight (in the
enableVolumeGroupSnapshots check calling
client.GroupsnapshotV1beta2().VolumeGroupSnapshots().List /
VolumeGroupSnapshotClasses().List / VolumeGroupSnapshotContents().List) and
returns/exit on error; change this to non-fatal behavior by logging a warning
(klog.Warningf) when any of those List calls fail, not returning false or
exiting, and defer activation: either disable volume-group-snapshot features
until CRDs are detected or spawn a background goroutine that retries those List
calls with backoff and enables the feature once successful; also remove/replace
any direct process exit at the exit path referenced (the code around the earlier
timeout exit) so the controller continues running and can recover when CRDs are
installed.
| containers: | ||
| - name: snapshot-controller | ||
| image: registry.k8s.io/sig-storage/snapshot-controller:v8.5.0 | ||
| image: registry.k8s.io/sig-storage/snapshot-controller:v8.4.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify snapshot-controller v8.4.0 exists and check for security issues
echo "=== Checking snapshot-controller v8.4.0 image ==="
docker manifest inspect "registry.k8s.io/sig-storage/snapshot-controller:v8.4.0" >/dev/null 2>&1 && echo "✓ Image exists" || echo "✗ Image not found"
echo ""
echo "=== Checking external-snapshotter v8.4.0 release ==="
gh api repos/kubernetes-csi/external-snapshotter/releases/tags/v8.4.0 --jq '.name, .published_at, .body' 2>/dev/null | head -20
echo ""
echo "=== Checking v8.5.0 release notes for critical fixes ==="
gh api repos/kubernetes-csi/external-snapshotter/releases/tags/v8.5.0 --jq '.name, .body' 2>/dev/null | head -30Repository: openshift/csi-external-snapshotter
Length of output: 1922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check registry.k8s.io manifest existence for the exact tag (no docker dependency)
echo "=== Checking registry.k8s.io manifest for snapshot-controller:v8.4.0 ==="
python3 - <<'PY'
import base64, json, os, sys, urllib.request
url = "https://registry.k8s.io/v2/sig-storage/snapshot-controller/manifests/v8.4.0"
req = urllib.request.Request(url, method="GET", headers={
"Accept": "application/vnd.docker.distribution.manifest.v2+json"
})
try:
with urllib.request.urlopen(req, timeout=20) as resp:
print("✓ Exists (HTTP %s)" % resp.status)
except urllib.error.HTTPError as e:
# 401/403 may indicate auth; still captures truthiness better than “not found”
print("✗ Not accessible as anonymous (HTTP %s)" % e.code)
except Exception as e:
print("✗ Error:", e)
PY
# 2) Fetch changelog content for v8.5.0 and v8.4.0 and search for security + VolumeGroupSnapshot v1beta2 references
echo ""
echo "=== Searching upstream CHANGELOG-8.5.md for CVE/security + VolumeGroupSnapshot v1beta2 ==="
python3 - <<'PY'
import re, sys, urllib.request
base = "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter"
def fetch(ref, path):
url = f"{base}/{ref}/{path}"
with urllib.request.urlopen(url, timeout=20) as r:
return r.read().decode("utf-8", "replace")
text_85 = fetch("v8.5.0", "CHANGELOG/CHANGELOG-8.5.md")
patterns = [
r"CVE[-\s]?\d+",
r"\bsecurity\b",
r"\bvulnerability\b",
r"VolumeGroupSnapshot",
r"v1beta2"
]
for pat in patterns:
hits = [(m.start(), m.group(0) if hasattr(m, "group") else "") for m in re.finditer(pat, text_85, flags=re.IGNORECASE)]
if hits:
print(f"== Matches for {pat} (showing up to 10 hits) ==")
for m in re.finditer(pat, text_85, flags=re.IGNORECASE):
# print containing line
line_start = text_85.rfind("\n", 0, m.start())+1
line_end = text_85.find("\n", m.end())
line = text_85[line_start:line_end]
print(line.strip())
if "CVE" in pat.upper() and len(hits) >= 10: break
# cap output globally
print()
PY
echo ""
echo "=== Searching upstream CHANGELOG-8.4.md for VolumeGroupSnapshot v1beta2 references ==="
python3 - <<'PY'
import re, urllib.request
base = "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter"
url = f"{base}/v8.4.0/CHANGELOG/CHANGELOG-8.4.md"
with urllib.request.urlopen(url, timeout=20) as r:
text = r.read().decode("utf-8", "replace")
for pat in [r"VolumeGroupSnapshot", r"v1beta2", r"beta2"]:
print(f"== Matches for {pat} ==")
found=0
for m in re.finditer(pat, text, flags=re.IGNORECASE):
line_start = text.rfind("\n", 0, m.start())+1
line_end = text.find("\n", m.end())
line = text[line_start:line_end]
if line.strip():
print(line.strip())
found += 1
if found>=20: break
if found==0: print("(none)")
print()
PYRepository: openshift/csi-external-snapshotter
Length of output: 3033
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Fetch changelog files from GitHub via gh api (avoids direct registry TLS issues)
fetch_md () {
local ref="$1"
local path="$2"
# Get file contents (base64) from GitHub
gh api "repos/kubernetes-csi/external-snapshotter/contents/${path}?ref=${ref}" --jq '.content' -r | base64 -d
}
echo "=== Fetch CHANGELOG-8.5.md ==="
CH85="$(fetch_md "v8.5.0" "CHANGELOG/CHANGELOG-8.5.md")"
echo "${CH85}" | head -n 60
echo ""
echo "=== Search CHANGELOG-8.5.md for security/CVE keywords ==="
echo "${CH85}" | rg -n -i "cve|security|vulnerability|fix|patched" | head -n 50 || true
echo ""
echo "=== Search CHANGELOG-8.5.md for VolumeGroupSnapshot / v1beta2 mentions ==="
echo "${CH85}" | rg -n -i "VolumeGroupSnapshot|v1beta2|beta2" | head -n 80 || true
echo ""
echo "=== Fetch CHANGELOG-8.4.md (context for VolumeGroupSnapshot support) ==="
CH84="$(fetch_md "v8.4.0" "CHANGELOG/CHANGELOG-8.4.md")"
echo "${CH84}" | head -n 60
echo ""
echo "=== Search CHANGELOG-8.4.md for VolumeGroupSnapshot / v1beta2 mentions ==="
echo "${CH84}" | rg -n -i "VolumeGroupSnapshot|v1beta2|beta2" | head -n 80 || true
echo ""
echo "=== Search CHANGELOG-8.4.md for security/CVE keywords ==="
echo "${CH84}" | rg -n -i "cve|security|vulnerability|fix|patched" | head -n 50 || true
echo ""
echo "=== Done ==="Repository: openshift/csi-external-snapshotter
Length of output: 1554
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fetch_md () {
local ref="$1"
local path="$2"
local b64
b64="$(gh api "repos/kubernetes-csi/external-snapshotter/contents/${path}?ref=${ref}" --jq '.content')"
echo "$b64" | tr -d '\n' | base64 -d
}
echo "=== Fetch CHANGELOG-8.5.md (first 60 lines) ==="
CH85="$(fetch_md "v8.5.0" "CHANGELOG/CHANGELOG-8.5.md")"
echo "$CH85" | head -n 60
echo ""
echo "=== Search CHANGELOG-8.5.md for security/CVE keywords ==="
echo "$CH85" | rg -n -i "cve|security|vulnerability|fix|patched" | head -n 50 || true
echo ""
echo "=== Search CHANGELOG-8.5.md for VolumeGroupSnapshot / v1beta2 mentions ==="
echo "$CH85" | rg -n -i "VolumeGroupSnapshot|v1beta2|beta2" | head -n 80 || true
echo ""
echo "=== Fetch CHANGELOG-8.4.md (first 60 lines) ==="
CH84="$(fetch_md "v8.4.0" "CHANGELOG/CHANGELOG-8.4.md")"
echo "$CH84" | head -n 60
echo ""
echo "=== Search CHANGELOG-8.4.md for VolumeGroupSnapshot / v1beta2 mentions ==="
echo "$CH84" | rg -n -i "VolumeGroupSnapshot|v1beta2|beta2" | head -n 80 || true
echo ""
echo "=== Search CHANGELOG-8.4.md for security/CVE keywords ==="
echo "$CH84" | rg -n -i "cve|security|vulnerability|fix|patched" | head -n 50 || trueRepository: openshift/csi-external-snapshotter
Length of output: 675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml"
echo "=== File: $FILE (lines 1-140) ==="
nl -ba "$FILE" | sed -n '1,140p'
echo ""
echo "=== Search for key security/scheduling fields ==="
rg -n "securityContext|runAsNonRoot|readOnlyRootFilesystem|allowPrivilegeEscalation|hostPID|hostNetwork|hostIPC|privileged|capabilities:|automountServiceAccountToken|resources:|limits:|requests:|livenessProbe|readinessProbe|minReadySeconds|--retry|crd|interval|maxUnavailable|podAntiAffinity|topologySpreadConstraints" "$FILE" || trueRepository: openshift/csi-external-snapshotter
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml"
echo "=== File: $FILE ==="
wc -l "$FILE"
echo ""
echo "=== Show first 200 lines with line numbers ==="
cat -n "$FILE" | sed -n '1,200p'
echo ""
echo "=== Search for key security/scheduling fields ==="
rg -n "securityContext|runAsNonRoot|readOnlyRootFilesystem|allowPrivilegeEscalation|hostPID|hostNetwork|hostIPC|privileged|capabilities:|automountServiceAccountToken|resources:|limits:|requests:|livenessProbe|readinessProbe|minReadySeconds|--retry|crd|interval|maxUnavailable|podAntiAffinity|topologySpreadConstraints" "$FILE" || trueRepository: openshift/csi-external-snapshotter
Length of output: 2687
Confirm snapshot-controller downgrade image + security posture
deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yamlusesregistry.k8s.io/sig-storage/snapshot-controller:v8.4.0; ensure this tag is present upstream, and that downgrading from v8.5.0 doesn’t reintroduce critical CVEs and still includes the HyperShift/VolumeGroupSnapshotv1beta2 handling fix.- The manifest lacks container
resourcesandsecurityContext(e.g.,runAsNonRoot,readOnlyRootFilesystem, drop capabilities,allowPrivilegeEscalation: false), has no liveness/readiness probes, and doesn’t setautomountServiceAccountToken: false(unless required). minReadySeconds: 35matches the CRD availability retry rationale in the comments.
🤖 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 `@deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml` at line
37, Verify the snapshot-controller image tag
registry.k8s.io/sig-storage/snapshot-controller:v8.4.0 exists upstream and
document CVE/patch differences vs v8.5.0 (ensure the
HyperShift/VolumeGroupSnapshot v1beta2 fix remains); then harden the pod spec
that runs the snapshot controller by adding container-level resources, a
securityContext (runAsNonRoot: true, readOnlyRootFilesystem: true,
dropCapabilities: [ALL], allowPrivilegeEscalation: false), livenessProbe and
readinessProbe entries, and set automountServiceAccountToken: false on the
Pod/Deployment spec; preserve minReadySeconds: 35 as noted. Include these
changes around the container declaration that references the image line so
reviewers can find the updated pod/deployment spec.
Comment on lines
+285
to
292
| // check if the snapshot is being used for restore a PVC, if yes, do nothing | ||
| // and wait until PVC restoration finishes | ||
| if content != nil && ctrl.isVolumeBeingCreatedFromSnapshot(snapshot) { | ||
| klog.V(4).Infof("checkandRemoveSnapshotFinalizersAndCheckandDeleteContent[%s]: snapshot is being used to restore a PVC", utils.SnapshotKey(snapshot)) | ||
| ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "SnapshotDeletePending", "Snapshot is being used to restore a PVC") | ||
| return fmt.Errorf("snapshot %s is in use (being used to restore a PVC), will retry deletion", utils.SnapshotKey(snapshot)) | ||
| // TODO(@xiangqian): should requeue this? | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Returning nil here can strand snapshots in terminating state.
Line 291 acknowledges a transient “delete pending” condition, but returning nil means the key is treated as handled and may not be retried promptly. This can leave finalizers stuck until a separate event happens.
🤖 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 `@pkg/common-controller/snapshot_controller.go` around lines 285 - 292, The
code in checkandRemoveSnapshotFinalizersAndCheckandDeleteContent returns nil
when isVolumeBeingCreatedFromSnapshot(snapshot) is true, which prevents
immediate requeue and can leave the Snapshot stuck; change this to trigger a
requeue instead of treating it as handled — e.g., return a requeue signal/error
so the controller retries later (for example, return a small requeue error like
fmt.Errorf("snapshot in use for PVC restore, requeue") or use the controller's
existing requeue helper if one exists), keeping the check using
isVolumeBeingCreatedFromSnapshot and preserving the eventRecorder log.
Comment on lines
299
to
+311
| func (opMgr *operationMetricsManager) scheduleOpsInFlightMetric(ctx context.Context) { | ||
| ticker := time.NewTicker(inFlightCheckInterval) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| func() { | ||
| opMgr.mu.Lock() | ||
| defer opMgr.mu.Unlock() | ||
| opMgr.opInFlight.Set(float64(len(opMgr.cache))) | ||
| }() | ||
| default: | ||
| for range time.NewTicker(inFlightCheckInterval).C { | ||
| func() { | ||
| opMgr.mu.Lock() | ||
| defer opMgr.mu.Unlock() | ||
| opMgr.opInFlight.Set(float64(len(opMgr.cache))) | ||
| }() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/metrics/metrics.go"
echo "== Relevant block =="
nl -ba "$FILE" | sed -n '294,314p'
echo
echo "== Pattern check: nested ticker loop without inner ctx.Done case =="
rg -n 'for range time.NewTicker\(inFlightCheckInterval\)\.C|ctx\.Done\(\)|ticker\.Stop\(' "$FILE"Repository: openshift/csi-external-snapshotter
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="pkg/metrics/metrics.go"
echo "== Relevant block (lines 294-314) =="
python3 - <<'PY'
path = "pkg/metrics/metrics.go"
start, end = 294, 314
with open(path, "r", encoding="utf-8") as f:
for i, line in enumerate(f, start=1):
if start <= i <= end:
print(f"{i:4d}\t{line.rstrip()}")
PY
echo
echo "== Pattern check =="
rg -n 'for range time\.NewTicker\(inFlightCheckInterval\)\.C|ctx\.Done\(\)|ticker\.Stop\(' "$FILE" || trueRepository: openshift/csi-external-snapshotter
Length of output: 782
Fix in-flight metrics goroutine cancellation + ticker lifecycle leak (pkg/metrics/metrics.go:299-313)
ctx.Done()is only checked before entering thefor range time.NewTicker(inFlightCheckInterval).Cloop, so cancellation won’t stop the goroutine.- The ticker created at line 305 is never
Stop()’d.
Proposed fix
func (opMgr *operationMetricsManager) scheduleOpsInFlightMetric(ctx context.Context) {
- for {
- select {
- case <-ctx.Done():
- return
- default:
- for range time.NewTicker(inFlightCheckInterval).C {
- func() {
- opMgr.mu.Lock()
- defer opMgr.mu.Unlock()
- opMgr.opInFlight.Set(float64(len(opMgr.cache)))
- }()
- }
- }
- }
+ ticker := time.NewTicker(inFlightCheckInterval)
+ defer ticker.Stop()
+ for {
+ select {
+ case <-ctx.Done():
+ return
+ case <-ticker.C:
+ opMgr.mu.Lock()
+ opMgr.opInFlight.Set(float64(len(opMgr.cache)))
+ opMgr.mu.Unlock()
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (opMgr *operationMetricsManager) scheduleOpsInFlightMetric(ctx context.Context) { | |
| ticker := time.NewTicker(inFlightCheckInterval) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case <-ticker.C: | |
| func() { | |
| opMgr.mu.Lock() | |
| defer opMgr.mu.Unlock() | |
| opMgr.opInFlight.Set(float64(len(opMgr.cache))) | |
| }() | |
| default: | |
| for range time.NewTicker(inFlightCheckInterval).C { | |
| func() { | |
| opMgr.mu.Lock() | |
| defer opMgr.mu.Unlock() | |
| opMgr.opInFlight.Set(float64(len(opMgr.cache))) | |
| }() | |
| } | |
| func (opMgr *operationMetricsManager) scheduleOpsInFlightMetric(ctx context.Context) { | |
| ticker := time.NewTicker(inFlightCheckInterval) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case <-ticker.C: | |
| opMgr.mu.Lock() | |
| opMgr.opInFlight.Set(float64(len(opMgr.cache))) | |
| opMgr.mu.Unlock() | |
| } | |
| } | |
| } |
🤖 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 `@pkg/metrics/metrics.go` around lines 299 - 311, The goroutine in
operationMetricsManager.scheduleOpsInFlightMetric creates a new time.Ticker in a
for-range and never stops it, and ctx.Done() is only checked outside that inner
loop; fix by creating a ticker once (ticker :=
time.NewTicker(inFlightCheckInterval)) and using a single select loop like: for
{ select { case <-ctx.Done(): ticker.Stop(); return; case <-ticker.C:
opMgr.mu.Lock(); opMgr.opInFlight.Set(float64(len(opMgr.cache)));
opMgr.mu.Unlock() } } so the ticker is stopped on cancellation and the goroutine
exits promptly.
Source: Coding guidelines
Comment on lines
405
to
+406
| // GetSecretReference for the group snapshot | ||
| func GetGroupSnapshotSecretReference(secretParams secretParamsMap, volumeGroupSnapshotClassParams map[string]string, groupSnapContentName string, volumeGroupSnapshot *groupsnapshotv1.VolumeGroupSnapshot) (*v1.SecretReference, error) { | ||
| func GetGroupSnapshotSecretReference(secretParams secretParamsMap, volumeGroupSnapshotClassParams map[string]string, groupSnapContentName string, volumeGroupSnapshot *crdv1beta2.VolumeGroupSnapshot) (*v1.SecretReference, error) { |
There was a problem hiding this comment.
Avoid replacing the exported v1 helper signatures in place.
These are public pkg/utils helpers. Changing their parameter types from group-snapshot v1 objects to v1beta2 objects breaks compilation for any downstream caller that still passes the old types. Add v1beta2-specific entry points or compatibility shims instead of rewriting the existing exported signatures in place.
Based on learnings, changes must be backward compatible; old code should still compile and runtime behavior cannot change in breaking ways.
Also applies to: 505-506, 517-518, 532-533, 586-665
🤖 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 `@pkg/utils/util.go` around lines 405 - 406, The exported helper
GetGroupSnapshotSecretReference was changed to accept v1beta2 types, which
breaks downstream callers; revert the public signature to accept the original v1
types and restore its original behavior, and add a new v1beta2-specific entry
point (e.g., GetGroupSnapshotSecretReferenceV1Beta2 or a wrapper function) that
converts v1beta2 inputs to v1 or delegates to shared internal logic; apply the
same pattern for the other exported helpers you modified (the additional helpers
flagged in the comment) by keeping their original v1 signatures and adding
separate v1beta2 wrappers/shims that perform any necessary type conversion or
call into common unexported functions.
Source: Learnings
Comment on lines
45
to
48
| srv := &http.Server{ | ||
| Handler: mux, | ||
| TLSConfig: tlsConfig, | ||
| ReadTimeout: 10 * time.Second, | ||
| WriteTimeout: 10 * time.Second, | ||
| IdleTimeout: 15 * time.Second, | ||
| Handler: mux, | ||
| TLSConfig: tlsConfig, | ||
| } |
There was a problem hiding this comment.
Restore HTTP server timeouts to avoid connection-hold DoS risk.
Lines 45-48 remove ReadTimeout/WriteTimeout/IdleTimeout, leaving the server vulnerable to slow or stalled connections consuming resources indefinitely.
Suggested fix
import (
"context"
"crypto/tls"
"fmt"
"net/http"
+ "time"
@@
srv := &http.Server{
- Handler: mux,
- TLSConfig: tlsConfig,
+ Handler: mux,
+ TLSConfig: tlsConfig,
+ ReadTimeout: 10 * time.Second,
+ WriteTimeout: 10 * time.Second,
+ IdleTimeout: 60 * time.Second,
}As per coding guidelines: “context.Context for cancellation and timeouts.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| srv := &http.Server{ | |
| Handler: mux, | |
| TLSConfig: tlsConfig, | |
| ReadTimeout: 10 * time.Second, | |
| WriteTimeout: 10 * time.Second, | |
| IdleTimeout: 15 * time.Second, | |
| Handler: mux, | |
| TLSConfig: tlsConfig, | |
| } | |
| srv := &http.Server{ | |
| Handler: mux, | |
| TLSConfig: tlsConfig, | |
| ReadTimeout: 10 * time.Second, | |
| WriteTimeout: 10 * time.Second, | |
| IdleTimeout: 60 * time.Second, | |
| } |
🤖 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 `@pkg/webhook/webhook.go` around lines 45 - 48, The http.Server instance
assigned to srv omits ReadTimeout/WriteTimeout/IdleTimeout, exposing the server
to connection-hold DoS; update the http.Server initialization (where srv is
constructed) to set sensible ReadTimeout, WriteTimeout, and IdleTimeout values
(e.g., seconds/minutes appropriate for your handlers) and ensure any
long-running operations respect context cancellation (use the request
context.Context in handler functions) so that connections/timeouts are enforced
and resources are not held indefinitely.
Source: Coding guidelines
Comment on lines
+11
to
+12
| - uses: actions/checkout@v6 | ||
| - uses: codespell-project/actions-codespell@master |
There was a problem hiding this comment.
Supply chain security: Multiple workflows use unpinned GitHub Actions. Both codespell.yml and trivy.yaml switched from pinned commit hashes to floating references (@v6, @master). The shared root cause is a violation of dependency pinning requirements—all three action references should use full commit SHAs to prevent supply chain attacks via compromised upstream repositories.
🤖 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 `@release-tools/.github/workflows/codespell.yml` around lines 11 - 12, The
workflow uses floating action references (actions/checkout@v6 and
codespell-project/actions-codespell@master); replace these with full commit SHAs
to pin dependencies and update any other workflows (e.g., trivy.yaml) that also
use unpinned refs. Locate the two uses in the workflow (the lines referencing
actions/checkout and codespell-project/actions-codespell) and change them to
their corresponding full commit SHA references (e.g.,
actions/checkout@<commit-sha> and
codespell-project/actions-codespell@<commit-sha>), ensuring you fetch the exact
commit SHAs from the actions' repositories and use those pinned SHAs
consistently across all workflows.
Source: Coding guidelines
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
Member
openshift-ci
Bot
changed the title
|
@redhat-chai-bot: This pull request references TRT-2702 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. 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. |
This reverts merge commit cab9af9 (PR openshift#219). The v8.6.0 rebase changed the snapshot-controller to require VolumeGroupSnapshot CRDs at startup when CSIVolumeGroupSnapshot=true. In HyperShift hosted clusters, CRDs are installed asynchronously in the guest cluster, creating a race condition that causes 6-7 crash-loop restarts of csi-snapshot-controller. This broke payload 5.0.0-0.ci-2026-06-11-001513 (hypershift-e2e-aks and hypershift-e2e-aws both at 100% failure rate).
redhat-chai-bot
force-pushed
the
06111240-bot-changes
branch
from
1c12147 to
56ba1dc
Compare
Member
|
/payload-job periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aks periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn |
|
@petr-muller: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/ecc294a0-659a-11f1-9ae7-703c60bf7b4a-0 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsafrane, redhat-chai-bot 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
|
/override ci/prow/unit
|
|
@jsafrane: Overrode contexts on behalf of jsafrane: ci/prow/unit 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. |
openshift-ci-robot
added
the
verified
|
@petr-muller: This PR has been marked as verified by 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. |
Member
|
/override ci/prow/e2e-aws-csi |
|
@petr-muller: Overrode contexts on behalf of petr-muller: ci/prow/e2e-aws-csi, ci/prow/e2e-gcp-csi, ci/prow/e2e-vsphere, ci/prow/e2e-vsphere-csi 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. |
Member
|
We are confident the reverted PR was the TRT-2702 culprit, so I'm expediting the merge so the payloads heal |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This reverts merge commit cab9af9 (PR #219).
Why
The v8.6.0 rebase changed the snapshot-controller to require
VolumeGroupSnapshotCRDs at startup whenCSIVolumeGroupSnapshot=true. In HyperShift hosted clusters, CRDs are installed asynchronously in the guest cluster, creating a race condition that causes 6–7 crash-loop restarts ofcsi-snapshot-controllerbefore the CRDs become available.This broke the 5.0 CI payload
5.0.0-0.ci-2026-06-11-001513(rejected — baseline5.0.0-0.ci-2026-06-10-181513was Accepted).Failing Jobs (100% failure rate)
hypershift-e2e-aks— 2/2 failureshypershift-e2e-aws— 2/2 failuresNotes
The original PR's presubmit CI had no HyperShift coverage, so the race condition was undetected before merge. The rebase should be re-landed with a fix that makes the
VolumeGroupSnapshotCRD dependency non-blocking at startup (e.g., retry loop or optional initialization)./cc @rvagner78
Summary by CodeRabbit
Release Notes
Breaking Changes
Chores