daemon: hide old experimental flags (#14455)

There is window between moving an experimental flag out of
experimental and possibly having a snapd revert to a version
where the experimental flag was used.

To avoid breaking devices that depend on those flags, instead
of removing them completely from state, let's hide them instead.

Also, Avoid pruning exact queries for experimental features just in
case there are snaps out there that gate some behaviour behind a flag
check (if so, it probably gets that specific config path and not all).

Signed-off-by: Zeyad Gouda <[email protected]>

daemon/api_snap_conf.go

@@ -28,6 +28,7 @@ import (
 	"github.com/snapcore/snapd/overlord/auth"
 	"github.com/snapcore/snapd/overlord/configstate"
 	"github.com/snapcore/snapd/overlord/configstate/config"
+	"github.com/snapcore/snapd/overlord/configstate/configcore"
 	"github.com/snapcore/snapd/overlord/state"
 	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/strutil"
@@ -78,6 +79,12 @@ func getSnapConf(c *Command, r *http.Request, user *auth.UserState) Response {
 				return InternalError("%v", err)
 			}
 		}
+
+		// Hide experimental features that are no longer required because it was
+		// either accepted or rejected
+		if snapName == "core" {
+			value = pruneExperimentalFlags(key, value)
+		}
 		if key == "" {
 			if len(keys) > 1 {
 				return BadRequest("keys contains zero-length string")
@@ -91,6 +98,50 @@ func getSnapConf(c *Command, r *http.Request, user *auth.UserState) Response {
 	return SyncResponse(currentConfValues)
 }
 
+// pruneExperimentalFlags returns a copy of val with unsupported experimental
+// features removed from the experimental configuration. This applies to
+// generic queries, where the key is either an empty string ("") or "experimental".
+// Exact queries (e.g. "core.experimental.old-flag") are not pruned to avoid breaking
+// snaps that gate some behaviour behind a flag check.
+//
+// This helper should only be called for core configurations. Any errors when parsing
+// core config are ignored and val is returned without modification.
+func pruneExperimentalFlags(key string, val interface{}) interface{} {
+	if val == nil {
+		return val
+	}
+
+	if key != "" && key != "experimental" {
+		// We only care about config that might contain old experimental features
+		// and exact queries (e.g. core.experimental.old-flag) are not pruned to
+		// avoid breaking snaps that gate some behaviour behind a flag check.
+		return val
+	}
+
+	experimentalFlags, ok := val.(map[string]interface{})
+	if !ok {
+		// XXX: This should never happen, skip cleaning
+		return val
+	}
+	if key == "" {
+		experimentalFlags, ok = experimentalFlags["experimental"].(map[string]interface{})
+		if !ok {
+			// No experimental key, do nothing
+			return val
+		}
+	}
+
+	for flag := range experimentalFlags {
+		if !configcore.IsSupportedExperimentalFlag(flag) {
+			// Hide the no longer supported experimental flag
+			delete(experimentalFlags, flag)
+		}
+	}
+
+	// Changes in experimentalFlags should reflect in values
+	return val
+}
+
 func setSnapConf(c *Command, r *http.Request, user *auth.UserState) Response {
 	vars := muxVars(r)
 	snapName := configstate.RemapSnapFromRequest(vars["name"])

daemon/api_snap_conf_test.go

@@ -33,6 +33,7 @@ import (
 	"github.com/snapcore/snapd/daemon"
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/overlord/configstate/config"
+	"github.com/snapcore/snapd/overlord/configstate/configcore"
 	"github.com/snapcore/snapd/testutil"
 )
 
@@ -47,6 +48,9 @@ func (s *snapConfSuite) SetUpTest(c *check.C) {
 
 	s.expectReadAccess(daemon.AuthenticatedAccess{Polkit: "io.snapcraft.snapd.manage-configuration"})
 	s.expectWriteAccess(daemon.AuthenticatedAccess{Polkit: "io.snapcraft.snapd.manage-configuration"})
+
+	// Skip fetching external configs in testing
+	config.ClearExternalConfigMap()
 }
 
 func (s *snapConfSuite) runGetConf(c *check.C, snapName string, keys []string, statusCode int) map[string]interface{} {
@@ -110,6 +114,66 @@ func (s *snapConfSuite) TestGetConfMissingKey(c *check.C) {
 	})
 }
 
+func (s *snapConfSuite) TestGetConfCoreUnsupportedExperimentalFlag(c *check.C) {
+	d := s.daemon(c)
+
+	// We are testing that experimental features that are no longer experimental
+	// are hidden
+
+	d.Overlord().State().Lock()
+	tr := config.NewTransaction(d.Overlord().State())
+	err := tr.Set("core", "experimental.old-flag", true)
+	c.Assert(err, check.IsNil)
+	err = tr.Set("core", "experimental.supported-flag", true)
+	c.Assert(err, check.IsNil)
+	tr.Commit()
+	d.Overlord().State().Unlock()
+
+	// Simulate that experimental.old-flag is now out of experimental
+	restore := configcore.MockSupportedExperimentalFlags([]string{"supported-flag"})
+	defer restore()
+
+	// Exact query to old experimental feature are not pruned
+	result := s.runGetConf(c, "core", []string{"experimental.old-flag"}, 200)
+	c.Check(result, check.DeepEquals, map[string]interface{}{
+		"experimental.old-flag": true,
+	})
+
+	// Generic experimental features query should hide old experimental
+	// features that are no longer supported
+	result = s.runGetConf(c, "core", []string{"experimental"}, 200)
+	c.Check(result, check.DeepEquals, map[string]interface{}{
+		"experimental": map[string]interface{}{
+			"supported-flag": true,
+		},
+	})
+	// Let's only check experimental config in root document
+	result = s.runGetConf(c, "core", nil, 200)
+	result = result["experimental"].(map[string]interface{})
+	c.Check(result, check.DeepEquals, map[string]interface{}{"supported-flag": true})
+
+	// Simulate the scenario where snapd is reverted to revision
+	// that supports a hidden experimental feature
+	restore = configcore.MockSupportedExperimentalFlags([]string{"supported-flag", "old-flag"})
+	defer restore()
+
+	// Exact queries are still shown
+	result = s.runGetConf(c, "core", []string{"experimental.old-flag"}, 200)
+	c.Check(result, check.DeepEquals, map[string]interface{}{"experimental.old-flag": true})
+
+	// Generic queries should now show previously hidden experimental feature
+	result = s.runGetConf(c, "core", []string{"experimental"}, 200)
+	c.Check(result, check.DeepEquals, map[string]interface{}{
+		"experimental": map[string]interface{}{
+			"old-flag":       true,
+			"supported-flag": true,
+		},
+	})
+	result = s.runGetConf(c, "core", nil, 200)
+	result = result["experimental"].(map[string]interface{})
+	c.Check(result, check.DeepEquals, map[string]interface{}{"old-flag": true, "supported-flag": true})
+}
+
 func (s *snapConfSuite) TestGetRootDocument(c *check.C) {
 	d := s.daemon(c)
 	d.Overlord().State().Lock()

overlord/configstate/config/export_test.go

@@ -33,10 +33,3 @@ var (
 	SortPatchKeysByDepth       = sortPatchKeysByDepth
 	OverlapsWithExternalConfig = overlapsWithExternalConfig
 )
-
-func ClearExternalConfigMap() {
-	externalConfigMu.Lock()
-	defer externalConfigMu.Unlock()
-
-	externalConfigMap = nil
-}

overlord/configstate/config/transaction.go

@@ -30,6 +30,7 @@ import (
 	"sync"
 
 	"github.com/snapcore/snapd/jsonutil"
+	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/overlord/state"
 )
 
@@ -566,3 +567,12 @@ func RegisterExternalConfig(snapName, key string, vf ExternalCfgFunc) error {
 	externalConfigMap[snapName][key] = vf
 	return nil
 }
+
+// ClearExternalConfigMap must only be used for testing.
+func ClearExternalConfigMap() {
+	osutil.MustBeTestBinary("ClearExternalConfigMap only can be used in tests")
+
+	externalConfigMu.Lock()
+	defer externalConfigMu.Unlock()
+	externalConfigMap = nil
+}

overlord/configstate/configcore/experimental.go

@@ -27,6 +27,7 @@ import (
 	"github.com/snapcore/snapd/features"
 	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/sysconfig"
+	"github.com/snapcore/snapd/testutil"
 )
 
 func init() {
@@ -87,3 +88,21 @@ func doExportExperimentalFlags(_ sysconfig.Device, tr ConfGetter, opts *fsOnlyCo
 func ExportExperimentalFlags(tr ConfGetter) error {
 	return doExportExperimentalFlags(nil, tr, nil)
 }
+
+// IsSupportedExperimentalFlag checks if passed flag is a supported experimental feature.
+func IsSupportedExperimentalFlag(flag string) bool {
+	return supportedConfigurations["core.experimental."+flag]
+}
+
+// MockSupportedExperimentalFlags mocks list of supported experimental flags for use in testing.
+func MockSupportedExperimentalFlags(flags []string) (restore func()) {
+	osutil.MustBeTestBinary("MockSupportedExperimentalFlags only can be used in tests")
+
+	restore = testutil.Backup(&supportedConfigurations)
+	newConfs := make(map[string]bool, len(flags))
+	for _, flag := range flags {
+		newConfs["core.experimental."+flag] = true
+	}
+	supportedConfigurations = newConfs
+	return restore
+}

tests/main/old-experimental-flags/task.yaml

@@ -0,0 +1,37 @@
+summary: Ensure that old experimental flag configs are hidden
+
+details: |
+    Check that experimental flag configs that used to exist but now
+    are out of experimental are hidden unless specifically referenced
+    and their values will be retained to ensure it works as before in
+    case of revert to previous snapd version.
+
+prepare: |
+    snap install --devmode jq
+
+restore: |
+    snap remove jq
+
+execute: |
+    echo "Check that users cannot set unsupported experimental features"
+    snap set core experimental.old-flag=true 2>&1 | MATCH "unsupported system option"
+    snap get core experimental.old-flag | NOMATCH "true"
+
+    # Stop snapd while editing state.json manually
+    systemctl stop snapd.service snapd.socket
+
+    echo "Force setting the unsupported experimental.old-flag"
+    # This simulates the situation where an experimental feature got out
+    # of experimental after a snapd refresh and now is an unsupported config
+    jq '.data.config.core.experimental += {"old-flag": true}' /var/lib/snapd/state.json > state.json
+    mv state.json /var/lib/snapd/state.json
+    echo "Check that experimental.old-flag is persisted in state.json"
+    jq '.data.config.core.experimental."old-flag"' /var/lib/snapd/state.json | MATCH "true"
+
+    systemctl start snapd.service snapd.socket
+    echo "Old experimental flags are hidden in generic queries"
+    snap get core experimental | NOMATCH "old-flag"
+    echo "But not removed for exact queries"
+    snap get core experimental.old-flag | MATCH "true"
+    echo "Also, old flag is not removed from state in case of a revert"
+    jq '.data.config.core.experimental."old-flag"' /var/lib/snapd/state.json | MATCH "true"