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

use chef handler to run inspec tests #113

Merged
merged 9 commits into from Oct 20, 2016

Conversation

Projects
None yet
5 participants
@vjeffrey
Collaborator

vjeffrey commented Oct 17, 2016

so many more things to do. just the beginning...working on viz integration parts now.

@binamov binamov added the in progress label Oct 17, 2016

@vjeffrey

This comment has been minimized.

Collaborator

vjeffrey commented Oct 18, 2016

successful converge with visibility with a change from:

# node.default['audit']['profiles'].merge!({
#   'nathen/tmp_compliance_profile' => {
#     'source' => "#{PROFILES_PATH}/tmp_compliance_profile"
#   }
# })

to
node.default['audit']['profiles'].push("#{PROFILES_PATH}/tmp_compliance_profile")

still need to modify to ensure json results get to the right place - right now just cli json output

vjeffrey added some commits Oct 17, 2016

fixing lint errors. most tests fixed.
Signed-off-by: Victoria Jeffrey <vjeffrey@chef.io>
@vjeffrey

This comment has been minimized.

Collaborator

vjeffrey commented Oct 19, 2016

need to fix the one of the viz tests, but this works with chef visibility.

@@ -1,37 +0,0 @@
---

This comment has been minimized.

@chris-rock

chris-rock Oct 19, 2016

Collaborator

we should not delete the examples. Instead we should migrate to work with the new version

@@ -46,11 +46,9 @@
# fail converge after posting report if any audits have failed
default['audit']['fail_if_any_audits_failed'] = false
# inspec gem version to install(e.g. '1.1.0')
default['audit']['inspec_version'] = '1.2.0'

This comment has been minimized.

@chris-rock

chris-rock Oct 19, 2016

Collaborator

we are going to need it for now

chris-rock and others added some commits Oct 19, 2016

recover examples
Signed-off-by: Christoph Hartmann <chris@lollyrock.com>
notifies :touch, "file[#{compliance_cache_directory}/#{p}]", :delayed
notifies :execute, "compliance_report[#{report_collector}]", :delayed
end
chef_inspec 'inspec' do

This comment has been minimized.

@chris-rock

chris-rock Oct 19, 2016

Collaborator

chef_inspec feels like a strange name

@@ -1,7 +1,7 @@
# encoding: utf-8
provides :chef_inspec

This comment has been minimized.

@chris-rock

chris-rock Oct 19, 2016

Collaborator

can we just inspec for provides and as a resource name instead of chef_inspec?

This comment has been minimized.

@vjeffrey

vjeffrey Oct 19, 2016

Collaborator

of course! let's do it!

quiet node['audit']['quiet'] unless node['audit']['quiet'].nil?
action :nothing
end if node['audit']['profiles'].values.any?
inspec_report 'report' do

This comment has been minimized.

@chris-rock

chris-rock Oct 19, 2016

Collaborator

Do you think we should combine:

chef_handler 'Chef::Handler::AuditReport' do
  source "#{handler_directory}/audit_report.rb"
  action :enable
end

inspec_report 'report' do
  action :execute
end

This comment has been minimized.

@vjeffrey

vjeffrey Oct 19, 2016

Collaborator

how do we combine them?

This comment has been minimized.

@vjeffrey

vjeffrey Oct 19, 2016

Collaborator

seems like the right way to go, just unsure how?

recipes/default.rb Outdated
notifies :touch, "file[#{compliance_cache_directory}/#{p}]", :delayed
notifies :execute, "compliance_report[#{report_collector}]", :delayed
end
chef_handler 'Chef::Handler::AuditReport' do

This comment has been minimized.

@chris-rock

chris-rock Oct 19, 2016

Collaborator

As discussed with @vjeffrey we try to find a way that does not require us to write a file on the host node

@@ -64,8 +64,6 @@ suites:
attributes:
audit:
profiles: &profiles

This comment has been minimized.

@chris-rock

chris-rock Oct 19, 2016

Collaborator

not 100% sure what &profiles should refer to

This comment has been minimized.

@vjeffrey

vjeffrey Oct 19, 2016

Collaborator

leftover from initial implementation.. Yaml syntax docs: "Repeated nodes are first identified by an anchor (marked with the ampersand - “&”), and are then aliased (referenced with an asterisk - “*”) thereafter."

@chris-rock chris-rock added this to the 2.0 milestone Oct 19, 2016

