Fix for LP #1917870: properly handle nil configuration (no config) when

restoring snaphots.

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

@@ -287,7 +287,9 @@ 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 {
+		// 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 !ok || config == nil {
 			config = make(map[string]*json.RawMessage)
 		}
 		applyChanges(config, snapChanges)

overlord/snapshotstate/snapshotmgr.go

@@ -267,6 +267,22 @@ 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 doRestore(task *state.Task, tomb *tomb.Tomb) error {
 	snapshot, oldCfg, reader, err := prepareRestore(task)
 	if err != nil {
@@ -286,7 +302,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 +311,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 +337,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

@@ -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")
@@ -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,10 @@ 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

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