diff --git a/interfaces/repo.go b/interfaces/repo.go index 93924d7b175..96f6470726e 100644 --- a/interfaces/repo.go +++ b/interfaces/repo.go @@ -27,6 +27,7 @@ import ( "github.com/snapcore/snapd/interfaces/hotplug" "github.com/snapcore/snapd/interfaces/utils" + "github.com/snapcore/snapd/osutil" "github.com/snapcore/snapd/snap" ) @@ -64,6 +65,17 @@ func NewRepository() *Repository { return repo } +func ResetRepository(repo *Repository) { + osutil.MustBeTestBinary("cannot use the ResetRepository method outside of tests") + repo.ifaces = make(map[string]Interface) + repo.hotplugIfaces = make(map[string]Interface) + repo.plugs = make(map[string]map[string]*snap.PlugInfo) + repo.slots = make(map[string]map[string]*snap.SlotInfo) + repo.slotPlugs = make(map[*snap.SlotInfo]map[*snap.PlugInfo]*Connection) + repo.plugSlots = make(map[*snap.PlugInfo]map[*snap.SlotInfo]*Connection) + repo.appSets = make(map[string]*SnapAppSet) +} + // Interface returns an interface with a given name. func (r *Repository) Interface(interfaceName string) Interface { r.m.Lock() diff --git a/overlord/configstate/configcore/refresh.go b/overlord/configstate/configcore/refresh.go index 0dd3819e5ea..037b3a7b189 100644 --- a/overlord/configstate/configcore/refresh.go +++ b/overlord/configstate/configcore/refresh.go @@ -46,10 +46,10 @@ func init() { } func reportOrIgnoreInvalidManageRefreshes(tr RunTransaction, optName string) error { - // check if the option is set as part of transaction changes; if not than - // it's already set in the config state and we shouldn't error out about it - // now. refreshScheduleManaged will do the right thing when refresh cannot - // be managed anymore. + // check if the option is set as part of transaction changes; if not + // than it's already set in the config state and we shouldn't error out + // about it now since the required conditions were met at the time it + // got set for _, k := range tr.Changes() { if k == "core."+optName { return fmt.Errorf("cannot set schedule to managed") @@ -109,6 +109,8 @@ func validateRefreshSchedule(tr RunTransaction) error { st.Lock() defer st.Unlock() + // ensure there is a snap entitled to managing the refreshes + // in an autonomic manner if !devicestate.CanManageRefreshes(st) { return reportOrIgnoreInvalidManageRefreshes(tr, "refresh.timer") } @@ -136,6 +138,7 @@ func validateRefreshSchedule(tr RunTransaction) error { st.Lock() defer st.Unlock() + // see the comment around refresh.timer if !devicestate.CanManageRefreshes(st) { return reportOrIgnoreInvalidManageRefreshes(tr, "refresh.schedule") } diff --git a/overlord/devicestate/devicestate.go b/overlord/devicestate/devicestate.go index 3905e229347..d573cbb731f 100644 --- a/overlord/devicestate/devicestate.go +++ b/overlord/devicestate/devicestate.go @@ -270,7 +270,6 @@ func delayedCrossMgrInit() { snapstate.AddCheckSnapCallback(checkGadgetRemodelCompatible) }) snapstate.CanAutoRefresh = canAutoRefresh - snapstate.CanManageRefreshes = CanManageRefreshes snapstate.IsOnMeteredConnection = netutil.IsOnMeteredConnection snapstate.DeviceCtx = DeviceCtx snapstate.RemodelingChange = RemodelingChange @@ -307,15 +306,14 @@ func interfaceConnected(st *state.State, snapName, ifName string) bool { return err == nil && len(conns) > 0 } -// CanManageRefreshes returns true if the device can be -// switched to the "core.refresh.schedule=managed" mode. +// CanManageRefreshes returns true if a snap entitled to setting the +// refresh-schedule to managed is installed in the system and the relevant +// interface is currently connected. // // TODO: // - Move the CanManageRefreshes code into the ifstate // - Look at the connections and find the connection for snapd-control // with the managed attribute -// - Take the snap from this connection and look at the snapstate to see -// if that snap has a snap declaration (to ensure it comes from the store) func CanManageRefreshes(st *state.State) bool { snapStates, err := snapstate.All(st) if err != nil { diff --git a/overlord/managers_test.go b/overlord/managers_test.go index 4574e5a379d..0a5b0e5cd38 100644 --- a/overlord/managers_test.go +++ b/overlord/managers_test.go @@ -39,6 +39,7 @@ import ( "sort" "strconv" "strings" + "sync" "time" . "gopkg.in/check.v1" @@ -148,6 +149,8 @@ type baseMgrsSuite struct { automaticSnapshots []automaticSnapshotCall logbuf *bytes.Buffer + + storeObserver func(r *http.Request) } var ( @@ -1062,6 +1065,10 @@ func (s *baseMgrsSuite) mockStore(c *C) *httptest.Server { } mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if s.storeObserver != nil { + s.storeObserver(r) + } + // all URLS are /api/v1/snaps/... or /v2/snaps/ or /v2/assertions/... so // check the url is sane and discard the common prefix // to simplify indexing into the comps slice. @@ -14379,3 +14386,262 @@ type: snapd` c.Assert(err, Equals, snapstate.ErrUnexpectedRuntimeRestart) }() } + +var snapWithSnapdControlRefreshScheduleManagedYAML = ` +name: snap-with-snapd-control +version: 1.0 +plugs: + snapd-control: + refresh-schedule: managed +` + +var coreWithSnapdControlOnlyYAML = ` +name: core +version: 1.0 +slots: + snapd-control: +` + +func makeMockRepoWithConnectedSnaps(c *C, repo *interfaces.Repository, info11, core11 *snap.Info, ifname string) { + info11AppSet, err := interfaces.NewSnapAppSet(info11, nil) + c.Assert(err, IsNil) + + err = repo.AddAppSet(info11AppSet) + c.Assert(err, IsNil) + + core11AppSet, err := interfaces.NewSnapAppSet(core11, nil) + c.Assert(err, IsNil) + + err = repo.AddAppSet(core11AppSet) + c.Assert(err, IsNil) + + _, err = repo.Connect(&interfaces.ConnRef{ + PlugRef: interfaces.PlugRef{Snap: info11.InstanceName(), Name: ifname}, + SlotRef: interfaces.SlotRef{Snap: core11.InstanceName(), Name: ifname}, + }, nil, nil, nil, nil, nil) + c.Assert(err, IsNil) + conns, err := repo.Connected(info11.RealName, ifname) + c.Assert(err, IsNil) + c.Assert(conns, HasLen, 1) +} + +func (s *mgrsSuite) testConnectionDurabilityDuringRefreshesAndAutoRefresh(c *C, hasPendingSecurityProfiles bool) { + // This test exists to verify that any disruptions of a refresh + // will maintain any pre-existing connection that was held before + // the snap is marked inactive. The goal of this test is to run all + // tasks that involve something making the snap unavailable/not active + // and then simulating a reboot inside the interface manager. We then + // re-verify the connection and see that functionality used return + // the expected values. The interface used for testing is snapd-control + // with the managed refresh schedule attribute. + // + // This was selected based on a customer case. Prior to version 2.58 this + // would cause the connection to be dropped, if a well-timed restart of snapd + // would occur during a refresh. In 2.58 a fix for this was merged, and this + // test should be passing. + mockServer := s.mockStore(c) + defer mockServer.Close() + + canAutoRefreshCalls := 0 + snapstate.CanAutoRefresh = func(*state.State) (bool, error) { + canAutoRefreshCalls++ + return true, nil + } + + // prevent catalog refresh + c.Assert(os.MkdirAll(dirs.SnapCacheDir, 0755), IsNil) + c.Assert(os.WriteFile(dirs.SnapNamesFile, nil, 0644), IsNil) + + st := s.o.State() + st.Lock() + defer st.Unlock() + + err := assertstate.Add(st, s.devAcct) + c.Assert(err, IsNil) + + var reqsLock sync.Mutex + var reqs []string + s.storeObserver = func(r *http.Request) { + c.Logf("request %v\n", r) + reqsLock.Lock() + defer reqsLock.Unlock() + reqs = append(reqs, r.URL.String()) + } + + snapNames := map[string]string{ + "snap-with-snapd-control": snapWithSnapdControlRefreshScheduleManagedYAML, + "core": coreWithSnapdControlOnlyYAML, + } + snapInfos := make(map[string]*snap.Info) + for name, yaml := range snapNames { + rev := snap.R(1) + if name != "core" { + snapDecl := s.prereqSnapAssertions(c, map[string]interface{}{ + "snap-name": name, + "format": "1", + "plugs": map[string]interface{}{ + "snapd-control": map[string]interface{}{ + "allow-installation": "true", + "auto-connect": "true", + "refresh-schedule": "managed", + }, + }, + }) + err = assertstate.Add(st, snapDecl) + c.Assert(err, IsNil) + } + snapInfos[name] = s.mockInstalledSnapWithRevAndFiles(c, yaml, rev, nil) + } + + // now add another snap revisions of "snap-with-snapd-control" + snapPath, _ := s.makeStoreTestSnap(c, snapWithSnapdControlRefreshScheduleManagedYAML, "2") + s.serveSnap(snapPath, "2") + + makeMockRepoWithConnectedSnaps(c, s.o.InterfaceManager().Repository(), snapInfos["snap-with-snapd-control"], snapInfos["core"], "snapd-control") + c.Check(devicestate.CanManageRefreshes(st), Equals, true) + + // setup connection in state to emulate that we have a snap installed with an active + // connection. + st.Set("conns", map[string]interface{}{ + "snap-with-snapd-control:snapd-control core:snapd-control": map[string]interface{}{ + "interface": "snapd-control", + "auto": true, + }, + }) + + // set config to have refresh schedule as managed. + tr := config.NewTransaction(st) + c.Assert(tr.Set("core", "refresh.schedule", "managed"), IsNil) + tr.Commit() + + chg := st.NewChange("update many change", "update change") + affected, tts, err := snapstate.UpdateMany(context.Background(), st, []string{"snap-with-snapd-control"}, nil, 0, nil) + c.Assert(err, IsNil) + c.Check(tts, HasLen, 2) + c.Check(affected, DeepEquals, []string{"snap-with-snapd-control"}) + + // Run only up until setup-profiles. We do not want to run past here, but simulate that a snapd + // restart occurs during a snap upgrade. + for _, t := range tts[0].Tasks() { + switch t.Kind() { + case "prerequisites", "download-snap", "validate-snap", "mount-snap", "stop-snap-services", "remove-aliases", "unlink-current-snap", "copy-snap-data", "run-hook", "setup-profiles": + if t.Kind() == "run-hook" && !strings.Contains(t.Summary(), "pre-refresh") { + continue + } + chg.AddTask(t) + } + } + dumpTasks(c, "task sets", tts[0].Tasks()) + dumpTasks(c, "tasks added to change", chg.Tasks()) + + st.Unlock() + err = s.o.Settle(settleTimeout) + st.Lock() + c.Assert(err, IsNil) + + c.Logf("requests: %v", reqs) + dumpTasks(c, "after settle", chg.Tasks()) + + // we hit the store for installation of the snap + c.Assert(len(reqs) > 0, Equals, true) + // and we hit the auto refresh path + c.Assert(canAutoRefreshCalls > 0, Equals, true) + + // check connections are fine after running the expected task-set. + var conns map[string]interface{} + st.Get("conns", &conns) + c.Logf("connections: %v", conns) + c.Assert(conns, DeepEquals, map[string]interface{}{ + "snap-with-snapd-control:snapd-control core:snapd-control": map[string]interface{}{ + "interface": "snapd-control", + "auto": true, + }, + }) + + var snapst snapstate.SnapState + err = snapstate.Get(st, "snap-with-snapd-control", &snapst) + c.Assert(err, IsNil) + if hasPendingSecurityProfiles { + c.Check(snapst.PendingSecurity, NotNil) + } else { + // simulate a pre 2.58 behavior, where there was no pending + // security profiles + snapst.PendingSecurity = nil + snapstate.Set(st, "snap-with-snapd-control", &snapst) + } + + // Simulate a restart in the interface manager, to emulate a restart of snapd + // has occurred and the startup code of interface manager runs again. In versions prior + // to 2.58 of snapd, this could result in interfaces being dropped as updating snaps that + // had not run "setup-profiles" and were marked inactive would not be added to the inteface + // repository. + ifmgr := s.o.InterfaceManager() + c.Assert(ifmgr, NotNil) + interfaces.ResetRepository(ifmgr.Repository()) + + st.Unlock() + err = ifmgr.StartUp() + st.Lock() + c.Assert(err, IsNil) + + // The connection state should not change + st.Get("conns", &conns) + c.Assert(conns, DeepEquals, map[string]interface{}{ + "snap-with-snapd-control:snapd-control core:snapd-control": map[string]interface{}{ + "interface": "snapd-control", + "auto": true, + }, + }) + + // because of a bug fixed in 2.58, the repository may or may not + // correctly reflect the connection state, but the test is also rigged + // to not execute auto-connect, which would restore the connection in + // the scenario when pending security profiles were not carried + ref, err := s.o.InterfaceManager().Repository().Connected("snap-with-snapd-control", "snapd-control") + if hasPendingSecurityProfiles { + c.Assert(err, IsNil) + // with pending security profiles the repo information is correctly restored + c.Assert(len(ref) > 0, Equals, true, Commentf("connection not restored to interfaces repo")) + // CanManageRefreshes must return true with the >= 2.58 fixes + c.Check(devicestate.CanManageRefreshes(st), Equals, true) + } else { + // but without the connection present in the state is not + // reflected by the repo + c.Assert(err, ErrorMatches, `snap "snap-with-snapd-control" has no .* "snapd-control"`) + c.Assert(ref, HasLen, 0) + // but returns false without the fix + c.Check(devicestate.CanManageRefreshes(st), Equals, false) + } + + // reset store requests and auto refresh call count + reqs = nil + canAutoRefreshCalls = 0 + st.Unlock() + c.Logf("-- calling ensure") + err = s.o.SnapManager().Ensure() + st.Lock() + c.Assert(err, IsNil) + + // no requests to the store + c.Assert(reqs, HasLen, 0) + // but we hit the auto refresh path + c.Assert(canAutoRefreshCalls > 0, Equals, true) + + // The refresh schedule must report managed + t1, t2, err := s.o.SnapManager().RefreshSchedule() + c.Assert(err, IsNil) + c.Check(t1, Equals, "managed") + c.Check(t2, Equals, true) +} + +func (s *mgrsSuite) TestConnectionDurabilityDuringRefreshesAndAutoRefresh(c *C) { + const hasPendingSecurityProfiles = true + s.testConnectionDurabilityDuringRefreshesAndAutoRefresh(c, hasPendingSecurityProfiles) +} + +func (s *mgrsSuite) TestOldNoConnectionDurabilityAndAutoRefresh(c *C) { + // simulate snapd < 2.58 where pending security profiles were not kept + // in the state + const hasPendingSecurityProfiles = false + s.testConnectionDurabilityDuringRefreshesAndAutoRefresh(c, hasPendingSecurityProfiles) +} diff --git a/overlord/snapstate/autorefresh.go b/overlord/snapstate/autorefresh.go index 0048f356c8b..9fca183946d 100644 --- a/overlord/snapstate/autorefresh.go +++ b/overlord/snapstate/autorefresh.go @@ -61,7 +61,6 @@ const defaultMaxInhibitionDays = 14 // hooks setup by devicestate var ( CanAutoRefresh func(st *state.State) (bool, error) - CanManageRefreshes func(st *state.State) bool IsOnMeteredConnection func() (bool, error) defaultRefreshSchedule = func() []*timeutil.Schedule { @@ -131,7 +130,6 @@ type autoRefresh struct { lastRefreshSchedule string nextRefresh time.Time lastRefreshAttempt time.Time - managedDeniedLogged bool restoredMonitoring bool } @@ -524,25 +522,12 @@ func (m *autoRefresh) refreshScheduleWithDefaultsFallback() (sched []*timeutil.S // user requests refreshes to be managed by an external snap if scheduleConf == "managed" { - if CanManageRefreshes == nil || !CanManageRefreshes(m.state) { - // there's no snap to manage refreshes so use default schedule - if !m.managedDeniedLogged { - logger.Noticef("managed refresh schedule denied, no properly configured snapd-control") - m.managedDeniedLogged = true - } - - return defaultRefreshSchedule, defaultRefreshScheduleStr, false, nil - } - if m.lastRefreshSchedule != "managed" { logger.Noticef("refresh is managed via the snapd-control interface") m.lastRefreshSchedule = "managed" } - m.managedDeniedLogged = false - return nil, "managed", legacy, nil } - m.managedDeniedLogged = false if scheduleConf == "" { return defaultRefreshSchedule, defaultRefreshScheduleStr, false, nil diff --git a/overlord/snapstate/autorefresh_test.go b/overlord/snapstate/autorefresh_test.go index a50575864a5..21f10b143a2 100644 --- a/overlord/snapstate/autorefresh_test.go +++ b/overlord/snapstate/autorefresh_test.go @@ -173,11 +173,6 @@ func (s *autoRefreshTestSuite) TestLastRefresh(c *C) { } func (s *autoRefreshTestSuite) TestLastRefreshRefreshManaged(c *C) { - snapstate.CanManageRefreshes = func(st *state.State) bool { - return true - } - defer func() { snapstate.CanManageRefreshes = nil }() - logbuf, restore := logger.MockLogger() defer restore() @@ -220,11 +215,6 @@ func (s *autoRefreshTestSuite) TestLastRefreshRefreshManaged(c *C) { } func (s *autoRefreshTestSuite) TestRefreshManagedTimerWins(c *C) { - snapstate.CanManageRefreshes = func(st *state.State) bool { - return true - } - defer func() { snapstate.CanManageRefreshes = nil }() - s.state.Lock() defer s.state.Unlock() @@ -248,18 +238,7 @@ func (s *autoRefreshTestSuite) TestRefreshManagedTimerWins(c *C) { c.Check(err, IsNil) } -func (s *autoRefreshTestSuite) TestRefreshManagedDenied(c *C) { - canManageCalled := false - snapstate.CanManageRefreshes = func(st *state.State) bool { - canManageCalled = true - // always deny - return false - } - defer func() { snapstate.CanManageRefreshes = nil }() - - logbuf, restore := logger.MockLogger() - defer restore() - +func (s *autoRefreshTestSuite) TestRefreshManagedIsRespected(c *C) { s.state.Lock() defer s.state.Unlock() @@ -275,23 +254,11 @@ func (s *autoRefreshTestSuite) TestRefreshManagedDenied(c *C) { err := af.Ensure() s.state.Lock() c.Check(err, IsNil) - c.Check(s.store.ops, DeepEquals, []string{"list-refresh"}) - - refreshScheduleStr, _, err := af.RefreshSchedule() - c.Check(refreshScheduleStr, Equals, snapstate.DefaultRefreshSchedule) - c.Check(err, IsNil) - c.Check(canManageCalled, Equals, true) - count := strings.Count(logbuf.String(), - ": managed refresh schedule denied, no properly configured snapd-control\n") - c.Check(count, Equals, 1, Commentf("too many occurrences:\n%s", logbuf.String())) - - canManageCalled = false + c.Check(s.store.ops, HasLen, 0) } // ensure clean config for the next run s.state.Set("config", nil) - logbuf.Reset() - canManageCalled = false } } diff --git a/overlord/snapstate/refreshhints.go b/overlord/snapstate/refreshhints.go index 1a6949df0bc..682c7034c1a 100644 --- a/overlord/snapstate/refreshhints.go +++ b/overlord/snapstate/refreshhints.go @@ -73,7 +73,7 @@ func (r *refreshHints) needsUpdate() (bool, error) { func (r *refreshHints) refresh() error { scheduleConf, _, _ := getRefreshScheduleConf(r.state) - refreshManaged := scheduleConf == "managed" && CanManageRefreshes(r.state) + refreshManaged := scheduleConf == "managed" var err error perfTimings := timings.New(map[string]string{"ensure": "refresh-hints"}) diff --git a/overlord/snapstate/refreshhints_test.go b/overlord/snapstate/refreshhints_test.go index 31bc2686f5d..8d03381dd53 100644 --- a/overlord/snapstate/refreshhints_test.go +++ b/overlord/snapstate/refreshhints_test.go @@ -48,6 +48,7 @@ type recordingStore struct { storetest.Store ops []string + opOpts []store.RefreshOptions refreshedSnaps []*snap.Info } @@ -66,7 +67,13 @@ func (r *recordingStore) SnapAction(ctx context.Context, currentSnaps []*store.C panic("expected refresh actions") } } + actionOpts := store.RefreshOptions{} + if opts != nil { + actionOpts = *opts + } + r.ops = append(r.ops, "list-refresh") + r.opOpts = append(r.opOpts, actionOpts) res := []store.SnapActionResult{} for _, rs := range r.refreshedSnaps { @@ -126,11 +133,34 @@ func (s *refreshHintsTestSuite) SetUpTest(c *C) { }) } -func (s *refreshHintsTestSuite) TestLastRefresh(c *C) { +func (s *refreshHintsTestSuite) TestListRefresh(c *C) { rh := snapstate.NewRefreshHints(s.state) err := rh.Ensure() c.Check(err, IsNil) c.Check(s.store.ops, DeepEquals, []string{"list-refresh"}) + c.Check(s.store.opOpts, DeepEquals, []store.RefreshOptions{ + {PrivacyKey: "privacy-key"}, + }) +} + +func (s *refreshHintsTestSuite) TestListRefreshReportsManaged(c *C) { + s.state.Lock() + defer s.state.Unlock() + + tr := config.NewTransaction(s.state) + err := tr.Set("core", "refresh.timer", "managed") + c.Assert(err, IsNil) + tr.Commit() + + s.state.Unlock() + rh := snapstate.NewRefreshHints(s.state) + err = rh.Ensure() + s.state.Lock() + c.Check(err, IsNil) + c.Check(s.store.ops, DeepEquals, []string{"list-refresh"}) + c.Check(s.store.opOpts, DeepEquals, []store.RefreshOptions{ + {RefreshManaged: true, PrivacyKey: "privacy-key"}, + }) } func (s *refreshHintsTestSuite) TestLastRefreshNoRefreshNeeded(c *C) { diff --git a/overlord/snapstate/snapstate_test.go b/overlord/snapstate/snapstate_test.go index 7c5fbef413a..c7706777573 100644 --- a/overlord/snapstate/snapstate_test.go +++ b/overlord/snapstate/snapstate_test.go @@ -4225,13 +4225,6 @@ func (s *snapmgrTestSuite) testEnsureRefreshesDisabledViaSnapdControl(c *C, conf }) defer restore() - // pretend the device is refresh-control: managed - oldCanManageRefreshes := snapstate.CanManageRefreshes - snapstate.CanManageRefreshes = func(*state.State) bool { - return true - } - defer func() { snapstate.CanManageRefreshes = oldCanManageRefreshes }() - tr := config.NewTransaction(st) confSet(tr) tr.Commit()