Add a way to exclude sources, checks and/or keys from results #228

rcritten · 2021-09-22T13:07:13Z

We allow to exclude on three different levels:

source
check
key

Excluding a source could be used to disable a misbehaving set of
checks, particularly one not provided by upstream. A check would
be similar.

Not all results have a key but most do. If we run into corner
cases we can address them as they come up.

The example I had in mind is an untracked certificate that is
otherwise legitimate. This could be marked as excluded by key
so ipa-healthcheck will no longer return failures.

Filtering happens twice. Any sources or checks excluded will simply
not be executed. keys are excluded after execution.

This adds new section, [exclusions], which will contain three types
of exclusions and can be repeated:

source
config
key

#176

Signed-off-by: Rob Crittenden rcritten@redhat.com

rcritten · 2021-09-22T13:08:17Z

An example config file may look like:

[default]

[excludes]
#source=ipahealthcheck.ipa.certs
#check=IPACertTracking
#check=IPADNSSystemRecordsCheck
key=20210910141452

So you can have source=ipahealthcheck.ipa to block out all but the pki healthchecks.

antoniotorresm · 2022-01-17T11:34:37Z

man/man5/ipahealthcheck.conf.5

@@ -31,11 +31,33 @@ Values should not be quoted, the quotes will not be stripped.
 Options must appear in the section named [default]. There are no other sections defined or used currently.

 Options may be defined that are not used. Be careful of misspellings, they will not be rejected.
+.SH "EXCLUDES"
+There may be reasons that a user will want to suppress some results. One example is a customer certificate that is generating a warning because it is unknown to IPA. Excluding a result byu key does not prevent it from running, it is filtered from the reported results. Excluding by source or key will prevent it from running at all. Services will not be excluded because other checks may rely on them (ipahealthcheck.meta.services).


Typo: "byu"

antoniotorresm · 2022-01-17T11:36:02Z

src/ipahealthcheck/core/config.py

 from ipahealthcheck.core.constants import DEFAULT_CONFIG

 logger = logging.getLogger()


+class DuplicateOrderedDict(OrderedDict):
+    def __setitem__(self, key, value):
+        """Duplicate keys will be concatonated strings separated by new-line"""


Typo: "concatonated"

antoniotorresm · 2022-01-17T12:33:16Z

Is it possible to use excludes_key, excludes_source and excludes_check options outside the [excludes] section? The man update gives that impression, but testing on my side fails:

[default]
excludes_key=DSVIRTLE0001

Traceback (most recent call last):
  File "/root/freeipa-healthcheck/venv/bin/ipa-healthcheck", line 33, in <module>
    sys.exit(load_entry_point('ipahealthcheck', 'console_scripts', 'ipa-healthcheck')())
  File "/root/freeipa-healthcheck/src/ipahealthcheck/core/main.py", line 52, in main
    sys.exit(ipachecks.run_healthcheck())
  File "/root/freeipa-healthcheck/src/ipahealthcheck/core/core.py", line 420, in run_healthcheck
    results = exclude_keys(config, results)
  File "/root/freeipa-healthcheck/src/ipahealthcheck/core/core.py", line 256, in exclude_keys
    result.kw.get("key") in config.excludes_key
TypeError: 'in <string>' requires string as left operand, not NoneType

antoniotorresm · 2022-01-17T12:34:07Z

Do you think it would be useful to have these options on the CLI as well?

rcritten · 2022-01-17T14:21:53Z

Is it possible to use excludes_key, excludes_source and excludes_check options outside the [excludes] section?

The excludes_ options should not have been in the man page. That was from a previous incarnation of the change. I dropped it.

But you're right, it was semi-processed anyway. The intention was to only process those in the new section so that format so I made excludes_* be skipped when processing [default] just to be certain.

Do you think it would be useful to have these options on the CLI as well?

Probably but I couldn't think of a use-case. One might do it for a one-off but then you'd need to remember to skip it with each run. I think in the config file one could log the reason for the skip.

antoniotorresm · 2022-01-19T10:45:12Z

Maybe you could add an example usage to the ipahealtcheck.conf man page. Other than that this LGTM.

rcritten · 2022-02-01T19:05:19Z

Man page updated.

We allow to exclude on three different levels: * source * check * key Excluding a source could be used to disable a misbehaving set of checks, particularly one not provided by upstream. A check would be similar. Not all results have a key but most do. If we run into corner cases we can address them as they come up. The example I had in mind is an untracked certificate that is otherwise legitimate. This could be marked as excluded by key so ipa-healthcheck will no longer return failures. Filtering happens twice. Any sources or checks excluded will simply not be executed. keys are excluded after execution. This adds new section, [exclusions], which will contain three types of exclusions and can be repeated: source config key freeipa#176 Signed-off-by: Rob Crittenden <rcritten@redhat.com>

Being able to suppress a specific key rather than a whole source or check is better. I'm not ready yet to assert that there is a key in each Result since that would be a rather impactful change but for the purposes of this change I added an assert and ran it through the unit tests. freeipa#176 Signed-off-by: Rob Crittenden <rcritten@redhat.com>

Create a new source and register two plugins to it. Use an Output plugin to collect the results in a global variable so the results can be evaluated after the run is complete. Use a temporary configuration file to set the test configuration. Test suppressing: - nothing - the source - one check - one key Signed-off-by: Rob Crittenden <rcritten@redhat.com> freeipa#176

A config option was added which needs to be passed in during the test call. Signed-off-by: Rob Crittenden <rcritten@redhat.com>

rcritten force-pushed the issue_176 branch from 01093b7 to 9b7d146 Compare January 10, 2022 20:44

antoniotorresm reviewed Jan 17, 2022

View reviewed changes

rcritten force-pushed the issue_176 branch from 8f0873e to 89d8356 Compare January 17, 2022 15:52

rcritten force-pushed the issue_176 branch from 89d8356 to ad3960e Compare February 1, 2022 18:43

rcritten force-pushed the issue_176 branch from ad3960e to b71fec2 Compare February 1, 2022 19:05

rcritten added 4 commits February 1, 2022 15:30

Update timeout test to use new API in run_plugins

56029bf

A config option was added which needs to be passed in during the test call. Signed-off-by: Rob Crittenden <rcritten@redhat.com>

rcritten force-pushed the issue_176 branch from b71fec2 to 56029bf Compare February 1, 2022 20:59

rcritten merged commit 896824f into freeipa:master Feb 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a way to exclude sources, checks and/or keys from results #228

Add a way to exclude sources, checks and/or keys from results #228

rcritten commented Sep 22, 2021

rcritten commented Sep 22, 2021

antoniotorresm Jan 17, 2022

antoniotorresm Jan 17, 2022

antoniotorresm commented Jan 17, 2022 •

edited

antoniotorresm commented Jan 17, 2022

rcritten commented Jan 17, 2022

antoniotorresm commented Jan 19, 2022

rcritten commented Feb 1, 2022

Add a way to exclude sources, checks and/or keys from results #228

Add a way to exclude sources, checks and/or keys from results #228

Conversation

rcritten commented Sep 22, 2021

rcritten commented Sep 22, 2021

antoniotorresm Jan 17, 2022

Choose a reason for hiding this comment

antoniotorresm Jan 17, 2022

Choose a reason for hiding this comment

antoniotorresm commented Jan 17, 2022 • edited

antoniotorresm commented Jan 17, 2022

rcritten commented Jan 17, 2022

antoniotorresm commented Jan 19, 2022

rcritten commented Feb 1, 2022

antoniotorresm commented Jan 17, 2022 •

edited