Merge pull request canonical#13516 from MiguelPires/aspect-exp-flag

many: add aspect configuration experimental feature flag

cmd/snap/cmd_get.go

@@ -27,6 +27,7 @@ import (
 
 	"github.com/jessevdk/go-flags"
 
+	"github.com/snapcore/snapd/features"
 	"github.com/snapcore/snapd/i18n"
 )
 
@@ -50,7 +51,6 @@ Nested values may be retrieved via a dotted path:
     frank
 `)
 
-// TODO: check the experimental feature is enabled and append this to get's help
 var longAspectGetHelp = i18n.G(`
 With the aspects experimental feature enabled, if the first argument passed
 into get is an aspect identifier matching the format <account-id>/<bundle>/<aspect>,
@@ -71,7 +71,11 @@ type cmdGet struct {
 }
 
 func init() {
-	addCommand("get", shortGetHelp, longGetHelp+longAspectGetHelp, func() flags.Commander { return &cmdGet{} },
+	if err := validateAspectFeatureFlag(); err == nil {
+		longGetHelp += longAspectGetHelp
+	}
+
+	addCommand("get", shortGetHelp, longGetHelp, func() flags.Commander { return &cmdGet{} },
 		map[string]string{
 			// TRANSLATORS: This should not start with a lowercase letter.
 			"d": i18n.G("Always return document, even with single key"),
@@ -252,8 +256,11 @@ func (x *cmdGet) Execute(args []string) error {
 
 	var conf map[string]interface{}
 	var err error
-	// TODO: check that the experimental aspect feature is enabled
 	if isAspectID(snapName) {
+		if err := validateAspectFeatureFlag(); err != nil {
+			return err
+		}
+
 		// first argument is an aspectID, use the aspects API
 		aspectID := snapName
 		if err := validateAspectID(aspectID); err != nil {
@@ -278,3 +285,11 @@ func (x *cmdGet) Execute(args []string) error {
 		return x.outputDefault(conf, snapName, confKeys)
 	}
 }
+
+func validateAspectFeatureFlag() error {
+	if !features.AspectsConfiguration.IsEnabled() {
+		_, confName := features.AspectsConfiguration.ConfigOption()
+		return fmt.Errorf(`aspect-based configuration is disabled: you must set '%s' to true`, confName)
+	}
+	return nil
+}

cmd/snap/cmd_get_test.go

@@ -24,6 +24,7 @@ import (
 	"net/http"
 	"strings"
 
+	"gopkg.in/check.v1"
 	. "gopkg.in/check.v1"
 
 	snapset "github.com/snapcore/snapd/cmd/snap"
@@ -226,23 +227,20 @@ func (s *SnapSuite) mockGetEmptyConfigServer(c *C) {
 	})
 }
 
-var _ = Suite(&aspectsGetSuite{})
-
-type aspectsGetSuite struct {
-	BaseSnapSuite
-}
-
 const syncResp = `{
   "type": "sync",
   "status-code": 200,
   "status": "OK",
   "result": %s
 }`
 
-func (s *aspectsGetSuite) TestAspectGet(c *C) {
+func (s *aspectsSuite) TestAspectGet(c *C) {
 	restore := snapset.MockIsStdinTTY(true)
 	defer restore()
 
+	restore = s.mockAspectsFlag(c)
+	defer restore()
+
 	var reqs int
 	s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
 		switch reqs {
@@ -273,10 +271,13 @@ func (s *aspectsGetSuite) TestAspectGet(c *C) {
 	c.Check(s.Stderr(), Equals, "")
 }
 
-func (s *aspectsGetSuite) TestAspectGetAsDocument(c *C) {
+func (s *aspectsSuite) TestAspectGetAsDocument(c *C) {
 	restore := snapset.MockIsStdinTTY(true)
 	defer restore()
 
+	restore = s.mockAspectsFlag(c)
+	defer restore()
+
 	var reqs int
 	s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
 		switch reqs {
@@ -311,10 +312,13 @@ func (s *aspectsGetSuite) TestAspectGetAsDocument(c *C) {
 	c.Check(s.Stderr(), Equals, "")
 }
 
-func (s *aspectsGetSuite) TestAspectGetMany(c *C) {
+func (s *aspectsSuite) TestAspectGetMany(c *C) {
 	restore := snapset.MockIsStdinTTY(true)
 	defer restore()
 
+	restore = s.mockAspectsFlag(c)
+	defer restore()
+
 	var reqs int
 	s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
 		switch reqs {
@@ -349,10 +353,13 @@ xyz  false
 	c.Check(s.Stderr(), Equals, "")
 }
 
-func (s *aspectsGetSuite) TestAspectGetManyAsDocument(c *C) {
+func (s *aspectsSuite) TestAspectGetManyAsDocument(c *C) {
 	restore := snapset.MockIsStdinTTY(true)
 	defer restore()
 
+	restore = s.mockAspectsFlag(c)
+	defer restore()
+
 	var reqs int
 	s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
 		switch reqs {
@@ -388,8 +395,29 @@ func (s *aspectsGetSuite) TestAspectGetManyAsDocument(c *C) {
 	c.Check(s.Stderr(), Equals, "")
 }
 
-func (s *aspectsSetSuite) TestAspectGetInvalidAspectID(c *C) {
+func (s *aspectsSuite) TestAspectGetInvalidAspectID(c *check.C) {
+	restore := s.mockAspectsFlag(c)
+	defer restore()
+
 	_, err := snapset.Parser(snapset.Client()).ParseArgs([]string{"get", "foo//bar", "foo=bar"})
 	c.Assert(err, NotNil)
 	c.Check(err.Error(), Equals, "aspect identifier must conform to format: <account-id>/<bundle>/<aspect>")
 }
+
+func (s *aspectsSuite) TestAspectGetDisabledFlag(c *check.C) {
+	var reqs int
+	s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
+		switch reqs {
+		default:
+			err := fmt.Errorf("expected to get no requests, now on %d (%v)", reqs+1, r)
+			w.WriteHeader(500)
+			fmt.Fprintf(w, `{"type": "error", "result": {"message": %q}}`, err)
+			c.Error(err)
+		}
+
+		reqs++
+	})
+
+	_, err := snapset.Parser(snapset.Client()).ParseArgs([]string{"get", "foo/bar/baz", "abc"})
+	c.Assert(err, check.ErrorMatches, "aspect-based configuration is disabled: you must set 'experimental.aspects-configuration' to true")
+}

cmd/snap/cmd_set.go

@@ -66,7 +66,10 @@ type cmdSet struct {
 }
 
 func init() {
-	addCommand("set", shortSetHelp, longSetHelp+longAspectSetHelp, func() flags.Commander { return &cmdSet{} },
+	if err := validateAspectFeatureFlag(); err == nil {
+		longSetHelp += longAspectSetHelp
+	}
+	addCommand("set", shortSetHelp, longSetHelp, func() flags.Commander { return &cmdSet{} },
 		waitDescs.also(map[string]string{
 			// TRANSLATORS: This should not start with a lowercase letter.
 			"t": i18n.G("Parse the value strictly as JSON document"),
@@ -123,8 +126,11 @@ func (x *cmdSet) Execute([]string) error {
 
 	var chgID string
 	var err error
-	// TODO: check that the experimental aspect feature is enabled
 	if isAspectID(snapName) {
+		if err := validateAspectFeatureFlag(); err != nil {
+			return err
+		}
+
 		// first argument is an aspectID, use the aspects API
 		aspectID := snapName
 		if err := validateAspectID(aspectID); err != nil {

cmd/snap/cmd_set_test.go

@@ -24,10 +24,13 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"os"
 
 	"gopkg.in/check.v1"
 
 	snapset "github.com/snapcore/snapd/cmd/snap"
+	"github.com/snapcore/snapd/dirs"
+	"github.com/snapcore/snapd/features"
 )
 
 type snapSetSuite struct {
@@ -159,10 +162,29 @@ func (s *snapSetSuite) mockSetConfigServer(c *check.C, expectedValue interface{}
 	})
 }
 
-var _ = check.Suite(&aspectsSetSuite{})
-
-type aspectsSetSuite struct {
+type aspectsSuite struct {
 	BaseSnapSuite
+	tmpDir string
+}
+
+var _ = check.Suite(&aspectsSuite{})
+
+func (s *aspectsSuite) SetUp(c *check.C) {
+	s.BaseSnapSuite.SetUpTest(c)
+	s.tmpDir = c.MkDir()
+}
+
+func (s *aspectsSuite) mockAspectsFlag(c *check.C) (restore func()) {
+	old := dirs.FeaturesDir
+	dirs.FeaturesDir = s.tmpDir
+
+	aspectsCtlFile := features.AspectsConfiguration.ControlFile()
+	c.Assert(os.WriteFile(aspectsCtlFile, []byte(nil), 0644), check.IsNil)
+
+	return func() {
+		c.Assert(os.Remove(aspectsCtlFile), check.IsNil)
+		dirs.FeaturesDir = old
+	}
 }
 
 const asyncResp = `{
@@ -171,7 +193,10 @@ const asyncResp = `{
 	"status-code": 202
 }`
 
-func (s *aspectsSetSuite) TestAspectSet(c *check.C) {
+func (s *aspectsSuite) TestAspectSet(c *check.C) {
+	restore := s.mockAspectsFlag(c)
+	defer restore()
+
 	var reqs int
 	s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
 		switch reqs {
@@ -208,7 +233,10 @@ func (s *aspectsSetSuite) TestAspectSet(c *check.C) {
 	c.Check(s.Stderr(), check.Equals, "")
 }
 
-func (s *aspectsSetSuite) TestAspectSetMany(c *check.C) {
+func (s *aspectsSuite) TestAspectSetMany(c *check.C) {
+	restore := s.mockAspectsFlag(c)
+	defer restore()
+
 	var reqs int
 	s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
 		switch reqs {
@@ -245,13 +273,19 @@ func (s *aspectsSetSuite) TestAspectSetMany(c *check.C) {
 	c.Check(s.Stderr(), check.Equals, "")
 }
 
-func (s *aspectsSetSuite) TestAspectSetInvalidAspectID(c *check.C) {
+func (s *aspectsSuite) TestAspectSetInvalidAspectID(c *check.C) {
+	restore := s.mockAspectsFlag(c)
+	defer restore()
+
 	_, err := snapset.Parser(snapset.Client()).ParseArgs([]string{"set", "foo//bar", "foo=bar"})
 	c.Assert(err, check.NotNil)
 	c.Check(err.Error(), check.Equals, "aspect identifier must conform to format: <account-id>/<bundle>/<aspect>")
 }
 
-func (s *aspectsSetSuite) TestAspectSetNoWait(c *check.C) {
+func (s *aspectsSuite) TestAspectSetNoWait(c *check.C) {
+	restore := s.mockAspectsFlag(c)
+	defer restore()
+
 	var reqs int
 	s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
 		switch reqs {
@@ -283,3 +317,21 @@ func (s *aspectsSetSuite) TestAspectSetNoWait(c *check.C) {
 	c.Check(s.Stdout(), check.Equals, "123\n")
 	c.Check(s.Stderr(), check.Equals, "")
 }
+
+func (s *aspectsSuite) TestAspectSetDisabledFlag(c *check.C) {
+	var reqs int
+	s.RedirectClientToTestServer(func(w http.ResponseWriter, r *http.Request) {
+		switch reqs {
+		default:
+			err := fmt.Errorf("expected to get no requests, now on %d (%v)", reqs+1, r)
+			w.WriteHeader(500)
+			fmt.Fprintf(w, `{"type": "error", "result": {"message": %q}}`, err)
+			c.Error(err)
+		}
+
+		reqs++
+	})
+
+	_, err := snapset.Parser(snapset.Client()).ParseArgs([]string{"set", "foo/bar/baz", "abc=1"})
+	c.Assert(err, check.ErrorMatches, "aspect-based configuration is disabled: you must set 'experimental.aspects-configuration' to true")
+}

daemon/api_aspects.go

@@ -25,8 +25,10 @@ import (
 	"net/http"
 
 	"github.com/snapcore/snapd/aspects"
+	"github.com/snapcore/snapd/features"
 	"github.com/snapcore/snapd/overlord/aspectstate"
 	"github.com/snapcore/snapd/overlord/auth"
+	"github.com/snapcore/snapd/overlord/configstate/config"
 	"github.com/snapcore/snapd/overlord/state"
 	"github.com/snapcore/snapd/strutil"
 )
@@ -42,7 +44,14 @@ var (
 )
 
 func getAspect(c *Command, r *http.Request, _ *auth.UserState) Response {
-	// TODO: check that the experimental aspects feature is enabled
+	st := c.d.state
+	st.Lock()
+	defer st.Unlock()
+
+	if err := validateAspectFeatureFlag(st); err != nil {
+		return err
+	}
+
 	vars := muxVars(r)
 	account, bundleName, aspect := vars["account"], vars["bundle"], vars["aspect"]
 	fields := strutil.CommaSeparatedList(r.URL.Query().Get("fields"))
@@ -51,10 +60,6 @@ func getAspect(c *Command, r *http.Request, _ *auth.UserState) Response {
 		return BadRequest("missing aspect fields")
 	}
 
-	st := c.d.state
-	st.Lock()
-	defer st.Unlock()
-
 	tx, err := aspectstate.NewTransaction(st, account, bundleName)
 	if err != nil {
 		return toAPIError(err)
@@ -85,7 +90,14 @@ func getAspect(c *Command, r *http.Request, _ *auth.UserState) Response {
 }
 
 func setAspect(c *Command, r *http.Request, _ *auth.UserState) Response {
-	// TODO: check that the experimental aspects feature is enabled
+	st := c.d.state
+	st.Lock()
+	defer st.Unlock()
+
+	if err := validateAspectFeatureFlag(st); err != nil {
+		return err
+	}
+
 	vars := muxVars(r)
 	account, bundleName, aspect := vars["account"], vars["bundle"], vars["aspect"]
 
@@ -95,10 +107,6 @@ func setAspect(c *Command, r *http.Request, _ *auth.UserState) Response {
 		return BadRequest("cannot decode aspect request body: %v", err)
 	}
 
-	st := c.d.state
-	st.Lock()
-	defer st.Unlock()
-
 	tx, err := aspectstate.NewTransaction(st, account, bundleName)
 	if err != nil {
 		return toAPIError(err)
@@ -136,3 +144,17 @@ func toAPIError(err error) *apiError {
 		return InternalError(err.Error())
 	}
 }
+
+func validateAspectFeatureFlag(st *state.State) *apiError {
+	tr := config.NewTransaction(st)
+	enabled, err := features.Flag(tr, features.AspectsConfiguration)
+	if err != nil && !config.IsNoOption(err) {
+		return InternalError(fmt.Sprintf("internal error: cannot check aspect configuration flag: %s", err))
+	}
+
+	if !enabled {
+		_, confName := features.AspectsConfiguration.ConfigOption()
+		return BadRequest(fmt.Sprintf("aspect-based configuration disabled: you must set '%s' to true", confName))
+	}
+	return nil
+}