Adding OCI image and a respective test class #734

mukultaneja · 2020-06-17T17:51:17Z

Following commit adds a class to read OCI images
along with tests to validate its functionality.

Work towards: #678

Signed-off-by: mukultaneja [email protected]

mukultaneja · 2020-06-17T17:54:10Z

@rnjudge should I update this config file under setup section to add an instruction to install skopeo utility?

tern/classes/oci_image.py

tests/test_class_oci_image.py

rnjudge · 2020-06-17T18:45:41Z

@rnjudge should I update this config file under setup section to add an instruction to install skopeo utility?

From what I can tell, circleci only supports ubuntu 1604 hosts for the test and skopeo needs 1804, correct? If this is the case, we should disable circleci tests and only use github actions to run the tests since github hosts use ubuntu 1804 as the default latest linux image. I think we should address this in a separate PR, though.

rnjudge · 2020-06-17T18:50:12Z

In general, some comments in your code would be nice :) This is helpful as we review the PR and also for future contributors. Note that for comments describing the functionality of a function we use the three backticks enclosures (''' coment ''') instead of # . # is used for single line comments in-line with the code. docker_image.py is a nice example of well-commented code for a new class.

mukultaneja · 2020-06-17T18:50:14Z

@rnjudge should I update this config file under setup section to add an instruction to install skopeo utility?

From what I can tell, circleci only supports ubuntu 1604 hosts for the test and skopeo needs 1804, correct? If this is the case, we should disable circleci tests and only use github actions to run the tests since github hosts use ubuntu 1804 as the default latest linux image. I think we should address this in a separate PR, though.

Yes, I checked and it uses 18.04.

rnjudge · 2020-06-17T18:51:58Z

@rnjudge should I update this config file under setup section to add an instruction to install skopeo utility?

From what I can tell, circleci only supports ubuntu 1604 hosts for the test and skopeo needs 1804, correct? If this is the case, we should disable circleci tests and only use github actions to run the tests since github hosts use ubuntu 1804 as the default latest linux image. I think we should address this in a separate PR, though.

Yes, I checked and it uses 18.04.

Ok, then let me disable circleci testing. Can you plan to open a separate PR to enable tests for images in the github config file (.github/workflows/pull_request.yml)?

mukultaneja · 2020-06-17T20:48:56Z

@rnjudge should I update this config file under setup section to add an instruction to install skopeo utility?

From what I can tell, circleci only supports ubuntu 1604 hosts for the test and skopeo needs 1804, correct? If this is the case, we should disable circleci tests and only use github actions to run the tests since github hosts use ubuntu 1804 as the default latest linux image. I think we should address this in a separate PR, though.

Yes, I checked and it uses 18.04.

Ok, then let me disable circleci testing. Can you plan to open a separate PR to enable tests for images in the github config file (.github/workflows/pull_request.yml)?

@rnjudge , can I raise an issue for this change?

rnjudge · 2020-06-17T21:42:01Z

@rnjudge should I update this config file under setup section to add an instruction to install skopeo utility?

From what I can tell, circleci only supports ubuntu 1604 hosts for the test and skopeo needs 1804, correct? If this is the case, we should disable circleci tests and only use github actions to run the tests since github hosts use ubuntu 1804 as the default latest linux image. I think we should address this in a separate PR, though.

Yes, I checked and it uses 18.04.

Ok, then let me disable circleci testing. Can you plan to open a separate PR to enable tests for images in the github config file (.github/workflows/pull_request.yml)?

@rnjudge , can I raise an issue for this change?

Please!

mukultaneja · 2020-06-22T18:00:23Z

In general, some comments in your code would be nice :) This is helpful as we review the PR and also for future contributors. Note that for comments describing the functionality of a function we use the three backticks enclosures (''' coment ''') instead of # . # is used for single line comments in-line with the code. docker_image.py is a nice example of well-commented code for a new class.

Done! Apologies for missing them at first place :)

nishakm · 2020-06-25T13:17:11Z

@mukultaneja I've triggered rerunning the builds so we will see if all the tests pass in a bit

nishakm

@mukultaneja it looks like you've just copied over the functionality in the DockerImage class. The only thing you will need to do in this class is to implement load_image. Here you will need to look at the tarball created with podman save --format oci-archive -o debian.tar debian:latest. You can use any extra functions to help with that.

nishakm · 2020-06-25T23:01:37Z

tern/classes/oci_image.py

+        to_dict: return a dict representation of the object
+    '''
+
+    def __init__(self, repotag=None, image_id=None, image_name=None):


I think we will have to use repotag here only. I will file an issue for that change to go through first.

The second may be to see if we can use the registry API to get the image. I can look at this further.

I think we will have to use repotag here only. I will file an issue for that change to go through first.

@nishakm I have resolved this comment in the commit.

tern/classes/oci_image.py

mukultaneja · 2020-06-26T18:08:46Z

@mukultaneja it looks like you've just copied over the functionality in the DockerImage class. The only thing you will need to do in this class is to implement load_image. Here you will need to look at the tarball created with podman save --format oci-archive -o debian.tar debian:latest. You can use any extra functions to help with that.

Could you please give me a little bit idea behind podman here? I am using below command to convert a docker image into OCI layout. Is the use of podman creating only one file which is index.json?

skopeo copy docker://<image_name> oci://<dir_name>/<image_name>:<tag_name>

tern/classes/oci_image.py

mukultaneja · 2020-07-06T19:28:53Z

@nishakm I have updated the code and tried to address all the given comments. Now in ths commit,

I am loading index.json only one time and reading all the other information such as manifest, config and layers from it. I am using image_path property for it.
Earlier, I was copying all the files inside working_dir location but now I am copying only layers and using the rest of the functionality as it is similar to docker images.
I have written a helper utility helpers.validate_image_path to validate the OCI image directory layout given as the input to the executable.
I am supporting two ways of image input oci:<image_location>:<image_tag> and oci:<image_name>:<image_tag> for example oci:/root/debian:latest and oci:debian:latest. In case of latter, I am assuming debain image layout is present in the current directory. I have already written method to check_image_string and parse_image_string similar to docker image.

Please review it once and share your feedback. I have also gone through the example you shared and if I understand it correctly then I think the architecture information is the key here to load the specific manifest. If it is correct then how should we get this information?

mukultaneja · 2020-07-06T19:33:09Z

tern/classes/oci_image.py

+            checksum_type = self.get_diff_checksum_type(image_index_data)
+            while layer_paths:
+                layer = ImageLayer(None, layer_paths.pop(0))
+                layer.set_checksum(checksum_type, layer.diff_id)


here layer.diff_id will be None

Turns out, if you are using a tool to convert from a docker image to an OCI image, you may be able to get the layer's diff_id. But in this case, the layer's checksum can be found from the manifest's layers list.

nishakm · 2020-07-10T15:31:52Z

Hi @mukultaneja, can you try to rebase your commit? I think your build will pass then. Sorry for the delay in replying.

Following commit adds a class to read OCI images along with tests to validate its functionality. Work towards: #678 Signed-off-by: mukultaneja <[email protected]>

mukultaneja · 2020-07-10T18:17:03Z

Hi @mukultaneja, can you try to rebase your commit? I think your build will pass then. Sorry for the delay in replying.

Hi @nishakm, no issues :). I just did rebase my branch and now I am only getting issues for importing missing modules import-error / Unable to import 'tern.analyze.oci' and I think it makes sense as of now.

nishakm

It looks like a couple of file changes are missing from this PR and the tests are referencing unknown modules.

Another thing I have found out from my experiments is that querying the Dockerhub API may get you an OCI image format. I still need to poke at that direction. So I am going to hold off on providing feedback on this PR until I have something concrete for you.

nishakm · 2020-07-10T19:49:55Z

tern/classes/oci_image.py

+            raise NameError("Image object initialized with no repotag")
+
+        # parse the repotag
+        repo_dict = general.parse_oci_image_string(self._repotag)


I guess I am expecting an addition of a function in general.py?

nishakm · 2020-07-10T19:51:39Z

tern/classes/oci_image.py

+        self._name = repo_dict.get('name')
+        self._tag = repo_dict.get('tag')
+        self._image_path = repo_dict.get('path')
+


I think what would be better is if this functionality got moved to the Image class so the DockerImage class can also take advantage of it.

yeah, I think self._name and self._tag are common so we can move it to Image class but self._image_path we have to keep it in oci_image.

nishakm · 2020-07-10T19:55:21Z

tern/classes/oci_image.py

+    def load_image(self):
+        '''Load OCI image metadata using manifest'''
+        try:
+            # validate OCI iamge's directory layout


s/iamge's/image's

sure. its a typo. my bad!

nishakm · 2020-07-10T19:57:17Z

tern/classes/oci_image.py

+        '''Load OCI image metadata using manifest'''
+        try:
+            # validate OCI iamge's directory layout
+            helpers.validate_image_path(self.image_path)


I think you can put this function in here instead of helpers.

ok. I am neutral to keep it here as well.

nishakm · 2020-07-10T19:59:31Z

tern/classes/oci_image.py

+            checksum_type = self.get_diff_checksum_type(image_index_data)
+            while layer_paths:
+                layer = ImageLayer(None, layer_paths.pop(0))
+                layer.set_checksum(checksum_type, layer.diff_id)


Turns out, if you are using a tool to convert from a docker image to an OCI image, you may be able to get the layer's diff_id. But in this case, the layer's checksum can be found from the manifest's layers list.

mukultaneja · 2020-07-11T15:19:51Z

@nishakm, I remember last time we discussed that we are not concerned to get diff_id for OCI images as primarily it is a docker image property. I think that is why you gave a comment to remove the method from the class.

mukultaneja · 2020-07-11T15:24:10Z

It looks like a couple of file changes are missing from this PR and the tests are referencing unknown modules.

Another thing I have found out from my experiments is that querying the Dockerhub API may get you an OCI image format. I still need to poke at that direction. So I am going to hold off on providing feedback on this PR until I have something concrete for you.

Sure. Please let me know if I can help with the experiment. Just one thing here to ask, is it going to change the oci image input to tern? If no, then cant we go with skopeo and avail the OCI image analysis first and keep updating it for better.

nishakm · 2020-07-13T14:28:22Z

It looks like a couple of file changes are missing from this PR and the tests are referencing unknown modules.
Another thing I have found out from my experiments is that querying the Dockerhub API may get you an OCI image format. I still need to poke at that direction. So I am going to hold off on providing feedback on this PR until I have something concrete for you.

Sure. Please let me know if I can help with the experiment. Just one thing here to ask, is it going to change the oci image input to tern? If no, then cant we go with skopeo and avail the OCI image analysis first and keep updating it for better.

It probably will in the sense that we don't need skopeo to do any of the conversion, but I will have to find out if that is true or not and how we can implement it. So stay tuned!

rnjudge reviewed Jun 17, 2020

View reviewed changes

tern/classes/oci_image.py Outdated Show resolved Hide resolved

rnjudge reviewed Jun 17, 2020

View reviewed changes

tests/test_class_oci_image.py Outdated Show resolved Hide resolved

rnjudge mentioned this pull request Jun 18, 2020

Remove circleci from running on pull requests #735

Closed

mukultaneja requested a review from rnjudge June 22, 2020 18:00

nishakm requested changes Jun 25, 2020

View reviewed changes

nishakm mentioned this pull request Jun 25, 2020

Deprecate instantiating a derived Image class with multiple options #747

Closed

nishakm requested changes Jun 30, 2020

View reviewed changes

mukultaneja requested a review from nishakm July 6, 2020 19:29

mukultaneja commented Jul 6, 2020

View reviewed changes

Adding OCI image and a respective test class

c0717f3

Following commit adds a class to read OCI images along with tests to validate its functionality. Work towards: #678 Signed-off-by: mukultaneja <[email protected]>

nishakm requested changes Jul 10, 2020

View reviewed changes

mukultaneja closed this Aug 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding OCI image and a respective test class #734

Adding OCI image and a respective test class #734

mukultaneja commented Jun 17, 2020

mukultaneja commented Jun 17, 2020

rnjudge commented Jun 17, 2020

rnjudge commented Jun 17, 2020

mukultaneja commented Jun 17, 2020

rnjudge commented Jun 17, 2020

mukultaneja commented Jun 17, 2020 •

edited

Loading

rnjudge commented Jun 17, 2020

mukultaneja commented Jun 22, 2020

nishakm commented Jun 25, 2020

nishakm left a comment

nishakm Jun 25, 2020

nishakm Jun 25, 2020

mukultaneja Jul 6, 2020

mukultaneja commented Jun 26, 2020

mukultaneja commented Jul 6, 2020 •

edited

Loading

mukultaneja Jul 6, 2020

nishakm Jul 10, 2020

nishakm commented Jul 10, 2020

mukultaneja commented Jul 10, 2020

nishakm left a comment

nishakm Jul 10, 2020

mukultaneja Jul 11, 2020

nishakm Jul 10, 2020

mukultaneja Jul 11, 2020

nishakm Jul 10, 2020

mukultaneja Jul 11, 2020

nishakm Jul 10, 2020

mukultaneja Jul 11, 2020

nishakm Jul 10, 2020

mukultaneja commented Jul 11, 2020 •

edited

Loading

mukultaneja commented Jul 11, 2020

nishakm commented Jul 13, 2020

Adding OCI image and a respective test class #734

Adding OCI image and a respective test class #734

Conversation

mukultaneja commented Jun 17, 2020

mukultaneja commented Jun 17, 2020

rnjudge commented Jun 17, 2020

rnjudge commented Jun 17, 2020

mukultaneja commented Jun 17, 2020

rnjudge commented Jun 17, 2020

mukultaneja commented Jun 17, 2020 • edited Loading

rnjudge commented Jun 17, 2020

mukultaneja commented Jun 22, 2020

nishakm commented Jun 25, 2020

nishakm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukultaneja commented Jun 26, 2020

mukultaneja commented Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishakm commented Jul 10, 2020

mukultaneja commented Jul 10, 2020

nishakm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukultaneja commented Jul 11, 2020 • edited Loading

mukultaneja commented Jul 11, 2020

nishakm commented Jul 13, 2020

mukultaneja commented Jun 17, 2020 •

edited

Loading

mukultaneja commented Jul 6, 2020 •

edited

Loading

mukultaneja commented Jul 11, 2020 •

edited

Loading