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

Implement an 'audit' reporter that will terminate Chef Client runs on profile failures #380

Merged

Conversation

sbabcoc
Copy link
Contributor

@sbabcoc sbabcoc commented Jul 12, 2019

Description

This pull request implements an audit enforcer reporter to replace the fail_if_any_audits_failed feature that was removed a while ago.

Issues Resolved

[List any existing issues this PR resolves]

Check List

@sbabcoc sbabcoc requested a review from a team July 12, 2019 18:59
Signed-off-by: Scott Babcock <scoba@hotmail.com>
Signed-off-by: Scott Babcock <scoba@hotmail.com>
Signed-off-by: Scott Babcock <scoba@hotmail.com>
Signed-off-by: Scott Babcock <scoba@hotmail.com>
@sbabcoc
Copy link
Contributor Author

sbabcoc commented Jul 14, 2019

Looks like the Travis failures were caused by a configuration issue that's been resolved.

@sbabcoc
Copy link
Contributor Author

sbabcoc commented Jul 14, 2019

Given that the purpose of this reporter is to throw an error, I don't know how to implement a test for it.

@alexpop
Copy link
Contributor

alexpop commented Jul 15, 2019

Thank you Scott for bringing this option back into the audit cookbook.

I pushed a commit to this branch:
https://github.com/chef-cookbooks/audit/commits/pr-380

Can you cherry-pick that into your fork please? It adds unit tests for AuditEnforcer and takes care of lint errors.

Signed-off-by: Alex Pop <apop@chef.io>
@sbabcoc
Copy link
Contributor Author

sbabcoc commented Jul 15, 2019

Merged unit tests and linting fixes from @alexpop

Copy link
Contributor Author

@sbabcoc sbabcoc left a comment

Choose a reason for hiding this comment

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

I reviewed the changes made by @alexpop, and they look great!

Copy link
Contributor

@alexpop alexpop left a comment

Choose a reason for hiding this comment

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

Much appreciated Scott!

Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

As far as I can see, this will make a reporter that fails. I'm not sure how much usefulness that has, since it is my understanding that the chef client run has already completed at this point, and you would only have an error in the reporter, not a node that failed to converge; I may be mistaken.

Of more concern to me is that this is to replicate a feature that was withdrawn. I'm trying to get context on why fail_if_any_audit_failed was removed, and whether those reasons are any different today.

Finally, the TravisCI failure is a linting failure - we need those addressed before we can accept. Pinning the chefstyle gem in the Gemfile may be advisable. Thanks!

Signed-off-by: Alex Pop <apop@chef.io>
@sbabcoc
Copy link
Contributor Author

sbabcoc commented Jul 18, 2019

The pull request that added the fail_if_any_audits_failed feature is here: #25
The pull request that removed this feature is here: #113

Here's the specific comment:

jeremymv2 on 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.

The intent of my PR is to fail actual chef-client runs when non-compliance is detected. The comment attached to the removal of the fail_if_any_audits_failed feature refers to kitchen, which is a completely different context.

@sbabcoc
Copy link
Contributor Author

sbabcoc commented Jul 19, 2019

As for usefulness, this enables me to terminate a pipeline if a non-compliant node is encountered. The current behavior is to passively report the failure and continue to the next stage, which will then choke because of the previously detected issue.

@sbabcoc sbabcoc closed this Jul 19, 2019
@sbabcoc sbabcoc reopened this Jul 19, 2019
@sbabcoc sbabcoc closed this Jul 20, 2019
@sbabcoc sbabcoc reopened this Jul 20, 2019
@sbabcoc
Copy link
Contributor Author

sbabcoc commented Jul 20, 2019

Sorry, it's really easy to close pull requests accidentally in the mobile view of GitHub.

@sbabcoc
Copy link
Contributor Author

sbabcoc commented Jul 24, 2019

@alexpop has been incredibly helpful on this PR! Many thanks!
He discovered that the audit enforced raises the error, but the audit cookbook is eating the failure. This happens in the run_report_safely method, which catches exceptions and only re-throws them if the fail_if_not_present attribute is specified. I'll push revisions that will also re-throw for audit enforcer.

Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

I see, it took me a bit to get when this would fire - thanks for this!

@clintoncwolfe clintoncwolfe added Expeditor: Bump Version Minor Used by github.minor_bump_labels to bump the Minor version number. Type: Enhancement Adds new functionality. labels Jul 24, 2019
@clintoncwolfe clintoncwolfe merged commit b4ca5c7 into chef-boneyard:master Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Expeditor: Bump Version Minor Used by github.minor_bump_labels to bump the Minor version number. Type: Enhancement Adds new functionality.
Development

Successfully merging this pull request may close these issues.

3 participants