-
Notifications
You must be signed in to change notification settings - Fork 587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
a/snapasserts, o/assertstate: implement validate-component task handler #13964
Conversation
17449b0
to
472604e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some small comments and questions
overlord/assertstate/assertmgr.go
Outdated
return err | ||
} | ||
|
||
// TODO: do we need to do something like snapasserts.CheckProvenanceWithVerifiedRevision for components? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like its already included in the checks above, but maybe I'm misunderstanding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedronis can you speak to the necessity of this check and the one in https://github.com/snapcore/snapd/pull/13964/files#r1604780776? They're modeled after the ones from doValidateSnap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is about checking provenance value from inside the blob, we need to check with snapcraft whether it's being stored atm or not, it might not yet be but it should though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per @mr-cal, the snapcraft side of this isn't implemented at the moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snapcraft takes the provenance
keyword from a snapcraft.yaml
and writes it into meta/snap.yaml
.
If I understand this correctly, snapcraft should always write the same provenance keyword in meta/component.yaml
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe that is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From discussions with @pedronis, we're gonna leave the TODO for now and come back to this when the snapcraft side is implemented
0d5a13a
to
2c08d8a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did a first quick pass, looks reasonable, couple initial small comments
overlord/assertstate/assertmgr.go
Outdated
return err | ||
} | ||
|
||
// TODO: do we need to do something like snapasserts.CheckProvenanceWithVerifiedRevision for components? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is about checking provenance value from inside the blob, we need to check with snapcraft whether it's being stored atm or not, it might not yet be but it should though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a test need changes as well and there's an error path not reached perhaps
provInf := "" | ||
if provenance != "" { | ||
provInf = fmt.Sprintf(" provenance: %s", provenance) | ||
} | ||
return fmt.Errorf("internal error: cannot find pre-populated snap-resource-revision assertion for %q: %s%s", name, hash, provInf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this bit of code doesn't seem reached by tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 3ad8649
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
digest := makeDigest(12) | ||
const size = uint64(1024) | ||
const resourceName = "test-component" | ||
const snapID = "snap-id-1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't being used below. Also, as a minor preference, not having these externalised unless it's necessary makes it easier to follow the test IMO
3ad8649
to
3591bce
Compare
This PR adds a task handler for performing the validity checks on a component from the store. This is mostly fairly consistent with how
validate-snap
works, with the addition of checking that there is asnap-resource-pair
that exists for the component and snap revision.