search

openshift / compliance-operator

Operator providing OpenShift cluster compliance checks
Apache License 2.0
110 stars 110 forks source link

add error to the result object as comment #721

Closed tjungblu closed 3 years ago

tjungblu commented 3 years ago

/label tide/merge-method-squash

JAORMX commented 3 years ago

/assign @jhrozek

jhrozek commented 3 years ago

I'd like to echo the comment @JAORMX had about testing, but (and maybe I just don't see the full picture), doesn't this approach of writing the ReasonForError into the body have the possibility to affect rules that just check for an existence of some yamlpath? I'm not sure we have any honestly and we should be defensive and check that the object fetched is what we intended to fetch, but writing errors into the object result makes me nervous.

A test would make things clearer.

JAORMX commented 3 years ago

I'd like to echo the comment @JAORMX had about testing, but (and maybe I just don't see the full picture), doesn't this approach of writing the ReasonForError into the body have the possibility to affect rules that just check for an existence of some yamlpath? I'm not sure we have any honestly and we should be defensive and check that the object fetched is what we intended to fetch, but writing errors into the object result makes me nervous.

A test would make things clearer.

As far as I can tell, this shouldn't really affect existing rules. But that's exactly why I asked you to take a look as well. Wanted a second opinion on this and if you could think of any rules that would be affected by this (I couldn't). I personally like the approach, as it would end up in an empty YAML file with merely a comment that contains the status.

tjungblu commented 3 years ago

added a test @JAORMX @jhrozek

tjungblu commented 3 years ago

just check for an existence of some yamlpath?

do you guys have an understanding of what rules might be impacted by this? I remember @JAORMX explaining that the rules mostly test whether metadata.Name exists (which requires a proper k8s object). Those shouldn't be impacted at all.

jhrozek commented 3 years ago

just check for an existence of some yamlpath?

do you guys have an understanding of what rules might be impacted by this? I remember @JAORMX explaining that the rules mostly test whether metadata.Name exists (which requires a proper k8s object). Those shouldn't be impacted at all.

It seems that we are safe. I did a git grep -A25 yamlfile_value in the CaC repo and looked at the rules and all either have a yamlpath (like metadata.name you mentioned) and all seem to have at least one value to check for at the yamlpath.

We do have a bunch of regexes that match against .* but those also match against an exact yamlpath. On the other hand, we do use several permissive yamlpaths ([:]) but those are used in combination with a very specific JQ-filter in the yamlpath.

So we should be safe. Sorry, I should have checked before writing the earlier comment.

jhrozek commented 3 years ago

LGTM (explicitly not using the gitopsy lgtm so that Ozz also has the time to look at the test)

jhrozek commented 3 years ago

On Tue, Sep 28, 2021 at 08:41:39AM -0700, Jakub Hrozek wrote:

We do have a bunch of regexes that match against .*

note: most of them would probably be better looking for .+, not that it makes much difference in this case where we hopefully have a yamlpath..

tjungblu commented 3 years ago

@JAORMX wdyt? Shall we merge? Anything I can add here?

JAORMX commented 3 years ago

@tjungblu sorry for the silence. I'm doing a little bit of research first before merging this. e.g. how does OpenSCAP's YAML probe react with empty YAML files, and how this could affect future rules. I don't think this will take me too long.

tjungblu commented 3 years ago

@JAORMX cool no worries. I'm not in a rush, but I'll be out next week. Merge at your own leisure or ping me if anything needs to be changed :) :rocket:

JAORMX commented 3 years ago

I think this change is fine. The YAML path will still issue a FAIL even if the file exists but the path isn't available.

JAORMX commented 3 years ago

/lgtm

openshift-ci[bot] commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JAORMX, tjungblu

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: - ~~[OWNERS](https://github.com/openshift/compliance-operator/blob/master/OWNERS)~~ [JAORMX] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment