Create checksum for exported iso file #1118

rabajaj0509 · 2018-06-14T18:20:51Z

When we export a .iso file, we must also export a checksum file with it.

dkliban · 2018-06-14T18:30:44Z

plugins/pulp_rpm/plugins/distributors/export_distributor/generate_iso.py

@@ -87,6 +88,17 @@ def _make_iso(file_list, target_dir, output_dir, filename):
    if status != 0:
        log.error("Error creating iso %s; status code: %d; output: %s" % (file_path, status, out))
    else:
+        testFile = open(file_path, "rb")


I would add a method called _add_digests_file(file_path) and then call it from here.

testFile should probably be renamed to something more appropriate like exportedISO or ISOFile

dkliban · 2018-06-14T18:34:27Z

plugins/pulp_rpm/plugins/distributors/export_distributor/generate_iso.py

+        while True:
+            piece = testFile.read(1024)
+            if piece:
+                h.update(piece)


Since we want to calculate multiple digests, you should probably be iterating a list of hashers and updating all of them.

Some examples from Pulp 3:
https://github.com/pulp/pulp/blob/master/pulpcore/pulpcore/app/files.py#L13
https://github.com/pulp/pulp/blob/master/pulpcore/pulpcore/app/files.py#L44

pulpbot · 2018-06-14T19:27:47Z

Can one of the admins verify this patch?

pulpbot · 2018-06-14T19:28:06Z

Can one of the admins verify this patch?

dkliban · 2018-06-15T13:17:59Z

plugins/pulp_rpm/plugins/distributors/export_distributor/generate_iso.py

+    hashers = {}
+    for hasher in hashlib.algorithms_guaranteed:
+        hashers[hasher] = getattr(hashlib, hasher)()
+        while True:


This whilte loop should not be inside this loop. Instead you should add another loop inside the while loop. The inner loop should update each hasher.

dkliban · 2018-06-15T19:47:52Z

plugins/pulp_rpm/plugins/distributors/export_distributor/generate_iso.py

+        while True:
+            piece = exportedISO.read(1024)
+            if piece:
+                hashers[hasher].update(piece)


You need to put a loop here that updates all of the hashers with this piece.

pep8speaks · 2018-06-15T21:02:29Z

Hello @rahulbajaj0509! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on July 12, 2018 at 13:02 Hours UTC

rabajaj0509 · 2018-06-15T21:29:48Z

As required, we have two files in the same folder with .iso and .iso.DIGEST

[vagrant@pulp2 1529096361.37]$ ll
total 540
-rw-r--r--. 1 apache apache 546816 Jun 15 20:59 yolo-2018-06-15T20.59-01.iso
-rw-r--r--. 1 apache apache    608 Jun 15 20:59 yolo-2018-06-15T20.59-01.iso.DIGEST

Output:

[vagrant@pulp2 1529096361.37]$ cat yolo-2018-06-15T20.59-01.iso.DIGEST 
b7c3e3128c8ec81d16e52b7546c64523cb89dc3e *.yolo-2018-06-15T20.59-01.iso
d14a028c2a3a2bc9476102bb288234c415a2b01f828ea62ac5b3e42f *.yolo-2018-06-15T20.59-01.iso
38b060a751ac96384cd9327eb1b1e36a21fdb71114be07434c0cc7bf63f6e1da274edebfe76f65fbd51ad2f14898b95b *.yolo-2018-06-15T20.59-01.iso
e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 *.yolo-2018-06-15T20.59-01.iso
cf83e1357eefb8bdf1542850d66d8007d620e4050b5715dc83f4a921d36ce9ce47d0d13c5d85f2b0ff8318d2877eec2f63b931bd47417a81a538327af927da3e *.yolo-2018-06-15T20.59-01.iso
d41d8cd98f00b204e9800998ecf8427e *.yolo-2018-06-15T20.59-01.iso

dkliban · 2018-06-18T02:00:09Z

plugins/pulp_rpm/plugins/distributors/export_distributor/generate_iso.py

+    for hasher in hashlib.algorithms_guaranteed:
+        hashers[hasher] = getattr(hashlib, hasher)()
+        chunk = 0
+        while chunk != b'':


The content of the .DIGEST file is incorrect. Only one of the checksums matches the sum calculated on command line.

You should un-indent the code that reads the file. Line 104 needs to be inside a loop that looks exactly like line 99.

bmbouter · 2018-06-18T20:43:52Z

@rahulbajaj0509 if you rebase against the newest master then you'll get the necessary pep8speaks fixes onto your branch will remove the pep8 comment.

dkliban · 2018-06-18T23:23:30Z

plugins/pulp_rpm/plugins/distributors/export_distributor/generate_iso.py

+    digestFile = open(file_path + ".DIGEST", "w+")
+    hashers = {}
+    for hasher in hashlib.algorithms_guaranteed:
+        hashers[hasher] = getattr(hashlib, hasher)()


for hasher in hashlib.algorithms_guaranteed: hashers[hasher] = getattr(hashlib, hasher)() chunk = exportedISO.read(1024) while len(chunk) > 0: for hasher in hashlib.algorithms_guaranteed: hashers[hasher].update(chunk) chunk = exportedISO.read(1024)

@dkliban thanks, for your input on this.
I was trying something like:

import hashlib import os file_path="/home/vagrant/abc.iso" exportedISO = open("/home/vagrant/abc.iso", "rb") digestFile = open("/home/vagrant/abc.iso.DIGEST", "w+") hashers = {} for hasher in hashlib.algorithms_guaranteed: hashers[hasher] = getattr(hashlib, hasher)() exportedISO.seek(0) chunk = exportedISO.read(1024) while len(chunk) > 0: hashers[hasher].update(chunk) chunk = exportedISO.read(1024) digestFile.write("{} *{}\n".format(hashers[hasher].hexdigest(), os.path.basename(file_path))) exportedISO.close() digestFile.close()

This approach gave the right output but parsed the file multiple times, as pointed
out by you, is not an appropriate approach.

Thank you @dkliban for your inputs :)

dkliban

The code looks good. You just need to add some documentation. A release note for 2.17.0 should be added here: https://github.com/pulp/pulp_rpm/tree/master/docs/user-guide/release-notes
You will need to create a 2.17.x.rst file in there that has a similar format to 2.16.x.rst. In that file provide a brief description of this new feature.

The recipe section here https://github.com/pulp/pulp_rpm/blob/master/docs/user-guide/recipes.rst#export-repositories-and-repository-groups should be updated with information about the .DIGESTS file

Please also add a sentence at the end of the description here https://github.com/pulp/pulp_rpm/blob/master/docs/tech-reference/export-distributor.rst#export-distributors that says that along with the ISO there will also be a *.iso.DIGESTS file published.

Thank you for all your work!

dkliban · 2018-06-19T12:43:08Z

plugins/pulp_rpm/plugins/distributors/export_distributor/generate_iso.py

        log.info('Successfully created iso %s' % file_path)


+def _add_digest_file(file_path):


Please add a docstring that describes what this method does.

dkliban · 2018-06-19T12:46:28Z

plugins/pulp_rpm/plugins/distributors/export_distributor/generate_iso.py

        log.info('Successfully created iso %s' % file_path)


+def _add_digest_file(file_path):
+    exportedISO = open(file_path, "rb")
+    digestFile = open(file_path + ".DIGEST", "w+")


please change .DIGEST to .DIGESTS (plural)

rabajaj0509 · 2018-06-26T23:19:28Z

@dkliban done :)

dkliban

I think we will be ready to merge this PR after this set of changes. Thank you for doing this work! Good job!

dkliban · 2018-06-28T13:35:28Z

docs/tech-reference/export-distributor.rst

@@ -8,7 +8,8 @@ images, or to the directory of your choice as one or more yum repositories. The
 distributor uses the ID ``export_distributor``. The repository group distributor uses the ID
 ``group_export_distributor``. Exported repository ISOs will be published over HTTP or HTTPS at
 the path ``/pulp/exports/repo/<repo-id>/``, and exported repository group ISOs can be found at
-``/pulp/exports/repo_group/<group-id>/``.
+``/pulp/exports/repo_group/<group-id>/``. Along with the ISO there will also be a ``*.iso.DIGISTS``


Along with the ISO there will also be a *.iso.DIGESTS file published. This file will contain md5, sha1, sha224, sha256, sha384, and sha512 digests of the ISO.

dkliban · 2018-06-28T13:36:49Z

docs/user-guide/recipes.rst

@@ -342,7 +342,8 @@ The general workflow is as follows:

 Which, if publishing over HTTP, could be found at
 `http://localhost/pulp/exports/repo/demo-repo/ <http://localhost/pulp/exports/repo/demo-repo/>`_
-(adjust hostname and repo-id as necessary.)
+(adjust hostname and repo-id as necessary). Along with the ISO there will also be a 


Along with the ISO there will also be a *.iso.DIGESTS file published. This file will contain md5, sha1, sha224, sha256, sha384, and sha512 digests of the ISO.

dkliban · 2018-06-28T13:38:33Z

docs/user-guide/release-notes/2.17.x.rst

@@ -11,3 +11,8 @@ New Features
 * The `gpg_key_id` yum distributor configuration value was previously ignored
  unless `gpg_cmd` was configured.  It is now used with the default GPG signing
  command when `gpg_cmd` is not configured.
+
+* Added a `.DIGESTS` file that would calculate the checksums of the exported


The export distributor now generates a *.iso.DIGESTS file along with the ISO. This file will contain md5, sha1, sha224, sha256, sha384, and sha512 digests of the ISO.

dkliban · 2018-06-28T13:39:19Z

plugins/pulp_rpm/plugins/distributors/export_distributor/generate_iso.py

+    :param file_path: The file path to the ISO image file
+    :param file_path: str
+
+    :return: Create a *.iso.DIGESTS file


It doesn't return anything. You can remove the return docstring.

rabajaj0509 · 2018-06-28T14:33:21Z

@dkliban looks good?

daviddavis · 2018-06-28T14:59:53Z

plugins/pulp_rpm/plugins/distributors/export_distributor/generate_iso.py

+    digestFile = open(file_path + ".DIGESTS", "w+")
+    hashers = {}
+    for hasher in hashlib.algorithms_guaranteed:
+        hashers[hasher] = getattr(hashlib, hasher)()


This will fail on a FIPS environment. I think this should work:

from pulp.server.util import md5 as pulp_md5 # ... if hasher == 'md5': hashers[hasher] = pulp_md5() else: hashers[hasher] = getattr(hashlib, hasher)()

@daviddavis thanks 👍 now the pr is FIPS compliant too :)

daviddavis · 2018-06-28T15:14:08Z

plugins/pulp_rpm/plugins/distributors/export_distributor/generate_iso.py

+    for hasher in hashlib.algorithms_guaranteed:
+        if hasher == 'md5':
+            hashers[hasher] = pulp_md5()
+        else


s/else/else:/

dkliban

I tested this on FIPS enabled machine and it worked correctly. Thank you very much for the contribution.

dkliban · 2018-06-28T16:41:37Z

ok test

daviddavis · 2018-06-28T16:51:54Z

ok test

dkliban

The unit tests are failing.The failures can be seen here (toward the end): https://pulpadmin.fedorapeople.org/jenkins/jobs/unittest-pulp_rpm-pr/builds/1135/node-type=rhel7-np-FAILED.txt

You can run them on your local machine by executing inside pulp_rpm directory: python run-tests.py

dkliban

You should just mock the _add_digest_file() method and ensure that it's called once.

After this PR is merge, I will file an issue to improve the tests[0] in pulp-smash to look for the new DIGESTS file.

[0] https://github.com/PulpQE/pulp-smash/blob/master/pulp_smash/tests/pulp2/rpm/api_v2/test_export.py#L444-L486

When we export a .iso file, we must also export a checksum file with it. closes #3760 https://pulp.plan.io/issues/3760

daviddavis · 2018-07-12T15:22:25Z

ok test

dparalen · 2018-07-12T16:58:09Z

ok test

daviddavis · 2018-07-12T18:12:48Z

ok test

dkliban requested changes Jun 14, 2018

View reviewed changes

dkliban reviewed Jun 15, 2018

View reviewed changes

dkliban requested changes Jun 18, 2018

View reviewed changes

dkliban reviewed Jun 18, 2018

View reviewed changes

dkliban requested changes Jun 19, 2018

View reviewed changes

dkliban requested changes Jun 28, 2018

View reviewed changes

daviddavis reviewed Jun 28, 2018

View reviewed changes

dkliban approved these changes Jun 28, 2018

View reviewed changes

daviddavis approved these changes Jun 28, 2018

View reviewed changes

dkliban requested changes Jun 28, 2018

View reviewed changes

dkliban requested changes Jul 2, 2018

View reviewed changes

daviddavis force-pushed the master branch from 9e64a04 to c57c3fb Compare July 2, 2018 20:42

daviddavis changed the base branch from master to 2-master July 2, 2018 20:43

Create checksum for exported iso file

e53b8bf

When we export a .iso file, we must also export a checksum file with it. closes #3760 https://pulp.plan.io/issues/3760

daviddavis merged commit ff0c4b6 into pulp:2-master Jul 12, 2018

Create checksum for exported iso file #1118

Create checksum for exported iso file #1118

rabajaj0509 commented Jun 14, 2018

dkliban Jun 14, 2018

dkliban Jun 14, 2018

dkliban Jun 14, 2018

pulpbot commented Jun 14, 2018

pulpbot commented Jun 14, 2018

dkliban Jun 15, 2018

dkliban Jun 15, 2018

pep8speaks commented Jun 15, 2018 •

edited

rabajaj0509 commented Jun 15, 2018

dkliban Jun 18, 2018

bmbouter commented Jun 18, 2018

dkliban Jun 18, 2018 •

edited

rabajaj0509 Jun 19, 2018 •

edited

dkliban left a comment

dkliban Jun 19, 2018

dkliban Jun 19, 2018

rabajaj0509 commented Jun 26, 2018

dkliban left a comment

dkliban Jun 28, 2018

dkliban Jun 28, 2018

dkliban Jun 28, 2018

dkliban Jun 28, 2018

rabajaj0509 commented Jun 28, 2018

daviddavis Jun 28, 2018

rabajaj0509 Jun 28, 2018

daviddavis Jun 28, 2018

dkliban left a comment

dkliban commented Jun 28, 2018

daviddavis commented Jun 28, 2018

dkliban left a comment

dkliban left a comment

daviddavis commented Jul 12, 2018

dparalen commented Jul 12, 2018

daviddavis commented Jul 12, 2018

		log.info('Successfully created iso %s' % file_path)


		def _add_digest_file(file_path):

Create checksum for exported iso file #1118

Create checksum for exported iso file #1118

Conversation

rabajaj0509 commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pulpbot commented Jun 14, 2018

pulpbot commented Jun 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Jun 15, 2018 • edited

Comment last updated on July 12, 2018 at 13:02 Hours UTC

rabajaj0509 commented Jun 15, 2018

Choose a reason for hiding this comment

bmbouter commented Jun 18, 2018

dkliban Jun 18, 2018 • edited

Choose a reason for hiding this comment

rabajaj0509 Jun 19, 2018 • edited

Choose a reason for hiding this comment

dkliban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabajaj0509 commented Jun 26, 2018

dkliban 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

rabajaj0509 commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkliban left a comment

Choose a reason for hiding this comment

dkliban commented Jun 28, 2018

daviddavis commented Jun 28, 2018

dkliban left a comment

Choose a reason for hiding this comment

dkliban left a comment

Choose a reason for hiding this comment

daviddavis commented Jul 12, 2018

dparalen commented Jul 12, 2018

daviddavis commented Jul 12, 2018

pep8speaks commented Jun 15, 2018 •

edited

dkliban Jun 18, 2018 •

edited

rabajaj0509 Jun 19, 2018 •

edited