sylabs / sif Goto Github PK
View Code? Open in Web Editor NEWSingularity Image Format (SIF) reference implementation.
License: Other
Singularity Image Format (SIF) reference implementation.
License: Other
Currently, NewDescriptorInput
does not place the resulting object in an object group. The vast majority of the case, objects are placed in the "default" group (group 1.) We should make this the default behaviour.
A new build constraint format was introduced in Go 1.17, and in the SIF codebase in #124. This format must exist alongside the old format (// +build
) until the minimum Go version supported by SIF understands the new format.
Once Go 1.17 is the minimum supported version, remove the old format build constraints.
The golang.org/x/crypto/openpgp
package is now deprecated (see https://golang.org/issue/44226 for details.) The integrity
package currently uses this (both v1
and v2
.) We should consider whether to migrate to a different package.
The Descriptor
type currently uses type embedding to expose the fields/methods of rawDescriptor
. The exported fields in rawDescriptor
reveal implementation details that are best left unexported. This needs to be addressed prior to the v2 stable release.
At present, the descriptor capacity of an image created via CreateContainer
or CreateContainerAtPath
is hard coded to 48. There may be use cases where it makes sense to support more (or fewer) descriptors in a given image.
Add a CreateOpt
to customize the descriptor capacity.
We should utilize the functional options pattern to provide a way to extend (*FileImage).SetPrimPart.
Passing zero to OptObjectAlignment()
causes a panic
to occur:
panic: runtime error: integer divide by zero [recovered]
panic: runtime error: integer divide by zero
Fix the code such that OptObjectAlignment(0)
does not panic
, and behaves equivalently to OptObjectAlignment(1)
.
The sif
package currently defines its own HashType
. The integrity
package translates crypto.Hash
values to/from HashType
values. We should treat HashType
as an implementation detail, and switch to type crypto.Hash
in the exported API.
Methods that return a time.Time
in the SIF API currently return values with the location set to UTC. By contrast, the time
package generally returns values in the local time.
I think it'd be better if we returned local time values, and left the decision of whether to convert to UTC to the caller.
func readableSize
returns invalid human-readable sizes for values >= 1 PiB. For example:
readableSize(1)
= "1"
readableSize(1024 * 1024 * 1024 * 1024)
= "1 TiB"
readableSize(1024 * 1024 * 1024 * 1024 * 1024)
= "1"
A SIF in excess of 1 PiB is certainly extreme. But we should probably handle this case gracefully.
We should utilize the functional options pattern to provide a way to extend (*FileImage).AddObject.
Reproducible builds are important as they provide an independently-verifiable path from source to binary code (ref).
The goreleaser
configuration follows some of the recommendations for reproducible builds available here. We do not currently make use of the -trimpath
flag, though arguably formal releases are always built by continuous integration, and thus use a consistent directory structure.
On the other hand the mage
build is not currently reproducible, as demonstrated by running the build twice from the same commit:
$ mage install
$ sha256sum ~/go/bin/siftool
d959a6f5bc4e17353df582ce60b5ae5cb2a50a85210c9e6f061a25774c54c348 /home/adam/go/bin/siftool
$ mage install
$ sha256sum ~/go/bin/siftool
f6a1b211003de62897ebc0fb476694945a81b9bdfa31d027670caaf96047a200 /home/adam/go/bin/siftool
I think it might make sense to make the siftool
build reproducible, and unify the build flags used between goreleaser
and mage
.
The integrity.VerifyCallback type passed to integrity.OptVerifyCallback uses the type integrity.VerifyResult interface.
As mentioned here, use of an interface type limits future extension, since an addition to the interface is a breaking change.
We should return a concrete type instead of an interface
to support future extension of the API.
DeleteObject
currently accepts a flags
field that accepts DelZero
and/or DelCompact
options. We should add a type DeleteOpt
that mirrors CreateOpt
, for consistency and clarity.
The Short
usage strings for siftool
all begin with an upper case letter, but the practice in Cobra internally is to start these with lowercase (see completion
command below). For consistency, we should update the siftool
commands to be consistent.
$ siftool help
A set of commands are provided to display elements such as the SIF global
header, the data object descriptors and to dump data objects. It is also
possible to modify a SIF file via this tool via the add/del commands.
Usage:
siftool [command]
Available Commands:
add Add a data object to a SIF file
completion generate the autocompletion script for the specified shell
del Delete a specified object descriptor and data from SIF file
dump Extract and output data objects from SIF files
header Display SIF global headers
help Help about any command
info Display detailed information of object descriptors
list List object descriptors from SIF files
new Create a new empty SIF image file
setprim Set primary system partition
version Print version information
Flags:
-h, --help help for siftool
Use "siftool [command] --help" for more information about a command.
sif.CreateContainer
currently accepts a sif.CreateInfo
:
Lines 403 to 410 in 3a39a27
With this pattern, users must understand the internals of SIF to effectively call CreateContainer
. It would be better if these were defaulted, with potential overrides supplied via the functional options pattern. For example:
Launchstr
: default to sif.HdrLaunch
Sifversion
: default to sif.HdrVersion
ID
: default to a generated ID (perhaps the public API can avoid a UUID package at all, to prevent issues such as #7).InputDescr
: default to empty list, allow DescriptorInput
via functional option.sif.CreateContainer
and *FileImage.AddObject
currently accept a sif.DescriptorInput
:
Lines 412 to 428 in 3a39a27
This struct is unwieldy, requiring knowledge of the implementation to use properly. For example, it is possible to provide Fp
, and/or Data
, but it's not obvious in what circumstances each is required.
I suggest we move to a functional options approach to create a DescriptorInput
, and make the following changes:
Datatype
: provide explicitly.Groupid
: default to zero (unused), override via functional option.Link
: default to zero (unused), override via functional option.Size
: remove? This can be calculated while reading underlying data object.Alignment
: default to memory page alignment (?), override via functional option.Fname
: default to empty, override via functional option?Fp
: provide explicitly?Data
: remove, redundant with Fp
?Image
: remove, unused?Descr
: remove, unused?Extra
: default to empty, override via functional option for specific Datatype
.func readableSize
does not correctly round size values. For example, readableSize(2047)
= "1KB"
.
The V1 API had sif.LoadContainerReader(), which allowed a user to do some operations on a SIF file from a source that did not support all of ReadWriter interface.
There is one use case where this is particularly useful, and that is inspecting the header and/or descriptors at the start of a SIF file as it streams by (from an http request, for example.)
I think we could accommodate this by exposing a type similar to bytes.Buffer that implements ReadWriter interface
. There may be other options worth exploring as well...
At present, images created via CreateContainer
or CreateContainerAtPath
are hard coded with the launch script "#!/usr/bin/env run-singularity\n"
. There may be use cases where it makes sense to use a different launch script, or disable the launch script altogether. For example, Singularity plugins utilize the SIF format, but the Singularity launch script is not appropriate for this use case.
Add a CreateOpt
to customize the launch script.
We should automate release publishing using goreleaser.
There are a few formatting functions that print various information contained in a FileImage
:
As far as I'm aware, these are used exclusively by the internal/app/siftool
package. I believe we should mark these functions as deprecated in the v1 API, and make a breaking change to remove these functions from pkg/sif
in the v2 API.
type ReadWriter
is a large interface. For the v2 API, we should see if it can be trimmed down.
The v1 module has a direct dependency on an unmaintained module which has a documented weakness as described in GHSA-4gh8-x3vv-phhg. Although a workaround is available, it requires manual intervention.
Once v2
is released, we should include a module deprecation comment instructing users to upgrade to github.com/sylabs/sif/v2
.
(Descriptor).GetData
and (Descriptor).GetReader
currently require a *FileImage
to be passed to obtain data associated with a Descriptor
. This is inconvenient as a user, because it requires a Descriptor
and FileImage
to be passed around.
We should embed the necessary information inside GetData
and GetReader
, and remove these parameters.
There are currently no unit tests for the internal/app/siftool
package. Adding unit tests here will support more rapid development of this portion of the code.
CreateContainer
creates a new SIF container at a specified path, and returns a *FileImage
:
Lines 219 to 222 in cde6de8
Unfortunately, the returned *FileImage
is not usable, because its Fp
field is closed by the time CreateContainer
returns. Most operations on fimg
return os.ErrClosed
.
Use sebdah/goldie for golden file testing, to replace custom code that does the same thing:
sif/pkg/integrity/main_test.go
Lines 87 to 124 in da34172
Data objects in a SIF are aligned by default, which is achieved with zero or more padding bytes before the data object. The number of padding bytes can be calculated by subtracting Size
from SizeWithPadding
.
In any given SIF image, data objects are included following a data object descriptor section. You can see this using siftool
:
$ siftool header test/images/one-group.sif
Launch: #!/usr/bin/env run-singularity
Version: 01
Arch: 386
ID: 6ecc76b7-a497-4f7f-9ebd-8da2a04c6be1
Ctime: 2020-05-22 19:30:59 +0000 UTC
Mtime: 2020-05-22 19:30:59 +0000 UTC
Dfree: 46
Dtotal: 48
Descoff: 4096
Descrlen: 28080
Dataoff: 32768
Datalen: 4100
In this image, the data object descriptor section starts at offset 4096, and the data objects start at offset 32768.
The data objects in this image are as follows:
$ siftool list test/images/one-group.sif
Container id: 6ecc76b7-a497-4f7f-9ebd-8da2a04c6be1
Created on: 2020-05-22 19:30:59 +0000 UTC
Modified on: 2020-05-22 19:30:59 +0000 UTC
----------------------------------------------------
Descriptor list:
ID |GROUP |LINK |SIF POSITION (start-end) |TYPE
------------------------------------------------------------------------------
1 |1 |NONE |32768-32772 |FS (Raw/System/386)
2 |1 |NONE |36864-36868 |FS (Squashfs/*System/386)
By default, data objects are added with a default alignment of 4096. We can see that in this image, with objects starting at offset 32768 and 36864. In order to achieve this, both objects are preceded by padding. With a bit of math, we can see that the object descriptor section ends at offset Descoff + Descrlen = 4096 + 28080 = 32176
. So, the padding is as follows:
32768 - 32176 = 592
bytes of padding36864 - 32772 = 4092
bytes of paddingUnfortunately, this is not reflected in Size
from SizeWithPadding
fields of the first descriptor:
SizeWithPadding - Size = 4 - 4 = 0
bytes of paddingSizeWithPadding - Size = 4096 - 4 = 4092
bytes of paddingThis actually doesn't appear to have any material impact on accessing data objects within a given SIF image, since only Offset
and Size
fields in the rawDescriptor
are used for this purpose. Regardless, I believe we should fix the code to correctly record data object padding.
The ability to construct a SIF image in a reproducible way has been raised as a requirement. Specifically, what is meant by reproducible here is that, given identical input(s) to CreateContainer
or CreateContainerAtPath
, identical SIFs should be produced.
This is technically possible in the V2 API already, but requires use of several options:
While this works, it forces the user to select consistent ID and time values, which are not likely meaningful.
I propose we add an OptCreateReproducible
that defaults the ID and time values to sensible defaults.
type Descriptor struct
contains UID/GID values:
Lines 332 to 333 in 08f0698
These are populated when a data object is created by calling user.Current
:
Lines 51 to 69 in 08f0698
UID/GID values are by their nature system specific, and thus these seem to be of limited utility in a container format that is meant to support mobility. I cannot find a single use of these in SingularityCE, nor any other projects using the github.com/sylabs/sif
module.
I suggest we deprecate these in github.com/sylabs/sif
. In github.com/sylabs/sif/v2
, I suggest we set these values to zero, and remove them from the exported API.
SIF's Go Version Compatibility statement makes it clear that this module aims to support the two most recent stable versions of Go.
At the moment, the code in v1
and master
can be built using Go 1.15:
go1.15.15 build ./...
But it actually can't be tested. In v1
:
$ go1.15.15 test ./...
# github.com/sylabs/sif/pkg/sif [github.com/sylabs/sif/pkg/sif.test]
pkg/sif/create_test.go:61:12: undefined: os.CreateTemp
pkg/sif/create_test.go:160:12: undefined: os.CreateTemp
? github.com/sylabs/sif/cmd/siftool [no test files]
? github.com/sylabs/sif/internal/app/siftool [no test files]
ok github.com/sylabs/sif/pkg/integrity 0.154s
FAIL github.com/sylabs/sif/pkg/sif [build failed]
? github.com/sylabs/sif/pkg/siftool [no test files]
FAIL
And master
:
$ go1.15.15 test ./...
# github.com/sylabs/sif/v2/internal/app/siftool [github.com/sylabs/sif/v2/internal/app/siftool.test]
internal/app/siftool/modif_test.go:24:13: undefined: os.CreateTemp
internal/app/siftool/modif_test.go:79:15: undefined: os.CreateTemp
internal/app/siftool/modif_test.go:104:13: undefined: os.CreateTemp
internal/app/siftool/modif_test.go:131:13: undefined: os.CreateTemp
# github.com/sylabs/sif/v2/pkg/siftool [github.com/sylabs/sif/v2/pkg/siftool.test]
pkg/siftool/new_test.go:24:15: undefined: os.CreateTemp
pkg/siftool/siftool_test.go:22:13: undefined: os.CreateTemp
# github.com/sylabs/sif/v2/pkg/sif [github.com/sylabs/sif/v2/pkg/sif.test]
pkg/sif/create_test.go:59:13: undefined: os.CreateTemp
pkg/sif/create_test.go:116:12: undefined: os.CreateTemp
# github.com/sylabs/sif/v2/pkg/integrity [github.com/sylabs/sif/v2/pkg/integrity.test]
pkg/integrity/metadata_test.go:26:12: undefined: os.ReadFile
pkg/integrity/metadata_test.go:70:15: undefined: os.ReadFile
pkg/integrity/metadata_test.go:77:15: undefined: os.ReadFile
pkg/integrity/sign_test.go:434:16: undefined: os.CreateTemp
? github.com/sylabs/sif/v2/cmd/siftool [no test files]
FAIL github.com/sylabs/sif/v2/internal/app/siftool [build failed]
FAIL github.com/sylabs/sif/v2/pkg/integrity [build failed]
FAIL github.com/sylabs/sif/v2/pkg/sif [build failed]
FAIL github.com/sylabs/sif/v2/pkg/siftool [build failed]
FAIL
To ensure it's always possible to build
/test
against the two most recent stable versions of Go, let's verify that explicitly in CI.
When a SIF is loaded from a non-local filesystem using LoadContainer
or LoadContainerFp
, a SIGBUS
may occur when an I/O issue occurs in the underlying storage.
The reason for this is that the SIF module memory maps the file when loading with LoadContainer
/LoadContainerFp
. When combined with non-local storage, mmap
is particularly risky. A SIGBUS
can be raised any time an underlying I/O operation fails. Since accessing SIF images from non-local storage is a common use case in SingularityCE, we can't ignore this risk.
In theory, the SIGBUS
could be caught with a signal handler, and a scheme could be devised to safely recover. In practice though, the github.com/sylabs/sif
module is generally imported into applications which may already define a signal handler. Since these are global, there would need to be some scheme devised to coordinate, and in the end this would be quite cumbersome and clunky.
As an alternative, I propose we look at whether we really need to use memory mapping in the first place. On the surface, it wouldn't appear that we're getting much benefit from it.
There are instances where we need to search by default group. We should add a const
for that.
Remove deprecated fields from type FileImage
. Remove any other unnecessary fields, and provide accessors as necessary.
The README.md
contains instructions on how to get the module:
Lines 17 to 21 in a7d8315
As of v2.0.0-beta.1
, that command returns an error:
$ go get github.com/sylabs/sif/v2
github.com/sylabs/sif/v2: no Go source files
package github.com/sylabs/sif/v2: build constraints exclude all Go files in /home/adam/src/sif
It looks like this will resolve itself in a future version of Go (golang/go#29268), but in the meantime we should fix our docs.
The primary purpose of the pkg/siftool
package is to reduce CLI code duplication between cmd/siftool
and the SingularityCE code base. This function currently exposes several functions that return a *cobra.Command
. This API could be reduced to a single call that adds the SIF command hierarchy to an existing *cobra.Command
. Something like:
func AddCommands(*cobra.Command, ...Option) error
The siftool header
command includes a Magic
field:
$ siftool header pkg/integrity/testdata/images/one-group.sif
Launch: #!/usr/bin/env run-singularity
Magic: SIF_MAGIC
Version: 01
Arch: 386
ID: 6ecc76b7-a497-4f7f-9ebd-8da2a04c6be1
Ctime: 2020-05-22 19:30:59 +0000 UTC
Mtime: 2020-05-22 19:30:59 +0000 UTC
Dfree: 46
Dtotal: 48
Descoff: 4096
Descrlen: 27KB
Dataoff: 32768
Datalen: 4KB
Magic
is a low level implementation detail which must contain "SIF_MAGIC"
for the SIF to be loaded in the first place, and cannot be set/modified by the user in the first place. I would argue it can be removed from the siftool header
command output, which will also reduce the API surface.
The SIF code base currently has test images in two locations in the repo:
pkg/integrity/testdata/images/
pkg/sif/testdata/
It would be nice to centralize these, and provide code that can generate existing images, and generate new images as required. pkg/integrity/testdata/gen_sifs.go contains Go code to do this for the images in pkg/integrity
, but it is skipped by tools such as go build
and golangci-lint
as it's in a testdata
directory.
I propose we move this code to its own internal package, and place the resulting images nearby.
Several portions of the integrity
package return object IDs rather than object descriptors. A quick survey of usage shows that in the vast majority of cases, the descriptor is immediately obtained by the caller. We should just return the descriptor directly, where applicable.
The repo currently uses a shell script to build that calls out to external tools on the build host (cat
, echo
, git
, sed
). We should consider using mage
to build/install, so that all build dependencies are versioned in the go.mod
.
The sif
API currently has three different funcs to load a container:
func LoadContainer(filename string, rdonly bool) (FileImage, error)
func LoadContainerFp(fp ReadWriter, rdonly bool) (fimg FileImage, err error)
func LoadContainerReader(b *bytes.Reader) (fimg FileImage, err error)
In conjunction with #75, we should consolidate these into a single function, and have it return a *FileImage
to be consistent with CreateContainer
.
(*Descriptor).GetReadSeeker
currently returns an io.ReadSeeker
, but current usage could be satisfied by the simpler io.Reader
interface.
With the introduction of GetReader
suggested by #14, I believe we should consider a breaking change to remove GetReadSeeker
in the v2 API.
(*Descriptor).GetReadSeeker
currently returns an io.ReadSeeker
, but I cannot find any usage of it that would not be satisfied by the simpler io.Reader interface. In the interest of simplifying the underlying implementation in a major version bump, I think we should consider deprecating GetReadSeeker
and introducing:
func (d *Descriptor) GetReader(fimg *FileImage) io.Reader
This change can be introduced in a minor version, which will provide time before the planned removal of GetReadSeeker
in a v2 API.
Running go test
causes an unclean working directory. If you start with a clean checkout:
$ git status
On branch master
Your branch is up to date with 'upstream/master'.
nothing to commit, working tree clean
Run the unit tests:
$ go test ./...
? github.com/sylabs/sif/cmd/siftool [no test files]
? github.com/sylabs/sif/internal/app/siftool [no test files]
ok github.com/sylabs/sif/pkg/integrity 0.157s
ok github.com/sylabs/sif/pkg/sif 0.047s
? github.com/sylabs/sif/pkg/siftool [no test files]
And pkg/sif/testdata/testcontainer.sif
has been modified:
$ git status
On branch master
Your branch is up to date with 'upstream/master'.
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: pkg/sif/testdata/testcontainer.sif
no changes added to commit (use "git add" and/or "git commit -a")
We should ensure that the unit tests clean up after themselves.
The SIF implementation currently specifies string values for various CPU architectures:
Lines 99 to 109 in b6599fa
Users of the github.com/sylabs/sif/pkg/sif
package receive these via *Descriptor.GetArch
, and pass them via *DescriptorInput.SetPartExtra
.
The Go runtime exposes the current CPU architecture via runtime.GOARCH
.
The SIF package provides sif.GetGoArch
and sif.GetSIFArch
for this purpose, but it would be easier for users if they could deal directly with GOARCH
as exposed by the Go runtime.
I believe we should switch to use native Go architecture strings in relevant APIs, and remove GetGoArch
/GetSIFArch
from the API.
The cmd/siftool
and pkg/siftool
packages contain duplicate CLI implementations. We should DRY the code by refactoring cmd/siftool
to use pkg/siftool
.
(*Descriptor).GetData
does not currently return a detailed error
, instead returning a nil
result when an error is encountered. This leads to code that does not explicitly handle errors at all, for example:
Lines 132 to 135 in 970418e
While (*Descriptor).GetReadSeeker
can be used to obtain a detailed error, a io.ReadSeeker
is less convenient than a []byte
in some use cases.
I believe we should consider a breaking change in the function signature to:
func (*Descriptor) GetData(*FileImage) ([]byte, error)
Each Descriptor
has an Extra
field that includes extra metadata for certain data types. There are a number of methods to get individual values from this field:
These eight functions apply to three message types. In general, it is necessary to call more than one accessor for a given data type to do anything useful with the data object. This results in redundant parsing, and needlessly verbose code.
I propose consolidation of these into three methods in the v2 API:
GetCryptoMessageMetadata
: consolidates GetFormatType
and GetMessageType
GetPartitionMetadata
: consolidates GetArch
, GetFormatType
, and GetMessageType
GetSignatureMetadata
: consolidates GetEntity
, GetEntityString
and GetHashType
The github.com/satori/go.uuid
module used by this project does not appear to be actively maintained (ref).
We should consider switching to the github.com/gofrs/uuid
package, or some other suitable alternative.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
๐ Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. ๐๐๐
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google โค๏ธ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.