From 905260aa1fd76185e75766ccf1d73ab985e1102f Mon Sep 17 00:00:00 2001 From: Andrew Phelps Date: Tue, 24 Sep 2024 13:08:33 -0400 Subject: [PATCH] o/snapstate: consider validation sets when installing/refreshing components with snaps --- overlord/snapstate/backend_test.go | 32 +- overlord/snapstate/snapstate_install_test.go | 370 +++++++++++++++++++ overlord/snapstate/snapstate_test.go | 2 + overlord/snapstate/storehelpers.go | 38 +- overlord/snapstate/target.go | 140 ++++++- 5 files changed, 560 insertions(+), 22 deletions(-) diff --git a/overlord/snapstate/backend_test.go b/overlord/snapstate/backend_test.go index 172fc7ac893..161049df17e 100644 --- a/overlord/snapstate/backend_test.go +++ b/overlord/snapstate/backend_test.go @@ -196,6 +196,16 @@ type fakeStore struct { snapResourcesFn func(*snap.Info) []store.SnapResourceResult downloadCallback func() + + namesToAssertedIDs map[string]string + idsToNames map[string]string + + mutateSnapInfo func(*snap.Info) error +} + +func (f *fakeStore) registerID(name, id string) { + f.namesToAssertedIDs[name] = id + f.idsToNames[id] = name } func (f *fakeStore) snapResources(info *snap.Info) []store.SnapResourceResult { @@ -262,7 +272,12 @@ func (f *fakeStore) snap(spec snapSpec) (*snap.Info, error) { typ := snap.TypeApp epoch := snap.E("1*") + snapID := spec.Name + "-id" + if id, ok := f.namesToAssertedIDs[spec.Name]; ok { + snapID = id + } + switch spec.Name { case "core", "core16", "ubuntu-core", "some-core": typ = snap.TypeOS @@ -421,6 +436,12 @@ func (f *fakeStore) snap(spec snapSpec) (*snap.Info, error) { info.SnapProvenance = "prov" } + if f.mutateSnapInfo != nil { + if err := f.mutateSnapInfo(info); err != nil { + return nil, err + } + } + return info, nil } @@ -534,7 +555,10 @@ func (f *fakeStore) lookupRefresh(cand refreshCand) (*snap.Info, error) { name = "kernel-snap-with-components" typ = snap.TypeKernel default: - panic(fmt.Sprintf("refresh: unknown snap-id: %s", cand.snapID)) + name = f.idsToNames[cand.snapID] + if name == "" { + panic(fmt.Sprintf("refresh: unknown snap-id: %s", cand.snapID)) + } } revno := snap.R(11) @@ -648,6 +672,12 @@ func (f *fakeStore) lookupRefresh(cand refreshCand) (*snap.Info, error) { } } + if f.mutateSnapInfo != nil { + if err := f.mutateSnapInfo(info); err != nil { + return nil, err + } + } + var hit snap.Revision if cand.revision != revno { hit = revno diff --git a/overlord/snapstate/snapstate_install_test.go b/overlord/snapstate/snapstate_install_test.go index 968b617827c..feeef29de67 100644 --- a/overlord/snapstate/snapstate_install_test.go +++ b/overlord/snapstate/snapstate_install_test.go @@ -4753,6 +4753,33 @@ func (s *validationSetsSuite) TestInstallManyRequiredForValidationSetOK(c *C) { c.Assert(s.fakeBackend.ops[1:], DeepEquals, expectedOps) } +func (s *validationSetsSuite) TestInstallManyRequiredForValidationSetWithOptionalRevisionOK(c *C) { + err := s.installManySnapReferencedByValidationSet(c, "required", "", "optional", "12") + c.Assert(err, IsNil) + + c.Assert(s.fakeBackend.ops, HasLen, 3) + expectedOps := fakeOps{{ + op: "storesvc-snap-action:action", + action: store.SnapAction{ + Action: "install", + InstanceName: "one", + Channel: "stable", + ValidationSets: []snapasserts.ValidationSetKey{"16/foo/bar/1"}, + }, + revno: snap.R(11), + }, { + op: "storesvc-snap-action:action", + action: store.SnapAction{ + Action: "install", + InstanceName: "two", + Revision: snap.R(12), + ValidationSets: []snapasserts.ValidationSetKey{"16/foo/bar/1"}, + }, + revno: snap.R(12), + }} + c.Assert(s.fakeBackend.ops[1:], DeepEquals, expectedOps) +} + func (s *validationSetsSuite) TestInstallManyRequiredRevisionForValidationSetOK(c *C) { err := s.installManySnapReferencedByValidationSet(c, "required", "11", "required", "2") c.Assert(err, IsNil) @@ -7387,3 +7414,346 @@ func (s *snapmgrTestSuite) TestInstallWithRegistry(c *C) { checkSnapsupHasRegistry(ts, c) } + +type testInstallComponentsValidationSetsOpts struct { + comps []string + err string + headers map[string]interface{} +} + +func (s *validationSetsSuite) TestInstallComponentsValidationSetsInvalid(c *C) { + s.testInstallComponentsValidationSets(c, testInstallComponentsValidationSetsOpts{ + comps: []string{"test-component"}, + err: `cannot install component "test-snap\+test-component" due to enforcing rules of validation set 16/foo/bar/1`, + headers: map[string]interface{}{ + "components": map[string]interface{}{ + "test-component": "invalid", + }, + }, + }) +} + +func (s *validationSetsSuite) TestInstallComponentsValidationSetsMissingRequired(c *C) { + s.testInstallComponentsValidationSets(c, testInstallComponentsValidationSetsOpts{ + comps: nil, + err: `cannot install snap "test-snap" without component "test-component" required by validation sets 16/foo/bar/1`, + headers: map[string]interface{}{ + "components": map[string]interface{}{ + "test-component": "required", + }, + }, + }) +} + +func (s *validationSetsSuite) TestInstallComponentsValidationSetsWrongRevision(c *C) { + s.testInstallComponentsValidationSets(c, testInstallComponentsValidationSetsOpts{ + comps: []string{"test-component"}, + err: `cannot install component "test-snap\+test-component" at revision 1 without --ignore-validation, revision 10 is required by validation sets: 16/foo/bar/1`, + headers: map[string]interface{}{ + "revision": "7", + "components": map[string]interface{}{ + "test-component": map[string]interface{}{ + "presence": "optional", + "revision": "10", + }, + }, + }, + }) +} + +func (s *validationSetsSuite) TestInstallComponentsValidationSetsCorrectRevision(c *C) { + s.testInstallComponentsValidationSets(c, testInstallComponentsValidationSetsOpts{ + comps: []string{"test-component"}, + headers: map[string]interface{}{ + "revision": "7", + "components": map[string]interface{}{ + "test-component": map[string]interface{}{ + "presence": "optional", + "revision": "1", + }, + }, + }, + }) +} + +func (s *validationSetsSuite) TestInstallComponentsValidationSetsRequired(c *C) { + s.testInstallComponentsValidationSets(c, testInstallComponentsValidationSetsOpts{ + comps: []string{"test-component"}, + headers: map[string]interface{}{ + "components": map[string]interface{}{ + "test-component": map[string]interface{}{ + "presence": "required", + }, + }, + }, + }) +} + +func (s *validationSetsSuite) testInstallComponentsValidationSets(c *C, opts testInstallComponentsValidationSetsOpts) { + s.state.Lock() + defer s.state.Unlock() + + const ( + snapName = "test-snap" + channel = "channel-for-components" + ) + + snapID := snaptest.AssertedSnapID(snapName) + + // make sure the fake store knows to use this id, since this is what will be + // in the validation set + s.fakeStore.registerID(snapName, snapID) + s.fakeStore.mutateSnapInfo = func(info *snap.Info) error { + info.Components = map[string]*snap.Component{ + "test-component": { + Type: snap.TestComponent, + Name: "test-component", + }, + } + return nil + } + + restore := snapstate.MockEnforcedValidationSets(func(st *state.State, extraVss ...*asserts.ValidationSet) (*snapasserts.ValidationSets, error) { + vs := snapasserts.NewValidationSets() + headers := map[string]interface{}{ + "id": snapID, + "name": snapName, + "presence": "optional", + } + for k, v := range opts.headers { + headers[k] = v + } + vsa := s.mockValidationSetAssert(c, "bar", "1", headers) + err := vs.Add(vsa.(*asserts.ValidationSet)) + c.Assert(err, IsNil) + return vs, nil + }) + defer restore() + + s.fakeStore.snapResourcesFn = func(info *snap.Info) []store.SnapResourceResult { + c.Assert(info.InstanceName(), DeepEquals, snapName) + return []store.SnapResourceResult{ + { + DownloadInfo: snap.DownloadInfo{ + DownloadURL: "http://example.com/test-component", + }, + Name: "test-component", + Revision: 1, + Type: "component/test", + Version: "1.0", + CreatedAt: "2024-01-01T00:00:00Z", + }, + } + } + + goal := snapstate.StoreInstallGoal(snapstate.StoreSnap{ + InstanceName: snapName, + Components: opts.comps, + RevOpts: snapstate.RevisionOptions{Channel: channel}, + }) + + _, _, err := snapstate.InstallOne(context.Background(), s.state, goal, snapstate.Options{}) + if len(opts.err) == 0 { + c.Assert(err, IsNil) + } else { + c.Assert(err, ErrorMatches, opts.err) + + goal := snapstate.StoreInstallGoal(snapstate.StoreSnap{ + InstanceName: snapName, + Components: opts.comps, + RevOpts: snapstate.RevisionOptions{Channel: channel}, + }) + + // if we're expecting an error, we should be able to ignore validation + // and the error shouldn't happen + _, _, err := snapstate.InstallOne(context.Background(), s.state, goal, snapstate.Options{ + Flags: snapstate.Flags{IgnoreValidation: true}, + }) + c.Assert(err, IsNil) + } +} + +type testUpdateComponentsValidationSetsOpts struct { + comps []string + err string + headers map[string]interface{} +} + +func (s *validationSetsSuite) TestUpdateComponentsValidationSetsToWrongRevision(c *C) { + s.testUpdateComponentsValidationSets(c, testUpdateComponentsValidationSetsOpts{ + comps: []string{"test-component"}, + err: `cannot update component "test-snap\+test-component" to revision 2 without --ignore-validation, revision 1 is required by validation sets: 16/foo/bar/1`, + headers: map[string]interface{}{ + "revision": "11", + "components": map[string]interface{}{ + "test-component": map[string]interface{}{ + "presence": "optional", + "revision": "1", + }, + }, + }, + }) +} + +func (s *validationSetsSuite) TestUpdateComponentsValidationSetsToCorrectRevision(c *C) { + s.testUpdateComponentsValidationSets(c, testUpdateComponentsValidationSetsOpts{ + comps: []string{"test-component"}, + headers: map[string]interface{}{ + "revision": "11", + "components": map[string]interface{}{ + "test-component": map[string]interface{}{ + "presence": "optional", + "revision": "2", + }, + }, + }, + }) +} + +func (s *validationSetsSuite) TestUpdateComponentsValidationSetsMissingRequired(c *C) { + s.testUpdateComponentsValidationSets(c, testUpdateComponentsValidationSetsOpts{ + comps: nil, + err: `cannot update snap "test-snap" without component "test-component" required by validation sets 16/foo/bar/1`, + headers: map[string]interface{}{ + "components": map[string]interface{}{ + "test-component": map[string]interface{}{ + "presence": "required", + }, + }, + }, + }) +} + +func (s *validationSetsSuite) TestUpdateComponentsValidationSetsWithRequired(c *C) { + s.testUpdateComponentsValidationSets(c, testUpdateComponentsValidationSetsOpts{ + comps: []string{"test-component"}, + headers: map[string]interface{}{ + "components": map[string]interface{}{ + "test-component": map[string]interface{}{ + "presence": "required", + }, + }, + }, + }) +} + +func (s *validationSetsSuite) testUpdateComponentsValidationSets(c *C, opts testUpdateComponentsValidationSetsOpts) { + s.state.Lock() + defer s.state.Unlock() + + tr := config.NewTransaction(s.state) + tr.Set("core", "experimental.parallel-instances", true) + tr.Commit() + + const ( + snapName = "test-snap" + instanceKey = "key" + channel = "channel-for-components" + ) + instanceName := snap.InstanceName(snapName, instanceKey) + + snapID := snaptest.AssertedSnapID(snapName) + + revisionSideState := sequence.NewRevisionSideState(&snap.SideInfo{ + RealName: snapName, + Revision: snap.R(7), + SnapID: snapID, + }, []*sequence.ComponentState{sequence.NewComponentState( + &snap.ComponentSideInfo{ + Component: naming.NewComponentRef(snapName, "test-component"), + Revision: snap.R(1), + }, + snap.TestComponent, + )}) + + snapstate.Set(s.state, instanceName, &snapstate.SnapState{ + Active: true, + Sequence: sequence.SnapSequence{Revisions: []*sequence.RevisionSideState{revisionSideState}}, + Current: snap.R(7), + InstanceKey: instanceKey, + SnapType: "app", + }) + + restore := snapstate.MockReadComponentInfo(func( + compMntDir string, snapInfo *snap.Info, csi *snap.ComponentSideInfo, + ) (*snap.ComponentInfo, error) { + return &snap.ComponentInfo{ + Component: csi.Component, + }, nil + }) + defer restore() + + // make sure the fake store knows to use this id, since this is what will be + // in the validation set + s.fakeStore.registerID(snapName, snapID) + s.fakeStore.mutateSnapInfo = func(info *snap.Info) error { + comps := make(map[string]*snap.Component) + for _, c := range opts.comps { + comps[c] = &snap.Component{ + Type: snap.TestComponent, + Name: c, + } + } + info.Components = comps + return nil + } + + restore = snapstate.MockEnforcedValidationSets(func(st *state.State, extraVss ...*asserts.ValidationSet) (*snapasserts.ValidationSets, error) { + vs := snapasserts.NewValidationSets() + headers := map[string]interface{}{ + "id": snapID, + "name": snapName, + "presence": "optional", + } + for k, v := range opts.headers { + headers[k] = v + } + vsa := s.mockValidationSetAssert(c, "bar", "1", headers) + err := vs.Add(vsa.(*asserts.ValidationSet)) + c.Assert(err, IsNil) + return vs, nil + }) + defer restore() + + s.fakeStore.snapResourcesFn = func(info *snap.Info) []store.SnapResourceResult { + c.Assert(info.InstanceName(), DeepEquals, instanceName) + results := make([]store.SnapResourceResult, 0, len(opts.comps)) + for _, c := range opts.comps { + results = append(results, store.SnapResourceResult{ + DownloadInfo: snap.DownloadInfo{ + DownloadURL: "http://example.com/" + c, + }, + Name: c, + Revision: 2, + Type: "component/test", + Version: "1.0", + CreatedAt: "2024-01-01T00:00:00Z", + }) + } + return results + } + + goal := snapstate.StoreUpdateGoal(snapstate.StoreUpdate{ + InstanceName: instanceName, + RevOpts: snapstate.RevisionOptions{Channel: channel}, + }) + + _, err := snapstate.UpdateOne(context.Background(), s.state, goal, nil, snapstate.Options{}) + if len(opts.err) == 0 { + c.Assert(err, IsNil) + } else { + c.Assert(err, ErrorMatches, opts.err) + + goal := snapstate.StoreUpdateGoal(snapstate.StoreUpdate{ + InstanceName: instanceName, + RevOpts: snapstate.RevisionOptions{Channel: channel}, + }) + + // if we're expecting an error, we should be able to ignore validation + // and the error shouldn't happen + _, err := snapstate.UpdateOne(context.Background(), s.state, goal, nil, snapstate.Options{ + Flags: snapstate.Flags{IgnoreValidation: true}, + }) + c.Assert(err, IsNil) + } +} diff --git a/overlord/snapstate/snapstate_test.go b/overlord/snapstate/snapstate_test.go index bb0fab5d44c..b7707baa309 100644 --- a/overlord/snapstate/snapstate_test.go +++ b/overlord/snapstate/snapstate_test.go @@ -174,6 +174,8 @@ func (s *snapmgrBaseTest) SetUpTest(c *C) { state: s.state, downloadError: make(map[string]error), refreshRevnos: make(map[string]snap.Revision), + idsToNames: make(map[string]string), + namesToAssertedIDs: make(map[string]string), } // make tests work consistently also in containers diff --git a/overlord/snapstate/storehelpers.go b/overlord/snapstate/storehelpers.go index d9a0e59334c..005f00478a1 100644 --- a/overlord/snapstate/storehelpers.go +++ b/overlord/snapstate/storehelpers.go @@ -689,7 +689,16 @@ func storeUpdatePlanCore( }) } - // TODO: verify validation sets here to catch invalid component revisions + for _, t := range plan.targets { + up, ok := updates[t.info.InstanceName()] + if !ok { + return updatePlan{}, fmt.Errorf("internal error: target created for snap without an update: %s", t.info.InstanceName()) + } + + if err := checkTargetAgainstValidationSets(t, "refresh", up.RevOpts.ValidationSets); err != nil { + return updatePlan{}, err + } + } return plan, nil } @@ -721,6 +730,19 @@ func currentComponentsAvailableInRevision(snapst *SnapState, info *snap.Info) ([ return intersection, nil } +// ignoreValidationSetsForRefresh returns a boolean indicating whether or not we +// should ignore validation sets when refreshing this snap. There are two cases +// to consider, the single refresh case and the refresh-all case. During a +// single refresh, we only consider the flag that was passed in. During a +// refresh-all, we respect the sticky ignore validation flag that is held in +// SnapState. +func ignoreValidationSetsForRefresh(snapst *SnapState, opts Options) bool { + if !opts.ExpectOneSnap { + return snapst.IgnoreValidation + } + return opts.Flags.IgnoreValidation +} + func collectCurrentSnapsAndActions( st *state.State, allSnaps map[string]*SnapState, @@ -764,20 +786,14 @@ func collectCurrentSnapsAndActions( InstanceName: installed.InstanceName, } + ignoreValidation := ignoreValidationSetsForRefresh(snapst, opts) + // TODO: this is silly, but it matches how we currently send these flags // now. we should probably just default to sending enforce, but that // would require updating a good number of tests. good candidate for a // follow-up PR. - // - // if we are expecting only one snap to be updated, we respect the flag - // that was passed in. this maintains the existing behavior of Update vs - // UpdateMany. - ignoreValidation := snapst.IgnoreValidation - if opts.ExpectOneSnap { - ignoreValidation = opts.Flags.IgnoreValidation - if !opts.Flags.IgnoreValidation && req.RevOpts.Revision.Unset() { - action.Flags = store.SnapActionEnforceValidation - } + if !ignoreValidation && opts.ExpectOneSnap && req.RevOpts.Revision.Unset() { + action.Flags = store.SnapActionEnforceValidation } if err := completeStoreAction(action, req.RevOpts, ignoreValidation); err != nil { diff --git a/overlord/snapstate/target.go b/overlord/snapstate/target.go index f5ca77e42a7..a90f1db322c 100644 --- a/overlord/snapstate/target.go +++ b/overlord/snapstate/target.go @@ -312,11 +312,103 @@ func (s *storeInstallGoal) toInstall(ctx context.Context, st *state.State, opts }) } - // TODO: check validation sets here to catch invalid component revisions + for _, t := range installs { + sn, ok := s.find(t.info.InstanceName()) + if !ok { + return nil, fmt.Errorf("internal error: snap to install was not requested: %s", t.info.InstanceName()) + } + + if err := checkTargetAgainstValidationSets(t, "install", sn.RevOpts.ValidationSets); err != nil { + return nil, err + } + } return installs, err } +func checkTargetAgainstValidationSets(target target, action string, vsets *snapasserts.ValidationSets) error { + constraints, err := vsets.Presence(target.info) + if err != nil { + return err + } + + if err := checkSnapAgainstConstraints(target.info.InstanceName(), target.info.Revision, constraints, action); err != nil { + return err + } + + comps := make(map[string]snap.Revision, len(target.components)) + for _, comp := range target.components { + comps[comp.ComponentName()] = comp.Revision() + } + + return checkComponentsAgainstConstraints(target.info.SnapName(), comps, constraints, action) +} + +// should something like this maybe be a method on snapasserts.ValidationSets? +// the error strings are the main thing that don't really fit well in there +func checkSnapAgainstConstraints( + instanceName string, + revision snap.Revision, + constraints snapasserts.SnapPresenceConstraints, + action string, +) error { + if constraints.Presence == asserts.PresenceInvalid { + verb := "install" + if action == "refresh" { + verb = "update" + } + + return fmt.Errorf( + "cannot %s snap %q due to enforcing rules of validation set %s", + verb, + instanceName, + constraints.Sets.CommaSeparated(), + ) + } + + if !constraints.Revision.Unset() && !revision.Unset() && revision != constraints.Revision { + return invalidRevisionError(action, instanceName, constraints.Sets, revision, constraints.Revision) + } + + return nil +} + +func checkComponentsAgainstConstraints(snapName string, comps map[string]snap.Revision, constraints snapasserts.SnapPresenceConstraints, action string) error { + verb := "install" + if action == "refresh" { + verb = "update" + } + + for compName, compRevision := range comps { + cp := constraints.Component(compName) + + if cp.Presence == asserts.PresenceInvalid { + return fmt.Errorf( + "cannot %s component %q due to enforcing rules of validation set %s", + verb, + naming.NewComponentRef(snapName, compName), + cp.Sets.CommaSeparated(), + ) + } + + if !cp.Revision.Unset() && compRevision != cp.Revision { + return invalidComponentRevisionError(action, snapName, compName, cp.Sets, compRevision, cp.Revision) + } + } + + for compName, compPres := range constraints.RequiredComponents() { + if _, ok := comps[compName]; !ok { + return fmt.Errorf("cannot %s snap %q without component %q required by validation sets %s", + verb, + snapName, + compName, + compPres.Sets.CommaSeparated(), + ) + } + } + return nil +} + type cachedValidationSets = func() (*snapasserts.ValidationSets, error) // cachedEnforcedValidationSets returns a function that will lazily load (and @@ -449,14 +541,23 @@ func completeStoreAction(action *store.SnapAction, revOpts RevisionOptions, igno ) } - // make sure that the caller-requested revision matches the revision - // required by the enforced validation sets - if !pres.Revision.Unset() && !revOpts.Revision.Unset() && pres.Revision != revOpts.Revision { - return invalidRevisionError(action, pres.Sets, revOpts.Revision, pres.Revision) + // note that we don't check that we're installing invalid components + // here, since we might not know what components we're going to install + // yet. specifically, if the rules in the currently enforced validation + // sets have been broken (from using --ignore-validation), then our + // components might be in an invalid state. to prevent disallowing + // moving to a valid state, we can't check until we know what components + // are available for this snap. + // + // in short, this check is really just an early check to make sure that + // the snap revision we're installing isn't invalid in the validation + // sets before we hit the store. + if err := checkSnapAgainstConstraints( + action.InstanceName, revOpts.Revision, pres, action.Action, + ); err != nil { + return err } - // TODO:COMPS: handle validation sets and components here - // we only need to send these if this snap is actually constrained by // the validation sets in some way if pres.Constrained() { @@ -480,10 +581,10 @@ func completeStoreAction(action *store.SnapAction, revOpts RevisionOptions, igno return nil } -func invalidRevisionError(a *store.SnapAction, sets []snapasserts.ValidationSetKey, requested, required snap.Revision) error { +func invalidRevisionError(action, snapName string, sets []snapasserts.ValidationSetKey, requested, required snap.Revision) error { verb := "install" preposition := "at" - if a.Action == "refresh" { + if action == "refresh" { verb = "update" preposition = "to" } @@ -491,7 +592,26 @@ func invalidRevisionError(a *store.SnapAction, sets []snapasserts.ValidationSetK return fmt.Errorf( "cannot %s snap %q %s revision %s without --ignore-validation, revision %s is required by validation sets: %s", verb, - a.InstanceName, + snapName, + preposition, + requested, + required, + snapasserts.ValidationSetKeySlice(sets).CommaSeparated(), + ) +} + +func invalidComponentRevisionError(action, snapName, componentName string, sets []snapasserts.ValidationSetKey, requested, required snap.Revision) error { + verb := "install" + preposition := "at" + if action == "refresh" { + verb = "update" + preposition = "to" + } + + return fmt.Errorf( + "cannot %s component %q %s revision %s without --ignore-validation, revision %s is required by validation sets: %s", + verb, + naming.NewComponentRef(snapName, componentName), preposition, requested, required,