Restrict volume mountpoint to /var/lib/kubelet/pods - #2957
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: srranjan-sri The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @srranjan-sri! |
|
Hi @srranjan-sri. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with 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. |
andyzhangx
left a comment
There was a problem hiding this comment.
this csi driver mount different folders under /var/lib/kubelet/, e.g. plugins, kerberos, kubernetes/csi, and azfilesauth may create different folders under this dir, if we restrict all dirs in this csi driver, that would make this csi driver difficult to functional well and difficult to maintain
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mittachaitu, srranjan-sri The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@andyzhangx i had added some new changes, can you plz review it?? |
There was a problem hiding this comment.
Pull request overview
This PR tightens the Azure File CSI node DaemonSet hostPath exposure by restricting the mountpoint directory to the kubelet pods subtree and introducing a dedicated hostPath for Kerberos cache/config files used by Managed Identity authentication.
Changes:
- Restricts
mountpoint-dirhostPath usage from the kubelet root to/var/lib/kubelet/pods(and Helm equivalent). - Adds a new
kerberos-dirhostPath volume mapped to/var/lib/kubelet/kerberosand mounts it into relevant containers. - Updates
azfilesrefresh(and Helm template equivalents) to usekerberos-dirfor identity-related files.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| deploy/csi-azurefile-node.yaml | Restricts the node DaemonSet’s kubelet mountpoint hostPath to /var/lib/kubelet/pods and adds a Kerberos hostPath. |
| charts/latest/azurefile-csi-driver/templates/csi-azurefile-node.yaml | Mirrors the mountpoint restriction and Kerberos hostPath additions in the Helm template. |
Comments suppressed due to low confidence (4)
charts/latest/azurefile-csi-driver/templates/csi-azurefile-node.yaml:171
kerberos-diris a file directory hostPath and does not needmountPropagation: Bidirectional; keeping it bidirectional expands host-mount influence beyond what's required for kerberos cache files.
- mountPath: /var/lib/kubelet/kerberos
mountPropagation: Bidirectional
name: kerberos-dir
charts/latest/azurefile-csi-driver/templates/csi-azurefile-node.yaml:252
kerberos-diris a file directory hostPath and does not needmountPropagation: Bidirectional; keeping it bidirectional expands host-mount influence beyond what's required for kerberos cache files.
- mountPath: /var/lib/kubelet/kerberos
mountPropagation: Bidirectional
name: kerberos-dir
deploy/csi-azurefile-node.yaml:170
kerberos-diris a plain directory hostPath;mountPropagation: Bidirectionalis unnecessary here and increases the pod's ability to affect host mounts. Consider dropping mountPropagation for this mount.
- mountPath: /var/lib/kubelet/kerberos
mountPropagation: Bidirectional
name: kerberos-dir
deploy/csi-azurefile-node.yaml:200
kerberos-diris a plain directory hostPath;mountPropagation: Bidirectionalis unnecessary here and increases the pod's ability to affect host mounts. Consider dropping mountPropagation for this mount.
- mountPath: /var/lib/kubelet/kerberos
mountPropagation: Bidirectional
name: kerberos-dir
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: mountpoint-dir | ||
| mountPath: "{{ .Values.linux.kubelet }}" | ||
| mountPropagation: Bidirectional |
| - hostPath: | ||
| path: /var/lib/kubelet/kerberos | ||
| type: DirectoryOrCreate | ||
| name: kerberos-dir |
andyzhangx
left a comment
There was a problem hiding this comment.
Thanks for the PR! The direction is right — restricting mountpoint-dir to /var/lib/kubelet/pods and separating kerberos paths is a good security hardening move. A few issues to address before this can merge:
-
Helm template: mountPath vs hostPath mismatch for azurefile container
Incsi-azurefile-node.yaml(Helm), theazurefilecontainer still mountsmountpoint-dirat{{ .Values.linux.kubelet }}(line ~109), but the volume's hostPath was changed to{{ .Values.linux.kubelet }}/pods. This asymmetry means the container sees/var/lib/kubeletinternally but it's actually backed by/var/lib/kubelet/podson the host. The mountPath should also be updated to{{ .Values.linux.kubelet }}/podsfor consistency. -
kerberos-dir hostPath is hardcoded
The newkerberos-dirvolume uses a hardcoded/var/lib/kubelet/kerberospath. This doesn't respect the{{ .Values.linux.kubelet }}variable. If a user has a custom kubelet root (e.g./data/kubelet), this will point to the wrong location. Please use{{ .Values.linux.kubelet }}/kerberosin the Helm template (and the corresponding variable in the static manifest if applicable). -
azfilesrefresh container lost access to pods mount
Previouslyazfilesrefreshmounted the entire kubelet dir. Now it only haskerberos-dir. Please confirm that the refresh logic doesn't need to traverse pod volume mounts (some Kerberos ticket refresh implementations do). If it doesn't, this is fine — just want to make sure. -
Missing tests / upgrade validation
Since this changes mount paths on the node DaemonSet, it would be good to at least runhelm templaterender tests to confirm the output is correct. An upgrade from the previous version should also be validated (pods with existing mounts).
/kind cleanup
|
@andyzhangx , following up on your comment, i had addressed the comments and pushed the changes , also confirmed that the refresh logic doesn't need to traverse pod volume mounts. i had also validated the code changes working fine by first installing the driver from the master branch chart and brought up two pods — a normal account-key pod and a managed-identity pod. The MI pod mounted the Azure file share over SMB with Kerberos ( I then upgraded the driver to my branch's chart ( |
|
/ok-to-test |
| - ALL | ||
| volumeMounts: | ||
| - mountPath: /var/lib/kubelet/ | ||
| - mountPath: /var/lib/kubelet/kerberos |
There was a problem hiding this comment.
the globalmount is under /var/lib/kubelet/plugins/kubernetes.io/csi/<driver>/<hash>/globalmount, we should keep that mount path, thus I would recommend keep /var/lib/kubelet/
your original test is not sufficient since node reboot just make remount happen again, does not test the refresh token functionality.
There was a problem hiding this comment.
@andyzhangx, I addressed your comment and added a dedicated plugins-dir hostPath for /var/lib/kubelet/plugins (Bidirectional), keeping mountpoint-dir scoped to /var/lib/kubelet/pods. This preserves globalmount without exposing all of /var/lib/kubelet/.
Also to test the refresh token functionality, I used workload identity, which rotates the projected token roughly every hour. I first installed the driver from the main-branch helm chart, created the WI pod, and had it append the timestamp + current token to a file on the mounted share every minute. After confirming the token had refreshed once, I upgraded the driver to my branch's helm chart. The pod kept running fine across the upgrade with no disruption to the refresh flow — the token refreshed again ~1h after the previous refresh, and once more an hour later. So the refresh-token functionality works both before and after the upgrade, independent of any remount.
`# before upgrade (driver from main)
09:12:19Z iat=09:11:33 exp=10:11:33 jti=643bf1d2…
10:00:29Z iat=09:59:37 exp=10:59:37 jti=b73d009c…
--- driver upgraded to v0.0.1 (my branch) @ 10:40 ---
after upgrade (my branch)
10:48:38Z iat=10:47:43 exp=11:47:43 jti=555e1530… (refresh #1)
11:36:48Z iat=11:36:29 exp=12:36:29 jti=660cba17… (refresh #2)`
There was a problem hiding this comment.
The workload identity token refresh test looks good, but I think there's a separate issue with the azfilesrefresh container for Managed Identity (MI) Kerberos auth specifically.
After this PR, azfilesrefresh only mounts kerberos-dir (/var/lib/kubelet/kerberos) — it no longer has mountpoint-dir. The problem is that azfilesrefresh discovers which CIFS mounts need ticket refresh by running mount -t cifs inside its own mount namespace (see AzFilesAuthenticator get_mount_options()):
def get_mount_options():
result = subprocess.run(['mount', '-t', 'cifs'], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
mount_output = result.stdout.decode('utf-8')
# ... filters for sec=krb5 mountsWithout mountpoint-dir (which had mountPropagation: Bidirectional), CIFS mounts created by kubelet under /var/lib/kubelet/pods/<uid>/volumes/... are no longer propagated into the azfilesrefresh container's mount namespace. So mount -t cifs returns empty → no tickets get refreshed → Kerberos tickets expire → MI-authed Azure Files mounts break.
The WI token refresh test above works because WI uses a different code path (projected service account tokens, not Kerberos). MI Kerberos auth is the affected path.
Fix: add mountpoint-dir back to azfilesrefresh:
- name: azfilesrefresh
volumeMounts:
- mountPath: /var/lib/kubelet/pods
mountPropagation: Bidirectional
name: mountpoint-dir # ← need this for mount -t cifs visibility
- mountPath: /var/lib/kubelet/kerberos
mountPropagation: Bidirectional
name: kerberos-dir
- name: azfilesauth
mountPath: /etc/azfilesauth
- name: log-dir
mountPath: /var/log/There was a problem hiding this comment.
also the token refresh could happen within one hour while the token expire may take ~1 day, so even it's not refreshed within a few hours, the existing cifs mount would be still valid
There was a problem hiding this comment.
added the fix. Fix: add mountpoint-dir back to azfilesrefresh:
There was a problem hiding this comment.
I think it's better have plugins-dir mounted in the azfilesrefresh container
- mountPath: /var/lib/kubelet/plugins
mountPropagation: Bidirectional
name: plugins-dir
| - mountPath: /csi | ||
| name: socket-dir | ||
| - mountPath: /var/lib/kubelet/ | ||
| - mountPath: /var/lib/kubelet/pods |
There was a problem hiding this comment.
the globalmount is under /var/lib/kubelet/plugins/kubernetes.io/csi///globalmount, we should keep that mount path, thus I would recommend keep /var/lib/kubelet/
# Conflicts: # charts/latest/azurefile-csi-driver-v0.0.0.tgz
|
/retest |
What type of PR is this?
What this PR does / why we need it:
Restrict Azure File CSI driver mountpoint-dir to /var/lib/kubelet/pods
added a dedicated Kerberos path for managed identity authentication.
What changed:
Which issue(s) this PR fixes:
Fixes #
Updated mountpoint-dir hostPath and mountPath from /var/lib/kubelet/ to /var/lib/kubelet/pods
Uses single kubelet variable with /pods suffix appended directly in templates
Requirements:
Special notes for your reviewer:
Release note: