From 55306fc890b9e1a09306bd5a8d6a1815361775c7 Mon Sep 17 00:00:00 2001 From: Zeyad Yasser Date: Thu, 21 Nov 2024 16:11:38 +0200 Subject: [PATCH] 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 --- daemon/api_snap_conf.go | 51 +++++++++++++++ daemon/api_snap_conf_test.go | 64 +++++++++++++++++++ overlord/configstate/config/export_test.go | 7 -- overlord/configstate/config/transaction.go | 10 +++ .../configstate/configcore/experimental.go | 19 ++++++ tests/main/old-experimental-flags/task.yaml | 37 +++++++++++ 6 files changed, 181 insertions(+), 7 deletions(-) create mode 100644 tests/main/old-experimental-flags/task.yaml 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"