o/configstate, o/snapshotstate: fix handling of nil snap config on snapshot restore

overlord/configstate/config/helpers.go

@@ -145,7 +145,8 @@ func GetSnapConfig(st *state.State, snapName string) (*json.RawMessage, error) {
 func SetSnapConfig(st *state.State, snapName string, snapcfg *json.RawMessage) error {
 	var config map[string]*json.RawMessage
 	err := st.Get("config", &config)
-	isNil := snapcfg == nil || len(*snapcfg) == 0
+	// empty nil snapcfg should be an empty message, but deal with "null" as well.
+	isNil := snapcfg == nil || len(*snapcfg) == 0 || bytes.Compare(*snapcfg, []byte("null")) == 0
 	if err == state.ErrNoState {
 		if isNil {
 			// bail out early

overlord/configstate/config/helpers_test.go

@@ -146,8 +146,13 @@ func (s *configHelpersSuite) TestSnapConfig(c *C) {
 	defer s.state.Unlock()
 
 	empty1 := json.RawMessage(nil)
+	buf, err := json.Marshal(nil)
+	c.Assert(err, IsNil)
+	empty2 := (*json.RawMessage)(&buf)
+	// sanity check
+	c.Check(bytes.Compare(*empty2, []byte(`null`)), Equals, 0)
 
-	for _, emptyCfg := range []*json.RawMessage{nil, &empty1, {}} {
+	for _, emptyCfg := range []*json.RawMessage{nil, &empty1, empty2, {}} {
 		rawCfg, err := config.GetSnapConfig(s.state, "snap1")
 		c.Assert(err, IsNil)
 		c.Check(rawCfg, IsNil)

overlord/configstate/config/transaction.go

@@ -286,8 +286,10 @@ func (t *Transaction) Commit() {
 
 	// Iterate through the write cache and save each item.
 	for instanceName, snapChanges := range t.changes {
-		config, ok := t.pristine[instanceName]
-		if !ok {
+		config := t.pristine[instanceName]
+		// due to LP #1917870 we might have a hook configure task in flight
+		// that tries to apply config over nil map, create it if nil.
+		if config == nil {
 			config = make(map[string]*json.RawMessage)
 		}
 		applyChanges(config, snapChanges)

overlord/configstate/config/transaction_test.go

@@ -430,6 +430,21 @@ func (b *brokenType) UnmarshalJSON(data []byte) error {
 	return nil
 }
 
+func (s *transactionSuite) TestCommitOverNilSnapConfig(c *C) {
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	// simulate invalid nil map created due to LP #1917870 by snap restore
+	s.state.Set("config", map[string]interface{}{"test-snap": nil})
+	t := config.NewTransaction(s.state)
+
+	c.Assert(t.Set("test-snap", "foo", "bar"), IsNil)
+	t.Commit()
+	var v string
+	t.Get("test-snap", "foo", &v)
+	c.Assert(v, Equals, "bar")
+}
+
 func (s *transactionSuite) TestGetUnmarshalError(c *C) {
 	s.state.Lock()
 	defer s.state.Unlock()

overlord/snapshotstate/snapshotmgr.go

@@ -196,15 +196,10 @@ func prepareSave(task *state.Task) (snapshot *snapshotSetup, cur *snap.Info, cfg
 	snapshot.Filename = filename(snapshot.SetID, cur)
 	task.Set("snapshot-setup", &snapshot)
 
-	rawCfg, err := configGetSnapConfig(st, snapshot.Snap)
+	cfg, err = unmarshalSnapConfig(st, snapshot.Snap)
 	if err != nil {
 		return nil, nil, nil, err
 	}
-	if rawCfg != nil {
-		if err := json.Unmarshal(*rawCfg, &cfg); err != nil {
-			return nil, nil, nil, err
-		}
-	}
 
 	// this should be done last because of it modifies the state and the caller needs to undo this if other operation fails.
 	if snapshot.Auto {
@@ -247,17 +242,10 @@ func prepareRestore(task *state.Task) (snapshot *snapshotSetup, oldCfg map[strin
 		return nil, nil, nil, taskGetErrMsg(task, err, "snapshot")
 	}
 
-	rawCfg, err := configGetSnapConfig(st, snapshot.Snap)
+	oldCfg, err = unmarshalSnapConfig(st, snapshot.Snap)
 	if err != nil {
-		return nil, nil, nil, fmt.Errorf("internal error: cannot obtain current snap config for snapshot restore: %v", err)
-	}
-
-	if rawCfg != nil {
-		if err := json.Unmarshal(*rawCfg, &oldCfg); err != nil {
-			return nil, nil, nil, fmt.Errorf("internal error: cannot decode current snap config: %v", err)
-		}
+		return nil, nil, nil, err
 	}
-
 	reader, err = backendOpen(snapshot.Filename, backend.ExtractFnameSetID)
 	if err != nil {
 		return nil, nil, nil, fmt.Errorf("cannot open snapshot: %v", err)
@@ -267,6 +255,36 @@ func prepareRestore(task *state.Task) (snapshot *snapshotSetup, oldCfg map[strin
 	return snapshot, oldCfg, reader, nil
 }
 
+// marshalSnapConfig encodes cfg to JSON and returns raw JSON message, unless
+// cfg is nil - in this case nil is returned.
+func marshalSnapConfig(cfg map[string]interface{}) (*json.RawMessage, error) {
+	if cfg == nil {
+		// do not marshal nil - this would result in "null" raw message which
+		// we want to avoid.
+		return nil, nil
+	}
+	buf, err := json.Marshal(cfg)
+	if err != nil {
+		return nil, err
+	}
+	raw := (*json.RawMessage)(&buf)
+	return raw, err
+}
+
+func unmarshalSnapConfig(st *state.State, snapName string) (map[string]interface{}, error) {
+	rawCfg, err := configGetSnapConfig(st, snapName)
+	if err != nil {
+		return nil, fmt.Errorf("internal error: cannot obtain current snap config: %v", err)
+	}
+	var cfg map[string]interface{}
+	if rawCfg != nil {
+		if err := json.Unmarshal(*rawCfg, &cfg); err != nil {
+			return nil, fmt.Errorf("internal error: cannot decode current snap config: %v", err)
+		}
+	}
+	return cfg, nil
+}
+
 func doRestore(task *state.Task, tomb *tomb.Tomb) error {
 	snapshot, oldCfg, reader, err := prepareRestore(task)
 	if err != nil {
@@ -286,7 +304,7 @@ func doRestore(task *state.Task, tomb *tomb.Tomb) error {
 		return err
 	}
 
-	buf, err := json.Marshal(reader.Conf)
+	raw, err := marshalSnapConfig(reader.Conf)
 	if err != nil {
 		backendRevert(restoreState)
 		return fmt.Errorf("cannot marshal saved config: %v", err)
@@ -295,7 +313,7 @@ func doRestore(task *state.Task, tomb *tomb.Tomb) error {
 	st.Lock()
 	defer st.Unlock()
 
-	if err := configSetSnapConfig(st, snapshot.Snap, (*json.RawMessage)(&buf)); err != nil {
+	if err := configSetSnapConfig(st, snapshot.Snap, raw); err != nil {
 		backendRevert(restoreState)
 		return fmt.Errorf("cannot set snap config: %v", err)
 	}
@@ -321,12 +339,12 @@ func undoRestore(task *state.Task, _ *tomb.Tomb) error {
 		return taskGetErrMsg(task, err, "snapshot")
 	}
 
-	buf, err := json.Marshal(restoreState.Config)
+	raw, err := marshalSnapConfig(restoreState.Config)
 	if err != nil {
 		return fmt.Errorf("cannot marshal saved config: %v", err)
 	}
 
-	if err := configSetSnapConfig(st, snapshot.Snap, (*json.RawMessage)(&buf)); err != nil {
+	if err := configSetSnapConfig(st, snapshot.Snap, raw); err != nil {
 		return fmt.Errorf("cannot restore saved config: %v", err)
 	}
 

overlord/snapshotstate/snapshotmgr_test.go

@@ -398,7 +398,7 @@ func (snapshotSuite) TestDoSaveFailsConfigError(c *check.C) {
 	})
 	st.Unlock()
 	err := snapshotstate.DoSave(task, &tomb.Tomb{})
-	c.Assert(err, check.ErrorMatches, "bzzt")
+	c.Assert(err, check.ErrorMatches, "internal error: cannot obtain current snap config: bzzt")
 }
 
 func (snapshotSuite) TestDoSaveFailsBadConfig(c *check.C) {
@@ -581,6 +581,48 @@ func (rs *readerSuite) TestDoRestore(c *check.C) {
 	c.Check(v, check.DeepEquals, map[string]interface{}{"config": map[string]interface{}{"old": "conf"}})
 }
 
+func (rs *readerSuite) TestDoRestoreNoConfig(c *check.C) {
+	defer snapshotstate.MockConfigGetSnapConfig(func(_ *state.State, snapname string) (*json.RawMessage, error) {
+		rs.calls = append(rs.calls, "get config")
+		c.Check(snapname, check.Equals, "a-snap")
+		// simulate old config
+		raw := json.RawMessage(`{"foo": "bar"}`)
+		return &raw, nil
+	})()
+	defer snapshotstate.MockBackendOpen(func(filename string, setID uint64) (*backend.Reader, error) {
+		rs.calls = append(rs.calls, "open")
+		// set id 0 tells backend.Open to use set id from the filename
+		c.Check(setID, check.Equals, uint64(0))
+		c.Check(filename, check.Equals, "/some/1_file.zip")
+		return &backend.Reader{
+			// snapshot has no configuration to restore
+			Snapshot: client.Snapshot{Snap: "a-snap", Conf: nil},
+		}, nil
+	})()
+	defer snapshotstate.MockBackendRestore(func(_ *backend.Reader, _ context.Context, _ snap.Revision, users []string, _ backend.Logf) (*backend.RestoreState, error) {
+		rs.calls = append(rs.calls, "restore")
+		c.Check(users, check.DeepEquals, []string{"a-user", "b-user"})
+		return &backend.RestoreState{}, nil
+	})()
+	defer snapshotstate.MockConfigSetSnapConfig(func(_ *state.State, snapname string, conf *json.RawMessage) error {
+		rs.calls = append(rs.calls, "set config")
+		c.Check(snapname, check.Equals, "a-snap")
+		c.Check(conf, check.IsNil)
+		return nil
+	})()
+
+	st := rs.task.State()
+	err := snapshotstate.DoRestore(rs.task, &tomb.Tomb{})
+	c.Assert(err, check.IsNil)
+	c.Check(rs.calls, check.DeepEquals, []string{"get config", "open", "restore", "set config"})
+
+	st.Lock()
+	defer st.Unlock()
+	var v map[string]interface{}
+	rs.task.Get("restore-state", &v)
+	c.Check(v, check.DeepEquals, map[string]interface{}{"config": map[string]interface{}{"foo": "bar"}})
+}
+
 func (rs *readerSuite) TestDoRestoreFailsNoTaskSnapshot(c *check.C) {
 	rs.task.State().Lock()
 	rs.task.Clear("snapshot-setup")
@@ -599,7 +641,7 @@ func (rs *readerSuite) TestDoRestoreFailsOnGetConfigError(c *check.C) {
 	})()
 
 	err := snapshotstate.DoRestore(rs.task, &tomb.Tomb{})
-	c.Assert(err, check.ErrorMatches, "internal error: cannot obtain current snap config for snapshot restore: bzzt")
+	c.Assert(err, check.ErrorMatches, "internal error: cannot obtain current snap config: bzzt")
 	c.Check(rs.calls, check.DeepEquals, []string{"get config"})
 }
 
@@ -662,6 +704,30 @@ func (rs *readerSuite) TestDoRestoreFailsAndRevertsOnSetConfigError(c *check.C)
 }
 
 func (rs *readerSuite) TestUndoRestore(c *check.C) {
+	defer snapshotstate.MockConfigSetSnapConfig(func(st *state.State, snapName string, raw *json.RawMessage) error {
+		rs.calls = append(rs.calls, "set config")
+		c.Check(string(*raw), check.Equals, `{"foo":"bar"}`)
+		return nil
+	})()
+
+	st := rs.task.State()
+	st.Lock()
+	v := map[string]interface{}{"config": map[string]interface{}{"foo": "bar"}}
+	rs.task.Set("restore-state", &v)
+	st.Unlock()
+
+	err := snapshotstate.UndoRestore(rs.task, &tomb.Tomb{})
+	c.Assert(err, check.IsNil)
+	c.Check(rs.calls, check.DeepEquals, []string{"set config", "revert"})
+}
+
+func (rs *readerSuite) TestUndoRestoreNoConfig(c *check.C) {
+	defer snapshotstate.MockConfigSetSnapConfig(func(st *state.State, snapName string, raw *json.RawMessage) error {
+		rs.calls = append(rs.calls, "set config")
+		c.Check(raw, check.IsNil)
+		return nil
+	})()
+
 	st := rs.task.State()
 	st.Lock()
 	var v map[string]interface{}

tests/main/snapshot-basic/task.yaml

@@ -158,3 +158,11 @@ execute: |
     [[ $(snap saved | grep -c test-snapd-sh) == "1" ]]
     snap saved --id=123 | MATCH "123 .+test-snapd-sh"
     snap restore 123 test-snapd-sh
+
+    # snap configuration should work after restoring snapshots that didn't
+    # have config (LP #1917870).
+    "$TESTSTOOLS"/snaps-state install-local test-snap
+    SET_ID=$( snap save test-snap | cut -d\  -f1 | tail -n1 )
+    snap restore "$SET_ID"
+    snap set test-snap foo=baz
+    snap get test-snap foo | MATCH "baz"

tests/main/snapshot-basic/test-snap/bin/sh

@@ -0,0 +1 @@
+#!/bin/sh

tests/main/snapshot-basic/test-snap/meta/hooks/configure

@@ -0,0 +1,4 @@
+#!/bin/sh
+
+# it is important for the test that this configure hook doesn't set
+# any snap config options via snapctl.

tests/main/snapshot-basic/test-snap/meta/snap.yaml

@@ -0,0 +1,6 @@
+name: test-snap
+version: 1.0
+
+apps:
+  sh:
+    command: bin/sh