o/registrystate: manage accesses to registries

This change changes the registries accesses to use GetTransaction which
determines which kind of access it's dealing with (e.g,. snap set,
snapctl set, in or out of a hook, registry hook or non-registry, etc)
and takes the appropriate action.
This omits the concurrency checks and I've also opted to removed the
bits allowing reading concurrently if the registry being read is
different than the one with a transaction ongoing (this will require
changes anyway since we'll add a load-registry change in the future).
At the moment, the user API endpoint isn't connected to this yet since
that will require more changes to it.

Signed-off-by: Miguel Pires <miguel.pires@canonical.com>

daemon/api_registry.go

@@ -60,6 +60,7 @@ func getView(c *Command, r *http.Request, _ *auth.UserState) Response {
 		fields = strutil.CommaSeparatedList(fieldStr)
 	}
 
+	// TODO: replace access w/ GetTransaction
 	results, err := registrystateGetViaView(st, account, registryName, view, fields)
 	if err != nil {
 		return toAPIError(err)
@@ -86,6 +87,7 @@ func setView(c *Command, r *http.Request, _ *auth.UserState) Response {
 		return BadRequest("cannot decode registry request body: %v", err)
 	}
 
+	// TODO: replace w/ GetTransaction + call ctx.Done() then return changeID
 	err := registrystateSetViaView(st, account, registryName, view, values)
 	if err != nil {
 		return toAPIError(err)

overlord/hookstate/ctlcmd/export_test.go

@@ -28,9 +28,11 @@ import (
 	"github.com/snapcore/snapd/client/clientutil"
 	"github.com/snapcore/snapd/overlord/devicestate"
 	"github.com/snapcore/snapd/overlord/hookstate"
+	"github.com/snapcore/snapd/overlord/registrystate"
 	"github.com/snapcore/snapd/overlord/servicestate"
 	"github.com/snapcore/snapd/overlord/snapstate"
 	"github.com/snapcore/snapd/overlord/state"
+	"github.com/snapcore/snapd/registry"
 	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/testutil"
 )
@@ -177,3 +179,11 @@ func MockNewStatusDecorator(f func(ctx context.Context, isGlobal bool, uid strin
 	newStatusDecorator = f
 	return restore
 }
+
+func MockRegistrystateGetTransaction(f func(*registrystate.Context, *state.State, *registry.View) (*registrystate.Transaction, error)) (restore func()) {
+	old := registrystateGetTransaction
+	registrystateGetTransaction = f
+	return func() {
+		registrystateGetTransaction = old
+	}
+}

overlord/hookstate/ctlcmd/get.go

@@ -39,6 +39,8 @@ import (
 	"github.com/snapcore/snapd/snap"
 )
 
+var registrystateGetTransaction = registrystate.GetTransaction
+
 type getCommand struct {
 	baseCommand
 
@@ -369,7 +371,8 @@ func (c *getCommand) getRegistryValues(ctx *hookstate.Context, plugName string,
 		return fmt.Errorf("cannot get registry: %v", err)
 	}
 
-	tx, err := registrystate.RegistryTransaction(ctx, view.Registry())
+	regCtx := registrystate.NewContext(ctx)
+	tx, err := registrystateGetTransaction(regCtx, ctx.State(), view)
 	if err != nil {
 		return err
 	}

overlord/hookstate/ctlcmd/get_test.go

@@ -641,57 +641,6 @@ func (s *registrySuite) TestRegistryGetNoRequest(c *C) {
 	c.Check(stderr, IsNil)
 }
 
-func (s *registrySuite) TestRegistryGetHappensTransactionally(c *C) {
-	s.state.Lock()
-	err := registrystate.SetViaView(s.state, s.devAccID, "network", "write-wifi", map[string]interface{}{
-		"ssid": "my-ssid",
-	})
-	s.state.Unlock()
-	c.Assert(err, IsNil)
-
-	// registry transaction is created when snapctl runs for the first time
-	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"get", "--view", ":read-wifi"}, 0)
-	c.Assert(err, IsNil)
-	c.Check(string(stdout), Equals, `{
-	"ssid": "my-ssid"
-}
-`)
-	c.Check(stderr, IsNil)
-
-	s.state.Lock()
-	err = registrystate.SetViaView(s.state, s.devAccID, "network", "write-wifi", map[string]interface{}{
-		"ssid": "other-ssid",
-	})
-	s.state.Unlock()
-	c.Assert(err, IsNil)
-
-	// the new write wasn't reflected because it didn't run in the same transaction
-	stdout, stderr, err = ctlcmd.Run(s.mockContext, []string{"get", "--view", ":read-wifi"}, 0)
-	c.Assert(err, IsNil)
-	c.Check(string(stdout), Equals, `{
-	"ssid": "my-ssid"
-}
-`)
-	c.Check(stderr, IsNil)
-
-	// make a new context so we get a new transaction
-	s.state.Lock()
-	task := s.state.NewTask("test-task", "my test task")
-	setup := &hookstate.HookSetup{Snap: "test-snap", Revision: snap.R(1), Hook: "test-hook"}
-	s.mockContext, err = hookstate.NewContext(task, s.state, setup, s.mockHandler, "")
-	s.state.Unlock()
-	c.Assert(err, IsNil)
-
-	// now we get the new data
-	stdout, stderr, err = ctlcmd.Run(s.mockContext, []string{"get", "--view", ":read-wifi"}, 0)
-	c.Assert(err, IsNil)
-	c.Check(string(stdout), Equals, `{
-	"ssid": "other-ssid"
-}
-`)
-	c.Check(stderr, IsNil)
-}
-
 func (s *registrySuite) TestRegistryGetInvalid(c *C) {
 	type testcase struct {
 		args []string
@@ -804,7 +753,6 @@ func (s *registrySuite) TestRegistryGetAndSetAssertionNotFound(c *C) {
 	c.Assert(err, ErrorMatches, fmt.Sprintf("cannot set registry: registry assertion %s/network not found", s.devAccID))
 	c.Check(stdout, IsNil)
 	c.Check(stderr, IsNil)
-
 }
 
 func (s *registrySuite) TestRegistryGetAndSetViewNotFound(c *C) {

overlord/hookstate/ctlcmd/set.go

@@ -235,13 +235,15 @@ func setRegistryValues(ctx *hookstate.Context, plugName string, requests map[str
 		return fmt.Errorf("cannot set registry: %v", err)
 	}
 
-	tx, err := registrystate.RegistryTransaction(ctx, view.Registry())
+	if registrystate.IsRegistryHook(ctx) && !strings.HasPrefix(ctx.HookName(), "change-view-") {
+		return fmt.Errorf("cannot modify registry in %q hook", ctx.HookName())
+	}
+
+	regCtx := registrystate.NewContext(ctx)
+	tx, err := registrystateGetTransaction(regCtx, ctx.State(), view)
 	if err != nil {
 		return err
 	}
 
-	// TODO: once we have hooks, check that we don't set values in the wrong hooks
-	// (e.g., "registry-changed" hooks can only read data)
-
 	return registrystate.SetViaViewInTx(tx, view, requests)
 }

overlord/hookstate/ctlcmd/set_test.go

@@ -33,6 +33,7 @@ import (
 	"github.com/snapcore/snapd/overlord/hookstate/hooktest"
 	"github.com/snapcore/snapd/overlord/registrystate"
 	"github.com/snapcore/snapd/overlord/state"
+	"github.com/snapcore/snapd/registry"
 	"github.com/snapcore/snapd/snap"
 )
 
@@ -405,6 +406,16 @@ func (s *setAttrSuite) TestSetCommandFailsOutsideOfValidContext(c *C) {
 }
 
 func (s *registrySuite) TestRegistrySetSingleView(c *C) {
+	s.state.Lock()
+	tx, err := registrystate.NewTransaction(s.state, s.devAccID, "network")
+	s.state.Unlock()
+	c.Assert(err, IsNil)
+
+	restore := ctlcmd.MockRegistrystateGetTransaction(func(*registrystate.Context, *state.State, *registry.View) (*registrystate.Transaction, error) {
+		return tx, nil
+	})
+	defer restore()
+
 	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"set", "--view", ":write-wifi", "ssid=other-ssid"}, 0)
 	c.Assert(err, IsNil)
 	c.Check(stdout, IsNil)
@@ -413,56 +424,34 @@ func (s *registrySuite) TestRegistrySetSingleView(c *C) {
 	c.Assert(s.mockContext.Done(), IsNil)
 	s.mockContext.Unlock()
 
-	s.state.Lock()
-	val, err := registrystate.GetViaView(s.state, s.devAccID, "network", "read-wifi", []string{"ssid"})
-	s.state.Unlock()
+	val, err := tx.Get("wifi.ssid")
 	c.Assert(err, IsNil)
-	c.Assert(val, DeepEquals, map[string]interface{}{"ssid": "other-ssid"})
+	c.Assert(val, DeepEquals, "other-ssid")
 }
 
 func (s *registrySuite) TestRegistrySetManyViews(c *C) {
-	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"set", "--view", ":write-wifi", "ssid=other-ssid", "password=other-secret"}, 0)
-	c.Assert(err, IsNil)
-	c.Check(stdout, IsNil)
-	c.Check(stderr, IsNil)
-	s.mockContext.Lock()
-	c.Assert(s.mockContext.Done(), IsNil)
-	s.mockContext.Unlock()
-
 	s.state.Lock()
-	val, err := registrystate.GetViaView(s.state, s.devAccID, "network", "read-wifi", []string{"ssid", "password"})
+	tx, err := registrystate.NewTransaction(s.state, s.devAccID, "network")
 	s.state.Unlock()
 	c.Assert(err, IsNil)
-	c.Assert(val, DeepEquals, map[string]interface{}{
-		"ssid":     "other-ssid",
-		"password": "other-secret",
+
+	restore := ctlcmd.MockRegistrystateGetTransaction(func(*registrystate.Context, *state.State, *registry.View) (*registrystate.Transaction, error) {
+		return tx, nil
 	})
-}
+	defer restore()
 
-func (s *registrySuite) TestRegistrySetHappensTransactionally(c *C) {
-	// sets values in a transaction
-	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"set", "--view", ":write-wifi", "ssid=my-ssid"}, 0)
+	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"set", "--view", ":write-wifi", "ssid=other-ssid", "password=other-secret"}, 0)
 	c.Assert(err, IsNil)
 	c.Check(stdout, IsNil)
 	c.Check(stderr, IsNil)
 
-	s.state.Lock()
-	_, err = registrystate.GetViaView(s.state, s.devAccID, "network", "read-wifi", []string{"ssid"})
-	s.state.Unlock()
-	c.Assert(err, ErrorMatches, ".*matching rules don't map to any values")
-
-	// commit transaction
-	s.mockContext.Lock()
-	c.Assert(s.mockContext.Done(), IsNil)
-	s.mockContext.Unlock()
+	val, err := tx.Get("wifi.ssid")
+	c.Assert(err, IsNil)
+	c.Assert(val, Equals, "other-ssid")
 
-	s.state.Lock()
-	val, err := registrystate.GetViaView(s.state, s.devAccID, "network", "read-wifi", []string{"ssid"})
-	s.state.Unlock()
+	val, err = tx.Get("wifi.psk")
 	c.Assert(err, IsNil)
-	c.Assert(val, DeepEquals, map[string]interface{}{
-		"ssid": "my-ssid",
-	})
+	c.Assert(val, Equals, "other-secret")
 }
 
 func (s *registrySuite) TestRegistrySetInvalid(c *C) {
@@ -491,21 +480,70 @@ func (s *registrySuite) TestRegistrySetInvalid(c *C) {
 }
 
 func (s *registrySuite) TestRegistrySetExclamationMark(c *C) {
-	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"set", "--view", ":write-wifi", "ssid=other-ssid", "password=other-secret"}, 0)
+	s.state.Lock()
+	tx, err := registrystate.NewTransaction(s.state, s.devAccID, "network")
+	s.state.Unlock()
 	c.Assert(err, IsNil)
-	c.Check(stdout, IsNil)
-	c.Check(stderr, IsNil)
 
-	stdout, stderr, err = ctlcmd.Run(s.mockContext, []string{"set", "--view", ":write-wifi", "password!"}, 0)
+	err = tx.Set("wifi.ssid", "foo")
+	c.Assert(err, IsNil)
+
+	err = tx.Set("wifi.psk", "bar")
+	c.Assert(err, IsNil)
+
+	restore := ctlcmd.MockRegistrystateGetTransaction(func(*registrystate.Context, *state.State, *registry.View) (*registrystate.Transaction, error) {
+		return tx, nil
+	})
+	defer restore()
+
+	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"set", "--view", ":write-wifi", "password!"}, 0)
 	c.Assert(err, IsNil)
 	c.Check(stdout, IsNil)
 	c.Check(stderr, IsNil)
 
 	stdout, stderr, err = ctlcmd.Run(s.mockContext, []string{"get", "--view", ":read-wifi"}, 0)
 	c.Assert(err, IsNil)
 	c.Check(string(stdout), Equals, `{
-	"ssid": "other-ssid"
+	"ssid": "foo"
 }
 `)
 	c.Check(stderr, IsNil)
+	s.state.Lock()
+}
+
+func (s *registrySuite) TestRegistryOnlyChangeViewCanSet(c *C) {
+	s.state.Lock()
+	defer s.state.Unlock()
+	task := s.state.NewTask("run-hook", "")
+
+	setup := &hookstate.HookSetup{Snap: "test-snap", Hook: "save-view-plug"}
+	ctx, err := hookstate.NewContext(task, s.state, setup, s.mockHandler, "")
+	c.Assert(err, IsNil)
+
+	tx, err := registrystate.NewTransaction(s.state, s.devAccID, "network")
+	c.Assert(err, IsNil)
+
+	restore := ctlcmd.MockRegistrystateGetTransaction(func(*registrystate.Context, *state.State, *registry.View) (*registrystate.Transaction, error) {
+		return tx, nil
+	})
+	defer restore()
+
+	s.state.Unlock()
+	stdout, stderr, err := ctlcmd.Run(ctx, []string{"set", "--view", ":write-wifi", "password=thing"}, 0)
+	s.state.Lock()
+	c.Assert(err, ErrorMatches, `cannot modify registry in "save-view-plug" hook`)
+	c.Check(stdout, IsNil)
+	c.Check(stderr, IsNil)
+
+	setup.Hook = "change-view-plug"
+	ctx, err = hookstate.NewContext(task, s.state, setup, s.mockHandler, "")
+	c.Assert(err, IsNil)
+
+	s.state.Unlock()
+	stdout, stderr, err = ctlcmd.Run(ctx, []string{"set", "--view", ":write-wifi", "password=thing"}, 0)
+	s.state.Lock()
+	c.Assert(err, IsNil)
+	c.Check(stdout, IsNil)
+	c.Check(stderr, IsNil)
+
 }

overlord/hookstate/ctlcmd/unset_test.go

@@ -30,6 +30,7 @@ import (
 	"github.com/snapcore/snapd/overlord/hookstate/hooktest"
 	"github.com/snapcore/snapd/overlord/registrystate"
 	"github.com/snapcore/snapd/overlord/state"
+	"github.com/snapcore/snapd/registry"
 	"github.com/snapcore/snapd/snap"
 )
 
@@ -164,52 +165,30 @@ func (s *unsetSuite) TestCommandWithoutContext(c *C) {
 
 func (s *registrySuite) TestRegistryUnsetManyViews(c *C) {
 	s.state.Lock()
-	err := registrystate.SetViaView(s.state, s.devAccID, "network", "write-wifi", map[string]interface{}{"ssid": "my-ssid", "password": "my-secret"})
+	tx, err := registrystate.NewTransaction(s.state, s.devAccID, "network")
 	s.state.Unlock()
 	c.Assert(err, IsNil)
 
-	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"unset", "--view", ":write-wifi", "ssid", "password"}, 0)
+	err = tx.Set("wifi.ssid", "foo")
 	c.Assert(err, IsNil)
-	c.Check(stdout, IsNil)
-	c.Check(stderr, IsNil)
-	s.mockContext.Lock()
-	c.Assert(s.mockContext.Done(), IsNil)
-	s.mockContext.Unlock()
 
-	s.state.Lock()
-	_, err = registrystate.GetViaView(s.state, s.devAccID, "network", "write-wifi", []string{"ssid", "password"})
-	s.state.Unlock()
-	c.Assert(err, ErrorMatches, `cannot get "ssid", "password" .*: matching rules don't map to any values`)
-}
-
-func (s *registrySuite) TestRegistryUnsetHappensTransactionally(c *C) {
-	s.state.Lock()
-	err := registrystate.SetViaView(s.state, s.devAccID, "network", "write-wifi", map[string]interface{}{"ssid": "my-ssid"})
-	s.state.Unlock()
+	err = tx.Set("wifi.psk", "bar")
 	c.Assert(err, IsNil)
 
-	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"unset", "--view", ":write-wifi", "ssid"}, 0)
+	ctlcmd.MockRegistrystateGetTransaction(func(*registrystate.Context, *state.State, *registry.View) (*registrystate.Transaction, error) {
+		return tx, nil
+	})
+
+	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"unset", "--view", ":write-wifi", "ssid", "password"}, 0)
 	c.Assert(err, IsNil)
 	c.Check(stdout, IsNil)
 	c.Check(stderr, IsNil)
 
-	s.state.Lock()
-	val, err := registrystate.GetViaView(s.state, s.devAccID, "network", "read-wifi", []string{"ssid"})
-	s.state.Unlock()
-	c.Assert(err, IsNil)
-	c.Assert(val, DeepEquals, map[string]interface{}{
-		"ssid": "my-ssid",
-	})
-
-	// commit transaction
-	s.mockContext.Lock()
-	c.Assert(s.mockContext.Done(), IsNil)
-	s.mockContext.Unlock()
+	_, err = tx.Get("wifi.ssid")
+	c.Assert(err, ErrorMatches, `no value was found under path "wifi.ssid"`)
 
-	s.state.Lock()
-	_, err = registrystate.GetViaView(s.state, s.devAccID, "network", "read-wifi", []string{"ssid"})
-	s.state.Unlock()
-	c.Assert(err, ErrorMatches, `cannot get "ssid" .*: matching rules don't map to any values`)
+	_, err = tx.Get("wifi.psk")
+	c.Assert(err, ErrorMatches, `no value was found under path "wifi.psk"`)
 }
 
 func (s *registrySuite) TestRegistryUnsetInvalid(c *C) {