attributes/default.rb Outdated
@@ -46,11 +48,9 @@
# fail converge after posting report if any audits have failed
default['audit']['fail_if_any_audits_failed'] = false

This comment has been minimized.

@jeremymv2

jeremymv2 Oct 19, 2016

Contributor

Since we're moving away from a resource converge I don't think 'fail_if_any_audits_failed' makes sense any more. The original intent was to fail during converge in a pipeline phase such as kitchen converge. Now, with the report handler, we'd rely on kitchen-inspec to reference the profiles and kitchen verify to provide a pass|fail status to the CI framework.

This comment has been minimized.

@vjeffrey

vjeffrey Oct 19, 2016

Collaborator

sounds good to me! i'll remove it

@vjeffrey

This comment has been minimized.

Collaborator

vjeffrey commented Oct 19, 2016

still need to figure out the chef_handler putting a source file on the system thing (https://github.com/chef-cookbooks/audit/pull/113/files#diff-55bf87238c9b8af164c5133a60721f12R9)
I've asked around...ppl are unsure of what to do about that. one suggestion: "maybe the chef_handler resource can delete the file if you use the :disable action. "

@vjeffrey vjeffrey changed the title from wip: use chef handler to run inspec tests to use chef handler to run inspec tests Oct 19, 2016

@jeremymv2

This comment has been minimized.

Contributor

jeremymv2 commented Oct 19, 2016

Most handlers that I've used lay down the handler libs as a versioned gem. One of the better examples is the DataDog handler: https://github.com/DataDog/chef-datadog/blob/master/recipes/dd-handler.rb#L32-L38

@vjeffrey

This comment has been minimized.

Collaborator

vjeffrey commented Oct 19, 2016

yup. so we were trying to avoid the gem thing in the case of an air-gapped env/to avoid outside deps (I know that doesn't make much sense right now since we are installing inspec gem but we are trying to find a way to move away from that and vendor) --- I spoke with joshua timberman and jj ashgar and both said ya, those are the only two ways of doing the chef handler thing, so i'm just gonna whip up a quick delete directory cleanup recipe and include it at the end of default with a good ol include_recipe call. coming soon! just have to make food for ppl real quick, then i'll do it :)

recipes/clean-up.rb Outdated
directory handler_directory do
recursive true
action :delete
end

This comment has been minimized.

@jeremymv2

jeremymv2 Oct 19, 2016

Contributor

Oh, I see what you're achieving now 😃 more clearly. However, I suppose I don't understand what the requirement is to cause us to remove the chef handler ruby library code.
To me it smells fishy because it seems like it's akin to running an app that deletes all traces of itself after execution. I can think of two potential concerns that arise from that:

  • It's not in the spirit of transparency - we're running code on nodes and then deleting that code from the nodes.
  • Deleting the handler code from the node could make troubleshooting harder since it requires version correlation against a git repo or chef server to determine what actual code was running when the handler fired. Having the code intact on the node gives you a 100% assurance that you know what executed.

If this moves forward, I recommend adding the standard header to the recipe which gets generated from chef generate recipe clean-up One last thing on this commit, just me perhaps, but using - in recipe names is not consistent with the requirement that cookbooks are not allowed to have - in the name 😄

#
# Cookbook Name:: audit
# Recipe:: clean-up
#
# Copyright (c) 2016 The Authors, All Rights Reserved.

This comment has been minimized.

@chris-rock

chris-rock Oct 19, 2016

Collaborator

@jeremymv2 thanks for the honest feedback. That is very helpful! Background: This was an approach @vjeffrey and I discussed before. From my perspective it feels strange to install code on a node just for runtime and not cleaning the tmp stuff up. Nevertheless, if that is not standard practice we should not do this approach.

@vjeffrey I propose we leave the handler files there for now, I've an idea how we can get around generating the file in future to get the best of both worlds. But lets try that in future PRs.

This comment has been minimized.

@vjeffrey

vjeffrey Oct 20, 2016

Collaborator

cool, sounds good! also, get some sleep @chris-rock!!

This comment has been minimized.

@vjeffrey

vjeffrey Oct 20, 2016

Collaborator

just updated by dropping that last commit. :) thanks for your help @jeremymv2!! good points!

readd version attribute for inspec gem
Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

@vjeffrey vjeffrey merged commit aed623f into master Oct 20, 2016

2 checks passed

DCO This commit has a DCO Signed-off-by
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iennae iennae removed the in progress label Oct 20, 2016

@vjeffrey vjeffrey deleted the audit-2.0 branch Oct 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment