From f9e69aa5441a3ae74d74804fdc9326c8d29ca5ce Mon Sep 17 00:00:00 2001 From: Miguel Pires Date: Mon, 14 Oct 2024 10:37:02 +0100 Subject: [PATCH] o/h/ctlcmd: hide snapctl registry commands behind feature flag Signed-off-by: Miguel Pires --- overlord/hookstate/ctlcmd/fail.go | 10 +++--- overlord/hookstate/ctlcmd/fail_test.go | 32 +++++------------- overlord/hookstate/ctlcmd/get.go | 34 +++++++++++++++++--- overlord/hookstate/ctlcmd/get_test.go | 25 ++++++++++++++ overlord/hookstate/ctlcmd/set.go | 4 +++ overlord/hookstate/ctlcmd/unset.go | 12 ++++--- overlord/registrystate/registrystate_test.go | 11 +++++-- 7 files changed, 87 insertions(+), 41 deletions(-) diff --git a/overlord/hookstate/ctlcmd/fail.go b/overlord/hookstate/ctlcmd/fail.go index 1476f30234c..7be225d60a6 100644 --- a/overlord/hookstate/ctlcmd/fail.go +++ b/overlord/hookstate/ctlcmd/fail.go @@ -24,7 +24,6 @@ import ( "fmt" "strings" - "github.com/snapcore/snapd/features" "github.com/snapcore/snapd/i18n" "github.com/snapcore/snapd/overlord/registrystate" ) @@ -52,16 +51,15 @@ func init() { } func (c *failCommand) Execute(args []string) error { - if !features.Registries.IsEnabled() { - _, confName := features.Registries.ConfigOption() - return fmt.Errorf(`cannot use "snapctl fail" without enabling the %q feature`, confName) - } - ctx, err := c.ensureContext() if err != nil { return err } + if err := validateRegistriesFeatureFlag(ctx.State()); err != nil { + return err + } + ctx.Lock() defer ctx.Unlock() diff --git a/overlord/hookstate/ctlcmd/fail_test.go b/overlord/hookstate/ctlcmd/fail_test.go index efaece8ecef..cf2e377aaca 100644 --- a/overlord/hookstate/ctlcmd/fail_test.go +++ b/overlord/hookstate/ctlcmd/fail_test.go @@ -20,13 +20,8 @@ package ctlcmd_test import ( - "os" - - "gopkg.in/check.v1" . "gopkg.in/check.v1" - "github.com/snapcore/snapd/dirs" - "github.com/snapcore/snapd/features" "github.com/snapcore/snapd/i18n" "github.com/snapcore/snapd/overlord/hookstate" "github.com/snapcore/snapd/overlord/hookstate/ctlcmd" @@ -34,23 +29,7 @@ import ( "github.com/snapcore/snapd/snap" ) -func (s *registrySuite) mockRegistriesFeature(c *C) (restore func()) { - oldDir := dirs.FeaturesDir - dirs.FeaturesDir = c.MkDir() - - registryCtlFile := features.Registries.ControlFile() - c.Assert(os.WriteFile(registryCtlFile, []byte(nil), 0644), check.IsNil) - - return func() { - c.Assert(os.Remove(registryCtlFile), IsNil) - dirs.FeaturesDir = oldDir - } -} - func (s *registrySuite) TestFailAbortsRegistryTransaction(c *C) { - restore := s.mockRegistriesFeature(c) - defer restore() - s.state.Lock() chg := s.state.NewChange("test", "") commitTask := s.state.NewTask("commit-registry-tx", "") @@ -89,13 +68,18 @@ func (s *registrySuite) TestFailAbortsRegistryTransaction(c *C) { } func (s *registrySuite) TestFailErrors(c *C) { + s.state.Lock() + s.setRegistryFlag(false, c) + s.state.Unlock() + stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{"fail", "reason"}, 0) - c.Assert(err, ErrorMatches, i18n.G(`cannot use "snapctl fail" without enabling the "experimental.registries" feature`)) + c.Assert(err, ErrorMatches, i18n.G(`"registries" feature flag is disabled: set 'experimental.registries' to true`)) c.Check(stdout, IsNil) c.Check(stderr, IsNil) - restore := s.mockRegistriesFeature(c) - defer restore() + s.state.Lock() + s.setRegistryFlag(true, c) + s.state.Unlock() stdout, stderr, err = ctlcmd.Run(s.mockContext, []string{"fail"}, 0) c.Assert(err, ErrorMatches, i18n.G("the required argument `:` was not provided")) diff --git a/overlord/hookstate/ctlcmd/get.go b/overlord/hookstate/ctlcmd/get.go index 9781bcef4ac..719edcca4a8 100644 --- a/overlord/hookstate/ctlcmd/get.go +++ b/overlord/hookstate/ctlcmd/get.go @@ -26,6 +26,7 @@ import ( "strings" "github.com/snapcore/snapd/asserts" + "github.com/snapcore/snapd/features" "github.com/snapcore/snapd/i18n" "github.com/snapcore/snapd/interfaces" "github.com/snapcore/snapd/overlord/assertstate" @@ -173,15 +174,20 @@ func (c *getCommand) Execute(args []string) error { if snap != "" { return fmt.Errorf(`"snapctl get %s" not supported, use "snapctl get :%s" instead`, c.Positional.PlugOrSlotSpec, parts[1]) } - // registry views can be read without fields - if !c.View && len(c.Positional.Keys) == 0 { - return errors.New(i18n.G("get which attribute?")) - } if c.View { + if err := validateRegistriesFeatureFlag(context.State()); err != nil { + return err + } + requests := c.Positional.Keys return c.getRegistryValues(context, name, requests, c.Pristine) } + + if len(c.Positional.Keys) == 0 { + return errors.New(i18n.G("get which attribute?")) + } + return c.getInterfaceSetting(context, name) } @@ -462,3 +468,23 @@ var getRegistryView = func(ctx *hookstate.Context, account, registryName, viewNa return view, nil } + +// validateRegistriesFeatureFlag checks whether the registries experimental flag +// is enabled. The state should not be locked by the caller. +func validateRegistriesFeatureFlag(st *state.State) error { + st.Lock() + defer st.Unlock() + + tr := config.NewTransaction(st) + enabled, err := features.Flag(tr, features.Registries) + if err != nil && !config.IsNoOption(err) { + return fmt.Errorf(i18n.G("internal error: cannot check registries feature flag: %v"), err) + } + + if !enabled { + _, confName := features.Registries.ConfigOption() + return fmt.Errorf(i18n.G(`"registries" feature flag is disabled: set '%s' to true`), confName) + } + + return nil +} diff --git a/overlord/hookstate/ctlcmd/get_test.go b/overlord/hookstate/ctlcmd/get_test.go index c987c8cffdb..203dccbab14 100644 --- a/overlord/hookstate/ctlcmd/get_test.go +++ b/overlord/hookstate/ctlcmd/get_test.go @@ -28,6 +28,8 @@ import ( "github.com/snapcore/snapd/asserts" "github.com/snapcore/snapd/asserts/assertstest" "github.com/snapcore/snapd/dirs" + "github.com/snapcore/snapd/features" + "github.com/snapcore/snapd/i18n" "github.com/snapcore/snapd/interfaces" "github.com/snapcore/snapd/interfaces/ifacetest" "github.com/snapcore/snapd/overlord/assertstate" @@ -592,6 +594,16 @@ slots: } _, err = repo.Connect(ref, nil, nil, nil, nil, nil) c.Assert(err, IsNil) + + s.setRegistryFlag(true, c) +} + +func (s *registrySuite) setRegistryFlag(val bool, c *C) { + tr := config.NewTransaction(s.state) + _, confOption := features.Registries.ConfigOption() + err := tr.Set("core", confOption, val) + c.Assert(err, IsNil) + tr.Commit() } func (s *registrySuite) TestRegistryGetSingleView(c *C) { @@ -877,3 +889,16 @@ func (s *registrySuite) TestRegistryGetDifferentViewThanOngoingTx(c *C) { c.Check(stdout, IsNil) c.Check(stderr, IsNil) } + +func (s *registrySuite) TestRegistryExperimentalFlag(c *C) { + s.state.Lock() + s.setRegistryFlag(false, c) + s.state.Unlock() + + for _, cmd := range []string{"get", "set", "unset"} { + stdout, stderr, err := ctlcmd.Run(s.mockContext, []string{cmd, "--view", ":read-wifi"}, 0) + c.Assert(err, ErrorMatches, i18n.G(`"registries" feature flag is disabled: set 'experimental.registries' to true`)) + c.Check(stdout, IsNil) + c.Check(stderr, IsNil) + } +} diff --git a/overlord/hookstate/ctlcmd/set.go b/overlord/hookstate/ctlcmd/set.go index fa8b93a2564..f38ea72bd3b 100644 --- a/overlord/hookstate/ctlcmd/set.go +++ b/overlord/hookstate/ctlcmd/set.go @@ -108,6 +108,10 @@ func (s *setCommand) Execute(args []string) error { } if s.View { + if err := validateRegistriesFeatureFlag(context.State()); err != nil { + return err + } + opts := &clientutil.ParseConfigOptions{String: s.String, Typed: s.Typed} requests, _, err := clientutil.ParseConfigValues(s.Positional.ConfValues, opts) if err != nil { diff --git a/overlord/hookstate/ctlcmd/unset.go b/overlord/hookstate/ctlcmd/unset.go index ce1bb7ae080..f5945cc23b0 100644 --- a/overlord/hookstate/ctlcmd/unset.go +++ b/overlord/hookstate/ctlcmd/unset.go @@ -65,11 +65,11 @@ func (s *unsetCommand) Execute(args []string) error { return err } - context.Lock() - tr := configstate.ContextTransaction(context) - context.Unlock() - if !s.View { + context.Lock() + tr := configstate.ContextTransaction(context) + context.Unlock() + // unsetting options for _, confKey := range s.Positional.ConfKeys { tr.Set(context.InstanceName(), confKey, nil) @@ -77,6 +77,10 @@ func (s *unsetCommand) Execute(args []string) error { return nil } + if err := validateRegistriesFeatureFlag(context.State()); err != nil { + return err + } + // unsetting registry data if !strings.HasPrefix(s.Positional.ConfKeys[0], ":") { return fmt.Errorf(i18n.G("cannot unset registry: plug must conform to format \":\": %s"), s.Positional.ConfKeys[0]) diff --git a/overlord/registrystate/registrystate_test.go b/overlord/registrystate/registrystate_test.go index 31877872c6c..7924d184f05 100644 --- a/overlord/registrystate/registrystate_test.go +++ b/overlord/registrystate/registrystate_test.go @@ -31,11 +31,13 @@ import ( "github.com/snapcore/snapd/asserts" "github.com/snapcore/snapd/asserts/assertstest" "github.com/snapcore/snapd/dirs" + "github.com/snapcore/snapd/features" "github.com/snapcore/snapd/interfaces" "github.com/snapcore/snapd/interfaces/ifacetest" "github.com/snapcore/snapd/overlord" "github.com/snapcore/snapd/overlord/assertstate" "github.com/snapcore/snapd/overlord/assertstate/assertstatetest" + "github.com/snapcore/snapd/overlord/configstate/config" "github.com/snapcore/snapd/overlord/hookstate" "github.com/snapcore/snapd/overlord/hookstate/ctlcmd" "github.com/snapcore/snapd/overlord/hookstate/hooktest" @@ -149,6 +151,12 @@ func (s *registryTestSuite) SetUpTest(c *C) { s.devAccID = devAccKey.AccountID() s.registry = as.(*asserts.Registry).Registry() + + tr := config.NewTransaction(s.state) + _, confOption := features.Registries.ConfigOption() + err = tr.Set("core", confOption, true) + c.Assert(err, IsNil) + tr.Commit() } func (s *registryTestSuite) TestGetView(c *C) { @@ -1002,9 +1010,6 @@ func (s *registryTestSuite) checkModifyRegistryChange(c *C, chg *state.Change, h c.Assert(val, Equals, "foo") } -func (s *registryTestSuite) TestGetTransactionDifferentFromOngoingOnlyForRead(c *C) { -} - func (s *registryTestSuite) TestGetTransactionFromChangeViewHook(c *C) { ctx := s.testGetReadableOngoingTransaction(c, "change-view-setup")