-
Notifications
You must be signed in to change notification settings - Fork 5k
Don't require both --mount for using --mount-string and remove default mount-string #21250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
/ok-to-test |
I agree, we should not mount the home folder to ppl's minikubes by default that was a horrible default of some drivers. I am okay with breaking change if we can get rid of it by default |
This comment has been minimized.
This comment has been minimized.
Changes in latest version:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
/retest |
/cc @afbjorklund |
/kind improvement |
/kind api-change |
/remove-kind api-change |
@@ -173,8 +173,8 @@ func initMinikubeFlags() { | |||
startCmd.Flags().Bool(keepContext, false, "This will keep the existing kubectl context and will create a minikube context.") | |||
startCmd.Flags().Bool(embedCerts, false, "if true, will embed the certs in kubeconfig.") | |||
startCmd.Flags().StringP(containerRuntime, "c", constants.DefaultContainerRuntime, fmt.Sprintf("The container runtime to be used. Valid options: %s (default: auto)", strings.Join(cruntime.ValidRuntimes(), ", "))) | |||
startCmd.Flags().Bool(createMount, false, "This will start the mount daemon and automatically mount files into minikube.") | |||
startCmd.Flags().String(mountString, constants.DefaultMountDir+":/minikube-host", "The argument to pass the minikube mount command on start.") | |||
startCmd.Flags().Bool(createMount, false, "Kept for backward compatibility, value is ignored.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can consider hiding this flag (we have a few flags that hidden since we dont want ppl to use them, but left for backward campatiblitiy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hiding the flag will hide the useful help message. I think we should keep if for now. If we don't use it in the next releases we can hide it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: does this mean we need to update this site page about "built-in host folder sharing"?
Driver mounts
Some hypervisors, have built-in host folder sharing. Driver mounts are reliable with good performance, but the paths are not predictable across operating systems or hypervis
This change does not effect built in mounts - these are not done using 9p so they should not be affected by ignoring the --mount flag. These should be disable by --disable-driver-mounts which we did not touch. So I don't think any update is needed, the only change is the detail of using --mount flag which is not documented in the handbook. |
This comment has been minimized.
This comment has been minimized.
The --mount-string argument defaults to `/Users` on darwin, and homedir.Homedir() on other platforms (e.g. $HOME on unix). This is wrong in many ways: - `/Users` is not HOME on darwin (the right path is `/Users/$USER`). Using the default mount we cannot access anything inside the guest in the user home directory. We can access the special `/Users/Shared` directory, but this should not be a default mount. - Mounting the user home directory inside the guest in read-write mode is a horrible default. This exposes the users private keys in .ssh/ to the guest, any sensitive files in the user home directory, and allows the guest to change any file on the host. - Using the `--mount` option mount the default mount directory silently. This is unexpected, surprising, and not documented in the minikube handbook[1]. Example access to user private key from the guest with the default mount: $ minikube start --mount $ minikube ssh cat /minikube-host/.ssh/id_ed25519 -----BEGIN OPENSSH PRIVATE KEY----- ... -----END OPENSSH PRIVATE KEY----- Fixed by removing the default mount directory and changing mount logic to check for non-empty mount-string instead of the mount flag. The mount flag is kept for backward compatibility, but its value is ignored. In the next release we want to use this flag for supporting multiple mounts. Example usage before: minikube start --mount --mount-string ~/models:/mnt/models Example usage after: minikube start --mount-string ~/models:/mnt/models Breaking changes: User depending the default mount will have to replace the command: minikube start --mount With: minikube start --mount-string $HOME:/minikube-host [1] https://minikube.sigs.k8s.io/docs/handbook/mount/
637b1d8
to
af7362d
Compare
Rebased to consume the docker service fix |
This comment has been minimized.
This comment has been minimized.
/retest-this-please |
kvm2 driver with docker runtime
Times for minikube start: 47.8s 48.8s 46.8s 45.9s 49.3s Times for minikube ingress: 14.9s 14.9s 14.9s 14.4s 14.9s docker driver with docker runtime
Times for minikube start: 22.6s 23.4s 24.6s 23.5s 21.5s Times for minikube (PR 21250) ingress: 12.3s 13.2s 12.7s 11.2s 13.2s docker driver with containerd runtime
Times for minikube start: 24.6s 23.4s 22.8s 22.5s 21.5s Times for minikube ingress: 23.2s 23.7s 23.7s 23.2s 22.7s |
/retest |
@@ -173,8 +173,8 @@ func initMinikubeFlags() { | |||
startCmd.Flags().Bool(keepContext, false, "This will keep the existing kubectl context and will create a minikube context.") | |||
startCmd.Flags().Bool(embedCerts, false, "if true, will embed the certs in kubeconfig.") | |||
startCmd.Flags().StringP(containerRuntime, "c", constants.DefaultContainerRuntime, fmt.Sprintf("The container runtime to be used. Valid options: %s (default: auto)", strings.Join(cruntime.ValidRuntimes(), ", "))) | |||
startCmd.Flags().Bool(createMount, false, "This will start the mount daemon and automatically mount files into minikube.") | |||
startCmd.Flags().String(mountString, constants.DefaultMountDir+":/minikube-host", "The argument to pass the minikube mount command on start.") | |||
startCmd.Flags().Bool(createMount, false, "Kept for backward compatibility, value is ignored.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
help text, this will be depricated and merged with --mount-string in the next version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #21291, I'll try to get this into 1.37.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: medyagh, nirs 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 |
The --mount-string argument defaults to `/Users` on darwin, and homedir.Homedir() on other platforms (e.g. $HOME on unix). This is wrong in many ways: - `/Users` is not HOME on darwin (the right path is `/Users/$USER`). Using the default mount we cannot access anything inside the guest in the user home directory. We can access the special `/Users/Shared` directory, but this should not be a default mount. - Mounting the user home directory inside the guest in read-write mode is a horrible default. This exposes the users private keys in .ssh/ to the guest, any sensitive files in the user home directory, and allows the guest to change any file on the host. - Using the `--mount` option mount the default mount directory silently. This is unexpected, surprising, and not documented in the minikube handbook[1]. Example access to user private key from the guest with the default mount: $ minikube start --mount $ minikube ssh cat /minikube-host/.ssh/id_ed25519 -----BEGIN OPENSSH PRIVATE KEY----- ... -----END OPENSSH PRIVATE KEY----- Fixed by removing the default mount directory and changing mount logic to check for non-empty mount-string instead of the mount flag. The mount flag is kept for backward compatibility, but its value is ignored. In the next release we want to use this flag for supporting multiple mounts. Example usage before: minikube start --mount --mount-string ~/models:/mnt/models Example usage after: minikube start --mount-string ~/models:/mnt/models Breaking changes: User depending the default mount will have to replace the command: minikube start --mount With: minikube start --mount-string $HOME:/minikube-host [1] https://minikube.sigs.k8s.io/docs/handbook/mount/
The
--mount-string
argument defaults to/Users
on darwin, and homedir.Homedir() on other platforms (e.g.$HOME
on unix).This is wrong in many ways:
/Users
is not$HOME
on darwin (the right path is/Users/$USER
). Using the default mount we cannot access anything inside the guest in the user home directory. We can access the special/Users/Shared
directory, but this should not be a default mount.Mounting the user home directory inside the guest in read-write mode is a horrible default. This exposes the users private keys in .ssh/ to the guest, any sensitive files in the user home directory, and allows the guest to change any file on the host.
Using the
--mount
option mounts the default mount directory silently. This is unexpected, surprising, and not documented in the minikube handbook[1].Example access to user private key from the guest with the default mount:
Fixed by removing the default mount directory and changing mount logic to check for non-empty mount-string instead of the mount flag.
The mount flag is kept for backward compatibility, but its value is ignored. In the next release we want to use this flag for supporting multiple mounts.
Example usage before:
Example usage after:
Breaking changes:
User depending the default mount will have to replace the command:
With:
[1] https://minikube.sigs.k8s.io/docs/handbook/mount/