fix: propagate read-only mount from staging path and volume capability in NodePublishVolume by andyzhangx · Pull Request #1067 · kubernetes-csi/csi-driver-smb

andyzhangx · 2026-04-19T02:30:50Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
When a PV has csi.readOnly: true or mount options including ro, but the pod spec volumeMounts does not explicitly set readOnly: true, the bind mount in NodePublishVolume was created without ro, allowing writes to the supposedly read-only volume.

How does it work:
NodePublishVolume now checks three sources for read-only intent:

req.GetReadonly() (existing — from pod spec volumeMounts.readOnly)
Volume capability access mode (MULTI_NODE_READER_ONLY, SINGLE_NODE_READER_ONLY)
Whether the staging mount path has ro in its mount options (propagated from PV mountOptions or csi.readOnly)

If any of these indicate read-only, the bind mount gets ro.

Which issue(s) this PR fixes:
Ref #987

Does this PR introduce a user-facing change?

Fix readOnly not being respected: propagate read-only mount option from staging path and volume capability access mode to the bind mount in NodePublishVolume.

k8s-ci-robot · 2026-04-19T02:30:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [andyzhangx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot

Pull request overview

Fixes a bug where NodePublishVolume could create a writable bind mount even when the underlying PV/staging mount and/or volume capability indicates the volume should be read-only.

Changes:

Derive effective readOnly from req.Readonly, volume capability access mode, and staging mount options.
Propagate ro to the bind mount options when any read-only signal is present.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…y in NodePublishVolume Previously, NodePublishVolume only checked req.GetReadonly() to decide whether to add 'ro' to the bind mount options. This meant that when a PV had csi.readOnly: true or mountOptions including 'ro', but the pod spec volumeMounts did not explicitly set readOnly: true, the bind mount would be writable. Fix by also checking: 1. Volume capability access mode (MULTI_NODE_READER_ONLY, SINGLE_NODE_READER_ONLY) 2. Whether the staging mount path has 'ro' in its mount options This ensures read-only intent from PV-level settings is properly propagated to the final bind mount. Ref 987

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

andyzhangx · 2026-04-19T06:47:27Z

/retest

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Guard against nil volCap.GetMount() to prevent panic with block access type - Add volumeID and target to 'ro' mount flags log message for debuggability - Add unit tests for MULTI_NODE_READER_ONLY and SINGLE_NODE_READER_ONLY access modes - Add unit test for mount flags 'ro' propagation to bind mount - Add unit test for nil Mount (block access type) to verify no panic

andyzhangx · 2026-04-19T12:58:28Z

Addressed review comments in daab8f2:

Code fixes:

Guard against nil volCap.GetMount() to prevent potential panic when AccessType is block (not mount)
Added volumeID and target to the ro mount flags log message for easier debugging

New unit tests:

MULTI_NODE_READER_ONLY access mode → read-only propagation
SINGLE_NODE_READER_ONLY access mode → read-only propagation
Mount flags containing ro → read-only propagation to bind mount
Nil Mount (block access type) → no panic

Re: outdated comments about staging mount path normalization and double mount list scan — those were addressed in the previous revision that removed the staging mount path inspection.

andyzhangx · 2026-04-19T14:45:30Z

/retest

andyzhangx · 2026-04-20T01:21:10Z

/cherrypick release-4.13

k8s-infra-cherrypick-robot · 2026-04-20T01:21:51Z

@andyzhangx: cannot checkout release-4.13: error checking out "release-4.13": exit status 1 error: pathspec 'release-4.13' did not match any file(s) known to git

Details

In response to this:

/cherrypick release-4.13

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.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Apr 19, 2026

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 19, 2026

k8s-ci-robot requested a review from jingxu97 April 19, 2026 02:30

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 19, 2026

andyzhangx force-pushed the fix-readonly-publish branch from d2f9c15 to 3693475 Compare April 19, 2026 02:32

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2026

andyzhangx requested a review from Copilot April 19, 2026 02:32

Copilot started reviewing on behalf of andyzhangx April 19, 2026 02:33 View session

Copilot AI reviewed Apr 19, 2026

View reviewed changes

Comment thread pkg/smb/nodeserver.go Outdated

Comment thread pkg/smb/nodeserver.go

Comment thread pkg/smb/nodeserver.go Outdated

Comment thread pkg/smb/nodeserver.go Outdated

andyzhangx force-pushed the fix-readonly-publish branch from 3693475 to 44f583e Compare April 19, 2026 02:44

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 19, 2026

andyzhangx requested a review from Copilot April 19, 2026 02:45

Copilot started reviewing on behalf of andyzhangx April 19, 2026 02:46 View session

Copilot AI reviewed Apr 19, 2026

View reviewed changes

Comment thread pkg/smb/nodeserver.go Outdated

Comment thread pkg/smb/nodeserver.go Outdated

Comment thread pkg/smb/nodeserver.go

andyzhangx requested a review from Copilot April 19, 2026 06:47

Copilot started reviewing on behalf of andyzhangx April 19, 2026 06:47 View session

Copilot AI reviewed Apr 19, 2026

View reviewed changes

Comment thread pkg/smb/nodeserver.go

andyzhangx requested a review from Copilot April 19, 2026 11:27

Copilot started reviewing on behalf of andyzhangx April 19, 2026 11:27 View session

Copilot AI reviewed Apr 19, 2026

View reviewed changes

Comment thread pkg/smb/nodeserver.go Outdated

k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 19, 2026

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 19, 2026

andyzhangx mentioned this pull request Apr 19, 2026

readOnly not being respected #987

Open

andyzhangx merged commit 9756db9 into kubernetes-csi:master Apr 20, 2026
20 of 21 checks passed

Conversation

andyzhangx commented Apr 19, 2026

Uh oh!

k8s-ci-robot commented Apr 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andyzhangx commented Apr 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

andyzhangx commented Apr 19, 2026

Uh oh!

andyzhangx commented Apr 19, 2026

Uh oh!

Uh oh!

andyzhangx commented Apr 20, 2026

Uh oh!

k8s-infra-cherrypick-robot commented Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants