OSAC-4187: proxy CSI attach/detach directly to vendor controllers (0.2 temporary) - #393
Conversation
|
@rgolangh: This pull request references OSAC-4187 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 story to target the "5.1.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. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 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 |
|
🤖 Finished Review · ✅ Success · Started 5:09 PM UTC · Completed 5:25 PM UTC Commit: |
ReviewFindingsHigh
Medium
Low
Labels: PR modifies CSI storage driver component Next steps:
Previous runReviewFindingsHigh
Medium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
|
| if err != nil { | ||
| if st, ok := status.FromError(err); ok && st.Code() == codes.AlreadyExists { | ||
| klog.Infof("Volume %s already published to node %s", req.GetVolumeId(), req.GetNodeId()) | ||
| return &csi.ControllerPublishVolumeResponse{}, nil |
There was a problem hiding this comment.
AlreadyExists returns an empty response here, so we drop the vendor's PublishContext. The attacher persists that on the VolumeAttachment and hands it to NodeStageVolume, so if this branch fires on a re-publish (e.g. controller restart) we overwrite a good context with an empty one and node staging loses the device/mount attrs.
A compliant vendor returns OK with the same context on re-publish rather than AlreadyExists, so the normal path is fine, but that's not guaranteed across Trident/VAST/Pure. Fine to leave for 0.2 with a comment; worth fixing in the 0.3 rework.
|
waiting for #405 to be merged, then rebase and push |
…2 temporary) Milestone-0.2 temporary arrangement (reworked in 0.3): the CSI controller proxies ControllerPublish/UnpublishVolume directly to the vendor CSI controller selected by the 'osac.backend' volume-context key (the StorageBackend name resolved from the volume's StorageTier). The sentinel 'none' marks node-local backends that need no controller-side attach, making publish/unpublish a no-op. - controller.go: route attach/detach to the vendor controller via the proxy manager, keyed by osac.backend; drop the ControlPlane attach path. - driver.go/main.go: NewDriver now takes vendorControllers; add the --vendor-controllers flag; generalize parseVendorSockets into parseBackendMap; remove the unused ControlPlaneClient/ControlPlaneStub. - chart: add controller.vendorControllers to values.yaml and render --vendor-controllers in the controller Deployment. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Roy Golan <rgolan@redhat.com>
86c1e44 to
5103986
Compare
|
🤖 Review · ❌ Terminated · Started 3:30 PM UTC · Ended 3:46 PM UTC Commit: |
|
/lgtm |
|
🤖 Finished Review · ✅ Success · Started 3:30 PM UTC · Completed 3:46 PM UTC Commit: |
Auto-dismissed: only Prow labels gate merging
|
New changes are detected. LGTM label has been removed. |
|
🤖 Review · Commit: |
7e6dc43 to
5103986
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rgolangh 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 |
|
🤖 Finished Review · ✅ Success · Started 5:06 PM UTC · Completed 5:22 PM UTC Commit: |
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
osac-csi-driver/cmd/osac-csi-driver/main.go:87: [high] logic-error
In stub mode, VolumeStub is created with backend name "default-backend", but the chart default vendorControllers has no entry for "default-backend" (only local=none,vast=...,pure=...,ontap=...). ControllerPublishVolume fails with codes.NotFound in stub/dev mode. The AGENTS.md claims the stub reports the "local" backend, but the code contradicts this.
Suggested fix: Change fulfillment.NewVolumeStub("default-backend", "nfs") to fulfillment.NewVolumeStub("local", "nfs") so stub-created volumes route to the "local" backend mapped to the "none" sentinel.
osac-csi-driver/test/sanity/sanity_test.go:51: [medium] test-inadequate
Neither the sanity test (backendName="fake") nor unit tests ("test-backend") exercise the real stub-mode path where the stub backend name must match the chart vendorControllers default. The backend name mismatch bug is invisible to the test suite.
Suggested fix: Add a test that constructs the driver as main.go does in stub mode and verifies ControllerPublishVolume succeeds.
osac-csi-driver/pkg/driver/controller.go(file-level): Line 785 · [low] secrets-in-transit
CSI secrets (req.GetSecrets()) are forwarded to vendor CSI controllers over plaintext gRPC. Extends the existing node-side pattern to controller-side attach/detach. Author flagged as ADVISORY; tracked for 0.3 rework.
osac-csi-driver/pkg/driver/controller.go(file-level): Line 866 · [low] trust-boundary-bypass
resolvePublishTarget trusts volume-context keys without cross-checking the fulfillment service when both values are present. A compromised CO is already cluster-admin; the CSI spec relies on faithful context forwarding. Author flagged as ADVISORY; tracked for 0.3 rework.
osac-csi-driver/pkg/driver/controller.go:340: [low] naming-convention
resolveVendorController lacks a doc comment, breaking the pattern where every private helper in this file and node.go has one.
osac-csi-driver/pkg/driver/controller.go(file-level): Line 786 · [low] volume-context-forwarding
The full VolumeContext including OSAC-internal keys (osac.backend, osac.volume-id, osac.protocol) is forwarded to the vendor CSI controller. Consider stripping OSAC-internal keys before forwarding.
Suggested fix: Strip OSAC-internal keys from the volume context before forwarding to the vendor.
Auto-dismissed: only Prow labels gate merging
6604965
Summary
ControllerPublishVolume/ControllerUnpublishVolumeinosac-csi-driverby proxying directly to the vendor CSI controller for the volume's backend, mirroring the existing node-plugin vendor-socket proxy. Routing key isosac.backend(the StorageBackend name resolved from the volume's StorageTier); the fulfillment volume id is translated to the vendor-side id and the vendor'sPublishContextis returned to the CO unchanged.nonesentinel endpoint so node-local backends (lvms/topolvm,attachRequired=false, no network CSI controller) make attach/detach a no-op instead of dialing a vendor.AlreadyExists/NotFound/Unimplementedvendor responses are treated as success.ControlPlaneClient/ControlPlaneStubindirection (never backed by a real fulfillment attach API); add the--vendor-controllersflag and wirecontroller.vendorControllersinto the csi-driver chart with a StorageBackend-name-keyed default (local=none,vast=…,pure=…,ontap=…:50051).Jira
OSAC-4187
Test plan
pkg/driver/controller_test.goexercise publish/unpublish against an in-process fake vendor CSI controller: vendor-id translation,PublishContextforwarding, context-vs-GetVolumeresolution, unknown-backendNotFound,AlreadyExists/NotFound/Unimplementedidempotency, and thenonesentinel no-op.test/sanity/) green with the newNewDriversignature.make test,pkg/driver79.9% coverage)make fmt,make lint0 issues,make build)helm lint charts/csi-driverclean;--vendor-controllersrenders on the controller containerNotes for reviewers
charts/csi-driver/values.yamldefault +controller-deployment.yamlflag) — review-sensitive per AGENTS.md. The default endpoints target theosac-csi-backendsServices on gRPC port 50051; keys are the expected StorageBackend naming convention.osac.backend/osac.volume-idvolume-context over authoritativeGetVolume(matches node-plugin trust model); secrets are forwarded to vendor controllers over the pre-existing plaintext in-cluster gRPC proxy.This PR description was drafted with AI assistance (create-pr v0.1.3). Review for accuracy