apps sc: S3 quota alerts per bucket

[Mlundm]

Warning

This is a public repository, ensure not to disclose:

• personal data beyond what is necessary for interacting with this pull request, nor
• business confidential information, such as customer names.

What kind of PR is this?

Required: Mark one of the following that is applicable:

• kind/feature
• kind/improvement
• kind/deprecation
• kind/documentation
• kind/clean-up
• kind/bug
• kind/other

Optional: Mark one or more of the following that are applicable:

Important

Breaking changes should be marked kind/admin-change or kind/dev-change depending on type
Critical security fixes should be marked with kind/security

• kind/admin-change
• kind/dev-change
• kind/security
• [kind/adr](set-me)

What does this PR do / why do we need this PR?

...

• Fixes [2] S3 quota alerts per bucket #2543

Information to reviewers

• Would be happy if anyone got any suggestions/improvements to the schema titles/descriptions for the new buckets section.

• I changed how the S3 Bucket Alert expression works in general because of how it was leaving "gaps" of data when you query the expression. This was due to while testing I noticed how changing the for: 1h to for: 10m would make the alert start pending but then return to Normal since there was no data (when it should have triggered). If you see any issue with it let me know. See below for example.

• New query/expression
  

• Old query/expression
  

• Example per bucket alerts (sc-config)

prometheus:
  s3BucketAlerts:
    objects:
      enabled: true
    size:
      enabled: true
    buckets:
      - name: marcus-v2-thanos
        size:
          enabled: true
          percent: 80
          sizeQuotaGB: 1000
        objects:
          enabled: true
          percent: 80
          count: 1638400
    exclude:
      - marcus-v2-thanos

Checklist

• Proper commit message prefix on all commits
• Change checks:
  • The change is transparent
  • The change is disruptive
  • The change requires no migration steps
  • The change requires migration steps
  • The change updates CRDs
  • The change updates the config and the schema
• Documentation checks:
  • The public documentation required no updates
  • The public documentation required an update - [link to change](set-me)
• Metrics checks:
  • The metrics are still exposed and present in Grafana after the change
  • The metrics names didn't change (Grafana dashboards and Prometheus alerts required no updates)
  • The metrics names did change (Grafana dashboards and Prometheus alerts required an update)
• Logs checks:
  • The logs do not show any errors after the change
• PodSecurityPolicy checks:
  • Any changed Pod is covered by Kubernetes Pod Security Standards
  • Any changed Pod is covered by Gatekeeper Pod Security Policies
  • The change does not cause any Pods to be blocked by Pod Security Standards or Policies
• NetworkPolicy checks:
  • Any changed Pod is covered by Network Policies
  • The change does not cause any dropped packets in the NetworkPolicy Dashboard
• Audit checks:
  • The change does not cause any unnecessary Kubernetes audit events
  • The change requires changes to Kubernetes audit policy
• Falco checks:
  • The change does not cause any alerts to be generated by Falco
• Bug checks:
  • The bug fix is covered by regression tests

[anders-elastisys]

Very nice work, I think the templating, config and schema looks great, added some potential improvements and one concern regarding the Bucket36hActivityCheck alert.

[anders-elastisys]

There is another alert Bucket36hActivityCheck defined in this file which also uses the exclude list which we probably still would want for the specified buckets, should perhaps the exclusion for buckets listed in s3BucketAlerts.buckets be done in the templating instead of manually adding them to the exclude list, or, should we also have configurable alerts per bucket for Bucket36hActivityCheck as well? 🤔

[Mlundm]

Good catch, I added templating for the exclusion of buckets so it creates a combined list for .exclude and names of .buckets. I don't think separate alerts per bucket for the activity check is necessary since there is not any configureable value there like number of objects or size.

Was trying to make a helper function for the templating but ended up spending too much time with debugging templating map/list issues due to that 😩

Ended up just doing it in the template file, which I think was just fine since it might have been overkill with a helper function either way.

[Zash]

Schema looks good, with one tweak.

[anders-elastisys]

LGTM, just added some comments about updating comments 😅

[anders-elastisys]

With the most recent changes you do not need to add bucket names to the exclude list right?

Suggested change

	# - name: <cluster>-thanos # Also add name to exclude from regular alerts.
	# - name: <cluster>-thanos # This gets excluded from regular object storage alerts

[anders-elastisys]

Same thing here:

Suggested change

	# - name: <cluster>-thanos # Also add name to exclude from regular alerts.
	# - name: <cluster>-thanos # This gets excluded from regular object storage alerts

[Mlundm]

Added some ops people if they have some opinions on the alerts 🙂