Remove RawDriver from persistence on disk

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

libmachine/host/host.go

@@ -30,7 +30,7 @@ type Host struct {
 	DriverName    string
 	HostOptions   *Options
 	Name          string
-	RawDriver     []byte
+	RawDriver     []byte `json:"-"`
 }
 
 type Options struct {

libmachine/host/migrate.go

@@ -11,14 +11,22 @@ import (
 	"github.com/docker/machine/libmachine/version"
 )
 
+var (
+	errConfigFromFuture = errors.New("Config version is from the future, please upgrade your Docker Machine client.")
+)
+
 type RawDataDriver struct {
 	*none.Driver
-	data []byte // passed directly back when invoking json.Marshal on this type
+	Data []byte // passed directly back when invoking json.Marshal on this type
 }
 
 func (r *RawDataDriver) MarshalJSON() ([]byte, error) {
-	// now marshal it back up
+	return r.Data, nil
-	return r.data, nil
+}
+
+func (r *RawDataDriver) UnmarshalJSON(data []byte) error {
+	r.Data = data
+	return nil
 }
 
 func getMigratedHostMetadata(data []byte) (*Metadata, error) {
@@ -51,10 +59,10 @@ func MigrateHost(h *Host, data []byte) (*Host, bool, error) {
 
 	globalStorePath := filepath.Dir(filepath.Dir(migratedHostMetadata.HostOptions.AuthOptions.StorePath))
 
-	driver := none.NewDriver(h.Name, globalStorePath)
+	driver := &RawDataDriver{none.NewDriver(h.Name, globalStorePath), nil}
 
 	if migratedHostMetadata.ConfigVersion > version.ConfigVersion {
-		return nil, false, errors.New("Config version is from the future, please upgrade your Docker Machine client.")
+		return nil, false, errConfigFromFuture
 	}
 
 	if migratedHostMetadata.ConfigVersion == version.ConfigVersion {
@@ -62,24 +70,6 @@ func MigrateHost(h *Host, data []byte) (*Host, bool, error) {
 		if err := json.Unmarshal(data, &h); err != nil {
 			return nil, migrationPerformed, fmt.Errorf("Error unmarshalling most recent host version: %s", err)
 		}
-
-		// We are config version 3, so we definitely should have a
-		// RawDriver field.  However, it's possible some might use
-		// older clients after already migrating, so check if it exists
-		// and create one if not.  The following code is an (admittedly
-		// fragile) attempt to account for the fact that the above code
-		// to forbid loading from future clients was not introduced
-		// sooner.
-		if h.RawDriver == nil {
-			log.Warn("It looks like you have used an older Docker Machine binary to interact with hosts after using a 0.5.0 binary.")
-			log.Warn("Please be advised that doing so can result in erratic behavior due to migrated configuration settings.")
-			log.Warn("Machine will attempt to re-migrate the configuration settings, but safety is not guaranteed.")
-			migrationNeeded = true
-
-			// Treat the data as config version 1, even though it
-			// says "latest".
-			migratedHostMetadata.ConfigVersion = 1
-		}
 	} else {
 		migrationNeeded = true
 	}
@@ -117,11 +107,14 @@ func MigrateHost(h *Host, data []byte) (*Host, bool, error) {
 					}
 				}
 				h = MigrateHostV2ToHostV3(hostV2, data, globalStorePath)
-				h.Driver = RawDataDriver{driver, nil}
+				driver.Data = h.RawDriver
+				h.Driver = driver
 			case 3:
 			}
 		}
 	}
 
+	h.RawDriver = driver.Data
+
 	return h, migrationPerformed, nil
 }

libmachine/host/migrate_test.go

