Skip to content
This repository has been archived by the owner on Sep 26, 2021. It is now read-only.

First attempt at docker-machine share #793

Closed
wants to merge 4 commits into from

Conversation

nathanleclaire
Copy link
Contributor

cc @bfirsh @ehazlett @sthulb @SvenDowideit @tianon

Hi, this is nowhere near complete, but I wanted to show what I have done so far so we can talk about it, explain the motivations, and hopefully then we can move forward with a clean and clear design.

Motivation

This has been discussed ad nauseum, so I won't rehash too much, but rather summarize quickly.

The only currently supported method for sharing folders between machine host and created machines is to auto-mount /Users as a VirtualBox shared folder in the boot2docker VM. This has some issues:

  1. Performance. NFS looks really tempting compared to vboxsf. See Mitchell's article, anecdata, etc.
  2. Security. One of the biggest advantages of running Docker in a VM is that it de-fangs random images you pull and run over the Internet. It doesn't completely mitigate the risk, but it certainly does help. Right now users are just one mis-copied or mis-typed command away from having their account credentials, private SSH keys, and more stolen by malicious images. Limiting the shares to very specific sub-paths on the host system will help with this. I know that the party line with the current method is "the risk is the same as running Docker natively", it doesn't persuade me that we shouldn't try to do better if we can.
  3. Flexibility. The current method works with boot2docker because of the init script baked into the ISO, but no such guarantees are made if the VM was just a vanilla Ubuntu image. There's no easy obvious way to share between your host and a cloud provider. Likewise, there is no common interface to use other options such as SSHFS, SFTP, NFS etc. Users aren't waiting for us on this and are concocting their own homebrew solutions. Debugging things will be hard if there's no officially supported way, here we at least will have logs and information available in inspect.
  4. UID / GID mapping. Right now if the users are mounting volumes into the containers which use non-root users, they are kind of hosed in terms of actually being able to flexibly read and write to/from those files unless they want to roll their own (share) mounting layer. A common interface could help us to cover some of those cases for users so they can specify UID mappings much more conveniently if need be.
  5. Reuse. Goal should be to prevent projects like Kitematic or Rancher from having to implement their own custom logic to do this kind of thing if they want to expose such functionality to users.

This is a continuation of the before-mentioned docker-machine share proposal: #179.

Implementation

So far, just to get something together, I've just done VBox shared folders, and need some help on the design for making this a more generic interface.

Example usage with this PR:

$ ./docker-machine_darwin-amd64 -D create -d virtualbox share-demo
... Machine create output ...

$ ./docker-machine_darwin-amd64 share --with share-demo --driver vboxsf .
INFO[0005] Waiting for VM to start...
INFO[0037] Share created successfully at /Users/nathanleclaire/go/src/github.com/docker/machine

$ ./docker-machine_darwin-amd64 inspect share-demo
{
    "DriverName": "virtualbox",
    "Driver": {
        "MachineName": "share-demo",
        "SSHUser": "docker",
        "SSHPort": 61063,
        "Memory": 1024,
        "DiskSize": 20000,
        "Boot2DockerURL": "",
        "CaCertPath": "/Users/nathanleclaire/.docker/machine/certs/ca.pem",
        "PrivateKeyPath": "/Users/nathanleclaire/.docker/machine/certs/ca-key.pem",
        "Shares": [
            {
                "Name": "cbdad6227294b5ae09248b88ecf70a392c56edacc95bfcd68b189eb3281410ca",
                "Path": "/Users/nathanleclaire/go/src/github.com/docker/machine",
                "Uid": 1000,
                "Gid": 50
            }
        ],
        "SwarmMaster": false,
        "SwarmHost": "tcp://0.0.0.0:3376",
        "SwarmDiscovery": ""
    },
    "CaCertPath": "/Users/nathanleclaire/.docker/machine/certs/ca.pem",
    "ServerCertPath": "",
    "ServerKeyPath": "",
    "PrivateKeyPath": "/Users/nathanleclaire/.docker/machine/certs/ca-key.pem",
    "ClientCertPath": "",
    "SwarmMaster": false,
    "SwarmHost": "tcp://0.0.0.0:3376",
    "SwarmDiscovery": ""
}

