o/registrystate: manage accesses to registries (canonical#14544)

* 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>

* o/registrystate: document Context; other improvements

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

* o/registrystate: improve comment, names

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

* many: simplify registry access for reads

Make reads through snapctl go through a different flow than the writes.
This will change in the future as the reads will also trigger a change
to load data but for now this makes access logic easier to reason about
and it's also slightly more correct.

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

* o/h/ctlcmd: minor improvements

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

* o/registrystate: return callback from GetStoredTransaction

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

---------

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

daemon/api_registry.go

@@ -86,6 +86,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 := registrystateSet(st, account, registryName, view, values)
 	if err != nil {
 		return toAPIError(err)

overlord/hookstate/ctlcmd/export_test.go

@@ -180,10 +180,18 @@ func MockNewStatusDecorator(f func(ctx context.Context, isGlobal bool, uid strin
 	return restore
 }
 
-func MockRegistrystateRegistryTransaction(f func(*hookstate.Context, *registry.Registry) (*registrystate.Transaction, error)) (restore func()) {
-	old := registrystateRegistryTransaction
-	registrystateRegistryTransaction = f
+func MockRegistrystateGetTransaction(f func(*registrystate.Context, *state.State, *registry.View) (*registrystate.Transaction, error)) (restore func()) {
+	old := registrystateGetTransaction
+	registrystateGetTransaction = f
 	return func() {
-		registrystateRegistryTransaction = old
+		registrystateGetTransaction = old
+	}
+}
+
+func MockGetRegistryView(f func(ctx *hookstate.Context, account, registryName, viewName string) (*registry.View, error)) (restore func()) {
+	old := getRegistryView
+	getRegistryView = f
+	return func() {
+		getRegistryView = old
 	}
 }

overlord/hookstate/ctlcmd/fail.go

@@ -70,12 +70,12 @@ func (c *failCommand) Execute(args []string) error {
 	}
 
 	t, _ := ctx.Task()
-	tx, commitTask, err := registrystate.GetStoredTransaction(t)
+	tx, saveChanges, err := registrystate.GetStoredTransaction(t)
 	if err != nil {
 		return fmt.Errorf(i18n.G("internal error: cannot get registry transaction to fail: %v"), err)
 	}
 
 	tx.Abort(ctx.InstanceName(), c.Positional.Reason)
-	commitTask.Set("registry-transaction", tx)
+	saveChanges()
 	return nil
 }

overlord/hookstate/ctlcmd/get.go

@@ -361,28 +361,26 @@ func (c *getCommand) getInterfaceSetting(context *hookstate.Context, plugOrSlot
 	})
 }
 
-var registrystateRegistryTransaction = registrystate.RegistryTransaction
-
 func (c *getCommand) getRegistryValues(ctx *hookstate.Context, plugName string, requests []string, pristine bool) error {
 	if c.ForcePlugSide || c.ForceSlotSide {
 		return errors.New(i18n.G("cannot use --plug or --slot with --view"))
 	}
 	ctx.Lock()
 	defer ctx.Unlock()
 
-	view, err := getRegistryView(ctx, plugName)
+	account, registryName, viewName, err := getRegistryViewID(ctx, plugName)
 	if err != nil {
-		return fmt.Errorf("cannot get registry: %v", err)
+		return err
 	}
 
-	tx, err := registrystateRegistryTransaction(ctx, view.Registry())
+	view, err := getRegistryView(ctx, account, registryName, viewName)
 	if err != nil {
 		return err
 	}
 
-	bag := registry.DataBag(tx)
-	if pristine {
-		bag = tx.Pristine()
+	bag, err := c.getDatabag(ctx, view, pristine)
+	if err != nil {
+		return err
 	}
 
 	res, err := registrystate.GetViaView(bag, view, requests)
@@ -393,23 +391,61 @@ func (c *getCommand) getRegistryValues(ctx *hookstate.Context, plugName string,
 	return c.printPatch(res)
 }
 
-func getRegistryView(ctx *hookstate.Context, plugName string) (*registry.View, error) {
+func (c *getCommand) getDatabag(ctx *hookstate.Context, view *registry.View, pristine bool) (bag registry.DataBag, err error) {
+	account, registryName := view.Registry().Account, view.Registry().Name
+
+	var tx *registrystate.Transaction
+	if registrystate.IsRegistryHook(ctx) {
+		// running in the context of a transaction, so if the referenced registry
+		// doesn't match that tx, we only allow the caller to read the other registry
+		t, _ := ctx.Task()
+		tx, _, err = registrystate.GetStoredTransaction(t)
+		if err != nil {
+			return nil, fmt.Errorf("cannot access registry view %s/%s/%s: cannot get transaction: %v", account, registryName, view.Name, err)
+		}
+
+		if tx.RegistryAccount != account || tx.RegistryName != registryName {
+			// we're reading a different transaction
+			tx = nil
+		}
+	}
+
+	// reading a view but there's no ongoing transaction for it, make a temporary
+	// transaction just as a pass-through databag
+	if tx == nil {
+		tx, err = registrystate.NewTransaction(ctx.State(), account, registryName)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	if pristine {
+		return tx.Pristine(), nil
+	}
+	return tx, nil
+}
+
+func getRegistryViewID(ctx *hookstate.Context, plugName string) (account, registryName, viewName string, err error) {
 	repo := ifacerepo.Get(ctx.State())
 
 	plug := repo.Plug(ctx.InstanceName(), plugName)
 	if plug == nil {
-		return nil, fmt.Errorf(i18n.G("cannot find plug :%s for snap %q"), plugName, ctx.InstanceName())
+		return "", "", "", fmt.Errorf(i18n.G("cannot find plug :%s for snap %q"), plugName, ctx.InstanceName())
 	}
 
 	if plug.Interface != "registry" {
-		return nil, fmt.Errorf(i18n.G("cannot use --view with non-registry plug :%s"), plugName)
+		return "", "", "", fmt.Errorf(i18n.G("cannot use --view with non-registry plug :%s"), plugName)
 	}
 
-	account, registryName, viewName, err := snap.RegistryPlugAttrs(plug)
+	account, registryName, viewName, err = snap.RegistryPlugAttrs(plug)
 	if err != nil {
-		return nil, fmt.Errorf(i18n.G("invalid plug :%s: %w"), plugName, err)
+		return "", "", "", fmt.Errorf(i18n.G("invalid plug :%s: %w"), plugName, err)
 	}
 
+	return account, registryName, viewName, nil
+}
+
+var getRegistryView = func(ctx *hookstate.Context, account, registryName, viewName string) (*registry.View, error) {
 	registryAssert, err := assertstate.Registry(ctx.State(), account, registryName)
 	if err != nil {
 		if errors.Is(err, &asserts.NotFoundError{}) {

overlord/hookstate/ctlcmd/get_test.go

@@ -559,6 +559,10 @@ plugs:
     account: %[1]s
     view: network/write-wifi
     role: custodian
+  other:
+    interface: registry
+    account: %[1]s
+    view: other/other
 `, s.devAccID)
 	info := mockInstalledSnap(c, s.state, snapYaml, "")
 
@@ -642,57 +646,6 @@ func (s *registrySuite) TestRegistryGetNoRequest(c *C) {
 	c.Check(stderr, IsNil)
 }
 
-func (s *registrySuite) TestRegistryGetHappensTransactionally(c *C) {
-	s.state.Lock()
-	err := registrystate.Set(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.Set(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
@@ -710,7 +663,7 @@ func (s *registrySuite) TestRegistryGetInvalid(c *C) {
 		},
 		{
 			args: []string{":non-existent"},
-			err:  `cannot get registry: cannot find plug :non-existent for snap "test-snap"`,
+			err:  `cannot find plug :non-existent for snap "test-snap"`,
 		},
 	}
 
@@ -773,12 +726,12 @@ slots:
 	s.state.Unlock()
 
 	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"get", "--view", ":my-plug"}, 0)
-	c.Assert(err, ErrorMatches, "cannot get registry: cannot use --view with non-registry plug :my-plug")
+	c.Assert(err, ErrorMatches, "cannot use --view with non-registry plug :my-plug")
 	c.Check(stdout, IsNil)
 	c.Check(stderr, IsNil)
 
 	stdout, stderr, err = ctlcmd.Run(s.mockContext, []string{"set", "--view", ":my-plug", "ssid=my-ssid"}, 0)
-	c.Assert(err, ErrorMatches, "cannot set registry: cannot use --view with non-registry plug :my-plug")
+	c.Assert(err, ErrorMatches, "cannot use --view with non-registry plug :my-plug")
 	c.Check(stdout, IsNil)
 	c.Check(stderr, IsNil)
 }
@@ -797,15 +750,14 @@ func (s *registrySuite) TestRegistryGetAndSetAssertionNotFound(c *C) {
 	s.state.Unlock()
 
 	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"get", "--view", ":read-wifi"}, 0)
-	c.Assert(err, ErrorMatches, fmt.Sprintf("cannot get registry: registry assertion %s/network not found", s.devAccID))
+	c.Assert(err, ErrorMatches, fmt.Sprintf("registry assertion %s/network not found", s.devAccID))
 	c.Check(stdout, IsNil)
 	c.Check(stderr, IsNil)
 
 	stdout, stderr, err = ctlcmd.Run(s.mockContext, []string{"set", "--view", ":write-wifi", "ssid=my-ssid"}, 0)
-	c.Assert(err, ErrorMatches, fmt.Sprintf("cannot set registry: registry assertion %s/network not found", s.devAccID))
+	c.Assert(err, ErrorMatches, fmt.Sprintf("registry assertion %s/network not found", s.devAccID))
 	c.Check(stdout, IsNil)
 	c.Check(stderr, IsNil)
-
 }
 
 func (s *registrySuite) TestRegistryGetAndSetViewNotFound(c *C) {
@@ -839,12 +791,12 @@ func (s *registrySuite) TestRegistryGetAndSetViewNotFound(c *C) {
 	s.state.Unlock()
 
 	stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"get", "--view", ":read-wifi"}, 0)
-	c.Assert(err, ErrorMatches, fmt.Sprintf("cannot get registry: view \"read-wifi\" not found in registry %s/network", s.devAccID))
+	c.Assert(err, ErrorMatches, fmt.Sprintf("view \"read-wifi\" not found in registry %s/network", s.devAccID))
 	c.Check(stdout, IsNil)
 	c.Check(stderr, IsNil)
 
 	stdout, stderr, err = ctlcmd.Run(s.mockContext, []string{"set", "--view", ":write-wifi", "ssid=my-ssid"}, 0)
-	c.Assert(err, ErrorMatches, fmt.Sprintf("cannot set registry: view \"write-wifi\" not found in registry %s/network", s.devAccID))
+	c.Assert(err, ErrorMatches, fmt.Sprintf("view \"write-wifi\" not found in registry %s/network", s.devAccID))
 	c.Check(stdout, IsNil)
 	c.Check(stderr, IsNil)
 }
@@ -858,21 +810,18 @@ func (s *registrySuite) TestRegistryGetPristine(c *C) {
 	})
 	c.Assert(err, IsNil)
 
-	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)
 
 	err = tx.Set("wifi.ssid", "bar")
 	c.Assert(err, IsNil)
 
-	restore := ctlcmd.MockRegistrystateRegistryTransaction(func(*hookstate.Context, *registry.Registry) (*registrystate.Transaction, error) {
-		return tx, nil
-	})
-	defer restore()
+	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)
+
+	task.Set("registry-transaction", tx)
 
 	s.state.Unlock()
 	defer s.state.Lock()
@@ -887,3 +836,44 @@ func (s *registrySuite) TestRegistryGetPristine(c *C) {
 	c.Check(string(stdout), Equals, "bar\n")
 	c.Check(stderr, IsNil)
 }
+
+func (s *registrySuite) TestRegistryGetDifferentViewThanOngoingTx(c *C) {
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	tx, err := registrystate.NewTransaction(s.state, s.devAccID, "network")
+	c.Assert(err, IsNil)
+
+	err = tx.Set("wifi.ssid", "foo")
+	c.Assert(err, IsNil)
+
+	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)
+
+	// set ongoing tx related to the network registry
+	task.Set("registry-transaction", tx)
+
+	s.state.Unlock()
+	defer s.state.Lock()
+
+	restore := ctlcmd.MockGetRegistryView(func(ctx *hookstate.Context, account, registryName, viewName string) (*registry.View, error) {
+		reg, err := registry.New(s.devAccID, "other", map[string]interface{}{
+			"other": map[string]interface{}{
+				"rules": []interface{}{
+					map[string]interface{}{"request": "ssid", "storage": "ssid"},
+				},
+			},
+		}, registry.NewJSONSchema())
+		c.Assert(err, IsNil)
+		return reg.View("other"), nil
+	})
+	defer restore()
+
+	stdout, stderr, err := ctlcmd.Run(ctx, []string{"get", "--view", ":other", "ssid"}, 0)
+	// error is for no stored value, meaning we read the right registry
+	c.Assert(err, ErrorMatches, `.* matching rules don't map to any values`)
+	c.Check(stdout, IsNil)
+	c.Check(stderr, IsNil)
+}

overlord/hookstate/ctlcmd/set.go

@@ -34,6 +34,8 @@ import (
 	"github.com/snapcore/snapd/overlord/registrystate"
 )
 
+var registrystateGetTransaction = registrystate.GetTransactionToModify
+
 type setCommand struct {
 	baseCommand
 
@@ -230,18 +232,25 @@ func setRegistryValues(ctx *hookstate.Context, plugName string, requests map[str
 	ctx.Lock()
 	defer ctx.Unlock()
 
-	view, err := getRegistryView(ctx, plugName)
+	account, registryName, viewName, err := getRegistryViewID(ctx, plugName)
 	if err != nil {
-		return fmt.Errorf("cannot set registry: %v", err)
+		return err
 	}
 
-	tx, err := registrystate.RegistryTransaction(ctx, view.Registry())
+	view, err := getRegistryView(ctx, account, registryName, viewName)
 	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)
+	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
+	}
 
 	return registrystate.SetViaView(tx, view, requests)
 }