Fix windows MountSensitive error #148
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @mboersma! |
k8s-ci-robot
added
the
size/XS
k8s-ci-robot
added
the
lgtm
|
@gnufied coud you approve this? Thanks. |
k8s-ci-robot
added
the
do-not-merge/hold
mount/mount_windows.go
Outdated
| @@ -99,13 +99,13 @@ func (mounter *Mounter) MountSensitive(source string, target string, fstype stri | |||
| getSMBMountMutex.LockKey(source) | |||
| defer getSMBMountMutex.UnlockKey(source) | |||
|
|
|||
| if output, err := newSMBMapping(options[0], options[1], source); err != nil { | |||
| if output, err := newSMBMapping(sensitiveOptions[0], sensitiveOptions[1], source); err != nil { | |||
There was a problem hiding this comment.
Nothing in the documentation indicates the order or distribution between options/sensitiveOptions is important... assuming a particular option will be in one or the other is fairly fragile. This was raised in #138 (comment)
There was a problem hiding this comment.
given the order and distribution of options is important here, I would expect something more like this:
allOptions := []string{}
allOptions = append(allOptions, options...)
allOptions = append(allOptions, sensitiveOptions...)
if len(allOptions) < 2 {
...
}
...
if output, err := newSMBMapping(allOptions[0], allOptions[1], source); err != nil {
...
}and to document that sensitiveOptions are always placed after options
There was a problem hiding this comment.
(and don't log the content of allOptions anywhere, since it can contain sensitive options)
There was a problem hiding this comment.
Would it be better to have a map and search for specific option keys rather than assuming the options are passed in a specific order?
There was a problem hiding this comment.
iiuc, these options don't have identifying keys
There was a problem hiding this comment.
they appear to be passed in the form "user=abc", "password=abc", so we could at least try to parse and search for the correct option?
|
was there a presubmit job that could have been run against kubernetes/kubernetes#88684 to catch this before merge? |
|
/kind bug |
k8s-ci-robot
added
kind/bug
|
Thanks for the comments on this hasty PR. I'll make changes and re-test locally. We think reverting just the azure_file.go changes in kubernetes/kubernetes#88684 would be a safer approach, and buy more time to fix this cleanly and to add unit tests. |
k8s-ci-robot
added
size/S
|
/lgtm |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
go ahead and squash to a single commit |
mboersma
force-pushed
the
fix-windows-mount-sensitive
branch
from
cad438f
to
9162b39
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@kkmsft: GitHub didn't allow me to request PR reviews from the following users: other, in, list, -, the. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. |
|
/assign |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx, kkmsft, liggitt, mboersma, saad-ali The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
After kubernetes/kubernetes#88684 was included in 1.18.0-rc.1, Windows volume mounts began failing in our AKS Engine e2e tests. It appears that the code path for Windows
MountSensitiveshould have been looking at thesensitiveOptionsinstead ofoptions. We seeoptionsempty in every call.We've tested this interactively and found it to resolve Windows mounting problems.
cc: @kkmsft @marosset