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
overlay: move the fs-verity measurement to the chunked package #1804
Conversation
7d1962d
to
ba3e8cf
Compare
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
ba3e8cf
to
e4c2a3d
Compare
@alexlarsson PTAL |
// If it is not required, ignore the error if the filesystem does not support it. | ||
if errors.Is(err, unix.ENOTSUP) || errors.Is(err, unix.ENOTTY) { | ||
return nil | ||
} |
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.
Should we not return err here?
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.
I guess maybe we shouldn't fail if the error is "already enabled", so we'll get the error in the measure call? If so, maybe add a comment about this?
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.
EnableVerity doesn't return an error on EEXIST, I'll add a comment to make it clearer
|
||
err := fsverity.EnableVerity(path, int(roFile.Fd())) | ||
if err != nil { | ||
if c.useFsVerity == graphdriver.DifferFsVerityRequired { |
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 should handle EEXIST for the fs-verity already enabled case and make sure to then read and record the existing digest.
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.
fsverity.EnableVerity
returns nil if fs-verity was already enabled, so we should already get into the fsverity.MeasureVerity
function even if DifferFsVerityRequired
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
avoid to reopen the files twice to calculate their fs-verity digest. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
e4c2a3d
to
539d0fe
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexlarsson, giuseppe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
:GT< |
move the logic to calculate the fs-verity for the files payload to the chunked package, so it is calculated when the files are first created.