diff --git a/daemon/api_snap_conf.go b/daemon/api_snap_conf.go index 07b79d08e50..ab01aaea26d 100644 --- a/daemon/api_snap_conf.go +++ b/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"]) diff --git a/daemon/api_snap_conf_test.go b/daemon/api_snap_conf_test.go index 2881d63aa9b..8fe96c1dad9 100644 --- a/daemon/api_snap_conf_test.go +++ b/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() diff --git a/overlord/configstate/config/export_test.go b/overlord/configstate/config/export_test.go index e80ac9a1948..948676cb0ec 100644 --- a/overlord/configstate/config/export_test.go +++ b/overlord/configstate/config/export_test.go @@ -33,10 +33,3 @@ var ( SortPatchKeysByDepth = sortPatchKeysByDepth OverlapsWithExternalConfig = overlapsWithExternalConfig ) - -func ClearExternalConfigMap() { - externalConfigMu.Lock() - defer externalConfigMu.Unlock() - - externalConfigMap = nil -} diff --git a/overlord/configstate/config/transaction.go b/overlord/configstate/config/transaction.go index 5599d2eebff..a67a7b5bf42 100644 --- a/overlord/configstate/config/transaction.go +++ b/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 +} diff --git a/overlord/configstate/configcore/experimental.go b/overlord/configstate/configcore/experimental.go index 125096d8f54..ab7e1784512 100644 --- a/overlord/configstate/configcore/experimental.go +++ b/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 +} diff --git a/tests/main/old-experimental-flags/task.yaml b/tests/main/old-experimental-flags/task.yaml new file mode 100644 index 00000000000..0ebd9590ddb --- /dev/null +++ b/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"