metrics: Add SELinux related audit metrics to the enricher

[jhrozek]

What type of PR is this?

/kind feature

What this PR does / why we need it:

On dispatching an AVC line from the enricher via GRPC, also issue a
metric with the details of the AVC, similarly to what we do in the
seccomp case.

Which issue(s) this PR fixes:

Fixes #609

Does this PR have test?

No, I'm not sure if our way of getting metrics through the service
is stable enough for tests.

Special notes for your reviewer:

N/A

Does this PR introduce a user-facing change?

The security_profiles_operator_selinux_profile_audit_total metric was actually
enabled and uses the appropriate labels scraped from the audit.log file.

[codecov-commenter]

Codecov Report

Merging #916 (6dd8132) into main (72a0786) will increase coverage by 0.10%.
The diff coverage is 61.53%.

@@            Coverage Diff             @@
##             main     #916      +/-   ##
==========================================
+ Coverage   50.64%   50.75%   +0.10%     
==========================================
  Files          42       42              
  Lines        4626     4660      +34     
==========================================
+ Hits         2343     2365      +22     
- Misses       2198     2209      +11     
- Partials       85       86       +1     

[jhrozek]

Thank you for the review @saschagrunert

[jhrozek]

/test pull-security-profiles-operator-test-e2e

[JAORMX]

I'm concerned about this dramatically increasing the cardinality of the SELinux metrics...

From an article on the topic

As a general rule of thumb I'd avoid having any metric whose cardinality on a /metrics could potentially go over 10 due to growth in label values. The way I think of it is that if it has already grown to be 10 today, it might be 15 in a year's time. It can be additionally okay to have no more than a literal handful within a given Prometheus that go to 100. These are only rules of thumb, if you know for example that the number of replicas of your service will always be tiny then you can exceed these a bit, conversely if you're expecting thousands of replicas I'd carefully consider every time series on the /metrics.

What are you trying to get with these metrics? Maybe there can be another solution instead of adding these new labels.

[JAORMX]

wouldn't doing this dramatically increase the cardinality of this metric? I really think we shouldn't do this...

[jhrozek]

What real-world implication does increasing the cardinality have?

[JAORMX]

The main issue (real-world implication) is that increasing cardinality of metrics will lead to overloading Prometheus. Thus, by not adding only what's minimally needed for us to debug problems at scale we make the SPO a noisy neighbour in terms of metrics

[jhrozek]

I'm concerned about this dramatically increasing the cardinality of the SELinux metrics...

From an article on the topic

As a general rule of thumb I'd avoid having any metric whose cardinality on a /metrics could potentially go over 10 due to growth in label values. The way I think of it is that if it has already grown to be 10 today, it might be 15 in a year's time. It can be additionally okay to have no more than a literal handful within a given Prometheus that go to 100. These are only rules of thumb, if you know for example that the number of replicas of your service will always be tiny then you can exceed these a bit, conversely if you're expecting thousands of replicas I'd carefully consider every time series on the /metrics.

What are you trying to get with these metrics? Maybe there can be another solution instead of adding these new labels.

Have a way of observing what is being recorded. There's two reasons:

• for the recording itself
• I'm still thinking about "watching" workloads with a policy when introducing the policy into production and I was thinking about using the log enricher so that you could see what AVCs does the workload trigger and can adjust either the workload or the policy.

If the number of labels is a concern, I guess we could remove /some/ but at least the source and target contexts should stay so that you could see what is touching what and go to see the full log enricher logs.

[jhrozek]

btw the problem with "watching log enricher logs" is that each spod instance has their own. The metrics allow you to watch all of them from a central place.

[saschagrunert]

/hold for @JAORMX

[jhrozek]

OK, I removed the perm and the class. This still retains scontext and tcontext which should give an idea of who is touching what and brings the amount of labels to just one more than the already existing seccomp metric

[JAORMX]

OK, I removed the perm and the class. This still retains scontext and tcontext which should give an idea of who is touching what and brings the amount of labels to just one more than the already existing seccomp metric

I think this is a good compromise. Thanks!

[jhrozek]

ugh another CI failure, let me reopen the PR..

[jhrozek]

/hold cancel

[saschagrunert]

/retest

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JAORMX, jhrozek, saschagrunert

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 [JAORMX,jhrozek,saschagrunert]

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

[JAORMX]

/retest