CMP-4040, CMP-4041: Add support for CEL based rules and profiles#14597
CMP-4040, CMP-4041: Add support for CEL based rules and profiles#14597yuumasato wants to merge 15 commits intoComplianceAsCode:masterfrom
Conversation
|
I verified PR #14597 and PR ComplianceAsCode/compliance-operator#1103 together. Generally it is good. The only problem is there is no |
e7d189f to
af527ae
Compare
|
Thanks for the review @xiaojiey. Hopefully I have addessed the BuildConfig issue in the last commit. |
af527ae to
188024f
Compare
188024f to
25fe7a6
Compare
|
@yuumasato Sorry, I forgot to highlight, there is one more need to be updated. The ################# without --datastream-only parameter |
applications/openshift-virtualization/kubevirt-nonroot-feature-gate-is-enabled/rule.yml
Outdated
Show resolved
Hide resolved
Vincent056
left a comment
There was a problem hiding this comment.
I think the PR looks good, just some questions on formatting and templating.
|
@xiaojiey Thanks, instead of removing '--datastream-only' I have added a new parameter '--cel-content=ocp4'. |
|
@yuumasato Thanks for the update. Now with/without CEL profiles, the profiles can be created successfully. |
|
/retest |
9fde596 to
2772f94
Compare
|
@Mab879 I have moved the CEL specific keys to its own file. |
|
/retest |
|
/test e2e-aws-openshift-node-compliance |
Mab879
left a comment
There was a problem hiding this comment.
Thanks for moving cel keys to the cel folder. Looks like the docs need to be updated.
| Run the following command: | ||
| <pre>$ oc get pods</pre> | ||
|
|
||
| failure_reason: |- # Optional: Custom failure message |
There was a problem hiding this comment.
This should be moved to cel folder in the docs as well.
There was a problem hiding this comment.
@Mab879 Not sure what you mean, the docs already mention failure_reason.
Do you mean that I should remove the example CEL rule from this file?
There was a problem hiding this comment.
I don't see it in master? What am I missing?
$ git log | head -n 1
commit 8aa33003dd0b592890cc10dd721e7188f9aff1e7
$ rg failure_reason | wc -l
0
rhmdnd
left a comment
There was a problem hiding this comment.
A few comments inline (most can be addressed in follow ups). Primary questions are focused on the ergonomics of build_product.
| - name: hcoList | ||
| kubernetes_input_spec: | ||
| api_version: hco.kubevirt.io/v1beta1 | ||
| resource: hyperconvergeds |
There was a problem hiding this comment.
followup: could add resource_name and resource_namespace here to simplify the filter (lines 11 - 12) below.
| - name: hcoList | ||
| kubernetes_input_spec: | ||
| api_version: hco.kubevirt.io/v1beta1 | ||
| resource: hyperconvergeds |
There was a problem hiding this comment.
followup: could add resource_name and resource_namespace here to simplify the filter (lines 11 - 12) below.
| - name: hcoList | ||
| kubernetes_input_spec: | ||
| api_version: hco.kubevirt.io/v1beta1 | ||
| resource: hyperconvergeds |
There was a problem hiding this comment.
followup: could add resource_name and resource_namespace here to simplify the filter (lines 11 - 12) below.
| - name: vms | ||
| kubernetes_input_spec: | ||
| api_version: kubevirt.io/v1 | ||
| resource: VirtualMachine |
There was a problem hiding this comment.
followup: virtualmachine to be consistent with the case used in the other rules (hyperconvergeds)
| # A rule uses CEL if it has both expression and inputs | ||
| # (loaded from cel/shared.yml during rule compilation) | ||
| has_expression = hasattr(rule, 'expression') and rule.expression | ||
| has_inputs = hasattr(rule, 'inputs') and rule.inputs |
There was a problem hiding this comment.
Aha - so this is the part that makes it so we don't need a scanner-type attribute in the rule?
There was a problem hiding this comment.
Yes, when these fields are present, the build system knows that it has support for CEL.
| # ARG_OPTIONAL_BOOLEAN([bash-scripts],[],[Build Bash remediation scripts for every profile],[on]) | ||
| # ARG_OPTIONAL_BOOLEAN([datastream-only],[d],[Build the data stream only. Do not build any of the guides, tables, etc],[off]) | ||
| # ARG_OPTIONAL_ACTION([datastream],[],[Build the data stream. Do not build any of the guides, tables, etc]) | ||
| # ARG_OPTIONAL_SINGLE([cel-content],[],[Product(s) to build CEL content for (comma-separated)],[off]) |
There was a problem hiding this comment.
The datastream-only and datastream arguments are specific to SCAP, and we're adding another variant that essentially does something similar with a new format (cel-content).
I wonder if we need something more generic to allow for better ergonomics?
# build it all (cel, scap datastreams, guides, tables)
$ build_project ...
# equivalent to legacy --datastream-only (but without two arguments for specifying datastream)
$ build_project --output-implementations scap
# only build CEL content
$ build_project --output-implementations cel| ``` | ||
|
|
||
| **Note:** | ||
| - Profiles use `scanner_type: CEL` to indicate they target the CEL checking engine and should be excluded from XCCDF/datastream builds. |
There was a problem hiding this comment.
What determines if a rule is included in CEL and if it's included in XCCDF? Is that determined by rule presence in profiles?
There was a problem hiding this comment.
All CEL rules loaded are included in the cel-content.yaml.
https://github.com/ComplianceAsCode/content/pull/14597/changes#diff-ce6393057aed5d58cf51f7e13532a07ec0063429a773d3e177e5eabfefe305a7R364
| ocil_clause: 'insecure registries are configured' | ||
|
|
||
| ocil: |- |
There was a problem hiding this comment.
What is the benefit of having OCIL in a CEL rule? Should the OCIL be present in CEL rules?
There was a problem hiding this comment.
When consuming the data stream, Compliance Operator exposes the OCIL as intructions on how to manually check the rule.
I just used the same input field ocil, in the output the CEL content has a field named instructions.
I could rename this to instructions, but keeping it as ocil is backwards compatible and won't cause problems when we start to migrate the rules in application/openshift to CEL.
We expect this profile to exclusively leverage the CEL rules.
2772f94 to
edf602f
Compare
Add a new build-script along with a new output type that builds the CEL rules into the yaml that can be loaded by Compliance Operator.
Copies the CEL content file to the content images.
Adds --cel-content parameter that takes a comma separated list of products to build cel-content for. Add the new parameter with OCP4 product where it makes sense.
With addition of '--cel-content' as an option to build CEL content. And with it being additional to data stream builds, having '--datastream-only' parameter feels weird. This add '--datastream' so that we can move away from '--datastream-only' and be more consistent.
Keep only the --datastream option, which builds the CMake target that generates the data stream files, in addition to any other target defined during script invocation.
Keeps the fields pertainint to CEL scanning engine separate from the rule.yml, which can remain agnostic. This facilitates the implementation of templates later on. 'scanner_type' is completely removed from rules, and inferred by presence of 'cel' directory or presence of 'expression' and 'input' keys.
Prefer the --datastream option that accommodates the use of --cel-content argument.
edf602f to
751a5cc
Compare
Description:
ocp4product.Rationale:
Review Hints: