Skip to content
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.

Sign up for GitHub

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

generateinsertlayer: ensure that we close the tarwriter #534

Merged
merged 3 commits into from
Feb 22, 2024
Merged

generateinsertlayer: ensure that we close the tarwriter #534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad29ed3
generateinsertlayer: close the tarwriter
mikemccracken Feb 21, 2024
653952b
generate: do not warn about err on success
mikemccracken Feb 21, 2024
7bb2940
test: check blobs with gnutar, add sm file insert
mikemccracken Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions oci/layer/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ func GenerateLayer(path string, deltas []mtree.InodeDelta, opt *RepackOptions) (
go func() (Err error) {
// Close with the returned error.
defer func() {
log.Warnf("could not generate layer: %v", Err)
if Err != nil {
log.Warnf("could not generate layer: %v", Err)
}
// #nosec G104
_ = writer.CloseWithError(errors.Wrap(Err, "generate layer"))
}()
Expand Down Expand Up @@ -135,13 +137,21 @@ func GenerateInsertLayer(root string, target string, opaque bool, opt *RepackOpt

go func() (Err error) {
defer func() {
log.Warnf("could not generate insert layer: %v", Err)
if Err != nil {
log.Warnf("could not generate insert layer: %v", Err)
}
// #nosec G104
_ = writer.CloseWithError(errors.Wrap(Err, "generate insert layer"))
}()

tg := newTarGenerator(writer, packOptions.MapOptions)

defer func() {
if err := tg.tw.Close(); err != nil {
log.Warnf("generate insert layer: could not close tar.Writer: %s", err)
}
}()

if opaque {
if err := tg.AddOpaqueWhiteout(target); err != nil {
return err
Expand Down
16 changes: 15 additions & 1 deletion test/helpers.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,21 @@ function requires() {
}

function image-verify() {
oci-image-tool validate --type "imageLayout" "$@"
local ocidir="$@"
# test that each generated targz file is valid according to gnutar:
for f in $(ls $ocidir/blobs/sha256/); do
file $ocidir/blobs/sha256/$f | grep "gzip" || {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It would be nice if this check was more "strictly correct" (i.e. it actually detected if the file was actually a tar archive rather than a gzip stream) but given this is just for tests that's not too important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree... I actually thought maybe the right thing was to have the oci-image-tool do this validation, but that seems to be abandoned.
Is there a different tool for standards validation out there somewhere that we should move this to?
Given the recent changes with artifacts, etc, it is probably extra stale now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were discussions about moving the image-validation infrastructure to umoci, but nothing has happened with regards to that topic since then. I can take a look at doing this after I do an umoci 0.5 release.

continue
}
zcat $ocidir/blobs/sha256/$f | tar tvf - >/dev/null || {
rc=$?
file $ocidir/blobs/sha256/$f
echo "error untarring $f: $rc"
return $rc
}
echo $f: valid tar archive
done
oci-image-tool validate --type "imageLayout" "$ocidir"
return $?
}

Expand Down
5 changes: 5 additions & 0 deletions test/insert.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ function teardown() {
touch "${INSERTDIR}/test/a"
touch "${INSERTDIR}/test/b"
chmod +x "${INSERTDIR}/test/b"
echo "foo" > "${INSERTDIR}/test/smallfile"

# Make sure rootless mode works.
mkdir -p "${INSERTDIR}/some/path"
Expand All @@ -68,6 +69,10 @@ function teardown() {
[ "$status" -eq 0 ]
image-verify "${IMAGE}"

umoci insert --image "${IMAGE}:${TAG}" "${INSERTDIR}/test/smallfile" /tester/smallfile
[ "$status" -eq 0 ]
image-verify "${IMAGE}"

# Unpack after the inserts.
new_bundle_rootfs
umoci unpack --image "${IMAGE}:${TAG}-new" "$BUNDLE"
Expand Down
Loading