-
Notifications
You must be signed in to change notification settings - Fork 186
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
Using OCI images instead of Docker Images #1006
Using OCI images instead of Docker Images #1006
Conversation
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.
Sorry for reviewing this so late! Thanks for continuing to work on this! It looks good to me. I wonder if you can add some tests for the OCIImage class?
Also, there are some prospector errors that come up, but seem to be easy to resolve. Let me know if you're stuck with anything.
@nishakm @rnjudge, Please review this PR. I have made the required changes and tested with the below command
|
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.
@mukultaneja Sorry for taking so long to get back to you on this. I have one comment on the implementation as it is causing a problem with the security linter. Also, can you rebase your change so you can pick up the latest prospector changes?
@nishakm, I tried to get away with |
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.
@mukultaneja I've added a suggestion. I will also try to test the changes.
Start analyzing OCI images using tern Work towards: #948 Signed-off-by: Mukul Taneja <[email protected]>
Adding 'download_container_image` method Work towards: #948 Signed-off-by: mtaneja <[email protected]>
Updating OCI Image class with required methods to load image. Work towards: #948 Signed-off-by: Mukul Taneja <[email protected]>
@nishakm I made all the requested changes and this time all tests got passed too :) |
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.
@rnjudge Need your input on this PR :)
image_attr = general.parse_image_string(docker_image) | ||
oci_image = 'oci://{0}/{1}'.format( | ||
rootfs.working_dir, image_attr.get('name')) | ||
docker_image = 'docker://{0}'.format(docker_image) |
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 just found out that if we use docker://
rather than dockerdaemon
we encounter a TLS error on the registry side. I also found that crane is a much more lightweight tool to pull container images, and conforms with OCI's on-disk layout spec. @rnjudge Do you want to merge this PR and then let @mukultaneja update it to use crane?
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.
What is the effect of the TLS error on the registry side?
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.
You can't download the image, because the certificate has expired or something like that. Usually, you need to set an option to disable TLS checking on the client side.
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.
The disadvantage of switching to crane is that you have to download the binary from github rather than install it via the package manager. But I feel that the install is easy and the source code is included with the binary distribution.
Hi @mukultaneja, sorry again for the delay in reviewing! I tried running tern with these changes. I get the following error:
Also, could you please rebase your changes on top of main? |
@mukultaneja few other requests:
|
Hi @mukultaneja. I have included your changes in #1090. Please take a look. I will close this PR once the more updated one is merged. Thanks again for your work on this! |
Start analyzing OCI images using tern
Work towards: #948
Signed-off-by: Mukul Taneja [email protected]