$ # No more homedir secrets in the b2d VM!
$ ./docker-machine_darwin-amd64 ssh share-demo -- cat /Users/$(whoami)/.ssh/id_rsa
cat: can't open '/Users/nathanleclaire/.ssh/id_rsa': No such file or directory
FATA[0000] exit status 1

$ ./docker-machine_darwin-amd64 ssh share-demo -- ls /Users/$(whoami)/go/src/github.com/docker/machine
CHANGES.md
CONTRIBUTING.md
Dockerfile
Godeps
LICENSE
MAINTAINERS
Makefile
README.md
ROADMAP.md
commands.go
commands_test.go
creds
docker-machine_darwin-amd64
docs
drivers
host.go
host_test.go
ignore
imports_windows.go
integration-tests
log.go
main.go
provider
script
share
ssh
state
store.go
store_test.go
test
utils
version.go

The code is kind of fun; essentially, in cmdShare I check to see if the driver fulfills the DriverWithShares interface. If it doesn't (every driver except VBox), I error out, but if it does, we go ahead and create the share. This requires us to do things like stopping and starting the machine, which is why in the proposed interface in the next section you pass in the Driver to some of the commands.

Future

Instead of having things sort-of-hardcoded like I do now, I think that where I'd like to move is to have a type Share interface which provides all the methods needed to create and maintain the share throughout a machine's lifecycle. Then there would be type VboxsfShare struct, type NfsShare struct, etc. which would all be managed through common code.

E.g.:

type DriverWithShares interface {
    GetShares() ([]*Share, error)
}

type ShareOptions struct {
    Name, SrcPath, DestPath string
    SrcUid, SrcGid, DestUid, DestGid int
}

type Share interface {
    ContractFulfilled(d Driver) bool
    Create(d Driver, options ShareOptions) error
    Name() string
    Mount(d Driver)
    Destroy(d Driver) error
}

ContractFulfilled would check to see if the share can actually be created / used. "Are the Guest Additions installed? Is NFS installed?" etc.

Host's Start method would be extended slightly to support doing the mounts at start time:

func (h *Host) Start() error {
    if err := h.Driver.Start(); err != nil {
        return err
    }
    // wait for SSH
    if shareDriver, ok := h.Driver.(DriverWithShares); ok {
        shares, err := shareDriver.GetShares()
        if err != nil {
            return err
        }
        for _, share := range shares {
            if err := share.Mount(shareDriver); err != nil {
                log.Warn("Error mounting a share (%s): %s", share.Name(), err)
            }
        }
    }
    return nil
}

This isn't super elegant (the problem is really hairy all around), but it'd be a start.

Instead of trying to jump into supporting shares on all drivers all at once, we could do them piecemeal by slowly implementing the DriverWithShares interface on a per-driver basis. That way, we could start with getting the basic idea right for local drivers (easier) and expand the reach gradually. Hopefully this can help with some of the concern over bringing them to remote providers as well: if it turns out that we're happy with just the local versions, then no need to implement the interface for the remaining drivers.

I really want to enable a mechanism whereby this kind of thing can be done:

$ docker-machine share --with foomachine --driver vboxsf --uidmap $(whoami):apache .

To relieve the pain of users who cannot use our current method because their container runs as a non-root user (which, it should be noted, is a security best practice that we should be promoting heavily).

All this type of thing could be added to the proposed declarative syntax as well.

Clarifications

  • I've waffled before on whether I want this in docker-machine or in a separate tool, I think that unless proven otherwise having another tool would just be too much overhead and for convenience it should just all go in one tool (machine) for now.
  • I am in favor of a separate docker-machine scp command which would handle both scp and rsync to machines under management separately from these kinds of "two-way" shares.
  • I just assume that path on the host will be the same as the path in the machine, which is good for "transparent" use of -v, but that won't necessarily
  • I might put together a demo NFS and/or SSHFS share driver if there's interest.

@sthulb
Copy link
Contributor

sthulb commented Mar 17, 2015

Man, I wish we had plugins, but the outline is good.

@sthulb
Copy link
Contributor

sthulb commented Mar 17, 2015

Going through the code, I'm wary of adding things to the driver. Other than for hypervisor shares, there's no real reason for driver additions. I wonder if there's a better way.

@nathanleclaire nathanleclaire force-pushed the share_attempt branch 2 times, most recently from 941de2c to c9d889c Compare March 18, 2015 02:21
@nathanleclaire
Copy link
Contributor Author

OK I've updated it a bit from the original implementation - now no new methods have been added to the drivers to implement this. It's a bit better in this form and pretty much completely ready for other share drivers to be implemented, so I might take a crack at an NFS share.

func NewShare(shareType, absPath string) (Share, error) {
switch shareType {
case "vboxsf":
return VBoxSharedFolder{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should just use NewShareWithOptions here

@ehazlett
Copy link
Contributor

I like this idea. However, right now I want to focus on getting the provisioner and related (cloudinit, etc). We will need to heavily review and vet as this affects core functionality and workflow from all Docker users using this for local dev. We will probably want to involve the kitematic team as they could provide valuable feedback from a local dev and requirement standpoint.

/cc @JeffDM

@nathanleclaire
Copy link
Contributor Author

However, right now I want to focus on getting the provisioner and related (cloudinit, etc).

I agree that's more important, I want to focus some time on getting that stuff in good shape first before making a big push to get this merged.

We will need to heavily review and vet as this affects core functionality and workflow from all Docker users using this for local dev. We will probably want to involve the kitematic team as they could provide valuable feedback from a local dev and requirement standpoint.

+1

@sthulb
Copy link
Contributor

sthulb commented Mar 18, 2015

Absolutely, I'd love to get my PRs merged to work on new things

Signed-off-by: Nathan LeClaire <nathan.leclaire@gmail.com>
It doesn't really work yet.  Beware

Signed-off-by: Nathan LeClaire <nathan.leclaire@gmail.com>
@apfritts
Copy link

Howdy! I would really like this functionality. Is there anything I can do to help it move forward? Looking at the comments, it mainly looks like its blocked on coordinating with other teams and generating the right documentation to inform folks of the change?

Signed-off-by: Nathan LeClaire <nathan.leclaire@gmail.com>
@blazarecki
Copy link

Big 👍 I'd love to use NFS with docker-machine !

@nathanleclaire
Copy link
Contributor Author

I should add the BIG BOLD CAVEAT for anyone considering trying this code out that the NFS driver doesn't really work yet and it's "use at your own risk" (I almost had some issues with data loss playing around with it), however if anyone is brave enough to consider helping I'm available on IRC: nathanleclaire

(I do intend to pick it up again at some point)

Signed-off-by: Nathan LeClaire <nathan.leclaire@gmail.com>
@SorraTheOrc
Copy link

This is addressing the very problem I'm struggling with today. Adding @ahmetalpbalkan to see if he has any thoughts from a Hyper-V and Azure perspective.

@SvenDowideit
Copy link
Contributor

for docker 1.7 experimental, there's also https://github.com/SvenDowideit/docker-volumes-nfs

@nathanleclaire
Copy link
Contributor Author

We need to really think carefully about how/when this might be implemented, so I'm closing this for now.

tomeon pushed a commit to tomeon/machine that referenced this pull request May 9, 2018
tomeon pushed a commit to tomeon/machine that referenced this pull request May 9, 2018
wait for disks to actually exist, closes docker#793
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants