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.

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

Singularity Image Support #125

Merged
merged 8 commits into from
Aug 2, 2022
Merged

Singularity Image Support #125

merged 8 commits into from
Aug 2, 2022

Conversation

tri-adam
Copy link
Contributor

@tri-adam tri-adam commented Apr 27, 2022

This PR is based on the work in #123, and is proposed as a potential way forward based on the feedback there and the discussion in the last community call. @Poluect has unfortunately had to step away from the Sylabs team on short notice, so I'm picking up where he left off.

This PR uses github.com/CalebQ42/squashfs to read the root filesystem contents. This is a pure Go library, and doesn't require a mount.

Structurally, this PR takes a different approach than #123, exposing Singularity Images via the existing func GetImageFromSource rather than adding a SIF-specific func GetSifImageFromSource. This adds a bit more complexity in stereoscope, but keeps the exported API tighter and makes it a lot easier to support SIF in projects consuming this module such as syft (I'll open a PR there shortly that uses pulls in this work.)

Related to anchore/syft#937, sylabs/sif#190

@tri-adam tri-adam marked this pull request as ready for review April 27, 2022 20:28
@tri-adam
Copy link
Contributor Author

Hi @wagoodman, just checking in to see if there's anything I can do to push this forward. Thanks!

@jonasagx
Copy link
Contributor

jonasagx commented May 18, 2022

Hey @tri-adam thank you for working on this feature, I ran CI validations and there are issues with licenses from these dependencies:

$ make static-analysis
Allow Rules: [BSD.* MIT.* Apache.* MPL.*]
Unallowable license (GPL-2.0) from "github.com/rasky/go-lzo"
Unallowable license (CC0-1.0) from "github.com/therootcompany/xz"

@wagoodman
Copy link
Contributor

@tri-adam we're going to review these new licenses internally to ensure we can pull in these dependencies safely (from a legal perspective), I'll report back soon

@wagoodman wagoodman self-requested a review May 19, 2022 19:11
@wagoodman wagoodman self-assigned this May 19, 2022
@tri-adam
Copy link
Contributor Author

tri-adam commented May 24, 2022

Hey @tri-adam thank you for working on this feature, I ran CI validations and there are issues with licenses from these dependencies:

$ make static-analysis
Allow Rules: [BSD.* MIT.* Apache.* MPL.*]
Unallowable license (GPL-2.0) from "github.com/rasky/go-lzo"
Unallowable license (CC0-1.0) from "github.com/therootcompany/xz"

@tri-adam we're going to review these new licenses internally to ensure we can pull in these dependencies safely (from a legal perspective), I'll report back soon

@jonasagx @wagoodman thanks. It looks like both of those are pulled in by github.com/CalebQ42/squashfs:

$ go mod why github.com/rasky/go-lzo github.com/therootcompany/xz
# github.com/rasky/go-lzo
github.com/anchore/stereoscope/pkg/file
github.com/CalebQ42/squashfs
github.com/CalebQ42/squashfs/internal/compression
github.com/rasky/go-lzo

# github.com/therootcompany/xz
github.com/anchore/stereoscope/pkg/file
github.com/CalebQ42/squashfs
github.com/CalebQ42/squashfs/internal/compression
github.com/therootcompany/xz

Let me know the results of the legal review, thanks.

@wagoodman
Copy link
Contributor

wagoodman commented May 25, 2022

Hey @tri-adam , sorry to report that GPL-2.0 licensed dependencies cannot be introduced into the codebase at this time (CC0-1.0 is ok to introduce though). Is seems that the LZO license is not something that could be controlled by the repo owner too:

Being a straightforward port of the original source code, it shares the same license (GPLv2) as I can't possibly claim any copyright on it.

