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

Add windows support to the processes resource #1878

Merged

Conversation

username-is-already-taken2
Copy link
Contributor

@username-is-already-taken2 username-is-already-taken2 commented May 31, 2017

Hi

This PR is my attempt to really fix #1851 rather than patch it, by adding windows support.

I've also added an exists method as I found it useful.

  • Add support for Windows

  • Add Exists Method

  • Create unit tests to include windows runner

  • Update the docs to include the an example for Exists

  • Update the docs to provide a windows example

    Exists Method example...

You are currently running on:

    OS platform:  ☺☻Windows Server 2012 R2 Standard☺☻
    OS family:  ☺☻windows☺☻
    OS release: ☺☻6.3.9600☺☻
inspec> describe processes('svchost') do
inspec>   it { should exist }
inspec> end

Profile: inspec-shell
Version: (not specified)

  Processes svchost
     [PASS]  should exist

Test Summary: 1 successful, 0 failures, 0 skipped

Any comments or feedback is appreciated.

Question regarding converting strings
Don't know if anyone has an opinion but I thought it was better to convert the cpu and memory stats to numbers to make it easier to use is something like is it greater than a number rather then use a regex to parse a string? (Hope that makes sense :) I saw that the pid,vsz,rss where already converted.

but thought it could be a breaking change which is why they are only converted on windows. Happy to strip out.

Well let me know what you think, happy to start work on the tests (something else to learn :)

Gary

@adamleff
Copy link
Contributor

@username-is-already-taken2 really loving your contributions - keep 'em coming!

Regarding casting the CPU/memory to numbers - you are correct that this could potentially be backward incompatible depending on how users have written their profiles. Hopefully, it wouldn't be (calling to_i on an Integer still returns an `Integer)... but you never know.

Our recommendation to users would be use the cmp matcher which will automatically try to convert the value to a number so a comparison will work. So I'd suggest simply leaving those conversions out and letting the cmp do its magic.

@username-is-already-taken2 username-is-already-taken2 force-pushed the gb/update_processes branch 2 times, most recently from 006f5ff to bff0a54 Compare June 4, 2017 20:12
@alexpop
Copy link
Contributor

alexpop commented Jun 5, 2017

Thanks for the contribution Gary!
Regarding the command mocking, take a look at this example:

https://github.com/chef/inspec/blob/v1.26.0/test/helper.rb#L186

https://github.com/chef/inspec/blob/v1.26.0/test/unit/mock/cmd/get-package-firefox

@username-is-already-taken2
Copy link
Contributor Author

Thanks very much @alexpop

Appreciate your help.

I'm building a list of things I want to add to README.md file, I'll include that.

I've added a some unit tests for windows to the processes_test.rb file using this MockLoader
resource = MockLoader.new(:windows).load_resource('processes', 'winlogon.exe') and while the powershell command is now mocked my resource object is nil and so the following tests fail

  it 'retrieves the users and states as arrays on windows os' do
    resource = MockLoader.new(:windows).load_resource('processes', 'winlogon.exe')
    _(resource.users.sort).must_equal ['NT AUTHORITY\\SYSTEM']
  end

  it 'process should exist' do
    resource = MockLoader.new(:windows).load_resource('processes', 'winlogon.exe')
    _(resource.exists?).must_equal true
  end

Yet I know the resource does work :(

inspec> describe processes('winlogon.exe') do
inspec>   its ('users') { should cmp 'NT AUTHORITY\\SYSTEM' }
inspec>   it { should exist }
inspec> end
 
Profile: inspec-shell
Version: (not specified)

  Processes winlogon.exe
     [PASS]  should exist
     [PASS]  users should cmp == "NT AUTHORITY\\SYSTEM"

Test Summary: 2 successful, 0 failures, 0 skipped

If I'm honest, I'm a little out of my depth with unit testing, I don't know if its my test or my resource :(

Anyone willing to help me out ?

@username-is-already-taken2
Copy link
Contributor Author

I think its something to do with the way the command get mocked, when the cmd is called and it reads in my sample data from get-process_processes its converting the control chars from \r\n to \n which throws the function as its expecting \r\n on windows

humm..... back to the drawing board.

[1] pry(#<MockLoader>)> cmd.call('get-process_processes')
=> #<struct Train::Transports::Mock::Connection::Command
 stdout=
  "PriorityClass,Id,CPU,PM,VirtualMemorySize,NPM,SessionId,Responding,StartTime,TotalProcessorTime,UserName,Path\nNormal,2456,0.296875,4808704,118202368,14576,1,True,5/31/2017 9:13:17 AM,00:00:00.2968750,WINVAGR-QQQNHPN\\Administrator,C:\\Windows\\system32\\mmc.exe\nHigh,396,0.15625,1323008,53710848,7776,1,True,5/31/2017 9:12:56 AM,00:00:00.1562500,NT AUTHORITY\\SYSTEM,C:\\Windows\\system32\\winlogon.exe\n",

@adamleff
Copy link
Contributor

adamleff commented Jun 5, 2017

@username-is-already-taken2 this is coming along quite nicely! How can we help you at this point?

Signed-off-by: username-is-already-taken2 <digitalgaz@hotmail.com>
@username-is-already-taken2 username-is-already-taken2 changed the title WIP - Add windows support to the processes resource Add windows support to the processes resource Jun 5, 2017
@username-is-already-taken2
Copy link
Contributor Author

Well @adamleff I think I'm there. Been a bit of a labour of love and such a learning experience :)

I think this is now ready for review.

@alexpop
Copy link
Contributor

alexpop commented Jun 6, 2017

windows_logo

Thank you Gary, great addition!

Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

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

Another great contribution, @username-is-already-taken2!

@adamleff adamleff added the Type: Enhancement Improves an existing feature label Jun 6, 2017
Copy link
Contributor

@arlimus arlimus left a comment

Choose a reason for hiding this comment

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

Amazing contribution, thank you Gary @username-is-already-taken2 !! 🎆

Also kudos @adamleff and @alexpop for your reviews and support 😄

@arlimus arlimus merged commit 871c626 into inspec:master Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants