CMP-2868: Add scannerType field, CustomRule CRD, and 'kind' property for rule #686
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
🤖 To deploy this PR, run the following command: |
|
/retest |
| kind: | ||
| description: |- | ||
| Type of the rule reference, either "Rule" or "CustomRule" | ||
| We will use "Rule" by default if not specified |
There was a problem hiding this comment.
Nit: Suggest using passive voice here to be consistent with the rest of the properties.
Type of rule, either "Rule" or "CustomRule". "Rule" is the default if not specified.
| kind: | ||
| description: |- | ||
| Type of the rule reference, either "Rule" or "CustomRule" | ||
| We will use "Rule" by default if not specified |
| kind: | ||
| description: |- | ||
| Type of the rule reference, either "Rule" or "CustomRule" | ||
| We will use "Rule" by default if not specified |
| type: object | ||
| spec: | ||
| properties: | ||
| availableFixes: |
There was a problem hiding this comment.
Is this carry over from Rules? Do we want to open this up right away?
| checkType: | ||
| description: |- | ||
| What type of check will this rule execute: | ||
| Platform, Node or none (represented by an empty string) |
There was a problem hiding this comment.
This can only be Platform initially, right? Do we want to state that explicitly?
| minLength: 1 | ||
| type: string | ||
| id: | ||
| description: The XCCDF ID |
There was a problem hiding this comment.
Does this have to be an XCCDF ID? Or does it just have to be an ID?
There was a problem hiding this comment.
I made some changes to the descriptions here, I tried to move the common part together, but it looks like we have rulePayLoad referenced in other part of the code that prevents from doing so, I wonder if we should keep the minimal changes and make the documentation clear
| const ( | ||
| ScannerTypeCEL ScannerType = "CEL" | ||
| ScannerTypeOpenSCAP ScannerType = "OpenSCAP" | ||
| ScannerTypeUnknown ScannerType = "Unknown" |
There was a problem hiding this comment.
Based on the code above - we'd never use this, right?
There was a problem hiding this comment.
no, we will not use that until later in the reconciler loop
Vincent056
changed the title
|
@Vincent056: This pull request references CMP-2868 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Vincent056
force-pushed
the
cel-scanner-crd
branch
2 times, most recently
from
1fed741 to
f10057f
Compare
|
🤖 To deploy this PR, run the following command: |
Vincent056
force-pushed
the
cel-scanner-crd
branch
from
f10057f to
866bd41
Compare
|
🤖 To deploy this PR, run the following command: |
|
Still failed to list customrule crd with the latest version. More permission needed from the apiGroups. Refer to #671 for info. |
Vincent056
force-pushed
the
cel-scanner-crd
branch
2 times, most recently
from
ce487a5 to
4446db3
Compare
|
🤖 To deploy this PR, run the following command: |
api transient issue |
|
/retest |
|
Verification pass:
|
|
/label qe-approved |
Vincent056
force-pushed
the
cel-scanner-crd
branch
from
4446db3 to
c1f92b2
Compare
|
🤖 To deploy this PR, run the following command: |
|
|
||
| type CELPayload struct { | ||
|
|
||
| // ScannerType specifies what type of check this rule performs |
There was a problem hiding this comment.
This bit is a little confusing to me since we also have the concept of platform and node checks.
Perhaps something like:
// ScannerType denotes the scanning implementation to use when evaluating rules
| // ScannerType specifies what type of check this rule performs | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:Enum=CEL | ||
| ScannerType ScannerType `json:"scannerType"` |
There was a problem hiding this comment.
I wonder if ScannerType should be defined outside the CELPayload. What happens if/when we add another implementation along side CEL? Will we need to define ScannerType in its payload, too?
| const ( | ||
| ScannerTypeCEL ScannerType = "CEL" | ||
| ScannerTypeOpenSCAP ScannerType = "OpenSCAP" | ||
| ScannerTypeUnknown ScannerType = "Unknown" |
| @@ -367,6 +372,19 @@ func (cs *ComplianceScan) GetScanTypeIfValid() (ComplianceScanType, error) { | |||
| return "", ErrUnkownScanType | |||
| } | |||
|
|
|||
| // GetScanerTypeIfValid returns scanner type we will be using if the scan has a valid one, else it returns | |||
| // an error | |||
| func (cs *ComplianceScan) GetScanerTypeIfValid() (ScannerType, error) { | |||
| @@ -50,6 +50,8 @@ const DefaultStorageRotation = 3 | |||
|
|
|||
| var ErrUnkownScanType = errors.New("Unknown scan type") | |||
|
|
|||
| var ErrUnkownScanerType = errors.New("Unknown scanner type") | |||
There was a problem hiding this comment.
Similar comment here as below (e.g., Scanner instead of Scaner)
| @@ -367,6 +372,19 @@ func (cs *ComplianceScan) GetScanTypeIfValid() (ComplianceScanType, error) { | |||
| return "", ErrUnkownScanType | |||
| } | |||
|
|
|||
| // GetScanerTypeIfValid returns scanner type we will be using if the scan has a valid one, else it returns | |||
| // an error | |||
| func (cs *ComplianceScan) GetScanerTypeIfValid() (ScannerType, error) { | |||
There was a problem hiding this comment.
nit: Could steal a leaf from the kubeclient book and name if GetScannerTypeOrDie) - or we could use OrFail if we remove the panic(err) eventually.
| name: | ||
| description: Name of the rule that's being referenced | ||
| type: string | ||
| rationale: | ||
| description: Rationale of why this rule is being selected/deselected | ||
| type: string | ||
| required: | ||
| - kind |
There was a problem hiding this comment.
Does this mean it's possible to have a CustomRule named foo and a Rule named foo?
I'm trying to think through a case where someone would want to disable a CustomRule. In that case, wouldn't they just not include it in the tailored profile?
There was a problem hiding this comment.
In that case, wouldn't they just not include it in the tailored profile?
no, they would not be included in the tailored profile in that case
| name: | ||
| description: Name of the rule that's being referenced | ||
| type: string | ||
| rationale: | ||
| description: Rationale of why this rule is being selected/deselected | ||
| type: string | ||
| required: | ||
| - kind |
| name: | ||
| description: Name of the rule that's being referenced | ||
| type: string | ||
| rationale: | ||
| description: Rationale of why this rule is being selected/deselected | ||
| type: string | ||
| required: | ||
| - kind |
There was a problem hiding this comment.
Similar comment here as above. Does it makes sense to support CustomRules from a manual rule perspective in a tailored profile?
There was a problem hiding this comment.
I can write a check later in tailoredProfile controller to check for that
Vincent056
force-pushed
the
cel-scanner-crd
branch
2 times, most recently
from
cb3392e to
401c06c
Compare
|
🤖 To deploy this PR, run the following command: |
|
/retest |
|
/label qe-approved |
| kind: | ||
| description: Type of rule, either "Rule" or "CustomRule". "Rule" | ||
| is the default if not specified. | ||
| type: string |
There was a problem hiding this comment.
I don't understand why we would support disabling a CustomRule. The use case for CustomRule is that:
- User creates a
CustomRulefor a check that doesn't exist in the default profiles - User creates a
TailoredProfilewith theCustomRulefrom step 1 in theenabledRules
Are you able to walk me through the use case for adding a CustomRule to the disabledRules?
| @@ -97,13 +107,18 @@ spec: | |||
| description: RuleReferenceSpec specifies a rule to be selected/deselected, | |||
| as well as the reason why | |||
| properties: | |||
| kind: | |||
| description: Type of rule, either "Rule" or "CustomRule". "Rule" | |||
| is the default if not specified. | |||
There was a problem hiding this comment.
Same comment here as the disabledRules above. Do we need to support CustomRules here initially? Or can we add that in later?
| availableFixes: | ||
| description: |- | ||
| The Available fixes | ||
| This is not supported with CustomRule |
There was a problem hiding this comment.
I feel if we're not going to support it - we should remove it from the CRD so it's not confusing to users, and not creating additional API surface area for us to test.
We could always add this in later.
Vincent056
force-pushed
the
cel-scanner-crd
branch
from
401c06c to
482271b
Compare
|
🤖 To deploy this PR, run the following command: |
| // Expression is the CEL expression to evaluate | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| Expression string `json:"expression"` |
There was a problem hiding this comment.
Evaluation (struct):
// CELEvaluation
Expression:
Inputs:
// BashEvaluation
Commands: (list of command strings or reference to config map)
// AnsibleEvaluation
Tasks: (reference to config map or CRDs)
// SomeNewThingWeDoNotKnowAboutYetEvaluationPerhaps we can learn from https://github.com/openshift/cluster-logging-operator/blob/master/api/observability/v1/output_types.go
Vincent056
force-pushed
the
cel-scanner-crd
branch
2 times, most recently
from
51a25a6 to
f7e5add
Compare
|
🤖 To deploy this PR, run the following command: |
Vincent056
force-pushed
the
cel-scanner-crd
branch
from
f7e5add to
26cdbb5
Compare
- Introduce scannerType to ComplianceScan and ComplianceSuite for specifying OpenSCAP or CEL. - Add custom rule CRD (compliance.openshift.io_customrules.yaml) and types. - Extend TailoredProfile with addtional EnableCustomRule field.
Vincent056
force-pushed
the
cel-scanner-crd
branch
from
26cdbb5 to
e5265d4
Compare
|
🤖 To deploy this PR, run the following command: |
|
@Vincent056: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
Added changes needed to CRDs to enable CEL scanner.
CustomRuleCRD and types.