Modify checksum logic to use system binaries #251

jerryaldrichiii · 2018-02-06T04:29:42Z

This modifies md5sum/sha256sum to use binaries on the target system vs Ruby on the machine running InSpec.

This fixes #235
This closes #236

Many thanks to @tarcinil for the initial work on this! It is greatly appreciated!

chris-rock

I like the improvements,since it saves a lot of time to transfer data, but we need to think about worst cases, where those commands are not available.

chris-rock · 2018-02-22T14:20:10Z

lib/train/file.rb

+        'digest -a md5'
+      else
+        'md5sum'
+      end


I think we need to think about the case where those commands are not available. In those case I would like to fall back to the old methods

Sounds good, added in last commit. Let me know what you think @chris-rock.

chris-rock · 2018-04-03T13:29:22Z

We are getting closer @jerryaldrichiii !!! I am going to request one other round for improvement. At this point we are detecting platforms and available commands again and again. Why not keep our decision for one connection. Therefore we have many implementations of the hash generation eg.:

md5
md5sum
rubymd5
...
Once we call the function the first time, we detect the right implementation and stick for to that decision while the connection stays available. Does this makes any sense?

jerryaldrichiii · 2018-06-04T18:14:58Z

Sure does @chris-rock. Let me know if the recent commits achieve what you're looking for.

chris-rock

Awesome work @jerryaldrichiii I added some style comments, nothing major. 👍

chris-rock · 2018-07-13T13:43:58Z

lib/train/file.rb

+
+    private
+
+    def perform_checksum(method)


lets call this perform_checksum_cmd

Technically you can call this method without setting the command before. I think we should either move the command detection into this method or assume it as an argument to minimize function side-effects.

chris-rock · 2018-07-13T13:47:01Z

lib/train/file.rb

+    # This pulls the content of the file to the machine running Train and uses
+    # Digest to perform the checksum. This is less efficient than using remote
+    # system binaries and can lead to incorrect results due to encoding.
+    def perform_ruby_checksum(method)


should this be called perform_checksum_ruby to keep names aligned?

Yup, I think that is fine. I've done a search and replace.

chris-rock · 2018-07-13T13:51:58Z

lib/train/file.rb

+      res = @backend.run_command(cmd)
+      return res.stdout.split(' ').first if res.exit_status == 0
+
+      perform_ruby_checksum(method)


I would return an error for that method instead of calling perform_ruby_checksum here

chris-rock · 2018-07-13T13:52:22Z

lib/train/file.rb

+      res = @backend.run_command(cmd)
+      return res.stdout.split("\r\n")[1].tr(' ', '') if res.exit_status == 0
+
+      perform_ruby_checksum(method)


As above, I would not perform the ruby checksum here, but return an error

chris-rock · 2018-07-13T13:53:21Z

lib/train/file.rb

+    end
+
+    def sha256sum
+      return perform_ruby_checksum(method) if defined?(@use_ruby_checksum)


I think method needs to be defined here

chris-rock · 2018-07-13T13:54:06Z

lib/train/file.rb

@@ -147,5 +129,85 @@ def mounted?

      !mounted.nil? && !mounted.stdout.nil? && !mounted.stdout.empty?
    end
+
+    def md5sum
+      return perform_ruby_checksum(method) if defined?(@use_ruby_checksum)


should this be perform_checksum_ruby(:md5)?

chris-rock · 2018-07-13T13:58:43Z

lib/train/file.rb

+
+      return perform_checksum(:md5) unless @backend.os.family == 'windows'
+
+      perform_checksum_windows(:md5)


I think this can be a bit more simple, something like:

method = :md5 if @hash_ruby_fallback hash, err = perform_checksum_windows(method) if os.family == 'windows' hash, err = perform_checksum_cmd(method) if os.family != 'windows' if err == nil return hash end end @hash_ruby_fallback = true return perform_checksum_ruby(method)

jerryaldrichiii · 2018-07-13T15:49:00Z

I took a different approach @chris-rock but the feedback was super valuable. Let me know if it is too divergent from what you had in mind.

jerryaldrichiii · 2018-07-18T06:14:22Z

I fixed the merge conflicts via the GUI. No sure how I feel, but hey we squash any way.

chris-rock

Great improvement @jerryaldrichiii

chris-rock · 2018-07-31T12:25:03Z

lib/train/file.rb

+                                         'sha256sum'
+                                       end
+
+                   perform_checksum_linux(@sha256_command)


should this be perform_checksum_unix?

Can't see why not. I've fixed it.

…. mocking tests to match Signed-off-by: Vern Burton <[email protected]>

Signed-off-by: Vern Burton <[email protected]>

Signed-off-by: Jerry Aldrich <[email protected]>

This removes the tests related to using `md5sum` or `sha356sum` on a FIFO file. These tests were not working previously and I was unable to make them work after my changes. I suspect that it is not possible since a FIFO file must be open at both ends simultaneously before you can do any I/O operations on it. See: https://linux.die.net/man/3/mkfifo Signed-off-by: Jerry Aldrich <[email protected]>

Signed-off-by: Jerry Aldrich <[email protected]>

jquick

Thanks @jerryaldrichiii

tas50 · 2018-08-15T23:18:54Z

This is great. Thanks!

jerryaldrichiii requested a review from a team February 6, 2018 04:29

jerryaldrichiii added the in progress label Feb 6, 2018

jerryaldrichiii force-pushed the ja/235-enable-remote-checksum branch 12 times, most recently from 6d61432 to 7dc6fa1 Compare February 6, 2018 23:56

jerryaldrichiii changed the title ~~WIP DO NOT MERGE: Modify checksum logic to use system binaries~~ Modify checksum logic to use system binaries Feb 7, 2018

miah approved these changes Feb 7, 2018

View reviewed changes

tarcinil mentioned this pull request Feb 9, 2018

Update Train::File to perform md5sum and sha256sum through train.connection #236

Closed

tarcinil approved these changes Feb 20, 2018

View reviewed changes

chris-rock suggested changes Feb 22, 2018

View reviewed changes

jerryaldrichiii force-pushed the ja/235-enable-remote-checksum branch from e8024d3 to 37b6108 Compare April 2, 2018 05:10

jerryaldrichiii force-pushed the ja/235-enable-remote-checksum branch from 8547322 to 0dcac56 Compare June 15, 2018 17:24

miah approved these changes Jun 18, 2018

View reviewed changes

chris-rock reviewed Jul 13, 2018

View reviewed changes

clintoncwolfe added Type: New Feature labels Jul 13, 2018

chris-rock approved these changes Jul 31, 2018

View reviewed changes

tarcinil and others added 11 commits July 31, 2018 10:42

using @backend.run_command to execute checksum locally on the machine…

74921a9

…. mocking tests to match Signed-off-by: Vern Burton <[email protected]>

removing digest/(md5/sha2) as they are no longer needed

0de0384

Signed-off-by: Vern Burton <[email protected]>

Move sha256sum/md5sum logic to OS class files

0c8d3cc

Signed-off-by: Jerry Aldrich <[email protected]>

Dry up code (many thanks to @miah!!!)

4320826

Signed-off-by: Jerry Aldrich <[email protected]>

Add checksum tests/support for QNX

7204337

Signed-off-by: Jerry Aldrich <[email protected]>

Add fallback to Ruby Digest if command isn't found

e4b87c2

Signed-off-by: Jerry Aldrich <[email protected]>

Modify checksum flow to reduce duplicate calls

c4e3497

Signed-off-by: Jerry Aldrich <[email protected]>

Add unit tests for Ruby md5sum and sha256sum

ee259ba

Signed-off-by: Jerry Aldrich <[email protected]>

Simplify all the things

9434a27

Signed-off-by: Jerry Aldrich <[email protected]>

Change Linux to Unix where appropriate

c5987b2

Signed-off-by: Jerry Aldrich <[email protected]>

jerryaldrichiii force-pushed the ja/235-enable-remote-checksum branch from ec2620f to c5987b2 Compare July 31, 2018 17:42

jquick approved these changes Aug 10, 2018

View reviewed changes

jquick merged commit 335cd3a into master Aug 10, 2018

jquick deleted the ja/235-enable-remote-checksum branch August 10, 2018 12:37

clintoncwolfe added Type: New Feature Adds new functionality and removed Type: New Feature labels Mar 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify checksum logic to use system binaries #251

Modify checksum logic to use system binaries #251

jerryaldrichiii commented Feb 6, 2018 •

edited

Loading

chris-rock left a comment

chris-rock Feb 22, 2018

jerryaldrichiii Apr 2, 2018

chris-rock commented Apr 3, 2018

jerryaldrichiii commented Jun 4, 2018

chris-rock left a comment

chris-rock Jul 13, 2018

chris-rock Jul 13, 2018

chris-rock Jul 13, 2018

jerryaldrichiii Jul 13, 2018

chris-rock Jul 13, 2018

chris-rock Jul 13, 2018

chris-rock Jul 13, 2018

chris-rock Jul 13, 2018

chris-rock Jul 13, 2018

jerryaldrichiii commented Jul 13, 2018

jerryaldrichiii commented Jul 18, 2018

chris-rock left a comment

chris-rock Jul 31, 2018

jerryaldrichiii Jul 31, 2018

jquick left a comment

tas50 commented Aug 15, 2018


		return perform_checksum(:md5) unless @backend.os.family == 'windows'

		perform_checksum_windows(:md5)

Modify checksum logic to use system binaries #251

Modify checksum logic to use system binaries #251

Conversation

jerryaldrichiii commented Feb 6, 2018 • edited Loading

chris-rock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-rock commented Apr 3, 2018

jerryaldrichiii commented Jun 4, 2018

chris-rock 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

jerryaldrichiii commented Jul 13, 2018

jerryaldrichiii commented Jul 18, 2018

chris-rock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jquick left a comment

Choose a reason for hiding this comment

tas50 commented Aug 15, 2018

jerryaldrichiii commented Feb 6, 2018 •

edited

Loading