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 manualRules to tailoredProfile CRD #12

Merged
merged 1 commit into from May 10, 2022

Conversation

Vincent056
Copy link

This PR enables users to disable automated checks on rules so that they can check those rules manually. They can do that by adding rules under manualRules in TailoredProfile Spec.
ex.

apiVersion: compliance.openshift.io/v1alpha1
kind: TailoredProfile
metadata:
  name: mod-node
spec:
  title: My modified profile with manual rules
  manualRules:
  - name: ocp4-kubelet-eviction-thresholds-set-soft-imagefs-available
    rationale: test
  description: test

@openshift-ci openshift-ci bot requested review from mrogers950 and rhmdnd May 4, 2022 07:10
@openshift-ci openshift-ci bot added the approved label May 4, 2022
@jhrozek
Copy link

jhrozek commented May 4, 2022

First off, I'd like to hear from others what do they think. To expand on what's been requested -- some of our rules, for example those that check SCCs for being very privileged don't take into account customer customizations.

The SCCs example checks that there exists one SCC that allows all capabilities and its name is privileged. We've seen reports where the rule would fail because there are custom SCCs that are almost as privileged so they make the rule fail. One option we've offered so far was to tailor out the rule completely, but we've seen pushback on that as well because a tailored rule is "hidden" from the reports and doesn't prompt you to actually go and check the SCCs. A rule that returns MANUAL would.

So I'm OK with this approach personally, but because it touches the API, I'd like to hear from others, maybe there is a different solution. @mrogers950 @rhmdnd @JAORMX

Copy link

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

Some nits inline, but the most important thing is to agree if we want to take this approach.

CHANGELOG.md Outdated
@@ -21,6 +21,8 @@ Versioning](https://semver.org/spec/v2.0.0.html).
- Added necessary permissions for api-resource-collector so that the new
[rule](https://github.com/ComplianceAsCode/content/pull/8511)
`cluster_logging_operator_exist` can be evaluated properly.
- Added `maunalRules` to `TailoredProfile` CRD, user can choose to add the rule
there so that those rules will show Manual as results.
Copy link

Choose a reason for hiding this comment

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

We should explicitly say that the remediations are not created in this case, both here and in the CRD docs.

pkg/utils/parse_arf_result.go Outdated Show resolved Hide resolved
pkg/utils/parse_arf_result_test.go Outdated Show resolved Hide resolved
tests/e2e/e2e_test.go Show resolved Hide resolved
@JAORMX
Copy link
Collaborator

JAORMX commented May 4, 2022

I think the approach makes sense.

@Vincent056 Vincent056 force-pushed the manual branch 5 times, most recently from 041a0be to 993a24e Compare May 5, 2022 17:53
Copy link

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented May 5, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhrozek, Vincent056

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
Copy link

openshift-ci bot commented May 5, 2022

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm label May 5, 2022
@Vincent056
Copy link
Author

/retest

2 similar comments
@Vincent056
Copy link
Author

/retest

@Vincent056
Copy link
Author

/retest

@Vincent056 Vincent056 force-pushed the manual branch 6 times, most recently from be5ca40 to dcdcae0 Compare May 9, 2022 21:14
This PR enables user to disable automated check on rules so that they can check those rules manually. They can do that by adding rules under manualRules in TailoredProfile Spec
@Vincent056 Vincent056 merged commit 5b84033 into ComplianceAsCode:master May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants