-
Notifications
You must be signed in to change notification settings - Fork 683
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 support for virtualization resource #1803
Conversation
Signed-off-by: Takaaki Furukawa <takaaki.frkw@gmail.com>
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.
@tkak This is a great addition. Thank you @tkak Can you add documentation to this PR as well? The docs files are located in https://github.com/chef/inspec/tree/master/docs/resources
lib/resources/virtualization.rb
Outdated
end | ||
end | ||
|
||
def [](name) |
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 think we should use role
, system
and systems
as proper methods, since this is more similar to other resources
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.
Since you have proper methods defined below around like 45/46, can we remove this method now?
@chris-rock Thank you for your feedback! I've fixed my PR. |
d441b33
to
f031e08
Compare
Signed-off-by: Takaaki Furukawa <takaaki.frkw@gmail.com>
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.
@tkak thank you so much for your contribution! This appears to be your first time contributing to InSpec as well, so welcome to the community!
I left some feedback that I think will tidy up this new resource and make it easier to understand, add unit tests, etc. Please let me know what you think and how we can help.
Again, thanks again for your contribution!
docs/resources/virtualization.md.erb
Outdated
### Test for Docker | ||
|
||
describe virtualization.system do | ||
it { should eq 'docker' } |
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'd recommend using its
here like the example below if not just to be consistent and to ensure the user gets good CLI formatter output. Otherwise, it will say something like "expected docker to == vbox" rather than being clear that the virtualization
resource is the item under test.
docs/resources/virtualization.md.erb
Outdated
### Detect the virtualization platform | ||
|
||
if virtualization.system == 'vbox' | ||
describe package('name')do |
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.
Missing a space between )
and do
lib/resources/virtualization.rb
Outdated
end | ||
end | ||
|
||
def [](name) |
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.
Since you have proper methods defined below around like 45/46, can we remove this method now?
lib/resources/virtualization.rb
Outdated
end | ||
|
||
def collect_data_linux # rubocop:disable Metrics/MethodLength, Metrics/PerceivedComplexity, Metrics/CyclomaticComplexity, Metrics/AbcSize | ||
virtualization = Hashie::Mash.new unless virtualization |
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.
virtualization
appears to be a local variable in this method, is that true? If so, I'm unsure when the unless
would ever get hit.
lib/resources/virtualization.rb
Outdated
end | ||
|
||
def collect_data_linux # rubocop:disable Metrics/MethodLength, Metrics/PerceivedComplexity, Metrics/CyclomaticComplexity, Metrics/AbcSize | ||
virtualization = Hashie::Mash.new unless virtualization |
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.
Since collect_data_linux
is used by each of the three defined accessor methods, it could be called multiple times in a single test (i.e. the user had multiple its
statements). Should we cache the data in an instance variable and then return it if it's defined? That would eliminate multiple runs through this method.
lib/resources/virtualization.rb
Outdated
inspec.command('nova').exist? | ||
end | ||
|
||
def collect_data_linux # rubocop:disable Metrics/MethodLength, Metrics/PerceivedComplexity, Metrics/CyclomaticComplexity, Metrics/AbcSize |
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.
Wow, one heck of a method!
My main concern is that it tests for EVERY virtualization possibility, even if one has already matched. For example, if we detect the host is a Xen host, we'll still test for vbox, openstack, etc.
I wonder if it would make more sense to break apart each of the detections into smaller methods that return true if it matched, false if it didn't... example:
def detect_xen
return false unless inspec.file('/proc/xen').exist?
@virtualization_data[:system] = 'xen'
# etc...
true
end
Then collect_data_linux
could be written like so:
def collect_data_linux
# cache data in an instance var to avoid doing multiple detections for a single test
@virtualization_data ||= Hashie::Mash.new
return unless @virtualization_data.empty?
# each detect method will return true if it matched and was successfully
# able to populate @virtualization_data with stuff.
return if detect_xen
return if detect_openstack
return if detect_foo
end
I think this would make the code a bit more readable/manageable, and will also make it easier to test with each system detection being its own method.
What do you think, @tkak?
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.
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.
@tkak ah ha! That makes sense. I think the ohai implementation could be improved too, but when running in InSpec against a remote system, it's even more critical to address this so we don't remotely check file status more than we need to, etc.
Looking forward to your update!
db421fe
to
04d6b5f
Compare
98e2212
to
b96543c
Compare
Signed-off-by: Takaaki Furukawa <takaaki.frkw@gmail.com>
@adamleff Thank you for your review. I made some changes. Would you be able to check them? |
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.
@tkak thanks for your patience! Last week for ChefConf for us, so we're still catching up.
I think this is super-close. I have one comment/question/suggestion about @virtualization_data[:systems]
and I think that should do it!
@@ -0,0 +1,13 @@ | |||
# encoding: utf-8 | |||
if ENV['DOCKER'] |
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.
We could simplify this to:
if ENV['DOCKER'] && os.linux
# rest of code here
end
... and eliminate one indentation level. Not critical to fix here, just something for the future :)
lib/resources/virtualization.rb
Outdated
# cache data in an instance var to avoid doing multiple detections for a single test | ||
@virtualization_data ||= Hashie::Mash.new | ||
return unless @virtualization_data.empty? | ||
@virtualization_data[:systems] ||= Hashie::Mash.new |
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.
In the original Ohai implementation, I think the @virtualization_data[:systems]
hash was intended to collect all the virtualization data in case a host fell into more than one category. I don't think that's the case, and this likely will confuse users. Can I suggest that we remove this and all entries in this Hash? I think the primary use cases here are best served with the system
and role
keys that exist at the top-level. What do you think?
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.
@adamleff OK, I'll remove @virtualization_data[:systems]
hash. :)
lib/resources/virtualization.rb
Outdated
it { should eq 'guest' } | ||
end | ||
|
||
if virtualization.system == 'docker' |
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 would prefer to have a control block with only_if here:
control 'test' do
describe file('/var/tmp/foo') do
it { should be_file }
end
only_if { virtualization.system == 'docker' }
end
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.
@chris-rock @tkak Thanks this closes a long standing request of mine when we started developing the os profiles. Great work! :) 👍
Agree, it is also a great way to show an example of good use of only_if
as it is intended. Can't wait to start using this resource in the OS security profiles.
Signed-off-by: Takaaki Furukawa <takaaki.frkw@gmail.com>
@tkak well done! This is a great addition. Thanks for working with us to make this PR great. |
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.
💯
NOTE TO NEXT APPROVER: this should be squashed via GitHub UI instead of merged, please.
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.
Awesome contribution, thank you so much @tkak !! 😄
Kudos @adamleff and @chris-rock for your reviews
* Add support for virtualization resource Signed-off-by: Takaaki Furukawa <takaaki.frkw@gmail.com> * Add some methods and documentation Signed-off-by: Takaaki Furukawa <takaaki.frkw@gmail.com> * Refactor collect_data_linux method Signed-off-by: Takaaki Furukawa <takaaki.frkw@gmail.com> * Remove unnecessary hash from virtualization resource and update examples Signed-off-by: Takaaki Furukawa <takaaki.frkw@gmail.com>
This PR adds support for virtualization resource to detect the virtualization platform. (#1005)
Examples: