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

Feat/skopeo #103

Closed
wants to merge 6 commits into from
Closed

Feat/skopeo #103

wants to merge 6 commits into from

Conversation

ivanstanev
Copy link
Contributor

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

Allow the plugin to process files saved by skopeo in the docker-archive format (equivalent to docker save). The format is slightly different than docker save so it needs small adjustments.

This change also allows to pass a file path to a docker-archive existing on the filesystem, which allows to:

  • skip calling docker save in the plugin
  • skip the "safe size" checks and perform static scanning anyway

How should this be manually tested?

Call .inspect() with the dockerArchivePath: string option.

Any background context you want to provide?

Team Runtime needs this for the kubernetes-monitor, we are removing the reliance on the Docker socket. We are using a tool called skopeo that works with container registries and is able to pull and save an image to disk in a docker save-style format (a.k.a. docker-archive).

With these changes we want to inform the plugin that the file is already present on disk, so it can be opened and inspected statically without using Docker.

What are the relevant tickets?

Modify the tar extraction logic to also process docker-archives as stored by Skopeo.
For kubernetes-monitor, the image is already pulled and saved as a docker-archive so there is no need to execute `docker save` by the plugin.

If a path to an archive is passed to the plugin, it will now attempt to use it directly.
@CLAassistant
Copy link

CLAassistant commented Sep 12, 2019

CLA assistant check
All committers have signed the CLA.

@@ -2,6 +2,5 @@
.idea/
bin/
npm-debug.log
dist/
Copy link
Contributor

Choose a reason for hiding this comment

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

is this on purpose? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

also all the files added from dist/ as part the effect of this can be removed I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karniwl I added the dist/ files because I was referencing the snyk-docker-plugin as a git branch inside package.json of kubernetes-monitor 😄For that to work there need to be some published/built assets in order to build our kubernetes-monitor container (I don't know why but that's the reality... npm install locally works but when inside the container/Dockerfile it just fails with missing files)

@ivanstanev
Copy link
Contributor Author

Closing; I had this opened only for visibility/comments 😄

@team-lumos
Copy link
Collaborator

🎉 This issue has been resolved in version 6.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

4 participants