Support both public and private VolumeGroupSnapshot APIs - #2662
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (11)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2026-07-21T04:41:07.092ZApplied to files:
🪛 ast-grep (0.45.1)internal/controller/cephfscg/volumegroupsourcehandler.go[warning] 301-302: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first. (log-injection-request-data-concat-go) 🔇 Additional comments (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds runtime support for public and private VolumeGroupSnapshot APIs. It introduces shared wrappers and helpers, updates controller registration and handlers, pins tests to the private API, upgrades dependencies, and updates external snapshotter manifests to release 8.6. ChangesVolumeGroupSnapshot API compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant VGSUtilities
participant CRDAPI
participant ControllerBuilder
Controller->>VGSUtilities: ensure local VGS API
VGSUtilities->>CRDAPI: query public and private CRDs
CRDAPI-->>VGSUtilities: selected GroupVersion
VGSUtilities->>ControllerBuilder: register selected ownership or watch
ControllerBuilder-->>Controller: complete controller setup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ELENAGER
force-pushed
the
public_vgs_api
branch
9 times, most recently
from
July 21, 2026 17:39
6fd08c5 to
6e9c8ce
Compare
ELENAGER
changed the title
ELENAGER
force-pushed
the
public_vgs_api
branch
3 times, most recently
from
July 23, 2026 10:08
b4c460b to
1db8351
Compare
ELENAGER
force-pushed
the
public_vgs_api
branch
16 times, most recently
from
August 11, 2026 13:48
888dae3 to
a3cdf17
Compare
ELENAGER
force-pushed
the
public_vgs_api
branch
4 times, most recently
from
August 11, 2026 14:17
49fd7d4 to
879d480
Compare
ELENAGER
marked this pull request as ready for review
August 11, 2026 17:32
ELENAGER
requested review from
BenamarMk,
ShyamsundarR,
nirs,
parikshithb and
raghavendra-talur
as code owners
August 11, 2026 17:32
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/controller/util/cephfs_cg_test.go (1)
310-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd public wrapper cases for the shared abstraction tests.
The changed fixtures exercise only
NewPrivateVGSCWrapperandNewPrivateVGSCWrappers. Add equivalent public API fixtures. This validates the publicgroupsnapshot.storage.k8s.io/v1metadata and driver access path.
internal/controller/util/cephfs_cg_test.go#L310-L334: run the matcher cases with a publicVolumeGroupSnapshotClasswrapper.internal/controller/drpolicy_peerclass_internal_test.go#L1429-L1464: add public wrapper fixtures for the grouped peer case.internal/controller/drpolicy_peerclass_internal_test.go#L1514-L1603: add public wrapper fixtures for the multiple grouped-peer case.🤖 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 `@internal/controller/util/cephfs_cg_test.go` around lines 310 - 334, Add equivalent public VolumeGroupSnapshotClass wrapper fixtures alongside the existing private-wrapper cases in util.VolumeGroupSnapshotClassMatchStorageProviders tests; update internal/controller/util/cephfs_cg_test.go lines 310-334, internal/controller/drpolicy_peerclass_internal_test.go lines 1429-1464, and internal/controller/drpolicy_peerclass_internal_test.go lines 1514-1603, preserving each case’s expected matcher result while exercising the public groupsnapshot.storage.k8s.io/v1 metadata and driver access path.
🤖 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 `@internal/controller/volumereplicationgroup_controller.go`:
- Around line 118-123: In the VolSync-enabled branch of the controller setup,
move the EnsureLocalVGSAPI call before addVolsyncOwnsAndWatches so VGS API
selection is initialized before registering ownership watches. Preserve the
existing error return for unavailable VolumeGroupSnapshot APIs and keep watch
registration after successful initialization.
In
`@test/drenv/addons/external_snapshotter/start-data/controller/kustomization.yaml`:
- Around line 3-4: Update the snapshot-controller resources referenced by the
kustomization so the deployed image is
registry.k8s.io/sig-storage/snapshot-controller:v8.6.0; either override the
image in the kustomization or reference manifests containing v8.6.0, while
preserving the existing RBAC and setup resources.
---
Nitpick comments:
In `@internal/controller/util/cephfs_cg_test.go`:
- Around line 310-334: Add equivalent public VolumeGroupSnapshotClass wrapper
fixtures alongside the existing private-wrapper cases in
util.VolumeGroupSnapshotClassMatchStorageProviders tests; update
internal/controller/util/cephfs_cg_test.go lines 310-334,
internal/controller/drpolicy_peerclass_internal_test.go lines 1429-1464, and
internal/controller/drpolicy_peerclass_internal_test.go lines 1514-1603,
preserving each case’s expected matcher result while exercising the public
groupsnapshot.storage.k8s.io/v1 metadata and driver access path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f9e8c1b6-9b28-4685-9c4b-8b1b0e318f5f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
cmd/main.gogo.modinternal/controller/cephfscg/cephfscg_suite_test.gointernal/controller/cephfscg/diffvolumegroupsourcehandler.gointernal/controller/cephfscg/utils.gointernal/controller/cephfscg/volumegroupsourcehandler.gointernal/controller/drclusterconfig_controller.gointernal/controller/drclusterconfig_controller_test.gointernal/controller/drpolicy_peerclass.gointernal/controller/drpolicy_peerclass_internal_test.gointernal/controller/fake_mcv_test.gointernal/controller/replicationgroupsource_controller.gointernal/controller/suite_test.gointernal/controller/util/cephfs_cg.gointernal/controller/util/cephfs_cg_test.gointernal/controller/util/mcv_util.gointernal/controller/util/util_suite_test.gointernal/controller/util/vgs_controller.gointernal/controller/util/vgs_wrapper.gointernal/controller/volsync/vshandler.gointernal/controller/volumereplicationgroup_controller.gotest/drenv/addons/external_snapshotter/config.pytest/drenv/addons/external_snapshotter/start-data/controller/kustomization.yamltest/drenv/addons/external_snapshotter/start-data/crds/kustomization.yaml
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.45.1)
internal/controller/cephfscg/volumegroupsourcehandler.go
[warning] 301-302: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: fmt.Errorf("failed to get PVC from VGS %s: %w",
vgs.GetNamespace()+"/"+*vs.Spec.Source.PersistentVolumeClaimName, err)
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🔇 Additional comments (20)
internal/controller/util/vgs_wrapper.go (1)
18-343: LGTM!internal/controller/util/mcv_util.go (1)
16-22: LGTM!Also applies to: 86-86, 342-425
internal/controller/util/cephfs_cg.go (1)
136-136: LGTM!Also applies to: 278-296
internal/controller/drpolicy_peerclass.go (1)
28-28: LGTM!Also applies to: 208-208, 710-727
internal/controller/fake_mcv_test.go (1)
55-55: LGTM!internal/controller/cephfscg/diffvolumegroupsourcehandler.go (1)
95-121: LGTM!Also applies to: 163-168, 189-207, 232-246, 263-275, 297-312, 356-368, 397-404
internal/controller/cephfscg/volumegroupsourcehandler.go (1)
127-158: LGTM!Also applies to: 176-196, 265-303
internal/controller/volsync/vshandler.go (1)
1236-1250: LGTM!internal/controller/util/vgs_controller.go (1)
17-80: LGTM!internal/controller/drclusterconfig_controller.go (1)
429-441: LGTM!Also applies to: 575-591
internal/controller/replicationgroupsource_controller.go (1)
226-231: LGTM!internal/controller/cephfscg/utils.go (1)
56-61: LGTM!cmd/main.go (1)
16-16: LGTM!Also applies to: 126-126
go.mod (1)
4-4: LGTM!Also applies to: 21-21, 34-40, 64-80, 144-157, 160-165
internal/controller/cephfscg/cephfscg_suite_test.go (1)
143-145: LGTM!internal/controller/drclusterconfig_controller_test.go (1)
36-36: LGTM!Also applies to: 176-179
internal/controller/suite_test.go (1)
248-250: LGTM!internal/controller/util/util_suite_test.go (1)
129-131: LGTM!test/drenv/addons/external_snapshotter/config.py (1)
7-8: LGTM!test/drenv/addons/external_snapshotter/start-data/crds/kustomization.yaml (1)
3-8: LGTM!
Comment on lines
+3
to
+4
| - https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/release-8.6/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml | ||
| - https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/release-8.6/deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
manifest="$(curl -fsSL 'https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/release-8.6/deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml')"
image="$(printf '%s\n' "$manifest" | awk '$1 == "image:" && $2 ~ /snapshot-controller:/ { print $2; exit }')"
test "$image" = "registry.k8s.io/sig-storage/snapshot-controller:v8.6.0"Repository: RamenDR/ramen
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/drenv/addons/external_snapshotter/start-data/controller/kustomization.yaml"
printf '%s\n' '--- kustomization ---'
cat -n "$file"
printf '%s\n' '--- referenced manifest image and API references ---'
manifest="$(curl -fsSL 'https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/release-8.6/deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml')"
printf '%s\n' "$manifest" | grep -E 'image:|apiVersion:|kind:'Repository: RamenDR/ramen
Length of output: 1045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- external-snapshotter release notes and API manifests ---'
for ref in release-8.5 release-8.6; do
printf '\n[%s setup image]\n' "$ref"
curl -fsSL "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${ref}/deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml" |
grep -E 'image:|feature-gates' || true
printf '[%s VGS API references]\n' "$ref"
curl -fsSL "https://api.github.com/repos/kubernetes-csi/external-snapshotter/contents/deploy/kubernetes?ref=${ref}" |
grep -o '"name": "[^"]*"' | grep -i group || true
done
printf '\n--- repository VGS v1 usage ---\n'
rg -n --glob '!vendor/**' --glob '!*.sum' 'groupsnapshot\.storage\.k8s\.io/v1|VolumeGroupSnapshot|CSIVolumeGroupSnapshot' .Repository: RamenDR/ramen
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- controller images ---'
for ref in release-8.5 release-8.6; do
image="$(
curl -fsSL "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${ref}/deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml" |
awk '$1 == "image:" && $2 ~ /snapshot-controller:/ { print $2; exit }'
)"
printf '%s %s\n' "$ref" "$image"
done
printf '%s\n' '--- release-8.6 changelog candidates ---'
curl -fsSL 'https://api.github.com/repos/kubernetes-csi/external-snapshotter/git/trees/release-8.6?recursive=1' |
jq -r '.tree[] | select(.path | test("(?i)(change|release|version|group.?snapshot)")) | .path' |
head -80
printf '%s\n' '--- repository API usage, excluding generated/vendor data ---'
rg -n --glob '!vendor/**' --glob '!**/go.sum' --glob '!**/*.lock' \
'groupsnapshot\.storage\.k8s\.io/v1|VolumeGroupSnapshot|CSIVolumeGroupSnapshot' \
test config deploy .github 2>/dev/null | head -120Repository: RamenDR/ramen
Length of output: 403
Use the v8.6.0 snapshot-controller image.
The release-8.6 manifest deploys registry.k8s.io/sig-storage/snapshot-controller:v8.5.0, not v8.6.0. Override the image to v8.6.0, or use a manifest that contains that image.
🤖 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
`@test/drenv/addons/external_snapshotter/start-data/controller/kustomization.yaml`
around lines 3 - 4, Update the snapshot-controller resources referenced by the
kustomization so the deployed image is
registry.k8s.io/sig-storage/snapshot-controller:v8.6.0; either override the
image in the kustomization or reference manifests containing v8.6.0, while
preserving the existing RBAC and setup resources.
Introduce dual public/private VolumeGroupSnapshot API support as a
standalone foundation layer. No controllers or handlers are wired to
use it yet; runtime behavior is unchanged.
Add internal/controller/util/vgs_wrapper.go with helpers that select
the API at runtime based on whether the public VGS CRD is installed
(groupsnapshot.storage.k8s.io):
- NewVolumeGroupSnapshot, GetVolumeGroupSnapshot,
ListVolumeGroupSnapshots
- VolumeGroupSnapshotIsReady, SetVolumeGroupSnapshotClassName,
SetVolumeGroupSnapshotSourceSelector
- VolumeGroupSnapshotClassWrapper and VGSC list/MCV unmarshaling
helpers (GetVolumeGroupSnapshotClasses, GetVGSClassFromMCV)
- Test helpers: NewPrivateVGSCWrapper, NewPrivateVGSCWrappers
Register the public VolumeGroupSnapshot API
(groupsnapshot.storage.k8s.io/v1) in the manager scheme (cmd/main.go)
so the types are available when callers are wired in a follow-up PR.
Bump github.com/kubernetes-csi/external-snapshotter/client/v8 to
v8.6.0, which provides the public volumegroupsnapshot/v1 types.
Retain the existing Kubernetes stack via replace directives for
k8s.io/api, k8s.io/apimachinery, and k8s.io/client-go so the bump
does not force a cluster-wide k8s dependency upgrade.
Follow-up PR will switch reconcile paths (CephFS CG, VolSync, MCV,
DRPolicy peer-class, DRClusterConfig, etc.) to use these helpers.
Signed-off-by: Elena Gershkovich <elenage@il.ibm.com>
ELENAGER
force-pushed
the
public_vgs_api
branch
3 times, most recently
from
August 12, 2026 10:33
51ed308 to
a11e237
Compare
Signed-off-by: Elena Gershkovich <elenage@il.ibm.com>
Signed-off-by: Elena Gershkovich <elenage@il.ibm.com>
Centralize controller-runtime registration for VolumeGroupSnapshot and
VolumeGroupSnapshotClass so public vs private API selection stays in
one place alongside UsePublicVGSAPI and UsePrivateVGSAPI.
Add internal/controller/util/vgs_controller.go with:
- OwnsVolumeGroupSnapshot: register Owns for the active VGS API
(public if its CRD is installed, else private). Returns whether
a VGS API is available.
- WatchesVolumeGroupSnapshotClass: register Watches for the active
VGSC API using the caller's event handler and predicates. Skips
registration when neither CRD is installed.
ReplicationGroupSourceReconciler:
- Replace private-only Owns wiring with OwnsVolumeGroupSnapshot.
- Own exactly one VGS type, matching runtime API selection.
- Improve error message when no VGS CRD is installed.
DRClusterConfigReconciler:
- Replace unconditional private VGSC Watches with
WatchesVolumeGroupSnapshotClass.
- Watch public or private VGSC based on installed CRDs.
Controllers no longer import both VGS API types for setup wiring;
type selection is fully encapsulated in util.
Signed-off-by: Elena Gershkovich <elenage@il.ibm.com>
Upgrade CRDs and snapshot-controller from release-8.4 to release-8.6 so public VolumeGroupSnapshot CRDs serve groupsnapshot.storage.k8s.io/v1, matching Ramen's public client. Signed-off-by: Elena Gershkovich <elenage@il.ibm.com>
ELENAGER
force-pushed
the
public_vgs_api
branch
from
August 12, 2026 11:06
a11e237 to
6e8c8f9
Compare
raghavendra-talur
approved these changes
Aug 13, 2026
Member
|
@ELENAGER I have reviewed the approach and the wiring for the wrapper in the code. LGTM. I haven't verified that every instance of vgs and vgsc access is behind the wrapper now. |
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
2 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.
Summary
Add dual support for the public VolumeGroupSnapshot API (
groupsnapshot.storage.k8s.io/v1) alongside the existing private OpenShift API (groupsnapshot.storage.openshift.io/v1beta1).Ramen selects the API from installed CRDs:
On the reconciler cluster, selection runs during controller
SetupWithManagerviaEnsureLocalVGSAPI. The chosen API is stored for the process lifetime and used to register watches and drive reconcile helpers. An operator restart is required to pick up later CRD changes.On the hub, for resources on managed clusters, selection is independent per cluster: fetch that cluster's VGS CRDs through ManagedClusterView, choose Group/Version the same way, then fetch VolumeGroupSnapshotClass through that API.
API differences are isolated in
vgs_wrapper.go. Existing reconcile flows stay largely the same; callers use wrapper helpers instead of private-only types.Changes
external-snapshotter/client/v8to v8.6.0 and register public VGS types in the manager schemevgs_wrapper.go,vgs_controller.go)