From 407b591aae9f75496b451b29776f7dc8084fbe81 Mon Sep 17 00:00:00 2001 From: Andrew Phelps Date: Wed, 9 Oct 2024 12:08:25 -0400 Subject: [PATCH] many: remove snapasserts.ValidationSets.CheckPresence* methods --- asserts/snapasserts/validation_sets.go | 43 ------------ asserts/snapasserts/validation_sets_test.go | 73 ++++++++------------- image/image_linux.go | 16 +++-- overlord/devicestate/devicestate.go | 4 +- overlord/snapstate/snapstate.go | 22 +++---- overlord/snapstate/target.go | 30 ++++----- 6 files changed, 59 insertions(+), 129 deletions(-) diff --git a/asserts/snapasserts/validation_sets.go b/asserts/snapasserts/validation_sets.go index ccd4ab2ad016..5c1d39d1e032 100644 --- a/asserts/snapasserts/validation_sets.go +++ b/asserts/snapasserts/validation_sets.go @@ -962,28 +962,6 @@ func (v *ValidationSets) constraintsForSnap(snapRef naming.SnapRef) *snapConstra return nil } -// CheckPresenceRequired returns the list of all validation sets that declare -// presence of the given snap as required and the required revision (or -// snap.R(0) if no specific revision is required). PresenceConstraintError is -// returned if presence of the snap is "invalid". -// The method assumes that validation sets are not in conflict. -func (v *ValidationSets) CheckPresenceRequired(snapRef naming.SnapRef) ([]ValidationSetKey, snap.Revision, error) { - p, err := v.Presence(snapRef) - if err != nil { - return nil, snap.Revision{}, err - } - - if p.Presence == asserts.PresenceInvalid { - return nil, snap.Revision{}, &PresenceConstraintError{snapRef.SnapName(), p.Presence} - } - - if p.Presence != asserts.PresenceRequired { - return nil, unspecifiedRevision, nil - } - - return p.Sets, p.Revision, nil -} - // CanBePresent returns true if a snap can be present in a situation in which // these validation sets are being applied. func (v *ValidationSets) CanBePresent(snapRef naming.SnapRef) bool { @@ -1012,27 +990,6 @@ func (v *ValidationSets) SnapConstrained(snapRef naming.SnapRef) bool { return v.constraintsForSnap(snapRef) != nil } -// CheckPresenceInvalid returns the list of all validation sets that declare -// presence of the given snap as invalid. PresenceConstraintError is returned if -// presence of the snap is "optional" or "required". -// The method assumes that validation sets are not in conflict. -func (v *ValidationSets) CheckPresenceInvalid(snapRef naming.SnapRef) ([]ValidationSetKey, error) { - if !v.SnapConstrained(snapRef) { - return nil, nil - } - - p, err := v.Presence(snapRef) - if err != nil { - return nil, err - } - - if p.Presence != asserts.PresenceInvalid { - return nil, &PresenceConstraintError{snapRef.SnapName(), p.Presence} - } - - return p.Sets, nil -} - type presence struct { Presence asserts.Presence Revision snap.Revision diff --git a/asserts/snapasserts/validation_sets_test.go b/asserts/snapasserts/validation_sets_test.go index a8675d9dfdb1..41986ef82596 100644 --- a/asserts/snapasserts/validation_sets_test.go +++ b/asserts/snapasserts/validation_sets_test.go @@ -1198,9 +1198,9 @@ func (s *validationSetsSuite) TestCheckPresenceRequired(c *C) { valsets := snapasserts.NewValidationSets() // no validation sets - vsKeys, _, err := valsets.CheckPresenceRequired(naming.Snap("my-snap")) + presence, err := valsets.Presence(naming.Snap("my-snap")) c.Assert(err, IsNil) - c.Check(vsKeys, HasLen, 0) + c.Check(presence.Constrained(), Equals, false) c.Assert(valsets.Add(valset1), IsNil) c.Assert(valsets.Add(valset2), IsNil) @@ -1209,39 +1209,31 @@ func (s *validationSetsSuite) TestCheckPresenceRequired(c *C) { // validity c.Assert(valsets.Conflict(), IsNil) - vsKeys, rev, err := valsets.CheckPresenceRequired(naming.Snap("my-snap")) + presence, err = valsets.Presence(naming.Snap("my-snap")) c.Assert(err, IsNil) - c.Check(rev, DeepEquals, snap.Revision{N: 7}) - c.Check(vsKeys, DeepEquals, []snapasserts.ValidationSetKey{"16/account-id/my-snap-ctl/1", "16/account-id/my-snap-ctl2/2", "16/account-id/my-snap-ctl3/1"}) + c.Check(presence.Revision, DeepEquals, snap.Revision{N: 7}) + c.Check(presence.Presence, DeepEquals, asserts.PresenceRequired) + c.Check(presence.Sets, DeepEquals, snapasserts.ValidationSetKeySlice{"16/account-id/my-snap-ctl/1", "16/account-id/my-snap-ctl2/2", "16/account-id/my-snap-ctl3/1"}) - vsKeys, rev, err = valsets.CheckPresenceRequired(naming.NewSnapRef("my-snap", "mysnapididididididididididididid")) + presence, err = valsets.Presence(naming.NewSnapRef("my-snap", "mysnapididididididididididididid")) c.Assert(err, IsNil) - c.Check(rev, DeepEquals, snap.Revision{N: 7}) - c.Check(vsKeys, DeepEquals, []snapasserts.ValidationSetKey{"16/account-id/my-snap-ctl/1", "16/account-id/my-snap-ctl2/2", "16/account-id/my-snap-ctl3/1"}) - - // other-snap is not required - vsKeys, rev, err = valsets.CheckPresenceRequired(naming.Snap("other-snap")) - c.Assert(err, ErrorMatches, `unexpected presence "invalid" for snap "other-snap"`) - pr, ok := err.(*snapasserts.PresenceConstraintError) - c.Assert(ok, Equals, true) - c.Check(pr.SnapName, Equals, "other-snap") - c.Check(pr.Presence, Equals, asserts.PresenceInvalid) - c.Check(rev, DeepEquals, snap.Revision{N: 0}) - c.Check(vsKeys, HasLen, 0) + c.Check(presence.Revision, DeepEquals, snap.Revision{N: 7}) + c.Check(presence.Presence, DeepEquals, asserts.PresenceRequired) + c.Check(presence.Sets, DeepEquals, snapasserts.ValidationSetKeySlice{"16/account-id/my-snap-ctl/1", "16/account-id/my-snap-ctl2/2", "16/account-id/my-snap-ctl3/1"}) // unknown snap is not required - vsKeys, rev, err = valsets.CheckPresenceRequired(naming.NewSnapRef("unknown-snap", "00000000idididididididididididid")) + presence, err = valsets.Presence(naming.NewSnapRef("unknown-snap", "00000000idididididididididididid")) c.Assert(err, IsNil) - c.Check(rev, DeepEquals, snap.Revision{N: 0}) - c.Check(vsKeys, HasLen, 0) + c.Check(presence.Constrained(), Equals, false) // just one set, required but no revision specified valsets = snapasserts.NewValidationSets() c.Assert(valsets.Add(valset3), IsNil) - vsKeys, rev, err = valsets.CheckPresenceRequired(naming.Snap("my-snap")) + presence, err = valsets.Presence(naming.Snap("my-snap")) c.Assert(err, IsNil) - c.Check(rev, DeepEquals, snap.Revision{N: 0}) - c.Check(vsKeys, DeepEquals, []snapasserts.ValidationSetKey{"16/account-id/my-snap-ctl3/1"}) + c.Check(presence.Revision, DeepEquals, snap.Revision{N: 0}) + c.Check(presence.Presence, DeepEquals, asserts.PresenceRequired) + c.Check(presence.Sets, DeepEquals, snapasserts.ValidationSetKeySlice{"16/account-id/my-snap-ctl3/1"}) } func (s *validationSetsSuite) TestIsPresenceInvalid(c *C) { @@ -1285,9 +1277,9 @@ func (s *validationSetsSuite) TestIsPresenceInvalid(c *C) { valsets := snapasserts.NewValidationSets() // no validation sets - vsKeys, err := valsets.CheckPresenceInvalid(naming.Snap("my-snap")) + presence, err := valsets.Presence(naming.Snap("my-snap")) c.Assert(err, IsNil) - c.Check(vsKeys, HasLen, 0) + c.Check(presence.Constrained(), Equals, false) c.Assert(valsets.Add(valset1), IsNil) c.Assert(valsets.Add(valset2), IsNil) @@ -1296,31 +1288,18 @@ func (s *validationSetsSuite) TestIsPresenceInvalid(c *C) { c.Assert(valsets.Conflict(), IsNil) // invalid in two sets - vsKeys, err = valsets.CheckPresenceInvalid(naming.Snap("my-snap")) + presence, err = valsets.Presence(naming.Snap("my-snap")) c.Assert(err, IsNil) - c.Check(vsKeys, DeepEquals, []snapasserts.ValidationSetKey{"16/account-id/my-snap-ctl/1", "16/account-id/my-snap-ctl2/2"}) + c.Check(presence.Sets, DeepEquals, snapasserts.ValidationSetKeySlice{"16/account-id/my-snap-ctl/1", "16/account-id/my-snap-ctl2/2"}) - vsKeys, err = valsets.CheckPresenceInvalid(naming.NewSnapRef("my-snap", "mysnapididididididididididididid")) + presence, err = valsets.Presence(naming.NewSnapRef("my-snap", "mysnapididididididididididididid")) c.Assert(err, IsNil) - c.Check(vsKeys, DeepEquals, []snapasserts.ValidationSetKey{"16/account-id/my-snap-ctl/1", "16/account-id/my-snap-ctl2/2"}) - - // other-snap isn't invalid - vsKeys, err = valsets.CheckPresenceInvalid(naming.Snap("other-snap")) - c.Assert(err, ErrorMatches, `unexpected presence "optional" for snap "other-snap"`) - pr, ok := err.(*snapasserts.PresenceConstraintError) - c.Assert(ok, Equals, true) - c.Check(pr.SnapName, Equals, "other-snap") - c.Check(pr.Presence, Equals, asserts.PresenceOptional) - c.Check(vsKeys, HasLen, 0) - - vsKeys, err = valsets.CheckPresenceInvalid(naming.NewSnapRef("other-snap", "123456ididididididididididididid")) - c.Assert(err, ErrorMatches, `unexpected presence "optional" for snap "other-snap"`) - c.Check(vsKeys, HasLen, 0) - - // unknown snap isn't invalid - vsKeys, err = valsets.CheckPresenceInvalid(naming.NewSnapRef("unknown-snap", "00000000idididididididididididid")) + c.Check(presence.Sets, DeepEquals, snapasserts.ValidationSetKeySlice{"16/account-id/my-snap-ctl/1", "16/account-id/my-snap-ctl2/2"}) + + // unknown snap isn't constrained + presence, err = valsets.Presence(naming.NewSnapRef("unknown-snap", "00000000idididididididididididid")) c.Assert(err, IsNil) - c.Check(vsKeys, HasLen, 0) + c.Check(presence.Constrained(), Equals, false) } func (s *validationSetsSuite) TestParseValidationSet(c *C) { diff --git a/image/image_linux.go b/image/image_linux.go index a6187999012d..c5f4962466a4 100644 --- a/image/image_linux.go +++ b/image/image_linux.go @@ -559,16 +559,18 @@ func (s *imageSeeder) validationSetKeysAndRevisionForSnap(snapName string) ([]sn return nil, snap.Revision{}, err } - // TODO: It's pointed out that here and some of the others uses of this - // may miss logic for optional snaps which have required revisions. This - // is not covered by the below check, and we may or may not have multiple places - // with a similar issue. - snapVsKeys, snapRev, err := allVss.CheckPresenceRequired(naming.Snap(snapName)) + pres, err := allVss.Presence(naming.Snap(snapName)) if err != nil { return nil, snap.Revision{}, err } - if len(snapVsKeys) > 0 { - return snapVsKeys, snapRev, nil + + // TODO: figure out if this is needed + if pres.Presence == asserts.PresenceInvalid { + return nil, snap.Revision{}, fmt.Errorf("snap %q is invalid in validation sets: %v", snapName, pres.Sets.CommaSeparated()) + } + + if pres.Constrained() { + return pres.Sets, pres.Revision, nil } return nil, s.w.Manifest().AllowedSnapRevision(snapName), nil } diff --git a/overlord/devicestate/devicestate.go b/overlord/devicestate/devicestate.go index aebce48590a6..c4556812403b 100644 --- a/overlord/devicestate/devicestate.go +++ b/overlord/devicestate/devicestate.go @@ -1745,14 +1745,14 @@ func CreateRecoverySystem(st *state.State, label string, opts CreateRecoverySyst } if sn.Presence != "required" { - sets, _, err := valsets.CheckPresenceRequired(sn) + pres, err := valsets.Presence(sn) if err != nil { return nil, err } // snap isn't already installed, and it isn't required by model or // any validation sets, so we should skip it - if len(sets) == 0 { + if pres.Presence != asserts.PresenceRequired { continue } } diff --git a/overlord/snapstate/snapstate.go b/overlord/snapstate/snapstate.go index db0f6c028cea..521c610a3879 100644 --- a/overlord/snapstate/snapstate.go +++ b/overlord/snapstate/snapstate.go @@ -3464,30 +3464,24 @@ func canRemove(st *state.State, si *snap.Info, snapst *SnapState, removeAll bool if enforcedSets == nil { return nil } - requiredValsets, requiredRevision, err := enforcedSets.CheckPresenceRequired(si) + pres, err := enforcedSets.Presence(si) if err != nil { - if _, ok := err.(*snapasserts.PresenceConstraintError); !ok { - return err - } - // else - presence is invalid, nothing to do (not really possible since - // it shouldn't be allowed to get installed in the first place). - return nil + return err } - if len(requiredValsets) == 0 { - // not required by any validation set (or is optional) + if pres.Presence != asserts.PresenceRequired { return nil } // removeAll is set if we're removing the snap completely if removeAll { - if requiredRevision.Unset() { - return fmt.Errorf("snap %q is required by validation sets: %s", si.InstanceName(), snapasserts.ValidationSetKeySlice(requiredValsets).CommaSeparated()) + if pres.Revision.Unset() { + return fmt.Errorf("snap %q is required by validation sets: %s", si.InstanceName(), pres.Sets.CommaSeparated()) } - return fmt.Errorf("snap %q at revision %s is required by validation sets: %s", si.InstanceName(), requiredRevision, snapasserts.ValidationSetKeySlice(requiredValsets).CommaSeparated()) + return fmt.Errorf("snap %q at revision %s is required by validation sets: %s", si.InstanceName(), pres.Revision, pres.Sets.CommaSeparated()) } // rev is set at this point (otherwise we would hit removeAll case) - if requiredRevision.N == rev.N { - return fmt.Errorf("snap %q at revision %s is required by validation sets: %s", si.InstanceName(), rev, snapasserts.ValidationSetKeySlice(requiredValsets).CommaSeparated()) + if pres.Revision.N == rev.N { + return fmt.Errorf("snap %q at revision %s is required by validation sets: %s", si.InstanceName(), rev, pres.Sets.CommaSeparated()) } // else - it's ok to remove a revision different than the required return nil } diff --git a/overlord/snapstate/target.go b/overlord/snapstate/target.go index 8948770dcf16..6e70895c696d 100644 --- a/overlord/snapstate/target.go +++ b/overlord/snapstate/target.go @@ -25,6 +25,7 @@ import ( "fmt" "sort" + "github.com/snapcore/snapd/asserts" "github.com/snapcore/snapd/asserts/snapasserts" "github.com/snapcore/snapd/client" "github.com/snapcore/snapd/logger" @@ -413,14 +414,12 @@ func completeStoreAction(action *store.SnapAction, revOpts RevisionOptions, igno // if the caller didn't provide any validation sets, make sure that // the snap is allowed by all of the enforced validation sets - invalidSets, err := vsets.CheckPresenceInvalid(naming.Snap(action.InstanceName)) + pres, err := vsets.Presence(naming.Snap(action.InstanceName)) if err != nil { - if _, ok := err.(*snapasserts.PresenceConstraintError); !ok { - return err - } // else presence is optional or required, carry on + return err } - if len(invalidSets) > 0 { + if pres.Presence == asserts.PresenceInvalid { verb := "install" if action.Action == "refresh" { verb = "update" @@ -430,29 +429,28 @@ func completeStoreAction(action *store.SnapAction, revOpts RevisionOptions, igno "cannot %s snap %q due to enforcing rules of validation set %s", verb, action.InstanceName, - snapasserts.ValidationSetKeySlice(invalidSets).CommaSeparated(), + pres.Sets.CommaSeparated(), ) } - requiredSets, requiredRev, err := vsets.CheckPresenceRequired(naming.Snap(action.InstanceName)) - if err != nil { - return err - } - // make sure that the caller-requested revision matches the revision // required by the enforced validation sets - if !requiredRev.Unset() && !revOpts.Revision.Unset() && requiredRev != revOpts.Revision { - return invalidRevisionError(action, requiredSets, revOpts.Revision, requiredRev) + if !pres.Revision.Unset() && !revOpts.Revision.Unset() && pres.Revision != revOpts.Revision { + return invalidRevisionError(action, pres.Sets, revOpts.Revision, pres.Revision) } // TODO:COMPS: handle validation sets and components here - action.ValidationSets = requiredSets + // we only need to send these if this snap is actually constrained by + // the validation sets in some way + if pres.Constrained() { + action.ValidationSets = pres.Sets + } - if !requiredRev.Unset() { + if !pres.Revision.Unset() { // make sure that we use the revision required by the enforced // validation sets - action.Revision = requiredRev + action.Revision = pres.Revision // we ignore the cohort if a validation set requires that the // snap is pinned to a specific revision