@@ -0,0 +1,180 @@
+package host
+
+import (
+	"testing"
+
+	"github.com/docker/machine/drivers/none"
+	"github.com/docker/machine/libmachine/auth"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestMigrateHost(t *testing.T) {
+	testCases := []struct {
+		description                string
+		hostBefore                 *Host
+		rawData                    []byte
+		expectedHostAfter          *Host
+		expectedMigrationPerformed bool
+		expectedMigrationError     error
+	}{
+		{
+			// Point of this test is largely that no matter what was in RawDriver
+			// before, it shoud load into the Host struct based on what is actually
+			// in the Driver field.
+			//
+			// Note that we don't check for the presence of RawDriver's literal "on
+			// disk" here.  It's intentional.
+			description: "Config version 3 load with existing RawDriver on disk",
+			hostBefore: &Host{
+				Name: "default",
+			},
+			rawData: []byte(`{
+    "ConfigVersion": 3,
+    "Driver": {"MachineName": "default"},
+    "DriverName": "virtualbox",
+    "HostOptions": {
+        "Driver": "",
+        "Memory": 0,
+        "Disk": 0,
+        "AuthOptions": {
+            "StorePath": "/Users/nathanleclaire/.docker/machine/machines/default"
+        }
+    },
+    "Name": "default",
+    "RawDriver": "eyJWQm94TWFuYWdlciI6e30sIklQQWRkcmVzcyI6IjE5Mi4xNjguOTkuMTAwIiwiTWFjaGluZU5hbWUiOiJkZWZhdWx0IiwiU1NIVXNlciI6ImRvY2tlciIsIlNTSFBvcnQiOjU4MTQ1LCJTU0hLZXlQYXRoIjoiL1VzZXJzL25hdGhhbmxlY2xhaXJlLy5kb2NrZXIvbWFjaGluZS9tYWNoaW5lcy9kZWZhdWx0L2lkX3JzYSIsIlN0b3JlUGF0aCI6Ii9Vc2Vycy9uYXRoYW5sZWNsYWlyZS8uZG9ja2VyL21hY2hpbmUiLCJTd2FybU1hc3RlciI6ZmFsc2UsIlN3YXJtSG9zdCI6InRjcDovLzAuMC4wLjA6MzM3NiIsIlN3YXJtRGlzY292ZXJ5IjoiIiwiQ1BVIjoxLCJNZW1vcnkiOjEwMjQsIkRpc2tTaXplIjoyMDAwMCwiQm9vdDJEb2NrZXJVUkwiOiIiLCJCb290MkRvY2tlckltcG9ydFZNIjoiIiwiSG9zdE9ubHlDSURSIjoiMTkyLjE2OC45OS4xLzI0IiwiSG9zdE9ubHlOaWNUeXBlIjoiODI1NDBFTSIsIkhvc3RPbmx5UHJvbWlzY01vZGUiOiJkZW55IiwiTm9TaGFyZSI6ZmFsc2V9"
+}`),
+			expectedHostAfter: &Host{
+				ConfigVersion: 3,
+				HostOptions: &Options{
+					AuthOptions: &auth.Options{
+						StorePath: "/Users/nathanleclaire/.docker/machine/machines/default",
+					},
+				},
+				Name:       "default",
+				DriverName: "virtualbox",
+				RawDriver:  []byte(`{"MachineName": "default"}`),
+				Driver: &RawDataDriver{
+					Data: []byte(`{"MachineName": "default"}`),
+
+					// TODO (nathanleclaire): The "." argument here is a already existing
+					// bug (or at least likely to cause them in the future) and most
+					// likely should be "/Users/nathanleclaire/.docker/machine"
+					//
+					// These default StorePath settings get over-written when we
+					// instantiate the plugin driver, but this seems entirely incidental.
+					Driver: none.NewDriver("default", "."),
+				},
+			},
+			expectedMigrationPerformed: false,
+			expectedMigrationError:     nil,
+		},
+		{
+			description: "Config version 4 (from the FUTURE) on disk",
+			hostBefore: &Host{
+				Name: "default",
+			},
+			rawData: []byte(`{
+    "ConfigVersion": 4,
+    "Driver": {"MachineName": "default"},
+    "DriverName": "virtualbox",
+    "HostOptions": {
+        "Driver": "",
+        "Memory": 0,
+        "Disk": 0,
+        "AuthOptions": {
+            "StorePath": "/Users/nathanleclaire/.docker/machine/machines/default"
+        }
+    },
+    "Name": "default"
+}`),
+			expectedHostAfter:          nil,
+			expectedMigrationPerformed: false,
+			expectedMigrationError:     errConfigFromFuture,
+		},
+		{
+			description: "Config version 3 load WITHOUT any existing RawDriver field on disk",
+			hostBefore: &Host{
+				Name: "default",
+			},
+			rawData: []byte(`{
+    "ConfigVersion": 3,
+    "Driver": {"MachineName": "default"},
+    "DriverName": "virtualbox",
+    "HostOptions": {
+        "Driver": "",
+        "Memory": 0,
+        "Disk": 0,
+        "AuthOptions": {
+            "StorePath": "/Users/nathanleclaire/.docker/machine/machines/default"
+        }
+    },
+    "Name": "default"
+}`),
+			expectedHostAfter: &Host{
+				ConfigVersion: 3,
+				HostOptions: &Options{
+					AuthOptions: &auth.Options{
+						StorePath: "/Users/nathanleclaire/.docker/machine/machines/default",
+					},
+				},
+				Name:       "default",
+				DriverName: "virtualbox",
+				RawDriver:  []byte(`{"MachineName": "default"}`),
+				Driver: &RawDataDriver{
+					Data: []byte(`{"MachineName": "default"}`),
+
+					// TODO: See note above.
+					Driver: none.NewDriver("default", "."),
+				},
+			},
+			expectedMigrationPerformed: false,
+			expectedMigrationError:     nil,
+		},
+		{
+			description: "Config version 2 load and migrate.  Ensure StorePath gets set properly.",
+			hostBefore: &Host{
+				Name: "default",
+			},
+			rawData: []byte(`{
+    "ConfigVersion": 2,
+    "Driver": {"MachineName": "default"},
+    "DriverName": "virtualbox",
+    "HostOptions": {
+        "Driver": "",
+        "Memory": 0,
+        "Disk": 0,
+        "AuthOptions": {
+            "StorePath": "/Users/nathanleclaire/.docker/machine/machines/default"
+        }
+    },
+    "StorePath": "/Users/nathanleclaire/.docker/machine/machines/default",
+    "Name": "default"
+}`),
+			expectedHostAfter: &Host{
+				ConfigVersion: 3,
+				HostOptions: &Options{
+					AuthOptions: &auth.Options{
+						StorePath: "/Users/nathanleclaire/.docker/machine/machines/default",
+					},
+				},
+				Name:       "default",
+				DriverName: "virtualbox",
+				RawDriver:  []byte(`{"MachineName":"default","StorePath":"/Users/nathanleclaire/.docker/machine"}`),
+				Driver: &RawDataDriver{
+					Data:   []byte(`{"MachineName":"default","StorePath":"/Users/nathanleclaire/.docker/machine"}`),
+					Driver: none.NewDriver("default", "/Users/nathanleclaire/.docker/machine"),
+				},
+			},
+			expectedMigrationPerformed: true,
+			expectedMigrationError:     nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		actualHostAfter, actualMigrationPerformed, actualMigrationError := MigrateHost(tc.hostBefore, tc.rawData)
+
+		assert.Equal(t, tc.expectedHostAfter, actualHostAfter)
+		assert.Equal(t, tc.expectedMigrationPerformed, actualMigrationPerformed)
+		assert.Equal(t, tc.expectedMigrationError, actualMigrationError)
+	}
+}

libmachine/persist/filestore_test.go

@@ -1,15 +1,16 @@
 package persist
 
 import (
+	"encoding/json"
 	"fmt"
 	"io/ioutil"
-	"log"
 	"os"
 	"path/filepath"
 	"testing"
 
 	"github.com/docker/machine/commands/mcndirs"
-	_ "github.com/docker/machine/drivers/none"
+	"github.com/docker/machine/drivers/none"
+	"github.com/docker/machine/libmachine/host"
 	"github.com/docker/machine/libmachine/hosttest"
 )
 
@@ -53,6 +54,44 @@ func TestStoreSave(t *testing.T) {
 	}
 }
 
+func TestStoreSaveOmitRawDriver(t *testing.T) {
+	defer cleanup()
+
+	store := getTestStore()
+
+	h, err := hosttest.GetDefaultTestHost()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err := store.Save(h); err != nil {
+		t.Fatal(err)
+	}
+
+	configJSONPath := filepath.Join(store.GetMachinesDir(), h.Name, "config.json")
+
+	f, err := os.Open(configJSONPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	configData, err := ioutil.ReadAll(f)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	fakeHost := make(map[string]interface{})
+
+	if err := json.Unmarshal(configData, &fakeHost); err != nil {
+		t.Fatal(err)
+	}
+
+	if rawDriver, ok := fakeHost["RawDriver"]; ok {
+		t.Fatal("Should not have gotten a value for RawDriver reading host from disk but got one: ", rawDriver)
+	}
+
+}
+
 func TestStoreRemove(t *testing.T) {
 	defer cleanup()
 
@@ -171,9 +210,22 @@ func TestStoreLoad(t *testing.T) {
 
 	h, err = store.Load(h.Name)
 	if err != nil {
-		log.Fatal(err)
+		t.Fatal(err)
 	}
 
+	rawDataDriver, ok := h.Driver.(*host.RawDataDriver)
+	if !ok {
+		t.Fatal("Expected driver loaded from store to be of type *host.RawDataDriver and it was not")
+	}
+
+	realDriver := none.NewDriver(h.Name, store.Path)
+
+	if err := json.Unmarshal(rawDataDriver.Data, &realDriver); err != nil {
+		t.Fatalf("Error unmarshaling rawDataDriver data into concrete 'none' driver: %s", err)
+	}
+
+	h.Driver = realDriver
+
 	actualURL, err := h.GetURL()
 	if err != nil {
 		t.Fatal(err)

libmachine/persist/plugin_store.go

@@ -1,10 +1,6 @@
 package persist
 
 import (
-	"fmt"
-
-	"encoding/json"
-
 	"github.com/docker/machine/drivers/errdriver"
 	"github.com/docker/machine/libmachine/drivers"
 	"github.com/docker/machine/libmachine/drivers/plugin/localbinary"
@@ -47,17 +43,6 @@ func NewPluginStore(path, caCertPath, caPrivateKeyPath string) *PluginStore {
 	}
 }
 
-func (ps PluginStore) Save(host *host.Host) error {
-	data, err := json.Marshal(host.Driver)
-	if err != nil {
-		return fmt.Errorf("Error getting raw config for driver: %s", err)
-	}
-
-	host.RawDriver = data
-
-	return ps.Filestore.Save(host)
-}
-
 func (ps PluginStore) Load(name string) (*host.Host, error) {
 	h, err := ps.Filestore.Load(name)
 	if err != nil {