There are a couple of paths forward:

  1. we can try to find an lzo alternative go implementation (I couldn't find one after a quick search)
  2. roll our own implementation of LZO (this looks pretty hard, given that LZO is a collection of a dozen or so algorithms that are very poorly documented)
  3. is LZO required for the SIF image functionality? if not, then we could fork the squashfs lib and remove the dependency? (haven't dug into this option at all to check feasibility)
  4. we could head back down the mount PR and work more closely on an opt-in approach
  5. insert-other-idea-here <-- open to more suggestions!

Thanks for bearing with us on this 🙏

tri-adam added a commit to tri-adam/squashfs that referenced this pull request May 26, 2022
LZO support requires the github.com/rasky/go-lzo module, which is GPLv2
licensed and thus not suitable for inclusion in some projects (see
discussion in anchore/stereoscope#125).
tri-adam added a commit to sylabs/squashfs that referenced this pull request May 26, 2022
LZO support requires the github.com/rasky/go-lzo module, which is GPLv2
licensed and thus not suitable for inclusion in some projects (see
discussion in anchore/stereoscope#125).

Signed-off-by: Adam Hughes <[email protected]>
@tri-adam
Copy link
Contributor Author

tri-adam commented May 26, 2022

There are a couple of paths forward:

  1. we can try to find an lzo alternative go implementation (I couldn't find one after a quick search)
  2. roll our own implementation of LZO (this looks pretty hard, given that LZO is a collection of a dozen or so algorithms that are very poorly documented)
  3. is LZO required for the SIF image functionality? if not, then we could fork the squashfs lib and remove the dependency? (haven't dug into this option at all to check feasibility)
  4. we could head back down the mount PR and work more closely on an opt-in approach
  5. insert-other-idea-here <-- open to more suggestions!

Took a stab at option 3, removing LZO support from the underlying module, and it's reasonably easy (CalebQ42/squashfs@v0.5.4...sylabs:remove-lzo-v0.5.4).

SingularityCE does not appear to rely on LZO compression, so I don't foresee any negative impacts in terms of functionality.

Still some further testing to do, but it's a start!

@wagoodman
Copy link
Contributor

SingularityCE does not appear to rely on LZO compression, so I don't foresee any negative impacts in terms of functionality.

great news!

go.mod Show resolved Hide resolved
@@ -76,6 +79,8 @@ func ParseSourceScheme(source string) Source {
return OciTarballSource
case "oci-registry", "registry":
return OciRegistrySource
case "singularity-image":
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could use a shorter name here such as singularity or sif (and "image" is redundant here) -- what do you think @tri-adam ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I went ahead and shorted it to singularity, however, I'll hold off on pulling the trigger on merging until I hear back from you on this particular change

Copy link
Contributor

Choose a reason for hiding this comment

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

sif would likely be my suggestion - as that's what has been adopted by support brought into containers/image and hence podman which are using sif as the 'transport' name.

Copy link
Contributor

@wagoodman wagoodman left a comment

Choose a reason for hiding this comment

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

FYI -- force pushed to resolve merge conflict in go.mod/go.sum

@wagoodman
Copy link
Contributor

wagoodman commented Jun 9, 2022

@tri-adam right now integration tests are failing due to lack of image types covered (since singularity was added). The next step is to add testing that covers:

  • symlinks:
    name string
    source string
    }{
    {
    name: "FromTarball",
    source: "docker-archive",
    },
    {
    name: "FromDocker",
    source: "docker",
    },
    {
    name: "FromPodman",
    source: "podman",
    },
    {
    name: "FromOciTarball",
    source: "oci-archive",
    },
    {
    name: "FromOciDirectory",
    source: "oci-dir",
    },
    }
  • a parsing go case:
    {
    name: "FromTarball",
    source: "docker-archive",
    imageMediaType: v1Types.DockerManifestSchema2,
    layerMediaType: v1Types.DockerLayer,
    tagCount: 1,
    },
    {
    name: "FromDocker",
    source: "docker",
    imageMediaType: v1Types.DockerManifestSchema2,
    layerMediaType: v1Types.DockerLayer,
    // name:hash
    // name:latest
    tagCount: 2,
    },
    {
    name: "FromPodman",
    source: "podman",
    imageMediaType: v1Types.DockerManifestSchema2,
    layerMediaType: v1Types.DockerLayer,
    tagCount: 2,
    },
    {
    name: "FromOciTarball",
    source: "oci-archive",
    imageMediaType: v1Types.OCIManifestSchema1,
    layerMediaType: v1Types.OCILayer,
    tagCount: 0,
    },
    {
    name: "FromOciDirectory",
    source: "oci-dir",
    imageMediaType: v1Types.OCIManifestSchema1,
    layerMediaType: v1Types.OCILayer,
    tagCount: 0,
    },

I think the one thing I'm hung up on is that these tests depend on building the same image and obtaining it in different ways -- is there a way to nicely achieve that in testing with singularity images? If not, that's ok, we can make one off tests to assert this functionality (in integration or unit-level testing).

Shout out if there is anything I can do to clarify or help!

@dtrudg
Copy link
Contributor

dtrudg commented Jun 10, 2022

Hi @wagoodman. Thanks for the review and comments on this! I work with @tri-adam at Sylabs and might be picking this work up from him for a short time.

I'll look into the tests in detail as soon as possible. With regard to building a SIF, within the workflow, that would be possible from the docker image that is generated here with an install of Singularity. We do package SingularityCE ourselves for Ubuntu, with those packages hosted on GitHub releases ... but I'm not sure whether you'd want to add an install into the CI workflows?

A final thought is that we could get close enough to a 'real' Singularity built SIF by directly leveraging mksquashfs and calling the github.com/sylabs/sif SIF code we are importing to construct a simple SIF.... but perhaps this is a bit too far removed from the intent to test against a real Singularity container image?

@wagoodman
Copy link
Contributor

wagoodman commented Jun 21, 2022

hey @dtrudg 👋

a. [it] would be possible from the docker image that is generated here with an install of Singularity...I'm not sure whether you'd want to add an install into the CI workflows?

b. ... we could get close enough to a 'real' Singularity built SIF by directly leveraging mksquashfs and calling the github.com/sylabs/sif SIF code we are importing to construct a simple SIF.

I'm ok with either approach, my only hesitation with option a is if the install time is long enough to make PR validations much slower --but if that is the more kosher approach then that might be an OK trade off.

The goal of any approach is to make certain that the content API of stereoscope functions with the new image type in a realistic setting (querying files by a glob or path, getting file contents, resolving symlinks properly, etc).

What's the approach that you suggest / lean towards?

@dtrudg
Copy link
Contributor

dtrudg commented Jun 21, 2022

a. [it] would be possible from the docker image that is generated here with an install of Singularity...I'm not sure whether you'd want to add an install into the CI workflows?

b. ... we could get close enough to a 'real' Singularity built SIF by directly leveraging mksquashfs and calling the github.com/sylabs/sif SIF code we are importing to construct a simple SIF.

I'm ok with either approach, my only hesitation with option a is if the install time is long enough to make PR validations much slower --but if that is the more kosher approach then that might be an OK trade off.

The goal of any approach is to make certain that the content API of stereoscope functions with the new image (querying files by a glob or path, getting file contents, resolving symlinks properly, etc).

What's the approach that you suggest / lean towards?

Thanks @wagoodman - I can work it up with (a) and if the impact is too high we can switch to (b). It would be nicer to actually build the container with 'real' Singularity, than construct it. ourselves. Hopefully I'll have time to put something together for this during the week here.

@dtrudg
Copy link
Contributor

dtrudg commented Jun 27, 2022

Hi @wagoodman - I've taken a go at fitting some testing for singularity SIFs in with the existing image_simple / image_symlinks.

Would be grateful for any feedback on whether this approach looks acceptable, and worth tidying up further, or we should go for separate tests. Many thanks for your patience!

@dtrudg
Copy link
Contributor

dtrudg commented Jun 28, 2022

I'm sorry for the test failure due to the apt install... that was a 🤦

Adam Hughes and others added 6 commits August 1, 2022 15:10
Use github.com/CalebQ42/squashfs module to read the contents of
Singularity Image Format (SIF) container images without mounting them.

Expose Singularity Images via GetImageFromSource rather than adding a
SIF-specific GetSifImageFromSource by providing a GGCR v1.Image
implementation backed by a SIF. This avoids the requirement of separate
code paths within consumers of stereoscope such as syft.

Signed-off-by: Adam Hughes <[email protected]>
Switch to fork of github.com/CalebQ42/squashfs, which removes the
GPLv2-licensed indirect github.com/rasky/go-lzo dependency.

Signed-off-by: Adam Hughes <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
Modify the TestSimpleImage and TestImageSymlinks code to incorporate
testing of a singularity sif image source.

This requires signficant adaptations as singularity squashes
containers down to a single layer.

Singularity is expected to be available, and is is now installed in
the ci-bootstrap Makefile target (from a GitHub release).

Signed-off-by: David Trudgian <[email protected]>
@dtrudg
Copy link
Contributor

dtrudg commented Aug 1, 2022

Hi - I've re-based again onto the latest main. Looks like the last CI run prior to the rebase did pass, so would be grateful for any feedback on the testing approach, and what cleanup might be needed here. Many thanks!

@wagoodman
Copy link
Contributor

sorry for the delay @dtrudg , approved the workflow and are reviewing the changes now.

Copy link
Contributor

@wagoodman wagoodman left a comment

Choose a reason for hiding this comment

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

The only change I made was making the singularity call more portable to help development on macs -- nothing too major. Thanks for the enhancement and updating the tests @tri-adam @dtrudg !!

@wagoodman wagoodman merged commit 49d33a1 into anchore:main Aug 2, 2022
@dtrudg
Copy link
Contributor

dtrudg commented Aug 2, 2022

Many thanks @wagoodman - I'll take a look at the related syft PR next.

Thanks for your help, and apologies this was pretty slow from our side.

@wagoodman
Copy link
Contributor

wagoodman commented Aug 2, 2022

@dtrudg no need to apologize, this slid on my side as well 👍 I'm looking at anchore/syft#974 and just pulled in the latest stereoscope and I think I'm seeing some integration issues, but too soon to tell really. I'll dig further and see what comes up. it works great (I spoke too soon 😄 )

@wagoodman
Copy link
Contributor

@dtrudg I updated anchore/syft#974 and it seems to be working great -- did you want to add anything or give it a shot before merging?

@dtrudg
Copy link
Contributor

dtrudg commented Aug 2, 2022

@dtrudg I updated anchore/syft#974 and it seems to be working great -- did you want to add anything or give it a shot before merging?

@wagoodman I'll take a quick check within the next couple of hours when I'm out of a couple of calls, and ping you over on that PR if that's okay? I think it should be good but don't want to overlook anything given I'm picking it up from tri-adam :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants